[PATCH v4 0/3] Modify AMD topology to use socket/dies/core/thread model

[PATCH v4 0/3] Modify AMD topology to use socket/dies/core/thread model

[Babu Moger]

This series fixes couple of issues with recent topology related code.

1. Modify AMD topology to use socket/dies/core/thread model

2. Error out if the user does not pass the dies information if EPYC cpu is numa
   configured.

3. Remove the node_id references in topology and use die_id instead.

4. With node_id removed in topology the uninitialized memory issue 
   with -device and CPU hotplug will be fixed.
   Link: https://bugzilla.redhat.com/show_bug.cgi?id=1828750
---

v4:
  Not much of a change. Just added few text changes.
  Error out configuration instead of warning if dies are not configured in EPYC.
  Few other text changes to clarify the removal of node_id, nr_nodes and nodes_per_pkg.

v3:
  https://lore.kernel.org/qemu-devel/159681772267.9679.1334429994189974662.stgit@naples-babu.amd.com/#r
  Added a new check to pass the dies for EPYC numa configuration.
  Added Simplify CPUID_8000_001E patch with some changes suggested by Igor.
  Dropped the patch to build the topology from CpuInstanceProperties.
  TODO: Not sure if we still need the Autonuma changes Igor mentioned.
  Needs more clarity on that.

v2:
   https://lore.kernel.org/qemu-devel/159362436285.36204.986406297373871949.stgit@naples-babu.amd.com/
 - Used the numa information from CpuInstanceProperties for building
   the apic_id suggested by Igor.
 - Also did some minor code re-aarangement to take care of changes.
 - Dropped the patch "Simplify CPUID_8000_001E" from v1. Will send
   it later.

v1:
 https://lore.kernel.org/qemu-devel/159164739269.20543.3074052993891532749.stgit@naples-babu.amd.com

Babu Moger (3):
      i386: Simplify CPUID_8000_001E for AMD
      hw/i386: Update the EPYC topology to use socket/dies/core/thread model
      hw/i386: Remove node_id, nr_nodes and nodes_per_pkg from topology


 hw/i386/pc.c               |    1 -
 hw/i386/x86.c              |    9 ++++-
 include/hw/i386/topology.h |   40 +++++---------------
 target/i386/cpu.c          |   86 +++++++++++++++++++-------------------------
 target/i386/cpu.h          |    1 -
 5 files changed, 55 insertions(+), 82 deletions(-)

--

[PATCH v4 1/3] i386: Simplify CPUID_8000_001E for AMD

[Babu Moger]

apic_id contains all the information required to build CPUID_8000_001E.
Also remove the restriction on number bits on core_id and node_id.
Remove all the hardcoded values and replace with generalized
fields.

Refer the Processor Programming Reference (PPR) documentation
available from the bugzilla Link below.

Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 target/i386/cpu.c |   77 +++++++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 588f32e136..c892432cae 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -388,57 +388,52 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
 {
     X86CPUTopoIDs topo_ids = {0};
     unsigned long nodes = MAX(topo_info->nodes_per_pkg, 1);
-    int shift;
 
     x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
 
     *eax = cpu->apic_id;
+
     /*
+     * CPUID_Fn8000001E_EBX [Core Identifiers] (CoreId)
+     * Read-only. Reset: 0000_XXXXh.
+     * See Core::X86::Cpuid::ExtApicId.
+     * Core::X86::Cpuid::CoreId_lthree[1:0]_core[3:0]_thread[1:0];
      * CPUID_Fn8000001E_EBX
-     * 31:16 Reserved
-     * 15:8  Threads per core (The number of threads per core is
-     *       Threads per core + 1)
-     *  7:0  Core id (see bit decoding below)
-     *       SMT:
-     *           4:3 node id
-     *             2 Core complex id
-     *           1:0 Core id
-     *       Non SMT:
-     *           5:4 node id
-     *             3 Core complex id
-     *           1:0 Core id
+     * Bits Description
+     * 31:16 Reserved.
+     * 15:8 ThreadsPerCore: threads per core. Read-only. Reset: XXh.
+     *      The number of threads per core is ThreadsPerCore+1.
+     *  7:0 CoreId: core ID. Read-only. Reset: XXh.
+     *
+     *  NOTE: CoreId is already part of apic_id. Just use it. We can
+     *  use all the 8 bits to represent the core_id here.
      */
-    *ebx = ((topo_info->threads_per_core - 1) << 8) | (topo_ids.node_id << 3) |
-            (topo_ids.core_id);
+    *ebx = ((topo_info->threads_per_core - 1) << 8) | (topo_ids.core_id & 0xFF);
+
     /*
+     * CPUID_Fn8000001E_ECX [Node Identifiers] (NodeId)
+     * Read-only. Reset: 0000_0XXXh.
+     * Core::X86::Cpuid::NodeId_lthree[1:0]_core[3:0]_thread[1:0];
      * CPUID_Fn8000001E_ECX
-     * 31:11 Reserved
-     * 10:8  Nodes per processor (Nodes per processor is number of nodes + 1)
-     *  7:0  Node id (see bit decoding below)
-     *         2  Socket id
-     *       1:0  Node id
+     * Bits Description
+     * 31:11 Reserved.
+     * 10:8 NodesPerProcessor: Node per processor. Read-only. Reset: XXXb.
+     *      ValidValues:
+     *      Value Description
+     *      000b  1 node per processor.
+     *      001b  2 nodes per processor.
+     *      010b Reserved.
+     *      011b 4 nodes per processor.
+     *      111b-100b Reserved.
+     *  7:0 NodeId: Node ID. Read-only. Reset: XXh.
+     *
+     * NOTE: Hardware reserves 3 bits for number of nodes per processor.
+     * But users can create more nodes than the actual hardware can
+     * support. To genaralize we can use all the upper 8 bits for nodes.
+     * NodeId is combination of node and socket_id which is already decoded
+     * in apic_id. Just use it by shifting.
      */
-    if (nodes <= 4) {
-        *ecx = ((nodes - 1) << 8) | (topo_ids.pkg_id << 2) | topo_ids.node_id;
-    } else {
-        /*
-         * Node id fix up. Actual hardware supports up to 4 nodes. But with
-         * more than 32 cores, we may end up with more than 4 nodes.
-         * Node id is a combination of socket id and node id. Only requirement
-         * here is that this number should be unique accross the system.
-         * Shift the socket id to accommodate more nodes. We dont expect both
-         * socket id and node id to be big number at the same time. This is not
-         * an ideal config but we need to to support it. Max nodes we can have
-         * is 32 (255/8) with 8 cores per node and 255 max cores. We only need
-         * 5 bits for nodes. Find the left most set bit to represent the total
-         * number of nodes. find_last_bit returns last set bit(0 based). Left
-         * shift(+1) the socket id to represent all the nodes.
-         */
-        nodes -= 1;
-        shift = find_last_bit(&nodes, 8);
-        *ecx = (nodes << 8) | (topo_ids.pkg_id << (shift + 1)) |
-               topo_ids.node_id;
-    }
+    *ecx = ((nodes - 1) << 8) | cpu->node_id;
     *edx = 0;
 }
 

[PATCH v4 2/3] hw/i386: Update the EPYC topology to use socket/dies/core/thread model

[Babu Moger]

Update the EPYC topology to use socket/dies/core/thread model. The EPYC
model does not use the smp dies to build the topology. Instead, it uses
numa nodes to build the topology. Internally both are similar concept
which divides the cores on L3 boundary. Combining both into one terminology
makes it simple to program.

Add a new check to error out when smp dies are not provided when EPYC
model is numa configured. Next task is to remove node_id, nr_nodes and
nodes_per_pkg from EPYC topology which will be done in next patch.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 hw/i386/x86.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 67bee1bcb8..e90c42d2fc 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -138,6 +138,14 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
 
     /* Check for apicid encoding */
     if (cpu_x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
+        if ((ms->numa_state->num_nodes > 0) &&
+            ms->numa_state->num_nodes != (ms->smp.sockets * x86ms->smp_dies)) {
+            error_setg(&error_fatal, "Numa configuration here requires smp "
+                       "'dies' parameter. Configure the cpu topology properly "
+                       "with max_cpus = sockets * dies * cores * threads. Dies"
+                       " is equivalent to number of numa nodes in a socket.");
+            return;
+        }
         x86_set_epyc_topo_handlers(ms);
     }
 

Re: [PATCH v4 2/3] hw/i386: Update the EPYC topology to use socket/dies/core/thread model

[Igor Mammedov]

On Fri, 14 Aug 2020 16:39:33 -0500
Babu Moger <babu.moger@amd.com> wrote:

> Update the EPYC topology to use socket/dies/core/thread model. The EPYC
> model does not use the smp dies to build the topology. Instead, it uses
> numa nodes to build the topology. Internally both are similar concept
> which divides the cores on L3 boundary. Combining both into one terminology
> makes it simple to program.
> 
> Add a new check to error out when smp dies are not provided when EPYC
> model is numa configured. Next task is to remove node_id, nr_nodes and
> nodes_per_pkg from EPYC topology which will be done in next patch.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  hw/i386/x86.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 67bee1bcb8..e90c42d2fc 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -138,6 +138,14 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>  
>      /* Check for apicid encoding */
>      if (cpu_x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
> +        if ((ms->numa_state->num_nodes > 0) &&

> +            ms->numa_state->num_nodes != (ms->smp.sockets * x86ms->smp_dies)) {
this case is gated by (ms->numa_state->num_nodes > 0) so it won't work in case
 -smp dies=>1 but there is no -numa options at all

we need to error out and ask to provide numa nodes corresponding to
   (ms->numa_state->num_nodes == 0) && (ms->smp.sockets * x86ms->smp_dies)

or better alternative would be to enable autonuma when EPYC cpu is enabled,
that will insure that this patch will work even if user hasn't specified -numa option,
since it will create a single numa node automatically.

The later will take care of (-smp 1,dies=1) case, which is broken due to
lack of explicit -numa we end up with CPU_UNSET_NUMA_NODE_ID in CPUID_Fn8000001E_ECX.

> +            error_setg(&error_fatal, "Numa configuration here requires smp "
> +                       "'dies' parameter. Configure the cpu topology properly "
> +                       "with max_cpus = sockets * dies * cores * threads. Dies"
> +                       " is equivalent to number of numa nodes in a socket.");
> +            return;
> +        }
>          x86_set_epyc_topo_handlers(ms);
>      }
>  
> 

RE: [PATCH v4 2/3] hw/i386: Update the EPYC topology to use socket/dies/core/thread model

[Babu Moger]

> -----Original Message-----
> From: Igor Mammedov <imammedo@redhat.com>
> Sent: Wednesday, August 19, 2020 6:26 AM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: pbonzini@redhat.com; rth@twiddle.net; ehabkost@redhat.com;
> mst@redhat.com; marcel.apfelbaum@gmail.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v4 2/3] hw/i386: Update the EPYC topology to use
> socket/dies/core/thread model
> 
> On Fri, 14 Aug 2020 16:39:33 -0500
> Babu Moger <babu.moger@amd.com> wrote:
> 
> > Update the EPYC topology to use socket/dies/core/thread model. The
> > EPYC model does not use the smp dies to build the topology. Instead,
> > it uses numa nodes to build the topology. Internally both are similar
> > concept which divides the cores on L3 boundary. Combining both into
> > one terminology makes it simple to program.
> >
> > Add a new check to error out when smp dies are not provided when EPYC
> > model is numa configured. Next task is to remove node_id, nr_nodes and
> > nodes_per_pkg from EPYC topology which will be done in next patch.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  hw/i386/x86.c |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/hw/i386/x86.c b/hw/i386/x86.c index
> > 67bee1bcb8..e90c42d2fc 100644
> > --- a/hw/i386/x86.c
> > +++ b/hw/i386/x86.c
> > @@ -138,6 +138,14 @@ void x86_cpus_init(X86MachineState *x86ms, int
> > default_cpu_version)
> >
> >      /* Check for apicid encoding */
> >      if (cpu_x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
> > +        if ((ms->numa_state->num_nodes > 0) &&
> 
> > +            ms->numa_state->num_nodes != (ms->smp.sockets *
> > + x86ms->smp_dies)) {
> this case is gated by (ms->numa_state->num_nodes > 0) so it won't work in case
> -smp dies=>1 but there is no -numa options at all

Looking back again, this check does not appear to be a good idea. The numa
information is coming from ACPI and die information is coming from CPU
internal hardware config. It may not match. I don't think this check will
be good option. Sorry. I did not think about that earlier.

> 
> we need to error out and ask to provide numa nodes corresponding to
>    (ms->numa_state->num_nodes == 0) && (ms->smp.sockets * x86ms-
> >smp_dies)
> 
> or better alternative would be to enable autonuma when EPYC cpu is enabled,
> that will insure that this patch will work even if user hasn't specified -numa
> option, since it will create a single numa node automatically.
> 
> The later will take care of (-smp 1,dies=1) case, which is broken due to lack of
> explicit -numa we end up with CPU_UNSET_NUMA_NODE_ID in
> CPUID_Fn8000001E_ECX.
> 
> > +            error_setg(&error_fatal, "Numa configuration here requires smp "
> > +                       "'dies' parameter. Configure the cpu topology properly "
> > +                       "with max_cpus = sockets * dies * cores * threads. Dies"
> > +                       " is equivalent to number of numa nodes in a socket.");
> > +            return;
> > +        }
> >          x86_set_epyc_topo_handlers(ms);
> >      }
> >
> >

[PATCH v4 3/3] hw/i386: Remove node_id, nr_nodes and nodes_per_pkg from topology

[Babu Moger]

Remove node_id, nr_nodes and nodes_per_pkg from topology. Use
die_id, nr_dies and dies_per_pkg which is already available.
Removes the confusion over two variables.

With node_id removed in topology the uninitialized memory issue
with -device and CPU hotplug will be fixed.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1828750
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 hw/i386/pc.c               |    1 -
 hw/i386/x86.c              |    1 -
 include/hw/i386/topology.h |   40 +++++++++-------------------------------
 target/i386/cpu.c          |   11 +++--------
 target/i386/cpu.h          |    1 -
 5 files changed, 12 insertions(+), 42 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 47c5ca3e34..0ae5cb3af4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1498,7 +1498,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
     init_topo_info(&topo_info, x86ms);
 
     env->nr_dies = x86ms->smp_dies;
-    env->nr_nodes = topo_info.nodes_per_pkg;
     env->pkg_offset = x86ms->apicid_pkg_offset(&topo_info);
 
     /*
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index e90c42d2fc..4efa1f8b87 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -62,7 +62,6 @@ inline void init_topo_info(X86CPUTopoInfo *topo_info,
 {
     MachineState *ms = MACHINE(x86ms);
 
-    topo_info->nodes_per_pkg = ms->numa_state->num_nodes / ms->smp.sockets;
     topo_info->dies_per_pkg = x86ms->smp_dies;
     topo_info->cores_per_die = ms->smp.cores;
     topo_info->threads_per_core = ms->smp.threads;
diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index 07239f95f4..05ddde7ba0 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -47,14 +47,12 @@ typedef uint32_t apic_id_t;
 
 typedef struct X86CPUTopoIDs {
     unsigned pkg_id;
-    unsigned node_id;
     unsigned die_id;
     unsigned core_id;
     unsigned smt_id;
 } X86CPUTopoIDs;
 
 typedef struct X86CPUTopoInfo {
-    unsigned nodes_per_pkg;
     unsigned dies_per_pkg;
     unsigned cores_per_die;
     unsigned threads_per_core;
@@ -89,11 +87,6 @@ static inline unsigned apicid_die_width(X86CPUTopoInfo *topo_info)
     return apicid_bitwidth_for_count(topo_info->dies_per_pkg);
 }
 
-/* Bit width of the node_id field per socket */
-static inline unsigned apicid_node_width_epyc(X86CPUTopoInfo *topo_info)
-{
-    return apicid_bitwidth_for_count(MAX(topo_info->nodes_per_pkg, 1));
-}
 /* Bit offset of the Core_ID field
  */
 static inline unsigned apicid_core_offset(X86CPUTopoInfo *topo_info)
@@ -114,30 +107,23 @@ static inline unsigned apicid_pkg_offset(X86CPUTopoInfo *topo_info)
     return apicid_die_offset(topo_info) + apicid_die_width(topo_info);
 }
 
-#define NODE_ID_OFFSET 3 /* Minimum node_id offset if numa configured */
+#define EPYC_DIE_OFFSET 3 /* Minimum die_id offset if numa configured */
 
 /*
- * Bit offset of the node_id field
- *
- * Make sure nodes_per_pkg >  0 if numa configured else zero.
+ * Bit offset of the die_id field
  */
