x86/cpuid: Fix handling of the CPUID.7[0].eax levelling MSR
diff mbox series

Message ID 20190909155124.19942-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/cpuid: Fix handling of the CPUID.7[0].eax levelling MSR
Related show

Commit Message

Andrew Cooper Sept. 9, 2019, 3:51 p.m. UTC
7a0 is an integer field, not a mask - taking the logical and of the hardware
and policy values results in nonsense.  Instead, take the policy value
directly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

Even Rome hardware has 7[0].eax still as 0, and there is no sensible reason to
set max_subleaf higher at this point, so this is only a latent bug for now.
---
 xen/arch/x86/domctl.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Roger Pau Monné Sept. 10, 2019, 9:29 a.m. UTC | #1
On Mon, Sep 09, 2019 at 04:51:24PM +0100, Andrew Cooper wrote:
> 7a0 is an integer field, not a mask - taking the logical and of the hardware
> and policy values results in nonsense.  Instead, take the policy value
> directly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> Even Rome hardware has 7[0].eax still as 0, and there is no sensible reason to
> set max_subleaf higher at this point, so this is only a latent bug for now.
> ---
>  xen/arch/x86/domctl.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 1e98fc8009..35ad8cb51c 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -218,11 +218,16 @@ static int update_domain_cpuid_info(struct domain *d,
>          if ( is_pv_domain(d) && ((levelling_caps & LCAP_7ab0) == LCAP_7ab0) )
>          {
>              uint64_t mask = cpuidmask_defaults._7ab0;
> -            uint32_t eax = ctl->eax;
> -            uint32_t ebx = p->feat._7b0;
>  
> -            if ( boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
> -                mask &= ((uint64_t)eax << 32) | ebx;
> +            /*
> +             * Leaf 7[0].eax is max_subleaf, not a feature mask.  Take it
> +             * wholesale from the policy, but clamp the features in 7[0].ebx
> +             * per usual.
> +             */
> +            if ( boot_cpu_data.x86_vendor &
> +                 (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
> +                mask = (((uint64_t)p->feat.max_subleaf << 32) |
> +                        ((uint32_t)mask | p->feat._7b0));

Why do you set the high bits of the mask (63:30) with the max subleaf?
According to the document I have bits 63:30 are reserved, and that
seems to match the expected CPUID return value, that lists CPUID
Fn0000_0007_EAX_x0 content as reserved.

Thanks, Roger.
Andrew Cooper Sept. 10, 2019, 9:35 a.m. UTC | #2
On 10/09/2019 10:29, Roger Pau Monné wrote:
> On Mon, Sep 09, 2019 at 04:51:24PM +0100, Andrew Cooper wrote:
>> 7a0 is an integer field, not a mask - taking the logical and of the hardware
>> and policy values results in nonsense.  Instead, take the policy value
>> directly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Even Rome hardware has 7[0].eax still as 0, and there is no sensible reason to
>> set max_subleaf higher at this point, so this is only a latent bug for now.
>> ---
>>  xen/arch/x86/domctl.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>> index 1e98fc8009..35ad8cb51c 100644
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -218,11 +218,16 @@ static int update_domain_cpuid_info(struct domain *d,
>>          if ( is_pv_domain(d) && ((levelling_caps & LCAP_7ab0) == LCAP_7ab0) )
>>          {
>>              uint64_t mask = cpuidmask_defaults._7ab0;
>> -            uint32_t eax = ctl->eax;
>> -            uint32_t ebx = p->feat._7b0;
>>  
>> -            if ( boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
>> -                mask &= ((uint64_t)eax << 32) | ebx;
>> +            /*
>> +             * Leaf 7[0].eax is max_subleaf, not a feature mask.  Take it
>> +             * wholesale from the policy, but clamp the features in 7[0].ebx
>> +             * per usual.
>> +             */
>> +            if ( boot_cpu_data.x86_vendor &
>> +                 (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
>> +                mask = (((uint64_t)p->feat.max_subleaf << 32) |
>> +                        ((uint32_t)mask | p->feat._7b0));
> Why do you set the high bits of the mask (63:30) with the max subleaf?

63:32

> According to the document I have bits 63:30 are reserved, and that
> seems to match the expected CPUID return value, that lists CPUID
> Fn0000_0007_EAX_x0 content as reserved.

Yes, but reserved doesn't mean "will #GP".  As I said on IRC, this MSR
*is* the value which gets forwarded into a CPUID invocation, and setting
max_subleaf to 0xdead does work fine.

The point here is that in the future, on hardware capable of
max_subleaf=2 and being levelled to max_subleaf=1, the value observed in
CPUID should be 1, not 0.

The latter is what the current logic does, and is broken.  The CPUID
derivation logic will ensure that policy max_subleaf <= hardware
max_subleaf.

~Andrew
Roger Pau Monné Sept. 10, 2019, 9:57 a.m. UTC | #3
On Tue, Sep 10, 2019 at 10:35:26AM +0100, Andrew Cooper wrote:
> On 10/09/2019 10:29, Roger Pau Monné wrote:
> > On Mon, Sep 09, 2019 at 04:51:24PM +0100, Andrew Cooper wrote:
> >> 7a0 is an integer field, not a mask - taking the logical and of the hardware
> >> and policy values results in nonsense.  Instead, take the policy value
> >> directly.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Wei Liu <wl@xen.org>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Even Rome hardware has 7[0].eax still as 0, and there is no sensible reason to
> >> set max_subleaf higher at this point, so this is only a latent bug for now.
> >> ---
> >>  xen/arch/x86/domctl.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> >> index 1e98fc8009..35ad8cb51c 100644
> >> --- a/xen/arch/x86/domctl.c
> >> +++ b/xen/arch/x86/domctl.c
> >> @@ -218,11 +218,16 @@ static int update_domain_cpuid_info(struct domain *d,
> >>          if ( is_pv_domain(d) && ((levelling_caps & LCAP_7ab0) == LCAP_7ab0) )
> >>          {
> >>              uint64_t mask = cpuidmask_defaults._7ab0;
> >> -            uint32_t eax = ctl->eax;
> >> -            uint32_t ebx = p->feat._7b0;
> >>  
> >> -            if ( boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
> >> -                mask &= ((uint64_t)eax << 32) | ebx;
> >> +            /*
> >> +             * Leaf 7[0].eax is max_subleaf, not a feature mask.  Take it
> >> +             * wholesale from the policy, but clamp the features in 7[0].ebx
> >> +             * per usual.
> >> +             */
> >> +            if ( boot_cpu_data.x86_vendor &
> >> +                 (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
> >> +                mask = (((uint64_t)p->feat.max_subleaf << 32) |
> >> +                        ((uint32_t)mask | p->feat._7b0));
> > Why do you set the high bits of the mask (63:30) with the max subleaf?
> 
> 63:32
> 
> > According to the document I have bits 63:30 are reserved, and that
> > seems to match the expected CPUID return value, that lists CPUID
> > Fn0000_0007_EAX_x0 content as reserved.
> 
> Yes, but reserved doesn't mean "will #GP".  As I said on IRC, this MSR
> *is* the value which gets forwarded into a CPUID invocation, and setting
> max_subleaf to 0xdead does work fine.
> 
> The point here is that in the future, on hardware capable of
> max_subleaf=2 and being levelled to max_subleaf=1, the value observed in
> CPUID should be 1, not 0.

Oh, and that's from Intel specification which does indeed list
Fn0000_0007_EAX_x0 as returning the max sub-leaf, and the expectation
is that AMD will follow suit.

> The latter is what the current logic does, and is broken.  The CPUID
> derivation logic will ensure that policy max_subleaf <= hardware
> max_subleaf.

Reviewed-by: Roger Pau Monné <roger.pau@cirtrix.com>

Thanks, Roger.
Jan Beulich Sept. 10, 2019, 10:24 a.m. UTC | #4
On 09.09.2019 17:51, Andrew Cooper wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -218,11 +218,16 @@ static int update_domain_cpuid_info(struct domain *d,
>          if ( is_pv_domain(d) && ((levelling_caps & LCAP_7ab0) == LCAP_7ab0) )
>          {
>              uint64_t mask = cpuidmask_defaults._7ab0;
> -            uint32_t eax = ctl->eax;
> -            uint32_t ebx = p->feat._7b0;
>  
> -            if ( boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
> -                mask &= ((uint64_t)eax << 32) | ebx;
> +            /*
> +             * Leaf 7[0].eax is max_subleaf, not a feature mask.  Take it
> +             * wholesale from the policy, but clamp the features in 7[0].ebx
> +             * per usual.
> +             */
> +            if ( boot_cpu_data.x86_vendor &
> +                 (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
> +                mask = (((uint64_t)p->feat.max_subleaf << 32) |
> +                        ((uint32_t)mask | p->feat._7b0));

Assuming you mean & instead of | here, with it fixed
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Otherwise I'm afraid I don't understand, and need further
explanation.

Jan
Andrew Cooper Sept. 10, 2019, 10:25 a.m. UTC | #5
On 10/09/2019 10:57, Roger Pau Monné wrote:
> On Tue, Sep 10, 2019 at 10:35:26AM +0100, Andrew Cooper wrote:
>> On 10/09/2019 10:29, Roger Pau Monné wrote:
>>> On Mon, Sep 09, 2019 at 04:51:24PM +0100, Andrew Cooper wrote:
>>>> 7a0 is an integer field, not a mask - taking the logical and of the hardware
>>>> and policy values results in nonsense.  Instead, take the policy value
>>>> directly.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Wei Liu <wl@xen.org>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Even Rome hardware has 7[0].eax still as 0, and there is no sensible reason to
>>>> set max_subleaf higher at this point, so this is only a latent bug for now.
>>>> ---
>>>>  xen/arch/x86/domctl.c | 13 +++++++++----
>>>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>>>> index 1e98fc8009..35ad8cb51c 100644
>>>> --- a/xen/arch/x86/domctl.c
>>>> +++ b/xen/arch/x86/domctl.c
>>>> @@ -218,11 +218,16 @@ static int update_domain_cpuid_info(struct domain *d,
>>>>          if ( is_pv_domain(d) && ((levelling_caps & LCAP_7ab0) == LCAP_7ab0) )
>>>>          {
>>>>              uint64_t mask = cpuidmask_defaults._7ab0;
>>>> -            uint32_t eax = ctl->eax;
>>>> -            uint32_t ebx = p->feat._7b0;
>>>>  
>>>> -            if ( boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
>>>> -                mask &= ((uint64_t)eax << 32) | ebx;
>>>> +            /*
>>>> +             * Leaf 7[0].eax is max_subleaf, not a feature mask.  Take it
>>>> +             * wholesale from the policy, but clamp the features in 7[0].ebx
>>>> +             * per usual.
>>>> +             */
>>>> +            if ( boot_cpu_data.x86_vendor &
>>>> +                 (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
>>>> +                mask = (((uint64_t)p->feat.max_subleaf << 32) |
>>>> +                        ((uint32_t)mask | p->feat._7b0));
>>> Why do you set the high bits of the mask (63:30) with the max subleaf?
>> 63:32
>>
>>> According to the document I have bits 63:30 are reserved, and that
>>> seems to match the expected CPUID return value, that lists CPUID
>>> Fn0000_0007_EAX_x0 content as reserved.
>> Yes, but reserved doesn't mean "will #GP".  As I said on IRC, this MSR
>> *is* the value which gets forwarded into a CPUID invocation, and setting
>> max_subleaf to 0xdead does work fine.
>>
>> The point here is that in the future, on hardware capable of
>> max_subleaf=2 and being levelled to max_subleaf=1, the value observed in
>> CPUID should be 1, not 0.
> Oh, and that's from Intel specification which does indeed list
> Fn0000_0007_EAX_x0 as returning the max sub-leaf, and the expectation
> is that AMD will follow suit.

The AMD CPUID documentation does identify CPUID 7[0].eax as
max_subleaf.  It just isn't cross referenced very well with the MSR.

>
>> The latter is what the current logic does, and is broken.  The CPUID
>> derivation logic will ensure that policy max_subleaf <= hardware
>> max_subleaf.
> Reviewed-by: Roger Pau Monné <roger.pau@cirtrix.com>

Thanks,

~Andrew
Andrew Cooper Sept. 10, 2019, 10:26 a.m. UTC | #6
On 10/09/2019 11:24, Jan Beulich wrote:
> On 09.09.2019 17:51, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -218,11 +218,16 @@ static int update_domain_cpuid_info(struct domain *d,
>>          if ( is_pv_domain(d) && ((levelling_caps & LCAP_7ab0) == LCAP_7ab0) )
>>          {
>>              uint64_t mask = cpuidmask_defaults._7ab0;
>> -            uint32_t eax = ctl->eax;
>> -            uint32_t ebx = p->feat._7b0;
>>  
>> -            if ( boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
>> -                mask &= ((uint64_t)eax << 32) | ebx;
>> +            /*
>> +             * Leaf 7[0].eax is max_subleaf, not a feature mask.  Take it
>> +             * wholesale from the policy, but clamp the features in 7[0].ebx
>> +             * per usual.
>> +             */
>> +            if ( boot_cpu_data.x86_vendor &
>> +                 (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
>> +                mask = (((uint64_t)p->feat.max_subleaf << 32) |
>> +                        ((uint32_t)mask | p->feat._7b0));
> Assuming you mean & instead of | here, with it fixed

Oops - I do.

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

Thanks,

~Andrew

Patch
diff mbox series

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 1e98fc8009..35ad8cb51c 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -218,11 +218,16 @@  static int update_domain_cpuid_info(struct domain *d,
         if ( is_pv_domain(d) && ((levelling_caps & LCAP_7ab0) == LCAP_7ab0) )
         {
             uint64_t mask = cpuidmask_defaults._7ab0;
-            uint32_t eax = ctl->eax;
-            uint32_t ebx = p->feat._7b0;
 
-            if ( boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
-                mask &= ((uint64_t)eax << 32) | ebx;
+            /*
+             * Leaf 7[0].eax is max_subleaf, not a feature mask.  Take it
+             * wholesale from the policy, but clamp the features in 7[0].ebx
+             * per usual.
+             */
+            if ( boot_cpu_data.x86_vendor &
+                 (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
+                mask = (((uint64_t)p->feat.max_subleaf << 32) |
+                        ((uint32_t)mask | p->feat._7b0));
 
             d->arch.pv.cpuidmasks->_7ab0 = mask;
         }