Re: [Xen-devel] Ping: [PATCH v2] x86/EFI: meet further spec requirements for runtime calls

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.11.16 at 11:41, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/11/16 10:09, Jan Beulich wrote:
>>>>> On 15.11.16 at 17:06, <JBeulich@xxxxxxxx> wrote:
>>>>>> On 15.11.16 at 16:47, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 14/11/16 10:32, Jan Beulich wrote:
>>>>> So far we didn't guarantee 16-byte alignment of the stack: While (so
>>>>> far) we don't tell the compiler to use smaller alignment, we also don't
>>>>> guarantee 16-byte alignment when establishing stack pointers for new
>>>>> vCPU-s. Runtime service functions using SSE instructions may end with
>>>>> #GP(0) without that.
>>>>>
>>>>> Note that making use of -mpreferred-stack-boundary=3, as mentioned in
>>>>> the comment, wouldn't help to reduce the needed alignment: The compiler
>>>>> would then be free to align the stack of the function with the aligned
>>>>> object, but would be permitted to place an odd number of 8-byte objects
>>>>> there, resulting in the callee to still run on an unaligned stack.
>>>>>
>>>>> (The only working alternative to the approach chosen here would be to
>>>>> use -mincoming-stack-boundary=3, but that would affect all functions in
>>>>> runtime.c, not just the ones actually making runtime services calls.
>>>>> And it would still require the manual alignment logic here to be used
>>>>> with gcc 5.2 and earlier - not permitting that command line option -,
>>>>> just that then the alignment amount would become conditional.)
>>>>>
>>>>> Hence enforce the needed alignment by making efi_rs_enter() return a
>>>>> suitably aligned structure, which the caller then necessarily has to
>>>>> store in a suitably aligned local variable, the address of which then
>>>>> gets passed to efi_rs_leave(). Also (to limit exposure) move the
>>>>> function declarations to where they belong: They're local to runtime.c,
>>>>> and shared only with compat.c (by the latter including the former).
>>>> Why does this guarantee alignment?  What prevents the compiler from
>>>> reordering the items in its stack layout?
>>> The compiler will always allocate stack variables such that called
>>> functions will see an ABI-compliant stack. Without variables of
>>> bigger alignment, it does this by implying that the current function
>>> also has an aligned stack. Since we start out with a stack frame on
>>> an 8 mod 16 boundary, said compiler behavior propagates
>>> this through all call hierarchies. With variables of bigger alignment
>>> the compiler arranges for the current frame to be suitably
>>> expanded, and it will of course continue to guarantee that all
>>> callees get to see a 16-byte aligned stack. IOW all we need to do
>>> is break this 8 mod 16 thing once.
>> Ping? We have a problem to solve here, so I think I can expect that
>> either the proposed solution (even if not covering all theoretical
>> cases of possibly compiler behavior) is accepted, or an alternative
>> proposal is put up.
> 
> Right, so the proposed patch is half a solution, in that it will resolve
> the issue when a compiler make one of two choices.  From this point of
> view, it is an improvement.
> 
>> I'd really like to avoid seeing 4.8 go out with the
>> problem un-addressed.
> 
> However, calling the issue addressed after this patch is false.  A
> compiler which chooses to reorder stack objects differently will still
> fail to maintain the alignment you are trying to impose.

Considering that the alternatives are (from what I can tell) more
expensive, I can't see why a compiler would do that. Or maybe
I'm still not getting the exact scenario you're thinking about ...

>> (From a strictly formal perspective, me being
>> the only maintainer of EFI code, I could put the patch in without any
>> ack [other than Wei's release one], but I'd like to avoid that if at all
>> possible.)
> 
> If you want an ack, then fine.  Acked-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx> but I don't expect this fully resolve the issue.

Thanks. And yes, we likely will want to make our stack handling
ABI compliant, at which point this patch could be reverted.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel