[RFC] x86emul: unconditionally deliver #UD for LWP insns
diff mbox series

Message ID d7b10cdc-b33a-0297-458d-1bad42727fa0@suse.com
State New
Headers show
Series
  • [RFC] x86emul: unconditionally deliver #UD for LWP insns
Related show

Commit Message

Jan Beulich July 17, 2019, 6:42 a.m. UTC
This is to accompany commit  ("x86/svm: Drop support for AMD's
Lightweight Profiling").

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
With AMD apparently having abandoned XOP encoded insns, another option
would seem to be to simply wire all unrecognized ones into #UD (rather
then returning UNIMPLEMENTED/UNRECOGNIZED).
---
TODO/RFC: Insert commit ID of referenced commit.

Comments

Andrew Cooper July 17, 2019, 11:42 a.m. UTC | #1
On 17/07/2019 07:42, Jan Beulich wrote:
> This is to accompany commit  ("x86/svm: Drop support for AMD's

91f86f8634

> Lightweight Profiling").
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> With AMD apparently having abandoned XOP encoded insns, another option
> would seem to be to simply wire all unrecognized ones into #UD (rather
> then returning UNIMPLEMENTED/UNRECOGNIZED).

There are still some XOP instructions which actually work on Fam17h
processors, if you ignore CPUID and go blindly executing.

Given no official statement that XOP is dead, I'd keep the support we
currently have.

~Andrew
Jan Beulich July 17, 2019, 1:07 p.m. UTC | #2
On 17.07.2019 13:42, Andrew Cooper wrote:
> On 17/07/2019 07:42, Jan Beulich wrote:
>> This is to accompany commit  ("x86/svm: Drop support for AMD's
> 
> 91f86f8634

Hmm, odd - I'm sure I had checked, resulting in the impression that
this didn't get committed yet.

>> Lightweight Profiling").
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
>> ---
>> With AMD apparently having abandoned XOP encoded insns, another option
>> would seem to be to simply wire all unrecognized ones into #UD (rather
>> then returning UNIMPLEMENTED/UNRECOGNIZED).
> 
> There are still some XOP instructions which actually work on Fam17h
> processors, if you ignore CPUID and go blindly executing.
> 
> Given no official statement that XOP is dead, I'd keep the support we
> currently have.

Then my remark wasn't clear enough: I'm not suggesting to rip out
XOP insn support we have. I'm instead considering whether to wire
all unsupported XOP encodings into #UD (rather than return
UNIMPLEMENTED/UNRECOGNIZED for them), not just the LWP ones.

Jan
Andrew Cooper July 17, 2019, 5:09 p.m. UTC | #3
On 17/07/2019 14:07, Jan Beulich wrote:
> On 17.07.2019 13:42, Andrew Cooper wrote:
>> On 17/07/2019 07:42, Jan Beulich wrote:
>>> This is to accompany commit  ("x86/svm: Drop support for AMD's
>> 91f86f8634
> Hmm, odd - I'm sure I had checked, resulting in the impression that
> this didn't get committed yet.
>
>>> Lightweight Profiling").
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>>> ---
>>> With AMD apparently having abandoned XOP encoded insns, another option
>>> would seem to be to simply wire all unrecognized ones into #UD (rather
>>> then returning UNIMPLEMENTED/UNRECOGNIZED).
>> There are still some XOP instructions which actually work on Fam17h
>> processors, if you ignore CPUID and go blindly executing.
>>
>> Given no official statement that XOP is dead, I'd keep the support we
>> currently have.
> Then my remark wasn't clear enough: I'm not suggesting to rip out
> XOP insn support we have. I'm instead considering whether to wire
> all unsupported XOP encodings into #UD (rather than return
> UNIMPLEMENTED/UNRECOGNIZED for them), not just the LWP ones.

Ah, in which case, no.  Turning all unknown instructions into
EXCEPTION/#UD will break introspection, which uses UNRECOGNISED to cover
the gaps in the emulator by single-stepping the vcpu.

~Andrew
Jan Beulich July 18, 2019, 9:18 a.m. UTC | #4
On 17.07.2019 19:09, Andrew Cooper wrote:
> On 17/07/2019 14:07, Jan Beulich wrote:
>> On 17.07.2019 13:42, Andrew Cooper wrote:
>>> On 17/07/2019 07:42, Jan Beulich wrote:
>>>> With AMD apparently having abandoned XOP encoded insns, another option
>>>> would seem to be to simply wire all unrecognized ones into #UD (rather
>>>> then returning UNIMPLEMENTED/UNRECOGNIZED).
>>> There are still some XOP instructions which actually work on Fam17h
>>> processors, if you ignore CPUID and go blindly executing.
>>>
>>> Given no official statement that XOP is dead, I'd keep the support we
>>> currently have.
>> Then my remark wasn't clear enough: I'm not suggesting to rip out
>> XOP insn support we have. I'm instead considering whether to wire
>> all unsupported XOP encodings into #UD (rather than return
>> UNIMPLEMENTED/UNRECOGNIZED for them), not just the LWP ones.
> 
> Ah, in which case, no.  Turning all unknown instructions into
> EXCEPTION/#UD will break introspection, which uses UNRECOGNISED to cover
> the gaps in the emulator by single-stepping the vcpu.

But there are no gaps: The only ones we didn't cover (afaik) were the
LWP ones, and those would get made raise #UD by the patch here anyway.
If there are really opcodes where "unrecognized" is relevant, then we
should special case _those_ imo, rather than leaving an opcode space
which apparently won't see further extensions all go the "unrecognized"
route.

Jan
Andrew Cooper July 18, 2019, 9:30 a.m. UTC | #5
On 18/07/2019 10:18, Jan Beulich wrote:
> On 17.07.2019 19:09, Andrew Cooper wrote:
>> On 17/07/2019 14:07, Jan Beulich wrote:
>>> On 17.07.2019 13:42, Andrew Cooper wrote:
>>>> On 17/07/2019 07:42, Jan Beulich wrote:
>>>>> With AMD apparently having abandoned XOP encoded insns, another option
>>>>> would seem to be to simply wire all unrecognized ones into #UD (rather
>>>>> then returning UNIMPLEMENTED/UNRECOGNIZED).
>>>> There are still some XOP instructions which actually work on Fam17h
>>>> processors, if you ignore CPUID and go blindly executing.
>>>>
>>>> Given no official statement that XOP is dead, I'd keep the support we
>>>> currently have.
>>> Then my remark wasn't clear enough: I'm not suggesting to rip out
>>> XOP insn support we have. I'm instead considering whether to wire
>>> all unsupported XOP encodings into #UD (rather than return
>>> UNIMPLEMENTED/UNRECOGNIZED for them), not just the LWP ones.
>> Ah, in which case, no.  Turning all unknown instructions into
>> EXCEPTION/#UD will break introspection, which uses UNRECOGNISED to cover
>> the gaps in the emulator by single-stepping the vcpu.
> But there are no gaps:

[F]XSAVE et al, VT-x, SVM to name several gaps.

Not to mention running current versions of Xen on newer hardware.

> The only ones we didn't cover (afaik) were the
> LWP ones, and those would get made raise #UD by the patch here anyway.
> If there are really opcodes where "unrecognized" is relevant, then we
> should special case _those_ imo, rather than leaving an opcode space
> which apparently won't see further extensions all go the "unrecognized"
> route.

In terms of "not crashing the guest", it is very important to know if
there are instructions which are accepted by the pipeline, trap for an
access violation, and that x86_emulate() doesn't know about.

If we do emulation support perfectly, along with CPUID feature handling
etc, then this set should be 0, but it is still nonzero even with this
patch.

For instructions we know about, and know that we told the guest it
couldn't use, then #UD is fine and correct to do.  Given that
approximately all of access violations are well-formed non-malicious
code, then an instruction which x86_emulate() doesn't know about is most
likely a hole or a CPUID handling issue.  Either way, blindly converting
this to #UD is not the appropriate course of action.

~Andrew
Jan Beulich July 18, 2019, 9:44 a.m. UTC | #6
On 18.07.2019 11:30, Andrew Cooper wrote:
> On 18/07/2019 10:18, Jan Beulich wrote:
>> On 17.07.2019 19:09, Andrew Cooper wrote:
>>> On 17/07/2019 14:07, Jan Beulich wrote:
>>>> On 17.07.2019 13:42, Andrew Cooper wrote:
>>>>> On 17/07/2019 07:42, Jan Beulich wrote:
>>>>>> With AMD apparently having abandoned XOP encoded insns, another option
>>>>>> would seem to be to simply wire all unrecognized ones into #UD (rather
>>>>>> then returning UNIMPLEMENTED/UNRECOGNIZED).
>>>>> There are still some XOP instructions which actually work on Fam17h
>>>>> processors, if you ignore CPUID and go blindly executing.
>>>>>
>>>>> Given no official statement that XOP is dead, I'd keep the support we
>>>>> currently have.
>>>> Then my remark wasn't clear enough: I'm not suggesting to rip out
>>>> XOP insn support we have. I'm instead considering whether to wire
>>>> all unsupported XOP encodings into #UD (rather than return
>>>> UNIMPLEMENTED/UNRECOGNIZED for them), not just the LWP ones.
>>> Ah, in which case, no.  Turning all unknown instructions into
>>> EXCEPTION/#UD will break introspection, which uses UNRECOGNISED to cover
>>> the gaps in the emulator by single-stepping the vcpu.
>> But there are no gaps:
> 
> [F]XSAVE et al, VT-x, SVM to name several gaps.
> 
> Not to mention running current versions of Xen on newer hardware.

None of these are XOP encoded. I've never been considering to wire
everything into #UD.

Jan

Patch
diff mbox series

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -10367,6 +10367,16 @@  x86_emulate(
          }
          goto unrecognized_insn;
  
+    case X86EMUL_OPC_XOP(09, 0x12): /* XOP Grp3 */
+        switch ( modrm_reg & 7 )
+        {
+        case 0: /* llwpcb r */
+        case 1: /* slwpcb r */
+            /* LWP is unsupported, so produce #UD unconditionally. */
+            generate_exception(EXC_UD);
+        }
+        goto unrecognized_insn;
+
      case X86EMUL_OPC_XOP(09, 0x82): /* vfrczss xmm/m128,xmm */
      case X86EMUL_OPC_XOP(09, 0x83): /* vfrczsd xmm/m128,xmm */
          generate_exception_if(vex.l, EXC_UD);
@@ -10451,6 +10461,16 @@  x86_emulate(
          break;
      }
  
+    case X86EMUL_OPC_XOP(0a, 0x12): /* XOP Grp4 */
+        switch ( modrm_reg & 7 )
+        {
+        case 0: /* lwpins $imm32,r/m,r */
+        case 1: /* lwpval $imm32,r/m,r */
+            /* LWP is unsupported, so produce #UD unconditionally. */
+            generate_exception(EXC_UD);
+        }
+        goto unrecognized_insn;
+
      default:
      unimplemented_insn:
          rc = X86EMUL_UNIMPLEMENTED;