Re: [Qemu-devel] [PATCH v6 7/9] i386: Add support for CPUID_8000_001E for AMD

2018-05-14 Thread Moger, Babu 


> -Original Message-
> From: Eduardo Habkost [mailto:ehabk...@redhat.com]
> Sent: Monday, May 14, 2018 4:12 PM
> To: Moger, Babu <babu.mo...@amd.com>
> Cc: m...@redhat.com; mar...@redhat.com; pbonz...@redhat.com;
> r...@twiddle.net; mtosa...@redhat.com; ge...@hostfission.com;
> k...@tripleback.net; qemu-devel@nongnu.org; k...@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.mo...@amd.com>
> > Tested-by: Geoffrey McRae <ge...@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_cpuid8005(CPUCacheInfo *cache)
> >   (((CORES_IN_CMPLX - 1) * 2) + 1)  : \
> >   (CORES_IN_CMPLX - 1))
> >
> > +/* Definitions used on CPUID Leaf 0x801E */
> > +#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[0x8008].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[0x8006].ECX and
> CPUID[0x8006].EDX
> >   * @l3 can be NULL.
> > @@ -4098,6 +4104,14 @@ void cpu_x86_cpuid(CPUX86State *env,
> uint32_t index, uint32_t count,
> >  break;
> >  }
> >  break;
> > +case 0x801E:
> > +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 thi

Re: [Qemu-devel] [PATCH v6 7/9] i386: Add support for CPUID_8000_001E for AMD

2018-05-14 Thread Eduardo Habkost 
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 
> Tested-by: Geoffrey McRae 
> ---
>  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_cpuid8005(CPUCacheInfo 
> *cache)
>   (((CORES_IN_CMPLX - 1) * 2) + 1)  : \
>   (CORES_IN_CMPLX - 1))
>  
> +/* Definitions used on CPUID Leaf 0x801E */
> +#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[0x8008].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[0x8006].ECX and CPUID[0x8006].EDX
>   * @l3 can be NULL.
> @@ -4098,6 +4104,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>  break;
>  }
>  break;
> +case 0x801E:
> +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 0xC000:
>  *eax = env->cpuid_xlevel2;
>  *ebx = 0;
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo

[Qemu-devel] [PATCH v6 7/9] i386: Add support for CPUID_8000_001E for AMD

2018-04-10 Thread Babu Moger 
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 
Tested-by: Geoffrey McRae 
---
 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_cpuid8005(CPUCacheInfo 
*cache)
  (((CORES_IN_CMPLX - 1) * 2) + 1)  : \
  (CORES_IN_CMPLX - 1))
 
+/* Definitions used on CPUID Leaf 0x801E */
+#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[0x8006].ECX and CPUID[0x8006].EDX
  * @l3 can be NULL.
@@ -4098,6 +4104,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
uint32_t count,
 break;
 }
 break;
+case 0x801E:
+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 0xC000:
 *eax = env->cpuid_xlevel2;
 *ebx = 0;
-- 
1.8.3.1