Re: [PATCH 2/4] VMX/vPMU: fix DebugCtl MSR handling

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

[-- Attachment #1.1: Type: text/plain, Size: 6184 bytes --]

On 12/08/14 10:15, Jan Beulich wrote:
> - writes with one of the vPMU-supported flags and some unsupported one
>   set got accepted, leading to a VM entry failure
> - writes with one of the vPMU-supported flags set but the Debug Store
>   feature unavailable produced a #GP (other than other writes to this
>   MSR with unsupported bits set)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1789,7 +1789,7 @@ static int svm_msr_write_intercept(unsig
>      case MSR_AMD_FAM15H_EVNTSEL3:
>      case MSR_AMD_FAM15H_EVNTSEL4:
>      case MSR_AMD_FAM15H_EVNTSEL5:
> -        vpmu_do_wrmsr(msr, msr_content);
> +        vpmu_do_wrmsr(msr, msr_content, 0);
>          break;
>  
>      case MSR_IA32_MCx_MISC(4): /* Threshold register */
> --- a/xen/arch/x86/hvm/svm/vpmu.c
> +++ b/xen/arch/x86/hvm/svm/vpmu.c
> @@ -278,11 +278,14 @@ static void context_update(unsigned int 
>      }
>  }
>  
> -static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
> +static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
> +                             uint64_t supported)
>  {
>      struct vcpu *v = current;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>  
> +    ASSERT(!supported);
> +
>      /* For all counters, enable guest only mode for HVM guest */
>      if ( (get_pmu_reg_type(msr) == MSR_TYPE_CTRL) &&
>          !(is_guest_mode(msr_content)) )
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2249,7 +2249,7 @@ static int vmx_msr_write_intercept(unsig
>          if ( msr_content & ~supported )
>          {
>              /* Perhaps some other bits are supported in vpmu. */
> -            if ( !vpmu_do_wrmsr(msr, msr_content) )
> +            if ( !vpmu_do_wrmsr(msr, msr_content, supported) )
>                  break;
>          }
>          if ( msr_content & IA32_DEBUGCTLMSR_LBR )
> @@ -2278,7 +2278,7 @@ static int vmx_msr_write_intercept(unsig
>              goto gp_fault;
>          break;
>      default:
> -        if ( vpmu_do_wrmsr(msr, msr_content) )
> +        if ( vpmu_do_wrmsr(msr, msr_content, 0) )
>              return X86EMUL_OKAY;
>          if ( passive_domain_do_wrmsr(msr, msr_content) )
>              return X86EMUL_OKAY;
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -454,7 +454,8 @@ static int core2_vpmu_msr_common_check(u
>      return 1;
>  }
>  
> -static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
> +static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
> +                               uint64_t supported)
>  {
>      u64 global_ctrl, non_global_ctrl;
>      char pmu_enable = 0;
> @@ -469,24 +470,26 @@ static int core2_vpmu_do_wrmsr(unsigned 
>          /* Special handling for BTS */
>          if ( msr == MSR_IA32_DEBUGCTLMSR )
>          {
> -            uint64_t supported = IA32_DEBUGCTLMSR_TR | IA32_DEBUGCTLMSR_BTS |
> -                                 IA32_DEBUGCTLMSR_BTINT;
> +            supported |= IA32_DEBUGCTLMSR_TR | IA32_DEBUGCTLMSR_BTS |
> +                         IA32_DEBUGCTLMSR_BTINT;
>  
>              if ( cpu_has(&current_cpu_data, X86_FEATURE_DSCPL) )
>                  supported |= IA32_DEBUGCTLMSR_BTS_OFF_OS |
>                               IA32_DEBUGCTLMSR_BTS_OFF_USR;
> -            if ( msr_content & supported )
> -            {
> -                if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> -                    return 1;
> -                gdprintk(XENLOG_WARNING, "Debug Store is not supported on this cpu\n");
> -                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> -                return 0;
> -            }
> +            if ( !(msr_content & ~supported) &&
> +                 vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> +                return 1;
> +            if ( (msr_content & supported) &&
> +                 !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
> +                printk(XENLOG_G_WARNING
> +                       "%pv: Debug Store unsupported on this CPU\n",
> +                       current);
>          }
>          return 0;
>      }
>  
> +    ASSERT(!supported);
> +
>      core2_vpmu_cxt = vpmu->context;
>      switch ( msr )
>      {
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -64,12 +64,12 @@ static void __init parse_vpmu_param(char
>      }
>  }
>  
> -int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
> +int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported)
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(current);
>  
>      if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr )
> -        return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content);
> +        return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content, supported);
>      return 0;
>  }
>  
> --- a/xen/include/asm-x86/hvm/vpmu.h
> +++ b/xen/include/asm-x86/hvm/vpmu.h
> @@ -45,7 +45,8 @@
>  
>  /* Arch specific operations shared by all vpmus */
>  struct arch_vpmu_ops {
> -    int (*do_wrmsr)(unsigned int msr, uint64_t msr_content);
> +    int (*do_wrmsr)(unsigned int msr, uint64_t msr_content,
> +                    uint64_t supported);
>      int (*do_rdmsr)(unsigned int msr, uint64_t *msr_content);
>      int (*do_interrupt)(struct cpu_user_regs *regs);
>      void (*do_cpuid)(unsigned int input,
> @@ -86,7 +87,7 @@ struct vpmu_struct {
>  #define vpmu_is_set(_vpmu, _x) ((_vpmu)->flags & (_x))
>  #define vpmu_clear(_vpmu)      ((_vpmu)->flags = 0)
>  
> -int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content);
> +int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported);

It might, for clarity sake, be preferable to name the new parameter
"supported_bits".  At the moment, "supported" could equally well refer
to the MSR itself.

~Andrew

>  int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content);
>  int vpmu_do_interrupt(struct cpu_user_regs *regs);
>  void vpmu_do_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 6857 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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