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.
