[PATCH v4 0/4] clean up MSR save/restore code

[PATCH v4 0/4] clean up MSR save/restore code

[Paul Durrant]

*** BLURB HERE ***

Paul Durrant (4):
  x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation
    code
  x86: move the saved value of MSR_IA32_XSS into struct vcpu_msrs
  x86: stop handling MSR_IA32_XSS save/restore in implementation code
  x86: remove defunct init/load/save_msr() hvm_funcs

 xen/arch/x86/domain.c          |  2 +-
 xen/arch/x86/hvm/hvm.c         | 58 ++++++---------------------
 xen/arch/x86/hvm/vmx/vmx.c     | 73 ++--------------------------------
 xen/arch/x86/msr.c             | 38 ++++++++++++++++++
 xen/include/asm-x86/hvm/hvm.h  |  8 +---
 xen/include/asm-x86/hvm/vcpu.h |  1 -
 xen/include/asm-x86/msr.h      |  5 +++
 7 files changed, 63 insertions(+), 122 deletions(-)
---
v2:
 - Drop patches #2 and #6 of the v1 series

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
-- 
2.20.1


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

[PATCH v4 1/4] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Paul Durrant]

Saving and restoring the value of this MSR is currently handled by
implementation-specific code despite it being architectural. This patch
moves handling of accesses to this MSR from hvm.c into the msr.c, thus
allowing the common MSR save/restore code to handle it.

NOTE: Because vmx_get/set_guest_bndcfgs() call vmx_vmcs_enter(), the
      struct vcpu pointer passed in, and hence the vcpu pointer passed to
      guest_rdmsr() cannot be const.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>

v4:
 - Cosmetic re-arrangements and an additional ASSERT requested by Jan

v3:
 - Address further comments from Jan
 - Dropped Kevin's R-b because of change to vmx.c

v2:
 - Addressed comments from Jan by largely removing hunks
 - Keeping Kevin's R-b since remaining hunks in vmx.c are as before
