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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Apr 05, 2023 at 12:15:26PM +0200, Jan Beulich wrote:
> 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.

Hm, I find this very odd, why would we fiddle with the VGA (or the GOP
here) if we don't intend to use it for output?

> > 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.

Given this is executed without a page-fault handler in place I thought
it was best to be safe rather than sorry, and avoid any pointer
dereferences.

> > @@ -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.)

I was originally checking for more characters here and didn't switch
when removing those.  I will add \t.

> 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).

Hm, I see, thanks for spotting. So I would need to do:

s > (strpbrk(opt, " ") ?: s)

So that we don't compare against NULL.

Also the original code was wrong AFAICT, as strpbrk() returning NULL
should result in vga=true (as it would imply console= is the last
option on the command line).

> > +                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.

But if we then describe this behavior in the documentation people
could rely on it.  Right now this is just an implementation detail (or
a bug I would say), and that would justify fixing boot/cmdline.c to
also respect the last instance only.

> 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).

Hm, I don't have a strong opinion, the expected behavior I have of
command line options is that the last one is the one that takes
effect, but it would simplify the change if we only cared about the
first one, albeit that's an odd behavior.

My preference would be to leave the code here respecting the last
instance only, and attempt to fix boot/cmdline.c so it does the same.

Thanks, Roger.