[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> On 12.08.14 at 11:44, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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@xxxxxxxx>
>>
>> --- 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.

Not really, with it being of type uint64_t. Nor does the context
really suggest a meaning like what you imply.

Jan

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.