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....@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.coop...@citrix.com>, preferably with the extra note in the commit message. ~Andrew
