[PATCH 0/8] Misc further emulation work

[PATCH 0/8] Misc further emulation work

[Andrew Cooper]

This series prevents the MSR path from raising faults behind the back of the
emulator.

Andrew Cooper (8):
  x86/shadow: Drop stale adjustment in the PAE second-half search
  x86/emul: Debugging improvements to the test harness
  x86/hvm: Assert some expectations in hvm_inject_event()
  x86/emul: Drop the last remaining uses of bool_t
  x86/hvm: Don't raise #GP behind the emulators back for MSR accesses
  x86/emul: Support speculative MSR reads
  x86/emul: Support CPUID fauilting via a speculative MSR read
  x86/emul: Implement the STAC and CLAC instructions

 tools/tests/x86_emulator/test_x86_emulator.c |  17 +++++
 tools/tests/x86_emulator/x86_emulate.c       |   2 -
 xen/arch/x86/hvm/emulate.c                   |  24 +++---
 xen/arch/x86/hvm/hvm.c                       |  17 ++++-
 xen/arch/x86/hvm/svm/svm.c                   |   4 +-
 xen/arch/x86/hvm/vmx/vmx.c                   |  32 +++++---
 xen/arch/x86/hvm/vmx/vvmx.c                  |  19 +++--
 xen/arch/x86/mm/shadow/multi.c               |   3 -
 xen/arch/x86/x86_emulate/x86_emulate.c       | 107 +++++++++++++++++----------
 xen/arch/x86/x86_emulate/x86_emulate.h       |  13 ++--
 xen/include/asm-x86/hvm/support.h            |  11 ++-
 11 files changed, 163 insertions(+), 86 deletions(-)

-- 
2.1.4


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

[PATCH 1/8] x86/shadow: Drop stale adjustment in the PAE second-half search

[Andrew Cooper]

This shouldn't have been present in c/s 29a57c992 "x86/emul: Rework emulator
event injection".  It was a leftover from a previous version of the series.

This conditional has no effect on the behaviour following it, as both
X86EMUL_EXCEPTION and X86EMUL_UNHANDLEABLE fall into the same "return back to
guest" path.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/mm/shadow/multi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 2696396..f494f7b 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3475,9 +3475,6 @@ static int sh_page_fault(struct vcpu *v,
             v->arch.paging.last_write_was_pt = 0;
             r = x86_emulate(&emul_ctxt.ctxt, emul_ops);
 