-static inline unsigned apicid_node_offset_epyc(X86CPUTopoInfo *topo_info)
+static inline unsigned apicid_die_offset_epyc(X86CPUTopoInfo *topo_info)
 {
-    unsigned offset = apicid_die_offset(topo_info) +
-                      apicid_die_width(topo_info);
+    unsigned offset = apicid_core_offset(topo_info) +
+                      apicid_core_width(topo_info);
 
-    if (topo_info->nodes_per_pkg) {
-        return MAX(NODE_ID_OFFSET, offset);
-    } else {
-        return offset;
-    }
+    return MAX(EPYC_DIE_OFFSET, offset);
 }
 
 /* Bit offset of the Pkg_ID (socket ID) field */
 static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info)
 {
-    return apicid_node_offset_epyc(topo_info) +
-           apicid_node_width_epyc(topo_info);
+    return apicid_die_offset_epyc(topo_info) + apicid_die_width(topo_info);
 }
 
 /*
@@ -150,8 +136,7 @@ x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
                               const X86CPUTopoIDs *topo_ids)
 {
     return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
-           (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) |
-           (topo_ids->die_id  << apicid_die_offset(topo_info)) |
+           (topo_ids->die_id  << apicid_die_offset_epyc(topo_info)) |
            (topo_ids->core_id << apicid_core_offset(topo_info)) |
            topo_ids->smt_id;
 }
@@ -160,15 +145,11 @@ static inline void x86_topo_ids_from_idx_epyc(X86CPUTopoInfo *topo_info,
                                               unsigned cpu_index,
                                               X86CPUTopoIDs *topo_ids)
 {
-    unsigned nr_nodes = MAX(topo_info->nodes_per_pkg, 1);
     unsigned nr_dies = topo_info->dies_per_pkg;
     unsigned nr_cores = topo_info->cores_per_die;
     unsigned nr_threads = topo_info->threads_per_core;
-    unsigned cores_per_node = DIV_ROUND_UP((nr_dies * nr_cores * nr_threads),
-                                            nr_nodes);
 
     topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads);
-    topo_ids->node_id = (cpu_index / cores_per_node) % nr_nodes;
     topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies;
     topo_ids->core_id = cpu_index / nr_threads % nr_cores;
     topo_ids->smt_id = cpu_index % nr_threads;
@@ -188,11 +169,8 @@ static inline void x86_topo_ids_from_apicid_epyc(apic_id_t apicid,
             (apicid >> apicid_core_offset(topo_info)) &
             ~(0xFFFFFFFFUL << apicid_core_width(topo_info));
     topo_ids->die_id =
-            (apicid >> apicid_die_offset(topo_info)) &
+            (apicid >> apicid_die_offset_epyc(topo_info)) &
             ~(0xFFFFFFFFUL << apicid_die_width(topo_info));
-    topo_ids->node_id =
-            (apicid >> apicid_node_offset_epyc(topo_info)) &
-            ~(0xFFFFFFFFUL << apicid_node_width_epyc(topo_info));
     topo_ids->pkg_id = apicid >> apicid_pkg_offset_epyc(topo_info);
 }
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c892432cae..ba0a24f6b8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -345,7 +345,6 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
                                        uint32_t *ecx, uint32_t *edx)
 {
     uint32_t l3_cores;
-    unsigned nodes = MAX(topo_info->nodes_per_pkg, 1);
 
     assert(cache->size == cache->line_size * cache->associativity *
                           cache->partitions * cache->sets);
@@ -355,10 +354,9 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
 
     /* L3 is shared among multiple cores */
     if (cache->level == 3) {
-        l3_cores = DIV_ROUND_UP((topo_info->dies_per_pkg *
-                                 topo_info->cores_per_die *
+        l3_cores = DIV_ROUND_UP((topo_info->cores_per_die *
                                  topo_info->threads_per_core),
-                                 nodes);
+                                 topo_info->dies_per_pkg);
         *eax |= (l3_cores - 1) << 14;
     } else {
         *eax |= ((topo_info->threads_per_core - 1) << 14);
@@ -387,7 +385,6 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
                                        uint32_t *ecx, uint32_t *edx)
 {
     X86CPUTopoIDs topo_ids = {0};
-    unsigned long nodes = MAX(topo_info->nodes_per_pkg, 1);
 
     x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
 
@@ -433,7 +430,7 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
      * NodeId is combination of node and socket_id which is already decoded
      * in apic_id. Just use it by shifting.
      */
-    *ecx = ((nodes - 1) << 8) | cpu->node_id;
+    *ecx = ((topo_info->dies_per_pkg - 1) << 8) | cpu->node_id;
     *edx = 0;
 }
 
@@ -5484,7 +5481,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     uint32_t signature[3];
     X86CPUTopoInfo topo_info;
 
-    topo_info.nodes_per_pkg = env->nr_nodes;
     topo_info.dies_per_pkg = env->nr_dies;
     topo_info.cores_per_die = cs->nr_cores;
     topo_info.threads_per_core = cs->nr_threads;
@@ -6944,7 +6940,6 @@ static void x86_cpu_initfn(Object *obj)
     FeatureWord w;
 
     env->nr_dies = 1;
-    env->nr_nodes = 1;
     cpu_set_cpustate_pointers(cpu);
 
     object_property_add(obj, "family", "int",
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index e1a5c174dc..4c89bee8d1 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1629,7 +1629,6 @@ typedef struct CPUX86State {
     TPRAccess tpr_access_type;
 
     unsigned nr_dies;
-    unsigned nr_nodes;
     unsigned pkg_offset;
 } CPUX86State;
 

Re: [PATCH v4 3/3] hw/i386: Remove node_id, nr_nodes and nodes_per_pkg from topology

[Igor Mammedov]

On Fri, 14 Aug 2020 16:39:40 -0500
Babu Moger <babu.moger@amd.com> wrote:

> Remove node_id, nr_nodes and nodes_per_pkg from topology. Use
> die_id, nr_dies and dies_per_pkg which is already available.
> Removes the confusion over two variables.
> 
> With node_id removed in topology the uninitialized memory issue
> with -device and CPU hotplug will be fixed.
> 
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1828750
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  hw/i386/pc.c               |    1 -
>  hw/i386/x86.c              |    1 -
>  include/hw/i386/topology.h |   40 +++++++++-------------------------------
>  target/i386/cpu.c          |   11 +++--------
>  target/i386/cpu.h          |    1 -
>  5 files changed, 12 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 47c5ca3e34..0ae5cb3af4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1498,7 +1498,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      init_topo_info(&topo_info, x86ms);
>  
>      env->nr_dies = x86ms->smp_dies;
> -    env->nr_nodes = topo_info.nodes_per_pkg;
>      env->pkg_offset = x86ms->apicid_pkg_offset(&topo_info);
>  
>      /*
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index e90c42d2fc..4efa1f8b87 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -62,7 +62,6 @@ inline void init_topo_info(X86CPUTopoInfo *topo_info,
>  {
>      MachineState *ms = MACHINE(x86ms);
>  
> -    topo_info->nodes_per_pkg = ms->numa_state->num_nodes / ms->smp.sockets;
>      topo_info->dies_per_pkg = x86ms->smp_dies;
>      topo_info->cores_per_die = ms->smp.cores;
>      topo_info->threads_per_core = ms->smp.threads;
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index 07239f95f4..05ddde7ba0 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -47,14 +47,12 @@ typedef uint32_t apic_id_t;
>  
>  typedef struct X86CPUTopoIDs {
>      unsigned pkg_id;
> -    unsigned node_id;
>      unsigned die_id;
>      unsigned core_id;
>      unsigned smt_id;
>  } X86CPUTopoIDs;
>  
>  typedef struct X86CPUTopoInfo {
> -    unsigned nodes_per_pkg;
>      unsigned dies_per_pkg;
>      unsigned cores_per_die;
>      unsigned threads_per_core;
> @@ -89,11 +87,6 @@ static inline unsigned apicid_die_width(X86CPUTopoInfo *topo_info)
>      return apicid_bitwidth_for_count(topo_info->dies_per_pkg);
>  }
>  
> -/* Bit width of the node_id field per socket */
> -static inline unsigned apicid_node_width_epyc(X86CPUTopoInfo *topo_info)
> -{
> -    return apicid_bitwidth_for_count(MAX(topo_info->nodes_per_pkg, 1));
> -}
>  /* Bit offset of the Core_ID field
>   */
>  static inline unsigned apicid_core_offset(X86CPUTopoInfo *topo_info)
> @@ -114,30 +107,23 @@ static inline unsigned apicid_pkg_offset(X86CPUTopoInfo *topo_info)
>      return apicid_die_offset(topo_info) + apicid_die_width(topo_info);
>  }
>  
> -#define NODE_ID_OFFSET 3 /* Minimum node_id offset if numa configured */
> +#define EPYC_DIE_OFFSET 3 /* Minimum die_id offset if numa configured */
                                                      ^^

from EPYC's pov NUMA is always configured, there no 'if'

but I have a question, is it possible to drop EPYC_DIE_OFFSET
and reuse apicid_die_offset(), will it break something?

The reason for question is that, we (deviating from spec)
do allow for unbound core/threads number so die_id, already could
be shifted beyound ApicId[5:4], likewise we do allow for unbound
sockets so ApicId[6] is also out of spec.
If we are fine with ApicId being encoded in these cases out of
spec, then why should we insist on DIE_OFFSET minumum at all?
Why not just allow existing apicid_die_width() to encode die_id?

In this case it will produce SPECed ApicId if user has provided
-smp that matches currently released EPYC's and in all other cases
it will be out of spec ApicId like when we set sockets/cores/trheads
to out of spec values.
  
>  /*
> - * Bit offset of the node_id field
> - *
> - * Make sure nodes_per_pkg >  0 if numa configured else zero.
> + * Bit offset of the die_id field
>   */
> -static inline unsigned apicid_node_offset_epyc(X86CPUTopoInfo *topo_info)
> +static inline unsigned apicid_die_offset_epyc(X86CPUTopoInfo *topo_info)
>  {
> -    unsigned offset = apicid_die_offset(topo_info) +
> -                      apicid_die_width(topo_info);
> +    unsigned offset = apicid_core_offset(topo_info) +
> +                      apicid_core_width(topo_info);
>  
> -    if (topo_info->nodes_per_pkg) {
> -        return MAX(NODE_ID_OFFSET, offset);
> -    } else {
> -        return offset;
> -    }
> +    return MAX(EPYC_DIE_OFFSET, offset);
>  }
>  
>  /* Bit offset of the Pkg_ID (socket ID) field */
>  static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info)
>  {
> -    return apicid_node_offset_epyc(topo_info) +
> -           apicid_node_width_epyc(topo_info);
> +    return apicid_die_offset_epyc(topo_info) + apicid_die_width(topo_info);
>  }
>  
>  /*
> @@ -150,8 +136,7 @@ x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
>                                const X86CPUTopoIDs *topo_ids)
>  {
>      return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
> -           (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) |
> -           (topo_ids->die_id  << apicid_die_offset(topo_info)) |
> +           (topo_ids->die_id  << apicid_die_offset_epyc(topo_info)) |
>             (topo_ids->core_id << apicid_core_offset(topo_info)) |
>             topo_ids->smt_id;
>  }
> @@ -160,15 +145,11 @@ static inline void x86_topo_ids_from_idx_epyc(X86CPUTopoInfo *topo_info,
>                                                unsigned cpu_index,
>                                                X86CPUTopoIDs *topo_ids)
>  {
> -    unsigned nr_nodes = MAX(topo_info->nodes_per_pkg, 1);
>      unsigned nr_dies = topo_info->dies_per_pkg;
>      unsigned nr_cores = topo_info->cores_per_die;
>      unsigned nr_threads = topo_info->threads_per_core;
> -    unsigned cores_per_node = DIV_ROUND_UP((nr_dies * nr_cores * nr_threads),
> -                                            nr_nodes);
>  
>      topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads);
> -    topo_ids->node_id = (cpu_index / cores_per_node) % nr_nodes;
>      topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies;
>      topo_ids->core_id = cpu_index / nr_threads % nr_cores;
>      topo_ids->smt_id = cpu_index % nr_threads;
> @@ -188,11 +169,8 @@ static inline void x86_topo_ids_from_apicid_epyc(apic_id_t apicid,
>              (apicid >> apicid_core_offset(topo_info)) &
>              ~(0xFFFFFFFFUL << apicid_core_width(topo_info));
>      topo_ids->die_id =
> -            (apicid >> apicid_die_offset(topo_info)) &
> +            (apicid >> apicid_die_offset_epyc(topo_info)) &
>              ~(0xFFFFFFFFUL << apicid_die_width(topo_info));
> -    topo_ids->node_id =
> -            (apicid >> apicid_node_offset_epyc(topo_info)) &
> -            ~(0xFFFFFFFFUL << apicid_node_width_epyc(topo_info));
>      topo_ids->pkg_id = apicid >> apicid_pkg_offset_epyc(topo_info);
>  }
>  
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c892432cae..ba0a24f6b8 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -345,7 +345,6 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>                                         uint32_t *ecx, uint32_t *edx)
>  {
>      uint32_t l3_cores;
> -    unsigned nodes = MAX(topo_info->nodes_per_pkg, 1);
>  
>      assert(cache->size == cache->line_size * cache->associativity *
>                            cache->partitions * cache->sets);
> @@ -355,10 +354,9 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>  
>      /* L3 is shared among multiple cores */
>      if (cache->level == 3) {
> -        l3_cores = DIV_ROUND_UP((topo_info->dies_per_pkg *
> -                                 topo_info->cores_per_die *
> +        l3_cores = DIV_ROUND_UP((topo_info->cores_per_die *
>                                   topo_info->threads_per_core),
> -                                 nodes);
> +                                 topo_info->dies_per_pkg);
>          *eax |= (l3_cores - 1) << 14;
>      } else {
>          *eax |= ((topo_info->threads_per_core - 1) << 14);
> @@ -387,7 +385,6 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>                                         uint32_t *ecx, uint32_t *edx)
>  {
>      X86CPUTopoIDs topo_ids = {0};
> -    unsigned long nodes = MAX(topo_info->nodes_per_pkg, 1);
>  
>      x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
>  
> @@ -433,7 +430,7 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>       * NodeId is combination of node and socket_id which is already decoded
>       * in apic_id. Just use it by shifting.
>       */
> -    *ecx = ((nodes - 1) << 8) | cpu->node_id;
> +    *ecx = ((topo_info->dies_per_pkg - 1) << 8) | cpu->node_id;
>      *edx = 0;
>  }
>  
> @@ -5484,7 +5481,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>      uint32_t signature[3];
>      X86CPUTopoInfo topo_info;
>  
> -    topo_info.nodes_per_pkg = env->nr_nodes;
>      topo_info.dies_per_pkg = env->nr_dies;
>      topo_info.cores_per_die = cs->nr_cores;
>      topo_info.threads_per_core = cs->nr_threads;
> @@ -6944,7 +6940,6 @@ static void x86_cpu_initfn(Object *obj)
>      FeatureWord w;
>  
>      env->nr_dies = 1;
> -    env->nr_nodes = 1;
>      cpu_set_cpustate_pointers(cpu);
>  
>      object_property_add(obj, "family", "int",
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index e1a5c174dc..4c89bee8d1 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1629,7 +1629,6 @@ typedef struct CPUX86State {
>      TPRAccess tpr_access_type;
>  
>      unsigned nr_dies;
> -    unsigned nr_nodes;
>      unsigned pkg_offset;
>  } CPUX86State;
>  
> 
> 

Re: [PATCH v4 3/3] hw/i386: Remove node_id, nr_nodes and nodes_per_pkg from topology

[Babu Moger]

On 8/19/20 7:18 AM, Igor Mammedov wrote:
> On Fri, 14 Aug 2020 16:39:40 -0500
> Babu Moger <babu.moger@amd.com> wrote:
> 
>> Remove node_id, nr_nodes and nodes_per_pkg from topology. Use
>> die_id, nr_dies and dies_per_pkg which is already available.
>> Removes the confusion over two variables.
>>
>> With node_id removed in topology the uninitialized memory issue
>> with -device and CPU hotplug will be fixed.
>>
>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1828750&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C466254703c904bd4c86c08d8443a0e91%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637334363427680286&amp;sdata=%2Fr7Mucca8Pr%2BrjvwJ6S9zxiIg3HCKAiPp4PvGP3nvZI%3D&amp;reserved=0
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  hw/i386/pc.c               |    1 -
>>  hw/i386/x86.c              |    1 -
>>  include/hw/i386/topology.h |   40 +++++++++-------------------------------
>>  target/i386/cpu.c          |   11 +++--------
>>  target/i386/cpu.h          |    1 -
>>  5 files changed, 12 insertions(+), 42 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 47c5ca3e34..0ae5cb3af4 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1498,7 +1498,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>      init_topo_info(&topo_info, x86ms);
>>  
>>      env->nr_dies = x86ms->smp_dies;
>> -    env->nr_nodes = topo_info.nodes_per_pkg;
>>      env->pkg_offset = x86ms->apicid_pkg_offset(&topo_info);
>>  
>>      /*
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index e90c42d2fc..4efa1f8b87 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -62,7 +62,6 @@ inline void init_topo_info(X86CPUTopoInfo *topo_info,
>>  {
>>      MachineState *ms = MACHINE(x86ms);
>>  
>> -    topo_info->nodes_per_pkg = ms->numa_state->num_nodes / ms->smp.sockets;
>>      topo_info->dies_per_pkg = x86ms->smp_dies;
>>      topo_info->cores_per_die = ms->smp.cores;
>>      topo_info->threads_per_core = ms->smp.threads;
>> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
>> index 07239f95f4..05ddde7ba0 100644
>> --- a/include/hw/i386/topology.h
>> +++ b/include/hw/i386/topology.h
>> @@ -47,14 +47,12 @@ typedef uint32_t apic_id_t;
>>  
>>  typedef struct X86CPUTopoIDs {
>>      unsigned pkg_id;
>> -    unsigned node_id;
>>      unsigned die_id;
>>      unsigned core_id;
>>      unsigned smt_id;
>>  } X86CPUTopoIDs;
>>  
>>  typedef struct X86CPUTopoInfo {
>> -    unsigned nodes_per_pkg;
>>      unsigned dies_per_pkg;
>>      unsigned cores_per_die;
>>      unsigned threads_per_core;
>> @@ -89,11 +87,6 @@ static inline unsigned apicid_die_width(X86CPUTopoInfo *topo_info)
>>      return apicid_bitwidth_for_count(topo_info->dies_per_pkg);
>>  }
>>  
>> -/* Bit width of the node_id field per socket */
>> -static inline unsigned apicid_node_width_epyc(X86CPUTopoInfo *topo_info)
>> -{
>> -    return apicid_bitwidth_for_count(MAX(topo_info->nodes_per_pkg, 1));
>> -}
>>  /* Bit offset of the Core_ID field
>>   */
>>  static inline unsigned apicid_core_offset(X86CPUTopoInfo *topo_info)
>> @@ -114,30 +107,23 @@ static inline unsigned apicid_pkg_offset(X86CPUTopoInfo *topo_info)
>>      return apicid_die_offset(topo_info) + apicid_die_width(topo_info);
>>  }
>>  
>> -#define NODE_ID_OFFSET 3 /* Minimum node_id offset if numa configured */
>> +#define EPYC_DIE_OFFSET 3 /* Minimum die_id offset if numa configured */
>                                                       ^^
> 
> from EPYC's pov NUMA is always configured, there no 'if'
> 
> but I have a question, is it possible to drop EPYC_DIE_OFFSET
> and reuse apicid_die_offset(), will it break something?

Yes. I am also thinking about it. We can go back to existing
apicid_die_width().

Looking back again, I think all these code changes related to node_id is
causing more issues than fixes. We have added all these code to handle
some non- SPECed configurations like
 https://bugzilla.redhat.com/show_bug.cgi?id=1728166.

Going forward it might create even more issues. Now, I think we should go
back and remove all these changes and just use the default decoding. Most
of the SPECed configuration would work just fine. With some non-SPECed
user inputs, it will create some sub-optimal configuration. If we can live
with that we will be just fine. I did not anticipate all these problems
when I originally started this work. Sorry, my bad.

> 
> The reason for question is that, we (deviating from spec)
> do allow for unbound core/threads number so die_id, already could
> be shifted beyound ApicId[5:4], likewise we do allow for unbound
> sockets so ApicId[6] is also out of spec.
> If we are fine with ApicId being encoded in these cases out of
> spec, then why should we insist on DIE_OFFSET minumum at all?
> Why not just allow existing apicid_die_width() to encode die_id?
> 
> In this case it will produce SPECed ApicId if user has provided
> -smp that matches currently released EPYC's and in all other cases
> it will be out of spec ApicId like when we set sockets/cores/trheads
> to out of spec values.
>   
>>  /*
>> - * Bit offset of the node_id field
>> - *
>> - * Make sure nodes_per_pkg >  0 if numa configured else zero.
>> + * Bit offset of the die_id field
>>   */
>> -static inline unsigned apicid_node_offset_epyc(X86CPUTopoInfo *topo_info)
>> +static inline unsigned apicid_die_offset_epyc(X86CPUTopoInfo *topo_info)
>>  {
>> -    unsigned offset = apicid_die_offset(topo_info) +
>> -                      apicid_die_width(topo_info);
>> +    unsigned offset = apicid_core_offset(topo_info) +
>> +                      apicid_core_width(topo_info);
>>  
>> -    if (topo_info->nodes_per_pkg) {
>> -        return MAX(NODE_ID_OFFSET, offset);
>> -    } else {
>> -        return offset;
>> -    }
>> +    return MAX(EPYC_DIE_OFFSET, offset);
>>  }
>>  
>>  /* Bit offset of the Pkg_ID (socket ID) field */
>>  static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info)
>>  {
>> -    return apicid_node_offset_epyc(topo_info) +
>> -           apicid_node_width_epyc(topo_info);
>> +    return apicid_die_offset_epyc(topo_info) + apicid_die_width(topo_info);
>>  }
>>  
>>  /*
>> @@ -150,8 +136,7 @@ x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
>>                                const X86CPUTopoIDs *topo_ids)
>>  {
>>      return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
>> -           (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) |
>> -           (topo_ids->die_id  << apicid_die_offset(topo_info)) |
>> +           (topo_ids->die_id  << apicid_die_offset_epyc(topo_info)) |
>>             (topo_ids->core_id << apicid_core_offset(topo_info)) |
>>             topo_ids->smt_id;
>>  }
>> @@ -160,15 +145,11 @@ static inline void x86_topo_ids_from_idx_epyc(X86CPUTopoInfo *topo_info,
>>                                                unsigned cpu_index,
>>                                                X86CPUTopoIDs *topo_ids)
>>  {
>> -    unsigned nr_nodes = MAX(topo_info->nodes_per_pkg, 1);
>>      unsigned nr_dies = topo_info->dies_per_pkg;
>>      unsigned nr_cores = topo_info->cores_per_die;
>>      unsigned nr_threads = topo_info->threads_per_core;
>> -    unsigned cores_per_node = DIV_ROUND_UP((nr_dies * nr_cores * nr_threads),
>> -                                            nr_nodes);
>>  
>>      topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads);
>> -    topo_ids->node_id = (cpu_index / cores_per_node) % nr_nodes;
>>      topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies;
>>      topo_ids->core_id = cpu_index / nr_threads % nr_cores;
>>      topo_ids->smt_id = cpu_index % nr_threads;
>> @@ -188,11 +169,8 @@ static inline void x86_topo_ids_from_apicid_epyc(apic_id_t apicid,
>>              (apicid >> apicid_core_offset(topo_info)) &
>>              ~(0xFFFFFFFFUL << apicid_core_width(topo_info));
>>      topo_ids->die_id =
>> -            (apicid >> apicid_die_offset(topo_info)) &
>> +            (apicid >> apicid_die_offset_epyc(topo_info)) &
>>              ~(0xFFFFFFFFUL << apicid_die_width(topo_info));
>> -    topo_ids->node_id =
>> -            (apicid >> apicid_node_offset_epyc(topo_info)) &
>> -            ~(0xFFFFFFFFUL << apicid_node_width_epyc(topo_info));
>>      topo_ids->pkg_id = apicid >> apicid_pkg_offset_epyc(topo_info);
>>  }
>>  
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index c892432cae..ba0a24f6b8 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -345,7 +345,6 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>>                                         uint32_t *ecx, uint32_t *edx)
>>  {
>>      uint32_t l3_cores;
>> -    unsigned nodes = MAX(topo_info->nodes_per_pkg, 1);
>>  
>>      assert(cache->size == cache->line_size * cache->associativity *
>>                            cache->partitions * cache->sets);
>> @@ -355,10 +354,9 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>>  
>>      /* L3 is shared among multiple cores */
>>      if (cache->level == 3) {
>> -        l3_cores = DIV_ROUND_UP((topo_info->dies_per_pkg *
>> -                                 topo_info->cores_per_die *
>> +        l3_cores = DIV_ROUND_UP((topo_info->cores_per_die *
>>                                   topo_info->threads_per_core),
>> -                                 nodes);
>> +                                 topo_info->dies_per_pkg);
>>          *eax |= (l3_cores - 1) << 14;
>>      } else {
>>          *eax |= ((topo_info->threads_per_core - 1) << 14);
>> @@ -387,7 +385,6 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>>                                         uint32_t *ecx, uint32_t *edx)
>>  {
>>      X86CPUTopoIDs topo_ids = {0};
>> -    unsigned long nodes = MAX(topo_info->nodes_per_pkg, 1);
>>  
>>      x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
>>  
>> @@ -433,7 +430,7 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>>       * NodeId is combination of node and socket_id which is already decoded
>>       * in apic_id. Just use it by shifting.
>>       */
>> -    *ecx = ((nodes - 1) << 8) | cpu->node_id;
>> +    *ecx = ((topo_info->dies_per_pkg - 1) << 8) | cpu->node_id;
>>      *edx = 0;
>>  }
>>  
>> @@ -5484,7 +5481,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>      uint32_t signature[3];
>>      X86CPUTopoInfo topo_info;
>>  
>> -    topo_info.nodes_per_pkg = env->nr_nodes;
>>      topo_info.dies_per_pkg = env->nr_dies;
>>      topo_info.cores_per_die = cs->nr_cores;
>>      topo_info.threads_per_core = cs->nr_threads;
>> @@ -6944,7 +6940,6 @@ static void x86_cpu_initfn(Object *obj)
>>      FeatureWord w;
>>  
>>      env->nr_dies = 1;
>> -    env->nr_nodes = 1;
>>      cpu_set_cpustate_pointers(cpu);
>>  
>>      object_property_add(obj, "family", "int",
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index e1a5c174dc..4c89bee8d1 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -1629,7 +1629,6 @@ typedef struct CPUX86State {
>>      TPRAccess tpr_access_type;
>>  
>>      unsigned nr_dies;
>> -    unsigned nr_nodes;
>>      unsigned pkg_offset;
>>  } CPUX86State;
>>  
>>
>>
> 

Re: [PATCH v4 3/3] hw/i386: Remove node_id, nr_nodes and nodes_per_pkg from topology

[Igor Mammedov]

On Wed, 19 Aug 2020 17:42:58 -0500
Babu Moger <babu.moger@amd.com> wrote:

> On 8/19/20 7:18 AM, Igor Mammedov wrote:
> > On Fri, 14 Aug 2020 16:39:40 -0500
> > Babu Moger <babu.moger@amd.com> wrote:
> >   
> >> Remove node_id, nr_nodes and nodes_per_pkg from topology. Use
> >> die_id, nr_dies and dies_per_pkg which is already available.
> >> Removes the confusion over two variables.
> >>
> >> With node_id removed in topology the uninitialized memory issue
> >> with -device and CPU hotplug will be fixed.
> >>
> >> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1828750&amp;data=02%7C01%7Cbabu.moger%40amd.com%7C466254703c904bd4c86c08d8443a0e91%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637334363427680286&amp;sdata=%2Fr7Mucca8Pr%2BrjvwJ6S9zxiIg3HCKAiPp4PvGP3nvZI%3D&amp;reserved=0
> >> Signed-off-by: Babu Moger <babu.moger@amd.com>
> >> ---
> >>  hw/i386/pc.c               |    1 -
> >>  hw/i386/x86.c              |    1 -
> >>  include/hw/i386/topology.h |   40 +++++++++-------------------------------
> >>  target/i386/cpu.c          |   11 +++--------
> >>  target/i386/cpu.h          |    1 -
> >>  5 files changed, 12 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 47c5ca3e34..0ae5cb3af4 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1498,7 +1498,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >>      init_topo_info(&topo_info, x86ms);
> >>  
> >>      env->nr_dies = x86ms->smp_dies;
> >> -    env->nr_nodes = topo_info.nodes_per_pkg;
> >>      env->pkg_offset = x86ms->apicid_pkg_offset(&topo_info);
> >>  
> >>      /*
> >> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> >> index e90c42d2fc..4efa1f8b87 100644
> >> --- a/hw/i386/x86.c
> >> +++ b/hw/i386/x86.c
> >> @@ -62,7 +62,6 @@ inline void init_topo_info(X86CPUTopoInfo *topo_info,
> >>  {
> >>      MachineState *ms = MACHINE(x86ms);
> >>  
> >> -    topo_info->nodes_per_pkg = ms->numa_state->num_nodes / ms->smp.sockets;
> >>      topo_info->dies_per_pkg = x86ms->smp_dies;
> >>      topo_info->cores_per_die = ms->smp.cores;
> >>      topo_info->threads_per_core = ms->smp.threads;
> >> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> >> index 07239f95f4..05ddde7ba0 100644
> >> --- a/include/hw/i386/topology.h
> >> +++ b/include/hw/i386/topology.h
> >> @@ -47,14 +47,12 @@ typedef uint32_t apic_id_t;
> >>  
> >>  typedef struct X86CPUTopoIDs {
> >>      unsigned pkg_id;
> >> -    unsigned node_id;
> >>      unsigned die_id;
> >>      unsigned core_id;
> >>      unsigned smt_id;
> >>  } X86CPUTopoIDs;
> >>  
> >>  typedef struct X86CPUTopoInfo {
> >> -    unsigned nodes_per_pkg;
> >>      unsigned dies_per_pkg;
> >>      unsigned cores_per_die;
> >>      unsigned threads_per_core;
> >> @@ -89,11 +87,6 @@ static inline unsigned apicid_die_width(X86CPUTopoInfo *topo_info)
> >>      return apicid_bitwidth_for_count(topo_info->dies_per_pkg);
> >>  }
> >>  
> >> -/* Bit width of the node_id field per socket */
> >> -static inline unsigned apicid_node_width_epyc(X86CPUTopoInfo *topo_info)
> >> -{
> >> -    return apicid_bitwidth_for_count(MAX(topo_info->nodes_per_pkg, 1));
> >> -}
> >>  /* Bit offset of the Core_ID field
> >>   */
> >>  static inline unsigned apicid_core_offset(X86CPUTopoInfo *topo_info)
> >> @@ -114,30 +107,23 @@ static inline unsigned apicid_pkg_offset(X86CPUTopoInfo *topo_info)
> >>      return apicid_die_offset(topo_info) + apicid_die_width(topo_info);
> >>  }
> >>  
> >> -#define NODE_ID_OFFSET 3 /* Minimum node_id offset if numa configured */
> >> +#define EPYC_DIE_OFFSET 3 /* Minimum die_id offset if numa configured */  
> >                                                       ^^
> > 
> > from EPYC's pov NUMA is always configured, there no 'if'
> > 
> > but I have a question, is it possible to drop EPYC_DIE_OFFSET
> > and reuse apicid_die_offset(), will it break something?  
> 
> Yes. I am also thinking about it. We can go back to existing
> apicid_die_width().

Given we are using die_id now, we probably can get rid of all
topo hooks to generate apicid.

> Looking back again, I think all these code changes related to node_id is
> causing more issues than fixes. We have added all these code to handle
> some non- SPECed configurations like
>  https://bugzilla.redhat.com/show_bug.cgi?id=1728166.
> 
> Going forward it might create even more issues. Now, I think we should go
> back and remove all these changes and just use the default decoding. Most
> of the SPECed configuration would work just fine. With some non-SPECed
> user inputs, it will create some sub-optimal configuration. If we can live
> with that we will be just fine. I did not anticipate all these problems
> when I originally started this work. Sorry, my bad.

Topology code is complex and sometimes it's easier to add a new thing,
it's just that not every time it turns out as expected.
We overlooked posiblilty to reuse die-id, which has lead to more
complex and fragile outcome.

But it's fine, as long as it gets fixed in the end.



> > The reason for question is that, we (deviating from spec)
> > do allow for unbound core/threads number so die_id, already could
> > be shifted beyound ApicId[5:4], likewise we do allow for unbound
> > sockets so ApicId[6] is also out of spec.
> > If we are fine with ApicId being encoded in these cases out of
> > spec, then why should we insist on DIE_OFFSET minumum at all?
> > Why not just allow existing apicid_die_width() to encode die_id?
> > 
> > In this case it will produce SPECed ApicId if user has provided
> > -smp that matches currently released EPYC's and in all other cases
> > it will be out of spec ApicId like when we set sockets/cores/trheads
> > to out of spec values.
> >     
> >>  /*
> >> - * Bit offset of the node_id field
> >> - *
> >> - * Make sure nodes_per_pkg >  0 if numa configured else zero.
> >> + * Bit offset of the die_id field
> >>   */
> >> -static inline unsigned apicid_node_offset_epyc(X86CPUTopoInfo *topo_info)
> >> +static inline unsigned apicid_die_offset_epyc(X86CPUTopoInfo *topo_info)
> >>  {
> >> -    unsigned offset = apicid_die_offset(topo_info) +
> >> -                      apicid_die_width(topo_info);
> >> +    unsigned offset = apicid_core_offset(topo_info) +
> >> +                      apicid_core_width(topo_info);
> >>  
> >> -    if (topo_info->nodes_per_pkg) {
> >> -        return MAX(NODE_ID_OFFSET, offset);
> >> -    } else {
> >> -        return offset;
> >> -    }
> >> +    return MAX(EPYC_DIE_OFFSET, offset);
> >>  }
> >>  
> >>  /* Bit offset of the Pkg_ID (socket ID) field */
> >>  static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info)
> >>  {
> >> -    return apicid_node_offset_epyc(topo_info) +
> >> -           apicid_node_width_epyc(topo_info);
> >> +    return apicid_die_offset_epyc(topo_info) + apicid_die_width(topo_info);
> >>  }
> >>  
> >>  /*
> >> @@ -150,8 +136,7 @@ x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
> >>                                const X86CPUTopoIDs *topo_ids)
> >>  {
> >>      return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
> >> -           (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) |
> >> -           (topo_ids->die_id  << apicid_die_offset(topo_info)) |
> >> +           (topo_ids->die_id  << apicid_die_offset_epyc(topo_info)) |
> >>             (topo_ids->core_id << apicid_core_offset(topo_info)) |
> >>             topo_ids->smt_id;
> >>  }
> >> @@ -160,15 +145,11 @@ static inline void x86_topo_ids_from_idx_epyc(X86CPUTopoInfo *topo_info,
> >>                                                unsigned cpu_index,
> >>                                                X86CPUTopoIDs *topo_ids)
> >>  {
> >> -    unsigned nr_nodes = MAX(topo_info->nodes_per_pkg, 1);
> >>      unsigned nr_dies = topo_info->dies_per_pkg;
> >>      unsigned nr_cores = topo_info->cores_per_die;
> >>      unsigned nr_threads = topo_info->threads_per_core;
> >> -    unsigned cores_per_node = DIV_ROUND_UP((nr_dies * nr_cores * nr_threads),
> >> -                                            nr_nodes);
> >>  
> >>      topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads);
> >> -    topo_ids->node_id = (cpu_index / cores_per_node) % nr_nodes;
> >>      topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies;
> >>      topo_ids->core_id = cpu_index / nr_threads % nr_cores;
> >>      topo_ids->smt_id = cpu_index % nr_threads;
> >> @@ -188,11 +169,8 @@ static inline void x86_topo_ids_from_apicid_epyc(apic_id_t apicid,
> >>              (apicid >> apicid_core_offset(topo_info)) &
> >>              ~(0xFFFFFFFFUL << apicid_core_width(topo_info));
> >>      topo_ids->die_id =
> >> -            (apicid >> apicid_die_offset(topo_info)) &
> >> +            (apicid >> apicid_die_offset_epyc(topo_info)) &
> >>              ~(0xFFFFFFFFUL << apicid_die_width(topo_info));
> >> -    topo_ids->node_id =
> >> -            (apicid >> apicid_node_offset_epyc(topo_info)) &
> >> -            ~(0xFFFFFFFFUL << apicid_node_width_epyc(topo_info));
> >>      topo_ids->pkg_id = apicid >> apicid_pkg_offset_epyc(topo_info);
> >>  }
> >>  
> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >> index c892432cae..ba0a24f6b8 100644
> >> --- a/target/i386/cpu.c
> >> +++ b/target/i386/cpu.c
> >> @@ -345,7 +345,6 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
> >>                                         uint32_t *ecx, uint32_t *edx)
> >>  {
> >>      uint32_t l3_cores;
> >> -    unsigned nodes = MAX(topo_info->nodes_per_pkg, 1);
> >>  
> >>      assert(cache->size == cache->line_size * cache->associativity *
> >>                            cache->partitions * cache->sets);
> >> @@ -355,10 +354,9 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
> >>  
> >>      /* L3 is shared among multiple cores */
> >>      if (cache->level == 3) {
> >> -        l3_cores = DIV_ROUND_UP((topo_info->dies_per_pkg *
> >> -                                 topo_info->cores_per_die *
> >> +        l3_cores = DIV_ROUND_UP((topo_info->cores_per_die *
> >>                                   topo_info->threads_per_core),
> >> -                                 nodes);
> >> +                                 topo_info->dies_per_pkg);
> >>          *eax |= (l3_cores - 1) << 14;
> >>      } else {
> >>          *eax |= ((topo_info->threads_per_core - 1) << 14);
> >> @@ -387,7 +385,6 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
> >>                                         uint32_t *ecx, uint32_t *edx)
> >>  {
> >>      X86CPUTopoIDs topo_ids = {0};
> >> -    unsigned long nodes = MAX(topo_info->nodes_per_pkg, 1);
> >>  
> >>      x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
> >>  
> >> @@ -433,7 +430,7 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
> >>       * NodeId is combination of node and socket_id which is already decoded
> >>       * in apic_id. Just use it by shifting.
> >>       */
> >> -    *ecx = ((nodes - 1) << 8) | cpu->node_id;
> >> +    *ecx = ((topo_info->dies_per_pkg - 1) << 8) | cpu->node_id;
> >>      *edx = 0;
> >>  }
> >>  
> >> @@ -5484,7 +5481,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >>      uint32_t signature[3];
> >>      X86CPUTopoInfo topo_info;
> >>  
> >> -    topo_info.nodes_per_pkg = env->nr_nodes;
> >>      topo_info.dies_per_pkg = env->nr_dies;
> >>      topo_info.cores_per_die = cs->nr_cores;
> >>      topo_info.threads_per_core = cs->nr_threads;
> >> @@ -6944,7 +6940,6 @@ static void x86_cpu_initfn(Object *obj)
> >>      FeatureWord w;
> >>  
> >>      env->nr_dies = 1;
> >> -    env->nr_nodes = 1;
> >>      cpu_set_cpustate_pointers(cpu);
> >>  
> >>      object_property_add(obj, "family", "int",
> >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> >> index e1a5c174dc..4c89bee8d1 100644
> >> --- a/target/i386/cpu.h
> >> +++ b/target/i386/cpu.h
> >> @@ -1629,7 +1629,6 @@ typedef struct CPUX86State {
> >>      TPRAccess tpr_access_type;
> >>  
> >>      unsigned nr_dies;
> >> -    unsigned nr_nodes;
> >>      unsigned pkg_offset;
> >>  } CPUX86State;
> >>  
> >>
> >>  
> >   
> 

Re: [PATCH v4 3/3] hw/i386: Remove node_id, nr_nodes and nodes_per_pkg from topology

[Babu Moger]

On 8/20/20 7:57 AM, Igor Mammedov wrote:
> On Wed, 19 Aug 2020 17:42:58 -0500
> Babu Moger <babu.moger@amd.com> wrote:
> 
>> On 8/19/20 7:18 AM, Igor Mammedov wrote:
>>> On Fri, 14 Aug 2020 16:39:40 -0500
>>> Babu Moger <babu.moger@amd.com> wrote:
>>>   
>>>> Remove node_id, nr_nodes and nodes_per_pkg from topology. Use
>>>> die_id, nr_dies and dies_per_pkg which is already available.
>>>> Removes the confusion over two variables.
>>>>
>>>> With node_id removed in topology the uninitialized memory issue
>>>> with -device and CPU hotplug will be fixed.
>>>>
>>>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1828750&amp;data=02%7C01%7Cbabu.moger%40amd.com%7Cf7ad03f0c04d44dd104c08d84508b0b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637335250919794679&amp;sdata=JT8rq3Ag29WMgD8p2v2tm%2Fmdo8nBnLHb8V7QUbCHCLs%3D&amp;reserved=0
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>>>  hw/i386/pc.c               |    1 -
>>>>  hw/i386/x86.c              |    1 -
>>>>  include/hw/i386/topology.h |   40 +++++++++-------------------------------
>>>>  target/i386/cpu.c          |   11 +++--------
>>>>  target/i386/cpu.h          |    1 -
>>>>  5 files changed, 12 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 47c5ca3e34..0ae5cb3af4 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -1498,7 +1498,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>>>      init_topo_info(&topo_info, x86ms);
>>>>  
>>>>      env->nr_dies = x86ms->smp_dies;
>>>> -    env->nr_nodes = topo_info.nodes_per_pkg;
>>>>      env->pkg_offset = x86ms->apicid_pkg_offset(&topo_info);
>>>>  
>>>>      /*
>>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>>>> index e90c42d2fc..4efa1f8b87 100644
>>>> --- a/hw/i386/x86.c
>>>> +++ b/hw/i386/x86.c
>>>> @@ -62,7 +62,6 @@ inline void init_topo_info(X86CPUTopoInfo *topo_info,
>>>>  {
>>>>      MachineState *ms = MACHINE(x86ms);
>>>>  
>>>> -    topo_info->nodes_per_pkg = ms->numa_state->num_nodes / ms->smp.sockets;
>>>>      topo_info->dies_per_pkg = x86ms->smp_dies;
>>>>      topo_info->cores_per_die = ms->smp.cores;
>>>>      topo_info->threads_per_core = ms->smp.threads;
>>>> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
>>>> index 07239f95f4..05ddde7ba0 100644
>>>> --- a/include/hw/i386/topology.h
>>>> +++ b/include/hw/i386/topology.h
>>>> @@ -47,14 +47,12 @@ typedef uint32_t apic_id_t;
>>>>  
>>>>  typedef struct X86CPUTopoIDs {
>>>>      unsigned pkg_id;
>>>> -    unsigned node_id;
>>>>      unsigned die_id;
>>>>      unsigned core_id;
>>>>      unsigned smt_id;
>>>>  } X86CPUTopoIDs;
>>>>  
>>>>  typedef struct X86CPUTopoInfo {
>>>> -    unsigned nodes_per_pkg;
>>>>      unsigned dies_per_pkg;
>>>>      unsigned cores_per_die;
>>>>      unsigned threads_per_core;
>>>> @@ -89,11 +87,6 @@ static inline unsigned apicid_die_width(X86CPUTopoInfo *topo_info)
>>>>      return apicid_bitwidth_for_count(topo_info->dies_per_pkg);
>>>>  }
>>>>  
>>>> -/* Bit width of the node_id field per socket */
>>>> -static inline unsigned apicid_node_width_epyc(X86CPUTopoInfo *topo_info)
>>>> -{
>>>> -    return apicid_bitwidth_for_count(MAX(topo_info->nodes_per_pkg, 1));
>>>> -}
>>>>  /* Bit offset of the Core_ID field
>>>>   */
>>>>  static inline unsigned apicid_core_offset(X86CPUTopoInfo *topo_info)
>>>> @@ -114,30 +107,23 @@ static inline unsigned apicid_pkg_offset(X86CPUTopoInfo *topo_info)
>>>>      return apicid_die_offset(topo_info) + apicid_die_width(topo_info);
>>>>  }
>>>>  
>>>> -#define NODE_ID_OFFSET 3 /* Minimum node_id offset if numa configured */
>>>> +#define EPYC_DIE_OFFSET 3 /* Minimum die_id offset if numa configured */  
>>>                                                       ^^
>>>
>>> from EPYC's pov NUMA is always configured, there no 'if'
>>>
>>> but I have a question, is it possible to drop EPYC_DIE_OFFSET
>>> and reuse apicid_die_offset(), will it break something?  
>>
>> Yes. I am also thinking about it. We can go back to existing
>> apicid_die_width().
> 
> Given we are using die_id now, we probably can get rid of all
> topo hooks to generate apicid.

Sure. Will start working on it.  thanks

> 
>> Looking back again, I think all these code changes related to node_id is
>> causing more issues than fixes. We have added all these code to handle
>> some non- SPECed configurations like
>>  https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1728166&amp;data=02%7C01%7Cbabu.moger%40amd.com%7Cf7ad03f0c04d44dd104c08d84508b0b8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637335250919794679&amp;sdata=pOGISuQdHxFm5dEjybkGuDHfPCO1QqwE%2B%2FHR0fOAqnQ%3D&amp;reserved=0.
>>
>> Going forward it might create even more issues. Now, I think we should go
>> back and remove all these changes and just use the default decoding. Most
>> of the SPECed configuration would work just fine. With some non-SPECed
>> user inputs, it will create some sub-optimal configuration. If we can live
>> with that we will be just fine. I did not anticipate all these problems
>> when I originally started this work. Sorry, my bad.
> 
> Topology code is complex and sometimes it's easier to add a new thing,
> it's just that not every time it turns out as expected.
> We overlooked posiblilty to reuse die-id, which has lead to more
> complex and fragile outcome.
> 
> But it's fine, as long as it gets fixed in the end.

Sure. Thanks.

> 
> 
> 
>>> The reason for question is that, we (deviating from spec)
>>> do allow for unbound core/threads number so die_id, already could
>>> be shifted beyound ApicId[5:4], likewise we do allow for unbound
>>> sockets so ApicId[6] is also out of spec.
>>> If we are fine with ApicId being encoded in these cases out of
>>> spec, then why should we insist on DIE_OFFSET minumum at all?
>>> Why not just allow existing apicid_die_width() to encode die_id?
>>>
>>> In this case it will produce SPECed ApicId if user has provided
>>> -smp that matches currently released EPYC's and in all other cases
>>> it will be out of spec ApicId like when we set sockets/cores/trheads
>>> to out of spec values.
>>>     
>>>>  /*
>>>> - * Bit offset of the node_id field
>>>> - *
>>>> - * Make sure nodes_per_pkg >  0 if numa configured else zero.
>>>> + * Bit offset of the die_id field
>>>>   */
>>>> -static inline unsigned apicid_node_offset_epyc(X86CPUTopoInfo *topo_info)
>>>> +static inline unsigned apicid_die_offset_epyc(X86CPUTopoInfo *topo_info)
>>>>  {
>>>> -    unsigned offset = apicid_die_offset(topo_info) +
>>>> -                      apicid_die_width(topo_info);
>>>> +    unsigned offset = apicid_core_offset(topo_info) +
>>>> +                      apicid_core_width(topo_info);
>>>>  
>>>> -    if (topo_info->nodes_per_pkg) {
>>>> -        return MAX(NODE_ID_OFFSET, offset);
>>>> -    } else {
>>>> -        return offset;
>>>> -    }
>>>> +    return MAX(EPYC_DIE_OFFSET, offset);
>>>>  }
>>>>  
>>>>  /* Bit offset of the Pkg_ID (socket ID) field */
>>>>  static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info)
>>>>  {
>>>> -    return apicid_node_offset_epyc(topo_info) +
>>>> -           apicid_node_width_epyc(topo_info);
>>>> +    return apicid_die_offset_epyc(topo_info) + apicid_die_width(topo_info);
>>>>  }
>>>>  
>>>>  /*
>>>> @@ -150,8 +136,7 @@ x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
>>>>                                const X86CPUTopoIDs *topo_ids)
>>>>  {
>>>>      return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
>>>> -           (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) |
>>>> -           (topo_ids->die_id  << apicid_die_offset(topo_info)) |
>>>> +           (topo_ids->die_id  << apicid_die_offset_epyc(topo_info)) |
>>>>             (topo_ids->core_id << apicid_core_offset(topo_info)) |
>>>>             topo_ids->smt_id;
>>>>  }
>>>> @@ -160,15 +145,11 @@ static inline void x86_topo_ids_from_idx_epyc(X86CPUTopoInfo *topo_info,
>>>>                                                unsigned cpu_index,
>>>>                                                X86CPUTopoIDs *topo_ids)
>>>>  {
>>>> -    unsigned nr_nodes = MAX(topo_info->nodes_per_pkg, 1);
>>>>      unsigned nr_dies = topo_info->dies_per_pkg;
>>>>      unsigned nr_cores = topo_info->cores_per_die;
>>>>      unsigned nr_threads = topo_info->threads_per_core;
>>>> -    unsigned cores_per_node = DIV_ROUND_UP((nr_dies * nr_cores * nr_threads),
>>>> -                                            nr_nodes);
>>>>  
>>>>      topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads);
>>>> -    topo_ids->node_id = (cpu_index / cores_per_node) % nr_nodes;
>>>>      topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies;
>>>>      topo_ids->core_id = cpu_index / nr_threads % nr_cores;
>>>>      topo_ids->smt_id = cpu_index % nr_threads;
>>>> @@ -188,11 +169,8 @@ static inline void x86_topo_ids_from_apicid_epyc(apic_id_t apicid,
>>>>              (apicid >> apicid_core_offset(topo_info)) &
>>>>              ~(0xFFFFFFFFUL << apicid_core_width(topo_info));
>>>>      topo_ids->die_id =
>>>> -            (apicid >> apicid_die_offset(topo_info)) &
>>>> +            (apicid >> apicid_die_offset_epyc(topo_info)) &
>>>>              ~(0xFFFFFFFFUL << apicid_die_width(topo_info));
>>>> -    topo_ids->node_id =
>>>> -            (apicid >> apicid_node_offset_epyc(topo_info)) &
>>>> -            ~(0xFFFFFFFFUL << apicid_node_width_epyc(topo_info));
>>>>      topo_ids->pkg_id = apicid >> apicid_pkg_offset_epyc(topo_info);
>>>>  }
>>>>  
>>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>>> index c892432cae..ba0a24f6b8 100644
>>>> --- a/target/i386/cpu.c
>>>> +++ b/target/i386/cpu.c
>>>> @@ -345,7 +345,6 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>>>>                                         uint32_t *ecx, uint32_t *edx)
>>>>  {
>>>>      uint32_t l3_cores;
>>>> -    unsigned nodes = MAX(topo_info->nodes_per_pkg, 1);
>>>>  
>>>>      assert(cache->size == cache->line_size * cache->associativity *
>>>>                            cache->partitions * cache->sets);
>>>> @@ -355,10 +354,9 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>>>>  
>>>>      /* L3 is shared among multiple cores */
>>>>      if (cache->level == 3) {
>>>> -        l3_cores = DIV_ROUND_UP((topo_info->dies_per_pkg *
>>>> -                                 topo_info->cores_per_die *
>>>> +        l3_cores = DIV_ROUND_UP((topo_info->cores_per_die *
>>>>                                   topo_info->threads_per_core),
>>>> -                                 nodes);
>>>> +                                 topo_info->dies_per_pkg);
>>>>          *eax |= (l3_cores - 1) << 14;
>>>>      } else {
>>>>          *eax |= ((topo_info->threads_per_core - 1) << 14);
>>>> @@ -387,7 +385,6 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>>>>                                         uint32_t *ecx, uint32_t *edx)
>>>>  {
>>>>      X86CPUTopoIDs topo_ids = {0};
>>>> -    unsigned long nodes = MAX(topo_info->nodes_per_pkg, 1);
>>>>  
>>>>      x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
>>>>  
>>>> @@ -433,7 +430,7 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>>>>       * NodeId is combination of node and socket_id which is already decoded
>>>>       * in apic_id. Just use it by shifting.
>>>>       */
>>>> -    *ecx = ((nodes - 1) << 8) | cpu->node_id;
>>>> +    *ecx = ((topo_info->dies_per_pkg - 1) << 8) | cpu->node_id;
>>>>      *edx = 0;
>>>>  }
>>>>  
>>>> @@ -5484,7 +5481,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>>>      uint32_t signature[3];
>>>>      X86CPUTopoInfo topo_info;
>>>>  
>>>> -    topo_info.nodes_per_pkg = env->nr_nodes;
>>>>      topo_info.dies_per_pkg = env->nr_dies;
>>>>      topo_info.cores_per_die = cs->nr_cores;
>>>>      topo_info.threads_per_core = cs->nr_threads;
>>>> @@ -6944,7 +6940,6 @@ static void x86_cpu_initfn(Object *obj)
>>>>      FeatureWord w;
>>>>  
>>>>      env->nr_dies = 1;
>>>> -    env->nr_nodes = 1;
>>>>      cpu_set_cpustate_pointers(cpu);
>>>>  
>>>>      object_property_add(obj, "family", "int",
>>>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>>>> index e1a5c174dc..4c89bee8d1 100644
>>>> --- a/target/i386/cpu.h
>>>> +++ b/target/i386/cpu.h
>>>> @@ -1629,7 +1629,6 @@ typedef struct CPUX86State {
>>>>      TPRAccess tpr_access_type;
>>>>  
>>>>      unsigned nr_dies;
>>>> -    unsigned nr_nodes;
>>>>      unsigned pkg_offset;
>>>>  } CPUX86State;
>>>>  
>>>>
>>>>  
>>>   
>>
> 

Re: [PATCH v4 0/3] Modify AMD topology to use socket/dies/core/thread model

[no-reply]

Patchew URL: https://patchew.org/QEMU/159744083536.39197.13827776633866601278.stgit@naples-babu.amd.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      tests/test-timed-average.o
  CC      tests/test-util-filemonitor.o
/tmp/qemu-test/src/tests/test-x86-cpuid.c: In function 'test_topo_bits':
/tmp/qemu-test/src/tests/test-x86-cpuid.c:34:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 1, 1};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:34:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:39:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 1, 1};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:39:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:48:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 1, 2};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:48:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:50:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 1, 3};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:50:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:52:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 1, 4};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:52:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:55:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 1, 14};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:55:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:57:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 1, 15};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:57:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:59:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 1, 16};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:59:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:61:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 1, 17};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:61:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:65:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 30, 2};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:65:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:67:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 31, 2};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:67:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:69:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 32, 2};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:69:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:71:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 33, 2};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:71:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:74:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 30, 2};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:74:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:76:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 2, 30, 2};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:76:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:78:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 3, 30, 2};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:78:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:80:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 4, 30, 2};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:80:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:88:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:88:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:94:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:94:5: error: (near initialization for '(anonymous)') [-Werror]
/tmp/qemu-test/src/tests/test-x86-cpuid.c:99:5: error: excess elements in struct initializer [-Werror]
     topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
     ^
/tmp/qemu-test/src/tests/test-x86-cpuid.c:99:5: error: (near initialization for '(anonymous)') [-Werror]
cc1: all warnings being treated as errors
  CC      tests/test-util-sockets.o
make: *** [tests/test-x86-cpuid.o] Error 1
make: *** Waiting for unfinished jobs....
  CC      tests/test-authz-simple.o
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=f1d81aa90662430dbfbbafedb6be792d', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-cccrxg3c/src/docker-src.2020-08-15-13.09.03.12551:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=f1d81aa90662430dbfbbafedb6be792d
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-cccrxg3c/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    3m9.661s
user    0m8.272s


The full log is available at
http://patchew.org/logs/159744083536.39197.13827776633866601278.stgit@naples-babu.amd.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com