Re: [PATCH] x86: slightly simplify MB2/EFI "magic" check

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Thu, Aug 8, 2024 at 9:49 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> A few dozen lines down from here we repeatedly use a pattern involving
> just a single (conditional) branch. Do so also when checking for the
> boot loader magic value.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> I further question the placement of the clearing of vga_text_buffer,
> just out of context: Shouldn't that be placed with the increments of
> efi_platform and skip_realmode? Or else is the terminology in comments
> ("on EFI platforms") wrong in one of the two places? In the end, if we
> are entered at __efi64_mb2_start but the magic doesn't match, we simply
> don't know what environment we're in. There may or may not be a VGA
> console at the default address, so we may as well (try to) write to it
> (just like we do when entered at start).
>

I don't think __efi64_mb2_start should be ever get called by anything
else than multiboot2. Was EFI supported by multiboot version 1 maybe?
As far as I can see we will print an error and halt the machine so we
expect something really wrong to have happened.
We could indeed be in a position where we have VGA available. Or EFI?
Or not? As said something really weird and unexpected happened. Maybe
the safer way would be to print on serial and try to print on VGA in
this case. At the moment we mix the 2 prints (each character is
duplicated), printing all message to serial and then trying to print
all message to VGA and then halt could be an option.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -233,13 +233,11 @@ __efi64_mb2_start:
>
>          /* Check for Multiboot2 bootloader. */
>          cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> -        je      .Lefi_multiboot2_proto
>
>          /* Jump to .Lnot_multiboot after switching CPU to x86_32 mode. */
>          lea     .Lnot_multiboot(%rip), %r15
> -        jmp     x86_32_switch
> +        jne     x86_32_switch
>

It looks good to me. Maybe a bit less readable.

> -.Lefi_multiboot2_proto:
>          /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
>          xor     %esi,%esi
>          xor     %edi,%edi
>

Frediano