Re: [PATCH 4/5] multiboot2: parse console= option when setting GOP mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.11.2022 16:45, Roger Pau Monne wrote:
> Only set the GOP mode if vga is selected in the console option,
> 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= option if present.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> I'm unsure why the parsing of the multiboot2 tags is done in assembly,
> it could very well be done in efi_multiboot2() in C, but I don't want
> to switch that code now.

I guess that's mainly mirroring the non-EFI boot path, where the amount
of work needed to eventually enter C land is quite a bit larger?
Anything beyond that Daniel may want to point out.

> @@ -265,6 +266,15 @@ __efi64_mb2_start:
>          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
>          je      .Lrun_bs
>  
> +        /*
> +         * Get command line from Multiboot2 information.
> +         * Must be last parsed tag.

Why? And how do you guarantee this?

> +         */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_CMDLINE,MB2_tag_type(%rcx)
> +        jne     .Lefi_mb2_next_tag
> +        mov     %rcx,%rdx
> +        add     $(MB2_tag_string),%rdx

Simply "lea MB2_tag_string(%rcx),%rdx"?

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -786,7 +786,22 @@ 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 last occurrence of opt in cmd. */

Is this sufficient in the general case (it may be for "console=", but
perhaps not for "vga=", which may also need finding as per below)?

> +static const char __init *get_option(const char *cmd, const char *opt)

Nit: The first * wants to move earlier.

> +{
> +    const char *s = cmd, *o = NULL;
> +
> +    while ( (s = strstr(s, opt)) != NULL )

I'm afraid this is too easy to break without considering separators as
well. If I'm not mistaken you'd also match e.g. "sync_console=1" for
the sole present caller.

> +    {
> +        s += strlen(opt);
> +        o = s;
> +    }
> +
> +    return o;
> +}
> +
> +void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable,
> +                           const char *cmdline)
>  {
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
>      EFI_HANDLE gop_handle;
> @@ -807,7 +822,21 @@ 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 = get_option(cmdline, "console=");
> +        bool vga = false;
> +
> +        if ( opt )
> +        {
> +            const char *s = strstr(opt, "vga");
> +
> +            if ( s && s < strpbrk(opt, " \0"))
> +                vga = true;
> +        }

Don't you also want to find a "vga=gfx-..." option, to avoid ...

> +        if ( vga )
> +        {
> +            gop_mode = efi_find_gop_mode(gop, 0, 0, 0);

... requesting a "random" mode here?

> +        }

Nit: No need for the braces in cases like this one.

Jan