Re: [PATCH v4 1/3] multiboot2: parse vga= option when setting GOP mode

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

On Thu, Jul 06, 2023 at 12:41:58PM +0200, Jan Beulich wrote:
> On 05.07.2023 13:47, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -795,7 +795,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 a pointer to the character after the first occurrence of opt in 
> > cmd */
> > +static const char __init *get_option(const char *cmd, const char *opt)
> 
> Nit: __init and * want to change places.

Hm, yes.  I assume that placing it before the return type is not OK?
(static const __init char ...)

> > @@ -816,7 +839,54 @@ 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 *cur = cmdline;
> > +        unsigned int width = 0, height = 0, depth = 0;
> > +        bool keep_current = false;
> > +
> > +        while ( (cur = get_option(cur, "vga=")) != NULL )
> > +        {
> > +#define VALID_TERMINATOR(c) \
> > +    (*(c) == ' ' || *(c) == '\t' || *(c) == '\0' || *(c) == ',')
> > +            if ( !strncmp(cur, "gfx-", 4) )
> > +            {
> > +                width = simple_strtoul(cur + 4, &cur, 10);
> > +
> > +                if ( *cur == 'x' )
> > +                    height = simple_strtoul(cur + 1, &cur, 10);
> > +                else
> > +                    goto error;
> > +
> > +                if ( *cur == 'x' )
> > +                    depth = simple_strtoul(cur + 1, &cur, 10);
> > +                else
> > +                    goto error;
> > +
> > +                if ( !VALID_TERMINATOR(cur) )
> > +                {
> > +error:
> 
> Nit: Labels want to be indented by at least one blank. Here I'm
> inclined to suggest indenting to the level of the enclosing curly
> braces.
> 
> > +                    PrintStr(L"Warning: Invalid gfx- option 
> > detected.\r\n");
> 
> Maybe better PrintErr() and no trailing full stop?

Yes, please adjust if you don't mind.

> 
> > +                    width = height = depth = 0;
> > +                }
> > +                keep_current = false;
> > +            }
> > +            else if ( !strncmp(cur, "current", 7) && VALID_TERMINATOR(cur 
> > + 7) )
> > +                keep_current = true;
> > +            else if ( !strncmp(cur, "keep", 4) && VALID_TERMINATOR(cur + 
> > 4) )
> > +            {
> > +                /* Ignore, handled in later vga= parsing. */
> > +            }
> > +            else
> > +            {
> > +                /* Fallback to defaults if unimplemented. */
> > +                width = height = depth = 0;
> > +                keep_current = false;
> > +                PrintStr(L"Warning: Cannot use selected vga option.\r\n");
> 
> Same here then?
> 
> With these addressed (which are all mechanical and hence can probably
> be done while committing, as long as we can reach agreement)
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

LGTM, please adjust if you don't mind, otherwise I can send an
adjusted version.

Thanks, Roger.