[PATCH 0/2] Fixes to RDTSCP interception

[PATCH 0/2] Fixes to RDTSCP interception

[Andrew Cooper]

For some reason (on EPYC at least) we've gained a regression into master
whereby VMs are defaulting to one of the emulated TSC modes.  This may be Xen
coming to the conclusion that there isn't a reliable TSC.  Combined with a
debug Xen, this breaks RDTSCP completely.

With this series in place and RDTSCP functioning correctly again, VMs are
still unwilling to boot.  I haven't managed to figure out why yet.

Andrew Cooper (2):
  x86/svm: Improve diagnostics when __get_instruction_length_from_list() fails
  x86/hvm: Corrections to RDTSCP intercept handling

 xen/arch/x86/hvm/svm/emulate.c        | 27 ++++++++++++++++++++-------
 xen/arch/x86/hvm/svm/svm.c            | 22 +++++++++++++++++-----
 xen/arch/x86/hvm/vmx/vmx.c            |  8 ++++++++
 xen/include/asm-x86/hvm/svm/emulate.h |  1 +
 4 files changed, 46 insertions(+), 12 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/2] x86/svm: Improve diagnostics when __get_instruction_length_from_list() fails

[Andrew Cooper]

Sadly, a lone:

  (XEN) emulate.c:156:d2v0 __get_instruction_length_from_list: Mismatch between expected and actual instruction: eip = fffff804564139c0

on the console is of no use trying to identify what went wrong.  Dump as much
state as we can to help identify what went wrong.

Reported-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>

RFC: __get_instruction_length_from_list() tries to cope with VMEXIT_IOIO, but
IN/OUT instructions aren't in the decode list and I can't spot an entry point
from the IOIO path.  Am I missing something?

Also, I'm not entirely convinced that making modrm an annonymous union is
going to work with older CentOS compilers, and therefore am not sure whether
that part of the change is worth it.  The instruction in question can be
obtained from the printed INSN_ constant alone.
---
 xen/arch/x86/hvm/svm/emulate.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index 3d04af0..71a1b6e 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -56,11 +56,14 @@ static unsigned long svm_nextrip_insn_length(struct vcpu *v)
 
 static const struct {
     unsigned int opcode;
-    struct {
-        unsigned int rm:3;
-        unsigned int reg:3;
-        unsigned int mod:2;
-#define MODRM(mod, reg, rm) { rm, reg, mod }
+    union {
+        struct {
+            unsigned int rm:3;
+            unsigned int reg:3;
+            unsigned int mod:2;
+        };
+        unsigned int raw;
+#define MODRM(mod, reg, rm) {{ rm, reg, mod }}
     } modrm;
 } opc_tab[INSTR_MAX_COUNT] = {
     [INSTR_PAUSE]   = { X86EMUL_OPC_F3(0, 0x90) },
@@ -152,8 +155,17 @@ int __get_instruction_length_from_list(struct vcpu *v,
     }
 
     gdprintk(XENLOG_WARNING,
-             "%s: Mismatch between expected and actual instruction: "
-             "eip = %lx\n",  __func__, (unsigned long)vmcb->rip);
+             "%s: Mismatch between expected and actual instruction:\n",
+             __func__);
+    gdprintk(XENLOG_WARNING,
+             "  list[0] val %d, { opc %#x, modrm %#x }, list entries: %u\n",
+             list[0], opc_tab[list[0]].opcode, opc_tab[list[0]].modrm.raw,
+             list_count);
+    gdprintk(XENLOG_WARNING, "  rip 0x%lx, nextrip 0x%lx, len %lu\n",
+             vmcb->rip, vmcb->nextrip, vmcb->nextrip - vmcb->rip);
+    hvm_dump_emulation_state(XENLOG_G_WARNING, "Insn_len",
+                             &ctxt, X86EMUL_UNHANDLEABLE);
+
     hvm_inject_hw_exception(TRAP_gp_fault, 0);
     return 0;
 }
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling

[Andrew Cooper]

For both VT-x and SVM, the RDTSCP intercept will trigger if the pipeline
supports the instruction, but the guest may have not have rdtscp in its
featureset.  Bring the vmexit handlers in line with the main emulator
behaviour by optionally handing back #UD.

Next on the AMD side, if RDTSCP actually ends up being intercepted on a debug
build, we first update regs->rcx, then call __get_instruction_length() asking
for RDTSC.  As the two instructions are different (and indeed, different
lengths!), __get_instruction_length_from_list() fails and hands back a #GP
fault.

