[PATCH v2 0/2] Be more strict about writing to Intel VPMU registers

[PATCH v2 0/2] Be more strict about writing to Intel VPMU registers

[Boris Ostrovsky]

* Add more checks when writing VPMU control registers
* Explicitly disable PEBS since calculating reserved bits in MSR_IA32_PEBS_ENABLE
  is somewhat non-trivial (and pointless since PEBS is not supported)


Boris Ostrovsky (2):
  x86/VPMU: Check more carefully which bits are allowed to be written
    to MSRs
  x86/VPMU: Don't allow any non-zero writes to MSR_IA32_PEBS_ENABLE

 xen/arch/x86/cpu/vpmu_intel.c |   31 ++++++++++++++++++++++---------
 1 files changed, 22 insertions(+), 9 deletions(-)

[PATCH v2 1/2] x86/VPMU: Check more carefully which bits are allowed to be written to MSRs

[Boris Ostrovsky]

Current Intel VPMU emulation needs to perform more checks when writing
PMU MSRs on guest's behalf:
* MSR_CORE_PERF_GLOBAL_CTRL is not checked at all
* MSR_CORE_PERF_FIXED_CTR_CTRL has more reserved bits in PMU version 2
* MSR_CORE_PERF_GLOBAL_OVF_CTRL's bit 61 is allowed on versions greater
* than 2.

We can also use precomputed mask in core2_vpmu_do_interrupt().

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/cpu/vpmu_intel.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 3eff1ae..03cfe50 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -87,7 +87,7 @@ static unsigned int __read_mostly arch_pmc_cnt, fixed_pmc_cnt;
 /* Masks used for testing whether and MSR is valid */
 #define ARCH_CTRL_MASK  (~((1ull << 32) - 1) | (1ull << 21))
 static uint64_t __read_mostly fixed_ctrl_mask, fixed_counters_mask;
-static uint64_t __read_mostly global_ovf_ctrl_mask;
+static uint64_t __read_mostly global_ovf_ctrl_mask, global_ctrl_mask;
 
 /* Total size of PMU registers block (copied to/from PV(H) guest) */
 static unsigned int __read_mostly regs_sz;
@@ -392,6 +392,8 @@ static int core2_vpmu_verify(struct vcpu *v)
 
     if ( core2_vpmu_cxt->global_ovf_ctrl & global_ovf_ctrl_mask )
         return -EINVAL;
+    if ( core2_vpmu_cxt->global_ctrl & global_ctrl_mask )
+        return -EINVAL;
 
     fixed_ctrl = core2_vpmu_cxt->fixed_ctrl;
     if ( fixed_ctrl & fixed_ctrl_mask )
@@ -627,6 +629,8 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
         gdprintk(XENLOG_WARNING, "Guest setting of DTS is ignored.\n");
         return 0;
     case MSR_CORE_PERF_GLOBAL_CTRL:
+        if ( msr_content & global_ctrl_mask )
+            return -EINVAL;
         core2_vpmu_cxt->global_ctrl = msr_content;
         break;
     case MSR_CORE_PERF_FIXED_CTR_CTRL:
@@ -820,7 +824,7 @@ static int core2_vpmu_do_interrupt(struct cpu_user_regs *regs)
         if ( is_pmc_quirk )
             handle_pmc_quirk(msr_content);
         core2_vpmu_cxt->global_status |= msr_content;
-        msr_content = 0xC000000700000000 | ((1 << arch_pmc_cnt) - 1);
+        msr_content = ~global_ovf_ctrl_mask;
         wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content);
     }
     else
@@ -1001,10 +1005,20 @@ int __init core2_vpmu_init(void)
     full_width_write = (caps >> 13) & 1;
 
     fixed_ctrl_mask = ~((1ull << (fixed_pmc_cnt * FIXED_CTR_CTRL_BITS)) - 1);
+    if ( version == 2 )
+        fixed_ctrl_mask |= 0x444;
     fixed_counters_mask = ~((1ull << core2_get_bitwidth_fix_count()) - 1);
+    global_ctrl_mask = ~((((1ULL << fixed_pmc_cnt) - 1) << 32) |
+                         ((1ULL << arch_pmc_cnt) - 1));
     global_ovf_ctrl_mask = ~(0xC000000000000000 |
                              (((1ULL << fixed_pmc_cnt) - 1) << 32) |
                              ((1ULL << arch_pmc_cnt) - 1));
+    if ( version > 2 )
+        /*
+         * Even though we don't support Uncore counters guests should be
+         * able to clear all available overflows.
+         */
+        global_ovf_ctrl_mask &= ~(1ULL << 61);
 
     regs_sz = (sizeof(struct xen_pmu_intel_ctxt) - regs_off) +
               sizeof(uint64_t) * fixed_pmc_cnt +
-- 
1.7.1

[PATCH v2 2/2] x86/VPMU: Don't allow any non-zero writes to MSR_IA32_PEBS_ENABLE

[Boris Ostrovsky]

Calculation reserved bits for MSR_IA32_PEBS_ENABLE is model-dependent
and since we don't support PEBS anyway we shouldn't allow any writes to
it (but let's still permit guests wishing to disable PEBS).

We should also report PEBS as unsupported to HVM, just like we do on PV.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/cpu/vpmu_intel.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 03cfe50..a179717 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -264,7 +264,6 @@ static void core2_vpmu_set_msr_bitmap(unsigned long *msr_bitmap)
          clear_bit(msraddr_to_bitpos(MSR_P6_EVNTSEL(i)), msr_bitmap);
 
     clear_bit(msraddr_to_bitpos(MSR_CORE_PERF_FIXED_CTR_CTRL), msr_bitmap);
-    clear_bit(msraddr_to_bitpos(MSR_IA32_PEBS_ENABLE), msr_bitmap);
     clear_bit(msraddr_to_bitpos(MSR_IA32_DS_AREA), msr_bitmap);
 }
 
@@ -296,7 +295,6 @@ static void core2_vpmu_unset_msr_bitmap(unsigned long *msr_bitmap)
         set_bit(msraddr_to_bitpos(MSR_P6_EVNTSEL(i)), msr_bitmap);
 
     set_bit(msraddr_to_bitpos(MSR_CORE_PERF_FIXED_CTR_CTRL), msr_bitmap);
-    set_bit(msraddr_to_bitpos(MSR_IA32_PEBS_ENABLE), msr_bitmap);
     set_bit(msraddr_to_bitpos(MSR_IA32_DS_AREA), msr_bitmap);
 }
 
@@ -368,7 +366,6 @@ static inline void __core2_vpmu_load(struct vcpu *v)
     wrmsrl(MSR_CORE_PERF_FIXED_CTR_CTRL, core2_vpmu_cxt->fixed_ctrl);
     if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) )
         wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
-    wrmsrl(MSR_IA32_PEBS_ENABLE, core2_vpmu_cxt->pebs_enable);
 
     if ( !has_hvm_container_vcpu(v) )
     {
@@ -394,6 +391,8 @@ static int core2_vpmu_verify(struct vcpu *v)
         return -EINVAL;
     if ( core2_vpmu_cxt->global_ctrl & global_ctrl_mask )
         return -EINVAL;
+    if ( core2_vpmu_cxt->pebs_enable )
+        return -EINVAL;
 
     fixed_ctrl = core2_vpmu_cxt->fixed_ctrl;
     if ( fixed_ctrl & fixed_ctrl_mask )
@@ -606,10 +605,9 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
                  "MSR_PERF_GLOBAL_STATUS(0x38E)!\n");
         return -EINVAL;
     case MSR_IA32_PEBS_ENABLE:
-        if ( msr_content & 1 )
-            gdprintk(XENLOG_WARNING, "Guest is trying to enable PEBS, "
-                     "which is not supported.\n");
-        core2_vpmu_cxt->pebs_enable = msr_content;
+        if ( msr_content )
+            /* PEBS is reported as unavailable in MSR_IA32_MISC_ENABLE */
+            return -EINVAL;
         return 0;
     case MSR_IA32_DS_AREA:
         if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
@@ -733,6 +731,7 @@ static int core2_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content)
         /* Extension for BTS */
         if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
             *msr_content &= ~MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;
+        *msr_content |= MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL;
     }
 
     return 0;
-- 
1.7.1

Re: [PATCH v2 2/2] x86/VPMU: Don't allow any non-zero writes to MSR_IA32_PEBS_ENABLE

[Tian, Kevin]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Thursday, December 24, 2015 12:52 AM
> 
> Calculation reserved bits for MSR_IA32_PEBS_ENABLE is model-dependent
> and since we don't support PEBS anyway we shouldn't allow any writes to
> it (but let's still permit guests wishing to disable PEBS).
> 
> We should also report PEBS as unsupported to HVM, just like we do on PV.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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