| Message ID | 20200417151432.46867.72601.stgit@localhost.localdomain |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386: Fix the CPUID leaf CPUID_Fn80000008 | expand |
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://lore.kernel.org/qemu-devel/158643709116.17430.15995069125716778943.malonedeb@wampee.canonical.com/#t > > 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://bugzilla.kernel.org/show_bug.cgi?id=206537 > > Reported-by: Philipp Eppelt <1871842@bugs.launchpad.net> > Signed-off-by: Babu Moger <babu.moger@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. > + 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? 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. > + *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. > + } 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://bugzilla.kernel.org/attachment.cgi?id=287395&action=edit
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 <1871842@bugs.launchpad.net> >> Signed-off-by: Babu Moger <babu.moger@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 > >
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; + 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); + *ecx = bits_required << 12 | max_apicids; + } else { + *ecx = 0; } + *edx = 0; break; case 0x8000000A: if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
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://lore.kernel.org/qemu-devel/158643709116.17430.15995069125716778943.malonedeb@wampee.canonical.com/#t 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://bugzilla.kernel.org/show_bug.cgi?id=206537 Reported-by: Philipp Eppelt <1871842@bugs.launchpad.net> Signed-off-by: Babu Moger <babu.moger@amd.com> --- target/i386/cpu.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)