|
[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(¤t_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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |