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

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

On Wed, May 31, 2023 at 11:15:44AM +0200, Jan Beulich wrote:
> On 30.05.2023 18:02, Roger Pau Monné wrote:
> > 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?
> 
> Because we also need to arrange for what Dom0 possibly wants to use.
> It has no way of setting the mode the low-level (BIOS or EFI) way.

I understand this might be needed when Xen is booted as an EFI
application, but otherwise it should be the bootloader that takes care
of setting such mode, as (most?) OSes are normally loaded with boot
services already exited.

I've removed the parsing of the console= option and unconditionally
parse vga= now.  We can always adjust later.

> >>> 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.
> 
> Hmm, I see. We don't do so elsewhere, though, I think.

If you insist I can remove it, otherwise I will just leave as-is.

> 
> >>> @@ -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).
> 
> I'm afraid I'm in trouble what "original code" you're referring to here.
> Iirc you really only add code to the function. And boot/cmdline.c has no
> use of strpbrk() afaics.

Original code in the patch, anyway this is now gone because I no
longer parse console=.

> >>> +                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.
> 
> Yes, fixing the non-EFI code is certainly an option (and then describing
> the hopefully consistent result in the doc).

OK, let me take a look, I expect that should be fairly trivial.

Thanks, Roger.