| Message ID | 5C9DE7E40200007800222BA7@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86emul: suppress general register update upon AVX gather failures | expand |
On 29/03/2019 09:39, Jan Beulich wrote: > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -8547,6 +8547,9 @@ x86_emulate( > invoke_stub("", "", "+m" (mask) : "a" (&mask)); > put_stub(stub); > > + if ( rc != X86EMUL_OKAY ) > + goto done; This obviously is needed to fix the problem as it manifested for George, but in the case that we take a fault/retry on iteration 0, shouldn't we skip the writeback as well? ~Andrew > + > state->simd_size = simd_none; > break; > } > >
>>> On 01.04.19 at 16:04, <andrew.cooper3@citrix.com> wrote: > On 29/03/2019 09:39, Jan Beulich wrote: >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -8547,6 +8547,9 @@ x86_emulate( >> invoke_stub("", "", "+m" (mask) : "a" (&mask)); >> put_stub(stub); >> >> + if ( rc != X86EMUL_OKAY ) >> + goto done; > > This obviously is needed to fix the problem as it manifested for George, > but in the case that we take a fault/retry on iteration 0, shouldn't we > skip the writeback as well? No, not really, or at least not in all cases: "If the data size and index size are different, part of the destination register and part of the mask register do not correspond to any elements being gathered. This instruction sets those parts to zero. It may do this to one or both of those registers even if the instruction triggers an exception, and even if the instruction triggers the exception before gathering any elements." I realize this is all "may", but iirc I had tried it out on some of the hardware I have, and it indeed did so there. Jan
On 01/04/2019 16:12, Jan Beulich wrote: >>>> On 01.04.19 at 16:04, <andrew.cooper3@citrix.com> wrote: >> On 29/03/2019 09:39, Jan Beulich wrote: >>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >>> @@ -8547,6 +8547,9 @@ x86_emulate( >>> invoke_stub("", "", "+m" (mask) : "a" (&mask)); >>> put_stub(stub); >>> >>> + if ( rc != X86EMUL_OKAY ) >>> + goto done; >> This obviously is needed to fix the problem as it manifested for George, >> but in the case that we take a fault/retry on iteration 0, shouldn't we >> skip the writeback as well? > No, not really, or at least not in all cases: > > "If the data size and index size are different, part of the destination > register and part of the mask register do not correspond to any > elements being gathered. This instruction sets those parts to zero. It > may do this to one or both of those registers even if the instruction > triggers an exception, and even if the instruction triggers the > exception before gathering any elements." > > I realize this is all "may", but iirc I had tried it out on some of the > hardware I have, and it indeed did so there. That does only talk about the mismatched parts of the registers, but the fact that it does call out the 0 case does demonstrate that a write occurs. What I'd missed is that the writes are only actually to registers, not to memory, which comes with the possibility of accumulating an unrelated fault. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -8547,6 +8547,9 @@ x86_emulate( invoke_stub("", "", "+m" (mask) : "a" (&mask)); put_stub(stub); + if ( rc != X86EMUL_OKAY ) + goto done; + state->simd_size = simd_none; break; }
While destination and mask registers may indeed need updating in this case, the rIP update in particular needs to be avoided, as well as e.g. raising a single step trap. Reported-by: George Dunlap <george.dunlap@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- This is surely a stable tree candidate, unless it could still make it into 4.12 before the release. --- Obviously I'm folding the same change into the AVX512F gather and scatter patches as well as the AVX512PF one.