This can demonstrated by putting a guest into tsc_mode="always emulate" and
executing an rdtscp instruction:

  (d1) --- Xen Test Framework ---
  (d1) Environment: HVM 64bit (Long mode 4 levels)
  (d1) Test rdtscp
  (d1) TSC mode 1
  (XEN) emulate.c:159:d1v0 __get_instruction_length_from_list: Mismatch between expected and actual instruction:
  (XEN) emulate.c:163:d1v0   list[0] val 8, { opc 0xf0031, modrm 0 }, list entries: 1
  (XEN) emulate.c:165:d1v0   rip 0x10475f, nextrip 0x104762, len 3
  (XEN) Insn_len emulation failed (1): d1v0 64bit @ 0008:0010475f -> 0f 01 f9 5b 31 ff 31 c0 e9 c4 db ff ff 00 00 00
  (d1) ******************************
  (d1) PANIC: Unhandled exception at 0008:000000000010475f
  (d1) Vec 13 #GP[0000]
  (d1) ******************************

First, teach __get_instruction_length() to cope with RDTSCP, and improve
svm_vmexit_do_rdtsc() to ask for the correct instruction.  Move the regs->rcx
adjustment into this function to ensure it gets done after we are done
potentially raising faults.

Reported-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
CC: Juergen Gross <jgross@suse.com>

There is a further 4.12 bug/regression here.  For some reason, master and
staging are now defaulting VMs into an emulated TSC mode.  I have yet to
figure out why.
---
 xen/arch/x86/hvm/svm/emulate.c        |  1 +
 xen/arch/x86/hvm/svm/svm.c            | 22 +++++++++++++++++-----
 xen/arch/x86/hvm/vmx/vmx.c            |  8 ++++++++
 xen/include/asm-x86/hvm/svm/emulate.h |  1 +
 4 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index 71a1b6e..0290264 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -78,6 +78,7 @@ static const struct {
     [INSTR_STGI]    = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 4) },
     [INSTR_CLGI]    = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 5) },
     [INSTR_INVLPGA] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 7) },
+    [INSTR_RDTSCP]  = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 7, 1) },
     [INSTR_INVD]    = { X86EMUL_OPC(0x0f, 0x08) },
     [INSTR_WBINVD]  = { X86EMUL_OPC(0x0f, 0x09) },
     [INSTR_WRMSR]   = { X86EMUL_OPC(0x0f, 0x30) },
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b9a8900..d8d3813 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2279,14 +2279,28 @@ static void svm_vmexit_do_hlt(struct vmcb_struct *vmcb,
     hvm_hlt(regs->eflags);
 }
 
-static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs)
+static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs, bool rdtscp)
 {
+    struct vcpu *curr = current;
+    const struct domain *currd = curr->domain;
+    enum instruction_index insn = rdtscp ? INSTR_RDTSCP : INSTR_RDTSC;
     unsigned int inst_len;
 
-    if ( (inst_len = __get_instruction_length(current, INSTR_RDTSC)) == 0 )
+    if ( rdtscp && !currd->arch.cpuid->extd.rdtscp &&
+         currd->arch.tsc_mode != TSC_MODE_PVRDTSCP )
+    {
+        hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
         return;
+    }
+
+    if ( (inst_len = __get_instruction_length(curr, insn)) == 0 )
+        return;
+
     __update_guest_eip(regs, inst_len);
 
+    if ( rdtscp )
+        regs->rcx = hvm_msr_tsc_aux(curr);
+
     hvm_rdtsc_intercept(regs);
 }
 
@@ -2968,10 +2982,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     case VMEXIT_RDTSCP:
-        regs->rcx = hvm_msr_tsc_aux(v);
-        /* fall through */
     case VMEXIT_RDTSC:
-        svm_vmexit_do_rdtsc(regs);
+        svm_vmexit_do_rdtsc(regs, exit_reason == VMEXIT_RDTSCP);
         break;
 
     case VMEXIT_MONITOR:
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 365eeb2..a9f9b9b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3589,6 +3589,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
     unsigned long exit_qualification, exit_reason, idtv_info, intr_info = 0;
     unsigned int vector = 0, mode;
     struct vcpu *v = current;
+    struct domain *currd = v->domain;
 
     __vmread(GUEST_RIP,    &regs->rip);
     __vmread(GUEST_RSP,    &regs->rsp);
@@ -3956,6 +3957,13 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         vmx_invlpg_intercept(exit_qualification);
         break;
     case EXIT_REASON_RDTSCP:
+        if ( !currd->arch.cpuid->extd.rdtscp &&
+             currd->arch.tsc_mode != TSC_MODE_PVRDTSCP )
+        {
+            hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
+            break;
+        }
+
         regs->rcx = hvm_msr_tsc_aux(v);
         /* fall through */
     case EXIT_REASON_RDTSC:
diff --git a/xen/include/asm-x86/hvm/svm/emulate.h b/xen/include/asm-x86/hvm/svm/emulate.h
index 3de8236..ca92abb 100644
--- a/xen/include/asm-x86/hvm/svm/emulate.h
+++ b/xen/include/asm-x86/hvm/svm/emulate.h
@@ -30,6 +30,7 @@ enum instruction_index {
     INSTR_HLT,
     INSTR_INT3,
     INSTR_RDTSC,
+    INSTR_RDTSCP,
     INSTR_PAUSE,
     INSTR_XSETBV,
     INSTR_VMRUN,
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86/svm: Improve diagnostics when __get_instruction_length_from_list() fails

[Woods, Brian]

On Fri, Nov 30, 2018 at 05:07:19PM +0000, Andy Cooper wrote:
> Sadly, a lone:
> 
>   (XEN) emulate.c:156:d2v0 __get_instruction_length_from_list: Mismatch between expected and actual instruction: eip = fffff804564139c0
> 
> on the console is of no use trying to identify what went wrong.  Dump as much
> state as we can to help identify what went wrong.
> 
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Brian Woods <brian.woods@amd.com>

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling

[Woods, Brian]

On Fri, Nov 30, 2018 at 05:07:20PM +0000, Andy Cooper wrote:
> For both VT-x and SVM, the RDTSCP intercept will trigger if the pipeline
> supports the instruction, but the guest may have not have rdtscp in its
> featureset.  Bring the vmexit handlers in line with the main emulator
> behaviour by optionally handing back #UD.
> 
> Next on the AMD side, if RDTSCP actually ends up being intercepted on a debug
> build, we first update regs->rcx, then call __get_instruction_length() asking
> for RDTSC.  As the two instructions are different (and indeed, different
> lengths!), __get_instruction_length_from_list() fails and hands back a #GP
> fault.
> 
> This can demonstrated by putting a guest into tsc_mode="always emulate" and
> executing an rdtscp instruction:
> 
>   (d1) --- Xen Test Framework ---
>   (d1) Environment: HVM 64bit (Long mode 4 levels)
>   (d1) Test rdtscp
>   (d1) TSC mode 1
>   (XEN) emulate.c:159:d1v0 __get_instruction_length_from_list: Mismatch between expected and actual instruction:
>   (XEN) emulate.c:163:d1v0   list[0] val 8, { opc 0xf0031, modrm 0 }, list entries: 1
>   (XEN) emulate.c:165:d1v0   rip 0x10475f, nextrip 0x104762, len 3
>   (XEN) Insn_len emulation failed (1): d1v0 64bit @ 0008:0010475f -> 0f 01 f9 5b 31 ff 31 c0 e9 c4 db ff ff 00 00 00
>   (d1) ******************************
>   (d1) PANIC: Unhandled exception at 0008:000000000010475f
>   (d1) Vec 13 #GP[0000]
>   (d1) ******************************
> 
> First, teach __get_instruction_length() to cope with RDTSCP, and improve
> svm_vmexit_do_rdtsc() to ask for the correct instruction.  Move the regs->rcx
> adjustment into this function to ensure it gets done after we are done
> potentially raising faults.
> 
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

As far as the SVM part:
Reviewed-by: Brian Woods <brian.woods@amd.com>

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86/svm: Improve diagnostics when __get_instruction_length_from_list() fails

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 30 November 2018 17:07
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Boris
> Ostrovsky <boris.ostrovsky@oracle.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>
> Subject: [PATCH 1/2] x86/svm: Improve diagnostics when
> __get_instruction_length_from_list() fails
> 
> Sadly, a lone:
> 
>   (XEN) emulate.c:156:d2v0 __get_instruction_length_from_list: Mismatch
> between expected and actual instruction: eip = fffff804564139c0
> 
> on the console is of no use trying to identify what went wrong.  Dump as
> much
> state as we can to help identify what went wrong.
> 
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
> 
> RFC: __get_instruction_length_from_list() tries to cope with VMEXIT_IOIO,
> but
> IN/OUT instructions aren't in the decode list and I can't spot an entry
> point
> from the IOIO path.  Am I missing something?


Yes, odd. IOIO are handled in the ifdef NDEBUG blocks but an IOIO exit does indeed not call into __get_instruction_length() but does the same calculation inline. 

> 
> Also, I'm not entirely convinced that making modrm an annonymous union is
> going to work with older CentOS compilers, and therefore am not sure
> whether
> that part of the change is worth it.  The instruction in question can be
> obtained from the printed INSN_ constant alone.

You could just dump the bitfield values individually.

  Paul

> ---
>  xen/arch/x86/hvm/svm/emulate.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/emulate.c
> b/xen/arch/x86/hvm/svm/emulate.c
> index 3d04af0..71a1b6e 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -56,11 +56,14 @@ static unsigned long svm_nextrip_insn_length(struct
> vcpu *v)
> 
>  static const struct {
>      unsigned int opcode;
> -    struct {
> -        unsigned int rm:3;
> -        unsigned int reg:3;
> -        unsigned int mod:2;
> -#define MODRM(mod, reg, rm) { rm, reg, mod }
> +    union {
> +        struct {
> +            unsigned int rm:3;
> +            unsigned int reg:3;
> +            unsigned int mod:2;
> +        };
> +        unsigned int raw;
> +#define MODRM(mod, reg, rm) {{ rm, reg, mod }}
>      } modrm;
>  } opc_tab[INSTR_MAX_COUNT] = {
>      [INSTR_PAUSE]   = { X86EMUL_OPC_F3(0, 0x90) },
> @@ -152,8 +155,17 @@ int __get_instruction_length_from_list(struct vcpu
> *v,
>      }
> 
>      gdprintk(XENLOG_WARNING,
> -             "%s: Mismatch between expected and actual instruction: "
> -             "eip = %lx\n",  __func__, (unsigned long)vmcb->rip);
> +             "%s: Mismatch between expected and actual instruction:\n",
> +             __func__);
> +    gdprintk(XENLOG_WARNING,
> +             "  list[0] val %d, { opc %#x, modrm %#x }, list entries:
> %u\n",
> +             list[0], opc_tab[list[0]].opcode,
> opc_tab[list[0]].modrm.raw,
> +             list_count);
> +    gdprintk(XENLOG_WARNING, "  rip 0x%lx, nextrip 0x%lx, len %lu\n",
> +             vmcb->rip, vmcb->nextrip, vmcb->nextrip - vmcb->rip);
> +    hvm_dump_emulation_state(XENLOG_G_WARNING, "Insn_len",
> +                             &ctxt, X86EMUL_UNHANDLEABLE);
> +
>      hvm_inject_hw_exception(TRAP_gp_fault, 0);
>      return 0;
>  }
> --
> 2.1.4

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 30 November 2018 17:07
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jun
> Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>;
> Boris Ostrovsky <boris.ostrovsky@oracle.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>;
> Juergen Gross <jgross@suse.com>
> Subject: [PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling
> 
> For both VT-x and SVM, the RDTSCP intercept will trigger if the pipeline
> supports the instruction, but the guest may have not have rdtscp in its
> featureset.  Bring the vmexit handlers in line with the main emulator
> behaviour by optionally handing back #UD.
> 
> Next on the AMD side, if RDTSCP actually ends up being intercepted on a
> debug
> build, we first update regs->rcx, then call __get_instruction_length()
> asking
> for RDTSC.  As the two instructions are different (and indeed, different
> lengths!), __get_instruction_length_from_list() fails and hands back a #GP
> fault.
> 
> This can demonstrated by putting a guest into tsc_mode="always emulate"
> and
> executing an rdtscp instruction:
> 
>   (d1) --- Xen Test Framework ---
>   (d1) Environment: HVM 64bit (Long mode 4 levels)
>   (d1) Test rdtscp
>   (d1) TSC mode 1
>   (XEN) emulate.c:159:d1v0 __get_instruction_length_from_list: Mismatch
> between expected and actual instruction:
>   (XEN) emulate.c:163:d1v0   list[0] val 8, { opc 0xf0031, modrm 0 }, list
> entries: 1
>   (XEN) emulate.c:165:d1v0   rip 0x10475f, nextrip 0x104762, len 3
>   (XEN) Insn_len emulation failed (1): d1v0 64bit @ 0008:0010475f -> 0f 01
> f9 5b 31 ff 31 c0 e9 c4 db ff ff 00 00 00
>   (d1) ******************************
>   (d1) PANIC: Unhandled exception at 0008:000000000010475f
>   (d1) Vec 13 #GP[0000]
>   (d1) ******************************
> 
> First, teach __get_instruction_length() to cope with RDTSCP, and improve
> svm_vmexit_do_rdtsc() to ask for the correct instruction.  Move the regs-
> >rcx
> adjustment into this function to ensure it gets done after we are done
> potentially raising faults.
> 
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

With one observation...

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
> CC: Juergen Gross <jgross@suse.com>
> 
> There is a further 4.12 bug/regression here.  For some reason, master and
> staging are now defaulting VMs into an emulated TSC mode.  I have yet to
> figure out why.
> ---
>  xen/arch/x86/hvm/svm/emulate.c        |  1 +
>  xen/arch/x86/hvm/svm/svm.c            | 22 +++++++++++++++++-----
>  xen/arch/x86/hvm/vmx/vmx.c            |  8 ++++++++
>  xen/include/asm-x86/hvm/svm/emulate.h |  1 +
>  4 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/emulate.c
> b/xen/arch/x86/hvm/svm/emulate.c
> index 71a1b6e..0290264 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -78,6 +78,7 @@ static const struct {
>      [INSTR_STGI]    = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 4) },
>      [INSTR_CLGI]    = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 5) },
>      [INSTR_INVLPGA] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 7) },
> +    [INSTR_RDTSCP]  = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 7, 1) },
>      [INSTR_INVD]    = { X86EMUL_OPC(0x0f, 0x08) },
>      [INSTR_WBINVD]  = { X86EMUL_OPC(0x0f, 0x09) },
>      [INSTR_WRMSR]   = { X86EMUL_OPC(0x0f, 0x30) },
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index b9a8900..d8d3813 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2279,14 +2279,28 @@ static void svm_vmexit_do_hlt(struct vmcb_struct
> *vmcb,
>      hvm_hlt(regs->eflags);
>  }
> 
> -static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs)
> +static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs, bool rdtscp)
>  {
> +    struct vcpu *curr = current;
> +    const struct domain *currd = curr->domain;
> +    enum instruction_index insn = rdtscp ? INSTR_RDTSCP : INSTR_RDTSC;
>      unsigned int inst_len;
> 
> -    if ( (inst_len = __get_instruction_length(current, INSTR_RDTSC)) == 0
> )
> +    if ( rdtscp && !currd->arch.cpuid->extd.rdtscp &&
> +         currd->arch.tsc_mode != TSC_MODE_PVRDTSCP )
> +    {
> +        hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
>          return;
> +    }
> +
> +    if ( (inst_len = __get_instruction_length(curr, insn)) == 0 )
> +        return;
> +
>      __update_guest_eip(regs, inst_len);
> 
> +    if ( rdtscp )
> +        regs->rcx = hvm_msr_tsc_aux(curr);
> +
>      hvm_rdtsc_intercept(regs);
>  }
> 
> @@ -2968,10 +2982,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>          break;
> 
>      case VMEXIT_RDTSCP:
> -        regs->rcx = hvm_msr_tsc_aux(v);
> -        /* fall through */
>      case VMEXIT_RDTSC:
> -        svm_vmexit_do_rdtsc(regs);
> +        svm_vmexit_do_rdtsc(regs, exit_reason == VMEXIT_RDTSCP);
>          break;
> 
>      case VMEXIT_MONITOR:
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 365eeb2..a9f9b9b 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3589,6 +3589,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>      unsigned long exit_qualification, exit_reason, idtv_info, intr_info =
> 0;
>      unsigned int vector = 0, mode;
>      struct vcpu *v = current;
> +    struct domain *currd = v->domain;

