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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.06.2023 15:05, Roger Pau Monne wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -226,9 +226,10 @@ __efi64_mb2_start:
>          jmp     x86_32_switch
>  
>  .Lefi_multiboot2_proto:
> -        /* Zero EFI SystemTable and EFI ImageHandle addresses. */
> +        /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
>          xor     %esi,%esi
>          xor     %edi,%edi
> +        xor     %edx,%edx

While perhaps better to leave this as you have it, ...

> @@ -266,6 +267,13 @@ __efi64_mb2_start:
>          cmove   MB2_efi64_ih(%rcx),%rdi
>          je      .Lefi_mb2_next_tag
>  
> +        /* 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:

... in new blocks of code I think it would be nice if commas were
followed by visually separating blanks.

> @@ -329,7 +337,8 @@ __efi64_mb2_start:
>  
>          /*
>           * efi_multiboot2() is called according to System V AMD64 ABI:
> -         *   - IN:  %rdi - EFI ImageHandle, %rsi - EFI SystemTable.
> +         *   - IN:  %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
> +         *          %rdx - MB2 cmdline
>           */
>          call    efi_multiboot2

All you obtain is a pointer to the string, which is fine from what I
was able to establish, but that's not entirely obvious: The MBI
structure used has a size field, so it could have been that the
string isn't nul-terminated, and hence the size would also need
passing. May I ask that this be mentioned at least in the description?

> --- 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 first 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 )
> +        return NULL;
> +
> +    while ( (s = strstr(s, opt)) != NULL )
> +    {
> +        if ( s == cmd || *(s - 1) == ' ' )

Iirc I had asked before: Not allowing for at least tab? (See
cmdline.c:delim_chars_comma[] for what the non-EFI parsing permits,
which in turn might be going a little too far especially with
permitting comma as well.)

> +        {
> +            o = s + strlen(opt);

I don't think the comment ahead of the function describes this
behavior, i.e. in particular the adding of the length of the
option.

> @@ -807,7 +830,41 @@ 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 *last = cmdline;

Nit: Maybe better "cur" than "last"?

> +        unsigned int width = 0, height = 0, depth = 0;
> +        bool keep_current = false;
> +
> +        while ( (last = get_option(last, "vga=")) != NULL )
> +        {
> +            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);
> +                if ( *last != ' ' && *last != '\t' && *last != '\0' &&
> +                     *last != ',' )
> +                    width = height = depth = 0;
> +                keep_current = false;
> +            }
> +            else if ( !strncmp(last, "current", 7) )
> +                keep_current = true;
> +            else if ( !strncmp(last, "keep", 4) )
> +            {
> +                /* Ignore. */

Maybe "Ignore here, handled in ..."?

Jan