[PATCH v3 1/3] x86/svm: Remove list functionality from __get_instruction_length_* infrastructure

[Andrew Cooper <andrew.cooper3@citrix.com>]

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>
Reviewed-by: 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
v3:
 * Deduplicate the calls to svm_nextrip_insn_length()
---
 xen/arch/x86/hvm/svm/emulate.c        | 76 +++++++++++++++++------------------
 xen/arch/x86/hvm/svm/nestedsvm.c      |  9 +++--
 xen/arch/x86/hvm/svm/svm.c            | 39 +++++++++---------
 xen/include/asm-x86/hvm/svm/emulate.h |  9 +----
 4 files changed, 61 insertions(+), 72 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index 4abeab8..7799908 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -84,28 +84,31 @@ 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)
+/*
+ * First-gen SVM didn't have the NextRIP feature, meaning that when we take a
+ * fault-style vmexit, we have to decode the instruction stream to calculate
+ * how many bytes to move %rip forwards by.
+ *
+ * To double check the implementation, in debug builds, always compare the
+ * hardware reported instruction length (if available) with the result from
+ * x86_decode_insn().
+ */
+unsigned 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;
 
-    /*
-     * In debug builds, always use x86_decode_insn() and compare with
-     * 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;
+    nrip_len = svm_nextrip_insn_length(v);
 
-    if ( vmcb->exitcode == VMEXIT_IOIO )
-        return vmcb->exitinfo2 - vmcb->rip;
+#ifdef NDEBUG
+    if ( nrip_len > 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);
@@ -115,41 +118,34 @@ 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 )
+    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 ( 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;
-        }
+        ASSERT_UNREACHABLE();
+        return 0;
+    }
+
+    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,
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 954822c..2584b90 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2244,8 +2244,8 @@ 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(
-        curr, rdmsr ? INSTR_RDMSR : INSTR_WRMSR);
+    int rc, inst_len = svm_get_insn_len(curr, rdmsr ? INSTR_RDMSR
+                                                    : INSTR_WRMSR);
 
     if ( inst_len == 0 )
         return;
@@ -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,6 @@ 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 ( rdtscp && !currd->arch.cpuid->extd.rdtscp )
@@ -2292,7 +2291,8 @@ static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs, bool rdtscp)
         return;
     }
 
-    if ( (inst_len = __get_instruction_length(curr, insn)) == 0 )
+    if ( (inst_len = svm_get_insn_len(curr, rdtscp ? INSTR_RDTSCP
+                                                   : INSTR_RDTSC)) == 0 )
         return;
 
     __update_guest_eip(regs, inst_len);
@@ -2307,7 +2307,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);
 
@@ -2374,7 +2374,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) ) 
@@ -2409,7 +2409,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) ) 
@@ -2477,13 +2477,12 @@ 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;
+    unsigned 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;
 
@@ -2758,7 +2757,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,
@@ -2775,7 +2774,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;
@@ -2866,7 +2865,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: {
@@ -2895,7 +2894,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 )
@@ -2951,14 +2950,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);
@@ -3012,7 +3011,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 ca92abb..82359ec 100644
--- a/xen/include/asm-x86/hvm/svm/emulate.h
+++ b/xen/include/asm-x86/hvm/svm/emulate.h
@@ -45,14 +45,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);
-}
+unsigned 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