diff mbox series

x86/svm: ignore accesses to EX_CFG

Message ID 20200916105426.6663-1-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86/svm: ignore accesses to EX_CFG | expand

Commit Message

Roger Pau Monne Sept. 16, 2020, 10:54 a.m. UTC
Windows 10 will try to unconditionally read EX_CFG on AMD hadrware,
and injecting a #GP fault will result in a panic:

svm.c:1964:d5v0 RDMSR 0xc001102c unimplemented
d5v0 VIRIDIAN CRASH: 7e ffffffffc0000096 fffff8054cbe5ffe fffffa0837a066e8 fffffa0837a05f30

Return 0 when trying to read the MSR and drop writes.

Fixes: 84e848fd7a16 ('x86/hvm: disallow access to unknown MSRs')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/svm/svm.c      | 2 ++
 xen/include/asm-x86/msr-index.h | 1 +
 2 files changed, 3 insertions(+)

Comments

Jan Beulich Sept. 16, 2020, 12:55 p.m. UTC | #1
On 16.09.2020 12:54, Roger Pau Monne wrote:
> Windows 10 will try to unconditionally read EX_CFG on AMD hadrware,
> and injecting a #GP fault will result in a panic:
> 
> svm.c:1964:d5v0 RDMSR 0xc001102c unimplemented
> d5v0 VIRIDIAN CRASH: 7e ffffffffc0000096 fffff8054cbe5ffe fffffa0837a066e8 fffffa0837a05f30
> 
> Return 0 when trying to read the MSR and drop writes.

So I've gone through a bunch of BKDGs and PPRs, without finding
this MSR mentioned in any of them. Could you point out on which
model(s) it actually exists? You must have found it somewhere,
or else you wouldn't know a name for it...

> @@ -2108,6 +2109,7 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>      case MSR_K8_TOP_MEM2:
>      case MSR_K8_SYSCFG:
>      case MSR_K8_VM_CR:
> +    case MSR_AMD64_EX_CFG:
>          /* ignore write. handle all bits as read-only. */
>          break;

Is this necessary, rather than having writes fault?

> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -330,6 +330,7 @@
>  #define MSR_AMD64_DC_CFG		0xc0011022
>  #define MSR_AMD64_DE_CFG		0xc0011029
>  #define AMD64_DE_CFG_LFENCE_SERIALISE	(_AC(1, ULL) << 1)
> +#define MSR_AMD64_EX_CFG                0xc001102c

Indentation here wants to match the siblings, i.e. use hard tabs
(for now). Easily addressed while committing, of course.

Jan
Roger Pau Monne Sept. 16, 2020, 1:04 p.m. UTC | #2
On Wed, Sep 16, 2020 at 02:55:52PM +0200, Jan Beulich wrote:
> On 16.09.2020 12:54, Roger Pau Monne wrote:
> > Windows 10 will try to unconditionally read EX_CFG on AMD hadrware,
> > and injecting a #GP fault will result in a panic:
> > 
> > svm.c:1964:d5v0 RDMSR 0xc001102c unimplemented
> > d5v0 VIRIDIAN CRASH: 7e ffffffffc0000096 fffff8054cbe5ffe fffffa0837a066e8 fffffa0837a05f30
> > 
> > Return 0 when trying to read the MSR and drop writes.
> 
> So I've gone through a bunch of BKDGs and PPRs, without finding
> this MSR mentioned in any of them. Could you point out on which
> model(s) it actually exists? You must have found it somewhere,
> or else you wouldn't know a name for it...

Yes, sorry it took me a while to find it also, and I should have added
a reference here. It's in "BIOS and Kernel Developer’s Guide (BKDG)
for AMD Family 15h Models 00h-0Fh Processors", albeit Windows will try
to access it on Family 17h also.

> > @@ -2108,6 +2109,7 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
> >      case MSR_K8_TOP_MEM2:
> >      case MSR_K8_SYSCFG:
> >      case MSR_K8_VM_CR:
> > +    case MSR_AMD64_EX_CFG:
> >          /* ignore write. handle all bits as read-only. */
> >          break;
> 
> Is this necessary, rather than having writes fault?

Hm, I'm not sure about that. This is the same that KVM did to handle
the MSR, see Linux commit 0e1b869fff60c81b510c2d00602d778f8f59dd9a.

I can try to return #GP for writes, but I don't see much issue in just
ignoring writes.

> > --- a/xen/include/asm-x86/msr-index.h
> > +++ b/xen/include/asm-x86/msr-index.h
> > @@ -330,6 +330,7 @@
> >  #define MSR_AMD64_DC_CFG		0xc0011022
> >  #define MSR_AMD64_DE_CFG		0xc0011029
> >  #define AMD64_DE_CFG_LFENCE_SERIALISE	(_AC(1, ULL) << 1)
> > +#define MSR_AMD64_EX_CFG                0xc001102c
> 
> Indentation here wants to match the siblings, i.e. use hard tabs
> (for now). Easily addressed while committing, of course.

Oh, sure, sorry for not realizing.

Thanks, Roger.
Jan Beulich Sept. 16, 2020, 1:28 p.m. UTC | #3
On 16.09.2020 15:04, Roger Pau Monné wrote:
> On Wed, Sep 16, 2020 at 02:55:52PM +0200, Jan Beulich wrote:
>> On 16.09.2020 12:54, Roger Pau Monne wrote:
>>> Windows 10 will try to unconditionally read EX_CFG on AMD hadrware,
>>> and injecting a #GP fault will result in a panic:
>>>
>>> svm.c:1964:d5v0 RDMSR 0xc001102c unimplemented
>>> d5v0 VIRIDIAN CRASH: 7e ffffffffc0000096 fffff8054cbe5ffe fffffa0837a066e8 fffffa0837a05f30
>>>
>>> Return 0 when trying to read the MSR and drop writes.
>>
>> So I've gone through a bunch of BKDGs and PPRs, without finding
>> this MSR mentioned in any of them. Could you point out on which
>> model(s) it actually exists? You must have found it somewhere,
>> or else you wouldn't know a name for it...
> 
> Yes, sorry it took me a while to find it also, and I should have added
> a reference here. It's in "BIOS and Kernel Developer’s Guide (BKDG)
> for AMD Family 15h Models 00h-0Fh Processors", albeit Windows will try
> to access it on Family 17h also.

Ah, and it's exclusively this one as it seems. The models 1xh one
again doesn't have it.

>>> @@ -2108,6 +2109,7 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>>      case MSR_K8_TOP_MEM2:
>>>      case MSR_K8_SYSCFG:
>>>      case MSR_K8_VM_CR:
>>> +    case MSR_AMD64_EX_CFG:
>>>          /* ignore write. handle all bits as read-only. */
>>>          break;
>>
>> Is this necessary, rather than having writes fault?
> 
> Hm, I'm not sure about that. This is the same that KVM did to handle
> the MSR, see Linux commit 0e1b869fff60c81b510c2d00602d778f8f59dd9a.

Looking at the sole bit that's defined there, I agree the main reason
for Win10 to read it would look to be to potentially also write it if
it finds certain bits unset. If so, perhaps we want to consider to
report a value with this/these bit(s) set?

> I can try to return #GP for writes, but I don't see much issue in just
> ignoring writes.

The reason for me asking is that I'd prefer if we didn't grow an
endless list of exceptions for no reason. In fact I wonder whether
some MSRs that we currently ignore writes for couldn't be dropped.