---
 xen/arch/x86/hvm/hvm.c        | 14 ++------------
 xen/arch/x86/hvm/vmx/vmx.c    | 24 ++++--------------------
 xen/arch/x86/msr.c            | 20 ++++++++++++++++++++
 xen/include/asm-x86/hvm/hvm.h |  4 ++--
 4 files changed, 28 insertions(+), 34 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8adbb61b57..e566d83f8b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1303,6 +1303,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
 static const uint32_t msrs_to_send[] = {
     MSR_SPEC_CTRL,
     MSR_INTEL_MISC_FEATURES_ENABLES,
+    MSR_IA32_BNDCFGS,
     MSR_AMD64_DR0_ADDRESS_MASK,
     MSR_AMD64_DR1_ADDRESS_MASK,
     MSR_AMD64_DR2_ADDRESS_MASK,
@@ -1440,6 +1441,7 @@ static int hvm_load_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
 
         case MSR_SPEC_CTRL:
         case MSR_INTEL_MISC_FEATURES_ENABLES:
+        case MSR_IA32_BNDCFGS:
         case MSR_AMD64_DR0_ADDRESS_MASK:
         case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
             rc = guest_wrmsr(v, ctxt->msr[i].index, ctxt->msr[i].val);
@@ -3467,12 +3469,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         *msr_content = v->arch.hvm.msr_xss;
         break;
 
-    case MSR_IA32_BNDCFGS:
-        if ( !d->arch.cpuid->feat.mpx ||
-             !hvm_get_guest_bndcfgs(v, msr_content) )
-            goto gp_fault;
-        break;
-
     case MSR_K8_ENABLE_C1E:
     case MSR_AMD64_NB_CFG:
          /*
@@ -3619,12 +3615,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
         v->arch.hvm.msr_xss = msr_content;
         break;
 
-    case MSR_IA32_BNDCFGS:
-        if ( !d->arch.cpuid->feat.mpx ||
-             !hvm_set_guest_bndcfgs(v, msr_content) )
-            goto gp_fault;
-        break;
-
     case MSR_AMD64_NB_CFG:
         /* ignore the write */
         break;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 725dd88c13..f8481d032a 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -805,17 +805,6 @@ static unsigned int __init vmx_init_msr(void)
 
 static void vmx_save_msr(struct vcpu *v, struct hvm_msr *ctxt)
 {
-    vmx_vmcs_enter(v);
-
-    if ( cpu_has_mpx && cpu_has_vmx_mpx )
-    {
-        __vmread(GUEST_BNDCFGS, &ctxt->msr[ctxt->count].val);
-        if ( ctxt->msr[ctxt->count].val )
-            ctxt->msr[ctxt->count++].index = MSR_IA32_BNDCFGS;
-    }
-
-    vmx_vmcs_exit(v);
-
     if ( cpu_has_xsaves && cpu_has_vmx_xsaves )
     {
         ctxt->msr[ctxt->count].val = v->arch.hvm.msr_xss;
@@ -835,14 +824,6 @@ static int vmx_load_msr(struct vcpu *v, struct hvm_msr *ctxt)
     {
         switch ( ctxt->msr[i].index )
         {
-        case MSR_IA32_BNDCFGS:
-            if ( cpu_has_mpx && cpu_has_vmx_mpx &&
-                 is_canonical_address(ctxt->msr[i].val) &&
-                 !(ctxt->msr[i].val & IA32_BNDCFGS_RESERVED) )
-                __vmwrite(GUEST_BNDCFGS, ctxt->msr[i].val);
-            else if ( ctxt->msr[i].val )
-                err = -ENXIO;
-            break;
         case MSR_IA32_XSS:
             if ( cpu_has_xsaves && cpu_has_vmx_xsaves )
                 v->arch.hvm.msr_xss = ctxt->msr[i].val;
@@ -1215,8 +1196,11 @@ static bool vmx_set_guest_bndcfgs(struct vcpu *v, u64 val)
     return true;
 }
 
-static bool vmx_get_guest_bndcfgs(struct vcpu *v, u64 *val)
+static bool vmx_get_guest_bndcfgs(const struct vcpu *cv, u64 *val)
 {
+    /* Get a non-const pointer for vmx_vmcs_enter() */
+    struct vcpu *v = cv->domain->vcpu[cv->vcpu_id];
+
     ASSERT(cpu_has_mpx && cpu_has_vmx_mpx);
 
     vmx_vmcs_enter(v);
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 4df4a59f4d..0e901d2397 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -160,6 +160,16 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         ret = guest_rdmsr_x2apic(v, msr, val);
         break;
 
+    case MSR_IA32_BNDCFGS:
+        if ( !cp->feat.mpx )
+            goto gp_fault;
+
+        ASSERT(is_hvm_domain(d));
+        if (!hvm_get_guest_bndcfgs(v, val) )
+            goto gp_fault;
+
+        break;
+
     case 0x40000000 ... 0x400001ff:
         if ( is_viridian_domain(d) )
         {
@@ -323,6 +333,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         ret = guest_wrmsr_x2apic(v, msr, val);
         break;
 
+    case MSR_IA32_BNDCFGS:
+        if ( !cp->feat.mpx )
+            goto gp_fault;
+
+        ASSERT(is_hvm_domain(d));
+        if ( !hvm_set_guest_bndcfgs(v, val) )
+            goto gp_fault;
+
+        break;
+
     case 0x40000000 ... 0x400001ff:
         if ( is_viridian_domain(d) )
         {
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 53ffebb2c5..d41ed63232 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -149,7 +149,7 @@ struct hvm_function_table {
     int  (*get_guest_pat)(struct vcpu *v, u64 *);
     int  (*set_guest_pat)(struct vcpu *v, u64);
 
-    bool (*get_guest_bndcfgs)(struct vcpu *v, u64 *);
+    bool (*get_guest_bndcfgs)(const struct vcpu *v, u64 *);
     bool (*set_guest_bndcfgs)(struct vcpu *v, u64);
 
     void (*set_tsc_offset)(struct vcpu *v, u64 offset, u64 at_tsc);
@@ -448,7 +448,7 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
     return hvm_funcs.get_shadow_gs_base(v);
 }
 
-static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val)
+static inline bool hvm_get_guest_bndcfgs(const struct vcpu *v, u64 *val)
 {
     return hvm_funcs.get_guest_bndcfgs &&
            hvm_funcs.get_guest_bndcfgs(v, val);
-- 
2.20.1


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

[PATCH v4 2/4] x86: move the saved value of MSR_IA32_XSS into struct vcpu_msrs

[Paul Durrant]

Currently the value is saved directly in struct hvm_vcpu. This patch simply
co-locates it with other saved MSR values. No functional change.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
---
 xen/arch/x86/domain.c          | 2 +-
 xen/arch/x86/hvm/hvm.c         | 4 ++--
 xen/arch/x86/hvm/vmx/vmx.c     | 4 ++--
 xen/include/asm-x86/hvm/vcpu.h | 1 -
 xen/include/asm-x86/msr.h      | 5 +++++
 5 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8d579e2cf9..aa38555736 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1653,7 +1653,7 @@ static void __context_switch(void)
                 BUG();
 
             if ( cpu_has_xsaves && is_hvm_vcpu(n) )
-                set_msr_xss(n->arch.hvm.msr_xss);
+                set_msr_xss(n->arch.msrs->xss.raw);
         }
         vcpu_restore_fpu_nonlazy(n, false);
         nd->arch.ctxt_switch->to(n);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e566d83f8b..dff590e658 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3466,7 +3466,7 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     case MSR_IA32_XSS:
         if ( !d->arch.cpuid->xstate.xsaves )
             goto gp_fault;
-        *msr_content = v->arch.hvm.msr_xss;
+        *msr_content = v->arch.msrs->xss.raw;
         break;
 
     case MSR_K8_ENABLE_C1E:
@@ -3612,7 +3612,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
         /* No XSS features currently supported for guests. */
         if ( !d->arch.cpuid->xstate.xsaves || msr_content != 0 )
             goto gp_fault;
-        v->arch.hvm.msr_xss = msr_content;
+        v->arch.msrs->xss.raw = msr_content;
         break;
 
     case MSR_AMD64_NB_CFG:
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f8481d032a..985e5735d2 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -807,7 +807,7 @@ static void vmx_save_msr(struct vcpu *v, struct hvm_msr *ctxt)
 {
     if ( cpu_has_xsaves && cpu_has_vmx_xsaves )
     {
-        ctxt->msr[ctxt->count].val = v->arch.hvm.msr_xss;
+        ctxt->msr[ctxt->count].val = v->arch.msrs->xss.raw;
         if ( ctxt->msr[ctxt->count].val )
             ctxt->msr[ctxt->count++].index = MSR_IA32_XSS;
     }
@@ -826,7 +826,7 @@ static int vmx_load_msr(struct vcpu *v, struct hvm_msr *ctxt)
         {
         case MSR_IA32_XSS:
             if ( cpu_has_xsaves && cpu_has_vmx_xsaves )
-                v->arch.hvm.msr_xss = ctxt->msr[i].val;
+                v->arch.msrs->xss.raw = ctxt->msr[i].val;
             else
                 err = -ENXIO;
             break;
diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
index 6c84d5a5a6..5563d28a4e 100644
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -176,7 +176,6 @@ struct hvm_vcpu {
     struct hvm_vcpu_asid n1asid;
 
     u64                 msr_tsc_adjust;
-    u64                 msr_xss;
 
     union {
         struct vmx_vcpu vmx;
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index a7244793bf..0d52c085f6 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -313,6 +313,11 @@ struct vcpu_msrs
      * values here may be stale in current context.
      */
     uint32_t dr_mask[4];
+
+    /* 0x00000da0 - MSR_IA32_XSS */
+    struct {
+        uint64_t raw;
+    } xss;
 };
 
 void init_guest_msr_policy(void);
-- 
2.20.1


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

[PATCH v4 3/4] x86: stop handling MSR_IA32_XSS save/restore in implementation code

[Paul Durrant]

Saving and restoring the value of this MSR is currently handled by
implementation-specific code despite it being architectural. This patch
moves handling of accesses to this MSR from hvm.c into the msr.c, thus
allowing the common MSR save/restore code to handle it.

This patch also adds proper checks of CPUID policy in the new get/set code.

NOTE: MSR_IA32_XSS is the last MSR to be saved and restored by
      implementation-specific code. This patch therefore removes the
      (VMX) definitions and of the init_msr(), save_msr() and
      load_msr() hvm_funcs, as they are no longer necessary. The
      declarations of and calls to those hvm_funcs will be cleaned up
      by a subsequent patch.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
---
 xen/arch/x86/hvm/hvm.c     | 15 ++----------
 xen/arch/x86/hvm/vmx/vmx.c | 49 --------------------------------------
 xen/arch/x86/msr.c         | 18 ++++++++++++++
 3 files changed, 20 insertions(+), 62 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index dff590e658..deb7fb2adb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1304,6 +1304,7 @@ static const uint32_t msrs_to_send[] = {
     MSR_SPEC_CTRL,
     MSR_INTEL_MISC_FEATURES_ENABLES,
     MSR_IA32_BNDCFGS,
+    MSR_IA32_XSS,
     MSR_AMD64_DR0_ADDRESS_MASK,
     MSR_AMD64_DR1_ADDRESS_MASK,
     MSR_AMD64_DR2_ADDRESS_MASK,
@@ -1442,6 +1443,7 @@ static int hvm_load_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
         case MSR_SPEC_CTRL:
         case MSR_INTEL_MISC_FEATURES_ENABLES:
         case MSR_IA32_BNDCFGS:
+        case MSR_IA32_XSS:
         case MSR_AMD64_DR0_ADDRESS_MASK:
         case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
             rc = guest_wrmsr(v, ctxt->msr[i].index, ctxt->msr[i].val);
@@ -3463,12 +3465,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
                                                     MTRRcap_VCNT))];
         break;
 
-    case MSR_IA32_XSS:
-        if ( !d->arch.cpuid->xstate.xsaves )
-            goto gp_fault;
-        *msr_content = v->arch.msrs->xss.raw;
-        break;
-
     case MSR_K8_ENABLE_C1E:
     case MSR_AMD64_NB_CFG:
          /*
@@ -3608,13 +3604,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
             goto gp_fault;
         break;
 
-    case MSR_IA32_XSS:
-        /* No XSS features currently supported for guests. */
-        if ( !d->arch.cpuid->xstate.xsaves || msr_content != 0 )
-            goto gp_fault;
-        v->arch.msrs->xss.raw = msr_content;
-        break;
-
     case MSR_AMD64_NB_CFG:
         /* ignore the write */
         break;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 985e5735d2..c46e05b91e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -797,52 +797,6 @@ static int vmx_load_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
     return 0;
 }
 
-static unsigned int __init vmx_init_msr(void)
-{
-    return (cpu_has_mpx && cpu_has_vmx_mpx) +
-           (cpu_has_xsaves && cpu_has_vmx_xsaves);
-}
-
-static void vmx_save_msr(struct vcpu *v, struct hvm_msr *ctxt)
-{
-    if ( cpu_has_xsaves && cpu_has_vmx_xsaves )
-    {
-        ctxt->msr[ctxt->count].val = v->arch.msrs->xss.raw;
-        if ( ctxt->msr[ctxt->count].val )
-            ctxt->msr[ctxt->count++].index = MSR_IA32_XSS;
-    }
-}
-
-static int vmx_load_msr(struct vcpu *v, struct hvm_msr *ctxt)
-{
-    unsigned int i;
-    int err = 0;
-
-    vmx_vmcs_enter(v);
-
-    for ( i = 0; i < ctxt->count; ++i )
-    {
-        switch ( ctxt->msr[i].index )
-        {
-        case MSR_IA32_XSS:
-            if ( cpu_has_xsaves && cpu_has_vmx_xsaves )
-                v->arch.msrs->xss.raw = ctxt->msr[i].val;
-            else
-                err = -ENXIO;
-            break;
-        default:
-            continue;
-        }
-        if ( err )
-            break;
-        ctxt->msr[i]._rsvd = 1;
-    }
-
-    vmx_vmcs_exit(v);
-
-    return err;
-}
-
 static void vmx_fpu_enter(struct vcpu *v)
 {
     vcpu_restore_fpu_lazy(v);
@@ -2282,9 +2236,6 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .vcpu_destroy         = vmx_vcpu_destroy,
     .save_cpu_ctxt        = vmx_save_vmcs_ctxt,
     .load_cpu_ctxt        = vmx_load_vmcs_ctxt,
-    .init_msr             = vmx_init_msr,
-    .save_msr             = vmx_save_msr,
-    .load_msr             = vmx_load_msr,
     .get_interrupt_shadow = vmx_get_interrupt_shadow,
     .set_interrupt_shadow = vmx_set_interrupt_shadow,
     .guest_x86_mode       = vmx_guest_x86_mode,
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 0e901d2397..4b5e000224 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -170,6 +170,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
 
         break;
 
+    case MSR_IA32_XSS:
+        if ( !cp->xstate.xsaves )
+            goto gp_fault;
+
+        *val = msrs->xss.raw;
+        break;
+
     case 0x40000000 ... 0x400001ff:
         if ( is_viridian_domain(d) )
         {
@@ -343,6 +350,17 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
 
         break;
 
+    case MSR_IA32_XSS:
+        if ( !cp->xstate.xsaves )
+            goto gp_fault;
+
+        /* No XSS features currently supported for guests */
+        if ( val != 0 )
+            goto gp_fault;
+
+        msrs->xss.raw = val;
+        break;
+
     case 0x40000000 ... 0x400001ff:
         if ( is_viridian_domain(d) )
         {
-- 
2.20.1


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

[PATCH v4 4/4] x86: remove defunct init/load/save_msr() hvm_funcs

[Paul Durrant]

These hvm_funcs are no longer required since no MSR values are saved or
restored by implementation-specific code.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>

v2:
 - Re-instate err check on loop in hvm_load_cpu_msrs()
---
 xen/arch/x86/hvm/hvm.c        | 29 +++++++++--------------------
 xen/include/asm-x86/hvm/hvm.h |  4 ----
 2 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index deb7fb2adb..f84bf11f0f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1310,7 +1310,6 @@ static const uint32_t msrs_to_send[] = {
     MSR_AMD64_DR2_ADDRESS_MASK,
     MSR_AMD64_DR3_ADDRESS_MASK,
 };
-static unsigned int __read_mostly msr_count_max = ARRAY_SIZE(msrs_to_send);
 
 static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
 {
@@ -1320,7 +1319,7 @@ static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
     int err;
 
     err = _hvm_init_entry(h, CPU_MSR_CODE, v->vcpu_id,
-                             HVM_CPU_MSR_SIZE(msr_count_max));
+                             HVM_CPU_MSR_SIZE(ARRAY_SIZE(msrs_to_send)));
     if ( err )
         return err;
     ctxt = (struct hvm_msr *)&h->data[h->cur];
@@ -1353,10 +1352,7 @@ static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
         ctxt->msr[ctxt->count++].val = val;
     }
 
-    if ( hvm_funcs.save_msr )
-        hvm_funcs.save_msr(v, ctxt);
-
-    ASSERT(ctxt->count <= msr_count_max);
+    ASSERT(ctxt->count <= ARRAY_SIZE(msrs_to_send));
 
     for ( i = 0; i < ctxt->count; ++i )
         ctxt->msr[i]._rsvd = 0;
@@ -1431,9 +1427,6 @@ static int hvm_load_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
             return -EOPNOTSUPP;
     /* Checking finished */
 
-    if ( hvm_funcs.load_msr )
-        err = hvm_funcs.load_msr(v, ctxt);
-
     for ( i = 0; !err && i < ctxt->count; ++i )
     {
         switch ( ctxt->msr[i].index )
@@ -1475,17 +1468,13 @@ static int __init hvm_register_CPU_save_and_restore(void)
                             sizeof(struct hvm_save_descriptor),
                         HVMSR_PER_VCPU);
 
-    if ( hvm_funcs.init_msr )
-        msr_count_max += hvm_funcs.init_msr();
-
-    if ( msr_count_max )
-        hvm_register_savevm(CPU_MSR_CODE,
-                            "CPU_MSR",
-                            hvm_save_cpu_msrs,
-                            hvm_load_cpu_msrs,
-                            HVM_CPU_MSR_SIZE(msr_count_max) +
-                                sizeof(struct hvm_save_descriptor),
-                            HVMSR_PER_VCPU);
+    hvm_register_savevm(CPU_MSR_CODE,
+                        "CPU_MSR",
+                        hvm_save_cpu_msrs,
+                        hvm_load_cpu_msrs,
+                        HVM_CPU_MSR_SIZE(ARRAY_SIZE(msrs_to_send)) +
+                            sizeof(struct hvm_save_descriptor),
+                        HVMSR_PER_VCPU);
 
     return 0;
 }
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index d41ed63232..a70fdbe298 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -115,10 +115,6 @@ struct hvm_function_table {
     void (*save_cpu_ctxt)(struct vcpu *v, struct hvm_hw_cpu *ctxt);
     int (*load_cpu_ctxt)(struct vcpu *v, struct hvm_hw_cpu *ctxt);
 
-    unsigned int (*init_msr)(void);
-    void (*save_msr)(struct vcpu *, struct hvm_msr *);
-    int (*load_msr)(struct vcpu *, struct hvm_msr *);
-
     /* Examine specifics of the guest state. */
     unsigned int (*get_interrupt_shadow)(struct vcpu *v);
     void (*set_interrupt_shadow)(struct vcpu *v, unsigned int intr_shadow);
-- 
2.20.1


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

Re: [PATCH v4 1/4] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Jan Beulich]

>>> On 14.03.19 at 14:51, <paul.durrant@citrix.com> wrote:
> Saving and restoring the value of this MSR is currently handled by
> implementation-specific code despite it being architectural. This patch
> moves handling of accesses to this MSR from hvm.c into the msr.c, thus
> allowing the common MSR save/restore code to handle it.
> 
> NOTE: Because vmx_get/set_guest_bndcfgs() call vmx_vmcs_enter(), the
>       struct vcpu pointer passed in, and hence the vcpu pointer passed to
>       guest_rdmsr() cannot be const.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

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



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

Re: [PATCH v4 1/4] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Jan Beulich]

>>> On 14.03.19 at 14:51, <paul.durrant@citrix.com> wrote:
> Saving and restoring the value of this MSR is currently handled by
> implementation-specific code despite it being architectural. This patch
> moves handling of accesses to this MSR from hvm.c into the msr.c, thus
> allowing the common MSR save/restore code to handle it.
> 
> NOTE: Because vmx_get/set_guest_bndcfgs() call vmx_vmcs_enter(), the
>       struct vcpu pointer passed in, and hence the vcpu pointer passed to
>       guest_rdmsr() cannot be const.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> 
> v4:
>  - Cosmetic re-arrangements and an additional ASSERT requested by Jan
> 
> v3:
>  - Address further comments from Jan
>  - Dropped Kevin's R-b because of change to vmx.c

Kevin,

you may not have noticed the above.

Thanks, Jan


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

Re: [Xen-devel] [PATCH v4 1/4] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Jan Beulich]

>>> On 14.03.19 at 14:51, <paul.durrant@citrix.com> wrote:
> Saving and restoring the value of this MSR is currently handled by
> implementation-specific code despite it being architectural. This patch
> moves handling of accesses to this MSR from hvm.c into the msr.c, thus
> allowing the common MSR save/restore code to handle it.
> 
> NOTE: Because vmx_get/set_guest_bndcfgs() call vmx_vmcs_enter(), the
>       struct vcpu pointer passed in, and hence the vcpu pointer passed to
>       guest_rdmsr() cannot be const.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> 
> v4:
>  - Cosmetic re-arrangements and an additional ASSERT requested by Jan
> 
> v3:
>  - Address further comments from Jan
>  - Dropped Kevin's R-b because of change to vmx.c

Kevin,

you may not have noticed the above.

Thanks, Jan


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

Re: [PATCH v4 1/4] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, April 5, 2019 7:30 PM
> 
> >>> On 14.03.19 at 14:51, <paul.durrant@citrix.com> wrote:
> > Saving and restoring the value of this MSR is currently handled by
> > implementation-specific code despite it being architectural. This patch
> > moves handling of accesses to this MSR from hvm.c into the msr.c, thus
> > allowing the common MSR save/restore code to handle it.
> >
> > NOTE: Because vmx_get/set_guest_bndcfgs() call vmx_vmcs_enter(), the
> >       struct vcpu pointer passed in, and hence the vcpu pointer passed to
> >       guest_rdmsr() cannot be const.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> > Cc: Jun Nakajima <jun.nakajima@intel.com>
> >
> > v4:
> >  - Cosmetic re-arrangements and an additional ASSERT requested by Jan
> >
> > v3:
> >  - Address further comments from Jan
> >  - Dropped Kevin's R-b because of change to vmx.c
> 
> Kevin,
> 
> you may not have noticed the above.
> 

yes indeed. Thanks for remind!

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

Re: [Xen-devel] [PATCH v4 1/4] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, April 5, 2019 7:30 PM
> 
> >>> On 14.03.19 at 14:51, <paul.durrant@citrix.com> wrote:
> > Saving and restoring the value of this MSR is currently handled by
> > implementation-specific code despite it being architectural. This patch
> > moves handling of accesses to this MSR from hvm.c into the msr.c, thus
> > allowing the common MSR save/restore code to handle it.
> >
> > NOTE: Because vmx_get/set_guest_bndcfgs() call vmx_vmcs_enter(), the
> >       struct vcpu pointer passed in, and hence the vcpu pointer passed to
> >       guest_rdmsr() cannot be const.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> > Cc: Jun Nakajima <jun.nakajima@intel.com>
> >
> > v4:
> >  - Cosmetic re-arrangements and an additional ASSERT requested by Jan
> >
> > v3:
> >  - Address further comments from Jan
> >  - Dropped Kevin's R-b because of change to vmx.c
> 
> Kevin,
> 
> you may not have noticed the above.
> 

yes indeed. Thanks for remind!

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

Re: [PATCH v4 1/4] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Andrew Cooper]

On 14/03/2019 13:51, Paul Durrant wrote:
> @@ -1215,8 +1196,11 @@ static bool vmx_set_guest_bndcfgs(struct vcpu *v, u64 val)
>      return true;
>  }
>  
> -static bool vmx_get_guest_bndcfgs(struct vcpu *v, u64 *val)
> +static bool vmx_get_guest_bndcfgs(const struct vcpu *cv, u64 *val)
>  {
> +    /* Get a non-const pointer for vmx_vmcs_enter() */
> +    struct vcpu *v = cv->domain->vcpu[cv->vcpu_id];
> +

I'm sorry not having got around to reviewing this series in a timely
fashion, but I am going to specifically nack de-consting games like
this.  There is now vcpu state corruption when the MSR is accessed
remotely - this hook *must* remain a mutable vcpu pointer.

There are also multiple other functional issues and regressions
introduced by this series.  I'm trying to put together a patch to fix
all of the fallout, but I also might revert the series wholesale
depending on the eventual complexity.

~Andrew

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

Re: [Xen-devel] [PATCH v4 1/4] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Andrew Cooper]

On 14/03/2019 13:51, Paul Durrant wrote:
> @@ -1215,8 +1196,11 @@ static bool vmx_set_guest_bndcfgs(struct vcpu *v, u64 val)
>      return true;
>  }
>  
> -static bool vmx_get_guest_bndcfgs(struct vcpu *v, u64 *val)
> +static bool vmx_get_guest_bndcfgs(const struct vcpu *cv, u64 *val)
>  {
> +    /* Get a non-const pointer for vmx_vmcs_enter() */
> +    struct vcpu *v = cv->domain->vcpu[cv->vcpu_id];
> +

I'm sorry not having got around to reviewing this series in a timely
fashion, but I am going to specifically nack de-consting games like
this.  There is now vcpu state corruption when the MSR is accessed
remotely - this hook *must* remain a mutable vcpu pointer.

There are also multiple other functional issues and regressions
introduced by this series.  I'm trying to put together a patch to fix
all of the fallout, but I also might revert the series wholesale
depending on the eventual complexity.

~Andrew

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

Re: [PATCH v4 1/4] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Jan Beulich]

>>> On 09.04.19 at 16:38, <andrew.cooper3@citrix.com> wrote:
> On 14/03/2019 13:51, Paul Durrant wrote:
>> @@ -1215,8 +1196,11 @@ static bool vmx_set_guest_bndcfgs(struct vcpu *v, u64 val)
>>      return true;
>>  }
>>  
>> -static bool vmx_get_guest_bndcfgs(struct vcpu *v, u64 *val)
>> +static bool vmx_get_guest_bndcfgs(const struct vcpu *cv, u64 *val)
>>  {
>> +    /* Get a non-const pointer for vmx_vmcs_enter() */
>> +    struct vcpu *v = cv->domain->vcpu[cv->vcpu_id];
>> +
> 
> I'm sorry not having got around to reviewing this series in a timely
> fashion, but I am going to specifically nack de-consting games like
> this.  There is now vcpu state corruption when the MSR is accessed
> remotely - this hook *must* remain a mutable vcpu pointer.

Would you mind enlightening me how / what vCPU state corruption
this is causing? There's no change in what struct vcpu instance is
being acted upon, after all.

> There are also multiple other functional issues and regressions
> introduced by this series.  I'm trying to put together a patch to fix
> all of the fallout, but I also might revert the series wholesale
> depending on the eventual complexity.

I'm sorry for not spotting any of them; I'm curious to learn what
I've missed.

Jan



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

Re: [Xen-devel] [PATCH v4 1/4] x86: stop handling MSR_IA32_BNDCFGS save/restore in implementation code

[Jan Beulich]

>>> On 09.04.19 at 16:38, <andrew.cooper3@citrix.com> wrote:
> On 14/03/2019 13:51, Paul Durrant wrote:
>> @@ -1215,8 +1196,11 @@ static bool vmx_set_guest_bndcfgs(struct vcpu *v, u64 val)
>>      return true;
>>  }
>>  
>> -static bool vmx_get_guest_bndcfgs(struct vcpu *v, u64 *val)
>> +static bool vmx_get_guest_bndcfgs(const struct vcpu *cv, u64 *val)
>>  {
>> +    /* Get a non-const pointer for vmx_vmcs_enter() */
>> +    struct vcpu *v = cv->domain->vcpu[cv->vcpu_id];
>> +
> 
> I'm sorry not having got around to reviewing this series in a timely
> fashion, but I am going to specifically nack de-consting games like
> this.  There is now vcpu state corruption when the MSR is accessed
> remotely - this hook *must* remain a mutable vcpu pointer.

Would you mind enlightening me how / what vCPU state corruption
this is causing? There's no change in what struct vcpu instance is
being acted upon, after all.

> There are also multiple other functional issues and regressions
> introduced by this series.  I'm trying to put together a patch to fix
> all of the fallout, but I also might revert the series wholesale
> depending on the eventual complexity.

I'm sorry for not spotting any of them; I'm curious to learn what
I've missed.

Jan



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