Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.09.16 at 16:27, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 24/08/16 16:31, Jan Beulich wrote:
>> Another place where we should try to behave like real hardware; see
>> the code comments.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3358,6 +3358,31 @@ void hvm_cpuid(unsigned int input, unsig
>>      if ( !edx )
>>          edx = &dummy;
>>  
>> +    if ( input & 0xffff )
>> +    {
>> +        /*
>> +         * Requests beyond the highest supported leaf within a group return
>> +         * zero on AMD and the highest basic leaf output on others.
>> +         */
>> +        unsigned int lvl;
>> +
>> +        hvm_cpuid(input & 0xffff0000, &lvl, NULL, NULL, NULL);
>> +        if ( ((lvl ^ input) >> 16) || input > lvl )
> 
> This logic isn't correct.  It doesn't cope in the Intel case when lvl
> aliases the upper 16 bits of input, despite input being an unknown group.
> 
> When I considered the problem before, the only functioning logic I came
> up with was to know that for Intel, input = 0x8000xxxx is the only
> special case which doesn't collapse into the highest basic leaf.

If we had followed to Intel model before the extended leaf got
introduced, we'd have been incompatible with the extended leaf.
If ever someone introduces another group, we'd then again be
screwed. Yes, we can follow the Intel model, but I intentionally
tried to make the code more forward compatible.

>> +        {
>> +            if ( d->arch.x86_vendor == X86_VENDOR_AMD )
>> +            {
>> +                *eax = 0;
>> +                *ebx = 0;
>> +                *ecx = 0;
>> +                *edx = 0;
>> +                return;
>> +            }
>> +            if ( input >> 16 )
>> +                hvm_cpuid(0, &lvl, NULL, NULL, NULL);
> 
> Is this really the right way round?  The AMD method of "reserved always
> as zero" is the more sane default to take.

If anything I'd then say let's _always_ follow the AMD model.

> I have looked at the Transmeta and Cyrix CPUID docs, and they are
> non-specific as to what reserved means.

While the Cyrix box I have doesn't stay up long enough anymore
to try out in practice, I do recall that in various aspects they very
closely followed what Intel does. There not being any 64-bit
Transmeta CPUs we support, I don't think we currently care about
their behavior.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel