Re: [Xen-devel] [PATCH] x86/cpuid: Fix handling of the CPUID.7[0].eax levelling MSR

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
> >> ---
> >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >> CC: Wei Liu <wl@xxxxxxx>
> >> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxx>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel