Re: [Xen-devel] [PATCH v25 04/15] x86/VPMU: Interface for setting PMU mode and flags

2015-07-08 Thread Tian, Kevin 
 From: Boris Ostrovsky [mailto:boris.ostrov...@oracle.com]
 Sent: Saturday, June 20, 2015 2:45 AM
 
 Add runtime interface for setting PMU mode and flags. Three main modes are
 provided:
 * XENPMU_MODE_OFF:  PMU is not virtualized
 * XENPMU_MODE_SELF: Guests can access PMU MSRs and receive PMU interrupts.
 * XENPMU_MODE_HV: Same as XENPMU_MODE_SELF for non-proviledged guests, dom0
   can profile itself and the hypervisor.
 
 Note that PMU modes are different from what can be provided at Xen's boot line
 with 'vpmu' argument. An 'off' (or '0') value is equivalent to 
 XENPMU_MODE_OFF.
 Any other value, on the other hand, will cause VPMU mode to be set to
 XENPMU_MODE_SELF during boot.
 
 For feature flags only Intel's BTS is currently supported.
 
 Mode and flags are set via HYPERVISOR_xenpmu_op hypercall.
 
 Signed-off-by: Boris Ostrovsky boris.ostrov...@oracle.com
 Acked-by: Daniel De Graaf dgde...@tycho.nsa.gov
 

Acked-by: Kevin Tian kevin.t...@intel.com

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

Re: [Xen-devel] [PATCH v25 04/15] x86/VPMU: Interface for setting PMU mode and flags

2015-07-07 Thread Dietmar Hahn 
Am Freitag 19 Juni 2015, 14:44:35 schrieb Boris Ostrovsky:
 Add runtime interface for setting PMU mode and flags. Three main modes are
 provided:
 * XENPMU_MODE_OFF:  PMU is not virtualized
 * XENPMU_MODE_SELF: Guests can access PMU MSRs and receive PMU interrupts.
 * XENPMU_MODE_HV: Same as XENPMU_MODE_SELF for non-proviledged guests, dom0
   can profile itself and the hypervisor.
 
 Note that PMU modes are different from what can be provided at Xen's boot line
 with 'vpmu' argument. An 'off' (or '0') value is equivalent to 
 XENPMU_MODE_OFF.
 Any other value, on the other hand, will cause VPMU mode to be set to
 XENPMU_MODE_SELF during boot.
 
 For feature flags only Intel's BTS is currently supported.
 
 Mode and flags are set via HYPERVISOR_xenpmu_op hypercall.
 
 Signed-off-by: Boris Ostrovsky boris.ostrov...@oracle.com
 Acked-by: Daniel De Graaf dgde...@tycho.nsa.gov
 
 ---
 Changes in v25:
 * Added (vpmu_features == pmu_params.val) check in 
 do_xenpmu_op:XENPMU_mode_get

You mean in do_xenpmu_op:XENPMU_feature_set I think?

Reviewed-by: Dietmar Hahn dietmar.h...@ts.fujitsu.com

Dietmar.


-- 
Company details: http://ts.fujitsu.com/imprint.html

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

Re: [Xen-devel] [PATCH v25 04/15] x86/VPMU: Interface for setting PMU mode and flags

2015-06-23 Thread Jan Beulich 
 On 22.06.15 at 18:10, boris.ostrov...@oracle.com wrote:
 On 06/22/2015 11:10 AM, Jan Beulich wrote:

 +switch ( op )
 +{
 +case XENPMU_mode_set:
 +{
 +if ( (pmu_params.val  ~(XENPMU_MODE_SELF | XENPMU_MODE_HV)) ||
 + (hweight64(pmu_params.val)  1) )
 +return -EINVAL;
 +
 +/* 32-bit dom0 can only sample itself. */
 +if ( is_pv_32bit_vcpu(current)  (pmu_params.val  
 XENPMU_MODE_HV) )
 +return -EINVAL;
 +
 +spin_lock(vpmu_lock);
 +
 +/*
 + * We can always safely switch between XENPMU_MODE_SELF and
 + * XENPMU_MODE_HV while other VPMUs are active.
 + */
 +if ( (vpmu_count == 0) || (vpmu_mode == pmu_params.val) ||
 + ((vpmu_mode ^ pmu_params.val) ==
 +  (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
 +vpmu_mode = pmu_params.val;
 +else
 +{
 I think this would better be

  if ( (vpmu_count == 0) ||
   ((vpmu_mode ^ pmu_params.val) ==
(XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
  vpmu_mode = pmu_params.val;
  else if ( vpmu_mode != pmu_params.val )
  {

 But there's no need to re-submit just because of this (it could be
 changed upon commit if you agree).
 
 This will generate an error (with a message to the log) even if we keep 
 the mode unchanged, something that I wanted to avoid. (Same thing for 
 XENPMU_feature_set, which is what Kevin mentioned last time).

I don't see this: The only difference to the code you have is -
afaics - that my variant avoids the pointless write to vpmu_mode.

 Or wait - I just checked again, and while I thought this was long
 addressed I still don't seen struct xen_pmu_params pad field
 being checked to be zero as input, and being stored as zero when
 only an output. Did this get lost? Did I forget about a reason why
 this isn't being done? Unless the latter is the case, the ack above
 is dependent upon this getting fixed.
 
 Yes, we did talk about this and as result I added major version check 
 (which I should have had there anyway): if we decide to start using this 
 field we'd need to increment the major version.

Okay then.

Jan


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

Re: [Xen-devel] [PATCH v25 04/15] x86/VPMU: Interface for setting PMU mode and flags

2015-06-23 Thread Boris Ostrovsky 

On 06/23/2015 04:26 AM, Jan Beulich wrote:

On 22.06.15 at 18:10, boris.ostrov...@oracle.com wrote:

On 06/22/2015 11:10 AM, Jan Beulich wrote:

+switch ( op )
+{
+case XENPMU_mode_set:
+{
+if ( (pmu_params.val  ~(XENPMU_MODE_SELF | XENPMU_MODE_HV)) ||
+ (hweight64(pmu_params.val)  1) )
+return -EINVAL;
+
+/* 32-bit dom0 can only sample itself. */
+if ( is_pv_32bit_vcpu(current)  (pmu_params.val  XENPMU_MODE_HV) )
+return -EINVAL;
+
+spin_lock(vpmu_lock);
+
+/*
+ * We can always safely switch between XENPMU_MODE_SELF and
+ * XENPMU_MODE_HV while other VPMUs are active.
+ */
+if ( (vpmu_count == 0) || (vpmu_mode == pmu_params.val) ||
+ ((vpmu_mode ^ pmu_params.val) ==
+  (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
+vpmu_mode = pmu_params.val;
+else
+{

I think this would better be

  if ( (vpmu_count == 0) ||
   ((vpmu_mode ^ pmu_params.val) ==
(XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
  vpmu_mode = pmu_params.val;
  else if ( vpmu_mode != pmu_params.val )
  {

But there's no need to re-submit just because of this (it could be
changed upon commit if you agree).

This will generate an error (with a message to the log) even if we keep
the mode unchanged, something that I wanted to avoid. (Same thing for
XENPMU_feature_set, which is what Kevin mentioned last time).

I don't see this: The only difference to the code you have is -
afaics - that my variant avoids the pointless write to vpmu_mode.


Oh, right, I was only looking at the change that you made to 'if', not 
to the 'esle'. Yes, that's good.


-boris


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

Re: [Xen-devel] [PATCH v25 04/15] x86/VPMU: Interface for setting PMU mode and flags

2015-06-22 Thread Boris Ostrovsky 

On 06/22/2015 11:10 AM, Jan Beulich wrote:



+switch ( op )
+{
+case XENPMU_mode_set:
+{
+if ( (pmu_params.val  ~(XENPMU_MODE_SELF | XENPMU_MODE_HV)) ||
+ (hweight64(pmu_params.val)  1) )
+return -EINVAL;
+
+/* 32-bit dom0 can only sample itself. */
+if ( is_pv_32bit_vcpu(current)  (pmu_params.val  XENPMU_MODE_HV) )
+return -EINVAL;
+
+spin_lock(vpmu_lock);
+
+/*
+ * We can always safely switch between XENPMU_MODE_SELF and
+ * XENPMU_MODE_HV while other VPMUs are active.
+ */
+if ( (vpmu_count == 0) || (vpmu_mode == pmu_params.val) ||
+ ((vpmu_mode ^ pmu_params.val) ==
+  (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
+vpmu_mode = pmu_params.val;
+else
+{

I think this would better be

 if ( (vpmu_count == 0) ||
  ((vpmu_mode ^ pmu_params.val) ==
   (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
 vpmu_mode = pmu_params.val;
 else if ( vpmu_mode != pmu_params.val )
 {

But there's no need to re-submit just because of this (it could be
changed upon commit if you agree).


This will generate an error (with a message to the log) even if we keep 
the mode unchanged, something that I wanted to avoid. (Same thing for 
XENPMU_feature_set, which is what Kevin mentioned last time).




Or wait - I just checked again, and while I thought this was long
addressed I still don't seen struct xen_pmu_params pad field
being checked to be zero as input, and being stored as zero when
only an output. Did this get lost? Did I forget about a reason why
this isn't being done? Unless the latter is the case, the ack above
is dependent upon this getting fixed.



Yes, we did talk about this and as result I added major version check 
(which I should have had there anyway): if we decide to start using this 
field we'd need to increment the major version.


-boris


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

Re: [Xen-devel] [PATCH v25 04/15] x86/VPMU: Interface for setting PMU mode and flags

2015-06-22 Thread Jan Beulich 
 On 19.06.15 at 20:44, boris.ostrov...@oracle.com wrote:
 Add runtime interface for setting PMU mode and flags. Three main modes are
 provided:
 * XENPMU_MODE_OFF:  PMU is not virtualized
 * XENPMU_MODE_SELF: Guests can access PMU MSRs and receive PMU interrupts.
 * XENPMU_MODE_HV: Same as XENPMU_MODE_SELF for non-proviledged guests, dom0
   can profile itself and the hypervisor.
 
 Note that PMU modes are different from what can be provided at Xen's boot 
 line
 with 'vpmu' argument. An 'off' (or '0') value is equivalent to 
 XENPMU_MODE_OFF.
 Any other value, on the other hand, will cause VPMU mode to be set to
 XENPMU_MODE_SELF during boot.
 
 For feature flags only Intel's BTS is currently supported.
 
 Mode and flags are set via HYPERVISOR_xenpmu_op hypercall.
 
 Signed-off-by: Boris Ostrovsky boris.ostrov...@oracle.com
 Acked-by: Daniel De Graaf dgde...@tycho.nsa.gov

Acked-by: Jan Beulich jbeul...@suse.com
with one minor comment

 +switch ( op )
 +{
 +case XENPMU_mode_set:
 +{
 +if ( (pmu_params.val  ~(XENPMU_MODE_SELF | XENPMU_MODE_HV)) ||
 + (hweight64(pmu_params.val)  1) )
 +return -EINVAL;
 +
 +/* 32-bit dom0 can only sample itself. */
 +if ( is_pv_32bit_vcpu(current)  (pmu_params.val  XENPMU_MODE_HV) )
 +return -EINVAL;
 +
 +spin_lock(vpmu_lock);
 +
 +/*
 + * We can always safely switch between XENPMU_MODE_SELF and
 + * XENPMU_MODE_HV while other VPMUs are active.
 + */
 +if ( (vpmu_count == 0) || (vpmu_mode == pmu_params.val) ||
 + ((vpmu_mode ^ pmu_params.val) ==
 +  (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
 +vpmu_mode = pmu_params.val;
 +else
 +{

I think this would better be

if ( (vpmu_count == 0) ||
 ((vpmu_mode ^ pmu_params.val) ==
  (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
vpmu_mode = pmu_params.val;
else if ( vpmu_mode != pmu_params.val )
{

But there's no need to re-submit just because of this (it could be
changed upon commit if you agree).

Or wait - I just checked again, and while I thought this was long
addressed I still don't seen struct xen_pmu_params pad field
being checked to be zero as input, and being stored as zero when
only an output. Did this get lost? Did I forget about a reason why
this isn't being done? Unless the latter is the case, the ack above
is dependent upon this getting fixed.

Jan


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

[Xen-devel] [PATCH v25 04/15] x86/VPMU: Interface for setting PMU mode and flags

2015-06-19 Thread Boris Ostrovsky 
Add runtime interface for setting PMU mode and flags. Three main modes are
provided:
* XENPMU_MODE_OFF:  PMU is not virtualized
* XENPMU_MODE_SELF: Guests can access PMU MSRs and receive PMU interrupts.
* XENPMU_MODE_HV: Same as XENPMU_MODE_SELF for non-proviledged guests, dom0
  can profile itself and the hypervisor.

Note that PMU modes are different from what can be provided at Xen's boot line
with 'vpmu' argument. An 'off' (or '0') value is equivalent to XENPMU_MODE_OFF.
Any other value, on the other hand, will cause VPMU mode to be set to
XENPMU_MODE_SELF during boot.

For feature flags only Intel's BTS is currently supported.

Mode and flags are set via HYPERVISOR_xenpmu_op hypercall.

Signed-off-by: Boris Ostrovsky boris.ostrov...@oracle.com
Acked-by: Daniel De Graaf dgde...@tycho.nsa.gov

---
Changes in v25:
* Added (vpmu_features == pmu_params.val) check in do_xenpmu_op:XENPMU_mode_get
  case (for consistency)
* Replaced 'return -EFAULT' with 'ret=-EFAULT' in XENPMU_feature_get


 tools/flask/policy/policy/modules/xen/xen.te |   3 +
 xen/arch/x86/domain.c|   4 +-
 xen/arch/x86/hvm/svm/vpmu.c  |   4 +-
 xen/arch/x86/hvm/vmx/vpmu_core2.c|  10 +-
 xen/arch/x86/hvm/vpmu.c  | 159 +--
 xen/arch/x86/x86_64/compat/entry.S   |   4 +
 xen/arch/x86/x86_64/entry.S  |   4 +
 xen/include/asm-x86/hvm/vpmu.h   |  27 +++--
 xen/include/public/pmu.h |  46 
 xen/include/public/xen.h |   1 +
 xen/include/xen/hypercall.h  |   4 +
 xen/include/xlat.lst |   1 +
 xen/include/xsm/dummy.h  |  15 +++
 xen/include/xsm/xsm.h|   6 +
 xen/xsm/dummy.c  |   1 +
 xen/xsm/flask/hooks.c|  18 +++
 xen/xsm/flask/policy/access_vectors  |   2 +
 17 files changed, 284 insertions(+), 25 deletions(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.te 
b/tools/flask/policy/policy/modules/xen/xen.te
index 51f59c5..45b5cb2 100644
--- a/tools/flask/policy/policy/modules/xen/xen.te
+++ b/tools/flask/policy/policy/modules/xen/xen.te
@@ -68,6 +68,9 @@ allow dom0_t xen_t:xen2 {
 resource_op
 psr_cmt_op
 };
+allow dom0_t xen_t:xen2 {
+pmu_ctrl
+};
 allow dom0_t xen_t:mmu memorymap;
 
 # Allow dom0 to use these domctls on itself. For domctls acting on other
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0363650..dc18565 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1546,7 +1546,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
 if ( is_hvm_domain(prevd) )
 {
 if (prev != next)
-vpmu_save(prev);
+vpmu_switch_from(prev);
 
 if ( !list_empty(prev-arch.hvm_vcpu.tm_list) )
 pt_save_timer(prev);
@@ -1591,7 +1591,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
 
 if (is_hvm_domain(nextd)  (prev != next) )
 /* Must be done with interrupts enabled */
-vpmu_load(next);
+vpmu_switch_to(next);
 
 context_saved(prev);
 
diff --git a/xen/arch/x86/hvm/svm/vpmu.c b/xen/arch/x86/hvm/svm/vpmu.c
index a8b79df..481ea7b 100644
--- a/xen/arch/x86/hvm/svm/vpmu.c
+++ b/xen/arch/x86/hvm/svm/vpmu.c
@@ -472,14 +472,14 @@ struct arch_vpmu_ops amd_vpmu_ops = {
 .arch_vpmu_dump = amd_vpmu_dump
 };
 
-int svm_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags)
+int svm_vpmu_initialise(struct vcpu *v)
 {
 struct vpmu_struct *vpmu = vcpu_vpmu(v);
 uint8_t family = current_cpu_data.x86;
 int ret = 0;
 
 /* vpmu enabled? */
-if ( !vpmu_flags )
+if ( vpmu_mode == XENPMU_MODE_OFF )
 return 0;
 
 switch ( family )
diff --git a/xen/arch/x86/hvm/vmx/vpmu_core2.c 
b/xen/arch/x86/hvm/vmx/vpmu_core2.c
index 6fc634c..cfcdf42 100644
--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
@@ -708,13 +708,13 @@ static int core2_vpmu_do_interrupt(struct cpu_user_regs 
*regs)
 return 1;
 }
 
-static int core2_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags)
+static int core2_vpmu_initialise(struct vcpu *v)
 {
 struct vpmu_struct *vpmu = vcpu_vpmu(v);
 u64 msr_content;
 static bool_t ds_warned;
 
-if ( !(vpmu_flags  VPMU_BOOT_BTS) )
+if ( !(vpmu_features  XENPMU_FEATURE_INTEL_BTS) )
 goto func_out;
 /* Check the 'Debug Store' feature in the CPUID.EAX[1]:EDX[21] */
 while ( boot_cpu_has(X86_FEATURE_DS) )
@@ -826,7 +826,7 @@ struct arch_vpmu_ops core2_no_vpmu_ops = {
 .do_cpuid = core2_no_vpmu_do_cpuid,
 };
 
-int vmx_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags)
+int vmx_vpmu_initialise(struct vcpu *v)
 {
 struct vpmu_struct *vpmu = vcpu_vpmu(v);
 uint8_t family = current_cpu_data.x86;
@@ -834,7 +834,7 @@ int vmx_vpmu_initialise(struct vcpu *v, unsigned int