[PATCH 0/2] x86: init improvements

[PATCH 0/2] x86: init improvements

[Roger Pau Monne]

Hello,

The following series contain two minor improvements for early boot: the
first one is an alignment check when building the initial page tables,
the second is a consistency fix for the GDT used by the BSP for the
trampoline code.

Both are a result of some debugging work done on a system with broken
firmware that resulted in Xen text not being loaded at a 2Mb aligned
address.  This resulted in corrupted page tables that would manifest as
the ljmp from compatibility mode in trampoline_protmode_entry causing a
triple fault due to the GDT being located in the Xen text section, and
the page table entry for that address being corrupt because Xen was not
loaded a 2Mb boundary.

The aim of the series (specially the first patch) is not to allow
booting on such broken firmware, but to print an error message instead
of causing a triple fault.

Thanks, Roger.

Roger Pau Monne (2):
  x86/head: check base address alignment
  x86/trampoline: load the GDT located in the trampoline page

 xen/arch/x86/boot/head.S       | 9 +++++++++
 xen/arch/x86/boot/trampoline.S | 6 ++++++
 2 files changed, 15 insertions(+)

-- 
2.40.0

[PATCH 1/2] x86/head: check base address alignment

[Roger Pau Monne]

Ensure that the base address is 2M aligned, or else the page table
entries created would be corrupt as reserved bits on the PDE end up
set.

We have found a broken firmware where the loader would end up loading
Xen at a non 2M aligned region, and that caused a very difficult to
debug triple fault.

If the alignment is not as required by the page tables print an error
message and stop the boot.

The check could be performed earlier, but so far the alignment is
required by the page tables, and hence feels more natural that the
check lives near to the piece of code that requires it.

Note that when booted as an EFI application from the PE entry point
the alignment check is already performed by
efi_arch_load_addr_check(), and hence there's no need to add another
check at the point where page tables get built in
efi_arch_memory_setup().

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/head.S | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 0fb7dd3029f2..ff73c1d274c4 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -121,6 +121,7 @@ multiboot2_header:
 .Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
 .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
 .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
+.Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
 
         .section .init.data, "aw", @progbits
         .align 4
@@ -146,6 +147,9 @@ bad_cpu:
 not_multiboot:
         add     $sym_offs(.Lbad_ldr_msg),%esi   # Error message
         jmp     .Lget_vtb
+not_aligned:
+        add     $sym_offs(.Lbag_alg_msg),%esi   # Error message
+        jmp     .Lget_vtb
 .Lmb2_no_st:
         /*
          * Here we are on EFI platform. vga_text_buffer was zapped earlier
@@ -670,6 +674,11 @@ trampoline_setup:
         cmp     %edi, %eax
         jb      1b
 
+        /* Check that the image base is aligned. */
+        lea     sym_esi(_start), %eax
+        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
+        jnz     not_aligned
+
         /* Map Xen into the higher mappings using 2M superpages. */
         lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax
         mov     $sym_offs(_start),   %ecx   /* %eax = PTE to write ^      */
-- 
2.40.0

[PATCH 2/2] x86/trampoline: load the GDT located in the trampoline page

[Roger Pau Monne]

When booting the BSP the portion of the code executed from the
trampoline page will be using the GDT located in the hypervisor
.text.head section rather than the GDT located in the trampoline page.

If skip_realmode is not set the GDT located in the trampoline page
will be loaded after having executed the BIOS call, otherwise the GDT
from .text.head will be used for all the protected mode trampoline
code execution.

Note that both gdt_boot_descr and gdt_48 contain the same entries, but
the former is located inside the hypervisor .text section, while the
later lives in the relocated trampoline page.

This is not harmful as-is, as both GDTs contain the same entries, but
for consistency with the APs switch the BSP trampoline code to also
use the GDT on the trampoline page.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/trampoline.S | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index cdecf949b410..e4b4b9091d0c 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -164,6 +164,12 @@ GLOBAL(trampoline_cpu_started)
 
         .code32
 trampoline_boot_cpu_entry:
+        /*
+         * Load the GDT from the relocated trampoline page rather than the
+         * hypervisor .text section.
+         */
+        lgdt    bootsym_rel(gdt_48, 4)
+
         cmpb    $0,bootsym_rel(skip_realmode,5)
         jnz     .Lskip_realmode
 
-- 
2.40.0

Re: [PATCH 2/2] x86/trampoline: load the GDT located in the trampoline page

[Andrew Cooper]

On 02/05/2023 10:22 am, Roger Pau Monne wrote:
> When booting the BSP the portion of the code executed from the
> trampoline page will be using the GDT located in the hypervisor
> .text.head section rather than the GDT located in the trampoline page.

It's more subtle than this.

gdt_boot_descr references the trampoline GDT, but by it's position in
the main Xen image.

>
> If skip_realmode is not set the GDT located in the trampoline page
> will be loaded after having executed the BIOS call, otherwise the GDT
> from .text.head will be used for all the protected mode trampoline
> code execution.
>
> Note that both gdt_boot_descr and gdt_48 contain the same entries, but
> the former is located inside the hypervisor .text section, while the
> later lives in the relocated trampoline page.
>
> This is not harmful as-is, as both GDTs contain the same entries, but
> for consistency with the APs switch the BSP trampoline code to also
> use the GDT on the trampoline page.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although ...

> ---
>  xen/arch/x86/boot/trampoline.S | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
> index cdecf949b410..e4b4b9091d0c 100644
> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -164,6 +164,12 @@ GLOBAL(trampoline_cpu_started)
>  
>          .code32
>  trampoline_boot_cpu_entry:
> +        /*
> +         * Load the GDT from the relocated trampoline page rather than the
> +         * hypervisor .text section.
> +         */
> +        lgdt    bootsym_rel(gdt_48, 4)

... I'd suggest rewording this to simply /* Switch to trampoline GDT */,
or perhaps with an "alias" in there somewhere.

The important point here is that we want to shed all pre-trampoline
state, and unexpectedly being on the wrong GDT alias certainly
complicated debugging this...


> +
>          cmpb    $0,bootsym_rel(skip_realmode,5)
>          jnz     .Lskip_realmode
>  

Re: [PATCH 1/2] x86/head: check base address alignment

[Andrew Cooper]

On 02/05/2023 10:22 am, Roger Pau Monne wrote:
> Ensure that the base address is 2M aligned, or else the page table
> entries created would be corrupt as reserved bits on the PDE end up
> set.
>
> We have found a broken firmware where the loader would end up loading
> Xen at a non 2M aligned region, and that caused a very difficult to
> debug triple fault.

It's probably worth saying that in this case, the OEM has fixed their
firmware.

>
> If the alignment is not as required by the page tables print an error
> message and stop the boot.
>
> The check could be performed earlier, but so far the alignment is
> required by the page tables, and hence feels more natural that the
> check lives near to the piece of code that requires it.
>
> Note that when booted as an EFI application from the PE entry point
> the alignment check is already performed by
> efi_arch_load_addr_check(), and hence there's no need to add another
> check at the point where page tables get built in
> efi_arch_memory_setup().
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/boot/head.S | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 0fb7dd3029f2..ff73c1d274c4 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -121,6 +121,7 @@ multiboot2_header:
>  .Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
>  .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
>  .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
> +.Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
>  
>          .section .init.data, "aw", @progbits
>          .align 4
> @@ -146,6 +147,9 @@ bad_cpu:
>  not_multiboot:
>          add     $sym_offs(.Lbad_ldr_msg),%esi   # Error message
>          jmp     .Lget_vtb
> +not_aligned:
> +        add     $sym_offs(.Lbag_alg_msg),%esi   # Error message
> +        jmp     .Lget_vtb
>  .Lmb2_no_st:
>          /*
>           * Here we are on EFI platform. vga_text_buffer was zapped earlier
> @@ -670,6 +674,11 @@ trampoline_setup:
>          cmp     %edi, %eax
>          jb      1b
>  
> +        /* Check that the image base is aligned. */
> +        lea     sym_esi(_start), %eax
> +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
> +        jnz     not_aligned

You just want to check the value in %esi, which is the base of the Xen
image.  Something like:

mov %esi, %eax
and ...
jnz

No need to reference the _start label, or use sym_esi().

Otherwise, LGTM.

~Andrew

Re: [PATCH 1/2] x86/head: check base address alignment

[Jan Beulich]

On 02.05.2023 11:54, Andrew Cooper wrote:
> On 02/05/2023 10:22 am, Roger Pau Monne wrote:
>> Ensure that the base address is 2M aligned, or else the page table
>> entries created would be corrupt as reserved bits on the PDE end up
>> set.
>>
>> We have found a broken firmware where the loader would end up loading
>> Xen at a non 2M aligned region, and that caused a very difficult to
>> debug triple fault.
> 
> It's probably worth saying that in this case, the OEM has fixed their
> firmware.

I'm curious: What firmware loads Xen directly? I thought there was
always a boot loader involved (except for xen.efi of course).

I'm further a little puzzled by this talking about alignment and not
xen.efi: xen.gz only specifies alignment for MB2 afaik. For MB1 all
it does specify is the physical address (2Mb) that it wants to be
loaded at. So maybe MB2 wants mentioning here as well, for clarity?

>> @@ -670,6 +674,11 @@ trampoline_setup:
>>          cmp     %edi, %eax
>>          jb      1b
>>  
>> +        /* Check that the image base is aligned. */
>> +        lea     sym_esi(_start), %eax
>> +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
>> +        jnz     not_aligned
> 
> You just want to check the value in %esi, which is the base of the Xen
> image.  Something like:
> 
> mov %esi, %eax
> and ...
> jnz

Or yet more simply "test $..., %esi" and then "jnz ..."?

Jan

Re: [PATCH 1/2] x86/head: check base address alignment

[Roger Pau Monné]

On Tue, May 02, 2023 at 10:54:55AM +0100, Andrew Cooper wrote:
> On 02/05/2023 10:22 am, Roger Pau Monne wrote:
> > Ensure that the base address is 2M aligned, or else the page table
> > entries created would be corrupt as reserved bits on the PDE end up
> > set.
> >
> > We have found a broken firmware where the loader would end up loading
> > Xen at a non 2M aligned region, and that caused a very difficult to
> > debug triple fault.
> 
> It's probably worth saying that in this case, the OEM has fixed their
> firmware.
> 
> >
> > If the alignment is not as required by the page tables print an error
> > message and stop the boot.
> >
> > The check could be performed earlier, but so far the alignment is
> > required by the page tables, and hence feels more natural that the
> > check lives near to the piece of code that requires it.
> >
> > Note that when booted as an EFI application from the PE entry point
> > the alignment check is already performed by
> > efi_arch_load_addr_check(), and hence there's no need to add another
> > check at the point where page tables get built in
> > efi_arch_memory_setup().
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/boot/head.S | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 0fb7dd3029f2..ff73c1d274c4 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -121,6 +121,7 @@ multiboot2_header:
> >  .Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
> >  .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
> >  .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
> > +.Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
> >  
> >          .section .init.data, "aw", @progbits
> >          .align 4
> > @@ -146,6 +147,9 @@ bad_cpu:
> >  not_multiboot:
> >          add     $sym_offs(.Lbad_ldr_msg),%esi   # Error message
> >          jmp     .Lget_vtb
> > +not_aligned:
> > +        add     $sym_offs(.Lbag_alg_msg),%esi   # Error message
> > +        jmp     .Lget_vtb
> >  .Lmb2_no_st:
> >          /*
> >           * Here we are on EFI platform. vga_text_buffer was zapped earlier
> > @@ -670,6 +674,11 @@ trampoline_setup:
> >          cmp     %edi, %eax
> >          jb      1b
> >  
> > +        /* Check that the image base is aligned. */
> > +        lea     sym_esi(_start), %eax
> > +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
> > +        jnz     not_aligned
> 
> You just want to check the value in %esi, which is the base of the Xen
> image.  Something like:
> 
> mov %esi, %eax
> and ...
> jnz
> 
> No need to reference the _start label, or use sym_esi().

The reason for using sym_esi(_start) is because that's exactly the
address used when building the PDE, so it's clearer to keep those in
sync IMO.

That's also the reason for doing the check here rather than earlier,
so it's closer to the point where the value is used and not being
aligned would lead to corrupt entries.

Thanks, Roger.

Re: [PATCH 2/2] x86/trampoline: load the GDT located in the trampoline page

[Roger Pau Monné]

On Tue, May 02, 2023 at 10:43:13AM +0100, Andrew Cooper wrote:
> On 02/05/2023 10:22 am, Roger Pau Monne wrote:
> > When booting the BSP the portion of the code executed from the
> > trampoline page will be using the GDT located in the hypervisor
> > .text.head section rather than the GDT located in the trampoline page.
> 
> It's more subtle than this.
> 
> gdt_boot_descr references the trampoline GDT, but by it's position in
> the main Xen image.

Right, gdt_boot_descr GDTR references gdt_48, but the instance on the
Xen .text section, not the trampoline.

I've tried to explain this in the commit message, but maybe I've
failed to do so.

> >
> > If skip_realmode is not set the GDT located in the trampoline page
> > will be loaded after having executed the BIOS call, otherwise the GDT
> > from .text.head will be used for all the protected mode trampoline
> > code execution.
> >
> > Note that both gdt_boot_descr and gdt_48 contain the same entries, but
> > the former is located inside the hypervisor .text section, while the
> > later lives in the relocated trampoline page.
> >
> > This is not harmful as-is, as both GDTs contain the same entries, but
> > for consistency with the APs switch the BSP trampoline code to also
> > use the GDT on the trampoline page.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although ...
> 
> > ---
> >  xen/arch/x86/boot/trampoline.S | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
> > index cdecf949b410..e4b4b9091d0c 100644
> > --- a/xen/arch/x86/boot/trampoline.S
> > +++ b/xen/arch/x86/boot/trampoline.S
> > @@ -164,6 +164,12 @@ GLOBAL(trampoline_cpu_started)
> >  
> >          .code32
> >  trampoline_boot_cpu_entry:
> > +        /*
> > +         * Load the GDT from the relocated trampoline page rather than the
> > +         * hypervisor .text section.
> > +         */
> > +        lgdt    bootsym_rel(gdt_48, 4)
> 
> ... I'd suggest rewording this to simply /* Switch to trampoline GDT */,
> or perhaps with an "alias" in there somewhere.

"Switch to the relocated trampoline GDT." maybe?

Thanks, Roger.

Re: [PATCH 1/2] x86/head: check base address alignment

[Jan Beulich]

On 02.05.2023 12:28, Roger Pau Monné wrote:
> On Tue, May 02, 2023 at 10:54:55AM +0100, Andrew Cooper wrote:
>> On 02/05/2023 10:22 am, Roger Pau Monne wrote:
>>> Ensure that the base address is 2M aligned, or else the page table
>>> entries created would be corrupt as reserved bits on the PDE end up
>>> set.
>>>
>>> We have found a broken firmware where the loader would end up loading
>>> Xen at a non 2M aligned region, and that caused a very difficult to
>>> debug triple fault.
>>
>> It's probably worth saying that in this case, the OEM has fixed their
>> firmware.
>>
>>>
>>> If the alignment is not as required by the page tables print an error
>>> message and stop the boot.
>>>
>>> The check could be performed earlier, but so far the alignment is
>>> required by the page tables, and hence feels more natural that the
>>> check lives near to the piece of code that requires it.
>>>
>>> Note that when booted as an EFI application from the PE entry point
>>> the alignment check is already performed by
>>> efi_arch_load_addr_check(), and hence there's no need to add another
>>> check at the point where page tables get built in
>>> efi_arch_memory_setup().
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>>  xen/arch/x86/boot/head.S | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
>>> index 0fb7dd3029f2..ff73c1d274c4 100644
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -121,6 +121,7 @@ multiboot2_header:
>>>  .Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
>>>  .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
>>>  .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
>>> +.Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
>>>  
>>>          .section .init.data, "aw", @progbits
>>>          .align 4
>>> @@ -146,6 +147,9 @@ bad_cpu:
>>>  not_multiboot:
>>>          add     $sym_offs(.Lbad_ldr_msg),%esi   # Error message
>>>          jmp     .Lget_vtb
>>> +not_aligned:
>>> +        add     $sym_offs(.Lbag_alg_msg),%esi   # Error message
>>> +        jmp     .Lget_vtb
>>>  .Lmb2_no_st:
>>>          /*
>>>           * Here we are on EFI platform. vga_text_buffer was zapped earlier
>>> @@ -670,6 +674,11 @@ trampoline_setup:
>>>          cmp     %edi, %eax
>>>          jb      1b
>>>  
>>> +        /* Check that the image base is aligned. */
>>> +        lea     sym_esi(_start), %eax
>>> +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
>>> +        jnz     not_aligned
>>
>> You just want to check the value in %esi, which is the base of the Xen
>> image.  Something like:
>>
>> mov %esi, %eax
>> and ...
>> jnz
>>
>> No need to reference the _start label, or use sym_esi().
> 
> The reason for using sym_esi(_start) is because that's exactly the
> address used when building the PDE, so it's clearer to keep those in
> sync IMO.

Hmm, while I see your point using sym_esi() here merely means
subtracting __XEN_VIRT_START. Which would better remain 2Mb- (and
even 1Gb-)aligned (and if you wanted to guard for that, you could
add a build-time check instead of a runtime one, e.g. for sym_esi(0)
to be suitably aligned).

Jan

> That's also the reason for doing the check here rather than earlier,
> so it's closer to the point where the value is used and not being
> aligned would lead to corrupt entries.
> 
> Thanks, Roger.

Re: [PATCH 1/2] x86/head: check base address alignment

[Andrew Cooper]

On 02/05/2023 11:28 am, Roger Pau Monné wrote:
> On Tue, May 02, 2023 at 10:54:55AM +0100, Andrew Cooper wrote:
>> On 02/05/2023 10:22 am, Roger Pau Monne wrote:
>>> Ensure that the base address is 2M aligned, or else the page table
>>> entries created would be corrupt as reserved bits on the PDE end up
>>> set.
>>>
>>> We have found a broken firmware where the loader would end up loading
>>> Xen at a non 2M aligned region, and that caused a very difficult to
>>> debug triple fault.
>> It's probably worth saying that in this case, the OEM has fixed their
>> firmware.
>>
>>> If the alignment is not as required by the page tables print an error
>>> message and stop the boot.
>>>
>>> The check could be performed earlier, but so far the alignment is
>>> required by the page tables, and hence feels more natural that the
>>> check lives near to the piece of code that requires it.
>>>
>>> Note that when booted as an EFI application from the PE entry point
>>> the alignment check is already performed by
>>> efi_arch_load_addr_check(), and hence there's no need to add another
>>> check at the point where page tables get built in
>>> efi_arch_memory_setup().
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>>  xen/arch/x86/boot/head.S | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
>>> index 0fb7dd3029f2..ff73c1d274c4 100644
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -121,6 +121,7 @@ multiboot2_header:
>>>  .Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
>>>  .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
>>>  .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
>>> +.Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
>>>  
>>>          .section .init.data, "aw", @progbits
>>>          .align 4
>>> @@ -146,6 +147,9 @@ bad_cpu:
>>>  not_multiboot:
>>>          add     $sym_offs(.Lbad_ldr_msg),%esi   # Error message
>>>          jmp     .Lget_vtb
>>> +not_aligned:
>>> +        add     $sym_offs(.Lbag_alg_msg),%esi   # Error message
>>> +        jmp     .Lget_vtb
>>>  .Lmb2_no_st:
>>>          /*
>>>           * Here we are on EFI platform. vga_text_buffer was zapped earlier
>>> @@ -670,6 +674,11 @@ trampoline_setup:
>>>          cmp     %edi, %eax
>>>          jb      1b
>>>  
>>> +        /* Check that the image base is aligned. */
>>> +        lea     sym_esi(_start), %eax
>>> +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
>>> +        jnz     not_aligned
>> You just want to check the value in %esi, which is the base of the Xen
>> image.  Something like:
>>
>> mov %esi, %eax
>> and ...
>> jnz
>>
>> No need to reference the _start label, or use sym_esi().
> The reason for using sym_esi(_start) is because that's exactly the
> address used when building the PDE, so it's clearer to keep those in
> sync IMO.