Jan
Roger Pau Monne Sept. 16, 2020, 1:53 p.m. UTC | #4
On Wed, Sep 16, 2020 at 03:28:28PM +0200, Jan Beulich wrote:
> On 16.09.2020 15:04, Roger Pau Monné wrote:
> > On Wed, Sep 16, 2020 at 02:55:52PM +0200, Jan Beulich wrote:
> >> On 16.09.2020 12:54, Roger Pau Monne wrote:
> >>> Windows 10 will try to unconditionally read EX_CFG on AMD hadrware,
> >>> and injecting a #GP fault will result in a panic:
> >>>
> >>> svm.c:1964:d5v0 RDMSR 0xc001102c unimplemented
> >>> d5v0 VIRIDIAN CRASH: 7e ffffffffc0000096 fffff8054cbe5ffe fffffa0837a066e8 fffffa0837a05f30
> >>>
> >>> Return 0 when trying to read the MSR and drop writes.
> >>
> >> So I've gone through a bunch of BKDGs and PPRs, without finding
> >> this MSR mentioned in any of them. Could you point out on which
> >> model(s) it actually exists? You must have found it somewhere,
> >> or else you wouldn't know a name for it...
> > 
> > Yes, sorry it took me a while to find it also, and I should have added
> > a reference here. It's in "BIOS and Kernel Developer’s Guide (BKDG)
> > for AMD Family 15h Models 00h-0Fh Processors", albeit Windows will try
> > to access it on Family 17h also.
> 
> Ah, and it's exclusively this one as it seems. The models 1xh one
> again doesn't have it.
> 
> >>> @@ -2108,6 +2109,7 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
> >>>      case MSR_K8_TOP_MEM2:
> >>>      case MSR_K8_SYSCFG:
> >>>      case MSR_K8_VM_CR:
> >>> +    case MSR_AMD64_EX_CFG:
> >>>          /* ignore write. handle all bits as read-only. */
> >>>          break;
> >>
> >> Is this necessary, rather than having writes fault?
> > 
> > Hm, I'm not sure about that. This is the same that KVM did to handle
> > the MSR, see Linux commit 0e1b869fff60c81b510c2d00602d778f8f59dd9a.
> 
> Looking at the sole bit that's defined there, I agree the main reason
> for Win10 to read it would look to be to potentially also write it if
> it finds certain bits unset. If so, perhaps we want to consider to
> report a value with this/these bit(s) set?

So the manual only reports the meaning of bit 54, yet my EPYC system
reports 0x0168000000000000.

> > I can try to return #GP for writes, but I don't see much issue in just
> > ignoring writes.
> 
> The reason for me asking is that I'd prefer if we didn't grow an
> endless list of exceptions for no reason. In fact I wonder whether
> some MSRs that we currently ignore writes for couldn't be dropped.

Let me see if I can make Windows happy by returning either bit 54 as 0
or 1, but given the value on bare metal I'm worried that Windows has
more insight on this value than just bit 54.

