[PATCH v4 0/2] VPMU management update

[PATCH v4 0/2] VPMU management update

[Boris Ostrovsky]

Changes to how the hypervisor allocates and keeps track of active
VPMUs. The main purpose is to fix vpmu_enabled() reporting but this
also makes VPMU ref counting more consistent.

Changes in v4:
* Rebased to latest staging (xen/arch/x86/cpuid.c changes in patch1)
* Fixed whitespace in patch 2.

Boris Ostrovsky (2):
  x86/vpmu: Add get/put_vpmu() and VPMU_AVAILABLE
  x86/vpmu: Disable VPMU if guest's CPUID indicates no PMU support

 xen/arch/x86/cpu/vpmu.c       | 111 ++++++++++++++++++++++++++++--------------
 xen/arch/x86/cpu/vpmu_intel.c |   4 ++
 xen/arch/x86/cpuid.c          |   6 +--
 xen/arch/x86/domain.c         |   5 ++
 xen/arch/x86/domctl.c         |  14 ++++++
 xen/arch/x86/hvm/svm/svm.c    |   5 --
 xen/arch/x86/hvm/vmx/vmx.c    |   5 --
 xen/include/asm-x86/cpuid.h   |  12 +++++
 xen/include/asm-x86/vpmu.h    |   6 ++-
 9 files changed, 117 insertions(+), 51 deletions(-)

-- 
1.8.3.1


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

[PATCH v4 1/2] x86/vpmu: Add get/put_vpmu() and VPMU_AVAILABLE

[Boris Ostrovsky]

vpmu_enabled() (used by hvm/pv_cpuid() to properly report 0xa leaf
for Intel processors) is based on the value of VPMU_CONTEXT_ALLOCATED
bit. This is problematic:
* For HVM guests VPMU context is allocated lazily, during the first
  access to VPMU MSRs. Since the leaf is typically queried before guest
  attempts to read or write the MSRs it is likely that CPUID will report
  no PMU support
* For PV guests the context is allocated eagerly but only in responce to
  guest's XENPMU_init hypercall. There is a chance that the guest will
  try to read CPUID before making this hypercall.

This patch introduces VPMU_AVAILABLE flag which is set (subject to vpmu_mode
constraints) during VCPU initialization for both PV and HVM guests. Since
this flag is expected to be managed together with vpmu_count, get/put_vpmu()
are added to simplify code.

vpmu_enabled() (renamed to vpmu_available()) can now use this new flag.

(As a side affect this patch also fixes a race in pvpmu_init() where we
increment vcpu_count in vpmu_initialise() after checking vpmu_mode)

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/cpu/vpmu.c    | 111 ++++++++++++++++++++++++++++++---------------
 xen/arch/x86/cpuid.c       |   6 +--
 xen/arch/x86/domain.c      |   5 ++
 xen/arch/x86/hvm/svm/svm.c |   5 --
 xen/arch/x86/hvm/vmx/vmx.c |   5 --
 xen/include/asm-x86/vpmu.h |   6 ++-
 6 files changed, 87 insertions(+), 51 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index c8615e8..1957dea 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -455,32 +455,21 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
     return 0;
 }
 
-void vpmu_initialise(struct vcpu *v)
+static int vpmu_arch_initialise(struct vcpu *v)
 {
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
     uint8_t vendor = current_cpu_data.x86_vendor;
     int ret;
-    bool_t is_priv_vpmu = is_hardware_domain(v->domain);
 
     BUILD_BUG_ON(sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ);
     BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ);
     BUILD_BUG_ON(sizeof(struct xen_pmu_regs) > XENPMU_REGS_PAD_SZ);
     BUILD_BUG_ON(sizeof(struct compat_pmu_regs) > XENPMU_REGS_PAD_SZ);
 
-    ASSERT(!vpmu->flags && !vpmu->context);
+    ASSERT(!(vpmu->flags & ~VPMU_AVAILABLE) && !vpmu->context);
 
-    if ( !is_priv_vpmu )
-    {
-        /*
-         * Count active VPMUs so that we won't try to change vpmu_mode while
-         * they are in use.
-         * vpmu_mode can be safely updated while dom0's VPMUs are active and
-         * so we don't need to include it in the count.
-         */
-        spin_lock(&vpmu_lock);
-        vpmu_count++;
-        spin_unlock(&vpmu_lock);
-    }
+    if ( !vpmu_available(v) )
+        return 0;
 
     switch ( vendor )
     {
@@ -500,7 +489,7 @@ void vpmu_initialise(struct vcpu *v)
             opt_vpmu_enabled = 0;
             vpmu_mode = XENPMU_MODE_OFF;
         }
-        return; /* Don't bother restoring vpmu_count, VPMU is off forever */
+        return -EINVAL;
     }
 
     vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | APIC_LVT_MASKED;
@@ -508,15 +497,63 @@ void vpmu_initialise(struct vcpu *v)
     if ( ret )
         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) ||
-          (vpmu_mode == XENPMU_MODE_ALL)) )
+    return ret;
+}
+
+static void get_vpmu(struct vcpu *v)
+{
+    spin_lock(&vpmu_lock);
+
+    /*
+     * Keep count of VPMUs in the system so that we won't try to change
+     * vpmu_mode while a guest might be using one.
+     * vpmu_mode can be safely updated while dom0's VPMUs are active and
+     * so we don't need to include it in the count.
+     */
+    if ( !is_hardware_domain(v->domain) &&
+        (vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
+    {
+        vpmu_count++;
+        vpmu_set(vcpu_vpmu(v), VPMU_AVAILABLE);
+    }
+    else if ( is_hardware_domain(v->domain) &&
+              (vpmu_mode != XENPMU_MODE_OFF) )
+        vpmu_set(vcpu_vpmu(v), VPMU_AVAILABLE);
+
+    spin_unlock(&vpmu_lock);
+}
+
+static void put_vpmu(struct vcpu *v)
+{
+    spin_lock(&vpmu_lock);
+
+    if ( !vpmu_available(v) )
+        goto out;
+
+    if ( !is_hardware_domain(v->domain) &&
+         (vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
     {
-        spin_lock(&vpmu_lock);
         vpmu_count--;
-        spin_unlock(&vpmu_lock);
+        vpmu_reset(vcpu_vpmu(v), VPMU_AVAILABLE);
     }
+    else if ( is_hardware_domain(v->domain) &&
+              (vpmu_mode != XENPMU_MODE_OFF) )
+        vpmu_reset(vcpu_vpmu(v), VPMU_AVAILABLE);
+
+ out:
+    spin_unlock(&vpmu_lock);
+}
+
+void vpmu_initialise(struct vcpu *v)
+{
+    get_vpmu(v);
+
+    /*
+     * Guests without LAPIC (i.e. PV) call vpmu_arch_initialise()
+     * from pvpmu_init().
+     */
+    if ( has_vlapic(v->domain) && vpmu_arch_initialise(v) )
+        put_vpmu(v);
 }
 
 static void vpmu_clear_last(void *arg)
@@ -525,7 +562,7 @@ static void vpmu_clear_last(void *arg)
         this_cpu(last_vcpu) = NULL;
 }
 
-void vpmu_destroy(struct vcpu *v)
+static void vpmu_arch_destroy(struct vcpu *v)
 {
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
 
@@ -550,11 +587,13 @@ void vpmu_destroy(struct vcpu *v)
                          vpmu_save_force, v, 1);
          vpmu->arch_vpmu_ops->arch_vpmu_destroy(v);
     }
+}
 
-    spin_lock(&vpmu_lock);
-    if ( !is_hardware_domain(v->domain) )
-        vpmu_count--;
-    spin_unlock(&vpmu_lock);
+void vpmu_destroy(struct vcpu *v)
+{
+    vpmu_arch_destroy(v);
+
+    put_vpmu(v);
 }
 
 static int pvpmu_init(struct domain *d, xen_pmu_params_t *params)
@@ -564,13 +603,15 @@ 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) ||
-         ((vpmu_mode & XENPMU_MODE_ALL) && !is_hardware_domain(d)) )
-        return -EINVAL;
-
     if ( (params->vcpu >= d->max_vcpus) || (d->vcpu[params->vcpu] == NULL) )
         return -EINVAL;
 
+    v = d->vcpu[params->vcpu];
+    vpmu = vcpu_vpmu(v);
+
+    if ( !vpmu_available(v) )
+        return -ENOENT;
+
     page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
     if ( !page )
         return -EINVAL;
@@ -581,9 +622,6 @@ static int pvpmu_init(struct domain *d, xen_pmu_params_t *params)
         return -EINVAL;
     }
 
-    v = d->vcpu[params->vcpu];
-    vpmu = vcpu_vpmu(v);
-
     spin_lock(&vpmu->vpmu_lock);
 
     if ( v->arch.vpmu.xenpmu_data )
@@ -601,7 +639,8 @@ static int pvpmu_init(struct domain *d, xen_pmu_params_t *params)
         return -ENOMEM;
     }
 
-    vpmu_initialise(v);
+    if ( vpmu_arch_initialise(v) )
+        put_vpmu(v);
 
     spin_unlock(&vpmu->vpmu_lock);
 
@@ -625,7 +664,7 @@ static void pvpmu_finish(struct domain *d, xen_pmu_params_t *params)
     vpmu = vcpu_vpmu(v);
     spin_lock(&vpmu->vpmu_lock);
 
-    vpmu_destroy(v);
+    vpmu_arch_destroy(v);
     xenpmu_data = vpmu->xenpmu_data;
     vpmu->xenpmu_data = NULL;
 
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 0e17670..0dd35dc 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -591,7 +591,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
     {
     case 0x0000000a: /* Architectural Performance Monitor Features (Intel) */
         if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
-             !vpmu_enabled(curr) )
+             !vpmu_available(curr) )
             goto unsupported;
 
         /* Report at most version 3 since that's all we currently emulate. */
@@ -629,7 +629,7 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
         break;
 
     case 0x0000000a: /* 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 || !vpmu_available(v) )
         {
             *res = EMPTY_LEAF;
             break;
@@ -752,7 +752,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
         res->b |= (v->vcpu_id * 2) << 24;
 
         /* TODO: Rework vPMU control in terms of toolstack choices. */
-        if ( vpmu_enabled(v) &&
+        if ( vpmu_available(v) &&
              vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) )
         {
             res->d |= cpufeat_mask(X86_FEATURE_DS);
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 103a3f4..479aee6 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -471,6 +471,8 @@ int vcpu_initialise(struct vcpu *v)
         if ( is_pv_domain(d) )
             xfree(v->arch.pv_vcpu.trap_ctxt);
     }
+    else if ( !is_idle_domain(v->domain) )
+        vpmu_initialise(v);
 
     return rc;
 }
@@ -488,6 +490,9 @@ void vcpu_destroy(struct vcpu *v)
 
     vcpu_destroy_fpu(v);
 
+    if ( !is_idle_domain(v->domain) )
+        vpmu_destroy(v);
+
     if ( has_hvm_container_vcpu(v) )
         hvm_vcpu_destroy(v);
     else
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index f4ea09e..b88f91d 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1172,10 +1172,6 @@ static int svm_vcpu_initialise(struct vcpu *v)
         return rc;
     }
 
-    /* PVH's VPMU is initialized via hypercall */
-    if ( has_vlapic(v->domain) )
-        vpmu_initialise(v);
-
     svm_guest_osvw_init(v);
 
     return 0;
@@ -1183,7 +1179,6 @@ static int svm_vcpu_initialise(struct vcpu *v)
 
 static void svm_vcpu_destroy(struct vcpu *v)
 {
-    vpmu_destroy(v);
     svm_destroy_vmcb(v);
     passive_domain_destroy(v);
 }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 60b7c92..5b1717d 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -332,10 +332,6 @@ static int vmx_vcpu_initialise(struct vcpu *v)
         }
     }
 
-    /* PVH's VPMU is initialized via hypercall */
-    if ( has_vlapic(v->domain) )
-        vpmu_initialise(v);
-
     vmx_install_vlapic_mapping(v);
 
     /* %eax == 1 signals full real-mode support to the guest loader. */
@@ -356,7 +352,6 @@ static void vmx_vcpu_destroy(struct vcpu *v)
      */
     vmx_vcpu_disable_pml(v);
     vmx_destroy_vmcs(v);
-    vpmu_destroy(v);
     passive_domain_destroy(v);
 }
 
diff --git a/xen/include/asm-x86/vpmu.h b/xen/include/asm-x86/vpmu.h
index e50618f..5e778ab 100644
--- a/xen/include/asm-x86/vpmu.h
+++ b/xen/include/asm-x86/vpmu.h
@@ -25,7 +25,7 @@
 
 #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 vpmu_available(vcpu) vpmu_is_set(vcpu_vpmu(vcpu), VPMU_AVAILABLE)
 
 #define MSR_TYPE_COUNTER            0
 #define MSR_TYPE_CTRL               1
@@ -74,6 +74,7 @@ struct vpmu_struct {
 #define VPMU_PASSIVE_DOMAIN_ALLOCATED       0x20
 /* PV(H) guests: VPMU registers are accessed by guest from shared page */
 #define VPMU_CACHED                         0x40
+#define VPMU_AVAILABLE                      0x80
 
 /* Intel-specific VPMU features */
 #define VPMU_CPU_HAS_DS                     0x100 /* Has Debug Store */
@@ -89,7 +90,8 @@ static inline void vpmu_reset(struct vpmu_struct *vpmu, const u32 mask)
 }
 static inline void vpmu_clear(struct vpmu_struct *vpmu)
 {
-    vpmu->flags = 0;
+    /* VPMU_AVAILABLE should be altered by get/put_vpmu(). */
+    vpmu->flags &= VPMU_AVAILABLE;
 }
 static inline bool_t vpmu_is_set(const struct vpmu_struct *vpmu, const u32 mask)
 {
-- 
1.8.3.1


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

[PATCH v4 2/2] x86/vpmu: Disable VPMU if guest's CPUID indicates no PMU support

[Boris Ostrovsky]

When toolstack overrides Intel CPUID leaf 0xa's PMU version with an
invalid value VPMU should not be available to the guest.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/cpu/vpmu_intel.c |  4 ++++
 xen/arch/x86/domctl.c         | 14 ++++++++++++++
 xen/include/asm-x86/cpuid.h   | 12 ++++++++++++
 3 files changed, 30 insertions(+)

diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 0ce68f1..626bed5 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -883,6 +883,10 @@ int vmx_vpmu_initialise(struct vcpu *v)
     if ( vpmu_mode == XENPMU_MODE_OFF )
         return 0;
 
+    if ( v->domain->arch.cpuid->basic.pmu_version <= 1 ||
+         v->domain->arch.cpuid->basic.pmu_version >= 5 )
+        return -EINVAL;
+
     if ( (arch_pmc_cnt + fixed_pmc_cnt) == 0 )
         return -EINVAL;
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index d5f8d2f..02b48e8 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -252,6 +252,20 @@ static int update_domain_cpuid_info(struct domain *d,
         }
         break;
 
+    case 0xa:
+        if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
+            break;
+
+        /* If PMU version is zero then the guest doesn't have VPMU */
+        if ( p->basic.pmu_version == 0 )
+        {
+            struct vcpu *v;
+
+            for_each_vcpu ( d, v )
+                vpmu_destroy(v);
+        }
+        break;
+
     case 0xd:
         if ( ctl->input[1] != 1 )
             break;
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 6d1990b..aa482b7 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -122,6 +122,18 @@ struct cpuid_policy
 
             /* Leaf 0x2 - TLB/Cache/Prefetch. */
             uint8_t l2_nr_queries; /* Documented as fixed to 1. */
+            uint8_t l2_desc[15];
+
+            uint64_t :64, :64; /* Leaf 0x3 - PSN. */
+            uint64_t :64, :64; /* Leaf 0x4 - Structured Cache. */
+            uint64_t :64, :64; /* Leaf 0x5 - MONITOR. */
+            uint64_t :64, :64; /* Leaf 0x6 - Therm/Perf. */
+            uint64_t :64, :64; /* Leaf 0x7 - Structured Features. */
+            uint64_t :64, :64; /* Leaf 0x8 - rsvd */
+            uint64_t :64, :64; /* Leaf 0x9 - DCA */
+
+            /* Leaf 0xa - Intel PMU. */
+            uint8_t pmu_version;
         };
     } basic;
 
-- 
1.8.3.1


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

Re: [PATCH v4 2/2] x86/vpmu: Disable VPMU if guest's CPUID indicates no PMU support

[Tian, Kevin]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Wednesday, March 01, 2017 11:07 PM
> 
> When toolstack overrides Intel CPUID leaf 0xa's PMU version with an
> invalid value VPMU should not be available to the guest.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

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