Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Dec 05, 2022 at 04:10:28PM +0100, Jan Beulich wrote:
> On 23.11.2022 16:45, Roger Pau Monne wrote:
> > @@ -265,6 +266,15 @@ __efi64_mb2_start:
> >          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
> >          je      .Lrun_bs
> >  
> > +        /*
> > +         * Get command line from Multiboot2 information.
> > +         * Must be last parsed tag.
> 
> Why? And how do you guarantee this?

I think the comment is misleading, must be the last checked for tag in
the loop that does this in assembly, because it's not using cmove.
I've adjusted to:

        /* Get command line from Multiboot2 information. */
        cmpl    $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx)
        jne     .Lno_cmdline
        lea     MB2_tag_string(%rcx),%rdx
        jmp     .Lefi_mb2_next_tag
.Lno_cmdline:

Maybe there's some instruction I'm missing similar to a conditional
lea?

> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -786,7 +786,22 @@ 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 last occurrence of opt in cmd. */
> 
> Is this sufficient in the general case (it may be for "console=", but
> perhaps not for "vga=", which may also need finding as per below)?

I guess for vga= it's possible to have something like:

vga=current vga=keep

And in that case we do care about the non-last option.

> > +static const char __init *get_option(const char *cmd, const char *opt)
> 
> Nit: The first * wants to move earlier.
> 
> > +{
> > +    const char *s = cmd, *o = NULL;
> > +
> > +    while ( (s = strstr(s, opt)) != NULL )
> 
> I'm afraid this is too easy to break without considering separators as
> well. If I'm not mistaken you'd also match e.g. "sync_console=1" for
> the sole present caller.

Right, wasn't taking into account that sync_console can also be a
boolean (and thus assigned).

> > +        }
> 
> Nit: No need for the braces in cases like this one.

I've added the braces here because it will be expended in the next
patch.  Since you asked for both patches to be merged this will go
away.

Thanks, Roger.