diff mbox series

[v3,4/8] x86/svm: handle BU_CFG and BU_CFG2 with cases

Message ID 20200901105445.22277-5-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series x86: switch default MSR behavior | expand

Commit Message

Roger Pau Monné Sept. 1, 2020, 10:54 a.m. UTC
Move the special handling of reads to it's own switch case, and also
add support for BU_CFG2. On the write side ignore writes if the MSR is
readable, otherwise return a #GP.

This is in preparation for changing the default MSR read/write
behavior, which will instead return #GP on not explicitly handled
cases.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Move the handling of reads to it's own case.
 - Drop writes if the MSR is readable, else return a #GP.

Changes since v1:
 - New in this version.
---
 xen/arch/x86/hvm/svm/svm.c | 43 ++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 13 deletions(-)

Comments

Andrew Cooper Sept. 2, 2020, 9:02 p.m. UTC | #1
On 01/09/2020 11:54, Roger Pau Monne wrote:
> @@ -1942,19 +1966,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>      default:
>          if ( rdmsr_safe(msr, *msr_content) == 0 )
>              break;
> -
> -        if ( boot_cpu_data.x86 == 0xf && msr == MSR_F10_BU_CFG )
> -        {
> -            /* Win2k8 x64 reads this MSR on revF chips, where it
> -             * wasn't publically available; it uses a magic constant
> -             * in %rdi as a password, which we don't have in
> -             * rdmsr_safe().  Since we'll ignore the later writes,
> -             * just use a plausible value here (the reset value from
> -             * rev10h chips) if the real CPU didn't provide one. */
> -            *msr_content = 0x0000000010200020ull;
> -            break;
> -        }
> -
>          goto gpf;
>      }
>  
> @@ -2110,6 +2121,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>          nsvm->ns_msr_hsavepa = msr_content;
>          break;
>  
> +    case MSR_F10_BU_CFG:
> +    case MSR_F10_BU_CFG2:
> +        if ( rdmsr_safe(msr, msr_content) )
> +            goto gpf;

The comment you've moved depends on this not having this behaviour, as
you'll now hand #GP back to Win2k8 on its write.

Honestly, given that how obsolete both Win2k8 and K10's are, I'm
seriously tempted to suggest dropping this workaround entirely.

~Andrew

> +        break;
> +
>      case MSR_AMD64_TSC_RATIO:
>          if ( msr_content & TSC_RATIO_RSVD_BITS )
>              goto gpf;
Roger Pau Monné Sept. 3, 2020, 8:15 a.m. UTC | #2
On Wed, Sep 02, 2020 at 10:02:33PM +0100, Andrew Cooper wrote:
> On 01/09/2020 11:54, Roger Pau Monne wrote:
> > @@ -1942,19 +1966,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
> >      default:
> >          if ( rdmsr_safe(msr, *msr_content) == 0 )
> >              break;
> > -
> > -        if ( boot_cpu_data.x86 == 0xf && msr == MSR_F10_BU_CFG )
> > -        {
> > -            /* Win2k8 x64 reads this MSR on revF chips, where it
> > -             * wasn't publically available; it uses a magic constant
> > -             * in %rdi as a password, which we don't have in
> > -             * rdmsr_safe().  Since we'll ignore the later writes,
> > -             * just use a plausible value here (the reset value from
> > -             * rev10h chips) if the real CPU didn't provide one. */
> > -            *msr_content = 0x0000000010200020ull;
> > -            break;
> > -        }
> > -
> >          goto gpf;
> >      }
> >  
> > @@ -2110,6 +2121,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
> >          nsvm->ns_msr_hsavepa = msr_content;
> >          break;
> >  
> > +    case MSR_F10_BU_CFG:
> > +    case MSR_F10_BU_CFG2:
> > +        if ( rdmsr_safe(msr, msr_content) )
> > +            goto gpf;
> 
> The comment you've moved depends on this not having this behaviour, as
> you'll now hand #GP back to Win2k8 on its write.

Oh, unless I'm very confused the comment was already wrong, since
svm_msr_write_intercept previous to this patch would return a #GP when
trying to write to BU_CFG if the underlying MSR is not readable, so
this just keeps the previous behavior.

Looking at the original commit (338db98dd8d) it seems the intention
was to only handle reads and let writes return a #GP?

Maybe Win2k8 would only read the MSR?

> Honestly, given that how obsolete both Win2k8 and K10's are, I'm
> seriously tempted to suggest dropping this workaround entirely.

