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

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

[Mohit Gambhir]

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@oracle.com>
---
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;
 
-- 
2.9.3


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

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

[Andrew Cooper]

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@oracle.com>

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

~Andrew

> ---
> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Mohit Gambhir]

[-- Attachment #1.1: Type: text/plain, Size: 3859 bytes --]



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@oracle.com>
> 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@redhat.com>
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@redhat.com>
     Signed-off-by: Avi Kivity<avi@redhat.com>

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;
>>   


[-- Attachment #1.2: Type: text/html, Size: 4937 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

>>> On 26.04.17 at 20:50, <mohit.gambhir@oracle.com> wrote:
> 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@oracle.com>
>> Out of interest, which hardware has this been observed on?
> 
> I have tested this on two Intel Broadwell machines.

Since by now all we have are indications that this is an erratum,
this information belongs into the commit message. As it is written
now, it means the bit can't be set on any hardware. If there are
reasons beyond this erratum to uniformly disallow the bit to be
set by guests, these should be named here too. After all the
way you do the change, you now refuse values with the bit set
everywhere.

Jan


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

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

[Boris Ostrovsky]

On 04/27/2017 03:32 AM, Jan Beulich wrote:
>>>> On 26.04.17 at 20:50, <mohit.gambhir@oracle.com> wrote:
>> 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@oracle.com>
>>> Out of interest, which hardware has this been observed on?
>> I have tested this on two Intel Broadwell machines.
> Since by now all we have are indications that this is an erratum,
> this information belongs into the commit message. As it is written
> now, it means the bit can't be set on any hardware. If there are
> reasons beyond this erratum to uniformly disallow the bit to be
> set by guests, these should be named here too. After all the
> way you do the change, you now refuse values with the bit set
> everywhere.

I don't think this is specific to Broadwell. I tried this on a Haswell
(model 60) and got a #GPF as well.

If I understand what this bit does, it is pretty pointless in a guest.
It is only useful in some sort of embedded setup, where something is
hooked up to a particular pin on the board. So perhaps this is not an
erratum but rather a not fully documented feature, where if nothing is
connected to that pin this bit should not be set.

Or maybe it is documented but I can't find anything on that. Either way,
we should mask this bit.

boris

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

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

[Jan Beulich]

>>> On 27.04.17 at 16:57, <boris.ostrovsky@oracle.com> wrote:
> On 04/27/2017 03:32 AM, Jan Beulich wrote:
>>>>> On 26.04.17 at 20:50, <mohit.gambhir@oracle.com> wrote:
>>> 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@oracle.com>
>>>> Out of interest, which hardware has this been observed on?
>>> I have tested this on two Intel Broadwell machines.
>> Since by now all we have are indications that this is an erratum,
>> this information belongs into the commit message. As it is written
>> now, it means the bit can't be set on any hardware. If there are
>> reasons beyond this erratum to uniformly disallow the bit to be
>> set by guests, these should be named here too. After all the
>> way you do the change, you now refuse values with the bit set
>> everywhere.
> 
> I don't think this is specific to Broadwell. I tried this on a Haswell
> (model 60) and got a #GPF as well.
> 
> If I understand what this bit does, it is pretty pointless in a guest.
> It is only useful in some sort of embedded setup, where something is
> hooked up to a particular pin on the board. So perhaps this is not an
> erratum but rather a not fully documented feature, where if nothing is
> connected to that pin this bit should not be set.
> 
> Or maybe it is documented but I can't find anything on that.

Kevin, Jun?

> Either way, we should mask this bit.

Sure, but: Refuse attempts to set it, or silently ignore such?

Jan


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

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

[Boris Ostrovsky]

On 04/27/2017 11:05 AM, Jan Beulich wrote:
>>>> On 27.04.17 at 16:57, <boris.ostrovsky@oracle.com> wrote:
>> On 04/27/2017 03:32 AM, Jan Beulich wrote:
>>>>>> On 26.04.17 at 20:50, <mohit.gambhir@oracle.com> wrote:
>>>> 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@oracle.com>
>>>>> Out of interest, which hardware has this been observed on?
>>>> I have tested this on two Intel Broadwell machines.
>>> Since by now all we have are indications that this is an erratum,
>>> this information belongs into the commit message. As it is written
>>> now, it means the bit can't be set on any hardware. If there are
>>> reasons beyond this erratum to uniformly disallow the bit to be
>>> set by guests, these should be named here too. After all the
>>> way you do the change, you now refuse values with the bit set
>>> everywhere.
>> I don't think this is specific to Broadwell. I tried this on a Haswell
>> (model 60) and got a #GPF as well.
>>
>> If I understand what this bit does, it is pretty pointless in a guest.
>> It is only useful in some sort of embedded setup, where something is
>> hooked up to a particular pin on the board. So perhaps this is not an
>> erratum but rather a not fully documented feature, where if nothing is
>> connected to that pin this bit should not be set.
>>
>> Or maybe it is documented but I can't find anything on that.
> Kevin, Jun?
>
>> Either way, we should mask this bit.
> Sure, but: Refuse attempts to set it, or silently ignore such?

I think the former, especially if what I surmised above is correct ---
the user shouldn't try to set it.

-boris

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

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

[Tian, Kevin]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Thursday, April 27, 2017 11:18 PM
> 
> On 04/27/2017 11:05 AM, Jan Beulich wrote:
> >>>> On 27.04.17 at 16:57, <boris.ostrovsky@oracle.com> wrote:
> >> On 04/27/2017 03:32 AM, Jan Beulich wrote:
> >>>>>> On 26.04.17 at 20:50, <mohit.gambhir@oracle.com> wrote:
> >>>> 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@oracle.com>
> >>>>> Out of interest, which hardware has this been observed on?
> >>>> I have tested this on two Intel Broadwell machines.
> >>> Since by now all we have are indications that this is an erratum,
> >>> this information belongs into the commit message. As it is written
> >>> now, it means the bit can't be set on any hardware. If there are
> >>> reasons beyond this erratum to uniformly disallow the bit to be
> >>> set by guests, these should be named here too. After all the
> >>> way you do the change, you now refuse values with the bit set
> >>> everywhere.
> >> I don't think this is specific to Broadwell. I tried this on a Haswell
> >> (model 60) and got a #GPF as well.
> >>
> >> If I understand what this bit does, it is pretty pointless in a guest.
> >> It is only useful in some sort of embedded setup, where something is
> >> hooked up to a particular pin on the board. So perhaps this is not an
> >> erratum but rather a not fully documented feature, where if nothing is
> >> connected to that pin this bit should not be set.
> >>
> >> Or maybe it is documented but I can't find anything on that.
> > Kevin, Jun?

I asked internally but didn't get a clarification yet.

> >
> >> Either way, we should mask this bit.
> > Sure, but: Refuse attempts to set it, or silently ignore such?
> 
> I think the former, especially if what I surmised above is correct ---
> the user shouldn't try to set it.
> 

I'm with the former too.

Thanks
Kevin

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

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

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Friday, April 28, 2017 2:43 PM
> 
> > From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> > Sent: Thursday, April 27, 2017 11:18 PM
> >
> > On 04/27/2017 11:05 AM, Jan Beulich wrote:
> > >>>> On 27.04.17 at 16:57, <boris.ostrovsky@oracle.com> wrote:
> > >> On 04/27/2017 03:32 AM, Jan Beulich wrote:
> > >>>>>> On 26.04.17 at 20:50, <mohit.gambhir@oracle.com> wrote:
> > >>>> 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@oracle.com>
> > >>>>> Out of interest, which hardware has this been observed on?
> > >>>> I have tested this on two Intel Broadwell machines.
> > >>> Since by now all we have are indications that this is an erratum,
> > >>> this information belongs into the commit message. As it is written
> > >>> now, it means the bit can't be set on any hardware. If there are
> > >>> reasons beyond this erratum to uniformly disallow the bit to be
> > >>> set by guests, these should be named here too. After all the
> > >>> way you do the change, you now refuse values with the bit set
> > >>> everywhere.
> > >> I don't think this is specific to Broadwell. I tried this on a Haswell
> > >> (model 60) and got a #GPF as well.
> > >>
> > >> If I understand what this bit does, it is pretty pointless in a guest.
> > >> It is only useful in some sort of embedded setup, where something is
> > >> hooked up to a particular pin on the board. So perhaps this is not an
> > >> erratum but rather a not fully documented feature, where if nothing is
> > >> connected to that pin this bit should not be set.
> > >>
> > >> Or maybe it is documented but I can't find anything on that.
> > > Kevin, Jun?
> 
> I asked internally but didn't get a clarification yet.
> 
> > >
> > >> Either way, we should mask this bit.
> > > Sure, but: Refuse attempts to set it, or silently ignore such?
> >
> > I think the former, especially if what I surmised above is correct ---
> > the user shouldn't try to set it.
> >
> 
> I'm with the former too.
> 

btw regardless of clarification which I'm trying to get, I think we do
need disallow such guest operation going to physical MSR. It's not
good to have guest impact physical PMU interrupt behavior. Even
when we want to support guest PC operation in the future, it needs
be emulated as the comment given in KVM side. If others also 
agree with this assumption, we could check-in this patch w/o
mentioning any possible erratum...

Thanks
Kevin

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

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

[Jan Beulich]

>>> On 28.04.17 at 08:52, <kevin.tian@intel.com> wrote:
> btw regardless of clarification which I'm trying to get, I think we do
> need disallow such guest operation going to physical MSR. It's not
> good to have guest impact physical PMU interrupt behavior. Even
> when we want to support guest PC operation in the future, it needs
> be emulated as the comment given in KVM side. If others also 
> agree with this assumption, we could check-in this patch w/o
> mentioning any possible erratum...

Good point - it would nevertheless require an adjustment to the
patch description, though, along the lines of the above.

Jan


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

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

[Mohit Gambhir]

On 04/28/2017 02:52 AM, Tian, Kevin wrote:
>> From: Tian, Kevin
>> Sent: Friday, April 28, 2017 2:43 PM
>>
>>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>>> Sent: Thursday, April 27, 2017 11:18 PM
>>>
>>> On 04/27/2017 11:05 AM, Jan Beulich wrote:
>>>>>>> On 27.04.17 at 16:57, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 04/27/2017 03:32 AM, Jan Beulich wrote:
>>>>>>>>> On 26.04.17 at 20:50, <mohit.gambhir@oracle.com> wrote:
>>>>>>> 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@oracle.com>
>>>>>>>> Out of interest, which hardware has this been observed on?
>>>>>>> I have tested this on two Intel Broadwell machines.
>>>>>> Since by now all we have are indications that this is an erratum,
>>>>>> this information belongs into the commit message. As it is written
>>>>>> now, it means the bit can't be set on any hardware. If there are
>>>>>> reasons beyond this erratum to uniformly disallow the bit to be
>>>>>> set by guests, these should be named here too. After all the
>>>>>> way you do the change, you now refuse values with the bit set
>>>>>> everywhere.
>>>>> I don't think this is specific to Broadwell. I tried this on a Haswell
>>>>> (model 60) and got a #GPF as well.
>>>>>
>>>>> If I understand what this bit does, it is pretty pointless in a guest.
>>>>> It is only useful in some sort of embedded setup, where something is
>>>>> hooked up to a particular pin on the board. So perhaps this is not an
>>>>> erratum but rather a not fully documented feature, where if nothing is
>>>>> connected to that pin this bit should not be set.
>>>>>
>>>>> Or maybe it is documented but I can't find anything on that.
>>>> Kevin, Jun?
>> I asked internally but didn't get a clarification yet.
>>
>>>>> Either way, we should mask this bit.
>>>> Sure, but: Refuse attempts to set it, or silently ignore such?
>>> I think the former, especially if what I surmised above is correct ---
>>> the user shouldn't try to set it.
>>>
>> I'm with the former too.
>>
> btw regardless of clarification which I'm trying to get, I think we do
> need disallow such guest operation going to physical MSR. It's not
> good to have guest impact physical PMU interrupt behavior. Even
> when we want to support guest PC operation in the future, it needs
> be emulated as the comment given in KVM side. If others also
> agree with this assumption, we could check-in this patch w/o
> mentioning any possible erratum...
Do we have a consensus on this? Should we push through with this patch 
or any other
changes/clarifications are required?

+ my personal email id.
> Thanks
> Kevin
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel


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

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

[Jan Beulich]

>>> On 03.05.17 at 19:11, <mohit.gambhir@oracle.com> wrote:

> 
> On 04/28/2017 02:52 AM, Tian, Kevin wrote:
>>> From: Tian, Kevin
>>> Sent: Friday, April 28, 2017 2:43 PM
>>>
>>>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>>>> Sent: Thursday, April 27, 2017 11:18 PM
>>>>
>>>> On 04/27/2017 11:05 AM, Jan Beulich wrote:
>>>>>>>> On 27.04.17 at 16:57, <boris.ostrovsky@oracle.com> wrote:
>>>>>> On 04/27/2017 03:32 AM, Jan Beulich wrote:
>>>>>>>>>> On 26.04.17 at 20:50, <mohit.gambhir@oracle.com> wrote:
>>>>>>>> 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@oracle.com>
>>>>>>>>> Out of interest, which hardware has this been observed on?
>>>>>>>> I have tested this on two Intel Broadwell machines.
>>>>>>> Since by now all we have are indications that this is an erratum,
>>>>>>> this information belongs into the commit message. As it is written
>>>>>>> now, it means the bit can't be set on any hardware. If there are
>>>>>>> reasons beyond this erratum to uniformly disallow the bit to be
>>>>>>> set by guests, these should be named here too. After all the
>>>>>>> way you do the change, you now refuse values with the bit set
>>>>>>> everywhere.
>>>>>> I don't think this is specific to Broadwell. I tried this on a Haswell
>>>>>> (model 60) and got a #GPF as well.
>>>>>>
>>>>>> If I understand what this bit does, it is pretty pointless in a guest.
>>>>>> It is only useful in some sort of embedded setup, where something is
>>>>>> hooked up to a particular pin on the board. So perhaps this is not an
>>>>>> erratum but rather a not fully documented feature, where if nothing is
>>>>>> connected to that pin this bit should not be set.
>>>>>>
>>>>>> Or maybe it is documented but I can't find anything on that.
>>>>> Kevin, Jun?
>>> I asked internally but didn't get a clarification yet.
>>>
>>>>>> Either way, we should mask this bit.
>>>>> Sure, but: Refuse attempts to set it, or silently ignore such?
>>>> I think the former, especially if what I surmised above is correct ---
>>>> the user shouldn't try to set it.
>>>>
>>> I'm with the former too.
>>>
>> btw regardless of clarification which I'm trying to get, I think we do
>> need disallow such guest operation going to physical MSR. It's not
>> good to have guest impact physical PMU interrupt behavior. Even
>> when we want to support guest PC operation in the future, it needs
>> be emulated as the comment given in KVM side. If others also
>> agree with this assumption, we could check-in this patch w/o
>> mentioning any possible erratum...
> Do we have a consensus on this? Should we push through with this patch 
> or any other
> changes/clarifications are required?

Iirc the main thing left at this point is to make the commit message
properly express why the change is being made.

Jan


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