[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 2/3] multiboot2: parse console= and vga= options when setting GOP mode
On 31.03.2023 11:59, Roger Pau Monne wrote: > Only set the GOP mode if vga is selected in the console option, This particular aspect of the behavior is inconsistent with legacy boot behavior: There "vga=" isn't qualified by what "console=" has. > otherwise just fetch the information from the current mode in order to > make it available to dom0. > > Introduce support for passing the command line to the efi_multiboot2() > helper, and parse the console= and vga= options if present. > > Add support for the 'gfx' and 'current' vga options, ignore the 'keep' > option, and print a warning message about other options not being > currently implemented. > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> >[...] > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -786,7 +786,30 @@ static bool __init > efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > > static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) > { } > > -void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > *SystemTable) > +/* Return the next occurrence of opt in cmd. */ > +static const char __init *get_option(const char *cmd, const char *opt) > +{ > + const char *s = cmd, *o = NULL; > + > + if ( !cmd || !opt ) I can see why you need to check "cmd", but there's no need to check "opt" I would say. > @@ -807,7 +830,60 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, > EFI_SYSTEM_TABLE *SystemTable > > if ( gop ) > { > - gop_mode = efi_find_gop_mode(gop, 0, 0, 0); > + const char *opt = NULL, *last = cmdline; > + /* Default console selection is "com1,vga". */ > + bool vga = true; > + > + /* For the console option the last occurrence is the enforced one. */ > + while ( (last = get_option(last, "console=")) != NULL ) > + opt = last; > + > + if ( opt ) > + { > + const char *s = strstr(opt, "vga"); > + > + if ( !s || s > strpbrk(opt, " ") ) Why strpbrk() and not the simpler strchr()? Or did you mean to also look for tabs, but then didn't include \t here (and in get_option())? (Legacy boot code also takes \r and \n as separators, btw, but I'm unconvinced of the need.) Also aiui this is UB when the function returns NULL, as relational operators (excluding equality ones) may only be applied when both addresses refer to the same object (or to the end of an involved array). > + vga = false; > + } > + > + if ( vga ) > + { > + unsigned int width = 0, height = 0, depth = 0; > + bool keep_current = false; > + > + last = cmdline; > + while ( (last = get_option(last, "vga=")) != NULL ) It's yet different for "vga=", I'm afraid: Early boot code (boot/cmdline.c) finds the first instance only. Normal command line handling respects the last instance only. So while "vga=gfx-... vga=keep" will have the expected effect, "vga=keep vga=gfx-..." won't (I think). It is certainly fine to be less inflexible here, but I think this then wants accompanying by an update to the command line doc, no matter that presently it doesn't really describe these peculiarities. Otoh it would end up being slightly cheaper to only look for the first instance here as well. In particular ... > + { > + if ( !strncmp(last, "gfx-", 4) ) > + { > + width = simple_strtoul(last + 4, &last, 10); > + if ( *last == 'x' ) > + height = simple_strtoul(last + 1, &last, 10); > + if ( *last == 'x' ) > + depth = simple_strtoul(last + 1, &last, 10); > + /* Allow depth to be 0 or unset. */ > + if ( !width || !height ) > + width = height = depth = 0; > + keep_current = false; > + } > + else if ( !strncmp(last, "current", 7) ) > + keep_current = true; > + else if ( !strncmp(last, "keep", 4) ) > + { > + /* Ignore. */ > + } > + else > + { > + /* Fallback to defaults if unimplemented. */ > + width = height = depth = 0; > + keep_current = false; ... this zapping of what was successfully parsed before would then not be needed in any event (else I would question why this is necessary). Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |