Re: [PATCH v12 for-xen-4.5 11/20] x86/VPMU: Interface for setting PMU mode and flags

[Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>]

Am Montag 29 September 2014, 14:59:43 schrieb Jan Beulich:
> >>> On 29.09.14 at 15:25, <dietmar.hahn@ts.fujitsu.com> wrote:
> > Only a minor note below.
> > 
> > Am Donnerstag 25 September 2014, 15:28:47 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.ostrovsky@oracle.com>
> >> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> ---
> >>  tools/flask/policy/policy/modules/xen/xen.te |   3 +
> >>  xen/arch/x86/domain.c                        |   6 +-
> >>  xen/arch/x86/hvm/svm/vpmu.c                  |   4 +-
> >>  xen/arch/x86/hvm/vmx/vpmu_core2.c            |  10 +-
> >>  xen/arch/x86/hvm/vpmu.c                      | 206 
> > +++++++++++++++++++++++++--
> >>  xen/arch/x86/x86_64/compat/entry.S           |   4 +
> >>  xen/arch/x86/x86_64/entry.S                  |   4 +
> >>  xen/include/Makefile                         |   2 +
> >>  xen/include/asm-x86/hvm/vpmu.h               |  27 ++--
> >>  xen/include/public/pmu.h                     |  44 ++++++
> >>  xen/include/public/xen.h                     |   1 +
> >>  xen/include/xen/hypercall.h                  |   4 +
> >>  xen/include/xlat.lst                         |   4 +
> >>  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 +
> >>  18 files changed, 334 insertions(+), 27 deletions(-)
> >> 
> >> diff --git a/tools/flask/policy/policy/modules/xen/xen.te 
> > b/tools/flask/policy/policy/modules/xen/xen.te
> >> index 1937883..fb761cd 100644
> >> --- a/tools/flask/policy/policy/modules/xen/xen.te
> >> +++ b/tools/flask/policy/policy/modules/xen/xen.te
> >> @@ -64,6 +64,9 @@ allow dom0_t xen_t:xen {
> >>  	getidle debug getcpuinfo heap pm_op mca_op lockprof cpupool_op tmem_op
> >>  	tmem_control getscheduler setscheduler
> >>  };
> >> +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 7b1dfe6..6a07737 100644
> >> --- a/xen/arch/x86/domain.c
> >> +++ b/xen/arch/x86/domain.c
> >> @@ -1503,7 +1503,7 @@ void context_switch(struct vcpu *prev, struct vcpu 
> > *next)
> >>      if ( is_hvm_vcpu(prev) )
> >>      {
> >>          if (prev != next)
> >> -            vpmu_save(prev);
> >> +            vpmu_switch_from(prev, next);
> >>  
> >>          if ( !list_empty(&prev->arch.hvm_vcpu.tm_list) )
> >>              pt_save_timer(prev);
> >> @@ -1546,9 +1546,9 @@ void context_switch(struct vcpu *prev, struct vcpu 
> > *next)
> >>                             !is_hardware_domain(next->domain));
> >>      }
> >>  
> >> -    if (is_hvm_vcpu(next) && (prev != next) )
> >> +    if ( is_hvm_vcpu(prev) && (prev != next) )
> >>          /* Must be done with interrupts enabled */
> >> -        vpmu_load(next);
> >> +        vpmu_switch_to(prev, next);
> >>  
> >>      context_saved(prev);
> >>  
> >> diff --git a/xen/arch/x86/hvm/svm/vpmu.c b/xen/arch/x86/hvm/svm/vpmu.c
> >> index 124b147..37d8228 100644
> >> --- a/xen/arch/x86/hvm/svm/vpmu.c
> >> +++ b/xen/arch/x86/hvm/svm/vpmu.c
> >> @@ -479,14 +479,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 beff5c3..c0a45cd 100644
> >> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> >> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> >> @@ -703,13 +703,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) )
> >> @@ -824,7 +824,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;
> >> @@ -832,7 +832,7 @@ int vmx_vpmu_initialise(struct vcpu *v, unsigned int 
> > vpmu_flags)
> >>      int ret = 0;
> >>  
> >>      vpmu->arch_vpmu_ops = &core2_no_vpmu_ops;
> >> -    if ( !vpmu_flags )
> >> +    if ( vpmu_mode == XENPMU_MODE_OFF )
> >>          return 0;
> >>  
> >>      if ( family == 6 )
> >> @@ -875,7 +875,7 @@ int vmx_vpmu_initialise(struct vcpu *v, unsigned int 
> > vpmu_flags)
> >>          /* future: */
> >>          case 0x3d:
> >>          case 0x4e:
> >> -            ret = core2_vpmu_initialise(v, vpmu_flags);
> >> +            ret = core2_vpmu_initialise(v);
> >>              if ( !ret )
> >>                  vpmu->arch_vpmu_ops = &core2_vpmu_ops;
> >>              return ret;
> >> diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
> >> index 071b869..5fcee0e 100644
> >> --- a/xen/arch/x86/hvm/vpmu.c
> >> +++ b/xen/arch/x86/hvm/vpmu.c
> >> @@ -21,6 +21,8 @@
> >>  #include <xen/config.h>
> >>  #include <xen/sched.h>
> >>  #include <xen/xenoprof.h>
> >> +#include <xen/event.h>
> >> +#include <xen/guest_access.h>
> >>  #include <asm/regs.h>
> >>  #include <asm/types.h>
> >>  #include <asm/msr.h>
> >> @@ -32,13 +34,22 @@
> >>  #include <asm/hvm/svm/vmcb.h>
> >>  #include <asm/apic.h>
> >>  #include <public/pmu.h>
> >> +#include <xen/tasklet.h>
> >> +#include <xsm/xsm.h>
> >> +
> >> +#include <compat/pmu.h>
> >> +CHECK_pmu_params;
> >> +CHECK_pmu_intel_ctxt;
> >> +CHECK_pmu_amd_ctxt;
> >> +CHECK_pmu_cntr_pair;
> >>  
> >>  /*
> >>   * "vpmu" :     vpmu generally enabled
> >>   * "vpmu=off" : vpmu generally disabled
> >>   * "vpmu=bts" : vpmu enabled and Intel BTS feature switched on.
> >>   */
> >> -static unsigned int __read_mostly opt_vpmu_enabled;
> >> +uint64_t __read_mostly vpmu_mode = XENPMU_MODE_OFF;
> >> +uint64_t __read_mostly vpmu_features = 0;
> >>  static void parse_vpmu_param(char *s);
> >>  custom_param("vpmu", parse_vpmu_param);
> >>  
> >> @@ -52,7 +63,7 @@ static void __init parse_vpmu_param(char *s)
> >>          break;
> >>      default:
> >>          if ( !strcmp(s, "bts") )
> >> -            opt_vpmu_enabled |= VPMU_BOOT_BTS;
> >> +            vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
> >>          else if ( *s )
> >>          {
> >>              printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
> >> @@ -60,7 +71,8 @@ static void __init parse_vpmu_param(char *s)
> >>          }
> >>          /* fall through */
> >>      case 1:
> >> -        opt_vpmu_enabled |= VPMU_BOOT_ENABLED;
> >> +        /* Default VPMU mode */
> >> +        vpmu_mode = XENPMU_MODE_SELF;
> >>          break;
> >>      }
> >>  }
> >> @@ -77,6 +89,9 @@ int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, 
> > uint64_t supported)
> >>  {
> >>      struct vpmu_struct *vpmu = vcpu_vpmu(current);
> >>  
> >> +    if ( !(vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
> >> +        return 0;
> >> +
> >>      if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr )
> >>          return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content, supported);
> >>      return 0;
> >> @@ -86,6 +101,9 @@ int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content)
> >>  {
> >>      struct vpmu_struct *vpmu = vcpu_vpmu(current);
> >>  
> >> +    if ( !(vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
> >> +        return 0;
> >> +
> >>      if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_rdmsr )
> >>          return vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content);
> >>      return 0;
> >> @@ -242,19 +260,19 @@ void vpmu_initialise(struct vcpu *v)
> >>      switch ( vendor )
> >>      {
> >>      case X86_VENDOR_AMD:
> >> -        if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
> >> -            opt_vpmu_enabled = 0;
> >> +        if ( svm_vpmu_initialise(v) != 0 )
> >> +            vpmu_mode = XENPMU_MODE_OFF;
> >>          break;
> >>  
> >>      case X86_VENDOR_INTEL:
> >> -        if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
> >> -            opt_vpmu_enabled = 0;
> >> +        if ( vmx_vpmu_initialise(v) != 0 )
> >> +            vpmu_mode = XENPMU_MODE_OFF;
> >>          break;
> >>  
> >>      default:
> >>          printk("VPMU: Initialization failed. "
> >>                 "Unknown CPU vendor %d\n", vendor);
> >> -        opt_vpmu_enabled = 0;
> >> +        vpmu_mode = XENPMU_MODE_OFF;
> >>          break;
> >>      }
> >>  }
> >> @@ -276,3 +294,175 @@ void vpmu_dump(struct vcpu *v)
> >>          vpmu->arch_vpmu_ops->arch_vpmu_dump(v);
> >>  }
> >>  
> >> +static atomic_t vpmu_sched_counter;
> >> +
> >> +static void vpmu_sched_checkin(unsigned long unused)
> >> +{
> >> +    atomic_inc(&vpmu_sched_counter);
> >> +}
> >> +
> >> +static int vpmu_force_context_switch(void)
> >> +{
> >> +    unsigned i, j, allbutself_num, mycpu;
> >> +    static s_time_t start, now;
> >> +    struct tasklet **sync_task;
> >> +    struct vcpu *curr_vcpu = current;
> >> +    int ret = 0;
> >> +
> >> +    allbutself_num = num_online_cpus() - 1;
> >> +
> >> +    sync_task = xzalloc_array(struct tasklet *, allbutself_num);
> >> +    if ( !sync_task )
> >> +    {
> >> +        printk(XENLOG_WARNING "vpmu_force_context_switch: out of 
> > memory\n");
> >> +        return -ENOMEM;
> >> +    }
> >> +
> >> +    for ( i = 0; i < allbutself_num; i++ )
> >> +    {
> >> +        sync_task[i] = xmalloc(struct tasklet);
> >> +        if ( sync_task[i] == NULL )
> >> +        {
> >> +            printk(XENLOG_WARNING "vpmu_force_context_switch: out of 
> > memory\n");
> >> +            ret = -ENOMEM;
> >> +            goto out;
> >> +        }
> >> +        tasklet_init(sync_task[i], vpmu_sched_checkin, 0);
> > 
> > Only a question of understanding.
> > Is there a special reason not to use a single memory allocation
> > except for memory fragmentation on systems with a large number of cpus?
> > 
> >      struct tasklet *sync_task;
> >      sync_task = xmalloc(sizeof(struct tasklet) * allbutself_num);
> 
> Apart from this then needing to be xmalloc_array() - yes, the
> reason here is to avoid non-order-zero runtime allocations. I.e.
> the alternative would be to provide something vmalloc()-like to
> be used here (or open code it as we do in a couple of other
> places).

Thank you for the hint!
Dietmar.

> 
> Jan
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

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