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....@citrix.com>
> ---
> 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
