[PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[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_ENABLED 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() 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>
---
 xen/arch/x86/cpu/vpmu.c    | 111 ++++++++++++++++++++++++++++++---------------
 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 ++-
 5 files changed, 84 insertions(+), 48 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 35a9403..283af80 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -456,32 +456,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_ENABLED) && !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_enabled(v) )
+        return 0;
 
     switch ( vendor )
     {
@@ -501,7 +490,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;
@@ -509,15 +498,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);
+
+    /*
+     * 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.
+     */
+    if ( !is_hardware_domain(v->domain) &&
+        (vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
+    {
+        vpmu_count++;
+        vpmu_set(vcpu_vpmu(v), VPMU_ENABLED);
+    }
+    else if ( is_hardware_domain(v->domain) &&
+              (vpmu_mode != XENPMU_MODE_OFF) )
+        vpmu_set(vcpu_vpmu(v), VPMU_ENABLED);
+
+    spin_unlock(&vpmu_lock);
+}
+
+static void put_vpmu(struct vcpu *v)
+{
+    if ( !vpmu_is_set(vcpu_vpmu(v), VPMU_ENABLED) )
+        return;
+
+    spin_lock(&vpmu_lock);
+
+    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_ENABLED);
     }
+    else if ( is_hardware_domain(v->domain) &&
+              (vpmu_mode != XENPMU_MODE_OFF) )
+        vpmu_reset(vcpu_vpmu(v), VPMU_ENABLED);
+
+    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)
@@ -526,7 +563,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);
 
@@ -551,11 +588,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)
@@ -565,13 +604,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_enabled(v) )
+        return -ENOENT;
+
     page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
     if ( !page )
         return -EINVAL;
@@ -582,9 +623,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 )
@@ -602,7 +640,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);
 
@@ -626,7 +665,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/domain.c b/xen/arch/x86/domain.c
index 71c0e3c..292442a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -458,6 +458,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;
 }
@@ -475,6 +477,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 ca2785c..27bfb3c 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1169,10 +1169,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;
@@ -1180,7 +1176,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 42f4fbd..d446e9e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -330,10 +330,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. */
@@ -354,7 +350,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..4e744dc 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_enabled(vcpu) vpmu_is_set(vcpu_vpmu(vcpu), VPMU_ENABLED)
 
 #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_ENABLED                        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_ENABLED should be altered by get/put_vpmu(). */
+    vpmu->flags &= VPMU_ENABLED;
 }
 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

Re: [PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[Jan Beulich]

>>> On 16.02.17 at 15:59, <boris.ostrovsky@oracle.com> wrote:
> 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_ENABLED 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.

I think VPMU_ENABLED is misleading, as it may as well mean the state
after the guest did enable it. How about VPMU_AVAILABLE?

> @@ -509,15 +498,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);
> +
> +    /*
> +     * 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.
> +     */
> +    if ( !is_hardware_domain(v->domain) &&
> +        (vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
> +    {
> +        vpmu_count++;
> +        vpmu_set(vcpu_vpmu(v), VPMU_ENABLED);
> +    }
> +    else if ( is_hardware_domain(v->domain) &&
> +              (vpmu_mode != XENPMU_MODE_OFF) )
> +        vpmu_set(vcpu_vpmu(v), VPMU_ENABLED);
> +
> +    spin_unlock(&vpmu_lock);
> +}
> +
> +static void put_vpmu(struct vcpu *v)
> +{
> +    if ( !vpmu_is_set(vcpu_vpmu(v), VPMU_ENABLED) )
> +        return;
> +
> +    spin_lock(&vpmu_lock);
> +
> +    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_ENABLED);

I think you need to re-check VPMU_ENABLED after acquiring the lock,
in order to avoid decrementing vpmu_count twice in case of a race.

Also this new model basically limits the opportunity to change the
mode to the case where no guest at all is running, iiuc. Previously
this would have been possible with any number of guests running,
as long as none of them actually used the vPMU.

>      }
> +    else if ( is_hardware_domain(v->domain) &&
> +              (vpmu_mode != XENPMU_MODE_OFF) )
> +        vpmu_reset(vcpu_vpmu(v), VPMU_ENABLED);
> +
> +    spin_unlock(&vpmu_lock);
> +}
> +
> +

No double blank lines please.

Jan


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

Re: [PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[Andrew Cooper]

On 16/02/17 16:59, Jan Beulich wrote:
>>>> On 16.02.17 at 15:59, <boris.ostrovsky@oracle.com> wrote:
>> 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_ENABLED 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.
> I think VPMU_ENABLED is misleading, as it may as well mean the state
> after the guest did enable it. How about VPMU_AVAILABLE?

The problem is a little deeper than that.

First, there is whether it is available based on hypervisor configuration.

Second, if it is available, has the toolstack chosen to allow the domain
to use it.  This should determine whether features/information are
visible in CPUID.

Finally, if vpmu is permitted, has the domain turned it on.

~Andrew

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

Re: [PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[Boris Ostrovsky]

On 02/16/2017 11:59 AM, Jan Beulich wrote:
>>>> On 16.02.17 at 15:59, <boris.ostrovsky@oracle.com> wrote:
>> 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_ENABLED 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.
>
> I think VPMU_ENABLED is misleading, as it may as well mean the state
> after the guest did enable it. How about VPMU_AVAILABLE?


OK. And then vpmu_enabled() should be renamed accordingly.

>
>> @@ -509,15 +498,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);
>> +
>> +    /*
>> +     * 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.
>> +     */
>> +    if ( !is_hardware_domain(v->domain) &&
>> +        (vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
>> +    {
>> +        vpmu_count++;
>> +        vpmu_set(vcpu_vpmu(v), VPMU_ENABLED);
>> +    }
>> +    else if ( is_hardware_domain(v->domain) &&
>> +              (vpmu_mode != XENPMU_MODE_OFF) )
>> +        vpmu_set(vcpu_vpmu(v), VPMU_ENABLED);
>> +
>> +    spin_unlock(&vpmu_lock);
>> +}
>> +
>> +static void put_vpmu(struct vcpu *v)
>> +{
>> +    if ( !vpmu_is_set(vcpu_vpmu(v), VPMU_ENABLED) )
>> +        return;
>> +
>> +    spin_lock(&vpmu_lock);
>> +
>> +    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_ENABLED);
>
> I think you need to re-check VPMU_ENABLED after acquiring the lock,
> in order to avoid decrementing vpmu_count twice in case of a race.

I can just move the check under the lock. This is never on critical path 
(and rarely will we come here with the bit off) so taking the lock 
unnecessarily shouldn't be a problem.

>
> Also this new model basically limits the opportunity to change the
> mode to the case where no guest at all is running, iiuc. Previously
> this would have been possible with any number of guests running,
> as long as none of them actually used the vPMU.

I don't think much changed. The only difference is that for PV guests we 
bump vpmu_count at VCPU creation as opposed to during the hypercall.

And HVM guests always incremented the count during vcpu_initialise().

With this patch we still can change between SELF and HV at any time, 
whether or not anyone is running.

-boris


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

Re: [PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[Boris Ostrovsky]

On 02/16/2017 12:09 PM, Andrew Cooper wrote:
> On 16/02/17 16:59, Jan Beulich wrote:
>>>>> On 16.02.17 at 15:59, <boris.ostrovsky@oracle.com> wrote:
>>> 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_ENABLED 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.
>> I think VPMU_ENABLED is misleading, as it may as well mean the state
>> after the guest did enable it. How about VPMU_AVAILABLE?
>
> The problem is a little deeper than that.
>
> First, there is whether it is available based on hypervisor configuration.

This bit is set only if vpmu_mode permits it.

>
> Second, if it is available, has the toolstack chosen to allow the domain
> to use it.  This should determine whether features/information are
> visible in CPUID.

You mean if toolstack masks out leaf 0xa on Intel? I chould check this 
in get_vpmu(). Is this information available by the time 
vcpu_initialise() runs?

>
> Finally, if vpmu is permitted, has the domain turned it on.

HVM domains always do and PV domains essentially too.


-boris

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

Re: [PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[Jan Beulich]

>>> On 16.02.17 at 19:09, <boris.ostrovsky@oracle.com> wrote:
> On 02/16/2017 12:09 PM, Andrew Cooper wrote:
>> Second, if it is available, has the toolstack chosen to allow the domain
>> to use it.  This should determine whether features/information are
>> visible in CPUID.
> 
> You mean if toolstack masks out leaf 0xa on Intel? I chould check this 
> in get_vpmu(). Is this information available by the time 
> vcpu_initialise() runs?

You shouldn't rely on this, even if it happened to be that way
right now. Instead you'd have to adjust accordingly when CPUID
info gets updated by the tool stack (update_domain_cpuid_info()
as the root hook point). Which gets us to another point: Shouldn't
we disallow CPUID info updates after the guest started running?
Or do we mean to trust the tool stack to not do this if it doesn't
want to confuse a guest?

>> Finally, if vpmu is permitted, has the domain turned it on.
> 
> HVM domains always do and PV domains essentially too.

How that? Or are you talking of just modern Linux guests?

Jan


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

Re: [PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[Jan Beulich]

>>> On 16.02.17 at 18:31, <boris.ostrovsky@oracle.com> wrote:
> On 02/16/2017 11:59 AM, Jan Beulich wrote:
>> Also this new model basically limits the opportunity to change the
>> mode to the case where no guest at all is running, iiuc. Previously
>> this would have been possible with any number of guests running,
>> as long as none of them actually used the vPMU.
> 
> I don't think much changed. The only difference is that for PV guests we 
> bump vpmu_count at VCPU creation as opposed to during the hypercall.
> 
> And HVM guests always incremented the count during vcpu_initialise().

True, so my earlier statement needs to be limited to PV guests.

Jan


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

Re: [PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[Boris Ostrovsky]

On 02/17/2017 03:27 AM, Jan Beulich wrote:
>>>> On 16.02.17 at 19:09, <boris.ostrovsky@oracle.com> wrote:
>> On 02/16/2017 12:09 PM, Andrew Cooper wrote:
>>> Second, if it is available, has the toolstack chosen to allow the domain
>>> to use it.  This should determine whether features/information are
>>> visible in CPUID.
>>
>> You mean if toolstack masks out leaf 0xa on Intel? I chould check this
>> in get_vpmu(). Is this information available by the time
>> vcpu_initialise() runs?
>
> You shouldn't rely on this, even if it happened to be that way
> right now. Instead you'd have to adjust accordingly when CPUID
> info gets updated by the tool stack (update_domain_cpuid_info()
> as the root hook point).

Right, that's what I was going to do --- destroy VPMU if CPUID indicates 
no support.


> Which gets us to another point: Shouldn't
> we disallow CPUID info updates after the guest started running?
> Or do we mean to trust the tool stack to not do this if it doesn't
> want to confuse a guest?

I believe currently it's the latter. I don't see anything preventing 
CPUID update to a running guest (except for pausing it while the update 
is in progress).

>
>>> Finally, if vpmu is permitted, has the domain turned it on.
>>
>> HVM domains always do and PV domains essentially too.
>
> How that? Or are you talking of just modern Linux guests?

The newer ones.


-boris

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

Re: [PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[Boris Ostrovsky]

On 02/17/2017 03:28 AM, Jan Beulich wrote:
>>>> On 16.02.17 at 18:31, <boris.ostrovsky@oracle.com> wrote:
>> On 02/16/2017 11:59 AM, Jan Beulich wrote:
>>> Also this new model basically limits the opportunity to change the
>>> mode to the case where no guest at all is running, iiuc. Previously
>>> this would have been possible with any number of guests running,
>>> as long as none of them actually used the vPMU.
>>
>> I don't think much changed. The only difference is that for PV guests we
>> bump vpmu_count at VCPU creation as opposed to during the hypercall.
>>
>> And HVM guests always incremented the count during vcpu_initialise().
>
> True, so my earlier statement needs to be limited to PV guests.

Which I think is still OK: as long as we promised a guest (via 
CPUID/vpmu_mode at the time of guest creation) that PMU is available we 
shouldn't be able to take it away by changing vpmu_mode, whether or not 
the guest is actually using it.

-boris

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

Re: [PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[Andrew Cooper]

On 17/02/17 14:17, Boris Ostrovsky wrote:
>
>
> On 02/17/2017 03:27 AM, Jan Beulich wrote:
>>>>> On 16.02.17 at 19:09, <boris.ostrovsky@oracle.com> wrote:
>>> On 02/16/2017 12:09 PM, Andrew Cooper wrote:
>>>> Second, if it is available, has the toolstack chosen to allow the
>>>> domain
>>>> to use it.  This should determine whether features/information are
>>>> visible in CPUID.
>>>
>>> You mean if toolstack masks out leaf 0xa on Intel? I chould check this
>>> in get_vpmu(). Is this information available by the time
>>> vcpu_initialise() runs?
>>
>> You shouldn't rely on this, even if it happened to be that way
>> right now. Instead you'd have to adjust accordingly when CPUID
>> info gets updated by the tool stack (update_domain_cpuid_info()
>> as the root hook point).
>
> Right, that's what I was going to do --- destroy VPMU if CPUID
> indicates no support.

From a CPUID side of things, the way I would eventually like things to
work is this:

* Hardware support and Xen command line options influence the visibility
of vPMU bits in {hvm,pv}_max_policy.

* Using *_max_policy and domain.cfg settings, a toolstack opts in to
allowing vPMU by setting vPMU details in the domains policy.

The eventual plan for toolstack API will be that the entire policy is
got/set at once (rather than individual leaves as it currently exists). 
Xen audits the toolstacks choice against {hvm,pv}_max_policy, rejecting
the update wholesale if it is bad.

Then, Xen will compare the old and proposed new policies to see what has
changed, and use this to enable subsystems for a domain (either
passively by having the subsystem rely on CPUID values, or actively by
calling into the subsystem to initialise things).  nested-virt was my
main target here, but vPMU looks like it fits into the same category, as
well as some of the debugging facilities such as LBR etc.


I realise that this infrastructure doesn't all exist yet,  but it would
be helpful if your fix can easily be turned into API matching the above,
when I complete the missing CPUID pieces.

>
>
>> Which gets us to another point: Shouldn't
>> we disallow CPUID info updates after the guest started running?
>> Or do we mean to trust the tool stack to not do this if it doesn't
>> want to confuse a guest?
>
> I believe currently it's the latter. I don't see anything preventing
> CPUID update to a running guest (except for pausing it while the
> update is in progress).

This is on my TODO list longer than d->creation_finished has existed. 
The CPUID policy should be immutable even by the toolstack once the
guest is running.

~Andrew

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

Re: [PATCH] x86/vpmu: Add get/put_vpmu() and VPMU_ENABLED

[Boris Ostrovsky]

On 02/17/2017 10:58 AM, Andrew Cooper wrote:
> On 17/02/17 14:17, Boris Ostrovsky wrote:
>>
>>
>> On 02/17/2017 03:27 AM, Jan Beulich wrote:
>>>>>> On 16.02.17 at 19:09, <boris.ostrovsky@oracle.com> wrote:
>>>> On 02/16/2017 12:09 PM, Andrew Cooper wrote:
>>>>> Second, if it is available, has the toolstack chosen to allow the
>>>>> domain
>>>>> to use it.  This should determine whether features/information are
>>>>> visible in CPUID.
>>>>
>>>> You mean if toolstack masks out leaf 0xa on Intel? I chould check this
>>>> in get_vpmu(). Is this information available by the time
>>>> vcpu_initialise() runs?
>>>
>>> You shouldn't rely on this, even if it happened to be that way
>>> right now. Instead you'd have to adjust accordingly when CPUID
>>> info gets updated by the tool stack (update_domain_cpuid_info()
>>> as the root hook point).
>>
>> Right, that's what I was going to do --- destroy VPMU if CPUID
>> indicates no support.
>
> From a CPUID side of things, the way I would eventually like things to
> work is this:
>
> * Hardware support and Xen command line options influence the visibility
> of vPMU bits in {hvm,pv}_max_policy.
>
> * Using *_max_policy and domain.cfg settings, a toolstack opts in to
> allowing vPMU by setting vPMU details in the domains policy.
>
> The eventual plan for toolstack API will be that the entire policy is
> got/set at once (rather than individual leaves as it currently exists).
> Xen audits the toolstacks choice against {hvm,pv}_max_policy, rejecting
> the update wholesale if it is bad.
>
> Then, Xen will compare the old and proposed new policies to see what has
> changed, and use this to enable subsystems for a domain (either
> passively by having the subsystem rely on CPUID values, or actively by
> calling into the subsystem to initialise things).  nested-virt was my
> main target here, but vPMU looks like it fits into the same category, as
> well as some of the debugging facilities such as LBR etc.
>
>
> I realise that this infrastructure doesn't all exist yet,  but it would
> be helpful if your fix can easily be turned into API matching the above,
> when I complete the missing CPUID pieces.


Currently, since we don't know whether the toolstack will be updating 
leaf 0xa, we have to initialize VPMU at VCPU initialization time and 
then possibly destroy it if CPUID update does come in.

When your suggested infrastructure is in place it we can defer VPMU 
initialization until after the policy has been loaded. And I think it 
should be a pretty easy conversion.

Let me send what I have now.

-boris

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