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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

> (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.

~Andrew

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