[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 08/12/2014 05:15 AM, 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);
Can we move this if statement out of VPMU code into
vmx_msr_write_intercept()? There is not a whole lot of VPMU-specific
logic here and I am not sure I see much reason to go through
vendor-independent code (vpmu.c) to do something that is very much
Intel-specific.
You will need to start using vpmu_is_set() there but the upside is that
there is no need to introduce another parameter that does nothing on AMD
(and even on Intel is useful only for this particular register)
Or maybe add another Intel-specific routine in vpmu_core2.c to handle
this register?
-boris
}
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);
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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
