| Message ID | 20230502092224.52265-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: init improvements | expand |
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
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
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.
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.
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
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.
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
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
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.
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.
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 ^ */
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(+)