Re: [Xen-devel] [PATCH] x86/vpmu: fix vpmu can't enabled in guest

[Kang, Luwei]

> On 14/02/17 02:19, Luwei Kang wrote:
> > vpmu_enable() can not use for check if vpmu is enabled in guest during
> > booting up. Because linux kernel get the status of if supported pmu is
> > earler than xen vpmu initialise. vpmu_enable() will return false even
> > if vpmu has been enabled in guest.
> 
> Sorry for breaking this.  However ...
> 
> > diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index
> > e0a387e..b63c5d8 100644
> > --- a/xen/arch/x86/cpuid.c
> > +++ b/xen/arch/x86/cpuid.c
> > @@ -713,8 +713,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, 
> > struct cpuid_leaf *res)
> >  }
> >  }
> >
> > -if ( vpmu_enabled(curr) &&
> > - vpmu_is_set(vcpu_vpmu(curr), VPMU_CPU_HAS_DS) )
> > +if ( opt_vpmu_enabled && boot_cpu_has(X86_FEATURE_DS) )
> 
> ... this is overly general.  The visibility of these flags must be per 
> domain, and not globally like this.
> 
> In particular, XENPMU_MODE_ALL needs to expose PMU to dom0, but hide it from 
> all other domains, while even in the
> XENPMU_MODE_SELF case, only domains explicitly configured with PMU should see 
> it (as it generally unsafe to migrate with).
> 
> Longterm (with the inclusion of the CPUID improvements), my plan was to have 
> PMU available in the max policy but hidden in the
> default policy, which requires the toolstack to explicitly opt in for 
> domains.  It would opt in/out by setting the version field in guests
> CPUID policy and the other feature bits.
> 
Get it. Thanks for your explanation.

Thanks,
Luwei Kang

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vpmu: fix vpmu can't enabled in guest

[Kang, Luwei]

> >>> On 14.02.17 at 03:19,  wrote:
> > vpmu_enable() can not use for check if vpmu is enabled in guest during
> > booting up. Because linux kernel get the status of if supported pmu is
> > earler than xen vpmu initialise. vpmu_enable() will return false even
> > if vpmu has been enabled in guest.
> >
> > Signed-off-by: Luwei Kang 
> 
> You've probably seen Boris' patch with the same overall goal. While his looks 
> to leave things a little too strict, yours looks to be widening
> things a little too much. Do both of you think we could find a middle ground, 
> or do we need to accept the effect of possibly misleading
> the guest by surfacing the CPUID data independent of vPMU mode, as is done 
> here?
> 
Sorry, I didn't check the mail list on time and don't know somebody is working 
on this.  I think Boris' patch is better than me.

Thanks,
Luwei Kang

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vpmu: fix vpmu can't enabled in guest

[Andrew Cooper]

On 14/02/17 02:19, Luwei Kang wrote:
> vpmu_enable() can not use for check if vpmu is enabled in guest during
> booting up. Because linux kernel get the status of if supported pmu
> is earler than xen vpmu initialise. vpmu_enable() will return false
> even if vpmu has been enabled in guest.

Sorry for breaking this.  However ...

> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> index e0a387e..b63c5d8 100644
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -713,8 +713,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, 
> struct cpuid_leaf *res)
>  }
>  }
>  
> -if ( vpmu_enabled(curr) &&
> - vpmu_is_set(vcpu_vpmu(curr), VPMU_CPU_HAS_DS) )
> +if ( opt_vpmu_enabled && boot_cpu_has(X86_FEATURE_DS) )

... this is overly general.  The visibility of these flags must be per
domain, and not globally like this.

In particular, XENPMU_MODE_ALL needs to expose PMU to dom0, but hide it
from all other domains, while even in the XENPMU_MODE_SELF case, only
domains explicitly configured with PMU should see it (as it generally
unsafe to migrate with).

Longterm (with the inclusion of the CPUID improvements), my plan was to
have PMU available in the max policy but hidden in the default policy,
which requires the toolstack to explicitly opt in for domains.  It would
opt in/out by setting the version field in guests CPUID policy and the
other feature bits.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vpmu: fix vpmu can't enabled in guest

[Jan Beulich]

>>> On 14.02.17 at 03:19,  wrote:
> vpmu_enable() can not use for check if vpmu is enabled in guest during
> booting up. Because linux kernel get the status of if supported pmu
> is earler than xen vpmu initialise. vpmu_enable() will return false
> even if vpmu has been enabled in guest.
> 
> Signed-off-by: Luwei Kang 

You've probably seen Boris' patch with the same overall goal. While
his looks to leave things a little too strict, yours looks to be widening
things a little too much. Do both of you think we could find a middle
ground, or do we need to accept the effect of possibly misleading
the guest by surfacing the CPUID data independent of vPMU mode,
as is done here?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH] x86/vpmu: fix vpmu can't enabled in guest

[Luwei Kang]

vpmu_enable() can not use for check if vpmu is enabled in guest during
booting up. Because linux kernel get the status of if supported pmu
is earler than xen vpmu initialise. vpmu_enable() will return false
even if vpmu has been enabled in guest.

Signed-off-by: Luwei Kang 
---
 xen/arch/x86/cpu/vpmu.c|  2 +-
 xen/arch/x86/cpuid.c   | 11 +--
 xen/include/asm-x86/vpmu.h |  2 +-
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 35a9403..8c8ffc9 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -50,7 +50,7 @@ CHECK_pmu_params;
  * "vpmu=arch" : vpmu enabled for predef arch counters only (restrictive)
  * flag combinations are allowed, eg, "vpmu=ipc,bts".
  */
-static unsigned int __read_mostly opt_vpmu_enabled;
+unsigned int __read_mostly opt_vpmu_enabled;
 unsigned int __read_mostly vpmu_mode = XENPMU_MODE_OFF;
 unsigned int __read_mostly vpmu_features = 0;
 static void parse_vpmu_params(char *s);
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index e0a387e..b63c5d8 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -713,8 +713,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, 
struct cpuid_leaf *res)
 }
 }
 
-if ( vpmu_enabled(curr) &&
- vpmu_is_set(vcpu_vpmu(curr), VPMU_CPU_HAS_DS) )
+if ( opt_vpmu_enabled && boot_cpu_has(X86_FEATURE_DS) )
 {
 res->d |= cpufeat_mask(X86_FEATURE_DS);
 if ( cpu_has(_cpu_data, X86_FEATURE_DTES64) )
@@ -726,7 +725,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, 
struct cpuid_leaf *res)
 
 case 0x000a: /* Architectural Performance Monitor Features (Intel) */
 if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
- !vpmu_enabled(curr) )
+ !opt_vpmu_enabled )
 goto unsupported;
 
 /* Report at most version 3 since that's all we currently emulate. */
@@ -793,8 +792,7 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, 
struct cpuid_leaf *res)
 if ( !hap_enabled(d) && !hvm_pae_enabled(v) )
 res->d &= ~cpufeat_mask(X86_FEATURE_PSE36);
 
-if ( vpmu_enabled(v) &&
- vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) )
+if ( opt_vpmu_enabled && boot_cpu_has(X86_FEATURE_DS) )
 {
 res->d |= cpufeat_mask(X86_FEATURE_DS);
 if ( cpu_has(_cpu_data, X86_FEATURE_DTES64) )
@@ -811,7 +809,8 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, 
struct cpuid_leaf *res)
 break;
 
 case 0x000a: /* Architectural Performance Monitor Features (Intel) */
-if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || !vpmu_enabled(v) )
+if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
+   !opt_vpmu_enabled )
 {
 *res = EMPTY_LEAF;
 break;
diff --git a/xen/include/asm-x86/vpmu.h b/xen/include/asm-x86/vpmu.h
index e50618f..1148d66 100644
--- a/xen/include/asm-x86/vpmu.h
+++ b/xen/include/asm-x86/vpmu.h
@@ -25,7 +25,6 @@
 
 #define vcpu_vpmu(vcpu)   (&(vcpu)->arch.vpmu)
 #define vpmu_vcpu(vpmu)   container_of((vpmu), struct vcpu, arch.vpmu)
-#define vpmu_enabled(vcpu) vpmu_is_set(vcpu_vpmu(vcpu), VPMU_CONTEXT_ALLOCATED)
 
 #define MSR_TYPE_COUNTER0
 #define MSR_TYPE_CTRL   1
@@ -121,6 +120,7 @@ static inline int vpmu_do_rdmsr(unsigned int msr, uint64_t 
*msr_content)
 return vpmu_do_msr(msr, msr_content, 0, 0);
 }
 
+extern unsigned int opt_vpmu_enabled;
 extern unsigned int vpmu_mode;
 extern unsigned int vpmu_features;
 
-- 
1.8.3.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel