Re: [PATCH v2 2/3] multiboot2: parse console= and vga= options when setting GOP mode

[Jan Beulich <jbeulich@xxxxxxxx>]

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