[Xen-devel] [PATCH v2] x86/msr: Fix fallout from mostly c/s 832c180

Andrew Cooper posted 1 patch 4 years, 11 months ago
Failed in applying to current master (apply log)
xen/arch/x86/hvm/vmx/vmx.c     |  5 +----
xen/arch/x86/msr.c             | 18 +++++-------------
xen/arch/x86/pv/emul-priv-op.c |  2 +-
xen/include/asm-x86/hvm/hvm.h  |  5 +++--
xen/include/asm-x86/msr.h      | 12 ++++++------
5 files changed, 16 insertions(+), 26 deletions(-)
[Xen-devel] [PATCH v2] x86/msr: Fix fallout from mostly c/s 832c180
Posted by Andrew Cooper 4 years, 11 months ago
 * Fix the shim build by providing a !CONFIG_HVM declaration for
   hvm_get_guest_bndcfgs(), and removing the introduced
   ASSERT(is_hvm_domain(d))'s.  They are needed for DCE to keep the build
   working.  Furthermore, in this way, the risk of runtime type confusion is
   removed.
 * Revert the de-const'ing of the vcpu pointer in vmx_get_guest_bndcfgs().
   vmx_vmcs_enter() really does mutate the vcpu, and may cause it to undergo a
   full de/reschedule, which is in violation of the ABI described by
   hvm_get_guest_bndcfgs().  guest_rdmsr() was always going to need to lose
   its const parameter, and this was the correct time for it to happen.
 * The MSRs in vcpu_msrs are in numeric order.  Re-position XSS to match.

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

v2:
 * Rephrase the commit message
---
 xen/arch/x86/hvm/vmx/vmx.c     |  5 +----
 xen/arch/x86/msr.c             | 18 +++++-------------
 xen/arch/x86/pv/emul-priv-op.c |  2 +-
 xen/include/asm-x86/hvm/hvm.h  |  5 +++--
 xen/include/asm-x86/msr.h      | 12 ++++++------
 5 files changed, 16 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c46e05b..283eb7b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1150,11 +1150,8 @@ static bool vmx_set_guest_bndcfgs(struct vcpu *v, u64 val)
     return true;
 }
 
-static bool vmx_get_guest_bndcfgs(const struct vcpu *cv, u64 *val)
+static bool vmx_get_guest_bndcfgs(struct vcpu *v, 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 815d599..0049a73 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -115,7 +115,7 @@ int init_vcpu_msr_policy(struct vcpu *v)
     return 0;
 }
 
-int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
+int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
 {
     const struct vcpu *curr = current;
     const struct domain *d = v->domain;
@@ -182,13 +182,9 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         break;
 
     case MSR_IA32_BNDCFGS:
-        if ( !cp->feat.mpx )
+        if ( !cp->feat.mpx || !is_hvm_domain(d) ||
+             !hvm_get_guest_bndcfgs(v, val) )
             goto gp_fault;
-
-        ASSERT(is_hvm_domain(d));
-        if (!hvm_get_guest_bndcfgs(v, val) )
-            goto gp_fault;
-
         break;
 
     case MSR_IA32_XSS:
@@ -375,13 +371,9 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         break;
 
     case MSR_IA32_BNDCFGS:
-        if ( !cp->feat.mpx )
+        if ( !cp->feat.mpx || !is_hvm_domain(d) ||
+             !hvm_set_guest_bndcfgs(v, val) )
             goto gp_fault;
-
-        ASSERT(is_hvm_domain(d));
-        if ( !hvm_set_guest_bndcfgs(v, val) )
-            goto gp_fault;
-
         break;
 
     case MSR_IA32_XSS:
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index a55a400..af74f50 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -819,7 +819,7 @@ static inline bool is_cpufreq_controller(const struct domain *d)
 static int read_msr(unsigned int reg, uint64_t *val,
                     struct x86_emulate_ctxt *ctxt)
 {
-    const struct vcpu *curr = current;
+    struct vcpu *curr = current;
     const struct domain *currd = curr->domain;
     bool vpmu_msr = false;
     int ret;
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index c811fa9..157f0de 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -145,7 +145,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)(const struct vcpu *v, u64 *);
+    bool (*get_guest_bndcfgs)(struct vcpu *v, u64 *);
     bool (*set_guest_bndcfgs)(struct vcpu *v, u64);
 
     void (*set_tsc_offset)(struct vcpu *v, u64 offset, u64 at_tsc);
@@ -444,7 +444,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(const struct vcpu *v, u64 *val)
+static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val)
 {
     return hvm_funcs.get_guest_bndcfgs &&
            hvm_funcs.get_guest_bndcfgs(v, val);
@@ -692,6 +692,7 @@ unsigned long hvm_get_shadow_gs_base(struct vcpu *v);
 void hvm_set_info_guest(struct vcpu *v);
 void hvm_cpuid_policy_changed(struct vcpu *v);
 void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, uint64_t at_tsc);