Hmm yeah, fair point.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, preferably with
the extra note in the commit message.

~Andrew

Re: [PATCH 1/2] x86/head: check base address alignment

[Roger Pau Monné]

On Tue, May 02, 2023 at 12:28:55PM +0200, Jan Beulich wrote:
> On 02.05.2023 11:54, Andrew Cooper wrote:
> > On 02/05/2023 10:22 am, Roger Pau Monne wrote:
> >> Ensure that the base address is 2M aligned, or else the page table
> >> entries created would be corrupt as reserved bits on the PDE end up
> >> set.
> >>
> >> We have found a broken firmware where the loader would end up loading
> >> Xen at a non 2M aligned region, and that caused a very difficult to
> >> debug triple fault.
> > 
> > It's probably worth saying that in this case, the OEM has fixed their
> > firmware.
> 
> I'm curious: What firmware loads Xen directly? I thought there was
> always a boot loader involved (except for xen.efi of course).

This was a result of a bug in firmware plus a bug in grub, there's
also one pending change for grub, see:

https://lists.gnu.org/archive/html/grub-devel/2023-04/msg00157.html

The firmware would return error for some calls to Boot Services
allocate_pages method, and that triggered a bug in grub that resulted
in the memory allocated for Xen not being aligned as requested.

> I'm further a little puzzled by this talking about alignment and not
> xen.efi: xen.gz only specifies alignment for MB2 afaik. For MB1 all
> it does specify is the physical address (2Mb) that it wants to be
> loaded at. So maybe MB2 wants mentioning here as well, for clarity?

"We have found a broken firmware where grub2 would end up loading Xen
at a non 2M aligned region when using the multiboot2 protocol, and
that caused a very difficult to debug triple fault."

Would that be better?

> >> @@ -670,6 +674,11 @@ trampoline_setup:
> >>          cmp     %edi, %eax
> >>          jb      1b
> >>  
> >> +        /* Check that the image base is aligned. */
> >> +        lea     sym_esi(_start), %eax
> >> +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
> >> +        jnz     not_aligned
> > 
> > You just want to check the value in %esi, which is the base of the Xen
> > image.  Something like:
> > 
> > mov %esi, %eax
> > and ...
> > jnz
> 
> Or yet more simply "test $..., %esi" and then "jnz ..."?

As replied to Andrew, I would rather keep this inline with the address
used to build the PDE, which is sym_esi(_start).

Thanks, Roger.

Re: [PATCH 1/2] x86/head: check base address alignment

[Jan Beulich]

On 02.05.2023 12:51, Roger Pau Monné wrote:
> On Tue, May 02, 2023 at 12:28:55PM +0200, Jan Beulich wrote:
>> On 02.05.2023 11:54, Andrew Cooper wrote:
>>> On 02/05/2023 10:22 am, Roger Pau Monne wrote:
>>>> Ensure that the base address is 2M aligned, or else the page table
>>>> entries created would be corrupt as reserved bits on the PDE end up
>>>> set.
>>>>
>>>> We have found a broken firmware where the loader would end up loading
>>>> Xen at a non 2M aligned region, and that caused a very difficult to
>>>> debug triple fault.
>>>
>>> It's probably worth saying that in this case, the OEM has fixed their
>>> firmware.
>>
>> I'm curious: What firmware loads Xen directly? I thought there was
>> always a boot loader involved (except for xen.efi of course).
> 
> This was a result of a bug in firmware plus a bug in grub, there's
> also one pending change for grub, see:
> 
> https://lists.gnu.org/archive/html/grub-devel/2023-04/msg00157.html
> 
> The firmware would return error for some calls to Boot Services
> allocate_pages method, and that triggered a bug in grub that resulted
> in the memory allocated for Xen not being aligned as requested.
> 
>> I'm further a little puzzled by this talking about alignment and not
>> xen.efi: xen.gz only specifies alignment for MB2 afaik. For MB1 all
>> it does specify is the physical address (2Mb) that it wants to be
>> loaded at. So maybe MB2 wants mentioning here as well, for clarity?
> 
> "We have found a broken firmware where grub2 would end up loading Xen
> at a non 2M aligned region when using the multiboot2 protocol, and
> that caused a very difficult to debug triple fault."
> 
> Would that be better?