Thanks, Roger.
Roger Pau Monne Sept. 17, 2020, 8:33 a.m. UTC | #5
On Wed, Sep 16, 2020 at 03:53:43PM +0200, Roger Pau Monné wrote:
> On Wed, Sep 16, 2020 at 03:28:28PM +0200, Jan Beulich wrote:
> > On 16.09.2020 15:04, Roger Pau Monné wrote:
> > > On Wed, Sep 16, 2020 at 02:55:52PM +0200, Jan Beulich wrote:
> > >> On 16.09.2020 12:54, Roger Pau Monne wrote:
> > >>> Windows 10 will try to unconditionally read EX_CFG on AMD hadrware,
> > >>> and injecting a #GP fault will result in a panic:
> > >>>
> > >>> svm.c:1964:d5v0 RDMSR 0xc001102c unimplemented
> > >>> d5v0 VIRIDIAN CRASH: 7e ffffffffc0000096 fffff8054cbe5ffe fffffa0837a066e8 fffffa0837a05f30
> > >>>
> > >>> Return 0 when trying to read the MSR and drop writes.
> > >>
> > >> So I've gone through a bunch of BKDGs and PPRs, without finding
> > >> this MSR mentioned in any of them. Could you point out on which
> > >> model(s) it actually exists? You must have found it somewhere,
> > >> or else you wouldn't know a name for it...
> > > 
> > > Yes, sorry it took me a while to find it also, and I should have added
> > > a reference here. It's in "BIOS and Kernel Developer’s Guide (BKDG)
> > > for AMD Family 15h Models 00h-0Fh Processors", albeit Windows will try
> > > to access it on Family 17h also.
> > 
> > Ah, and it's exclusively this one as it seems. The models 1xh one
> > again doesn't have it.
> > 
> > >>> @@ -2108,6 +2109,7 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
> > >>>      case MSR_K8_TOP_MEM2:
> > >>>      case MSR_K8_SYSCFG:
> > >>>      case MSR_K8_VM_CR:
> > >>> +    case MSR_AMD64_EX_CFG:
> > >>>          /* ignore write. handle all bits as read-only. */
> > >>>          break;
> > >>
> > >> Is this necessary, rather than having writes fault?
> > > 
> > > Hm, I'm not sure about that. This is the same that KVM did to handle
> > > the MSR, see Linux commit 0e1b869fff60c81b510c2d00602d778f8f59dd9a.
> > 
> > Looking at the sole bit that's defined there, I agree the main reason
> > for Win10 to read it would look to be to potentially also write it if
> > it finds certain bits unset. If so, perhaps we want to consider to
> > report a value with this/these bit(s) set?
> 
> So the manual only reports the meaning of bit 54, yet my EPYC system
> reports 0x0168000000000000.
> 
> > > I can try to return #GP for writes, but I don't see much issue in just
> > > ignoring writes.
> > 
> > The reason for me asking is that I'd prefer if we didn't grow an
> > endless list of exceptions for no reason. In fact I wonder whether
> > some MSRs that we currently ignore writes for couldn't be dropped.
> 
> Let me see if I can make Windows happy by returning either bit 54 as 0
> or 1, but given the value on bare metal I'm worried that Windows has
> more insight on this value than just bit 54.

So I've tried reporting bit 54 set on rdmsr, and #GP on writes, and
Windows 10 will try to write to the MSR to set additional bits:

(XEN) svm.c:2155:d1v0 WRMSR 0xc001102c val 0x0048000000000000 unimplemented

So without knowing why Windows is trying to set some hidden bits there
I think the best option ATM is to return 0 from reads and ignore
writes (like the current patch).

Trying to report a value that would make Windows happy seems fragile,
as information about the meaning of bits on the MSR isn't public, so
it's likely to get out of sync.