+bool hvm_get_guest_bndcfgs(struct vcpu *v, uint64_t *val);
 
 /* End of prototype list */
 
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 0d52c08..3cbbc65 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -296,6 +296,11 @@ struct vcpu_msrs
         };
     } misc_features_enables;
 
+    /* 0x00000da0 - MSR_IA32_XSS */
+    struct {
+        uint64_t raw;
+    } xss;
+
     /*
      * 0xc0000103 - MSR_TSC_AUX
      *
@@ -313,11 +318,6 @@ 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);
@@ -333,7 +333,7 @@ int init_vcpu_msr_policy(struct vcpu *v);
  * These functions are also used by the migration logic, so need to cope with
  * being used outside of v's context.
  */
-int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val);
+int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val);
 int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val);
 
 #endif /* !__ASSEMBLY__ */
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/msr: Fix fallout from mostly c/s 832c180
Posted by Tian, Kevin 4 years, 11 months ago
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Monday, April 15, 2019 8:03 PM
> 
>  * Fix the shim build by providing a !CONFIG_HVM declaration for
>    hvm_get_guest_bndcfgs(), and removing the introduced
>    ASSERT(is_hvm_domain(d))'s.  They are needed for DCE to keep the build
>    working.  Furthermore, in this way, the risk of runtime type confusion is
>    removed.
>  * Revert the de-const'ing of the vcpu pointer in vmx_get_guest_bndcfgs().
>    vmx_vmcs_enter() really does mutate the vcpu, and may cause it to undergo
> a
>    full de/reschedule, which is in violation of the ABI described by
>    hvm_get_guest_bndcfgs().  guest_rdmsr() was always going to need to lose
>    its const parameter, and this was the correct time for it to happen.
>  * The MSRs in vcpu_msrs are in numeric order.  Re-position XSS to match.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/msr: Fix fallout from mostly c/s 832c180
Posted by Paul Durrant 4 years, 11 months ago
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Andrew Cooper
> Sent: 15 April 2019 13:03
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Kevin Tian <kevin.tian@intel.com>; Wei Liu <wei.liu2@citrix.com>; Jan Beulich <JBeulich@suse.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Roger Pau Monne
> <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH v2] x86/msr: Fix fallout from mostly c/s 832c180
> 
>  * Fix the shim build by providing a !CONFIG_HVM declaration for
>    hvm_get_guest_bndcfgs(), and removing the introduced
>    ASSERT(is_hvm_domain(d))'s.  They are needed for DCE to keep the build
>    working.  Furthermore, in this way, the risk of runtime type confusion is
>    removed.
>  * Revert the de-const'ing of the vcpu pointer in vmx_get_guest_bndcfgs().
>    vmx_vmcs_enter() really does mutate the vcpu, and may cause it to undergo a
>    full de/reschedule, which is in violation of the ABI described by
>    hvm_get_guest_bndcfgs().  guest_rdmsr() was always going to need to lose
>    its const parameter, and this was the correct time for it to happen.
>  * The MSRs in vcpu_msrs are in numeric order.  Re-position XSS to match.

