Re: [PATCH v25 14/15] x86/VPMU: Add privileged PMU mode

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Saturday, June 20, 2015 2:45 AM
> 
> Add support for privileged PMU mode (XENPMU_MODE_ALL) which allows privileged
> domain (dom0) profile both itself (and the hypervisor) and the guests. While
> this mode is on profiling in guests is disabled.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  xen/arch/x86/hvm/vpmu.c  | 40
> +++++++++++++++++++++++++++++-----------
>  xen/arch/x86/traps.c     | 13 +++++++++++++
>  xen/include/public/pmu.h |  3 +++
>  3 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
> index 9d6ca93..3ad0b94 100644
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -108,8 +108,10 @@ int vpmu_do_msr(unsigned int msr, uint64_t *msr_content,
>      const struct arch_vpmu_ops *ops;
>      int ret = 0;
> 
> -    if ( likely(vpmu_mode == XENPMU_MODE_OFF) )
> -        goto nop;
> +    if ( likely(vpmu_mode == XENPMU_MODE_OFF) ||
> +         ((vpmu_mode & XENPMU_MODE_ALL) &&
> +          !is_hardware_domain(current->domain)) )
> +         goto nop;
> 
>      vpmu = vcpu_vpmu(curr);
>      ops = vpmu->arch_vpmu_ops;
> @@ -164,8 +166,12 @@ void vpmu_do_interrupt(struct cpu_user_regs *regs)
>      struct vlapic *vlapic;
>      u32 vlapic_lvtpc;
> 
> -    /* dom0 will handle interrupt for special domains (e.g. idle domain) */
> -    if ( sampled->domain->domain_id >= DOMID_FIRST_RESERVED )
> +    /*
> +     * dom0 will handle interrupt for special domains (e.g. idle domain) or,
> +     * in XENPMU_MODE_ALL, for everyone.
> +     */
> +    if ( (vpmu_mode & XENPMU_MODE_ALL) ||
> +         (sampled->domain->domain_id >= DOMID_FIRST_RESERVED) )
>      {
>          sampling = choose_hwdom_vcpu();
>          if ( !sampling )
> @@ -179,16 +185,17 @@ void vpmu_do_interrupt(struct cpu_user_regs *regs)
>          return;
> 
>      /* PV(H) guest */
> -    if ( !is_hvm_vcpu(sampling) )
> +    if ( !is_hvm_vcpu(sampling) || (vpmu_mode & XENPMU_MODE_ALL) )
>      {
>          const struct cpu_user_regs *cur_regs;
>          uint64_t *flags = &vpmu->xenpmu_data->pmu.pmu_flags;
> -        domid_t domid = DOMID_SELF;
> +        domid_t domid;
> 
>          if ( !vpmu->xenpmu_data )
>              return;
> 
>          if ( is_pvh_vcpu(sampling) &&
> +             !(vpmu_mode & XENPMU_MODE_ALL) &&
>               !vpmu->arch_vpmu_ops->do_interrupt(regs) )
>              return;
> 
> @@ -205,6 +212,11 @@ void vpmu_do_interrupt(struct cpu_user_regs *regs)
>          else
>              *flags = PMU_SAMPLE_PV;
> 
> +        if ( sampled == sampling )
> +            domid = DOMID_SELF;
> +        else
> +            domid = sampled->domain->domain_id;
> +
>          /* Store appropriate registers in xenpmu_data */
>          /* FIXME: 32-bit PVH should go here as well */
>          if ( is_pv_32bit_vcpu(sampling) )
> @@ -233,7 +245,8 @@ void vpmu_do_interrupt(struct cpu_user_regs *regs)
> 
>              if ( (vpmu_mode & XENPMU_MODE_SELF) )
>                  cur_regs = guest_cpu_user_regs();
> -            else if ( !guest_mode(regs) && is_hardware_domain(sampling->domain) )
> +            else if ( !guest_mode(regs) &&
> +                      is_hardware_domain(sampling->domain) )
>              {
>                  cur_regs = regs;
>                  domid = DOMID_XEN;
> @@ -472,7 +485,9 @@ void vpmu_initialise(struct vcpu *v)
>          printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", v);
> 
>      /* Intel needs to initialize VPMU ops even if VPMU is not in use */
> -    if ( !is_priv_vpmu && (ret || (vpmu_mode == XENPMU_MODE_OFF)) )
> +    if ( !is_priv_vpmu &&
> +         (ret || (vpmu_mode == XENPMU_MODE_OFF) ||
> +          (vpmu_mode == XENPMU_MODE_ALL)) )
>      {
>          spin_lock(&vpmu_lock);
>          vpmu_count--;
> @@ -525,7 +540,8 @@ static int pvpmu_init(struct domain *d, xen_pmu_params_t
> *params)
>      struct page_info *page;
>      uint64_t gfn = params->val;
> 
> -    if ( vpmu_mode == XENPMU_MODE_OFF )
> +    if ( (vpmu_mode == XENPMU_MODE_OFF) ||
> +         ((vpmu_mode & XENPMU_MODE_ALL) && !is_hardware_domain(d)) )
>          return -EINVAL;
> 
>      if ( (params->vcpu >= d->max_vcpus) || (d->vcpu[params->vcpu] == NULL) )
> @@ -645,12 +661,14 @@ long do_xenpmu_op(unsigned int op,
> XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>      {
>      case XENPMU_mode_set:
>      {
> -        if ( (pmu_params.val & ~(XENPMU_MODE_SELF | XENPMU_MODE_HV)) ||
> +        if ( (pmu_params.val &
> +              ~(XENPMU_MODE_SELF | XENPMU_MODE_HV | XENPMU_MODE_ALL)) ||
>               (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) )
> +        if ( is_pv_32bit_vcpu(current) &&
> +             (pmu_params.val & (XENPMU_MODE_HV | XENPMU_MODE_ALL)) )
>              return -EINVAL;
> 
>          spin_lock(&vpmu_lock);
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index d2ff1a9..ac5622f 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -2654,6 +2654,10 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
>          case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
>                  if ( vpmu_msr || (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>                  {
> +                    if ( (vpmu_mode & XENPMU_MODE_ALL) &&
> +                         !is_hardware_domain(v->domain) )
> +                        break;
> +
>                      if ( vpmu_do_wrmsr(regs->ecx, msr_content, 0) )
>                          goto fail;
>                  }
> @@ -2777,6 +2781,15 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
>          case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
>                  if ( vpmu_msr || (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>                  {
> +
> +                    if ( (vpmu_mode & XENPMU_MODE_ALL) &&
> +                         !is_hardware_domain(v->domain) )
> +                    {
> +                        /* Don't leak PMU MSRs to unprivileged domains */
> +                        regs->eax = regs->edx = 0;
> +                        break;
> +                    }
> +
>                      if ( vpmu_do_rdmsr(regs->ecx, &val) )
>                          goto fail;
> 
> diff --git a/xen/include/public/pmu.h b/xen/include/public/pmu.h
> index 7a45783..1149678 100644
> --- a/xen/include/public/pmu.h
> +++ b/xen/include/public/pmu.h
> @@ -52,10 +52,13 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_params_t);
>   * - XENPMU_MODE_SELF:  Guests can profile themselves
>   * - XENPMU_MODE_HV:    Guests can profile themselves, dom0 profiles
>   *                      itself and Xen
> + * - XENPMU_MODE_ALL:   Only dom0 has access to VPMU and it profiles
> + *                      everyone: itself, the hypervisor and the guests.
>   */
>  #define XENPMU_MODE_OFF           0
>  #define XENPMU_MODE_SELF          (1<<0)
>  #define XENPMU_MODE_HV            (1<<1)
> +#define XENPMU_MODE_ALL           (1<<2)
> 
>  /*
>   * PMU features:
> --
> 1.8.1.4