I'm fine to just drop the workaround, but it seems quite trivial to
just keep it as is (reads returns a know value, writes #GP). I can
adjust the comment to be clearer.

Thanks, Roger.
Jan Beulich Sept. 3, 2020, 8:29 a.m. UTC | #3
On 02.09.2020 23:02, Andrew Cooper wrote:
> On 01/09/2020 11:54, Roger Pau Monne wrote:
>> @@ -1942,19 +1966,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>>      default:
>>          if ( rdmsr_safe(msr, *msr_content) == 0 )
>>              break;
>> -
>> -        if ( boot_cpu_data.x86 == 0xf && msr == MSR_F10_BU_CFG )
>> -        {
>> -            /* Win2k8 x64 reads this MSR on revF chips, where it
>> -             * wasn't publically available; it uses a magic constant
>> -             * in %rdi as a password, which we don't have in
>> -             * rdmsr_safe().  Since we'll ignore the later writes,
>> -             * just use a plausible value here (the reset value from
>> -             * rev10h chips) if the real CPU didn't provide one. */
>> -            *msr_content = 0x0000000010200020ull;
>> -            break;
>> -        }
>> -
>>          goto gpf;
>>      }
>>  
>> @@ -2110,6 +2121,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>          nsvm->ns_msr_hsavepa = msr_content;
>>          break;
>>  
>> +    case MSR_F10_BU_CFG:
>> +    case MSR_F10_BU_CFG2:
>> +        if ( rdmsr_safe(msr, msr_content) )
>> +            goto gpf;
> 
> The comment you've moved depends on this not having this behaviour, as
> you'll now hand #GP back to Win2k8 on its write.
> 
> Honestly, given that how obsolete both Win2k8 and K10's are, I'm
> seriously tempted to suggest dropping this workaround entirely.

Let's not (yet). I'm told we (as a company) still support such guests.

Jan
Jan Beulich Sept. 4, 2020, 8:39 a.m. UTC | #4
On 03.09.2020 10:15, Roger Pau Monné wrote:
> On Wed, Sep 02, 2020 at 10:02:33PM +0100, Andrew Cooper wrote:
>> On 01/09/2020 11:54, Roger Pau Monne wrote:
>>> @@ -1942,19 +1966,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>>>      default:
>>>          if ( rdmsr_safe(msr, *msr_content) == 0 )
>>>              break;
>>> -
>>> -        if ( boot_cpu_data.x86 == 0xf && msr == MSR_F10_BU_CFG )
>>> -        {
>>> -            /* Win2k8 x64 reads this MSR on revF chips, where it
>>> -             * wasn't publically available; it uses a magic constant
>>> -             * in %rdi as a password, which we don't have in
>>> -             * rdmsr_safe().  Since we'll ignore the later writes,
>>> -             * just use a plausible value here (the reset value from
>>> -             * rev10h chips) if the real CPU didn't provide one. */
>>> -            *msr_content = 0x0000000010200020ull;
>>> -            break;
>>> -        }
>>> -
>>>          goto gpf;
>>>      }
>>>  
>>> @@ -2110,6 +2121,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>>          nsvm->ns_msr_hsavepa = msr_content;
>>>          break;
>>>  
>>> +    case MSR_F10_BU_CFG:
>>> +    case MSR_F10_BU_CFG2:
>>> +        if ( rdmsr_safe(msr, msr_content) )
>>> +            goto gpf;
>>
>> The comment you've moved depends on this not having this behaviour, as
>> you'll now hand #GP back to Win2k8 on its write.
> 
> Oh, unless I'm very confused the comment was already wrong, since
> svm_msr_write_intercept previous to this patch would return a #GP when
> trying to write to BU_CFG if the underlying MSR is not readable, so
> this just keeps the previous behavior.
> 
> Looking at the original commit (338db98dd8d) it seems the intention
> was to only handle reads and let writes return a #GP?

I agree. And hence while moving it I think you want to also adjust
that comment.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index af584ff5d1..0e43154c7e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1864,6 +1864,30 @@  static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         *msr_content = 1ULL << 61; /* MC4_MISC.Locked */
         break;
 
+    case MSR_F10_BU_CFG:
+        if ( !rdmsr_safe(msr, *msr_content) )
+            break;
+
+        if ( boot_cpu_data.x86 == 0xf )
+        {
+            /*
+             * Win2k8 x64 reads this MSR on revF chips, where it wasn't
+             * publically available; it uses a magic constant in %rdi as a
+             * password, which we don't have in rdmsr_safe().  Since we'll
+             * ignore the later writes, just use a plausible value here (the
+             * reset value from rev10h chips) if the real CPU didn't provide
+             * one.
+             */
+            *msr_content = 0x0000000010200020ull;
+            break;
+        }
+        goto gpf;
+
+    case MSR_F10_BU_CFG2:
+        if ( rdmsr_safe(msr, *msr_content) )
+            goto gpf;
+        break;
+
     case MSR_IA32_EBC_FREQUENCY_ID:
         /*
          * This Intel-only register may be accessed if this HVM guest
@@ -1942,19 +1966,6 @@  static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     default:
         if ( rdmsr_safe(msr, *msr_content) == 0 )
             break;
-
-        if ( boot_cpu_data.x86 == 0xf && msr == MSR_F10_BU_CFG )
-        {
-            /* Win2k8 x64 reads this MSR on revF chips, where it
-             * wasn't publically available; it uses a magic constant
-             * in %rdi as a password, which we don't have in
-             * rdmsr_safe().  Since we'll ignore the later writes,
-             * just use a plausible value here (the reset value from
-             * rev10h chips) if the real CPU didn't provide one. */
-            *msr_content = 0x0000000010200020ull;
-            break;
-        }
-
         goto gpf;
     }
 
@@ -2110,6 +2121,12 @@  static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         nsvm->ns_msr_hsavepa = msr_content;
         break;
 
+    case MSR_F10_BU_CFG:
+    case MSR_F10_BU_CFG2:
+        if ( rdmsr_safe(msr, msr_content) )
+            goto gpf;
+        break;
+
     case MSR_AMD64_TSC_RATIO:
         if ( msr_content & TSC_RATIO_RSVD_BITS )
             goto gpf;