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

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

[Jan Beulich]

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@suse.com>
---
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).

--- 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
 
-.Lefi_multiboot2_proto:
         /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
         xor     %esi,%esi
         xor     %edi,%edi

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

[Frediano Ziglio]

On Thu, Aug 8, 2024 at 9:49 AM Jan Beulich <jbeulich@suse.com> 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@suse.com>
> ---
> 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

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

[Jan Beulich]

On 08.08.2024 11:36, Frediano Ziglio wrote:
> On Thu, Aug 8, 2024 at 9:49 AM Jan Beulich <jbeulich@suse.com> 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@suse.com>
>> ---
>> 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?

No, there was no EFI with MB1.

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

Sounds like you see something wrong with the "mixing" as you call it?
I'm suggesting exactly that: Try to output something to both possible
channels. Whether there's serial at the default port we don't know
either, after all.

What we kind of know is that we're in 64-bit mode (yet even that could
probably do with verifying, when we already have to assume something
is very broken). Even there I'd expect the VGA range to be mapped by
the identity page tables that our caller must have put in place.

Jan

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

[Alejandro Vallejo]

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

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

[Jan Beulich]

On 12.08.2024 16:34, Alejandro Vallejo wrote:
> 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@suse.com>
>> ---
>> 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).

How is being in 64-bit mode correlated with there being a VGA console?
(I question "fair to assume" here anyway. We're on a path where we know
something's wonky.)

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

You did look at the pattern though that I'm referring to in the description?
Knowing that generally paring the CMP/TEST with the Jcc, I would have
switched things around. Yet I wanted to make things as similar as possible,
in the hope that be(com)ing consistent would make it easiest to get such a
minor change in.

Jan

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

[Alejandro Vallejo]

Hi,

On Mon Aug 12, 2024 at 3:43 PM BST, Jan Beulich wrote:
> On 12.08.2024 16:34, Alejandro Vallejo wrote:
> > 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@suse.com>
> >> ---
> >> 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).
>
> How is being in 64-bit mode correlated with there being a VGA console?
> (I question "fair to assume" here anyway. We're on a path where we know
> something's wonky.)

The only way in which you could plausibly have a text-mode console in 64bits is
if you booted from BIOS and didn't set a VESA mode, so it boils down to what
failure modes you want to consider. For "anything goes" you're right that we
can't even be sure of being in 64bit (or 32bit) mode, but that's too draconian
an assumption to try to uphold, imo. I think that while details in the boot
protocol might be incorrect (like the magic tag), broad strokes (like being in
long mode and having a UEFI runtime) must still hold. Trying to use the serial
is fine (worst-case scenario it doesn't work), but trying to use a framebuffer
you're not sure about is not unlikely to triple fault your machine prematurely
and then debugging it is a pain even with an emulator.

>
> >> --- 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.
>
> You did look at the pattern though that I'm referring to in the description?
> Knowing that generally paring the CMP/TEST with the Jcc, I would have
> switched things around. Yet I wanted to make things as similar as possible,
> in the hope that be(com)ing consistent would make it easiest to get such a
> minor change in.
>
> Jan

I'm not sure about the pattern you mention. Seems like a standard set of
doX+check+cond_jump finished in an unconditional jump. All of them pretty
normal.

Regardless, I'm not arguing against this. While I happen to find it easier to
mentally parse it in its current form we do save a jump instruction after your
change. It's just that it'd be easier to follow with the mentioned reversal of
lea and cmp.

Cheers,
Alejandro

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

[Andrew Cooper]

On 08/08/2024 9:49 am, Jan Beulich wrote:
> --- 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
>  
> -.Lefi_multiboot2_proto:
>          /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
>          xor     %esi,%esi
>          xor     %edi,%edi


You've split the logical in two, and now the comment is in the wrong
position.

If you're going to make this change, it wants to end up reading:

    /* Jump to .Lnot_multiboot after switching CPU to x86_32 mode. */
    lea     .Lnot_multiboot(%rip), %r15

    /* Check for Multiboot2 bootloader. */
    cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
    jne     x86_32_switch


Not that it's really relevant, but this form also macrofuses nicely.

~Andrew

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

[Jan Beulich]

On 12.08.2024 23:40, Andrew Cooper wrote:
> On 08/08/2024 9:49 am, Jan Beulich wrote:
>> --- 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
>>  
>> -.Lefi_multiboot2_proto:
>>          /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
>>          xor     %esi,%esi
>>          xor     %edi,%edi
> 
> 
> You've split the logical in two, and now the comment is in the wrong
> position.
> 
> If you're going to make this change, it wants to end up reading:
> 
>     /* Jump to .Lnot_multiboot after switching CPU to x86_32 mode. */
>     lea     .Lnot_multiboot(%rip), %r15
> 
>     /* Check for Multiboot2 bootloader. */
>     cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
>     jne     x86_32_switch
> 
> 
> Not that it's really relevant, but this form also macrofuses nicely.

All true, yet I'm sure you also read my response to Alejandro: Then we
ought to also change the other similar instances. The goal of this simple
change, after all, is to get all similar pieces of code into consistent
shape.

Jan