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 <1871...@bugs.launchpad.net>
>> Signed-off-by: Babu Moger <babu.mo...@amd.com>
>> ---
>> 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&data=02%7C01%7Cbabu.moger%40amd.com%7C1b8d59370cdb403dd54308d7e303adb7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637227477274521298&sdata=UsM3h4vp3dTgigqOvt7GrGiIUHvH8Kn1g%2BO%2FfGMav%2Bc%3D&reserved=0
>
>
