Re: [PATCH] target/i386: Fix the CPUID leaf CPUID_Fn80000008

[Babu Moger]

On 4/17/20 2:15 PM, Eduardo Habkost wrote:
> Good catch, thanks for the patch.  Comments below:
> 
> On Fri, Apr 17, 2020 at 10:14:32AM -0500, Babu Moger wrote:
>> CPUID leaf CPUID_Fn80000008_ECX provides information about the
>> number of threads supported by the processor. It was found that
>> the field ApicIdSize(bits 15-12) was not set correctly.
>>
>> ApicIdSize is defined as the number of bits required to represent
>> all the ApicId values within a package.
>>
>> Valid Values: Value Description
>> 3h-0h                Reserved.
>> 4h           up to 16 threads.
>> 5h           up to 32 threads.
>> 6h           up to 64 threads.
>> 7h           up to 128 threads.
>> Fh-8h                Reserved.
>>
>> Fix the bit appropriately.
>>
>> This came up during following thread.
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F158643709116.17430.15995069125716778943.malonedeb%40wampee.canonical.com%2F%23t&data=02%7C01%7Cbabu.moger%40amd.com%7C1b8d59370cdb403dd54308d7e303adb7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637227477274521298&sdata=NZHLwOkQrbjkGeqYSI0wgRNUd3QHRCf7lBtdqoR5XfI%3D&reserved=0
>>
>> Refer the Processor Programming Reference (PPR) for AMD Family 17h
>> Model 01h, Revision B1 Processors. The documentation is available
>> from the bugzilla Link below.
>> Link: 
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.moger%40amd.com%7C1b8d59370cdb403dd54308d7e303adb7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637227477274521298&sdata=oNLqu0J49eTrJ8pQ6GKg64ZUDfV3egZN2VVkU0DwMaU%3D&reserved=0
>>
>> Reported-by: Philipp Eppelt <address@hidden>
>> Signed-off-by: Babu Moger <address@hidden>
>> ---
>>  target/i386/cpu.c |   12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 90ffc5f..68210f6 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -5830,11 +5830,17 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
>> uint32_t count,
>>              *eax = cpu->phys_bits;
>>          }
>>          *ebx = env->features[FEAT_8000_0008_EBX];
>> -        *ecx = 0;
>> -        *edx = 0;
>>          if (cs->nr_cores * cs->nr_threads > 1) {
>> -            *ecx |= (cs->nr_cores * cs->nr_threads) - 1;
> 
> I'm not sure we want a compatibility flag to keep ABI on older
> machine types, here.  Strictly speaking, CPUID must never change
> on older machine types, but sometimes trying hard to emulate bugs
> of old QEMU versions is a pointless exercise.

Not sure about this. But it seemed like nobody cared about this field before.
> 
> 
>> +            unsigned int max_apicids, bits_required;
>> +
>> +            max_apicids = (cs->nr_cores * cs->nr_threads) - 1;
>> +            /* Find out the number of bits to represent all the apicids */
>> +            bits_required = 32 - clz32(max_apicids);
> 
> This won't work if nr_cores > 1 and nr_threads is not a power of
> 2, will it?

It seem to work. Tested with threads=5,cores=3.

> 
> For reference, the field is documented[1] as:
> 
> "The number of bits in the initial Core::X86::Apic::ApicId[ApicId]
> value that indicate thread ID within a package"
> 
> This sounds like the value already stored at
> CPUX86State::pkg_offset.

Yes, it is already in pkg_offset. We can use it.

> 
> 
>> +            *ecx = bits_required << 12 | max_apicids;
> 
> Bits 7:0 are documented as "The number of threads in the package
> is NC+1", with no reference to APIC IDs at all.
> 
> Using ((nr_cores * nr_threads) - 1) for bits 7:0 sounds correct,
> but the variable name seems misleading.

I can change the variable name to num_threads.
> 
> 
>> +        } else {
>> +            *ecx = 0;
>>          }
>> +        *edx = 0;
>>          break;
>>      case 0x8000000A:
>>          if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
>>
>>
> 
> References:
> 
> [1] Processor Programming Reference (PPR) for
>     AMD Family 17h Model 18h, Revision B1 Processors
>     55570-B1 Rev 3.14 - Sep 26, 2019
>     
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fattachment.cgi%3Fid%3D287395%26action%3Dedit&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C1b8d59370cdb403dd54308d7e303adb7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637227477274521298&amp;sdata=UsM3h4vp3dTgigqOvt7GrGiIUHvH8Kn1g%2BO%2FfGMav%2Bc%3D&amp;reserved=0
> 
>