... following the usual rules, you should now convert all uses of v->domain in this function to use currd.

> 
>      __vmread(GUEST_RIP,    &regs->rip);
>      __vmread(GUEST_RSP,    &regs->rsp);
> @@ -3956,6 +3957,13 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          vmx_invlpg_intercept(exit_qualification);
>          break;
>      case EXIT_REASON_RDTSCP:
> +        if ( !currd->arch.cpuid->extd.rdtscp &&
> +             currd->arch.tsc_mode != TSC_MODE_PVRDTSCP )
> +        {
> +            hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
> +            break;
> +        }
> +
>          regs->rcx = hvm_msr_tsc_aux(v);
>          /* fall through */
>      case EXIT_REASON_RDTSC:
> diff --git a/xen/include/asm-x86/hvm/svm/emulate.h b/xen/include/asm-
> x86/hvm/svm/emulate.h
> index 3de8236..ca92abb 100644
> --- a/xen/include/asm-x86/hvm/svm/emulate.h
> +++ b/xen/include/asm-x86/hvm/svm/emulate.h
> @@ -30,6 +30,7 @@ enum instruction_index {
>      INSTR_HLT,
>      INSTR_INT3,
>      INSTR_RDTSC,
> +    INSTR_RDTSCP,
>      INSTR_PAUSE,
>      INSTR_XSETBV,
>      INSTR_VMRUN,
> --
> 2.1.4

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86/svm: Improve diagnostics when __get_instruction_length_from_list() fails

[Jan Beulich]

>>> On 30.11.18 at 18:07, <andrew.cooper3@citrix.com> wrote:
> Also, I'm not entirely convinced that making modrm an annonymous union is
> going to work with older CentOS compilers,

It certainly won't.

> and therefore am not sure whether
> that part of the change is worth it.  The instruction in question can be
> obtained from the printed INSN_ constant alone.
> ---
>  xen/arch/x86/hvm/svm/emulate.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
> index 3d04af0..71a1b6e 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -56,11 +56,14 @@ static unsigned long svm_nextrip_insn_length(struct vcpu *v)
>  
>  static const struct {
>      unsigned int opcode;
> -    struct {
> -        unsigned int rm:3;
> -        unsigned int reg:3;
> -        unsigned int mod:2;
> -#define MODRM(mod, reg, rm) { rm, reg, mod }
> +    union {
> +        struct {
> +            unsigned int rm:3;
> +            unsigned int reg:3;
> +            unsigned int mod:2;
> +        };
> +        unsigned int raw;

Why unsigned int instead of uint8_t?

> @@ -152,8 +155,17 @@ int __get_instruction_length_from_list(struct vcpu *v,
>      }
>  
>      gdprintk(XENLOG_WARNING,
> -             "%s: Mismatch between expected and actual instruction: "
> -             "eip = %lx\n",  __func__, (unsigned long)vmcb->rip);
> +             "%s: Mismatch between expected and actual instruction:\n",
> +             __func__);
> +    gdprintk(XENLOG_WARNING,
> +             "  list[0] val %d, { opc %#x, modrm %#x }, list entries: %u\n",
> +             list[0], opc_tab[list[0]].opcode, opc_tab[list[0]].modrm.raw,
> +             list_count);
> +    gdprintk(XENLOG_WARNING, "  rip 0x%lx, nextrip 0x%lx, len %lu\n",
> +             vmcb->rip, vmcb->nextrip, vmcb->nextrip - vmcb->rip);
> +    hvm_dump_emulation_state(XENLOG_G_WARNING, "Insn_len",
> +                             &ctxt, X86EMUL_UNHANDLEABLE);
> +
>      hvm_inject_hw_exception(TRAP_gp_fault, 0);
>      return 0;
>  }

The gdprintk()s all expanding to nothing in release builds I'm
not fully convinced the added verbosity is worth it. In debug
builds adding some debugging code like this shouldn't be a
big hurdle.

In any event %#lx instead of 0x%lx please.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86/svm: Improve diagnostics when __get_instruction_length_from_list() fails

[Jan Beulich]

>>> On 03.12.18 at 10:10, <Paul.Durrant@citrix.com> wrote:
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 30 November 2018 17:07
>> 
>> RFC: __get_instruction_length_from_list() tries to cope with VMEXIT_IOIO,
>> but
>> IN/OUT instructions aren't in the decode list and I can't spot an entry
>> point
>> from the IOIO path.  Am I missing something?
> 
> Yes, odd. IOIO are handled in the ifdef NDEBUG blocks but an IOIO exit does 
> indeed not call into __get_instruction_length() but does the same calculation 
> inline. 

Indeed commit 147808b8b8 introduced this without there being
a visible or stated reason. Andre, any chance you can reconstruct
the reason from almost 9 years ago? The __get_instruction_length()
invocation the change added is VMX specific ...

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling

[Jan Beulich]

>>> On 30.11.18 at 18:07, <andrew.cooper3@citrix.com> wrote:
> For both VT-x and SVM, the RDTSCP intercept will trigger if the pipeline
> supports the instruction, but the guest may have not have rdtscp in its
> featureset.  Bring the vmexit handlers in line with the main emulator
> behaviour by optionally handing back #UD.
> 
> Next on the AMD side, if RDTSCP actually ends up being intercepted on a debug
> build, we first update regs->rcx, then call __get_instruction_length() asking
> for RDTSC.  As the two instructions are different (and indeed, different
> lengths!), __get_instruction_length_from_list() fails and hands back a #GP
> fault.
> 
> This can demonstrated by putting a guest into tsc_mode="always emulate" and
> executing an rdtscp instruction:
> 
>   (d1) --- Xen Test Framework ---
>   (d1) Environment: HVM 64bit (Long mode 4 levels)
>   (d1) Test rdtscp
>   (d1) TSC mode 1
>   (XEN) emulate.c:159:d1v0 __get_instruction_length_from_list: Mismatch between expected and actual instruction:
>   (XEN) emulate.c:163:d1v0   list[0] val 8, { opc 0xf0031, modrm 0 }, list entries: 1
>   (XEN) emulate.c:165:d1v0   rip 0x10475f, nextrip 0x104762, len 3
>   (XEN) Insn_len emulation failed (1): d1v0 64bit @ 0008:0010475f -> 0f 01 f9 5b 31 ff 31 c0 e9 c4 db ff ff 00 00 00
>   (d1) ******************************
>   (d1) PANIC: Unhandled exception at 0008:000000000010475f
>   (d1) Vec 13 #GP[0000]
>   (d1) ******************************
> 
> First, teach __get_instruction_length() to cope with RDTSCP, and improve
> svm_vmexit_do_rdtsc() to ask for the correct instruction.  Move the regs->rcx
> adjustment into this function to ensure it gets done after we are done
> potentially raising faults.
> 
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with or without Paul's additional remark addressed.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling

[Andrew Cooper]

On 03/12/2018 09:17, Paul Durrant wrote:
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 365eeb2..a9f9b9b 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3589,6 +3589,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>      unsigned long exit_qualification, exit_reason, idtv_info, intr_info =
>> 0;
>>      unsigned int vector = 0, mode;
>>      struct vcpu *v = current;
>> +    struct domain *currd = v->domain;
> ... following the usual rules, you should now convert all uses of v->domain in this function to use currd.

If this were new development work then perhaps (although it would take a
series to clean vmx_vmexit_handler() up to style).

In this case however, the patch needs backporting to all the stable
trees, at which point minimum perturbance is the most important aspect.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 03 December 2018 11:14
> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>;
> Kevin Tian <kevin.tian@intel.com>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>;
> Juergen Gross <jgross@suse.com>
> Subject: Re: [PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling
> 
> On 03/12/2018 09:17, Paul Durrant wrote:
> >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> >> index 365eeb2..a9f9b9b 100644
> >> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> @@ -3589,6 +3589,7 @@ void vmx_vmexit_handler(struct cpu_user_regs
> *regs)
> >>      unsigned long exit_qualification, exit_reason, idtv_info,
> intr_info =
> >> 0;
> >>      unsigned int vector = 0, mode;
> >>      struct vcpu *v = current;
> >> +    struct domain *currd = v->domain;
> > ... following the usual rules, you should now convert all uses of v-
> >domain in this function to use currd.
> 
> If this were new development work then perhaps (although it would take a
> series to clean vmx_vmexit_handler() up to style).
> 
> In this case however, the patch needs backporting to all the stable
> trees, at which point minimum perturbance is the most important aspect.

Fair enough.

  Paul

> 
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86/svm: Improve diagnostics when __get_instruction_length_from_list() fails

[Andrew Cooper]

On 03/12/2018 10:32, Jan Beulich wrote:
>>>> On 30.11.18 at 18:07, <andrew.cooper3@citrix.com> wrote:
>> Also, I'm not entirely convinced that making modrm an annonymous union is
>> going to work with older CentOS compilers,
> It certainly won't.
>
>> and therefore am not sure whether
>> that part of the change is worth it.  The instruction in question can be
>> obtained from the printed INSN_ constant alone.
>> ---
>>  xen/arch/x86/hvm/svm/emulate.c | 26 +++++++++++++++++++-------
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
>> index 3d04af0..71a1b6e 100644
>> --- a/xen/arch/x86/hvm/svm/emulate.c
>> +++ b/xen/arch/x86/hvm/svm/emulate.c
>> @@ -56,11 +56,14 @@ static unsigned long svm_nextrip_insn_length(struct vcpu *v)
>>  
>>  static const struct {
>>      unsigned int opcode;
>> -    struct {
>> -        unsigned int rm:3;
>> -        unsigned int reg:3;
>> -        unsigned int mod:2;
>> -#define MODRM(mod, reg, rm) { rm, reg, mod }
>> +    union {
>> +        struct {
>> +            unsigned int rm:3;
>> +            unsigned int reg:3;
>> +            unsigned int mod:2;
>> +        };
>> +        unsigned int raw;
> Why unsigned int instead of uint8_t?

Because to being with, this was a diagnostic patch and copied the type
above without thinking too much.

>
>> @@ -152,8 +155,17 @@ int __get_instruction_length_from_list(struct vcpu *v,
>>      }
>>  
>>      gdprintk(XENLOG_WARNING,
>> -             "%s: Mismatch between expected and actual instruction: "
>> -             "eip = %lx\n",  __func__, (unsigned long)vmcb->rip);
>> +             "%s: Mismatch between expected and actual instruction:\n",
>> +             __func__);
>> +    gdprintk(XENLOG_WARNING,
>> +             "  list[0] val %d, { opc %#x, modrm %#x }, list entries: %u\n",
>> +             list[0], opc_tab[list[0]].opcode, opc_tab[list[0]].modrm.raw,
>> +             list_count);
>> +    gdprintk(XENLOG_WARNING, "  rip 0x%lx, nextrip 0x%lx, len %lu\n",
>> +             vmcb->rip, vmcb->nextrip, vmcb->nextrip - vmcb->rip);
>> +    hvm_dump_emulation_state(XENLOG_G_WARNING, "Insn_len",
>> +                             &ctxt, X86EMUL_UNHANDLEABLE);
>> +
>>      hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>      return 0;
>>  }
> The gdprintk()s all expanding to nothing in release builds I'm
> not fully convinced the added verbosity is worth it. In debug
> builds adding some debugging code like this shouldn't be a
> big hurdle.

You and I know what diagnostics to put here, but I think Pauls reaction
to finding that message demonstrates that most others don't.  Nor should
they - the peculiarities of first-gen AMD hardware needn't be mandatory
knowledge for most contributors.

For these diagnostics, they are only reachable in debug builds, and this
codepath is only even reachable in release builds on first-gen hardware.

However, the difference between what is currently present and this is
enough information to actually diagnose the problem.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86/hvm: Corrections to RDTSCP intercept handling

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Saturday, December 1, 2018 1:07 AM
> 
> For both VT-x and SVM, the RDTSCP intercept will trigger if the pipeline
> supports the instruction, but the guest may have not have rdtscp in its
> featureset.  Bring the vmexit handlers in line with the main emulator
> behaviour by optionally handing back #UD.
> 
> Next on the AMD side, if RDTSCP actually ends up being intercepted on a
> debug
> build, we first update regs->rcx, then call __get_instruction_length() asking
> for RDTSC.  As the two instructions are different (and indeed, different
> lengths!), __get_instruction_length_from_list() fails and hands back a #GP
> fault.
> 
> This can demonstrated by putting a guest into tsc_mode="always emulate"
> and
> executing an rdtscp instruction:
> 
>   (d1) --- Xen Test Framework ---
>   (d1) Environment: HVM 64bit (Long mode 4 levels)
>   (d1) Test rdtscp
>   (d1) TSC mode 1
>   (XEN) emulate.c:159:d1v0 __get_instruction_length_from_list: Mismatch
> between expected and actual instruction:
>   (XEN) emulate.c:163:d1v0   list[0] val 8, { opc 0xf0031, modrm 0 }, list
> entries: 1
>   (XEN) emulate.c:165:d1v0   rip 0x10475f, nextrip 0x104762, len 3
>   (XEN) Insn_len emulation failed (1): d1v0 64bit @ 0008:0010475f -> 0f 01
> f9 5b 31 ff 31 c0 e9 c4 db ff ff 00 00 00
>   (d1) ******************************
>   (d1) PANIC: Unhandled exception at 0008:000000000010475f
>   (d1) Vec 13 #GP[0000]
>   (d1) ******************************
> 
> First, teach __get_instruction_length() to cope with RDTSCP, and improve
> svm_vmexit_do_rdtsc() to ask for the correct instruction.  Move the regs-
> >rcx
> adjustment into this function to ensure it gets done after we are done
> potentially raising faults.
> 
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel