| Message ID | 5846D5C30200007800125C9C@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/12/16 14:14, Jan Beulich wrote: > The memory clobber is rather harsh here. Does removing it actually make a difference? I can't spot anything which could reasonably be reordered around the asm() blocks. > 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> > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -784,7 +784,7 @@ do { > }) > > struct fpu_insn_ctxt { > - uint8_t insn_bytes; > + uint8_t insn_bytes; /* Must be first! */ And one single byte. The compiler would previously have caught an accidental breaking of this requirement. As an alternative, how about using ACCESS_ONCE() to read exn_raised? It would allow you to drop the memory clobber and also not generalise the fic.insn_bytes memory parameter to fic. ~Andrew
>>> On 07.12.16 at 16:16, <andrew.cooper3@citrix.com> wrote: > On 06/12/16 14:14, Jan Beulich wrote: >> The memory clobber is rather harsh here. > > Does removing it actually make a difference? I can't spot anything > which could reasonably be reordered around the asm() blocks. It's not so much the re-ordering but the potential dropping of something that was cached in a register. Indeed there was no change to the generated code at all with current gcc, but the compiler gets better over time, and we shouldn't restrict it unduly. >> 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> >> >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -784,7 +784,7 @@ do { >> }) >> >> struct fpu_insn_ctxt { >> - uint8_t insn_bytes; >> + uint8_t insn_bytes; /* Must be first! */ > > And one single byte. The compiler would previously have caught an > accidental breaking of this requirement. There was no such requirement before. Of course we could add an immediate operand to all the asm()s, specifying the offsetof(). But then again we already have a hidden dependency on its size anyway. > As an alternative, how about using ACCESS_ONCE() to read exn_raised? It > would allow you to drop the memory clobber and also not generalise the > fic.insn_bytes memory parameter to fic. There's no ACCESS_ONCE() in the x86 code, and even less so in what the emulator code can use. Nor would what you suggest allow to legitimately drop the memory clobber. Jan
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -784,7 +784,7 @@ do { }) struct fpu_insn_ctxt { - uint8_t insn_bytes; + uint8_t insn_bytes; /* Must be first! */ int8_t exn_raised; }; @@ -878,23 +878,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 { \