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.coop...@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <jbeul...@suse.com>
>>>> CC: Wei Liu <w...@xen.org>
>>>> CC: Roger Pau Monné <roger....@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....@cirtrix.com>
Thanks,
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