Thanks, Roger.
Jan Beulich Sept. 17, 2020, 8:37 a.m. UTC | #6
On 17.09.2020 10:33, Roger Pau Monné wrote:
> On Wed, Sep 16, 2020 at 03:53:43PM +0200, Roger Pau Monné wrote:
>> On Wed, Sep 16, 2020 at 03:28:28PM +0200, Jan Beulich wrote:
>>> On 16.09.2020 15:04, Roger Pau Monné wrote:
>>>> On Wed, Sep 16, 2020 at 02:55:52PM +0200, Jan Beulich wrote:
>>>>> On 16.09.2020 12:54, Roger Pau Monne wrote:
>>>>>> Windows 10 will try to unconditionally read EX_CFG on AMD hadrware,
>>>>>> and injecting a #GP fault will result in a panic:
>>>>>>
>>>>>> svm.c:1964:d5v0 RDMSR 0xc001102c unimplemented
>>>>>> d5v0 VIRIDIAN CRASH: 7e ffffffffc0000096 fffff8054cbe5ffe fffffa0837a066e8 fffffa0837a05f30
>>>>>>
>>>>>> Return 0 when trying to read the MSR and drop writes.
>>>>>
>>>>> So I've gone through a bunch of BKDGs and PPRs, without finding
>>>>> this MSR mentioned in any of them. Could you point out on which
>>>>> model(s) it actually exists? You must have found it somewhere,
>>>>> or else you wouldn't know a name for it...
>>>>
>>>> Yes, sorry it took me a while to find it also, and I should have added
>>>> a reference here. It's in "BIOS and Kernel Developer’s Guide (BKDG)
>>>> for AMD Family 15h Models 00h-0Fh Processors", albeit Windows will try
>>>> to access it on Family 17h also.
>>>
>>> Ah, and it's exclusively this one as it seems. The models 1xh one
>>> again doesn't have it.
>>>
>>>>>> @@ -2108,6 +2109,7 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>>>>>      case MSR_K8_TOP_MEM2:
>>>>>>      case MSR_K8_SYSCFG:
>>>>>>      case MSR_K8_VM_CR:
>>>>>> +    case MSR_AMD64_EX_CFG:
>>>>>>          /* ignore write. handle all bits as read-only. */
>>>>>>          break;
>>>>>
>>>>> Is this necessary, rather than having writes fault?
>>>>
>>>> Hm, I'm not sure about that. This is the same that KVM did to handle
>>>> the MSR, see Linux commit 0e1b869fff60c81b510c2d00602d778f8f59dd9a.
>>>
>>> Looking at the sole bit that's defined there, I agree the main reason
>>> for Win10 to read it would look to be to potentially also write it if
>>> it finds certain bits unset. If so, perhaps we want to consider to
>>> report a value with this/these bit(s) set?
>>
>> So the manual only reports the meaning of bit 54, yet my EPYC system
>> reports 0x0168000000000000.
>>
>>>> I can try to return #GP for writes, but I don't see much issue in just
>>>> ignoring writes.
>>>
>>> The reason for me asking is that I'd prefer if we didn't grow an
>>> endless list of exceptions for no reason. In fact I wonder whether
>>> some MSRs that we currently ignore writes for couldn't be dropped.
>>
>> Let me see if I can make Windows happy by returning either bit 54 as 0
>> or 1, but given the value on bare metal I'm worried that Windows has
>> more insight on this value than just bit 54.
> 
> So I've tried reporting bit 54 set on rdmsr, and #GP on writes, and
> Windows 10 will try to write to the MSR to set additional bits:
> 
> (XEN) svm.c:2155:d1v0 WRMSR 0xc001102c val 0x0048000000000000 unimplemented
> 
> So without knowing why Windows is trying to set some hidden bits there
> I think the best option ATM is to return 0 from reads and ignore
> writes (like the current patch).
> 
> Trying to report a value that would make Windows happy seems fragile,
> as information about the meaning of bits on the MSR isn't public, so
> it's likely to get out of sync.

Agreed:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

We will want to figure out why Windows is doing what they do, and if
we perhaps ought to do the same for the benefit of all guests.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 136445972e..5037c0fe7d 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1942,6 +1942,7 @@  static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     case MSR_K8_TOP_MEM1:
     case MSR_K8_TOP_MEM2:
     case MSR_K8_VM_CR:
+    case MSR_AMD64_EX_CFG:
         *msr_content = 0;
         break;
 
@@ -2108,6 +2109,7 @@  static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
     case MSR_K8_TOP_MEM2:
     case MSR_K8_SYSCFG:
     case MSR_K8_VM_CR:
+    case MSR_AMD64_EX_CFG:
         /* ignore write. handle all bits as read-only. */
         break;
 
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 4fd54fb5c9..c433eeba92 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -330,6 +330,7 @@ 
 #define MSR_AMD64_DC_CFG		0xc0011022
 #define MSR_AMD64_DE_CFG		0xc0011029
 #define AMD64_DE_CFG_LFENCE_SERIALISE	(_AC(1, ULL) << 1)
+#define MSR_AMD64_EX_CFG                0xc001102c
 
 #define MSR_AMD64_DR0_ADDRESS_MASK	0xc0011027
 #define MSR_AMD64_DR1_ADDRESS_MASK	0xc0011019