[PATCH v2 0/3] Fixes to RDTSCP interception

[PATCH v2 0/3] Fixes to RDTSCP interception

[Andrew Cooper]

Updates in v2:
  * Rework svm_get_insn_len() to be more simple.
  * Drop anonymous union which will surely fail to compile on RHEL 6.x vintage
    compilers.
  * Rebase other changes

Andrew Cooper (3):
  x86/svm: Simplify svm_get_insn_len()
  x86/svm: Improve diagnostics when svm_get_insn_len() fails
  x86/hvm: Corrections to RDTSCP intercept handling

 xen/arch/x86/hvm/svm/emulate.c        | 79 ++++++++++++++++++-----------------
 xen/arch/x86/hvm/svm/nestedsvm.c      |  9 ++--
 xen/arch/x86/hvm/svm/svm.c            | 54 ++++++++++++++----------
 xen/arch/x86/hvm/vmx/vmx.c            |  8 ++++
 xen/include/asm-x86/hvm/svm/emulate.h | 10 +----
 5 files changed, 88 insertions(+), 72 deletions(-)

-- 
2.1.4


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

[PATCH v2 1/3] x86/svm: Simplify svm_get_insn_len()

[Andrew Cooper]

The existing __get_instruction_length_from_list() has a single user
which uses the list functionality.  That user however should be looking
specifically for INVD or WBINVD, as reported by the vmexit exit reason.

Modify svm_vmexit_do_invalidate_cache() to ask for the correct
instruction, and drop all list functionality from the helper.

Take the opportunity to rename it to svm_get_insn_len(), and drop the
IOIO length handling whch has never been used.

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: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>

v2:
 * New
---
 xen/arch/x86/hvm/svm/emulate.c        | 65 ++++++++++++++++-------------------
 xen/arch/x86/hvm/svm/nestedsvm.c      |  9 ++---
 xen/arch/x86/hvm/svm/svm.c            | 34 +++++++++---------
 xen/include/asm-x86/hvm/svm/emulate.h |  9 +----
 4 files changed, 51 insertions(+), 66 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index 3d04af0..3f695b9 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -83,13 +83,12 @@ static const struct {
     [INSTR_CPUID]   = { X86EMUL_OPC(0x0f, 0xa2) },
 };
 
