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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

Hi,

On Thu Aug 8, 2024 at 9:49 AM BST, Jan Beulich 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).

It's fair to assume we're in 64bits, and in that situation it's also fair to
assume the text console is long gone (pun intended). Seeing how this would be a
boot protocol bug, I think the most reasonable thing to do is to leave a poison
value in RAX and then hang (deadbeef..., badcafe..., take a pick). That would
point the poor sod debugging this to the right part of the code.

Though this is a largely theoretical issue that I can't see happening in
practice.

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

I don't think there's much benefit to this, but it would read more naturally if
lea was before cmp. Then cmp would be next to its (new) associated jne.

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

Cheers,
Alejandro