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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -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?

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

Jan