-            if ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending )
-                r = X86EMUL_UNHANDLEABLE;
-
             /*
              * Only continue the search for the second half if there are no
              * exceptions or pending actions.  Otherwise, give up and re-enter
-- 
2.1.4


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

[PATCH 2/8] x86/emul: Debugging improvements to the test harness

[Andrew Cooper]

Disable stdout buffering, so logging gets out even if the harness crashes.
Add a verbose option (compile time disabled) which dumps all read/write calls
the harness makes

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/tests/x86_emulator/test_x86_emulator.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index b54fd11..b5eca86 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -12,6 +12,8 @@
 #include "x86_emulate/x86_emulate.h"
 #include "blowfish.h"
 
+#define verbose false /* Switch to true for far more logging. */
+
 static const struct {
     const void *code;
     size_t size;
@@ -47,6 +49,9 @@ static int read(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    if ( verbose )
+        printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
+
     bytes_read += bytes;
     memcpy(p_data, (void *)offset, bytes);
     return X86EMUL_OKAY;
@@ -59,6 +64,9 @@ static int fetch(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    if ( verbose )
+        printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
+
     memcpy(p_data, (void *)offset, bytes);
     return X86EMUL_OKAY;
 }
@@ -70,6 +78,9 @@ static int write(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    if ( verbose )
+        printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
+
     memcpy((void *)offset, p_data, bytes);
     return X86EMUL_OKAY;
 }
@@ -82,6 +93,9 @@ static int cmpxchg(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    if ( verbose )
+        printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
+
     memcpy((void *)offset, new, bytes);
     return X86EMUL_OKAY;
 }
@@ -233,6 +247,9 @@ int main(int argc, char **argv)
     unsigned int bcdres_native, bcdres_emul;
 #endif
 
+    /* Disable output buffering. */
+    setbuf(stdout, NULL);
+
     ctxt.regs = &regs;
     ctxt.force_writeback = 0;
     ctxt.addr_size = 8 * sizeof(void *);
-- 
2.1.4


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

[PATCH 3/8] x86/hvm: Assert some expectations in hvm_inject_event()

[Andrew Cooper]

Check that event->error_code is appropriate for the type/vector combination.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hvm/hvm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e0f936b..ac207e4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1679,6 +1679,15 @@ void hvm_triple_fault(void)
 void hvm_inject_event(const struct x86_event *event)
 {
     struct vcpu *curr = current;
+    const uint8_t vector = event->vector;
+    const bool has_ec = ((event->type == X86_EVENTTYPE_HW_EXCEPTION) &&
+                         (vector < 32) && ((TRAP_HAVE_EC & 1u << vector)));
+
+    ASSERT(vector == event->vector); /* Confirm no truncation. */
+    if ( has_ec )
+        ASSERT(event->error_code != X86_EVENT_NO_EC);
+    else
+        ASSERT(event->error_code == X86_EVENT_NO_EC);
 
     if ( nestedhvm_enabled(curr->domain) &&
          !nestedhvm_vmswitch_in_progress(curr) &&
-- 
2.1.4


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

[PATCH 4/8] x86/emul: Drop the last remaining uses of bool_t

[Andrew Cooper]

And drop the compatibility typedef from the userspace harness

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/tests/x86_emulator/x86_emulate.c |  2 --
 xen/arch/x86/x86_emulate/x86_emulate.c | 43 ++++++++++++++++++----------------
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c
index 897b9ab..14a6fc2 100644
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate.c
@@ -8,8 +8,6 @@
 
 #include "x86_emulate/x86_emulate.h"
 
-typedef bool bool_t;
-
 #define is_canonical_address(x) (((int64_t)(x) >> 47) == ((int64_t)(x) >> 63))
 
 #define EFER_SCE       (1 << 0)
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 108ff8a..877023d 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -692,7 +692,7 @@ static inline int mkec(uint8_t e, int32_t ec, ...)
  * Given byte has even parity (even number of 1s)? SDM Vol. 1 Sec. 3.4.3.1,
  * "Status Flags": EFLAGS.PF reflects parity of least-sig. byte of result only.
  */
-static bool_t even_parity(uint8_t v)
+static bool even_parity(uint8_t v)
 {
 #ifdef __GCC_ASM_FLAG_OUTPUTS__
     asm ( "test %1,%1" : "=@ccp" (v) : "q" (v) );
@@ -997,9 +997,9 @@ static int read_ulong(
  * IN:  Multiplicand=m[0], Multiplier=m[1]
  * OUT: Return CF/OF (overflow status); Result=m[1]:m[0]
  */
-static bool_t mul_dbl(unsigned long m[2])
+static bool mul_dbl(unsigned long m[2])
 {
-    bool_t rc;
+    bool rc;
 
 #ifdef __GCC_ASM_FLAG_OUTPUTS__
     asm ( "mul %1" : "+a" (m[0]), "+d" (m[1]), "=@cco" (rc) );
@@ -1016,9 +1016,9 @@ static bool_t mul_dbl(unsigned long m[2])
  * IN:  Multiplicand=m[0], Multiplier=m[1]
  * OUT: Return CF/OF (overflow status); Result=m[1]:m[0]
  */
-static bool_t imul_dbl(unsigned long m[2])
+static bool imul_dbl(unsigned long m[2])
 {
-    bool_t rc;
+    bool rc;
 
 #ifdef __GCC_ASM_FLAG_OUTPUTS__
     asm ( "imul %1" : "+a" (m[0]), "+d" (m[1]), "=@cco" (rc) );
@@ -1036,7 +1036,7 @@ static bool_t imul_dbl(unsigned long m[2])
  * OUT: Return 1: #DE
  *      Return 0: Quotient=u[0], Remainder=u[1]
  */
-static bool_t div_dbl(unsigned long u[2], unsigned long v)
+static bool div_dbl(unsigned long u[2], unsigned long v)
 {
     if ( (v == 0) || (u[1] >= v) )
         return 1;
@@ -1052,9 +1052,9 @@ static bool_t div_dbl(unsigned long u[2], unsigned long v)
  * NB. We don't use idiv directly as it's moderately hard to work out
  *     ahead of time whether it will #DE, which we cannot allow to happen.
  */
-static bool_t idiv_dbl(unsigned long u[2], long v)
+static bool idiv_dbl(unsigned long u[2], long v)
 {
-    bool_t negu = (long)u[1] < 0, negv = v < 0;
+    bool negu = (long)u[1] < 0, negv = v < 0;
 
     /* u = abs(u) */
     if ( negu )
@@ -1086,7 +1086,7 @@ static bool_t idiv_dbl(unsigned long u[2], long v)
     return 0;
 }
 
-static bool_t
+static bool
 test_cc(
     unsigned int condition, unsigned int flags)
 {
@@ -1218,7 +1218,7 @@ static int ioport_access_check(
     return rc;
 }
 
-static bool_t
+static bool
 in_realmode(
     struct x86_emulate_ctxt *ctxt,
     const struct x86_emulate_ops  *ops)
@@ -1233,7 +1233,7 @@ in_realmode(
     return (!rc && !(cr0 & CR0_PE));
 }
 
-static bool_t
+static bool
 in_protmode(
     struct x86_emulate_ctxt *ctxt,
     const struct x86_emulate_ops  *ops)
@@ -1246,7 +1246,7 @@ in_protmode(
 #define EDX 2
 #define EBX 3
 
-static bool_t vcpu_has(
+static bool vcpu_has(
     unsigned int eax,
     unsigned int reg,
     unsigned int bit,
@@ -1350,7 +1350,7 @@ realmode_load_seg(
 static int
 protmode_load_seg(
     enum x86_segment seg,
-    uint16_t sel, bool_t is_ret,
+    uint16_t sel, bool is_ret,
     struct segment_register *sreg,
     struct x86_emulate_ctxt *ctxt,
     const struct x86_emulate_ops *ops)
@@ -1527,7 +1527,7 @@ protmode_load_seg(
 static int
 load_seg(
     enum x86_segment seg,
-    uint16_t sel, bool_t is_ret,
+    uint16_t sel, bool is_ret,
     struct segment_register *sreg,
     struct x86_emulate_ctxt *ctxt,
     const struct x86_emulate_ops *ops)
@@ -4864,9 +4864,10 @@ x86_emulate(
         break;
     }
 
-    case X86EMUL_OPC(0x0f, 0x35): /* sysexit */ {
+    case X86EMUL_OPC(0x0f, 0x35): /* sysexit */
+    {
         uint64_t msr_content;
-        bool_t user64 = !!(rex_prefix & REX_W);
+        bool user64 = rex_prefix & REX_W;
 
         generate_exception_if(!mode_ring0(), EXC_GP, 0);
         generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
@@ -5191,8 +5192,9 @@ x86_emulate(
         emulate_2op_SrcV_nobyte("btc", src, dst, _regs.eflags);
         break;
 
-    case X86EMUL_OPC(0x0f, 0xbc): /* bsf or tzcnt */ {
-        bool_t zf;
+    case X86EMUL_OPC(0x0f, 0xbc): /* bsf or tzcnt */
+    {
+        bool zf;
 
 #ifdef __GCC_ASM_FLAG_OUTPUTS__
         asm ( "bsf %2,%0"
@@ -5223,8 +5225,9 @@ x86_emulate(
         break;
     }
 
-    case X86EMUL_OPC(0x0f, 0xbd): /* bsr or lzcnt */ {
-        bool_t zf;
+    case X86EMUL_OPC(0x0f, 0xbd): /* bsr or lzcnt */
+    {
+        bool zf;
 
 #ifdef __GCC_ASM_FLAG_OUTPUTS__
         asm ( "bsr %2,%0"
-- 
2.1.4


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

[PATCH 5/8] x86/hvm: Don't raise #GP behind the emulators back for MSR accesses

[Andrew Cooper]

The current hvm_msr_{read,write}_intercept() infrastructure calls
hvm_inject_hw_exception() directly to latch a fault, and returns
X86EMUL_EXCEPTION to its caller.

This behaviour is problematic for the hvmemul_{read,write}_msr() paths, as the
fault is raised behind the back of the x86 emulator.

Alter the behaviour so hvm_msr_{read,write}_intercept() simply returns
X86EMUL_EXCEPTION, leaving the callers to actually inject the #GP fault.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.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>
---
 xen/arch/x86/hvm/emulate.c        | 14 ++++++++++++--
 xen/arch/x86/hvm/hvm.c            |  8 +++++---
 xen/arch/x86/hvm/svm/svm.c        |  4 ++--
 xen/arch/x86/hvm/vmx/vmx.c        | 32 +++++++++++++++++++++-----------
 xen/arch/x86/hvm/vmx/vvmx.c       | 19 ++++++++++++++-----
 xen/include/asm-x86/hvm/support.h | 11 ++++++++---
 6 files changed, 62 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index d0a043b..b182d57 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1531,7 +1531,12 @@ static int hvmemul_read_msr(
     uint64_t *val,
     struct x86_emulate_ctxt *ctxt)
 {
-    return hvm_msr_read_intercept(reg, val);
+    int rc = hvm_msr_read_intercept(reg, val);
+
+    if ( rc == X86EMUL_EXCEPTION )
+        x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
+
+    return rc;
 }
 
 static int hvmemul_write_msr(
@@ -1539,7 +1544,12 @@ static int hvmemul_write_msr(
     uint64_t val,
     struct x86_emulate_ctxt *ctxt)
 {
-    return hvm_msr_write_intercept(reg, val, 1);
+    int rc = hvm_msr_write_intercept(reg, val, 1);
+
+    if ( rc == X86EMUL_EXCEPTION )
+        x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
+
+    return rc;
 }
 
 static int hvmemul_wbinvd(
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ac207e4..863adfc 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v)
 
         if ( w->do_write.msr )
         {
-            hvm_msr_write_intercept(w->msr, w->value, 0);
+            int rc = hvm_msr_write_intercept(w->msr, w->value, 0);
+
+            if ( rc == X86EMUL_EXCEPTION )
+                hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
             w->do_write.msr = 0;
         }
 
@@ -3896,7 +3900,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     return ret;
 
  gp_fault:
-    hvm_inject_hw_exception(TRAP_gp_fault, 0);
     ret = X86EMUL_EXCEPTION;
     *msr_content = -1ull;
     goto out;
@@ -4054,7 +4057,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
     return ret;
 
 gp_fault:
-    hvm_inject_hw_exception(TRAP_gp_fault, 0);
     return X86EMUL_EXCEPTION;
 }
 
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 1588b2f..810b0d4 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1788,7 +1788,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     return X86EMUL_OKAY;
 
  gpf:
-    hvm_inject_hw_exception(TRAP_gp_fault, 0);
     return X86EMUL_EXCEPTION;
 }
 
@@ -1945,7 +1944,6 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
     return result;
 
  gpf:
-    hvm_inject_hw_exception(TRAP_gp_fault, 0);
     return X86EMUL_EXCEPTION;
 }
 
@@ -1976,6 +1974,8 @@ static void svm_do_msr_access(struct cpu_user_regs *regs)
 
     if ( rc == X86EMUL_OKAY )
         __update_guest_eip(regs, inst_len);
+    else if ( rc == X86EMUL_EXCEPTION )
+        hvm_inject_hw_exception(TRAP_gp_fault, 0);
 }
 
 static void svm_vmexit_do_hlt(struct vmcb_struct *vmcb,
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index afde634..ddfb410 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2691,7 +2691,6 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     return X86EMUL_OKAY;
 
 gp_fault:
-    hvm_inject_hw_exception(TRAP_gp_fault, 0);
     return X86EMUL_EXCEPTION;
 }
 
@@ -2920,7 +2919,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
     return X86EMUL_OKAY;
 
 gp_fault:
-    hvm_inject_hw_exception(TRAP_gp_fault, 0);
     return X86EMUL_EXCEPTION;
 }
 
@@ -3632,23 +3630,35 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         break;
     case EXIT_REASON_MSR_READ:
     {
-        uint64_t msr_content;
-        if ( hvm_msr_read_intercept(regs->ecx, &msr_content) == X86EMUL_OKAY )
+        uint64_t msr_content = 0;
+
+        switch ( hvm_msr_read_intercept(regs->_ecx, &msr_content) )
         {
-            regs->eax = (uint32_t)msr_content;
-            regs->edx = (uint32_t)(msr_content >> 32);
+        case X86EMUL_OKAY:
+            regs->rax = (uint32_t)msr_content;
+            regs->rdx = (uint32_t)(msr_content >> 32);
             update_guest_eip(); /* Safe: RDMSR */
+            break;
+
+        case X86EMUL_EXCEPTION:
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+            break;
         }
         break;
     }
     case EXIT_REASON_MSR_WRITE:
-    {
-        uint64_t msr_content;
-        msr_content = ((uint64_t)regs->edx << 32) | (uint32_t)regs->eax;
-        if ( hvm_msr_write_intercept(regs->ecx, msr_content, 1) == X86EMUL_OKAY )
+        switch ( hvm_msr_write_intercept(
+                     regs->_ecx, (regs->rdx << 32) | regs->_eax, 1) )
+        {
+        case X86EMUL_OKAY:
             update_guest_eip(); /* Safe: WRMSR */
+            break;
+
+        case X86EMUL_EXCEPTION:
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+            break;
+        }
         break;
-    }
 
     case EXIT_REASON_VMXOFF:
         if ( nvmx_handle_vmxoff(regs) == X86EMUL_OKAY )
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index e6e9ebd..87f02ef 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1000,6 +1000,7 @@ static void load_shadow_guest_state(struct vcpu *v)
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
     u32 control;
     u64 cr_gh_mask, cr_read_shadow;
+    int rc;
 
     static const u16 vmentry_fields[] = {
         VM_ENTRY_INTR_INFO,
@@ -1021,8 +1022,12 @@ static void load_shadow_guest_state(struct vcpu *v)
     if ( control & VM_ENTRY_LOAD_GUEST_PAT )
         hvm_set_guest_pat(v, get_vvmcs(v, GUEST_PAT));
     if ( control & VM_ENTRY_LOAD_PERF_GLOBAL_CTRL )
-        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
-                                get_vvmcs(v, GUEST_PERF_GLOBAL_CTRL), 0);
+    {
+        rc = hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
+                                     get_vvmcs(v, GUEST_PERF_GLOBAL_CTRL), 0);
+        if ( rc == X86EMUL_EXCEPTION )
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+    }
 
     hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
 
@@ -1193,7 +1198,7 @@ static void sync_vvmcs_ro(struct vcpu *v)
 
 static void load_vvmcs_host_state(struct vcpu *v)
 {
-    int i;
+    int i, rc;
     u64 r;
     u32 control;
 
@@ -1211,8 +1216,12 @@ static void load_vvmcs_host_state(struct vcpu *v)
     if ( control & VM_EXIT_LOAD_HOST_PAT )
         hvm_set_guest_pat(v, get_vvmcs(v, HOST_PAT));
     if ( control & VM_EXIT_LOAD_PERF_GLOBAL_CTRL )
-        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
-                                get_vvmcs(v, HOST_PERF_GLOBAL_CTRL), 1);
+    {
+        rc = hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
+                                     get_vvmcs(v, HOST_PERF_GLOBAL_CTRL), 1);
+        if ( rc == X86EMUL_EXCEPTION )
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+    }
 
     hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
 
diff --git a/xen/include/asm-x86/hvm/support.h b/xen/include/asm-x86/hvm/support.h
index 3d767d7..2bff1f4 100644
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -122,13 +122,18 @@ int hvm_set_efer(uint64_t value);
 int hvm_set_cr0(unsigned long value, bool_t may_defer);
 int hvm_set_cr3(unsigned long value, bool_t may_defer);
 int hvm_set_cr4(unsigned long value, bool_t may_defer);
-int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
-int hvm_msr_write_intercept(
-    unsigned int msr, uint64_t msr_content, bool_t may_defer);
 int hvm_mov_to_cr(unsigned int cr, unsigned int gpr);
 int hvm_mov_from_cr(unsigned int cr, unsigned int gpr);
 void hvm_ud_intercept(struct cpu_user_regs *);
 
+/*
+ * May return X86EMUL_EXCEPTION, at which point the caller is responsible for
+ * injecting a #GP fault.  Used to support speculative reads.
+ */
+int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
+int hvm_msr_write_intercept(
+    unsigned int msr, uint64_t msr_content, bool_t may_defer);
+
 #endif /* __ASM_X86_HVM_SUPPORT_H__ */
 
 /*
-- 
2.1.4


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

[PATCH 6/8] x86/emul: Support speculative MSR reads

[Andrew Cooper]

Update the read_msr() hook to take an additional parameter, indicating that
there should be no side effects of the read.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
---
 xen/arch/x86/hvm/emulate.c             |  3 ++-
 xen/arch/x86/x86_emulate/x86_emulate.c | 24 ++++++++++++------------
 xen/arch/x86/x86_emulate/x86_emulate.h |  6 +++++-
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index b182d57..bce0b00 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1529,11 +1529,12 @@ static int hvmemul_write_cr(
 static int hvmemul_read_msr(
     unsigned int reg,
     uint64_t *val,
+    bool speculative,
     struct x86_emulate_ctxt *ctxt)
 {
     int rc = hvm_msr_read_intercept(reg, val);
 
-    if ( rc == X86EMUL_EXCEPTION )
+    if ( rc == X86EMUL_EXCEPTION && !speculative )
         x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
 
     return rc;
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 877023d..5cba7ec 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1319,7 +1319,7 @@ in_longmode(
     uint64_t efer;
 
     if ( !ops->read_msr ||
-         unlikely(ops->read_msr(MSR_EFER, &efer, ctxt) != X86EMUL_OKAY) )
+         unlikely(ops->read_msr(MSR_EFER, &efer, false, ctxt) != X86EMUL_OKAY) )
         return -1;
 
     return !!(efer & EFER_LMA);
@@ -4412,7 +4412,7 @@ x86_emulate(
         {
             uint64_t tsc_aux;
             fail_if(ops->read_msr == NULL);
-            if ( (rc = ops->read_msr(MSR_TSC_AUX, &tsc_aux, ctxt)) != 0 )
+            if ( (rc = ops->read_msr(MSR_TSC_AUX, &tsc_aux, false, ctxt)) != 0 )
                 goto done;
             _regs.ecx = (uint32_t)tsc_aux;
             goto rdtsc;
@@ -4548,11 +4548,11 @@ x86_emulate(
 
         /* Inject #UD if syscall/sysret are disabled. */
         fail_if(ops->read_msr == NULL);
-        if ( (rc = ops->read_msr(MSR_EFER, &msr_content, ctxt)) != 0 )
+        if ( (rc = ops->read_msr(MSR_EFER, &msr_content, false, ctxt)) != 0 )
             goto done;
         generate_exception_if((msr_content & EFER_SCE) == 0, EXC_UD);
 
-        if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
+        if ( (rc = ops->read_msr(MSR_STAR, &msr_content, false, ctxt)) != 0 )
             goto done;
 
         cs.sel = (msr_content >> 32) & ~3; /* SELECTOR_RPL_MASK */
@@ -4574,11 +4574,11 @@ x86_emulate(
             _regs.r11 = _regs.eflags & ~EFLG_RF;
 
             if ( (rc = ops->read_msr(mode_64bit() ? MSR_LSTAR : MSR_CSTAR,
-                                     &msr_content, ctxt)) != 0 )
+                                     &msr_content, false, ctxt)) != 0 )
                 goto done;
             _regs.rip = msr_content;
 
-            if ( (rc = ops->read_msr(MSR_FMASK, &msr_content, ctxt)) != 0 )
+            if ( (rc = ops->read_msr(MSR_FMASK, &msr_content, false, ctxt)) != 0 )
                 goto done;
             _regs.eflags &= ~(msr_content | EFLG_RF);
         }
@@ -4793,7 +4793,7 @@ x86_emulate(
             generate_exception_if(cr4 & CR4_TSD, EXC_GP, 0);
         }
         fail_if(ops->read_msr == NULL);
-        if ( (rc = ops->read_msr(MSR_TSC, &val, ctxt)) != 0 )
+        if ( (rc = ops->read_msr(MSR_TSC, &val, false, ctxt)) != 0 )
             goto done;
         _regs.edx = (uint32_t)(val >> 32);
         _regs.eax = (uint32_t)(val >>  0);
@@ -4804,7 +4804,7 @@ x86_emulate(
         uint64_t val;
         generate_exception_if(!mode_ring0(), EXC_GP, 0);
         fail_if(ops->read_msr == NULL);
-        if ( (rc = ops->read_msr((uint32_t)_regs.ecx, &val, ctxt)) != 0 )
+        if ( (rc = ops->read_msr((uint32_t)_regs.ecx, &val, false, ctxt)) != 0 )
             goto done;
         _regs.edx = (uint32_t)(val >> 32);
         _regs.eax = (uint32_t)(val >>  0);
@@ -4825,7 +4825,7 @@ x86_emulate(
         generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
 
         fail_if(ops->read_msr == NULL);
-        if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) != 0 )
+        if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, false, ctxt)) != 0 )
             goto done;
 
         generate_exception_if(!(msr_content & 0xfffc), EXC_GP, 0);
@@ -4853,11 +4853,11 @@ x86_emulate(
              (rc = ops->write_segment(x86_seg_ss, &sreg, ctxt)) != 0 )
             goto done;
 
-        if ( (rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt)) != 0 )
+        if ( (rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, false, ctxt)) != 0 )
             goto done;
         _regs.eip = lm ? msr_content : (uint32_t)msr_content;
 
-        if ( (rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt)) != 0 )
+        if ( (rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, false, ctxt)) != 0 )
             goto done;
         _regs.esp = lm ? msr_content : (uint32_t)msr_content;
 
@@ -4873,7 +4873,7 @@ x86_emulate(
         generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
 
         fail_if(ops->read_msr == NULL);
-        if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) != 0 )
+        if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, false, ctxt)) != 0 )
             goto done;
 
         generate_exception_if(!(msr_content & 0xfffc), EXC_GP, 0);
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 164fc24..89cf20d 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -372,15 +372,19 @@ struct x86_emulate_ops
     /*
      * read_msr: Read from model-specific register.
      *  @reg:   [IN ] Register to read.
+     *  @val:   [OUT] Value read (only valid on X86EMUL_OKAY)
+     *  @speculative [IN] Speculative read?
      */
     int (*read_msr)(
         unsigned int reg,
         uint64_t *val,
+        bool speculative,
         struct x86_emulate_ctxt *ctxt);
 
     /*
-     * write_dr: Write to model-specific register.
+     * write_msr: Write to model-specific register.
      *  @reg:   [IN ] Register to write.
+     *  @val:   [IN ] Value to write.
      */
     int (*write_msr)(
         unsigned int reg,
-- 
2.1.4


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

[PATCH 7/8] x86/emul: Support CPUID fauilting via a speculative MSR read

[Andrew Cooper]

Use the new speculative read support to query MSR_INTEL_MISC_FEATURES for
active CPUID faulting, without raising #GP as a side effect.

This removes the need for every cpuid() emulation hook to individually support
CPUID faulting.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>

Jan: Probably worth waiting to rebase over your changes moving the
architectrual defines elsewhere
---
 xen/arch/x86/hvm/emulate.c             |  9 ---------
 xen/arch/x86/x86_emulate/x86_emulate.c | 16 +++++++++++++---
 xen/arch/x86/x86_emulate/x86_emulate.h |  7 +------
 3 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index bce0b00..1a132e7 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1567,15 +1567,6 @@ int hvmemul_cpuid(
     unsigned int *edx,
     struct x86_emulate_ctxt *ctxt)
 {
-    /*
-     * x86_emulate uses this function to query CPU features for its own internal
-     * use. Make sure we're actually emulating CPUID before emulating CPUID
-     * faulting.
-     */
-    if ( ctxt->opcode == X86EMUL_OPC(0x0f, 0xa2) &&
-         hvm_check_cpuid_faulting(current) )
-        return X86EMUL_EXCEPTION;
-
     hvm_funcs.cpuid_intercept(eax, ebx, ecx, edx);
     return X86EMUL_OKAY;
 }
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 5cba7ec..67495eb 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -406,6 +406,8 @@ typedef union {
 
 /* MSRs. */
 #define MSR_TSC          0x00000010
+#define MSR_INTEL_MISC_FEATURES 0x00000140
+#define MISC_FEATURES_CPUID_FAULTING (1 << 0)
 #define MSR_SYSENTER_CS  0x00000174
 #define MSR_SYSENTER_ESP 0x00000175
 #define MSR_SYSENTER_EIP 0x00000176
@@ -5044,13 +5046,21 @@ x86_emulate(
         src.val = x86_seg_fs;
         goto pop_seg;
 
-    case X86EMUL_OPC(0x0f, 0xa2): /* cpuid */ {
+    case X86EMUL_OPC(0x0f, 0xa2): /* cpuid */
+    {
         unsigned int eax = _regs.eax, ebx = _regs.ebx;
         unsigned int ecx = _regs.ecx, edx = _regs.edx;
+        uint64_t misc_features;
+
         fail_if(ops->cpuid == NULL);
+        generate_exception_if(
+            ops->read_msr &&
+            ops->read_msr(MSR_INTEL_MISC_FEATURES,
+                          &misc_features, true, ctxt) == X86EMUL_OKAY &&
+            (misc_features & MISC_FEATURES_CPUID_FAULTING) &&
+            !mode_ring0(), EXC_GP, 0); /* CPUID Faulting? */
+
         rc = ops->cpuid(&eax, &ebx, &ecx, &edx, ctxt);
-        generate_exception_if(rc == X86EMUL_EXCEPTION,
-                              EXC_GP, 0); /* CPUID Faulting? */
         if ( rc != X86EMUL_OKAY )
             goto done;
         _regs.eax = eax; _regs.ebx = ebx;
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 89cf20d..46c863f 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -395,12 +395,7 @@ struct x86_emulate_ops
     int (*wbinvd)(
         struct x86_emulate_ctxt *ctxt);
 
-    /*
-     * cpuid: Emulate CPUID via given set of EAX-EDX inputs/outputs.
-     *
-     * May return X86EMUL_EXCEPTION, which causes the emulator to inject
-     * #GP[0].  Used to implement CPUID faulting.
-     */
+    /* cpuid: Emulate CPUID via given set of EAX-EDX inputs/outputs. */
     int (*cpuid)(
         unsigned int *eax,
         unsigned int *ebx,
-- 
2.1.4


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

[PATCH 8/8] x86/emul: Implement the STAC and CLAC instructions

[Andrew Cooper]

Note that unlike most privilege restricted instructions, STAC and CLAC are
documented to raise #UD rather than #GP[0], and indeed do so.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 67495eb..111bb91 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -429,6 +429,7 @@ typedef union {
 #define CR4_OSXMMEXCPT (1<<10)
 #define CR4_UMIP       (1<<11)
 #define CR4_OSXSAVE    (1<<18)
+#define CR4_SMAP       (1<<21)
 
 /* EFLAGS bit definitions. */
 #define EFLG_VIP  (1<<20)
@@ -4362,11 +4363,27 @@ x86_emulate(
 
         switch( modrm )
         {
-#ifdef __XEN__
-        case 0xd1: /* xsetbv */
-        {
             unsigned long cr4;
 
+        case 0xca: /* clac */
+        case 0xcb: /* stac */
+            generate_exception_if(
+                lock_prefix || (_regs.eflags & EFLG_VM), EXC_UD);
+            if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
+                cr4 = 0;
+            /*
+             * Contrary to expectation (i.e. #GP[0]), #UD for the CPL check is
+             * the documented and observed behaviour.
+             */
+            generate_exception_if(!(cr4 & CR4_SMAP) || !mode_ring0(), EXC_UD);
+
+            _regs.eflags &= ~EFLG_AC;
+            if ( modrm == 0xcb )
+                _regs.eflags |= EFLG_AC;
+            goto no_writeback;
+
+#ifdef __XEN__
+        case 0xd1: /* xsetbv */
             generate_exception_if(vex.pfx, EXC_UD);
             if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
                 cr4 = 0;
@@ -4376,7 +4393,6 @@ x86_emulate(
                                                 _regs._eax | (_regs.rdx << 32)),
                                   EXC_GP, 0);
             goto no_writeback;
-        }
 #endif
 
         case 0xd4: /* vmfunc */
-- 
2.1.4


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

Re: [PATCH 1/8] x86/shadow: Drop stale adjustment in the PAE second-half search

[Tim Deegan]

At 10:09 +0000 on 05 Dec (1480932564), Andrew Cooper wrote:
> This shouldn't have been present in c/s 29a57c992 "x86/emul: Rework emulator
> event injection".  It was a leftover from a previous version of the series.

IMO this hunk was correct in 29a57c992, but it's fine to remove it now.

Acked-by: Tim Deegan <tim@xen.org>

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

Re: [PATCH 2/8] x86/emul: Debugging improvements to the test harness

[Jan Beulich]

>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote:
> @@ -47,6 +49,9 @@ static int read(
>      unsigned int bytes,
>      struct x86_emulate_ctxt *ctxt)
>  {
> +    if ( verbose )
> +        printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);

There appear to be two stray commas here, or do they serve a
purpose?

Other than that
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 3/8] x86/hvm: Assert some expectations in hvm_inject_event()

[Jan Beulich]

>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1679,6 +1679,15 @@ void hvm_triple_fault(void)
>  void hvm_inject_event(const struct x86_event *event)
>  {
>      struct vcpu *curr = current;
> +    const uint8_t vector = event->vector;
> +    const bool has_ec = ((event->type == X86_EVENTTYPE_HW_EXCEPTION) &&
> +                         (vector < 32) && ((TRAP_HAVE_EC & 1u << vector)));

With the operands of << parenthesized
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 4/8] x86/emul: Drop the last remaining uses of bool_t

[Jan Beulich]

>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote:
> And drop the compatibility typedef from the userspace harness
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 5/8] x86/hvm: Don't raise #GP behind the emulators back for MSR accesses

[Jan Beulich]

>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v)
>  
>          if ( w->do_write.msr )
>          {
> -            hvm_msr_write_intercept(w->msr, w->value, 0);
> +            int rc = hvm_msr_write_intercept(w->msr, w->value, 0);
> +
> +            if ( rc == X86EMUL_EXCEPTION )
> +                hvm_inject_hw_exception(TRAP_gp_fault, 0);

The use of a local variable looks kind of pointless here.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1788,7 +1788,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>      return X86EMUL_OKAY;
>  
>   gpf:
> -    hvm_inject_hw_exception(TRAP_gp_fault, 0);
>      return X86EMUL_EXCEPTION;
>  }
>  
> @@ -1945,7 +1944,6 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>      return result;
>  
>   gpf:
> -    hvm_inject_hw_exception(TRAP_gp_fault, 0);
>      return X86EMUL_EXCEPTION;
>  }

In cases like these it would certainly be nice to get rid of the now
rather pointless goto-s, but of course we can equally well do this
in a later patch.

> @@ -1976,6 +1974,8 @@ static void svm_do_msr_access(struct cpu_user_regs *regs)
>  
>      if ( rc == X86EMUL_OKAY )
>          __update_guest_eip(regs, inst_len);
> +    else if ( rc == X86EMUL_EXCEPTION )
> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);

    else
        ASSERT_UNREACHABLE();

? (And then similarly for VMX.)

> +/*
> + * May return X86EMUL_EXCEPTION, at which point the caller is responsible for
> + * injecting a #GP fault.  Used to support speculative reads.
> + */
> +int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
> +int hvm_msr_write_intercept(
> +    unsigned int msr, uint64_t msr_content, bool_t may_defer);

Please add __must_check to both. With at least this one taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 6/8] x86/emul: Support speculative MSR reads

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 05 December 2016 10:09
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH 6/8] x86/emul: Support speculative MSR reads
> 
> Update the read_msr() hook to take an additional parameter, indicating that
> there should be no side effects of the read.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>

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

> ---
>  xen/arch/x86/hvm/emulate.c             |  3 ++-
>  xen/arch/x86/x86_emulate/x86_emulate.c | 24 ++++++++++++------------
>  xen/arch/x86/x86_emulate/x86_emulate.h |  6 +++++-
>  3 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index b182d57..bce0b00 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1529,11 +1529,12 @@ static int hvmemul_write_cr(
>  static int hvmemul_read_msr(
>      unsigned int reg,
>      uint64_t *val,
> +    bool speculative,
>      struct x86_emulate_ctxt *ctxt)
>  {
>      int rc = hvm_msr_read_intercept(reg, val);
> 
> -    if ( rc == X86EMUL_EXCEPTION )
> +    if ( rc == X86EMUL_EXCEPTION && !speculative )
>          x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
> 
>      return rc;
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index 877023d..5cba7ec 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1319,7 +1319,7 @@ in_longmode(
>      uint64_t efer;
> 
>      if ( !ops->read_msr ||
> -         unlikely(ops->read_msr(MSR_EFER, &efer, ctxt) != X86EMUL_OKAY) )
> +         unlikely(ops->read_msr(MSR_EFER, &efer, false, ctxt) !=
> X86EMUL_OKAY) )
>          return -1;
> 
>      return !!(efer & EFER_LMA);
> @@ -4412,7 +4412,7 @@ x86_emulate(
>          {
>              uint64_t tsc_aux;
>              fail_if(ops->read_msr == NULL);
> -            if ( (rc = ops->read_msr(MSR_TSC_AUX, &tsc_aux, ctxt)) != 0 )
> +            if ( (rc = ops->read_msr(MSR_TSC_AUX, &tsc_aux, false, ctxt)) != 0 )
>                  goto done;
>              _regs.ecx = (uint32_t)tsc_aux;
>              goto rdtsc;
> @@ -4548,11 +4548,11 @@ x86_emulate(
> 
>          /* Inject #UD if syscall/sysret are disabled. */
>          fail_if(ops->read_msr == NULL);
> -        if ( (rc = ops->read_msr(MSR_EFER, &msr_content, ctxt)) != 0 )
> +        if ( (rc = ops->read_msr(MSR_EFER, &msr_content, false, ctxt)) != 0 )
>              goto done;
>          generate_exception_if((msr_content & EFER_SCE) == 0, EXC_UD);
> 
> -        if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
> +        if ( (rc = ops->read_msr(MSR_STAR, &msr_content, false, ctxt)) != 0 )
>              goto done;
> 
>          cs.sel = (msr_content >> 32) & ~3; /* SELECTOR_RPL_MASK */
> @@ -4574,11 +4574,11 @@ x86_emulate(
>              _regs.r11 = _regs.eflags & ~EFLG_RF;
> 
>              if ( (rc = ops->read_msr(mode_64bit() ? MSR_LSTAR : MSR_CSTAR,
> -                                     &msr_content, ctxt)) != 0 )
> +                                     &msr_content, false, ctxt)) != 0 )
>                  goto done;
>              _regs.rip = msr_content;
> 
> -            if ( (rc = ops->read_msr(MSR_FMASK, &msr_content, ctxt)) != 0 )
> +            if ( (rc = ops->read_msr(MSR_FMASK, &msr_content, false, ctxt)) != 0
> )
>                  goto done;
>              _regs.eflags &= ~(msr_content | EFLG_RF);
>          }
> @@ -4793,7 +4793,7 @@ x86_emulate(
>              generate_exception_if(cr4 & CR4_TSD, EXC_GP, 0);
>          }
>          fail_if(ops->read_msr == NULL);
> -        if ( (rc = ops->read_msr(MSR_TSC, &val, ctxt)) != 0 )
> +        if ( (rc = ops->read_msr(MSR_TSC, &val, false, ctxt)) != 0 )
>              goto done;
>          _regs.edx = (uint32_t)(val >> 32);
>          _regs.eax = (uint32_t)(val >>  0);
> @@ -4804,7 +4804,7 @@ x86_emulate(
>          uint64_t val;
>          generate_exception_if(!mode_ring0(), EXC_GP, 0);
>          fail_if(ops->read_msr == NULL);
> -        if ( (rc = ops->read_msr((uint32_t)_regs.ecx, &val, ctxt)) != 0 )
> +        if ( (rc = ops->read_msr((uint32_t)_regs.ecx, &val, false, ctxt)) != 0 )
>              goto done;
>          _regs.edx = (uint32_t)(val >> 32);
>          _regs.eax = (uint32_t)(val >>  0);
> @@ -4825,7 +4825,7 @@ x86_emulate(
>          generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
> 
>          fail_if(ops->read_msr == NULL);
> -        if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) != 0 )
> +        if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, false,
> ctxt)) != 0 )
>              goto done;
> 
>          generate_exception_if(!(msr_content & 0xfffc), EXC_GP, 0);
> @@ -4853,11 +4853,11 @@ x86_emulate(
>               (rc = ops->write_segment(x86_seg_ss, &sreg, ctxt)) != 0 )
>              goto done;
> 
> -        if ( (rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt)) != 0 )
> +        if ( (rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, false,
> ctxt)) != 0 )
>              goto done;
>          _regs.eip = lm ? msr_content : (uint32_t)msr_content;
> 
> -        if ( (rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt)) != 0
> )
> +        if ( (rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, false,
> ctxt)) != 0 )
>              goto done;
>          _regs.esp = lm ? msr_content : (uint32_t)msr_content;
> 
> @@ -4873,7 +4873,7 @@ x86_emulate(
>          generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
> 
>          fail_if(ops->read_msr == NULL);
> -        if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) != 0 )
> +        if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, false,
> ctxt)) != 0 )
>              goto done;
> 
>          generate_exception_if(!(msr_content & 0xfffc), EXC_GP, 0);
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h
> b/xen/arch/x86/x86_emulate/x86_emulate.h
> index 164fc24..89cf20d 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -372,15 +372,19 @@ struct x86_emulate_ops
>      /*
>       * read_msr: Read from model-specific register.
>       *  @reg:   [IN ] Register to read.
> +     *  @val:   [OUT] Value read (only valid on X86EMUL_OKAY)
> +     *  @speculative [IN] Speculative read?
>       */
>      int (*read_msr)(
>          unsigned int reg,
>          uint64_t *val,
> +        bool speculative,
>          struct x86_emulate_ctxt *ctxt);
> 
>      /*
> -     * write_dr: Write to model-specific register.
> +     * write_msr: Write to model-specific register.
>       *  @reg:   [IN ] Register to write.
> +     *  @val:   [IN ] Value to write.
>       */
>      int (*write_msr)(
>          unsigned int reg,
> --
> 2.1.4


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

Re: [PATCH 7/8] x86/emul: Support CPUID fauilting via a speculative MSR read

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 05 December 2016 10:10
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH 7/8] x86/emul: Support CPUID fauilting via a speculative
> MSR read
> 
> Use the new speculative read support to query MSR_INTEL_MISC_FEATURES
> for
> active CPUID faulting, without raising #GP as a side effect.
> 
> This removes the need for every cpuid() emulation hook to individually
> support
> CPUID faulting.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>

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

