Message ID | 20190909155124.19942-1-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/cpuid: Fix handling of the CPUID.7[0].eax levelling MSR | expand |
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.
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
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.
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
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
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
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; }
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(-)