| Message ID | 584954570200007800126ABB@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 08.12.16 at 12:38, <JBeulich@suse.com> wrote: > The memory clobber is rather harsh here. However, fic.exn_raised may be > modified as a side effect, so we need to let the compiler know that all > of "fic" may be changed (preventing it from moving around accesses to > the exn_raised field). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Ping? (The v1 discussion had stalled, so at that point it seemed best to simply re-send with the updates to the earlier patches.) > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -759,7 +759,7 @@ do { > }) > > struct fpu_insn_ctxt { > - uint8_t insn_bytes; > + uint8_t insn_bytes; /* Must be first! */ > int8_t exn_raised; > }; > > @@ -853,23 +853,21 @@ do { > "movb $2f-1f,%0 \n" \ > "1: " _op " \n" \ > "2: \n" \ > - : "=m" (fic.insn_bytes) : : "memory" ) > + : "+m" (fic) ) > > #define emulate_fpu_insn_memdst(_op, _arg) \ > asm volatile ( \ > "movb $2f-1f,%0 \n" \ > "1: " _op " %1 \n" \ > "2: \n" \ > - : "=m" (fic.insn_bytes), "=m" (_arg) \ > - : : "memory" ) > + : "+m" (fic), "=m" (_arg) ) > > #define emulate_fpu_insn_memsrc(_op, _arg) \ > asm volatile ( \ > "movb $2f-1f,%0 \n" \ > "1: " _op " %1 \n" \ > "2: \n" \ > - : "=m" (fic.insn_bytes) \ > - : "m" (_arg) : "memory" ) > + : "+m" (fic) : "m" (_arg) ) > > #define emulate_fpu_insn_stub(_bytes...) \ > do { \
On 15/12/16 09:52, Jan Beulich wrote: >>>> On 08.12.16 at 12:38, <JBeulich@suse.com> wrote: >> The memory clobber is rather harsh here. However, fic.exn_raised may be >> modified as a side effect, so we need to let the compiler know that all >> of "fic" may be changed (preventing it from moving around accesses to >> the exn_raised field). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > Ping? (The v1 discussion had stalled, so at that point it seemed best > to simply re-send with the updates to the earlier patches.) Sorry, yes. Having considered this further (when considering my other exception plans), I am now more convinced the code is already latently buggy, and fixing that latent bug avoids the need for this. exn_raised can be set at any point between a {get,put}_fpu(), not just within these stubs. It therefore should already be accessed with a volatile reference to avoid breaking C's model of the world, as the memory clobbers here are already not sufficient. Fixing the volatility of access to exn_raised will allow the safe dropping of the memory clobbers, without further modifying the memory operands. I already have a patch to do similar improvements to the time init code from my memory barrier series. I will see about getting that posted. ~Andrew
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -759,7 +759,7 @@ do { }) struct fpu_insn_ctxt { - uint8_t insn_bytes; + uint8_t insn_bytes; /* Must be first! */ int8_t exn_raised; }; @@ -853,23 +853,21 @@ do { "movb $2f-1f,%0 \n" \ "1: " _op " \n" \ "2: \n" \ - : "=m" (fic.insn_bytes) : : "memory" ) + : "+m" (fic) ) #define emulate_fpu_insn_memdst(_op, _arg) \ asm volatile ( \ "movb $2f-1f,%0 \n" \ "1: " _op " %1 \n" \ "2: \n" \ - : "=m" (fic.insn_bytes), "=m" (_arg) \ - : : "memory" ) + : "+m" (fic), "=m" (_arg) ) #define emulate_fpu_insn_memsrc(_op, _arg) \ asm volatile ( \ "movb $2f-1f,%0 \n" \ "1: " _op " %1 \n" \ "2: \n" \ - : "=m" (fic.insn_bytes) \ - : "m" (_arg) : "memory" ) + : "+m" (fic) : "m" (_arg) ) #define emulate_fpu_insn_stub(_bytes...) \ do { \