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

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Nov 22, 2016 at 03:09:37AM -0700, Jan Beulich wrote:
> >>> On 15.11.16 at 17:06, <JBeulich@suse.com> wrote:
> >>>> On 15.11.16 at 16:47, <andrew.cooper3@citrix.com> 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. I'd really like to avoid seeing 4.8 go out with the
> problem un-addressed. (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.)
> 

I can't judge the technical correctness of this patch. I think it would
be good to fix this, so:

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

> Jan
> 

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