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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of 
> Andrew Cooper
> Sent: 15 April 2019 13:03
> To: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Jan 
> Beulich <JBeulich@xxxxxxxx>;
> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jun Nakajima 
> <jun.nakajima@xxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel