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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.