| Message ID | 1523402169-113351-8-git-send-email-babu.moger@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 10, 2018 at 07:16:07PM -0400, Babu Moger wrote: > Populate threads/core_id/apic_ids/socket_id when CPUID_EXT3_TOPOEXT > feature is supported. This is required to support hyperthreading feature > on AMD CPUs. This is supported via CPUID_8000_001E extended functions. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > Tested-by: Geoffrey McRae <geoff@hostfission.com> > --- > target/i386/cpu.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 63f2d31..24b39c6 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -315,6 +315,12 @@ static uint32_t encode_cache_cpuid80000005(CPUCacheInfo *cache) > (((CORES_IN_CMPLX - 1) * 2) + 1) : \ > (CORES_IN_CMPLX - 1)) > > +/* Definitions used on CPUID Leaf 0x8000001E */ > +#define EXTENDED_APIC_ID(threads, socket_id, core_id, thread_id) \ > + ((threads) ? \ > + ((socket_id << 6) | (core_id << 1) | thread_id) : \ > + ((socket_id << 6) | core_id)) Using the AMD64 Architecture Programmer's Manual Volume 3 as reference below. The formula above assumes MNC = (2 ^ 6). The current code in QEMU sets ApicIdCoreIdSize (CPUID[0x80000008].ECX[bits 15:12]) to 0, which means MNC is calculated using the legacy method: MNC = CPUID Fn8000_0008_ECX[NC] + 1. The current code sets CPUID Fn8000_0008_ECX[NC] to: (cs->nr_cores * cs->nr_threads) - 1. This means we cannot hardcode MNC, and must calculate it based on nr_cores and nr_threads. Probably it's a good idea to fill ApicIdCoreIdSize if TOPOEXT is set, so we will know MNC will always be a power of 2 and the formula here will be compatible with the existing APIC ID calculation logic. Note that we already have a comment at the top of topology.h: * This code should be compatible with AMD's "Extended Method" described at: * AMD CPUID Specification (Publication #25481) * Section 3: Multiple Core Calcuation * as long as: * nr_threads is set to 1; * OFFSET_IDX is assumed to be 0; * CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width(). So you can already use cpu->apic_id here as long as ApicIdCoreSize is set to apicid_core_width(). Probably compatibility with AMD methods will be kept even if nr_threads > 1, but I didn't confirm that. Actually, I strongly advise you use cpu->apic_id here. Otherwise the Extended APIC ID seen by the guest here won't match the APIC ID used everywhere else inside QEMU and KVM code. > + > /* > * Encode cache info for CPUID[0x80000006].ECX and CPUID[0x80000006].EDX > * @l3 can be NULL. > @@ -4098,6 +4104,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > break; > } > break; > + case 0x8000001E: > + assert(cpu->core_id <= 255); > + *eax = EXTENDED_APIC_ID((cs->nr_threads - 1), > + cpu->socket_id, cpu->core_id, cpu->thread_id); > + *ebx = (cs->nr_threads - 1) << 8 | cpu->core_id; > + *ecx = cpu->socket_id; Terminology is confusing here, so let's confirm this is really what we want to do: * ThreadsPerComputeUnit is set to nr_threads-1. Looks correct. * ComputeUnitId is being set to core_id. Is this really what we want? * NodesPerProcessor is being set to 0 (meaning 1 node per processor) * NodeId is being set to socket_id. Is this really right, considering that NodesPerProcessor is being set to 0? If this is really what you intend to do, I'd like to see it better documented, to avoid confusion in the future. We could either add wrapper functions that make the logic more explicit (e.g. x86_cpu_node_id(), x86_cpu_nodes_per_processor(), etc.) or add comments explaining how the QEMU socket/core/thread abstractions are being mapped to the AMD socket/node/compute-unit/thread abstractions (also, how a "Logical CCX L3 complex" is mapped into the QEMU abstractions, here?) > + *edx = 0; > + break; > case 0xC0000000: > *eax = env->cpuid_xlevel2; > *ebx = 0; > -- > 1.8.3.1 > >
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Monday, May 14, 2018 4:12 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: mst@redhat.com; marcel@redhat.com; pbonzini@redhat.com; > rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com; > kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org > Subject: Re: [Qemu-devel] [PATCH v6 7/9] i386: Add support for > CPUID_8000_001E for AMD > > On Tue, Apr 10, 2018 at 07:16:07PM -0400, Babu Moger wrote: > > Populate threads/core_id/apic_ids/socket_id when CPUID_EXT3_TOPOEXT > > feature is supported. This is required to support hyperthreading feature > > on AMD CPUs. This is supported via CPUID_8000_001E extended functions. > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > Tested-by: Geoffrey McRae <geoff@hostfission.com> > > --- > > target/i386/cpu.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index 63f2d31..24b39c6 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -315,6 +315,12 @@ static uint32_t > encode_cache_cpuid80000005(CPUCacheInfo *cache) > > (((CORES_IN_CMPLX - 1) * 2) + 1) : \ > > (CORES_IN_CMPLX - 1)) > > > > +/* Definitions used on CPUID Leaf 0x8000001E */ > > +#define EXTENDED_APIC_ID(threads, socket_id, core_id, thread_id) \ > > + ((threads) ? \ > > + ((socket_id << 6) | (core_id << 1) | thread_id) : \ > > + ((socket_id << 6) | core_id)) > > Using the AMD64 Architecture Programmer's Manual Volume 3 as > reference below. > > The formula above assumes MNC = (2 ^ 6). > > The current code in QEMU sets ApicIdCoreIdSize > (CPUID[0x80000008].ECX[bits 15:12]) to 0, which means MNC is > calculated using the legacy method: > MNC = CPUID Fn8000_0008_ECX[NC] + 1. > > The current code sets CPUID Fn8000_0008_ECX[NC] to: > (cs->nr_cores * cs->nr_threads) - 1. > > This means we cannot hardcode MNC, and must calculate it based on > nr_cores and nr_threads. > > Probably it's a good idea to fill ApicIdCoreIdSize if TOPOEXT is > set, so we will know MNC will always be a power of 2 and the > formula here will be compatible with the existing APIC ID > calculation logic. > > Note that we already have a comment at the top of topology.h: > > * This code should be compatible with AMD's "Extended Method" described > at: > * AMD CPUID Specification (Publication #25481) > * Section 3: Multiple Core Calcuation > * as long as: > * nr_threads is set to 1; > * OFFSET_IDX is assumed to be 0; > * CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to > apicid_core_width(). > > So you can already use cpu->apic_id here as long as > ApicIdCoreSize is set to apicid_core_width(). Probably > compatibility with AMD methods will be kept even if > nr_threads > 1, but I didn't confirm that. > > Actually, I strongly advise you use cpu->apic_id here. Otherwise Yes. We should be able to use cpu->apic_id here. Thanks for pointing this out. Will run more tests to confirm. Thanks > the Extended APIC ID seen by the guest here won't match the APIC > ID used everywhere else inside QEMU and KVM code. > > > > + > > /* > > * Encode cache info for CPUID[0x80000006].ECX and > CPUID[0x80000006].EDX > > * @l3 can be NULL. > > @@ -4098,6 +4104,14 @@ void cpu_x86_cpuid(CPUX86State *env, > uint32_t index, uint32_t count, > > break; > > } > > break; > > + case 0x8000001E: > > + assert(cpu->core_id <= 255); > > + *eax = EXTENDED_APIC_ID((cs->nr_threads - 1), > > + cpu->socket_id, cpu->core_id, cpu->thread_id); > > + *ebx = (cs->nr_threads - 1) << 8 | cpu->core_id; > > + *ecx = cpu->socket_id; > > Terminology is confusing here, so let's confirm this is really > what we want to do: > * ThreadsPerComputeUnit is set to nr_threads-1. Looks correct. > * ComputeUnitId is being set to core_id. Is this really what we > want? > * NodesPerProcessor is being set to 0 (meaning 1 node per > processor) > * NodeId is being set to socket_id. Is this really right, > considering that NodesPerProcessor is being set to 0? Yes. You are right. ComputeUnitId, NodesPerProcessor and core_id is not set correctly. Let me study little bit and ask around here. Will respond again. > > If this is really what you intend to do, I'd like to see it > better documented, to avoid confusion in the future. > > We could either add wrapper functions that make the logic more > explicit (e.g. x86_cpu_node_id(), x86_cpu_nodes_per_processor(), > etc.) or add comments explaining how the QEMU socket/core/thread > abstractions are being mapped to the AMD > socket/node/compute-unit/thread abstractions (also, how a > "Logical CCX L3 complex" is mapped into the QEMU abstractions, > here?) > > > > + *edx = 0; > > + break; > > case 0xC0000000: > > *eax = env->cpuid_xlevel2; > > *ebx = 0; > > -- > > 1.8.3.1 > > > > > > -- > Eduardo
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 63f2d31..24b39c6 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -315,6 +315,12 @@ static uint32_t encode_cache_cpuid80000005(CPUCacheInfo *cache) (((CORES_IN_CMPLX - 1) * 2) + 1) : \ (CORES_IN_CMPLX - 1)) +/* Definitions used on CPUID Leaf 0x8000001E */ +#define EXTENDED_APIC_ID(threads, socket_id, core_id, thread_id) \ + ((threads) ? \ + ((socket_id << 6) | (core_id << 1) | thread_id) : \ + ((socket_id << 6) | core_id)) + /* * Encode cache info for CPUID[0x80000006].ECX and CPUID[0x80000006].EDX * @l3 can be NULL. @@ -4098,6 +4104,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, break; } break; + case 0x8000001E: + assert(cpu->core_id <= 255); + *eax = EXTENDED_APIC_ID((cs->nr_threads - 1), + cpu->socket_id, cpu->core_id, cpu->thread_id); + *ebx = (cs->nr_threads - 1) << 8 | cpu->core_id; + *ecx = cpu->socket_id; + *edx = 0; + break; case 0xC0000000: *eax = env->cpuid_xlevel2; *ebx = 0;