Yes indeed, thanks.

>>>> @@ -670,6 +674,11 @@ trampoline_setup:
>>>>          cmp     %edi, %eax
>>>>          jb      1b
>>>>  
>>>> +        /* Check that the image base is aligned. */
>>>> +        lea     sym_esi(_start), %eax
>>>> +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
>>>> +        jnz     not_aligned
>>>
>>> You just want to check the value in %esi, which is the base of the Xen
>>> image.  Something like:
>>>
>>> mov %esi, %eax
>>> and ...
>>> jnz
>>
>> Or yet more simply "test $..., %esi" and then "jnz ..."?
> 
> As replied to Andrew, I would rather keep this inline with the address
> used to build the PDE, which is sym_esi(_start).

Well, I won't insist, and you've got Andrew's R-b already. (What I would
appreciate though as a minimal change is to switch from AND to TEST. We
really should avoid using AND or SUB when in fact we only care about the
flags output, and hence TEST or CMP can be used. It doesn't matter much
on one-time paths like the one here, but each unnecessary use sets a bad
precedent.)

Jan

Re: [PATCH 1/2] x86/head: check base address alignment

[Jan Beulich]

On 02.05.2023 13:05, Jan Beulich wrote:
> On 02.05.2023 12:51, Roger Pau Monné wrote:
>> On Tue, May 02, 2023 at 12:28:55PM +0200, Jan Beulich wrote:
>>> On 02.05.2023 11:54, Andrew Cooper wrote:
>>>> On 02/05/2023 10:22 am, Roger Pau Monne wrote:
>>>>> @@ -670,6 +674,11 @@ trampoline_setup:
>>>>>          cmp     %edi, %eax
>>>>>          jb      1b
>>>>>  
>>>>> +        /* Check that the image base is aligned. */
>>>>> +        lea     sym_esi(_start), %eax
>>>>> +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
>>>>> +        jnz     not_aligned
>>>>
>>>> You just want to check the value in %esi, which is the base of the Xen
>>>> image.  Something like:
>>>>
>>>> mov %esi, %eax
>>>> and ...
>>>> jnz
>>>
>>> Or yet more simply "test $..., %esi" and then "jnz ..."?
>>
>> As replied to Andrew, I would rather keep this inline with the address
>> used to build the PDE, which is sym_esi(_start).
> 
> Well, I won't insist, and you've got Andrew's R-b already.

Actually, one more remark here: While using sym_esi() is more in line
with the actual consumer of the data, the check triggering because of
the transformation yielding a misaligned value (in turn because of a
bug elsewhere) would yield a misleading error message: We might well
have been loaded at a 2Mb-aligned boundary, and instead its internal
logic which would then have been wrong. (I'm sorry, now you'll get to
judge whether keeping the check in line with other code or with the
diagnostic is going to be better. Or split things into a build-time
and a runtime check, as previously suggested.)

Jan

Re: [PATCH 1/2] x86/head: check base address alignment

[Roger Pau Monné]