> 
> Jan: Probably worth waiting to rebase over your changes moving the
> architectrual defines elsewhere
> ---
>  xen/arch/x86/hvm/emulate.c             |  9 ---------
>  xen/arch/x86/x86_emulate/x86_emulate.c | 16 +++++++++++++---
>  xen/arch/x86/x86_emulate/x86_emulate.h |  7 +------
>  3 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index bce0b00..1a132e7 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1567,15 +1567,6 @@ int hvmemul_cpuid(
>      unsigned int *edx,
>      struct x86_emulate_ctxt *ctxt)
>  {
> -    /*
> -     * x86_emulate uses this function to query CPU features for its own
> internal
> -     * use. Make sure we're actually emulating CPUID before emulating CPUID
> -     * faulting.
> -     */
> -    if ( ctxt->opcode == X86EMUL_OPC(0x0f, 0xa2) &&
> -         hvm_check_cpuid_faulting(current) )
> -        return X86EMUL_EXCEPTION;
> -
>      hvm_funcs.cpuid_intercept(eax, ebx, ecx, edx);
>      return X86EMUL_OKAY;
>  }
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index 5cba7ec..67495eb 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -406,6 +406,8 @@ typedef union {
> 
>  /* MSRs. */
>  #define MSR_TSC          0x00000010
> +#define MSR_INTEL_MISC_FEATURES 0x00000140
> +#define MISC_FEATURES_CPUID_FAULTING (1 << 0)
>  #define MSR_SYSENTER_CS  0x00000174
>  #define MSR_SYSENTER_ESP 0x00000175
>  #define MSR_SYSENTER_EIP 0x00000176
> @@ -5044,13 +5046,21 @@ x86_emulate(
>          src.val = x86_seg_fs;
>          goto pop_seg;
> 
> -    case X86EMUL_OPC(0x0f, 0xa2): /* cpuid */ {
> +    case X86EMUL_OPC(0x0f, 0xa2): /* cpuid */
> +    {
>          unsigned int eax = _regs.eax, ebx = _regs.ebx;
>          unsigned int ecx = _regs.ecx, edx = _regs.edx;
> +        uint64_t misc_features;
> +
>          fail_if(ops->cpuid == NULL);
> +        generate_exception_if(
> +            ops->read_msr &&
> +            ops->read_msr(MSR_INTEL_MISC_FEATURES,
> +                          &misc_features, true, ctxt) == X86EMUL_OKAY &&
> +            (misc_features & MISC_FEATURES_CPUID_FAULTING) &&
> +            !mode_ring0(), EXC_GP, 0); /* CPUID Faulting? */
> +
>          rc = ops->cpuid(&eax, &ebx, &ecx, &edx, ctxt);
> -        generate_exception_if(rc == X86EMUL_EXCEPTION,
> -                              EXC_GP, 0); /* CPUID Faulting? */
>          if ( rc != X86EMUL_OKAY )
>              goto done;
>          _regs.eax = eax; _regs.ebx = ebx;
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h
> b/xen/arch/x86/x86_emulate/x86_emulate.h
> index 89cf20d..46c863f 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -395,12 +395,7 @@ struct x86_emulate_ops
>      int (*wbinvd)(
>          struct x86_emulate_ctxt *ctxt);
> 
> -    /*
> -     * cpuid: Emulate CPUID via given set of EAX-EDX inputs/outputs.
> -     *
> -     * May return X86EMUL_EXCEPTION, which causes the emulator to inject
> -     * #GP[0].  Used to implement CPUID faulting.
> -     */
> +    /* cpuid: Emulate CPUID via given set of EAX-EDX inputs/outputs. */
>      int (*cpuid)(
>          unsigned int *eax,
>          unsigned int *ebx,
> --
> 2.1.4


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

Re: [PATCH 1/8] x86/shadow: Drop stale adjustment in the PAE second-half search

[Andrew Cooper]

On 05/12/16 10:16, Tim Deegan wrote:
> At 10:09 +0000 on 05 Dec (1480932564), Andrew Cooper wrote:
>> This shouldn't have been present in c/s 29a57c992 "x86/emul: Rework emulator
>> event injection".  It was a leftover from a previous version of the series.
> IMO this hunk was correct in 29a57c992, but it's fine to remove it now.

It wasn't necessary in 29a57c992, for the same reason it can be dropped now.

Given the comment introduced earlier in 1d60efc57, I'd intended not to
have any modification to this path.

>
> Acked-by: Tim Deegan <tim@xen.org>

Thanks,

~Andrew

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

Re: [PATCH 2/8] x86/emul: Debugging improvements to the test harness

[Andrew Cooper]

On 05/12/16 12:00, Jan Beulich wrote:
>>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote:
>> @@ -47,6 +49,9 @@ static int read(
>>      unsigned int bytes,
>>      struct x86_emulate_ctxt *ctxt)
>>  {
>> +    if ( verbose )
>> +        printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
> There appear to be two stray commas here, or do they serve a
> purpose?

Ah - that is my shorthand for omitted parameters.  It is helpful if
there are a lot of integer parameters.

~Andrew

>
> Other than that
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Jan
>


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

Re: [PATCH 6/8] x86/emul: Support speculative MSR reads

[Jan Beulich]

>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1319,7 +1319,7 @@ in_longmode(
>      uint64_t efer;
>  
>      if ( !ops->read_msr ||
> -         unlikely(ops->read_msr(MSR_EFER, &efer, ctxt) != X86EMUL_OKAY) )
> +         unlikely(ops->read_msr(MSR_EFER, &efer, false, ctxt) != X86EMUL_OKAY) )

Don't you mean "true" here (and the "no functional change" dropped
from the commit message)?

> @@ -4412,7 +4412,7 @@ x86_emulate(
>          {
>              uint64_t tsc_aux;
>              fail_if(ops->read_msr == NULL);
> -            if ( (rc = ops->read_msr(MSR_TSC_AUX, &tsc_aux, ctxt)) != 0 )
> +            if ( (rc = ops->read_msr(MSR_TSC_AUX, &tsc_aux, false, ctxt)) != 0 )

And here too? And basically everywhere except in the actual
RDMSR emulation code? Which then raises the question
whether this extra parameter is a useful thing to have, as
there's exactly one place where you want the #GP(0). I
guess the hook should simply return boolean, and the caller
determine whether to generate an exception.

Jan


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

Re: [PATCH 7/8] x86/emul: Support CPUID fauilting via a speculative MSR read

[Jan Beulich]

>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote:
> Jan: Probably worth waiting to rebase over your changes moving the
> architectrual defines elsewhere

I have no such changes yet; I only recall us discussing this would be
a good thing at some point.

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1567,15 +1567,6 @@ int hvmemul_cpuid(
>      unsigned int *edx,
>      struct x86_emulate_ctxt *ctxt)
>  {
> -    /*
> -     * x86_emulate uses this function to query CPU features for its own internal
> -     * use. Make sure we're actually emulating CPUID before emulating CPUID
> -     * faulting.
> -     */
> -    if ( ctxt->opcode == X86EMUL_OPC(0x0f, 0xa2) &&
> -         hvm_check_cpuid_faulting(current) )
> -        return X86EMUL_EXCEPTION;

With this gone, is hvm_check_cpuid_faulting() as useful function to
retain, considering it then has only one caller in VMX code?

Other than that
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Would allow me to drop half a hunk from the pv privop patch, so I'd
be glad to see this go in soon.

Jan


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

Re: [PATCH 8/8] x86/emul: Implement the STAC and CLAC instructions

[Jan Beulich]

>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote:
> @@ -4362,11 +4363,27 @@ x86_emulate(
>  
>          switch( modrm )
>          {
> -#ifdef __XEN__
> -        case 0xd1: /* xsetbv */
> -        {
>              unsigned long cr4;
>  
> +        case 0xca: /* clac */
> +        case 0xcb: /* stac */
> +            generate_exception_if(
> +                lock_prefix || (_regs.eflags & EFLG_VM), EXC_UD);
> +            if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
> +                cr4 = 0;
> +            /*
> +             * Contrary to expectation (i.e. #GP[0]), #UD for the CPL check is
> +             * the documented and observed behaviour.
> +             */
> +            generate_exception_if(!(cr4 & CR4_SMAP) || !mode_ring0(), EXC_UD);

If documentation is to be trusted, then there's no CR4.SMAP check
supposed to be here, but just a CPUID one.

Otoh I assume documentation can't be trusted regarding the use of
prefixes 66, F2, and F3: Just like they're apparently illegal to use with
VMFUNC (thread still pending with Intel) and like documented for e.g.
XGETBV and XSETBV, I would think you need a vex.pfx check here
despite the SDM not explicitly saying so.

Jan


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

Re: [PATCH 5/8] x86/hvm: Don't raise #GP behind the emulators back for MSR accesses

[Andrew Cooper]

On 05/12/16 12:10, Jan Beulich wrote:
>>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v)
>>  
>>          if ( w->do_write.msr )
>>          {
>> -            hvm_msr_write_intercept(w->msr, w->value, 0);
>> +            int rc = hvm_msr_write_intercept(w->msr, w->value, 0);
>> +
>> +            if ( rc == X86EMUL_EXCEPTION )
>> +                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> The use of a local variable looks kind of pointless here.

The first version had

if ( hvm_msr_write_intercept(w->msr, w->value, 0) ==
     X86EMUL_EXCEPTION )

but this looked rather ugly to read.  I prefer the version as submitted,
but am not too fussed if you insist for the latter?

>
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1788,7 +1788,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>>      return X86EMUL_OKAY;
>>  
>>   gpf:
>> -    hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>      return X86EMUL_EXCEPTION;
>>  }
>>  
>> @@ -1945,7 +1944,6 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>      return result;
>>  
>>   gpf:
>> -    hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>      return X86EMUL_EXCEPTION;
>>  }
> In cases like these it would certainly be nice to get rid of the now
> rather pointless goto-s, but of course we can equally well do this
> in a later patch.

I will do a cleanup patch and add it to v2.

~Andrew

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

Re: [PATCH 5/8] x86/hvm: Don't raise #GP behind the emulators back for MSR accesses

[Jan Beulich]

>>> On 05.12.16 at 17:29, <andrew.cooper3@citrix.com> wrote:
> On 05/12/16 12:10, Jan Beulich wrote:
>>>>> On 05.12.16 at 11:09, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -509,7 +509,11 @@ void hvm_do_resume(struct vcpu *v)
>>>  
>>>          if ( w->do_write.msr )
>>>          {
>>> -            hvm_msr_write_intercept(w->msr, w->value, 0);
>>> +            int rc = hvm_msr_write_intercept(w->msr, w->value, 0);
>>> +
>>> +            if ( rc == X86EMUL_EXCEPTION )
>>> +                hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> The use of a local variable looks kind of pointless here.
> 
> The first version had
> 
> if ( hvm_msr_write_intercept(w->msr, w->value, 0) ==
>      X86EMUL_EXCEPTION )
> 
> but this looked rather ugly to read.  I prefer the version as submitted,
> but am not too fussed if you insist for the latter?

I won't insist, it was just a suggestion to make the code look better
to my eyes. If you like it better as is, keep it.

Jan


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

Re: [PATCH 5/8] x86/hvm: Don't raise #GP behind the emulators back for MSR accesses

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Monday, December 05, 2016 6:09 PM
> 
> The current hvm_msr_{read,write}_intercept() infrastructure calls
> hvm_inject_hw_exception() directly to latch a fault, and returns
> X86EMUL_EXCEPTION to its caller.
> 
> This behaviour is problematic for the hvmemul_{read,write}_msr() paths, as the
> fault is raised behind the back of the x86 emulator.
> 
> Alter the behaviour so hvm_msr_{read,write}_intercept() simply returns
> X86EMUL_EXCEPTION, leaving the callers to actually inject the #GP fault.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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