-int __get_instruction_length_from_list(struct vcpu *v,
-        const enum instruction_index *list, unsigned int list_count)
+int svm_get_insn_len(struct vcpu *v, enum instruction_index insn)
 {
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
     struct hvm_emulate_ctxt ctxt;
     struct x86_emulate_state *state;
-    unsigned long inst_len, j;
+    unsigned long nrip_len, emul_len;
     unsigned int modrm_rm, modrm_reg;
     int modrm_mod;
 
@@ -98,13 +97,10 @@ int __get_instruction_length_from_list(struct vcpu *v,
      * hardware.
      */
 #ifdef NDEBUG
-    if ( (inst_len = svm_nextrip_insn_length(v)) > MAX_INST_LEN )
-        gprintk(XENLOG_WARNING, "NRip reported inst_len %lu\n", inst_len);
-    else if ( inst_len != 0 )
-        return inst_len;
-
-    if ( vmcb->exitcode == VMEXIT_IOIO )
-        return vmcb->exitinfo2 - vmcb->rip;
+    if ( (nrip_len = svm_nextrip_insn_length(v)) > MAX_INST_LEN )
+        gprintk(XENLOG_WARNING, "NRip reported inst_len %lu\n", nrip_len);
+    else if ( nrip_len != 0 )
+        return nrip_len;
 #endif
 
     ASSERT(v == current);
@@ -114,46 +110,43 @@ int __get_instruction_length_from_list(struct vcpu *v,
     if ( IS_ERR_OR_NULL(state) )
         return 0;
 
-    inst_len = x86_insn_length(state, &ctxt.ctxt);
+    emul_len = x86_insn_length(state, &ctxt.ctxt);
     modrm_mod = x86_insn_modrm(state, &modrm_rm, &modrm_reg);
     x86_emulate_free_state(state);
+
 #ifndef NDEBUG
-    if ( vmcb->exitcode == VMEXIT_IOIO )
-        j = vmcb->exitinfo2 - vmcb->rip;
-    else
-        j = svm_nextrip_insn_length(v);
-    if ( j && j != inst_len )
+    nrip_len = svm_nextrip_insn_length(v);
+    if ( nrip_len && nrip_len != emul_len )
     {
         gprintk(XENLOG_WARNING, "insn-len[%02x]=%lu (exp %lu)\n",
-                ctxt.ctxt.opcode, inst_len, j);
-        return j;
+                ctxt.ctxt.opcode, nrip_len, emul_len);
+        return nrip_len;
     }
 #endif
 
-    for ( j = 0; j < list_count; j++ )
+    if ( (unsigned int)insn >= ARRAY_SIZE(opc_tab) )
     {
-        unsigned int instr = list[j];
-
-        if ( instr >= ARRAY_SIZE(opc_tab) )
-        {
-            ASSERT_UNREACHABLE();
-            break;
-        }
-        if ( opc_tab[instr].opcode == ctxt.ctxt.opcode )
-        {
-            if ( !opc_tab[instr].modrm.mod )
-                return inst_len;
-
-            if ( modrm_mod == opc_tab[instr].modrm.mod &&
-                 (modrm_rm & 7) == opc_tab[instr].modrm.rm &&
-                 (modrm_reg & 7) == opc_tab[instr].modrm.reg )
-                return inst_len;
-        }
+        gdprintk(XENLOG_ERR, "insn %d out of range\n", insn);
+        ASSERT_UNREACHABLE();
+        goto out;
+    }
+
+    if ( opc_tab[insn].opcode == ctxt.ctxt.opcode )
+    {
+        if ( !opc_tab[insn].modrm.mod )
+            return emul_len;
+
+        if ( modrm_mod == opc_tab[insn].modrm.mod &&
+             (modrm_rm & 7) == opc_tab[insn].modrm.rm &&
+             (modrm_reg & 7) == opc_tab[insn].modrm.reg )
+            return emul_len;
     }
 
     gdprintk(XENLOG_WARNING,
              "%s: Mismatch between expected and actual instruction: "
              "eip = %lx\n",  __func__, (unsigned long)vmcb->rip);
+
+ out:
     hvm_inject_hw_exception(TRAP_gp_fault, 0);
     return 0;
 }
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 9660202..35c1a04 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -743,8 +743,9 @@ nsvm_vcpu_vmrun(struct vcpu *v, struct cpu_user_regs *regs)
     struct nestedvcpu *nv = &vcpu_nestedhvm(v);
     struct nestedsvm *svm = &vcpu_nestedsvm(v);
 
-    inst_len = __get_instruction_length(v, INSTR_VMRUN);
-    if (inst_len == 0) {
+    inst_len = svm_get_insn_len(v, INSTR_VMRUN);
+    if ( inst_len == 0 )
+    {
         svm->ns_vmexit.exitcode = VMEXIT_SHUTDOWN;
         return -1;
     }
@@ -1616,7 +1617,7 @@ void svm_vmexit_do_stgi(struct cpu_user_regs *regs, struct vcpu *v)
         return;
     }
 
-    if ( (inst_len = __get_instruction_length(v, INSTR_STGI)) == 0 )
+    if ( (inst_len = svm_get_insn_len(v, INSTR_STGI)) == 0 )
         return;
 
     nestedsvm_vcpu_stgi(v);
@@ -1637,7 +1638,7 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v)
         return;
     }
 
-    if ( (inst_len = __get_instruction_length(v, INSTR_CLGI)) == 0 )
+    if ( (inst_len = svm_get_insn_len(v, INSTR_CLGI)) == 0 )
         return;
 
     nestedsvm_vcpu_clgi(v);
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 40937bf..f8b7e8b 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2244,7 +2244,7 @@ static void svm_do_msr_access(struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
     bool rdmsr = curr->arch.hvm.svm.vmcb->exitinfo1 == 0;
-    int rc, inst_len = __get_instruction_length(
+    int rc, inst_len = svm_get_insn_len(
         curr, rdmsr ? INSTR_RDMSR : INSTR_WRMSR);
 
     if ( inst_len == 0 )
@@ -2272,7 +2272,7 @@ static void svm_vmexit_do_hlt(struct vmcb_struct *vmcb,
 {
     unsigned int inst_len;
 
-    if ( (inst_len = __get_instruction_length(current, INSTR_HLT)) == 0 )
+    if ( (inst_len = svm_get_insn_len(current, INSTR_HLT)) == 0 )
         return;
     __update_guest_eip(regs, inst_len);
 
@@ -2283,7 +2283,7 @@ static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs)
 {
     unsigned int inst_len;
 
-    if ( (inst_len = __get_instruction_length(current, INSTR_RDTSC)) == 0 )
+    if ( (inst_len = svm_get_insn_len(current, INSTR_RDTSC)) == 0 )
         return;
     __update_guest_eip(regs, inst_len);
 
@@ -2294,7 +2294,7 @@ static void svm_vmexit_do_pause(struct cpu_user_regs *regs)
 {
     unsigned int inst_len;
 
-    if ( (inst_len = __get_instruction_length(current, INSTR_PAUSE)) == 0 )
+    if ( (inst_len = svm_get_insn_len(current, INSTR_PAUSE)) == 0 )
         return;
     __update_guest_eip(regs, inst_len);
 
@@ -2361,7 +2361,7 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
     unsigned int inst_len;
     struct page_info *page;
 
-    if ( (inst_len = __get_instruction_length(v, INSTR_VMLOAD)) == 0 )
+    if ( (inst_len = svm_get_insn_len(v, INSTR_VMLOAD)) == 0 )
         return;
 
     if ( !nsvm_efer_svm_enabled(v) ) 
@@ -2396,7 +2396,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
     unsigned int inst_len;
     struct page_info *page;
 
-    if ( (inst_len = __get_instruction_length(v, INSTR_VMSAVE)) == 0 )
+    if ( (inst_len = svm_get_insn_len(v, INSTR_VMSAVE)) == 0 )
         return;
 
     if ( !nsvm_efer_svm_enabled(v) ) 
@@ -2464,13 +2464,11 @@ static void svm_wbinvd_intercept(void)
         flush_all(FLUSH_CACHE);
 }
 
-static void svm_vmexit_do_invalidate_cache(struct cpu_user_regs *regs)
+static void svm_vmexit_do_invalidate_cache(struct cpu_user_regs *regs,
+                                           bool invld)
 {
-    static const enum instruction_index list[] = { INSTR_INVD, INSTR_WBINVD };
-    int inst_len;
+    int inst_len = svm_get_insn_len(current, invld ? INSTR_INVD : INSTR_WBINVD);
 
-    inst_len = __get_instruction_length_from_list(
-        current, list, ARRAY_SIZE(list));
     if ( inst_len == 0 )
         return;
 
@@ -2745,7 +2743,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
             else
             {
                 trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
-                inst_len = __get_instruction_length(v, INSTR_ICEBP);
+                inst_len = svm_get_insn_len(v, INSTR_ICEBP);
             }
 
             rc = hvm_monitor_debug(regs->rip,
@@ -2762,7 +2760,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     case VMEXIT_EXCEPTION_BP:
-        inst_len = __get_instruction_length(v, INSTR_INT3);
+        inst_len = svm_get_insn_len(v, INSTR_INT3);
 
         if ( inst_len == 0 )
              break;
@@ -2853,7 +2851,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 
     case VMEXIT_INVD:
     case VMEXIT_WBINVD:
-        svm_vmexit_do_invalidate_cache(regs);
+        svm_vmexit_do_invalidate_cache(regs, exit_reason == VMEXIT_INVD);
         break;
 
     case VMEXIT_TASK_SWITCH: {
@@ -2882,7 +2880,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 
     case VMEXIT_CPUID:
     {
-        unsigned int inst_len = __get_instruction_length(v, INSTR_CPUID);
+        unsigned int inst_len = svm_get_insn_len(v, INSTR_CPUID);
         int rc = 0;
 
         if ( inst_len == 0 )
@@ -2938,14 +2936,14 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
             hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
             break;
         }
-        if ( (inst_len = __get_instruction_length(v, INSTR_INVLPGA)) == 0 )
+        if ( (inst_len = svm_get_insn_len(v, INSTR_INVLPGA)) == 0 )
             break;
         svm_invlpga_intercept(v, regs->rax, regs->ecx);
         __update_guest_eip(regs, inst_len);
         break;
 
     case VMEXIT_VMMCALL:
-        if ( (inst_len = __get_instruction_length(v, INSTR_VMCALL)) == 0 )
+        if ( (inst_len = svm_get_insn_len(v, INSTR_VMCALL)) == 0 )
             break;
         BUG_ON(vcpu_guestmode);
         HVMTRACE_1D(VMMCALL, regs->eax);
@@ -3001,7 +2999,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     case VMEXIT_XSETBV:
         if ( vmcb_get_cpl(vmcb) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
-        else if ( (inst_len = __get_instruction_length(v, INSTR_XSETBV)) &&
+        else if ( (inst_len = svm_get_insn_len(v, INSTR_XSETBV)) &&
                   hvm_handle_xsetbv(regs->ecx, msr_fold(regs)) == X86EMUL_OKAY )
             __update_guest_eip(regs, inst_len);
         break;
diff --git a/xen/include/asm-x86/hvm/svm/emulate.h b/xen/include/asm-x86/hvm/svm/emulate.h
index 3de8236..1d062d2 100644
--- a/xen/include/asm-x86/hvm/svm/emulate.h
+++ b/xen/include/asm-x86/hvm/svm/emulate.h
@@ -44,14 +44,7 @@ enum instruction_index {
 
 struct vcpu;
 
-int __get_instruction_length_from_list(
-    struct vcpu *, const enum instruction_index *, unsigned int list_count);
-
-static inline int __get_instruction_length(
-    struct vcpu *v, enum instruction_index instr)
-{
-    return __get_instruction_length_from_list(v, &instr, 1);
-}
+int svm_get_insn_len(struct vcpu *v, enum instruction_index instr);
 
 #endif /* __ASM_X86_HVM_SVM_EMULATE_H__ */
 
-- 
2.1.4


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

[PATCH v2 2/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails

[Andrew Cooper]

Sadly, a lone:

  (XEN) emulate.c:156:d2v0 svm_get_insn_len: 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>
---
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>

v2:
 * Drop anonymous union
 * Rebase
---
 xen/arch/x86/hvm/svm/emulate.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index 3f695b9..73cef5b 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -143,8 +143,17 @@ int svm_get_insn_len(struct vcpu *v, enum instruction_index insn)
     }
 
     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,
+             "  insn_index %d, opcode %#x modrm %#x\n",
+             insn, opc_tab[insn].opcode, ((opc_tab[insn].modrm.rm  << 6) |
+                                          (opc_tab[insn].modrm.reg << 3) |
+                                          (opc_tab[insn].modrm.mod)));
+    gdprintk(XENLOG_WARNING, "  rip %#lx, nextrip %#lx, len %lu\n",
+             vmcb->rip, vmcb->nextrip, vmcb->nextrip - vmcb->rip);
+    hvm_dump_emulation_state(XENLOG_G_WARNING, "SVM Insn len",
+                             &ctxt, X86EMUL_UNHANDLEABLE);
 
  out:
     hvm_inject_hw_exception(TRAP_gp_fault, 0);
-- 
2.1.4


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

[PATCH v2 3/3] 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:147:d1v0 svm_get_insn_len: Mismatch between expected and actual instruction:
  (XEN) emulate.c:152:d1v0   insn_index 8, opcode 0xf0031 modrm 0
  (XEN) emulate.c:154:d1v0   rip 0x10475f, nextrip 0x104762, len 3
  (XEN) SVM insn len emulation failed (1): d1v0 64bit @ 0008:0010475f -> 0f 01 f9 0f 31 5b 31 ff 31 c0 e9 c2 db ff ff 00
  (d1) ******************************
  (d1) PANIC: Unhandled exception at 0008:000000000010475f
  (d1) Vec 13 #GP[0000]
  (d1) ******************************

First, teach svm_get_insn_len() 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: Brian Woods <brian.woods@amd.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>

v2:
 * Rebase
---
 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 73cef5b..0778dc7 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -75,6 +75,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 f8b7e8b..d2c0d64 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 = svm_get_insn_len(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 = svm_get_insn_len(curr, insn)) == 0 )
+        return;
+
     __update_guest_eip(regs, inst_len);
 
+    if ( rdtscp )
+        regs->rcx = hvm_msr_tsc_aux(curr);
+
     hvm_rdtsc_intercept(regs);
 }
 
@@ -2966,10 +2980,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 7f77d1f..2166b0d 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 1d062d2..1f7a075 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 v2 2/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 13 December 2018 20:23
> 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 v2 2/3] x86/svm: Improve diagnostics when
> svm_get_insn_len() fails
> 
> Sadly, a lone:
> 
>   (XEN) emulate.c:156:d2v0 svm_get_insn_len: 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>

Reviewed-by: Paul Durrant <paul.durrant@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>
> 
> v2:
>  * Drop anonymous union
>  * Rebase
> ---
>  xen/arch/x86/hvm/svm/emulate.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/emulate.c
> b/xen/arch/x86/hvm/svm/emulate.c
> index 3f695b9..73cef5b 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -143,8 +143,17 @@ int svm_get_insn_len(struct vcpu *v, enum
> instruction_index insn)
>      }
> 
>      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,
> +             "  insn_index %d, opcode %#x modrm %#x\n",
> +             insn, opc_tab[insn].opcode, ((opc_tab[insn].modrm.rm  << 6)
> |
> +                                          (opc_tab[insn].modrm.reg << 3)
> |
> +                                          (opc_tab[insn].modrm.mod)));
> +    gdprintk(XENLOG_WARNING, "  rip %#lx, nextrip %#lx, len %lu\n",
> +             vmcb->rip, vmcb->nextrip, vmcb->nextrip - vmcb->rip);
> +    hvm_dump_emulation_state(XENLOG_G_WARNING, "SVM Insn len",
> +                             &ctxt, X86EMUL_UNHANDLEABLE);
> 
>   out:
>      hvm_inject_hw_exception(TRAP_gp_fault, 0);
> --
> 2.1.4

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

Re: [PATCH v2 1/3] x86/svm: Simplify svm_get_insn_len()

[Jan Beulich]

>>> On 13.12.18 at 21:22, <andrew.cooper3@citrix.com> wrote:
> @@ -98,13 +97,10 @@ int __get_instruction_length_from_list(struct vcpu *v,
>       * hardware.
>       */
>  #ifdef NDEBUG
> -    if ( (inst_len = svm_nextrip_insn_length(v)) > MAX_INST_LEN )
> -        gprintk(XENLOG_WARNING, "NRip reported inst_len %lu\n", inst_len);
> -    else if ( inst_len != 0 )
> -        return inst_len;
> -
> -    if ( vmcb->exitcode == VMEXIT_IOIO )
> -        return vmcb->exitinfo2 - vmcb->rip;
> +    if ( (nrip_len = svm_nextrip_insn_length(v)) > MAX_INST_LEN )

With this and you now using variables slightly differently, wouldn't
it be better to pull this out of the #ifdef to avoid ...

> @@ -114,46 +110,43 @@ int __get_instruction_length_from_list(struct vcpu *v,
>      if ( IS_ERR_OR_NULL(state) )
>          return 0;
>  
> -    inst_len = x86_insn_length(state, &ctxt.ctxt);
> +    emul_len = x86_insn_length(state, &ctxt.ctxt);
>      modrm_mod = x86_insn_modrm(state, &modrm_rm, &modrm_reg);
>      x86_emulate_free_state(state);
> +
>  #ifndef NDEBUG
> -    if ( vmcb->exitcode == VMEXIT_IOIO )
> -        j = vmcb->exitinfo2 - vmcb->rip;
> -    else
> -        j = svm_nextrip_insn_length(v);
> -    if ( j && j != inst_len )
> +    nrip_len = svm_nextrip_insn_length(v);

... duplication here? The potentially unnecessary call would
happen in debug builds only.

Preferably with these taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH v2 2/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails

[Jan Beulich]

>>> On 13.12.18 at 21:22, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -143,8 +143,17 @@ int svm_get_insn_len(struct vcpu *v, enum instruction_index insn)
>      }
>  
>      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__);

Is __func__ really useful in a gdprintk()?

> +    gdprintk(XENLOG_WARNING,
> +             "  insn_index %d, opcode %#x modrm %#x\n",
> +             insn, opc_tab[insn].opcode, ((opc_tab[insn].modrm.rm  << 6) |
> +                                          (opc_tab[insn].modrm.reg << 3) |
> +                                          (opc_tab[insn].modrm.mod)));

This is going to be misleading when bit 3 of modrm.reg or modrm.rm
are set.

> +    gdprintk(XENLOG_WARNING, "  rip %#lx, nextrip %#lx, len %lu\n",
> +             vmcb->rip, vmcb->nextrip, vmcb->nextrip - vmcb->rip);

If you find it necessary to log the length here in the first place, is
there any reason not to use nrip_len here?

Jan



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

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

[Roger Pau Monné]

On Thu, Dec 13, 2018 at 08:22:52PM +0000, Andrew 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
                                              ^ extra have
> 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:147:d1v0 svm_get_insn_len: Mismatch between expected and actual instruction:
>   (XEN) emulate.c:152:d1v0   insn_index 8, opcode 0xf0031 modrm 0
>   (XEN) emulate.c:154:d1v0   rip 0x10475f, nextrip 0x104762, len 3
>   (XEN) SVM insn len emulation failed (1): d1v0 64bit @ 0008:0010475f -> 0f 01 f9 0f 31 5b 31 ff 31 c0 e9 c2 db ff ff 00
>   (d1) ******************************
>   (d1) PANIC: Unhandled exception at 0008:000000000010475f
>   (d1) Vec 13 #GP[0000]
>   (d1) ******************************
> 
> First, teach svm_get_insn_len() 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: Brian Woods <brian.woods@amd.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 7f77d1f..2166b0d 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;

Given the usage of currd below I think this could be constified.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

Re: [PATCH v2 2/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails

[Andrew Cooper]

On 14/12/2018 10:16, Jan Beulich wrote:
>>>> On 13.12.18 at 21:22, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/emulate.c
>> +++ b/xen/arch/x86/hvm/svm/emulate.c
>> @@ -143,8 +143,17 @@ int svm_get_insn_len(struct vcpu *v, enum instruction_index insn)
>>      }
>>  
>>      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__);
> Is __func__ really useful in a gdprintk()?

Well - you are the author of that code...

>
>> +    gdprintk(XENLOG_WARNING,
>> +             "  insn_index %d, opcode %#x modrm %#x\n",
>> +             insn, opc_tab[insn].opcode, ((opc_tab[insn].modrm.rm  << 6) |
>> +                                          (opc_tab[insn].modrm.reg << 3) |
>> +                                          (opc_tab[insn].modrm.mod)));
> This is going to be misleading when bit 3 of modrm.reg or modrm.rm
> are set.

I'll switch to 0x%02x

>
>> +    gdprintk(XENLOG_WARNING, "  rip %#lx, nextrip %#lx, len %lu\n",
>> +             vmcb->rip, vmcb->nextrip, vmcb->nextrip - vmcb->rip);
> If you find it necessary to log the length here in the first place, is
> there any reason not to use nrip_len here?

No - a rebasing oversight.

~Andrew

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

Re: [PATCH v2 2/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails

[Jan Beulich]

>>> On 17.12.18 at 16:43, <andrew.cooper3@citrix.com> wrote:
> On 14/12/2018 10:16, Jan Beulich wrote:
>>>>> On 13.12.18 at 21:22, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/hvm/svm/emulate.c
>>> +++ b/xen/arch/x86/hvm/svm/emulate.c
>>> @@ -143,8 +143,17 @@ int svm_get_insn_len(struct vcpu *v, enum 
> instruction_index insn)
>>>      }
>>>  
>>>      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__);
>> Is __func__ really useful in a gdprintk()?
> 
> Well - you are the author of that code...

I've touched the neighboring line a while ago, but I didn't add __func__
(and I hardly ever do, due to not finding it helpful in the common case).

>>> +    gdprintk(XENLOG_WARNING,
>>> +             "  insn_index %d, opcode %#x modrm %#x\n",
>>> +             insn, opc_tab[insn].opcode, ((opc_tab[insn].modrm.rm  << 6) |
>>> +                                          (opc_tab[insn].modrm.reg << 3) |
>>> +                                          (opc_tab[insn].modrm.mod)));
>> This is going to be misleading when bit 3 of modrm.reg or modrm.rm
>> are set.
> 
> I'll switch to 0x%02x

How's that going to help? You'd need to print the fields individually.

Jan



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