Re: [Xen-devel] [PATCH v2] x86/vpmu_intel: Fix hypervisor crash by masking PC bit in MSR_P6_EVNTSEL

[Mohit Gambhir <mohit.gambhir@xxxxxxxxxx>]

On 04/26/2017 02:19 PM, Andrew Cooper wrote:

On 26/04/17 19:11, Mohit Gambhir wrote:

Setting Pin Control (PC) bit (19) in MSR_P6_EVNTSEL results in a General
Protection Fault and thus results in a hypervisor crash. This patch fixes the
crash by masking PC bit and returning an error in case any guest tries to write
to it.

Signed-off-by: Mohit Gambhir <mohit.gambhir@xxxxxxxxxx>

Out of interest, which hardware has this been observed on?

~Andrew

I have tested this on two Intel Broadwell machines.

Also, Boris Ostrovsky brought this KVM patch to our attention in the previous patch thread,
which makes me believe that it may be a wider problem.

Quoting his email from before -

"""
I checked how Linux treats this bit and there is this interesting commit:


commit a7b9d2ccc3d86303ee9314612d301966e04011c7
Author: Gleb Natapov <gleb@xxxxxxxxxx>
Date:   Sun Feb 26 16:55:40 2012 +0200

    KVM: PMU: warn when pin control is set in eventsel msr
   
    Print warning once if pin control bit is set in eventsel msr since
    emulation does not support it yet.
   
    Signed-off-by: Gleb Natapov <gleb@xxxxxxxxxx>
    Signed-off-by: Avi Kivity <avi@xxxxxxxxxx>

diff --git a/arch/x86/include/asm/perf_event.h
b/arch/x86/include/asm/perf_event.h
index 096c975..f1f7182 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -23,6 +23,7 @@
 #define ARCH_PERFMON_EVENTSEL_USR                      (1ULL << 16)
 #define ARCH_PERFMON_EVENTSEL_OS                       (1ULL << 17)
 #define ARCH_PERFMON_EVENTSEL_EDGE                     (1ULL << 18)
+#define ARCH_PERFMON_EVENTSEL_PIN_CONTROL              (1ULL << 19)
 #define ARCH_PERFMON_EVENTSEL_INT                      (1ULL << 20)
 #define ARCH_PERFMON_EVENTSEL_ANY                      (1ULL << 21)
 #define ARCH_PERFMON_EVENTSEL_ENABLE                   (1ULL << 22)
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 3e48c1d..6af9a54 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -210,6 +210,9 @@ static void reprogram_gp_counter(struct kvm_pmc
*pmc, u64 eventsel)
        unsigned config, type = PERF_TYPE_RAW;
        u8 event_select, unit_mask;
 
+       if (eventsel & ARCH_PERFMON_EVENTSEL_PIN_CONTROL)
+               printk_once("kvm pmu: pin control bit is ignored\n");
+
        pmc->eventsel = eventsel;
 
        stop_counter(pmc);



-boris

"""

---
v2 of this patch takes a different approach to fixing the hypervisor crash.
As stated in the commit message v2 masks PC flag control bit unlike v1 that 
used wrmsr_safe while writing to the MSR. v1 posed a complication in returning
the error to the HVM guests.

With this approach the writes to PC bits are protected for both native MSR 
writes and VMX data structure writes (for HVM guests)
---
 xen/arch/x86/cpu/vpmu_intel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 3f0322c..6d768cb 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -76,12 +76,13 @@ static bool_t __read_mostly full_width_write;
 #define FIXED_CTR_CTRL_ANYTHREAD_MASK 0x4
 
 #define ARCH_CNTR_ENABLED   (1ULL << 22)
+#define ARCH_CNTR_PIN_CONTROL (1ULL << 19)
 
 /* Number of general-purpose and fixed performance counters */
 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))
+#define ARCH_CTRL_MASK  (~((1ull << 32) - 1) | (1ull << 21) | ARCH_CNTR_PIN_CONTROL)
 static uint64_t __read_mostly fixed_ctrl_mask, fixed_counters_mask;
 static uint64_t __read_mostly global_ovf_ctrl_mask, global_ctrl_mask;
 

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