[PATCH 0/2] AMD VPMU fixes

[PATCH 0/2] AMD VPMU fixes

[Boris Ostrovsky]

A couple of AMD VPMU fixes
* Handle original (pre-family 15h) MSR range reserved for PMU use
* Stop reporting error back to the guest when reserved PMU MSR bits are
  modified since apparently guests (Linux at least) may assume those bits
  to be zero. Just make sure those bits are set/cleared prior to being
  written according to values discovered during initialization.

Boris Ostrovsky (2):
  AMD/VPMU: 0xc0010000 - 0xc001007 MSRs are in PMU range
  AMD/VPMU: Keep reserved MSR bits untouched but allow the rest to be
    written


 xen/arch/x86/cpu/vpmu_amd.c | 16 ++++------------
 xen/arch/x86/traps.c        |  2 ++
 2 files changed, 6 insertions(+), 12 deletions(-)

-- 
1.8.3.1


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

[PATCH 1/2] AMD/VPMU: 0xc0010000 - 0xc001007 MSRs are in PMU range

[Boris Ostrovsky]

We need to check for older PMU MSR range when emulating MSR
accesses for PV guests.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/traps.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 767d0b0..79a3516 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2903,6 +2903,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
             {
                 vpmu_msr = 1;
         case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
+        case MSR_K7_EVNTSEL0...MSR_K7_PERFCTR3:
                 if ( vpmu_msr || (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
                 {
                     if ( (vpmu_mode & XENPMU_MODE_ALL) &&
@@ -3030,6 +3031,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
             {
                 vpmu_msr = 1;
         case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
+        case MSR_K7_EVNTSEL0...MSR_K7_PERFCTR3:
                 if ( vpmu_msr || (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
                 {
 
-- 
1.8.3.1


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

[PATCH 2/2] AMD/VPMU: Keep reserved MSR bits untouched but allow the rest to be written

[Boris Ostrovsky]

While AMD APM suggests that reserved MSR bits are not supposed to be
touched, it is not clear how (or whether) HW enforces this for PMU
registers. At least on some family 10h processors writes of these bits
are apparently ignored: guests (such as Linux) assume that the bits
are zero and write the MSRs with that assumption in mind even though
the bits are set by the time OS/hypervisor starts runnning.

With that in mind, let's stop reporting an error to the guest if it
tries to modify those bits and instead adjust the value to be written
safely.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/cpu/vpmu_amd.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu_amd.c b/xen/arch/x86/cpu/vpmu_amd.c
index 55d03b3..6147ec4 100644
--- a/xen/arch/x86/cpu/vpmu_amd.c
+++ b/xen/arch/x86/cpu/vpmu_amd.c
@@ -247,15 +247,7 @@ static int amd_vpmu_load(struct vcpu *v, bool_t from_guest)
 
         for ( i = 0; i < num_counters; i++ )
         {
-            if ( (ctrl_regs[i] & CTRL_RSVD_MASK) != ctrl_rsvd[i] )
-            {
-                /*
-                 * Not necessary to re-init context since we should never load
-                 * it until guest provides valid values. But just to be safe.
-                 */
-                amd_vpmu_init_regs(ctxt);
-                return -EINVAL;
-            }
+            ctrl_regs[i] = (ctrl_regs[i] & ~CTRL_RSVD_MASK) | ctrl_rsvd[i];
 
             if ( is_pmu_enabled(ctrl_regs[i]) )
                 is_running = 1;
@@ -363,9 +355,9 @@ static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
 
     ASSERT(!supported);
 
-    if ( (type == MSR_TYPE_CTRL ) &&
-         ((msr_content & CTRL_RSVD_MASK) != ctrl_rsvd[idx]) )
-        return -EINVAL;
+    /* Make sure reserved bits are not touched */
+    if ( type == MSR_TYPE_CTRL )
+        msr_content = (msr_content & ~CTRL_RSVD_MASK) | ctrl_rsvd[idx];
 
     /* For all counters, enable guest only mode for HVM guest */
     if ( has_hvm_container_vcpu(v) && (type == MSR_TYPE_CTRL) &&
-- 
1.8.3.1


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

Re: [PATCH 1/2] AMD/VPMU: 0xc0010000 - 0xc001007 MSRs are in PMU range

[Jan Beulich]

>>> On 08.08.16 at 15:41, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -2903,6 +2903,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
>              {
>                  vpmu_msr = 1;
>          case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
> +        case MSR_K7_EVNTSEL0...MSR_K7_PERFCTR3:
>                  if ( vpmu_msr || (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>                  {
>                      if ( (vpmu_mode & XENPMU_MODE_ALL) &&
> @@ -3030,6 +3031,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
>              {
>                  vpmu_msr = 1;
>          case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
> +        case MSR_K7_EVNTSEL0...MSR_K7_PERFCTR3:
>                  if ( vpmu_msr || (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>                  {
>  

And all the logic in vpmu_amd.c is already in suitable shape? Namely
I'm wondering whether CTRL_RSVD_MASK indeed applies uniformly
to both the pre-Fam15 and Fam15+ MSRs.

Jan


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

Re: [PATCH 2/2] AMD/VPMU: Keep reserved MSR bits untouched but allow the rest to be written

[Jan Beulich]

>>> On 08.08.16 at 15:41, <boris.ostrovsky@oracle.com> wrote:
> While AMD APM suggests that reserved MSR bits are not supposed to be
> touched, it is not clear how (or whether) HW enforces this for PMU
> registers. At least on some family 10h processors writes of these bits
> are apparently ignored: guests (such as Linux) assume that the bits
> are zero and write the MSRs with that assumption in mind even though
> the bits are set by the time OS/hypervisor starts runnning.

So how did these bits become non-zero then?

Independent of that I think the relaxation would better only be done
for those older CPUs.

Jan


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

Re: [PATCH 1/2] AMD/VPMU: 0xc0010000 - 0xc001007 MSRs are in PMU range

[Boris Ostrovsky]

On 08/08/2016 09:53 AM, Jan Beulich wrote:
>>>> On 08.08.16 at 15:41, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -2903,6 +2903,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
>>              {
>>                  vpmu_msr = 1;
>>          case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
>> +        case MSR_K7_EVNTSEL0...MSR_K7_PERFCTR3:
>>                  if ( vpmu_msr || (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>>                  {
>>                      if ( (vpmu_mode & XENPMU_MODE_ALL) &&
>> @@ -3030,6 +3031,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
>>              {
>>                  vpmu_msr = 1;
>>          case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
>> +        case MSR_K7_EVNTSEL0...MSR_K7_PERFCTR3:
>>                  if ( vpmu_msr || (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>>                  {
>>  
> And all the logic in vpmu_amd.c is already in suitable shape? Namely
> I'm wondering whether CTRL_RSVD_MASK indeed applies uniformly
> to both the pre-Fam15 and Fam15+ MSRs.

The reserved bits look the same on all supported families --- bits
63:42, 39:36, 21 and 19. Except apparently on family 12h bit 19 is MBZ.

-boris

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

Re: [PATCH 1/2] AMD/VPMU: 0xc0010000 - 0xc001007 MSRs are in PMU range

[Jan Beulich]

>>> On 08.08.16 at 16:06, <boris.ostrovsky@oracle.com> wrote:
> On 08/08/2016 09:53 AM, Jan Beulich wrote:
>>>>> On 08.08.16 at 15:41, <boris.ostrovsky@oracle.com> wrote:
>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -2903,6 +2903,7 @@ static int emulate_privileged_op(struct cpu_user_regs 
> *regs)
>>>              {
>>>                  vpmu_msr = 1;
>>>          case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
>>> +        case MSR_K7_EVNTSEL0...MSR_K7_PERFCTR3:
>>>                  if ( vpmu_msr || (boot_cpu_data.x86_vendor == 
> X86_VENDOR_AMD) )
>>>                  {
>>>                      if ( (vpmu_mode & XENPMU_MODE_ALL) &&
>>> @@ -3030,6 +3031,7 @@ static int emulate_privileged_op(struct cpu_user_regs 
> *regs)
>>>              {
>>>                  vpmu_msr = 1;
>>>          case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
>>> +        case MSR_K7_EVNTSEL0...MSR_K7_PERFCTR3:
>>>                  if ( vpmu_msr || (boot_cpu_data.x86_vendor == 
> X86_VENDOR_AMD) )
>>>                  {
>>>  
>> And all the logic in vpmu_amd.c is already in suitable shape? Namely
>> I'm wondering whether CTRL_RSVD_MASK indeed applies uniformly
>> to both the pre-Fam15 and Fam15+ MSRs.
> 
> The reserved bits look the same on all supported families --- bits
> 63:42, 39:36, 21 and 19. Except apparently on family 12h bit 19 is MBZ.

Isn't MBZ == reserved for all practical purposes? In any event the
patch is fine then afaic.

Jan


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

Re: [PATCH 2/2] AMD/VPMU: Keep reserved MSR bits untouched but allow the rest to be written

[Boris Ostrovsky]

On 08/08/2016 09:56 AM, Jan Beulich wrote:
>>>> On 08.08.16 at 15:41, <boris.ostrovsky@oracle.com> wrote:
>> While AMD APM suggests that reserved MSR bits are not supposed to be
>> touched, it is not clear how (or whether) HW enforces this for PMU
>> registers. At least on some family 10h processors writes of these bits
>> are apparently ignored: guests (such as Linux) assume that the bits
>> are zero and write the MSRs with that assumption in mind even though
>> the bits are set by the time OS/hypervisor starts runnning.
> So how did these bits become non-zero then?

Apparently one of the systems in our lab always had somewhat strange
post-BIOS state of these registers, with some bits (I think 19, can't
remember now) set. I never picked this up before since we don't really
test VPMU, we just initialize it and failure to initialize is non-fatal.

>
> Independent of that I think the relaxation would better only be done
> for those older CPUs.

Why? This can easily happen on any family.

-boris

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

Re: [PATCH 1/2] AMD/VPMU: 0xc0010000 - 0xc001007 MSRs are in PMU range

[Boris Ostrovsky]

On 08/08/2016 10:09 AM, Jan Beulich wrote:
>> The reserved bits look the same on all supported families --- bits
>> 63:42, 39:36, 21 and 19. Except apparently on family 12h bit 19 is MBZ.
> Isn't MBZ == reserved for all practical purposes? 

My understanding is that when something is MBZ we know we shouldn't
write 1 there. Reserved can be either 1 or 0 --- that's what my system
may have with bit 19 set. (Although it's also possible that POST or BIOS
are sloppy).

-boris

> In any event the
> patch is fine then afaic.
>
> Jan
>



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

Re: [PATCH 2/2] AMD/VPMU: Keep reserved MSR bits untouched but allow the rest to be written

[Jan Beulich]

>>> On 08.08.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
> On 08/08/2016 09:56 AM, Jan Beulich wrote:
>>>>> On 08.08.16 at 15:41, <boris.ostrovsky@oracle.com> wrote:
>>> While AMD APM suggests that reserved MSR bits are not supposed to be
>>> touched, it is not clear how (or whether) HW enforces this for PMU
>>> registers. At least on some family 10h processors writes of these bits
>>> are apparently ignored: guests (such as Linux) assume that the bits
>>> are zero and write the MSRs with that assumption in mind even though
>>> the bits are set by the time OS/hypervisor starts runnning.
>> So how did these bits become non-zero then?
> 
> Apparently one of the systems in our lab always had somewhat strange
> post-BIOS state of these registers, with some bits (I think 19, can't
> remember now) set. I never picked this up before since we don't really
> test VPMU, we just initialize it and failure to initialize is non-fatal.

Hm, if we find reserved bits set during boot, maybe we could
simply record that and allow those bits to retain their non-zero
value during later writes (along with getting cleared to zero)?

>> Independent of that I think the relaxation would better only be done
>> for those older CPUs.
> 
> Why? This can easily happen on any family.

Can it? Your description reads more like this is an exception, not
the rule. I'd prefer for us to be conservative here. Suravee?

Jan


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

Re: [PATCH 1/2] AMD/VPMU: 0xc0010000 - 0xc001007 MSRs are in PMU range

[Jan Beulich]

>>> On 08.08.16 at 17:05, <boris.ostrovsky@oracle.com> wrote:
> On 08/08/2016 10:09 AM, Jan Beulich wrote:
>>> The reserved bits look the same on all supported families --- bits
>>> 63:42, 39:36, 21 and 19. Except apparently on family 12h bit 19 is MBZ.
>> Isn't MBZ == reserved for all practical purposes? 
> 
> My understanding is that when something is MBZ we know we shouldn't
> write 1 there. Reserved can be either 1 or 0 --- that's what my system
> may have with bit 19 set. (Although it's also possible that POST or BIOS
> are sloppy).

All understood, yet for us it still means: Don't touch those bits.

Jan


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

Re: [PATCH 2/2] AMD/VPMU: Keep reserved MSR bits untouched but allow the rest to be written

[Boris Ostrovsky]

On 08/08/2016 11:10 AM, Jan Beulich wrote:
>>>> On 08.08.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>> On 08/08/2016 09:56 AM, Jan Beulich wrote:
>>>>>> On 08.08.16 at 15:41, <boris.ostrovsky@oracle.com> wrote:
>>>> While AMD APM suggests that reserved MSR bits are not supposed to be
>>>> touched, it is not clear how (or whether) HW enforces this for PMU
>>>> registers. At least on some family 10h processors writes of these bits
>>>> are apparently ignored: guests (such as Linux) assume that the bits
>>>> are zero and write the MSRs with that assumption in mind even though
>>>> the bits are set by the time OS/hypervisor starts runnning.
>>> So how did these bits become non-zero then?
>> Apparently one of the systems in our lab always had somewhat strange
>> post-BIOS state of these registers, with some bits (I think 19, can't
>> remember now) set. I never picked this up before since we don't really
>> test VPMU, we just initialize it and failure to initialize is non-fatal.
> Hm, if we find reserved bits set during boot, maybe we could
> simply record that and allow those bits to retain their non-zero
> value during later writes (along with getting cleared to zero)?

But that's exactly what I am trying to do, no? ctrl_rsvd[] is state of
those bits when the hypervisor first reads the registers.

There is one hole here (and it always was there) in that we assume that
all processors in the system behave the same in this regard. I.e. we
only read ctrl_rsvd on the boot processor.

>
>>> Independent of that I think the relaxation would better only be done
>>> for those older CPUs.
>> Why? This can easily happen on any family.
> Can it? Your description reads more like this is an exception, not
> the rule. I'd prefer for us to be conservative here. Suravee?

So my theory is that even when bits are declared reserved, BIOS/POST
(who presumably know exactly how HW behaves and therefore know that, for
example, HW will ignore those bits) for whatever reason may flip those
bits and not restore them to zero. So it's really not HW property but
firmware not cleaning up. If that's the case, however, we know that it's
safe to set the bits since BIOS just did that.
-boris



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

Re: [PATCH 2/2] AMD/VPMU: Keep reserved MSR bits untouched but allow the rest to be written

[Jan Beulich]

>>> On 08.08.16 at 17:28, <boris.ostrovsky@oracle.com> wrote:
> On 08/08/2016 11:10 AM, Jan Beulich wrote:
>>>>> On 08.08.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>>> On 08/08/2016 09:56 AM, Jan Beulich wrote:
>>>>>>> On 08.08.16 at 15:41, <boris.ostrovsky@oracle.com> wrote:
>>>>> While AMD APM suggests that reserved MSR bits are not supposed to be
>>>>> touched, it is not clear how (or whether) HW enforces this for PMU
>>>>> registers. At least on some family 10h processors writes of these bits
>>>>> are apparently ignored: guests (such as Linux) assume that the bits
>>>>> are zero and write the MSRs with that assumption in mind even though
>>>>> the bits are set by the time OS/hypervisor starts runnning.
>>>> So how did these bits become non-zero then?
>>> Apparently one of the systems in our lab always had somewhat strange
>>> post-BIOS state of these registers, with some bits (I think 19, can't
>>> remember now) set. I never picked this up before since we don't really
>>> test VPMU, we just initialize it and failure to initialize is non-fatal.
>> Hm, if we find reserved bits set during boot, maybe we could
>> simply record that and allow those bits to retain their non-zero
>> value during later writes (along with getting cleared to zero)?
> 
> But that's exactly what I am trying to do, no? ctrl_rsvd[] is state of
> those bits when the hypervisor first reads the registers.

Not really - you always or in the settings you found on boot.

> There is one hole here (and it always was there) in that we assume that
> all processors in the system behave the same in this regard. I.e. we
> only read ctrl_rsvd on the boot processor.

If the processors are of exactly the same stepping that shouldn't
by itself not be a problem (and mixed steppings are frwoned upon
anyway). However ...

>>>> Independent of that I think the relaxation would better only be done
>>>> for those older CPUs.
>>> Why? This can easily happen on any family.
>> Can it? Your description reads more like this is an exception, not
>> the rule. I'd prefer for us to be conservative here. Suravee?
> 
> So my theory is that even when bits are declared reserved, BIOS/POST
> (who presumably know exactly how HW behaves and therefore know that, for
> example, HW will ignore those bits) for whatever reason may flip those
> bits and not restore them to zero. So it's really not HW property but
> firmware not cleaning up. If that's the case, however, we know that it's
> safe to set the bits since BIOS just did that.

... this is not a really correct statement, as it neglects the possibility
that certain bit combinations are invalid.

Jan


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

Re: [PATCH 2/2] AMD/VPMU: Keep reserved MSR bits untouched but allow the rest to be written

[Boris Ostrovsky]

On 08/08/2016 11:54 AM, Jan Beulich wrote:
>>>> On 08.08.16 at 17:28, <boris.ostrovsky@oracle.com> wrote:
>> On 08/08/2016 11:10 AM, Jan Beulich wrote:
>>>>>> On 08.08.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>>>> On 08/08/2016 09:56 AM, Jan Beulich wrote:
>>>>>>>> On 08.08.16 at 15:41, <boris.ostrovsky@oracle.com> wrote:
>>>>>> While AMD APM suggests that reserved MSR bits are not supposed to be
>>>>>> touched, it is not clear how (or whether) HW enforces this for PMU
>>>>>> registers. At least on some family 10h processors writes of these bits
>>>>>> are apparently ignored: guests (such as Linux) assume that the bits
>>>>>> are zero and write the MSRs with that assumption in mind even though
>>>>>> the bits are set by the time OS/hypervisor starts runnning.
>>>>> So how did these bits become non-zero then?
>>>> Apparently one of the systems in our lab always had somewhat strange
>>>> post-BIOS state of these registers, with some bits (I think 19, can't
>>>> remember now) set. I never picked this up before since we don't really
>>>> test VPMU, we just initialize it and failure to initialize is non-fatal.
>>> Hm, if we find reserved bits set during boot, maybe we could
>>> simply record that and allow those bits to retain their non-zero
>>> value during later writes (along with getting cleared to zero)?
>> But that's exactly what I am trying to do, no? ctrl_rsvd[] is state of
>> those bits when the hypervisor first reads the registers.
> Not really - you always or in the settings you found on boot.

But this is done after a register has its reserved bits cleared, so only
reserved bits that were set on boot are set again:

    msr_content = (msr_content & ~CTRL_RSVD_MASK) | ctrl_rsvd[idx];


>
>> There is one hole here (and it always was there) in that we assume that
>> all processors in the system behave the same in this regard. I.e. we
>> only read ctrl_rsvd on the boot processor.
> If the processors are of exactly the same stepping that shouldn't
> by itself not be a problem (and mixed steppings are frwoned upon
> anyway). However ...
>
>>>>> Independent of that I think the relaxation would better only be done
>>>>> for those older CPUs.
>>>> Why? This can easily happen on any family.
>>> Can it? Your description reads more like this is an exception, not
>>> the rule. I'd prefer for us to be conservative here. Suravee?
>> So my theory is that even when bits are declared reserved, BIOS/POST
>> (who presumably know exactly how HW behaves and therefore know that, for
>> example, HW will ignore those bits) for whatever reason may flip those
>> bits and not restore them to zero. So it's really not HW property but
>> firmware not cleaning up. If that's the case, however, we know that it's
>> safe to set the bits since BIOS just did that.
> ... this is not a really correct statement, as it neglects the possibility
> that certain bit combinations are invalid.

Not sure I understand why: we save into ctrl_rsvd the combination (of
reserved bits)
that we know is valid, otherwise we wouldn't have read it.

-boris



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

Re: [PATCH 2/2] AMD/VPMU: Keep reserved MSR bits untouched but allow the rest to be written

[Jan Beulich]

>>> On 08.08.16 at 19:00, <boris.ostrovsky@oracle.com> wrote:
> On 08/08/2016 11:54 AM, Jan Beulich wrote:
>>>>> On 08.08.16 at 17:28, <boris.ostrovsky@oracle.com> wrote:
>>> On 08/08/2016 11:10 AM, Jan Beulich wrote:
>>>>>>> On 08.08.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 08/08/2016 09:56 AM, Jan Beulich wrote:
>>>>>>>>> On 08.08.16 at 15:41, <boris.ostrovsky@oracle.com> wrote:
>>>>>>> While AMD APM suggests that reserved MSR bits are not supposed to be
>>>>>>> touched, it is not clear how (or whether) HW enforces this for PMU
>>>>>>> registers. At least on some family 10h processors writes of these bits
>>>>>>> are apparently ignored: guests (such as Linux) assume that the bits
>>>>>>> are zero and write the MSRs with that assumption in mind even though
>>>>>>> the bits are set by the time OS/hypervisor starts runnning.
>>>>>> So how did these bits become non-zero then?
>>>>> Apparently one of the systems in our lab always had somewhat strange
>>>>> post-BIOS state of these registers, with some bits (I think 19, can't
>>>>> remember now) set. I never picked this up before since we don't really
>>>>> test VPMU, we just initialize it and failure to initialize is non-fatal.
>>>> Hm, if we find reserved bits set during boot, maybe we could
>>>> simply record that and allow those bits to retain their non-zero
>>>> value during later writes (along with getting cleared to zero)?
>>> But that's exactly what I am trying to do, no? ctrl_rsvd[] is state of
>>> those bits when the hypervisor first reads the registers.
>> Not really - you always or in the settings you found on boot.
> 
> But this is done after a register has its reserved bits cleared, so only
> reserved bits that were set on boot are set again:
> 
>     msr_content = (msr_content & ~CTRL_RSVD_MASK) | ctrl_rsvd[idx];

But that's different from what I said we'd probably prefer: "allow
those bits to retain their non-zero value during later writes (along
with getting cleared to zero)".

>>>>>> Independent of that I think the relaxation would better only be done
>>>>>> for those older CPUs.
>>>>> Why? This can easily happen on any family.
>>>> Can it? Your description reads more like this is an exception, not
>>>> the rule. I'd prefer for us to be conservative here. Suravee?
>>> So my theory is that even when bits are declared reserved, BIOS/POST
>>> (who presumably know exactly how HW behaves and therefore know that, for
>>> example, HW will ignore those bits) for whatever reason may flip those
>>> bits and not restore them to zero. So it's really not HW property but
>>> firmware not cleaning up. If that's the case, however, we know that it's
>>> safe to set the bits since BIOS just did that.
>> ... this is not a really correct statement, as it neglects the possibility
>> that certain bit combinations are invalid.
> 
> Not sure I understand why: we save into ctrl_rsvd the combination (of
> reserved bits)
> that we know is valid, otherwise we wouldn't have read it.

But those set bits in the reserved group could conflict with certain
settings of non-reserved bits. Anyway, without input from AMD I
don't think we're going to come to a conclusion here, not the least
because I'm not convinced PRM and BKDG can really be relied upon
wrt the distinction between "reserved" and "mbz".

Jan


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