My R-b was actually contingent on a comment in the header above the sruct to state the desire for the MSRs to be in numeric order.

  Paul

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> 
> v2:
>  * Rephrase the commit message
> ---
>  xen/arch/x86/hvm/vmx/vmx.c     |  5 +----
>  xen/arch/x86/msr.c             | 18 +++++-------------
>  xen/arch/x86/pv/emul-priv-op.c |  2 +-
>  xen/include/asm-x86/hvm/hvm.h  |  5 +++--
>  xen/include/asm-x86/msr.h      | 12 ++++++------
>  5 files changed, 16 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index c46e05b..283eb7b 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1150,11 +1150,8 @@ static bool vmx_set_guest_bndcfgs(struct vcpu *v, u64 val)
>      return true;
>  }
> 
> -static bool vmx_get_guest_bndcfgs(const struct vcpu *cv, u64 *val)
> +static bool vmx_get_guest_bndcfgs(struct vcpu *v, 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 815d599..0049a73 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -115,7 +115,7 @@ int init_vcpu_msr_policy(struct vcpu *v)
>      return 0;
>  }
> 
> -int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
> +int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>  {
>      const struct vcpu *curr = current;
>      const struct domain *d = v->domain;
> @@ -182,13 +182,9 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          break;
> 
>      case MSR_IA32_BNDCFGS:
> -        if ( !cp->feat.mpx )
> +        if ( !cp->feat.mpx || !is_hvm_domain(d) ||
> +             !hvm_get_guest_bndcfgs(v, val) )
>              goto gp_fault;
> -
> -        ASSERT(is_hvm_domain(d));
> -        if (!hvm_get_guest_bndcfgs(v, val) )
> -            goto gp_fault;
> -
>          break;
> 
>      case MSR_IA32_XSS:
> @@ -375,13 +371,9 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>          break;
> 
>      case MSR_IA32_BNDCFGS:
> -        if ( !cp->feat.mpx )
> +        if ( !cp->feat.mpx || !is_hvm_domain(d) ||
> +             !hvm_set_guest_bndcfgs(v, val) )
>              goto gp_fault;
> -
> -        ASSERT(is_hvm_domain(d));
> -        if ( !hvm_set_guest_bndcfgs(v, val) )
> -            goto gp_fault;
> -
>          break;
> 
>      case MSR_IA32_XSS:
> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
> index a55a400..af74f50 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -819,7 +819,7 @@ static inline bool is_cpufreq_controller(const struct domain *d)
>  static int read_msr(unsigned int reg, uint64_t *val,
>                      struct x86_emulate_ctxt *ctxt)
>  {
> -    const struct vcpu *curr = current;
> +    struct vcpu *curr = current;
>      const struct domain *currd = curr->domain;
>      bool vpmu_msr = false;
>      int ret;
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index c811fa9..157f0de 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -145,7 +145,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)(const struct vcpu *v, u64 *);
> +    bool (*get_guest_bndcfgs)(struct vcpu *v, u64 *);
>      bool (*set_guest_bndcfgs)(struct vcpu *v, u64);
> 
>      void (*set_tsc_offset)(struct vcpu *v, u64 offset, u64 at_tsc);
> @@ -444,7 +444,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(const struct vcpu *v, u64 *val)
> +static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val)
>  {
>      return hvm_funcs.get_guest_bndcfgs &&
>             hvm_funcs.get_guest_bndcfgs(v, val);
> @@ -692,6 +692,7 @@ unsigned long hvm_get_shadow_gs_base(struct vcpu *v);
>  void hvm_set_info_guest(struct vcpu *v);
>  void hvm_cpuid_policy_changed(struct vcpu *v);
>  void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, uint64_t at_tsc);
> +bool hvm_get_guest_bndcfgs(struct vcpu *v, uint64_t *val);
> 
>  /* End of prototype list */
> 
> diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
> index 0d52c08..3cbbc65 100644
> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -296,6 +296,11 @@ struct vcpu_msrs
>          };
>      } misc_features_enables;
> 
> +    /* 0x00000da0 - MSR_IA32_XSS */
> +    struct {
> +        uint64_t raw;
> +    } xss;
> +
>      /*
>       * 0xc0000103 - MSR_TSC_AUX
>       *
> @@ -313,11 +318,6 @@ 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);
> @@ -333,7 +333,7 @@ int init_vcpu_msr_policy(struct vcpu *v);
>   * These functions are also used by the migration logic, so need to cope with
>   * being used outside of v's context.
>   */
> -int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val);
> +int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val);
>  int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val);
> 
>  #endif /* !__ASSEMBLY__ */
> --
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/msr: Fix fallout from mostly c/s 832c180
Posted by Jan Beulich 4 years, 11 months ago
>>> Andrew Cooper <andrew.cooper3@citrix.com> 04/15/19 2:03 PM >>>
 >* Fix the shim build by providing a !CONFIG_HVM declaration for
>hvm_get_guest_bndcfgs(), and removing the introduced
>ASSERT(is_hvm_domain(d))'s.  They are needed for DCE to keep the build
>working.  Furthermore, in this way, the risk of runtime type confusion is
>removed.
>* Revert the de-const'ing of the vcpu pointer in vmx_get_guest_bndcfgs().
>vmx_vmcs_enter() really does mutate the vcpu, and may cause it to undergo a
>full de/reschedule, which is in violation of the ABI described by
>hvm_get_guest_bndcfgs().  guest_rdmsr() was always going to need to lose
>its const parameter, and this was the correct time for it to happen.

With the ABI violation claim here removed (e.g. replaced by saying that it
might be confusing to the reader)

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

This wrong (at least I continue to be of the opinion that it is) claim - just to
make it explicit - is what I've previously called being factual incorrect on the
v1 thread. As said before, there's no "ABI promise" to be derived from "const".

In fact I'd prefer VMCS reads in general to be viewed as reading "state", not
memory, and it just so happens that in certain cases some syncing between
memory and CPU needs to be done for correct "state" to be returned. This
doesn't alter what the caller sees (i.e. there's no change to "state", and any
locks acquired get released again before returning). I'd like to re-emphasize
the similarity to C++'s mutable here.


Jan


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