Dietmar Hahn
2013-Mar-13 09:55 UTC
[PATCH] vpmu intel: Add cpuid handling when vpmu disabled
Even though vpmu is disabled in the hypervisor in the HVM guest the call of
cpuid(0xa) returns informations about usable performance counters.
This may confuse guest software when trying to use the counters and nothing
happens.
This patch clears most bits in registers eax and edx of cpuid(0xa) instruction
for the guest when vpmu is disabled:
- version ID of architectural performance counting
- number of general pmu registers
- width of general pmu registers
- number of fixed pmu registers
- width of ixed pmu registers
Thanks.
Dietmar.
Signed-off-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
diff -r a6b81234b189 xen/arch/x86/hvm/svm/vpmu.c
--- a/xen/arch/x86/hvm/svm/vpmu.c Mon Mar 11 16:13:42 2013 +0000
+++ b/xen/arch/x86/hvm/svm/vpmu.c Wed Mar 13 09:54:00 2013 +0100
@@ -370,6 +370,10 @@ int svm_vpmu_initialise(struct vcpu *v,
uint8_t family = current_cpu_data.x86;
int ret = 0;
+ /* vpmu enabled? */
+ if (!vpmu_flags)
+ return 0;
+
switch ( family )
{
case 0x10:
diff -r a6b81234b189 xen/arch/x86/hvm/vmx/vpmu_core2.c
--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c Mon Mar 11 16:13:42 2013 +0000
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c Wed Mar 13 09:54:00 2013 +0100
@@ -727,6 +727,59 @@ struct arch_vpmu_ops core2_vpmu_ops = {
.arch_vpmu_load = core2_vpmu_load
};
+/* cpuid 0xa - eax */
+#define X86_FEATURE_PMU_VER_OFF 0 /* Version */
+#define FEATURE_PMU_VER_BITS 8 /* 8 bits */
+#define X86_FEATURE_NUM_GEN_OFF 8 /* Number of general pmu registers */
+#define FEATURE_NUM_GEN_BITS 8 /* 8 bits */
+#define X86_FEATURE_GEN_WIDTH_OFF 16 /* Width of general pmu registers */
+#define FEATURE_GEN_WIDTH_BITS 8 /* 8 bits */
+
+/* cpuid 0xa - edx */
+#define X86_FEATURE_NUM_FIX_OFF 0 /* Number of fixed pmu registers */
+#define FEATURE_NUM_FIX_BITS 5 /* 5 bits */
+#define X86_FEATURE_FIX_WIDTH_OFF 5 /* Width of fixed pmu registers */
+#define FEATURE_FIX_WIDTH_BITS 8 /* 8 bits */
+
+static void core2_no_vpmu_do_cpuid(unsigned int input,
+ unsigned int *eax, unsigned int *ebx,
+ unsigned int *ecx, unsigned int *edx)
+{
+ /*
+ * As in this case the vpmu is not enabled reset some bits in the
+ * pmu part of the cpuid.
+ */
+ if (input == 0xa)
+ {
+ *eax &= ~(((1 << FEATURE_PMU_VER_BITS) -1) <<
X86_FEATURE_PMU_VER_OFF);
+ *eax &= ~(((1 << FEATURE_NUM_GEN_BITS) -1) <<
X86_FEATURE_NUM_GEN_OFF);
+ *eax &= ~(((1 << FEATURE_GEN_WIDTH_BITS) -1) <<
X86_FEATURE_GEN_WIDTH_OFF);
+
+ *edx &= ~(((1 << FEATURE_NUM_FIX_BITS) -1) <<
X86_FEATURE_NUM_FIX_OFF);
+ *edx &= ~(((1 << FEATURE_FIX_WIDTH_BITS) -1) <<
X86_FEATURE_FIX_WIDTH_OFF);
+ }
+}
+
+/*
+ * If it''s a vpmu msr set it to 0.
+ */
+static int core2_no_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content)
+{
+ int type = -1, index = -1;
+ if ( !is_core2_vpmu_msr(msr, &type, &index) )
+ return 0;
+ *msr_content = 0;
+ return 1;
+}
+
+/*
+ * These functions are used in case vpmu is not enabled.
+ */
+struct arch_vpmu_ops core2_no_vpmu_ops = {
+ .do_rdmsr = core2_no_vpmu_do_rdmsr,
+ .do_cpuid = core2_no_vpmu_do_cpuid,
+};
+
int vmx_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags)
{
struct vpmu_struct *vpmu = vcpu_vpmu(v);
@@ -734,6 +787,10 @@ int vmx_vpmu_initialise(struct vcpu *v,
uint8_t cpu_model = current_cpu_data.x86_model;
int ret = 0;
+ vpmu->arch_vpmu_ops = &core2_no_vpmu_ops;
+ if ( !vpmu_flags )
+ return 0;
+
if ( family == 6 )
{
switch ( cpu_model )
diff -r a6b81234b189 xen/arch/x86/hvm/vpmu.c
--- a/xen/arch/x86/hvm/vpmu.c Mon Mar 11 16:13:42 2013 +0000
+++ b/xen/arch/x86/hvm/vpmu.c Wed Mar 13 09:54:00 2013 +0100
@@ -67,7 +67,7 @@ int vpmu_do_wrmsr(unsigned int msr, uint
{
struct vpmu_struct *vpmu = vcpu_vpmu(current);
- if ( vpmu->arch_vpmu_ops )
+ if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr
)
return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content);
return 0;
}
@@ -76,7 +76,7 @@ int vpmu_do_rdmsr(unsigned int msr, uint
{
struct vpmu_struct *vpmu = vcpu_vpmu(current);
- if ( vpmu->arch_vpmu_ops )
+ if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_rdmsr
)
return vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content);
return 0;
}
@@ -85,7 +85,7 @@ int vpmu_do_interrupt(struct cpu_user_re
{
struct vpmu_struct *vpmu = vcpu_vpmu(current);
- if ( vpmu->arch_vpmu_ops )
+ if ( vpmu->arch_vpmu_ops &&
vpmu->arch_vpmu_ops->do_interrupt )
return vpmu->arch_vpmu_ops->do_interrupt(regs);
return 0;
}
@@ -104,7 +104,7 @@ void vpmu_save(struct vcpu *v)
{
struct vpmu_struct *vpmu = vcpu_vpmu(v);
- if ( vpmu->arch_vpmu_ops )
+ if ( vpmu->arch_vpmu_ops &&
vpmu->arch_vpmu_ops->arch_vpmu_save )
vpmu->arch_vpmu_ops->arch_vpmu_save(v);
}
@@ -112,7 +112,7 @@ void vpmu_load(struct vcpu *v)
{
struct vpmu_struct *vpmu = vcpu_vpmu(v);
- if ( vpmu->arch_vpmu_ops )
+ if ( vpmu->arch_vpmu_ops &&
vpmu->arch_vpmu_ops->arch_vpmu_load )
vpmu->arch_vpmu_ops->arch_vpmu_load(v);
}
@@ -121,9 +121,6 @@ void vpmu_initialise(struct vcpu *v)
struct vpmu_struct *vpmu = vcpu_vpmu(v);
uint8_t vendor = current_cpu_data.x86_vendor;
- if ( !opt_vpmu_enabled )
- return;
-
if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
vpmu_destroy(v);
vpmu_clear(vpmu);
@@ -153,7 +150,7 @@ void vpmu_destroy(struct vcpu *v)
{
struct vpmu_struct *vpmu = vcpu_vpmu(v);
- if ( vpmu->arch_vpmu_ops )
+ if ( vpmu->arch_vpmu_ops &&
vpmu->arch_vpmu_ops->arch_vpmu_destroy )
vpmu->arch_vpmu_ops->arch_vpmu_destroy(v);
}
--
Company details: http://ts.fujitsu.com/imprint.html
Dietmar Hahn
2013-Mar-20 06:40 UTC
Re: [PATCH] vpmu intel: Add cpuid handling when vpmu disabled
Am Mittwoch 13 März 2013, 10:55:29 schrieb Dietmar Hahn:> Even though vpmu is disabled in the hypervisor in the HVM guest the call of > cpuid(0xa) returns informations about usable performance counters. > This may confuse guest software when trying to use the counters and nothing > happens. > This patch clears most bits in registers eax and edx of cpuid(0xa) instruction > for the guest when vpmu is disabled: > - version ID of architectural performance counting > - number of general pmu registers > - width of general pmu registers > - number of fixed pmu registers > - width of ixed pmu registers > > Thanks. > > Dietmar.Ping?> > Signed-off-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> > > diff -r a6b81234b189 xen/arch/x86/hvm/svm/vpmu.c > --- a/xen/arch/x86/hvm/svm/vpmu.c Mon Mar 11 16:13:42 2013 +0000 > +++ b/xen/arch/x86/hvm/svm/vpmu.c Wed Mar 13 09:54:00 2013 +0100 > @@ -370,6 +370,10 @@ int svm_vpmu_initialise(struct vcpu *v, > uint8_t family = current_cpu_data.x86; > int ret = 0; > > + /* vpmu enabled? */ > + if (!vpmu_flags) > + return 0; > + > switch ( family ) > { > case 0x10: > diff -r a6b81234b189 xen/arch/x86/hvm/vmx/vpmu_core2.c > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c Mon Mar 11 16:13:42 2013 +0000 > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c Wed Mar 13 09:54:00 2013 +0100 > @@ -727,6 +727,59 @@ struct arch_vpmu_ops core2_vpmu_ops = { > .arch_vpmu_load = core2_vpmu_load > }; > > +/* cpuid 0xa - eax */ > +#define X86_FEATURE_PMU_VER_OFF 0 /* Version */ > +#define FEATURE_PMU_VER_BITS 8 /* 8 bits */ > +#define X86_FEATURE_NUM_GEN_OFF 8 /* Number of general pmu registers */ > +#define FEATURE_NUM_GEN_BITS 8 /* 8 bits */ > +#define X86_FEATURE_GEN_WIDTH_OFF 16 /* Width of general pmu registers */ > +#define FEATURE_GEN_WIDTH_BITS 8 /* 8 bits */ > + > +/* cpuid 0xa - edx */ > +#define X86_FEATURE_NUM_FIX_OFF 0 /* Number of fixed pmu registers */ > +#define FEATURE_NUM_FIX_BITS 5 /* 5 bits */ > +#define X86_FEATURE_FIX_WIDTH_OFF 5 /* Width of fixed pmu registers */ > +#define FEATURE_FIX_WIDTH_BITS 8 /* 8 bits */ > + > +static void core2_no_vpmu_do_cpuid(unsigned int input, > + unsigned int *eax, unsigned int *ebx, > + unsigned int *ecx, unsigned int *edx) > +{ > + /* > + * As in this case the vpmu is not enabled reset some bits in the > + * pmu part of the cpuid. > + */ > + if (input == 0xa) > + { > + *eax &= ~(((1 << FEATURE_PMU_VER_BITS) -1) << X86_FEATURE_PMU_VER_OFF); > + *eax &= ~(((1 << FEATURE_NUM_GEN_BITS) -1) << X86_FEATURE_NUM_GEN_OFF); > + *eax &= ~(((1 << FEATURE_GEN_WIDTH_BITS) -1) << X86_FEATURE_GEN_WIDTH_OFF); > + > + *edx &= ~(((1 << FEATURE_NUM_FIX_BITS) -1) << X86_FEATURE_NUM_FIX_OFF); > + *edx &= ~(((1 << FEATURE_FIX_WIDTH_BITS) -1) << X86_FEATURE_FIX_WIDTH_OFF); > + } > +} > + > +/* > + * If it's a vpmu msr set it to 0. > + */ > +static int core2_no_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content) > +{ > + int type = -1, index = -1; > + if ( !is_core2_vpmu_msr(msr, &type, &index) ) > + return 0; > + *msr_content = 0; > + return 1; > +} > + > +/* > + * These functions are used in case vpmu is not enabled. > + */ > +struct arch_vpmu_ops core2_no_vpmu_ops = { > + .do_rdmsr = core2_no_vpmu_do_rdmsr, > + .do_cpuid = core2_no_vpmu_do_cpuid, > +}; > + > int vmx_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags) > { > struct vpmu_struct *vpmu = vcpu_vpmu(v); > @@ -734,6 +787,10 @@ int vmx_vpmu_initialise(struct vcpu *v, > uint8_t cpu_model = current_cpu_data.x86_model; > int ret = 0; > > + vpmu->arch_vpmu_ops = &core2_no_vpmu_ops; > + if ( !vpmu_flags ) > + return 0; > + > if ( family == 6 ) > { > switch ( cpu_model ) > diff -r a6b81234b189 xen/arch/x86/hvm/vpmu.c > --- a/xen/arch/x86/hvm/vpmu.c Mon Mar 11 16:13:42 2013 +0000 > +++ b/xen/arch/x86/hvm/vpmu.c Wed Mar 13 09:54:00 2013 +0100 > @@ -67,7 +67,7 @@ int vpmu_do_wrmsr(unsigned int msr, uint > { > struct vpmu_struct *vpmu = vcpu_vpmu(current); > > - if ( vpmu->arch_vpmu_ops ) > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr ) > return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content); > return 0; > } > @@ -76,7 +76,7 @@ int vpmu_do_rdmsr(unsigned int msr, uint > { > struct vpmu_struct *vpmu = vcpu_vpmu(current); > > - if ( vpmu->arch_vpmu_ops ) > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_rdmsr ) > return vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content); > return 0; > } > @@ -85,7 +85,7 @@ int vpmu_do_interrupt(struct cpu_user_re > { > struct vpmu_struct *vpmu = vcpu_vpmu(current); > > - if ( vpmu->arch_vpmu_ops ) > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_interrupt ) > return vpmu->arch_vpmu_ops->do_interrupt(regs); > return 0; > } > @@ -104,7 +104,7 @@ void vpmu_save(struct vcpu *v) > { > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > - if ( vpmu->arch_vpmu_ops ) > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_save ) > vpmu->arch_vpmu_ops->arch_vpmu_save(v); > } > > @@ -112,7 +112,7 @@ void vpmu_load(struct vcpu *v) > { > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > - if ( vpmu->arch_vpmu_ops ) > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_load ) > vpmu->arch_vpmu_ops->arch_vpmu_load(v); > } > > @@ -121,9 +121,6 @@ void vpmu_initialise(struct vcpu *v) > struct vpmu_struct *vpmu = vcpu_vpmu(v); > uint8_t vendor = current_cpu_data.x86_vendor; > > - if ( !opt_vpmu_enabled ) > - return; > - > if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) ) > vpmu_destroy(v); > vpmu_clear(vpmu); > @@ -153,7 +150,7 @@ void vpmu_destroy(struct vcpu *v) > { > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > - if ( vpmu->arch_vpmu_ops ) > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_destroy ) > vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); > } > > >-- Company details: http://ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Konrad Rzeszutek Wilk
2013-Mar-25 14:59 UTC
Re: [PATCH] vpmu intel: Add cpuid handling when vpmu disabled
On Wed, Mar 20, 2013 at 07:40:58AM +0100, Dietmar Hahn wrote:> Am Mittwoch 13 März 2013, 10:55:29 schrieb Dietmar Hahn: > > Even though vpmu is disabled in the hypervisor in the HVM guest the call of > > cpuid(0xa) returns informations about usable performance counters. > > This may confuse guest software when trying to use the counters and nothing > > happens. > > This patch clears most bits in registers eax and edx of cpuid(0xa) instruction > > for the guest when vpmu is disabled: > > - version ID of architectural performance counting > > - number of general pmu registers > > - width of general pmu registers > > - number of fixed pmu registers > > - width of ixed pmu registers > > > > Thanks. > > > > Dietmar. > > Ping?You need to CC the AMD maintainer as well. CC-ing him here.> > > > > Signed-off-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> > > > > diff -r a6b81234b189 xen/arch/x86/hvm/svm/vpmu.c > > --- a/xen/arch/x86/hvm/svm/vpmu.c Mon Mar 11 16:13:42 2013 +0000 > > +++ b/xen/arch/x86/hvm/svm/vpmu.c Wed Mar 13 09:54:00 2013 +0100 > > @@ -370,6 +370,10 @@ int svm_vpmu_initialise(struct vcpu *v, > > uint8_t family = current_cpu_data.x86; > > int ret = 0; > > > > + /* vpmu enabled? */ > > + if (!vpmu_flags) > > + return 0; > > + > > switch ( family ) > > { > > case 0x10: > > diff -r a6b81234b189 xen/arch/x86/hvm/vmx/vpmu_core2.c > > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c Mon Mar 11 16:13:42 2013 +0000 > > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c Wed Mar 13 09:54:00 2013 +0100 > > @@ -727,6 +727,59 @@ struct arch_vpmu_ops core2_vpmu_ops = { > > .arch_vpmu_load = core2_vpmu_load > > }; > > > > +/* cpuid 0xa - eax */Do you think it might make sense to point out where these are defined in the Intel SDM?> > +#define X86_FEATURE_PMU_VER_OFF 0 /* Version */ > > +#define FEATURE_PMU_VER_BITS 8 /* 8 bits */ > > +#define X86_FEATURE_NUM_GEN_OFF 8 /* Number of general pmu registers */ > > +#define FEATURE_NUM_GEN_BITS 8 /* 8 bits */ > > +#define X86_FEATURE_GEN_WIDTH_OFF 16 /* Width of general pmu registers */ > > +#define FEATURE_GEN_WIDTH_BITS 8 /* 8 bits */ > > + > > +/* cpuid 0xa - edx */ > > +#define X86_FEATURE_NUM_FIX_OFF 0 /* Number of fixed pmu registers */ > > +#define FEATURE_NUM_FIX_BITS 5 /* 5 bits */ > > +#define X86_FEATURE_FIX_WIDTH_OFF 5 /* Width of fixed pmu registers */ > > +#define FEATURE_FIX_WIDTH_BITS 8 /* 8 bits */ > > + > > +static void core2_no_vpmu_do_cpuid(unsigned int input, > > + unsigned int *eax, unsigned int *ebx, > > + unsigned int *ecx, unsigned int *edx) > > +{ > > + /* > > + * As in this case the vpmu is not enabled reset some bits in the > > + * pmu part of the cpuid. > > + */ > > + if (input == 0xa) > > + { > > + *eax &= ~(((1 << FEATURE_PMU_VER_BITS) -1) << X86_FEATURE_PMU_VER_OFF); > > + *eax &= ~(((1 << FEATURE_NUM_GEN_BITS) -1) << X86_FEATURE_NUM_GEN_OFF); > > + *eax &= ~(((1 << FEATURE_GEN_WIDTH_BITS) -1) << X86_FEATURE_GEN_WIDTH_OFF); > > + > > + *edx &= ~(((1 << FEATURE_NUM_FIX_BITS) -1) << X86_FEATURE_NUM_FIX_OFF); > > + *edx &= ~(((1 << FEATURE_FIX_WIDTH_BITS) -1) << X86_FEATURE_FIX_WIDTH_OFF); > > + } > > +} > > + > > +/* > > + * If it''s a vpmu msr set it to 0... s/it/its value/> > + */ > > +static int core2_no_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content) > > +{ > > + int type = -1, index = -1; > > + if ( !is_core2_vpmu_msr(msr, &type, &index) ) > > + return 0; > > + *msr_content = 0; > > + return 1; > > +} > > + > > +/* > > + * These functions are used in case vpmu is not enabled. > > + */ > > +struct arch_vpmu_ops core2_no_vpmu_ops = { > > + .do_rdmsr = core2_no_vpmu_do_rdmsr, > > + .do_cpuid = core2_no_vpmu_do_cpuid, > > +}; > > + > > int vmx_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags) > > { > > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > @@ -734,6 +787,10 @@ int vmx_vpmu_initialise(struct vcpu *v, > > uint8_t cpu_model = current_cpu_data.x86_model; > > int ret = 0; > > > > + vpmu->arch_vpmu_ops = &core2_no_vpmu_ops; > > + if ( !vpmu_flags ) > > + return 0; > > + > > if ( family == 6 ) > > { > > switch ( cpu_model ) > > diff -r a6b81234b189 xen/arch/x86/hvm/vpmu.c > > --- a/xen/arch/x86/hvm/vpmu.c Mon Mar 11 16:13:42 2013 +0000 > > +++ b/xen/arch/x86/hvm/vpmu.c Wed Mar 13 09:54:00 2013 +0100 > > @@ -67,7 +67,7 @@ int vpmu_do_wrmsr(unsigned int msr, uint > > { > > struct vpmu_struct *vpmu = vcpu_vpmu(current); > > > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr ) > > return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content); > > return 0; > > } > > @@ -76,7 +76,7 @@ int vpmu_do_rdmsr(unsigned int msr, uint > > { > > struct vpmu_struct *vpmu = vcpu_vpmu(current); > > > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_rdmsr ) > > return vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content); > > return 0; > > } > > @@ -85,7 +85,7 @@ int vpmu_do_interrupt(struct cpu_user_re > > { > > struct vpmu_struct *vpmu = vcpu_vpmu(current); > > > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_interrupt ) > > return vpmu->arch_vpmu_ops->do_interrupt(regs);> > return 0; > > } > > @@ -104,7 +104,7 @@ void vpmu_save(struct vcpu *v) > > { > > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_save ) > > vpmu->arch_vpmu_ops->arch_vpmu_save(v); > > } > > > > @@ -112,7 +112,7 @@ void vpmu_load(struct vcpu *v) > > { > > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_load ) > > vpmu->arch_vpmu_ops->arch_vpmu_load(v); > > } > > > > @@ -121,9 +121,6 @@ void vpmu_initialise(struct vcpu *v) > > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > uint8_t vendor = current_cpu_data.x86_vendor; > > > > - if ( !opt_vpmu_enabled ) > > - return; > > - > > if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) ) > > vpmu_destroy(v); > > vpmu_clear(vpmu); > > @@ -153,7 +150,7 @@ void vpmu_destroy(struct vcpu *v) > > { > > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_destroy ) > > vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); > > } > > > > > > > -- > Company details: http://ts.fujitsu.com/imprint.html > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >
Konrad Rzeszutek Wilk
2013-Mar-25 14:59 UTC
Re: [PATCH] vpmu intel: Add cpuid handling when vpmu disabled
On Mon, Mar 25, 2013 at 10:59:22AM -0400, Konrad Rzeszutek Wilk wrote:> On Wed, Mar 20, 2013 at 07:40:58AM +0100, Dietmar Hahn wrote: > > Am Mittwoch 13 März 2013, 10:55:29 schrieb Dietmar Hahn: > > > Even though vpmu is disabled in the hypervisor in the HVM guest the call of > > > cpuid(0xa) returns informations about usable performance counters. > > > This may confuse guest software when trying to use the counters and nothing > > > happens. > > > This patch clears most bits in registers eax and edx of cpuid(0xa) instruction > > > for the guest when vpmu is disabled: > > > - version ID of architectural performance counting > > > - number of general pmu registers > > > - width of general pmu registers > > > - number of fixed pmu registers > > > - width of ixed pmu registers > > > > > > Thanks. > > > > > > Dietmar. > > > > Ping? > > You need to CC the AMD maintainer as well. CC-ing him here.This time doing it> > > > > > > > Signed-off-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> > > > > > > diff -r a6b81234b189 xen/arch/x86/hvm/svm/vpmu.c > > > --- a/xen/arch/x86/hvm/svm/vpmu.c Mon Mar 11 16:13:42 2013 +0000 > > > +++ b/xen/arch/x86/hvm/svm/vpmu.c Wed Mar 13 09:54:00 2013 +0100 > > > @@ -370,6 +370,10 @@ int svm_vpmu_initialise(struct vcpu *v, > > > uint8_t family = current_cpu_data.x86; > > > int ret = 0; > > > > > > + /* vpmu enabled? */ > > > + if (!vpmu_flags) > > > + return 0; > > > + > > > switch ( family ) > > > { > > > case 0x10: > > > diff -r a6b81234b189 xen/arch/x86/hvm/vmx/vpmu_core2.c > > > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c Mon Mar 11 16:13:42 2013 +0000 > > > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c Wed Mar 13 09:54:00 2013 +0100 > > > @@ -727,6 +727,59 @@ struct arch_vpmu_ops core2_vpmu_ops = { > > > .arch_vpmu_load = core2_vpmu_load > > > }; > > > > > > +/* cpuid 0xa - eax */ > > Do you think it might make sense to point out where these are defined > in the Intel SDM? > > > > +#define X86_FEATURE_PMU_VER_OFF 0 /* Version */ > > > +#define FEATURE_PMU_VER_BITS 8 /* 8 bits */ > > > +#define X86_FEATURE_NUM_GEN_OFF 8 /* Number of general pmu registers */ > > > +#define FEATURE_NUM_GEN_BITS 8 /* 8 bits */ > > > +#define X86_FEATURE_GEN_WIDTH_OFF 16 /* Width of general pmu registers */ > > > +#define FEATURE_GEN_WIDTH_BITS 8 /* 8 bits */ > > > + > > > +/* cpuid 0xa - edx */ > > > +#define X86_FEATURE_NUM_FIX_OFF 0 /* Number of fixed pmu registers */ > > > +#define FEATURE_NUM_FIX_BITS 5 /* 5 bits */ > > > +#define X86_FEATURE_FIX_WIDTH_OFF 5 /* Width of fixed pmu registers */ > > > +#define FEATURE_FIX_WIDTH_BITS 8 /* 8 bits */ > > > + > > > +static void core2_no_vpmu_do_cpuid(unsigned int input, > > > + unsigned int *eax, unsigned int *ebx, > > > + unsigned int *ecx, unsigned int *edx) > > > +{ > > > + /* > > > + * As in this case the vpmu is not enabled reset some bits in the > > > + * pmu part of the cpuid. > > > + */ > > > + if (input == 0xa) > > > + { > > > + *eax &= ~(((1 << FEATURE_PMU_VER_BITS) -1) << X86_FEATURE_PMU_VER_OFF); > > > + *eax &= ~(((1 << FEATURE_NUM_GEN_BITS) -1) << X86_FEATURE_NUM_GEN_OFF); > > > + *eax &= ~(((1 << FEATURE_GEN_WIDTH_BITS) -1) << X86_FEATURE_GEN_WIDTH_OFF); > > > + > > > + *edx &= ~(((1 << FEATURE_NUM_FIX_BITS) -1) << X86_FEATURE_NUM_FIX_OFF); > > > + *edx &= ~(((1 << FEATURE_FIX_WIDTH_BITS) -1) << X86_FEATURE_FIX_WIDTH_OFF); > > > + } > > > +} > > > + > > > +/* > > > + * If it''s a vpmu msr set it to 0. > > .. s/it/its value/ > > > > + */ > > > +static int core2_no_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content) > > > +{ > > > + int type = -1, index = -1; > > > + if ( !is_core2_vpmu_msr(msr, &type, &index) ) > > > + return 0; > > > + *msr_content = 0; > > > + return 1; > > > +} > > > + > > > +/* > > > + * These functions are used in case vpmu is not enabled. > > > + */ > > > +struct arch_vpmu_ops core2_no_vpmu_ops = { > > > + .do_rdmsr = core2_no_vpmu_do_rdmsr, > > > + .do_cpuid = core2_no_vpmu_do_cpuid, > > > +}; > > > + > > > int vmx_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags) > > > { > > > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > > @@ -734,6 +787,10 @@ int vmx_vpmu_initialise(struct vcpu *v, > > > uint8_t cpu_model = current_cpu_data.x86_model; > > > int ret = 0; > > > > > > + vpmu->arch_vpmu_ops = &core2_no_vpmu_ops; > > > + if ( !vpmu_flags ) > > > + return 0; > > > + > > > if ( family == 6 ) > > > { > > > switch ( cpu_model ) > > > diff -r a6b81234b189 xen/arch/x86/hvm/vpmu.c > > > --- a/xen/arch/x86/hvm/vpmu.c Mon Mar 11 16:13:42 2013 +0000 > > > +++ b/xen/arch/x86/hvm/vpmu.c Wed Mar 13 09:54:00 2013 +0100 > > > @@ -67,7 +67,7 @@ int vpmu_do_wrmsr(unsigned int msr, uint > > > { > > > struct vpmu_struct *vpmu = vcpu_vpmu(current); > > > > > > - if ( vpmu->arch_vpmu_ops ) > > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr ) > > > return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content); > > > return 0; > > > } > > > @@ -76,7 +76,7 @@ int vpmu_do_rdmsr(unsigned int msr, uint > > > { > > > struct vpmu_struct *vpmu = vcpu_vpmu(current); > > > > > > - if ( vpmu->arch_vpmu_ops ) > > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_rdmsr ) > > > return vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content); > > > return 0; > > > } > > > @@ -85,7 +85,7 @@ int vpmu_do_interrupt(struct cpu_user_re > > > { > > > struct vpmu_struct *vpmu = vcpu_vpmu(current); > > > > > > - if ( vpmu->arch_vpmu_ops ) > > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_interrupt ) > > > return vpmu->arch_vpmu_ops->do_interrupt(regs); > > > > return 0; > > > } > > > @@ -104,7 +104,7 @@ void vpmu_save(struct vcpu *v) > > > { > > > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > > > > > - if ( vpmu->arch_vpmu_ops ) > > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_save ) > > > vpmu->arch_vpmu_ops->arch_vpmu_save(v); > > > } > > > > > > @@ -112,7 +112,7 @@ void vpmu_load(struct vcpu *v) > > > { > > > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > > > > > - if ( vpmu->arch_vpmu_ops ) > > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_load ) > > > vpmu->arch_vpmu_ops->arch_vpmu_load(v); > > > } > > > > > > @@ -121,9 +121,6 @@ void vpmu_initialise(struct vcpu *v) > > > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > > uint8_t vendor = current_cpu_data.x86_vendor; > > > > > > - if ( !opt_vpmu_enabled ) > > > - return; > > > - > > > if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) ) > > > vpmu_destroy(v); > > > vpmu_clear(vpmu); > > > @@ -153,7 +150,7 @@ void vpmu_destroy(struct vcpu *v) > > > { > > > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > > > > > - if ( vpmu->arch_vpmu_ops ) > > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_destroy ) > > > vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); > > > } > > > > > > > > > > > -- > > Company details: http://ts.fujitsu.com/imprint.html > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel > >
Suravee Suthikulanit
2013-Mar-25 19:41 UTC
Re: [PATCH] vpmu intel: Add cpuid handling when vpmu disabled
On 3/25/2013 9:59 AM, Konrad Rzeszutek Wilk wrote:> On Mon, Mar 25, 2013 at 10:59:22AM -0400, Konrad Rzeszutek Wilk wrote: >> On Wed, Mar 20, 2013 at 07:40:58AM +0100, Dietmar Hahn wrote: >>> Am Mittwoch 13 März 2013, 10:55:29 schrieb Dietmar Hahn: >>>> Even though vpmu is disabled in the hypervisor in the HVM guest the call of >>>> cpuid(0xa) returns informations about usable performance counters. >>>> This may confuse guest software when trying to use the counters and nothing >>>> happens. >>>> This patch clears most bits in registers eax and edx of cpuid(0xa) instruction >>>> for the guest when vpmu is disabled: >>>> - version ID of architectural performance counting >>>> - number of general pmu registers >>>> - width of general pmu registers >>>> - number of fixed pmu registers >>>> - width of ixed pmu registers >>>> >>>> Thanks. >>>> >>>> Dietmar. >>> Ping? >> You need to CC the AMD maintainer as well. CC-ing him here. > This time doing itThis looks fine. AMD code doesn''t currently use the vpmu_flags in the AMD-specific code. Suravee>>>> Signed-off-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com> >>>> >>>> diff -r a6b81234b189 xen/arch/x86/hvm/svm/vpmu.c >>>> --- a/xen/arch/x86/hvm/svm/vpmu.c Mon Mar 11 16:13:42 2013 +0000 >>>> +++ b/xen/arch/x86/hvm/svm/vpmu.c Wed Mar 13 09:54:00 2013 +0100 >>>> @@ -370,6 +370,10 @@ int svm_vpmu_initialise(struct vcpu *v, >>>> uint8_t family = current_cpu_data.x86; >>>> int ret = 0; >>>> >>>> + /* vpmu enabled? */ >>>> + if (!vpmu_flags) >>>> + return 0; >>>> + >>>> switch ( family ) >>>> { >>>> case 0x10: >>>> diff -r a6b81234b189 xen/arch/x86/hvm/vmx/vpmu_core2.c >>>> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c Mon Mar 11 16:13:42 2013 +0000 >>>> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c Wed Mar 13 09:54:00 2013 +0100 >>>> @@ -727,6 +727,59 @@ struct arch_vpmu_ops core2_vpmu_ops = { >>>> .arch_vpmu_load = core2_vpmu_load >>>> }; >>>> >>>> +/* cpuid 0xa - eax */ >> Do you think it might make sense to point out where these are defined >> in the Intel SDM? >> >>>> +#define X86_FEATURE_PMU_VER_OFF 0 /* Version */ >>>> +#define FEATURE_PMU_VER_BITS 8 /* 8 bits */ >>>> +#define X86_FEATURE_NUM_GEN_OFF 8 /* Number of general pmu registers */ >>>> +#define FEATURE_NUM_GEN_BITS 8 /* 8 bits */ >>>> +#define X86_FEATURE_GEN_WIDTH_OFF 16 /* Width of general pmu registers */ >>>> +#define FEATURE_GEN_WIDTH_BITS 8 /* 8 bits */ >>>> + >>>> +/* cpuid 0xa - edx */ >>>> +#define X86_FEATURE_NUM_FIX_OFF 0 /* Number of fixed pmu registers */ >>>> +#define FEATURE_NUM_FIX_BITS 5 /* 5 bits */ >>>> +#define X86_FEATURE_FIX_WIDTH_OFF 5 /* Width of fixed pmu registers */ >>>> +#define FEATURE_FIX_WIDTH_BITS 8 /* 8 bits */ >>>> + >>>> +static void core2_no_vpmu_do_cpuid(unsigned int input, >>>> + unsigned int *eax, unsigned int *ebx, >>>> + unsigned int *ecx, unsigned int *edx) >>>> +{ >>>> + /* >>>> + * As in this case the vpmu is not enabled reset some bits in the >>>> + * pmu part of the cpuid. >>>> + */ >>>> + if (input == 0xa) >>>> + { >>>> + *eax &= ~(((1 << FEATURE_PMU_VER_BITS) -1) << X86_FEATURE_PMU_VER_OFF); >>>> + *eax &= ~(((1 << FEATURE_NUM_GEN_BITS) -1) << X86_FEATURE_NUM_GEN_OFF); >>>> + *eax &= ~(((1 << FEATURE_GEN_WIDTH_BITS) -1) << X86_FEATURE_GEN_WIDTH_OFF); >>>> + >>>> + *edx &= ~(((1 << FEATURE_NUM_FIX_BITS) -1) << X86_FEATURE_NUM_FIX_OFF); >>>> + *edx &= ~(((1 << FEATURE_FIX_WIDTH_BITS) -1) << X86_FEATURE_FIX_WIDTH_OFF); >>>> + } >>>> +} >>>> + >>>> +/* >>>> + * If it''s a vpmu msr set it to 0. >> .. s/it/its value/ >> >>>> + */ >>>> +static int core2_no_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content) >>>> +{ >>>> + int type = -1, index = -1; >>>> + if ( !is_core2_vpmu_msr(msr, &type, &index) ) >>>> + return 0; >>>> + *msr_content = 0; >>>> + return 1; >>>> +} >>>> + >>>> +/* >>>> + * These functions are used in case vpmu is not enabled. >>>> + */ >>>> +struct arch_vpmu_ops core2_no_vpmu_ops = { >>>> + .do_rdmsr = core2_no_vpmu_do_rdmsr, >>>> + .do_cpuid = core2_no_vpmu_do_cpuid, >>>> +}; >>>> + >>>> int vmx_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags) >>>> { >>>> struct vpmu_struct *vpmu = vcpu_vpmu(v); >>>> @@ -734,6 +787,10 @@ int vmx_vpmu_initialise(struct vcpu *v, >>>> uint8_t cpu_model = current_cpu_data.x86_model; >>>> int ret = 0; >>>> >>>> + vpmu->arch_vpmu_ops = &core2_no_vpmu_ops; >>>> + if ( !vpmu_flags ) >>>> + return 0; >>>> + >>>> if ( family == 6 ) >>>> { >>>> switch ( cpu_model ) >>>> diff -r a6b81234b189 xen/arch/x86/hvm/vpmu.c >>>> --- a/xen/arch/x86/hvm/vpmu.c Mon Mar 11 16:13:42 2013 +0000 >>>> +++ b/xen/arch/x86/hvm/vpmu.c Wed Mar 13 09:54:00 2013 +0100 >>>> @@ -67,7 +67,7 @@ int vpmu_do_wrmsr(unsigned int msr, uint >>>> { >>>> struct vpmu_struct *vpmu = vcpu_vpmu(current); >>>> >>>> - if ( vpmu->arch_vpmu_ops ) >>>> + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr ) >>>> return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content); >>>> return 0; >>>> } >>>> @@ -76,7 +76,7 @@ int vpmu_do_rdmsr(unsigned int msr, uint >>>> { >>>> struct vpmu_struct *vpmu = vcpu_vpmu(current); >>>> >>>> - if ( vpmu->arch_vpmu_ops ) >>>> + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_rdmsr ) >>>> return vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content); >>>> return 0; >>>> } >>>> @@ -85,7 +85,7 @@ int vpmu_do_interrupt(struct cpu_user_re >>>> { >>>> struct vpmu_struct *vpmu = vcpu_vpmu(current); >>>> >>>> - if ( vpmu->arch_vpmu_ops ) >>>> + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_interrupt ) >>>> return vpmu->arch_vpmu_ops->do_interrupt(regs); >>>> return 0; >>>> } >>>> @@ -104,7 +104,7 @@ void vpmu_save(struct vcpu *v) >>>> { >>>> struct vpmu_struct *vpmu = vcpu_vpmu(v); >>>> >>>> - if ( vpmu->arch_vpmu_ops ) >>>> + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_save ) >>>> vpmu->arch_vpmu_ops->arch_vpmu_save(v); >>>> } >>>> >>>> @@ -112,7 +112,7 @@ void vpmu_load(struct vcpu *v) >>>> { >>>> struct vpmu_struct *vpmu = vcpu_vpmu(v); >>>> >>>> - if ( vpmu->arch_vpmu_ops ) >>>> + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_load ) >>>> vpmu->arch_vpmu_ops->arch_vpmu_load(v); >>>> } >>>> >>>> @@ -121,9 +121,6 @@ void vpmu_initialise(struct vcpu *v) >>>> struct vpmu_struct *vpmu = vcpu_vpmu(v); >>>> uint8_t vendor = current_cpu_data.x86_vendor; >>>> >>>> - if ( !opt_vpmu_enabled ) >>>> - return; >>>> - >>>> if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) ) >>>> vpmu_destroy(v); >>>> vpmu_clear(vpmu); >>>> @@ -153,7 +150,7 @@ void vpmu_destroy(struct vcpu *v) >>>> { >>>> struct vpmu_struct *vpmu = vcpu_vpmu(v); >>>> >>>> - if ( vpmu->arch_vpmu_ops ) >>>> + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_destroy ) >>>> vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); >>>> } >>>> >>>> >>>> >>> -- >>> Company details: http://ts.fujitsu.com/imprint.html >>> >>> _______________________________________________ >>> Xen-devel mailing list >>> Xen-devel@lists.xen.org >>> http://lists.xen.org/xen-devel >>>