On Tue, May 02, 2023 at 01:11:12PM +0200, Jan Beulich wrote:
> On 02.05.2023 13:05, Jan Beulich wrote:
> > On 02.05.2023 12:51, Roger Pau Monné wrote:
> >> On Tue, May 02, 2023 at 12:28:55PM +0200, Jan Beulich wrote:
> >>> On 02.05.2023 11:54, Andrew Cooper wrote:
> >>>> On 02/05/2023 10:22 am, Roger Pau Monne wrote:
> >>>>> @@ -670,6 +674,11 @@ trampoline_setup:
> >>>>>          cmp     %edi, %eax
> >>>>>          jb      1b
> >>>>>  
> >>>>> +        /* Check that the image base is aligned. */
> >>>>> +        lea     sym_esi(_start), %eax
> >>>>> +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
> >>>>> +        jnz     not_aligned
> >>>>
> >>>> You just want to check the value in %esi, which is the base of the Xen
> >>>> image.  Something like:
> >>>>
> >>>> mov %esi, %eax
> >>>> and ...
> >>>> jnz
> >>>
> >>> Or yet more simply "test $..., %esi" and then "jnz ..."?
> >>
> >> As replied to Andrew, I would rather keep this inline with the address
> >> used to build the PDE, which is sym_esi(_start).
> > 
> > Well, I won't insist, and you've got Andrew's R-b already.
> 
> Actually, one more remark here: While using sym_esi() is more in line
> with the actual consumer of the data, the check triggering because of
> the transformation yielding a misaligned value (in turn because of a
> bug elsewhere) would yield a misleading error message: We might well
> have been loaded at a 2Mb-aligned boundary, and instead its internal
> logic which would then have been wrong. (I'm sorry, now you'll get to
> judge whether keeping the check in line with other code or with the
> diagnostic is going to be better. Or split things into a build-time
> and a runtime check, as previously suggested.)

What about adding a build time check that XEN_VIRT_START is 2MB
aligned, and then just switching to test instead of and, would that be
acceptable?

I know that using sym_esi(_start) instead of just esi won't change the
result of the check if XEN_VIRT_START is aligned, but I would prefer
to keep the usage of sym_esi(_start) for consistency with the value
used to build the page tables, as I think it's clearer.

Thanks, Roger.

Re: [PATCH 1/2] x86/head: check base address alignment

[Jan Beulich]

On 02.05.2023 15:02, Roger Pau Monné wrote:
> On Tue, May 02, 2023 at 01:11:12PM +0200, Jan Beulich wrote:
>> On 02.05.2023 13:05, Jan Beulich wrote:
>>> On 02.05.2023 12:51, Roger Pau Monné wrote:
>>>> On Tue, May 02, 2023 at 12:28:55PM +0200, Jan Beulich wrote:
>>>>> On 02.05.2023 11:54, Andrew Cooper wrote:
>>>>>> On 02/05/2023 10:22 am, Roger Pau Monne wrote:
>>>>>>> @@ -670,6 +674,11 @@ trampoline_setup:
>>>>>>>          cmp     %edi, %eax
>>>>>>>          jb      1b
>>>>>>>  
>>>>>>> +        /* Check that the image base is aligned. */
>>>>>>> +        lea     sym_esi(_start), %eax
>>>>>>> +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
>>>>>>> +        jnz     not_aligned
>>>>>>
>>>>>> You just want to check the value in %esi, which is the base of the Xen
>>>>>> image.  Something like:
>>>>>>
>>>>>> mov %esi, %eax
>>>>>> and ...
>>>>>> jnz
>>>>>
>>>>> Or yet more simply "test $..., %esi" and then "jnz ..."?
>>>>
>>>> As replied to Andrew, I would rather keep this inline with the address
>>>> used to build the PDE, which is sym_esi(_start).
>>>
>>> Well, I won't insist, and you've got Andrew's R-b already.
>>
>> Actually, one more remark here: While using sym_esi() is more in line
>> with the actual consumer of the data, the check triggering because of
>> the transformation yielding a misaligned value (in turn because of a
>> bug elsewhere) would yield a misleading error message: We might well
>> have been loaded at a 2Mb-aligned boundary, and instead its internal
>> logic which would then have been wrong. (I'm sorry, now you'll get to
>> judge whether keeping the check in line with other code or with the
>> diagnostic is going to be better. Or split things into a build-time
>> and a runtime check, as previously suggested.)
> 
> What about adding a build time check that XEN_VIRT_START is 2MB
> aligned, and then just switching to test instead of and, would that be
> acceptable?

Hmm, yes, why not. (Except I would still express it as sym_offs(0)
rather than a direct use of XEN_VIRT_START, once again to better
match surrounding code.)

Jan

> I know that using sym_esi(_start) instead of just esi won't change the
> result of the check if XEN_VIRT_START is aligned, but I would prefer
> to keep the usage of sym_esi(_start) for consistency with the value
> used to build the page tables, as I think it's clearer.
> 
> Thanks, Roger.