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.
