x86emul: suppress general register update upon AVX gather failures
diff mbox series

Message ID 5C9DE7E40200007800222BA7@prv1-mh.provo.novell.com
State New, archived
Headers show
Series
  • x86emul: suppress general register update upon AVX gather failures
Related show

Commit Message

Jan Beulich March 29, 2019, 9:39 a.m. UTC
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.

Comments

Andrew Cooper April 1, 2019, 2:04 p.m. UTC | #1
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;
>      }
>
>
Jan Beulich April 1, 2019, 3:12 p.m. UTC | #2
>>> 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
Andrew Cooper April 1, 2019, 4:11 p.m. UTC | #3
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>

Patch
diff mbox series

--- 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;
     }