[PATCH v6 00/10] Remove EPYC mode apicid decode and use generic decode

[PATCH v6 00/10] Remove EPYC mode apicid decode and use generic decode

[Babu Moger]

To support some of the complex topology, we introduced EPYC mode apicid decode.
But, EPYC mode decode is running into problems. Also it can become quite a
maintenance problem in the future. So, it was decided to remove that code and
use the generic decode which works for majority of the topology. Most of the
SPECed configuration would work just fine. With some non-SPECed user inputs,
it will create some sub-optimal configuration.

Here is the discussion thread.
https://lore.kernel.org/qemu-devel/c0bcc1a6-1d84-a6e7-e468-d5b437c1b254@amd.com/
https://lore.kernel.org/qemu-devel/20200826143849.59f6970b@redhat.com/

This series removes all the EPYC mode specific apicid changes and use the generic
apicid decode.
--
v6:
 Found out that numa configuration is not mandatory for all the EPYC model topology.
 We can use the generic decode which works pretty well. Also noticed that
 cpuid does not changes when the numa nodes change(NPS- Nodes per socket).
 Took care of couple comments from Igor and Eduardo.
 Thank you Igor, Daniel, David, Eduardo for your feedback.  

v5:
 https://lore.kernel.org/qemu-devel/159804762216.39954.15502128500494116468.stgit@naples-babu.amd.com/
 Revert EPYC specific decode.
 Simplify CPUID_8000_001E

v4:
  https://lore.kernel.org/qemu-devel/159744083536.39197.13827776633866601278.stgit@naples-babu.amd.com/
  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 (10):
      Revert "i386: Fix pkg_id offset for EPYC cpu models"
      Revert "target/i386: Enable new apic id encoding for EPYC based cpus models"
      Revert "hw/i386: Move arch_id decode inside x86_cpus_init"
      Revert "i386: Introduce use_epyc_apic_id_encoding in X86CPUDefinition"
      Revert "hw/i386: Introduce apicid functions inside X86MachineState"
      Revert "target/i386: Cleanup and use the EPYC mode topology functions"
      Revert "hw/386: Add EPYC mode topology decoding functions"
      Revert "hw/i386: Update structures to save the number of nodes per package"
      i386: Simplify CPUID_8000_001E for AMD
      i386: Simplify CPUID_8000_001E for AMD


 hw/i386/pc.c               |    8 +--
 hw/i386/x86.c              |   43 ++------------
 include/hw/i386/topology.h |  101 ----------------------------------
 include/hw/i386/x86.h      |    9 ---
 target/i386/cpu.c          |  131 +++++++++++++++++---------------------------
 target/i386/cpu.h          |    3 -
 tests/test-x86-cpuid.c     |   40 +++++++------
 7 files changed, 81 insertions(+), 254 deletions(-)

--

[PATCH v6 01/10] Revert "i386: Fix pkg_id offset for EPYC cpu models"

[Babu Moger]

This reverts commit 7b225762c8c05fd31d4c2be116aedfbc00383f8b.

Remove the EPYC specific apicid decoding and use the generic
default decoding.

Also fix all the references of pkg_offset.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 hw/i386/pc.c      |    1 -
 target/i386/cpu.c |    9 ++++-----
 target/i386/cpu.h |    1 -
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5d8d5ef8b3..6b708f4341 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1502,7 +1502,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 
     env->nr_dies = x86ms->smp_dies;
     env->nr_nodes = topo_info.nodes_per_pkg;
-    env->pkg_offset = x86ms->apicid_pkg_offset(&topo_info);
 
     /*
      * If APIC ID is not set,
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 588f32e136..173e6f4a07 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5678,7 +5678,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *ecx |= CPUID_TOPOLOGY_LEVEL_SMT;
             break;
         case 1:
-            *eax = env->pkg_offset;
+            *eax = apicid_pkg_offset(&topo_info);
             *ebx = cs->nr_cores * cs->nr_threads;
             *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
             break;
@@ -5712,7 +5712,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
             break;
         case 2:
-            *eax = env->pkg_offset;
+            *eax = apicid_pkg_offset(&topo_info);
             *ebx = env->nr_dies * cs->nr_cores * cs->nr_threads;
             *ecx |= CPUID_TOPOLOGY_LEVEL_DIE;
             break;
@@ -5889,11 +5889,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             /*
              * Bits 15:12 is "The number of bits in the initial
              * Core::X86::Apic::ApicId[ApicId] value that indicate
-             * thread ID within a package". This is already stored at
-             * CPUX86State::pkg_offset.
+             * thread ID within a package".
              * Bits 7:0 is "The number of threads in the package is NC+1"
              */
-            *ecx = (env->pkg_offset << 12) |
+            *ecx = (apicid_pkg_offset(&topo_info) << 12) |
                    ((cs->nr_cores * cs->nr_threads) - 1);
         } else {
             *ecx = 0;
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index e1a5c174dc..d5ad42d694 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1630,7 +1630,6 @@ typedef struct CPUX86State {
 
     unsigned nr_dies;
     unsigned nr_nodes;
-    unsigned pkg_offset;
 } CPUX86State;
 
 struct kvm_msrs;

[PATCH v6 02/10] Revert "target/i386: Enable new apic id encoding for EPYC based cpus models"

[Babu Moger]

This reverts commit 247b18c593ec298446645af8d5d28911daf653b1.

Remove the EPYC specific apicid decoding and use the generic
default decoding.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 173e6f4a07..c9c1e681c2 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3995,7 +3995,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .xlevel = 0x8000001E,
         .model_id = "AMD EPYC Processor",
         .cache_info = &epyc_cache_info,
-        .use_epyc_apic_id_encoding = 1,
         .versions = (X86CPUVersionDefinition[]) {
             { .version = 1 },
             {
@@ -4123,7 +4122,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .xlevel = 0x8000001E,
         .model_id = "AMD EPYC-Rome Processor",
         .cache_info = &epyc_rome_cache_info,
-        .use_epyc_apic_id_encoding = 1,
     },
 };
 

[PATCH v6 03/10] Revert "hw/i386: Move arch_id decode inside x86_cpus_init"

[Babu Moger]

This reverts commit 2e26f4ab3bf8390a2677d3afd9b1a04f015d7721.

Remove the EPYC specific apicid decoding and use the generic
default decoding.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 hw/i386/pc.c  |    6 +++---
 hw/i386/x86.c |   37 +++++++------------------------------
 2 files changed, 10 insertions(+), 33 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6b708f4341..0677d6a272 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1556,14 +1556,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
         topo_ids.die_id = cpu->die_id;
         topo_ids.core_id = cpu->core_id;
         topo_ids.smt_id = cpu->thread_id;
-        cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids);
+        cpu->apic_id = x86_apicid_from_topo_ids(&topo_info, &topo_ids);
     }
 
     cpu_slot = pc_find_cpu_slot(MACHINE(pcms), cpu->apic_id, &idx);
     if (!cpu_slot) {
         MachineState *ms = MACHINE(pcms);
 
-        x86ms->topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
+        x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
         error_setg(errp,
             "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
             " APIC ID %" PRIu32 ", valid index range 0:%d",
@@ -1584,7 +1584,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
     /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn()
      * once -smp refactoring is complete and there will be CPU private
      * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */
-    x86ms->topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
+    x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
     if (cpu->socket_id != -1 && cpu->socket_id != topo_ids.pkg_id) {
         error_setg(errp, "property socket-id: %u doesn't match set apic-id:"
             " 0x%x (socket-id: %u)", cpu->socket_id, cpu->apic_id,
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index cf384b9743..3cc2318212 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -68,22 +68,6 @@ inline void init_topo_info(X86CPUTopoInfo *topo_info,
     topo_info->threads_per_core = ms->smp.threads;
 }
 
-/*
- * Set up with the new EPYC topology handlers
- *
- * AMD uses different apic id encoding for EPYC based cpus. Override
- * the default topo handlers with EPYC encoding handlers.
- */
-static void x86_set_epyc_topo_handlers(MachineState *machine)
-{
-    X86MachineState *x86ms = X86_MACHINE(machine);
-
-    x86ms->apicid_from_cpu_idx = x86_apicid_from_cpu_idx_epyc;
-    x86ms->topo_ids_from_apicid = x86_topo_ids_from_apicid_epyc;
-    x86ms->apicid_from_topo_ids = x86_apicid_from_topo_ids_epyc;
-    x86ms->apicid_pkg_offset = apicid_pkg_offset_epyc;
-}
-
 /*
  * Calculates initial APIC ID for a specific CPU index
  *
@@ -102,7 +86,7 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
 
     init_topo_info(&topo_info, x86ms);
 
-    correct_id = x86ms->apicid_from_cpu_idx(&topo_info, cpu_index);
+    correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index);
     if (x86mc->compat_apic_id_mode) {
         if (cpu_index != correct_id && !warned && !qtest_enabled()) {
             error_report("APIC IDs set in compatibility mode, "
@@ -136,11 +120,6 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
     MachineState *ms = MACHINE(x86ms);
     MachineClass *mc = MACHINE_GET_CLASS(x86ms);
 
-    /* Check for apicid encoding */
-    if (cpu_x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
-        x86_set_epyc_topo_handlers(ms);
-    }
-
     x86_cpu_set_default_version(default_cpu_version);
 
     /*
@@ -154,12 +133,6 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
     x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
                                                       ms->smp.max_cpus - 1) + 1;
     possible_cpus = mc->possible_cpu_arch_ids(ms);
-
-    for (i = 0; i < ms->possible_cpus->len; i++) {
-        ms->possible_cpus->cpus[i].arch_id =
-            x86_cpu_apic_id_from_index(x86ms, i);
-    }
-
     for (i = 0; i < ms->smp.cpus; i++) {
         x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal);
     }
@@ -184,7 +157,8 @@ int64_t x86_get_default_cpu_node_id(const MachineState *ms, int idx)
    init_topo_info(&topo_info, x86ms);
 
    assert(idx < ms->possible_cpus->len);
-   x86_topo_ids_from_idx(&topo_info, idx, &topo_ids);
+   x86_topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id,
+                            &topo_info, &topo_ids);
    return topo_ids.pkg_id % ms->numa_state->num_nodes;
 }
 
@@ -215,7 +189,10 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
 
         ms->possible_cpus->cpus[i].type = ms->cpu_type;
         ms->possible_cpus->cpus[i].vcpus_count = 1;
-        x86_topo_ids_from_idx(&topo_info, i, &topo_ids);
+        ms->possible_cpus->cpus[i].arch_id =
+            x86_cpu_apic_id_from_index(x86ms, i);
+        x86_topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id,
+                                 &topo_info, &topo_ids);
         ms->possible_cpus->cpus[i].props.has_socket_id = true;
         ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id;
         if (x86ms->smp_dies > 1) {

[PATCH v6 04/10] Revert "i386: Introduce use_epyc_apic_id_encoding in X86CPUDefinition"

[Babu Moger]

This reverts commit 0c1538cb1a26287c072645f4759b9872b1596d79.

Remove the EPYC specific apicid decoding and use the generic
default decoding.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c |   16 ----------------
 target/i386/cpu.h |    1 -
 2 files changed, 17 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c9c1e681c2..b72b4b08ac 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1638,10 +1638,6 @@ typedef struct X86CPUDefinition {
     FeatureWordArray features;
     const char *model_id;
     CPUCaches *cache_info;
-
-    /* Use AMD EPYC encoding for apic id */
-    bool use_epyc_apic_id_encoding;
-
     /*
      * Definitions for alternative versions of CPU model.
      * List is terminated by item with version == 0.
@@ -1683,18 +1679,6 @@ static const X86CPUVersionDefinition *x86_cpu_def_get_versions(X86CPUDefinition
     return def->versions ?: default_version_list;
 }
 
-bool cpu_x86_use_epyc_apic_id_encoding(const char *cpu_type)
-{
-    X86CPUClass *xcc = X86_CPU_CLASS(object_class_by_name(cpu_type));
-
-    assert(xcc);
-    if (xcc->model && xcc->model->cpudef) {
-        return xcc->model->cpudef->use_epyc_apic_id_encoding;
-    } else {
-        return false;
-    }
-}
-
 static CPUCaches epyc_cache_info = {
     .l1d_cache = &(CPUCacheInfo) {
         .type = DATA_CACHE,
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index d5ad42d694..5ff8ad8427 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1918,7 +1918,6 @@ void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
 void host_vendor_fms(char *vendor, int *family, int *model, int *stepping);
-bool cpu_x86_use_epyc_apic_id_encoding(const char *cpu_type);
 
 /* helper.c */
 bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,

[PATCH v6 05/10] Revert "hw/i386: Introduce apicid functions inside X86MachineState"

[Babu Moger]

This reverts commit 6121c7fbfd98dbc3af1b00b56ff2eef66df87828.

Remove the EPYC specific apicid decoding and use the generic
default decoding.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 hw/i386/x86.c         |    5 -----
 include/hw/i386/x86.h |    9 ---------
 2 files changed, 14 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 3cc2318212..727c4a0f1d 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -896,11 +896,6 @@ static void x86_machine_initfn(Object *obj)
     x86ms->smm = ON_OFF_AUTO_AUTO;
     x86ms->acpi = ON_OFF_AUTO_AUTO;
     x86ms->smp_dies = 1;
-
-    x86ms->apicid_from_cpu_idx = x86_apicid_from_cpu_idx;
-    x86ms->topo_ids_from_apicid = x86_topo_ids_from_apicid;
-    x86ms->apicid_from_topo_ids = x86_apicid_from_topo_ids;
-    x86ms->apicid_pkg_offset = apicid_pkg_offset;
 }
 
 static void x86_machine_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index b79f24e285..4d9a26326d 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -63,15 +63,6 @@ typedef struct {
     OnOffAuto smm;
     OnOffAuto acpi;
 
-    /* Apic id specific handlers */
-    uint32_t (*apicid_from_cpu_idx)(X86CPUTopoInfo *topo_info,
-                                    unsigned cpu_index);
-    void (*topo_ids_from_apicid)(apic_id_t apicid, X86CPUTopoInfo *topo_info,
-                                 X86CPUTopoIDs *topo_ids);
-    apic_id_t (*apicid_from_topo_ids)(X86CPUTopoInfo *topo_info,
-                                      const X86CPUTopoIDs *topo_ids);
-    uint32_t (*apicid_pkg_offset)(X86CPUTopoInfo *topo_info);
-
     /*
      * Address space used by IOAPIC device. All IOAPIC interrupts
      * will be translated to MSI messages in the address space.

[PATCH v6 06/10] Revert "target/i386: Cleanup and use the EPYC mode topology functions"

[Babu Moger]

This reverts commit dd08ef0318e2b61d14bc069590d174913f7f437a.

Remove the EPYC specific apicid decoding and use the generic
default decoding.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c |  161 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 127 insertions(+), 34 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b72b4b08ac..256bfa669f 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -338,15 +338,68 @@ static void encode_cache_cpuid80000006(CPUCacheInfo *l2,
     }
 }
 
+/*
+ * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
+ * Please refer to the AMD64 Architecture Programmer’s Manual Volume 3.
+ * Define the constants to build the cpu topology. Right now, TOPOEXT
+ * feature is enabled only on EPYC. So, these constants are based on
+ * EPYC supported configurations. We may need to handle the cases if
+ * these values change in future.
+ */
+/* Maximum core complexes in a node */
+#define MAX_CCX 2
+/* Maximum cores in a core complex */
+#define MAX_CORES_IN_CCX 4
+/* Maximum cores in a node */
+#define MAX_CORES_IN_NODE 8
+/* Maximum nodes in a socket */
+#define MAX_NODES_PER_SOCKET 4
+
+/*
+ * Figure out the number of nodes required to build this config.
+ * Max cores in a node is 8
+ */
+static int nodes_in_socket(int nr_cores)
+{
+    int nodes;
+
+    nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
+
+   /* Hardware does not support config with 3 nodes, return 4 in that case */
+    return (nodes == 3) ? 4 : nodes;
+}
+
+/*
+ * Decide the number of cores in a core complex with the given nr_cores using
+ * following set constants MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE and
+ * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
+ * L3 cache is shared across all cores in a core complex. So, this will also
+ * tell us how many cores are sharing the L3 cache.
+ */
+static int cores_in_core_complex(int nr_cores)
+{
+    int nodes;
+
+    /* Check if we can fit all the cores in one core complex */
+    if (nr_cores <= MAX_CORES_IN_CCX) {
+        return nr_cores;
+    }
+    /* Get the number of nodes required to build this config */
+    nodes = nodes_in_socket(nr_cores);
+
+    /*
+     * Divide the cores accros all the core complexes
+     * Return rounded up value
+     */
+    return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
+}
+
 /* Encode cache info for CPUID[8000001D] */
-static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
-                                       X86CPUTopoInfo *topo_info,
-                                       uint32_t *eax, uint32_t *ebx,
-                                       uint32_t *ecx, uint32_t *edx)
+static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
+                                uint32_t *eax, uint32_t *ebx,
+                                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,13 +408,10 @@ 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 *
-                                 topo_info->threads_per_core),
-                                 nodes);
-        *eax |= (l3_cores - 1) << 14;
+        l3_cores = cores_in_core_complex(cs->nr_cores);
+        *eax |= ((l3_cores * cs->nr_threads) - 1) << 14;
     } else {
-        *eax |= ((topo_info->threads_per_core - 1) << 14);
+        *eax |= ((cs->nr_threads - 1) << 14);
     }
 
     assert(cache->line_size > 0);
@@ -381,17 +431,55 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
            (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
 }
 
+/* Data structure to hold the configuration info for a given core index */
+struct core_topology {
+    /* core complex id of the current core index */
+    int ccx_id;
+    /*
+     * Adjusted core index for this core in the topology
+     * This can be 0,1,2,3 with max 4 cores in a core complex
+     */
+    int core_id;
+    /* Node id for this core index */
+    int node_id;
+    /* Number of nodes in this config */
+    int num_nodes;
+};
+
+/*
+ * Build the configuration closely match the EPYC hardware. Using the EPYC
+ * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE)
+ * right now. This could change in future.
+ * nr_cores : Total number of cores in the config
+ * core_id  : Core index of the current CPU
+ * topo     : Data structure to hold all the config info for this core index
+ */
+static void build_core_topology(int nr_cores, int core_id,
+                                struct core_topology *topo)
+{
+    int nodes, cores_in_ccx;
+
+    /* First get the number of nodes required */
+    nodes = nodes_in_socket(nr_cores);
+
+    cores_in_ccx = cores_in_core_complex(nr_cores);
+
+    topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
+    topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
+    topo->core_id = core_id % cores_in_ccx;
+    topo->num_nodes = nodes;
+}
+
 /* Encode cache info for CPUID[8000001E] */
-static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
+static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
                                        uint32_t *eax, uint32_t *ebx,
                                        uint32_t *ecx, uint32_t *edx)
 {
-    X86CPUTopoIDs topo_ids = {0};
-    unsigned long nodes = MAX(topo_info->nodes_per_pkg, 1);
+    struct core_topology topo = {0};
+    unsigned long nodes;
     int shift;
 
-    x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
-
+    build_core_topology(cs->nr_cores, cpu->core_id, &topo);
     *eax = cpu->apic_id;
     /*
      * CPUID_Fn8000001E_EBX
@@ -408,8 +496,12 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
      *             3 Core complex id
      *           1:0 Core id
      */
-    *ebx = ((topo_info->threads_per_core - 1) << 8) | (topo_ids.node_id << 3) |
-            (topo_ids.core_id);
+    if (cs->nr_threads - 1) {
+        *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
+                (topo.ccx_id << 2) | topo.core_id;
+    } else {
+        *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
+    }
     /*
      * CPUID_Fn8000001E_ECX
      * 31:11 Reserved
@@ -418,8 +510,9 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
      *         2  Socket id
      *       1:0  Node id
      */
-    if (nodes <= 4) {
-        *ecx = ((nodes - 1) << 8) | (topo_ids.pkg_id << 2) | topo_ids.node_id;
+    if (topo.num_nodes <= 4) {
+        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << 2) |
+                topo.node_id;
     } else {
         /*
          * Node id fix up. Actual hardware supports up to 4 nodes. But with
@@ -434,10 +527,10 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
          * 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;
+        nodes = topo.num_nodes - 1;
         shift = find_last_bit(&nodes, 8);
-        *ecx = (nodes << 8) | (topo_ids.pkg_id << (shift + 1)) |
-               topo_ids.node_id;
+        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << (shift + 1)) |
+                topo.node_id;
     }
     *edx = 0;
 }
@@ -5471,7 +5564,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;
@@ -5902,20 +5994,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         }
         switch (count) {
         case 0: /* L1 dcache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache,
-                                       &topo_info, eax, ebx, ecx, edx);
+            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
+                                       eax, ebx, ecx, edx);
             break;
         case 1: /* L1 icache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache,
-                                       &topo_info, eax, ebx, ecx, edx);
+            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
+                                       eax, ebx, ecx, edx);
             break;
         case 2: /* L2 cache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache,
-                                       &topo_info, eax, ebx, ecx, edx);
+            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
+                                       eax, ebx, ecx, edx);
             break;
         case 3: /* L3 cache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache,
-                                       &topo_info, eax, ebx, ecx, edx);
+            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
+                                       eax, ebx, ecx, edx);
             break;
         default: /* end of info */
             *eax = *ebx = *ecx = *edx = 0;
@@ -5924,7 +6016,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         break;
     case 0x8000001E:
         assert(cpu->core_id <= 255);
-        encode_topo_cpuid8000001e(&topo_info, cpu, eax, ebx, ecx, edx);
+        encode_topo_cpuid8000001e(cs, cpu,
+                                  eax, ebx, ecx, edx);
         break;
     case 0xC0000000:
         *eax = env->cpuid_xlevel2;

[PATCH v6 07/10] Revert "hw/386: Add EPYC mode topology decoding functions"

[Babu Moger]

This reverts commit 7568b205555a6405042f62c64af3268f4330aed5.

Remove the EPYC specific apicid decoding and use the generic
default decoding.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 include/hw/i386/topology.h |  100 --------------------------------------------
 1 file changed, 100 deletions(-)

diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index 07239f95f4..b9593b9905 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -47,7 +47,6 @@ typedef uint32_t apic_id_t;
 
 typedef struct X86CPUTopoIDs {
     unsigned pkg_id;
-    unsigned node_id;
     unsigned die_id;
     unsigned core_id;
     unsigned smt_id;
@@ -89,11 +88,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,100 +108,6 @@ 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 */
-
-/*
- * Bit offset of the node_id field
- *
- * Make sure nodes_per_pkg >  0 if numa configured else zero.
- */
-static inline unsigned apicid_node_offset_epyc(X86CPUTopoInfo *topo_info)
-{
-    unsigned offset = apicid_die_offset(topo_info) +
-                      apicid_die_width(topo_info);
-
-    if (topo_info->nodes_per_pkg) {
-        return MAX(NODE_ID_OFFSET, offset);
-    } else {
-        return 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);
-}
-
-/*
- * Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
- *
- * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
- */
-static inline apic_id_t
-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->core_id << apicid_core_offset(topo_info)) |
-           topo_ids->smt_id;
-}
-
-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;
-}
-
-/*
- * Calculate thread/core/package IDs for a specific topology,
- * based on APIC ID
- */
-static inline void x86_topo_ids_from_apicid_epyc(apic_id_t apicid,
-                                            X86CPUTopoInfo *topo_info,
-                                            X86CPUTopoIDs *topo_ids)
-{
-    topo_ids->smt_id = apicid &
-            ~(0xFFFFFFFFUL << apicid_smt_width(topo_info));
-    topo_ids->core_id =
-            (apicid >> apicid_core_offset(topo_info)) &
-            ~(0xFFFFFFFFUL << apicid_core_width(topo_info));
-    topo_ids->die_id =
-            (apicid >> apicid_die_offset(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);
-}
-
-/*
- * Make APIC ID for the CPU 'cpu_index'
- *
- * 'cpu_index' is a sequential, contiguous ID for the CPU.
- */
-static inline apic_id_t x86_apicid_from_cpu_idx_epyc(X86CPUTopoInfo *topo_info,
-                                                     unsigned cpu_index)
-{
-    X86CPUTopoIDs topo_ids;
-    x86_topo_ids_from_idx_epyc(topo_info, cpu_index, &topo_ids);
-    return x86_apicid_from_topo_ids_epyc(topo_info, &topo_ids);
-}
 /* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
  *
  * The caller must make sure core_id < nr_cores and smt_id < nr_threads.

[PATCH v6 08/10] Revert "hw/i386: Update structures to save the number of nodes per package"

[Babu Moger]

This reverts commit c24a41bb53c0854d22c96b30d57cfcaa543c409d.

Remove the EPYC specific apicid decoding and use the generic
default decoding.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 hw/i386/pc.c               |    1 -
 hw/i386/x86.c              |    1 -
 include/hw/i386/topology.h |    1 -
 target/i386/cpu.c          |    1 -
 target/i386/cpu.h          |    1 -
 tests/test-x86-cpuid.c     |   40 ++++++++++++++++++++--------------------
 6 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0677d6a272..d11daacc23 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1501,7 +1501,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;
 
     /*
      * If APIC ID is not set,
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 727c4a0f1d..c1954db152 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 b9593b9905..81573f6cfd 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -53,7 +53,6 @@ typedef struct X86CPUTopoIDs {
 } X86CPUTopoIDs;
 
 typedef struct X86CPUTopoInfo {
-    unsigned nodes_per_pkg;
     unsigned dies_per_pkg;
     unsigned cores_per_die;
     unsigned threads_per_core;
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 256bfa669f..ba4667b33c 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7023,7 +7023,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 5ff8ad8427..d3097be6a5 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;
 } CPUX86State;
 
 struct kvm_msrs;
diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
index 049030a50e..bfabc0403a 100644
--- a/tests/test-x86-cpuid.c
+++ b/tests/test-x86-cpuid.c
@@ -31,12 +31,12 @@ static void test_topo_bits(void)
     X86CPUTopoInfo topo_info = {0};
 
     /* simple tests for 1 thread per core, 1 core per die, 1 die per package */
-    topo_info = (X86CPUTopoInfo) {0, 1, 1, 1};
+    topo_info = (X86CPUTopoInfo) {1, 1, 1};
     g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 0);
     g_assert_cmpuint(apicid_core_width(&topo_info), ==, 0);
     g_assert_cmpuint(apicid_die_width(&topo_info), ==, 0);
 
-    topo_info = (X86CPUTopoInfo) {0, 1, 1, 1};
+    topo_info = (X86CPUTopoInfo) {1, 1, 1};
     g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 0), ==, 0);
     g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1), ==, 1);
     g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2), ==, 2);
@@ -45,39 +45,39 @@ static void test_topo_bits(void)
 
     /* Test field width calculation for multiple values
      */
-    topo_info = (X86CPUTopoInfo) {0, 1, 1, 2};
+    topo_info = (X86CPUTopoInfo) {1, 1, 2};
     g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 1);
-    topo_info = (X86CPUTopoInfo) {0, 1, 1, 3};
+    topo_info = (X86CPUTopoInfo) {1, 1, 3};
     g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 2);
-    topo_info = (X86CPUTopoInfo) {0, 1, 1, 4};
+    topo_info = (X86CPUTopoInfo) {1, 1, 4};
     g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 2);
 
-    topo_info = (X86CPUTopoInfo) {0, 1, 1, 14};
+    topo_info = (X86CPUTopoInfo) {1, 1, 14};
     g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 4);
-    topo_info = (X86CPUTopoInfo) {0, 1, 1, 15};
+    topo_info = (X86CPUTopoInfo) {1, 1, 15};
     g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 4);
-    topo_info = (X86CPUTopoInfo) {0, 1, 1, 16};
+    topo_info = (X86CPUTopoInfo) {1, 1, 16};
     g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 4);
-    topo_info = (X86CPUTopoInfo) {0, 1, 1, 17};
+    topo_info = (X86CPUTopoInfo) {1, 1, 17};
     g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 5);
 
 
-    topo_info = (X86CPUTopoInfo) {0, 1, 30, 2};
+    topo_info = (X86CPUTopoInfo) {1, 30, 2};
     g_assert_cmpuint(apicid_core_width(&topo_info), ==, 5);
-    topo_info = (X86CPUTopoInfo) {0, 1, 31, 2};
+    topo_info = (X86CPUTopoInfo) {1, 31, 2};
     g_assert_cmpuint(apicid_core_width(&topo_info), ==, 5);
-    topo_info = (X86CPUTopoInfo) {0, 1, 32, 2};
+    topo_info = (X86CPUTopoInfo) {1, 32, 2};
     g_assert_cmpuint(apicid_core_width(&topo_info), ==, 5);
-    topo_info = (X86CPUTopoInfo) {0, 1, 33, 2};
+    topo_info = (X86CPUTopoInfo) {1, 33, 2};
     g_assert_cmpuint(apicid_core_width(&topo_info), ==, 6);
 
-    topo_info = (X86CPUTopoInfo) {0, 1, 30, 2};
+    topo_info = (X86CPUTopoInfo) {1, 30, 2};
     g_assert_cmpuint(apicid_die_width(&topo_info), ==, 0);
-    topo_info = (X86CPUTopoInfo) {0, 2, 30, 2};
+    topo_info = (X86CPUTopoInfo) {2, 30, 2};
     g_assert_cmpuint(apicid_die_width(&topo_info), ==, 1);
-    topo_info = (X86CPUTopoInfo) {0, 3, 30, 2};
+    topo_info = (X86CPUTopoInfo) {3, 30, 2};
     g_assert_cmpuint(apicid_die_width(&topo_info), ==, 2);
-    topo_info = (X86CPUTopoInfo) {0, 4, 30, 2};
+    topo_info = (X86CPUTopoInfo) {4, 30, 2};
     g_assert_cmpuint(apicid_die_width(&topo_info), ==, 2);
 
     /* build a weird topology and see if IDs are calculated correctly
@@ -85,18 +85,18 @@ static void test_topo_bits(void)
 
     /* This will use 2 bits for thread ID and 3 bits for core ID
      */
-    topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
+    topo_info = (X86CPUTopoInfo) {1, 6, 3};
     g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 2);
     g_assert_cmpuint(apicid_core_offset(&topo_info), ==, 2);
     g_assert_cmpuint(apicid_die_offset(&topo_info), ==, 5);
     g_assert_cmpuint(apicid_pkg_offset(&topo_info), ==, 5);
 
-    topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
+    topo_info = (X86CPUTopoInfo) {1, 6, 3};
     g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 0), ==, 0);
     g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1), ==, 1);
     g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2), ==, 2);
 
-    topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
+    topo_info = (X86CPUTopoInfo) {1, 6, 3};
     g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1 * 3 + 0), ==,
                      (1 << 2) | 0);
     g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1 * 3 + 1), ==,

[PATCH v6 09/10] i386: Simplify CPUID_8000_001E for AMD

[Babu Moger]

Remove all the hardcoded values and replace with generalized
fields.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c |   31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ba4667b33c..d434c8545a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -395,9 +395,10 @@ static int cores_in_core_complex(int nr_cores)
 }
 
 /* Encode cache info for CPUID[8000001D] */
-static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
-                                uint32_t *eax, uint32_t *ebx,
-                                uint32_t *ecx, uint32_t *edx)
+static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
+                                       X86CPUTopoInfo *topo_info,
+                                       uint32_t *eax, uint32_t *ebx,
+                                       uint32_t *ecx, uint32_t *edx)
 {
     uint32_t l3_cores;
     assert(cache->size == cache->line_size * cache->associativity *
@@ -408,10 +409,12 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
 
     /* L3 is shared among multiple cores */
     if (cache->level == 3) {
-        l3_cores = cores_in_core_complex(cs->nr_cores);
-        *eax |= ((l3_cores * cs->nr_threads) - 1) << 14;
+        l3_cores = DIV_ROUND_UP((topo_info->cores_per_die *
+                                 topo_info->threads_per_core),
+                                 topo_info->dies_per_pkg);
+        *eax |= (l3_cores - 1) << 14;
     } else {
-        *eax |= ((cs->nr_threads - 1) << 14);
+        *eax |= ((topo_info->threads_per_core - 1) << 14);
     }
 
     assert(cache->line_size > 0);
@@ -5994,20 +5997,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         }
         switch (count) {
         case 0: /* L1 dcache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
-                                       eax, ebx, ecx, edx);
+            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache,
+                                       &topo_info, eax, ebx, ecx, edx);
             break;
         case 1: /* L1 icache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
-                                       eax, ebx, ecx, edx);
+            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache,
+                                       &topo_info, eax, ebx, ecx, edx);
             break;
         case 2: /* L2 cache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
-                                       eax, ebx, ecx, edx);
+            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache,
+                                       &topo_info, eax, ebx, ecx, edx);
             break;
         case 3: /* L3 cache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
-                                       eax, ebx, ecx, edx);
+            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache,
+                                       &topo_info, eax, ebx, ecx, edx);
             break;
         default: /* end of info */
             *eax = *ebx = *ecx = *edx = 0;

[PATCH v6 10/10] i386: Simplify CPUID_8000_001E for AMD

[Babu Moger]

apic_id contains all the information required to build
CPUID_8000_001E. core_id and node_id is already part of
apic_id generated by x86_topo_ids_from_apicid.

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.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c |  195 ++++++++++++-----------------------------------------
 1 file changed, 45 insertions(+), 150 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d434c8545a..ada9ec8f3a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -338,62 +338,6 @@ static void encode_cache_cpuid80000006(CPUCacheInfo *l2,
     }
 }
 
-/*
- * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
- * Please refer to the AMD64 Architecture Programmer’s Manual Volume 3.
- * Define the constants to build the cpu topology. Right now, TOPOEXT
- * feature is enabled only on EPYC. So, these constants are based on
- * EPYC supported configurations. We may need to handle the cases if
- * these values change in future.
- */
-/* Maximum core complexes in a node */
-#define MAX_CCX 2
-/* Maximum cores in a core complex */
-#define MAX_CORES_IN_CCX 4
-/* Maximum cores in a node */
-#define MAX_CORES_IN_NODE 8
-/* Maximum nodes in a socket */
-#define MAX_NODES_PER_SOCKET 4
-
-/*
- * Figure out the number of nodes required to build this config.
- * Max cores in a node is 8
- */
-static int nodes_in_socket(int nr_cores)
-{
-    int nodes;
-
-    nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
-
-   /* Hardware does not support config with 3 nodes, return 4 in that case */
-    return (nodes == 3) ? 4 : nodes;
-}
-
-/*
- * Decide the number of cores in a core complex with the given nr_cores using
- * following set constants MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE and
- * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
- * L3 cache is shared across all cores in a core complex. So, this will also
- * tell us how many cores are sharing the L3 cache.
- */
-static int cores_in_core_complex(int nr_cores)
-{
-    int nodes;
-
-    /* Check if we can fit all the cores in one core complex */
-    if (nr_cores <= MAX_CORES_IN_CCX) {
-        return nr_cores;
-    }
-    /* Get the number of nodes required to build this config */
-    nodes = nodes_in_socket(nr_cores);
-
-    /*
-     * Divide the cores accros all the core complexes
-     * Return rounded up value
-     */
-    return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
-}
-
 /* Encode cache info for CPUID[8000001D] */
 static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
                                        X86CPUTopoInfo *topo_info,
@@ -434,107 +378,58 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
            (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
 }
 
-/* Data structure to hold the configuration info for a given core index */
-struct core_topology {
-    /* core complex id of the current core index */
-    int ccx_id;
-    /*
-     * Adjusted core index for this core in the topology
-     * This can be 0,1,2,3 with max 4 cores in a core complex
-     */
-    int core_id;
-    /* Node id for this core index */
-    int node_id;
-    /* Number of nodes in this config */
-    int num_nodes;
-};
-
-/*
- * Build the configuration closely match the EPYC hardware. Using the EPYC
- * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE)
- * right now. This could change in future.
- * nr_cores : Total number of cores in the config
- * core_id  : Core index of the current CPU
- * topo     : Data structure to hold all the config info for this core index
- */
-static void build_core_topology(int nr_cores, int core_id,
-                                struct core_topology *topo)
-{
-    int nodes, cores_in_ccx;
-
-    /* First get the number of nodes required */
-    nodes = nodes_in_socket(nr_cores);
-
-    cores_in_ccx = cores_in_core_complex(nr_cores);
-
-    topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
-    topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
-    topo->core_id = core_id % cores_in_ccx;
-    topo->num_nodes = nodes;
-}
-
 /* Encode cache info for CPUID[8000001E] */
-static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
-                                       uint32_t *eax, uint32_t *ebx,
-                                       uint32_t *ecx, uint32_t *edx)
+static void encode_topo_cpuid8000001e(X86CPU *cpu, X86CPUTopoInfo *topo_info,
+                                      uint32_t *eax, uint32_t *ebx,
+                                      uint32_t *ecx, uint32_t *edx)
 {
-    struct core_topology topo = {0};
-    unsigned long nodes;
-    int shift;
+    X86CPUTopoIDs topo_ids;
+
+    x86_topo_ids_from_apicid(cpu->apic_id, topo_info, &topo_ids);
 
-    build_core_topology(cs->nr_cores, cpu->core_id, &topo);
     *eax = cpu->apic_id;
+
     /*
-     * 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
+     * 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];
+     * 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.
      */
-    if (cs->nr_threads - 1) {
-        *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
-                (topo.ccx_id << 2) | topo.core_id;
-    } else {
-        *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
-    }
+    *ebx = ((topo_info->threads_per_core - 1) << 8) | (topo_ids.core_id & 0xFF);
+
     /*
-     * 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
+     * CPUID_Fn8000001E_ECX [Node Identifiers] (NodeId)
+     * Read-only. Reset: 0000_0XXXh.
+     * Core::X86::Cpuid::NodeId_lthree[1:0]_core[3:0]_thread[1:0];
+     * 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 (topo.num_nodes <= 4) {
-        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << 2) |
-                topo.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 = topo.num_nodes - 1;
-        shift = find_last_bit(&nodes, 8);
-        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << (shift + 1)) |
-                topo.node_id;
-    }
+    *ecx = ((topo_info->dies_per_pkg - 1) << 8) |
+           ((cpu->apic_id >> apicid_die_offset(topo_info)) & 0xFF);
+
     *edx = 0;
 }
 
@@ -6019,7 +5914,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         break;
     case 0x8000001E:
         assert(cpu->core_id <= 255);
-        encode_topo_cpuid8000001e(cs, cpu,
+        encode_topo_cpuid8000001e(cpu, &topo_info,
                                   eax, ebx, ecx, edx);
         break;
     case 0xC0000000:

Re: [PATCH v6 09/10] i386: Simplify CPUID_8000_001E for AMD

[Igor Mammedov]

On Mon, 31 Aug 2020 13:43:01 -0500
Babu Moger <babu.moger@amd.com> wrote:

> Remove all the hardcoded values and replace with generalized
> fields.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c |   31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ba4667b33c..d434c8545a 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -395,9 +395,10 @@ static int cores_in_core_complex(int nr_cores)
>  }
>  
>  /* Encode cache info for CPUID[8000001D] */
> -static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
> -                                uint32_t *eax, uint32_t *ebx,
> -                                uint32_t *ecx, uint32_t *edx)
> +static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
> +                                       X86CPUTopoInfo *topo_info,
> +                                       uint32_t *eax, uint32_t *ebx,
> +                                       uint32_t *ecx, uint32_t *edx)
>  {
>      uint32_t l3_cores;
>      assert(cache->size == cache->line_size * cache->associativity *
> @@ -408,10 +409,12 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
>  
>      /* L3 is shared among multiple cores */
>      if (cache->level == 3) {
> -        l3_cores = cores_in_core_complex(cs->nr_cores);
> -        *eax |= ((l3_cores * cs->nr_threads) - 1) << 14;
> +        l3_cores = DIV_ROUND_UP((topo_info->cores_per_die *
> +                                 topo_info->threads_per_core),
> +                                 topo_info->dies_per_pkg);

from spec:
"
NumSharingCache: number of '''logical processors''' sharing cache.
"

s/l3_cores/l3_vcpus|l3_threads/

Also why not use just:

  val = topo_info->cores_per_die * topo_info->threads_per_core



> +        *eax |= (l3_cores - 1) << 14;
>      } else {
> -        *eax |= ((cs->nr_threads - 1) << 14);
> +        *eax |= ((topo_info->threads_per_core - 1) << 14);
>      }
>  
>      assert(cache->line_size > 0);
> @@ -5994,20 +5997,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          }
>          switch (count) {
>          case 0: /* L1 dcache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
> -                                       eax, ebx, ecx, edx);
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache,
> +                                       &topo_info, eax, ebx, ecx, edx);
>              break;
>          case 1: /* L1 icache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
> -                                       eax, ebx, ecx, edx);
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache,
> +                                       &topo_info, eax, ebx, ecx, edx);
>              break;
>          case 2: /* L2 cache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
> -                                       eax, ebx, ecx, edx);
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache,
> +                                       &topo_info, eax, ebx, ecx, edx);
>              break;
>          case 3: /* L3 cache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
> -                                       eax, ebx, ecx, edx);
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache,
> +                                       &topo_info, eax, ebx, ecx, edx);
>              break;
>          default: /* end of info */
>              *eax = *ebx = *ecx = *edx = 0;
> 

Re: [PATCH v6 10/10] i386: Simplify CPUID_8000_001E for AMD

[Igor Mammedov]

On Mon, 31 Aug 2020 13:43:07 -0500
Babu Moger <babu.moger@amd.com> wrote:

> apic_id contains all the information required to build
> CPUID_8000_001E. core_id and node_id is already part of
> apic_id generated by x86_topo_ids_from_apicid.
> 
> 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.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target/i386/cpu.c |  195 ++++++++++++-----------------------------------------
>  1 file changed, 45 insertions(+), 150 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index d434c8545a..ada9ec8f3a 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -338,62 +338,6 @@ static void encode_cache_cpuid80000006(CPUCacheInfo *l2,
>      }
>  }
>  
> -/*
> - * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
> - * Please refer to the AMD64 Architecture Programmer’s Manual Volume 3.
> - * Define the constants to build the cpu topology. Right now, TOPOEXT
> - * feature is enabled only on EPYC. So, these constants are based on
> - * EPYC supported configurations. We may need to handle the cases if
> - * these values change in future.
> - */
> -/* Maximum core complexes in a node */
> -#define MAX_CCX 2
> -/* Maximum cores in a core complex */
> -#define MAX_CORES_IN_CCX 4
> -/* Maximum cores in a node */
> -#define MAX_CORES_IN_NODE 8
> -/* Maximum nodes in a socket */
> -#define MAX_NODES_PER_SOCKET 4
> -
> -/*
> - * Figure out the number of nodes required to build this config.
> - * Max cores in a node is 8
> - */
> -static int nodes_in_socket(int nr_cores)
> -{
> -    int nodes;
> -
> -    nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
> -
> -   /* Hardware does not support config with 3 nodes, return 4 in that case */
> -    return (nodes == 3) ? 4 : nodes;
> -}
> -
> -/*
> - * Decide the number of cores in a core complex with the given nr_cores using
> - * following set constants MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE and
> - * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
> - * L3 cache is shared across all cores in a core complex. So, this will also
> - * tell us how many cores are sharing the L3 cache.
> - */
> -static int cores_in_core_complex(int nr_cores)
> -{
> -    int nodes;
> -
> -    /* Check if we can fit all the cores in one core complex */
> -    if (nr_cores <= MAX_CORES_IN_CCX) {
> -        return nr_cores;
> -    }
> -    /* Get the number of nodes required to build this config */
> -    nodes = nodes_in_socket(nr_cores);
> -
> -    /*
> -     * Divide the cores accros all the core complexes
> -     * Return rounded up value
> -     */
> -    return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
> -}
> -
>  /* Encode cache info for CPUID[8000001D] */
>  static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>                                         X86CPUTopoInfo *topo_info,
> @@ -434,107 +378,58 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>             (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
>  }
>  
> -/* Data structure to hold the configuration info for a given core index */
> -struct core_topology {
> -    /* core complex id of the current core index */
> -    int ccx_id;
> -    /*
> -     * Adjusted core index for this core in the topology
> -     * This can be 0,1,2,3 with max 4 cores in a core complex
> -     */
> -    int core_id;
> -    /* Node id for this core index */
> -    int node_id;
> -    /* Number of nodes in this config */
> -    int num_nodes;
> -};
> -
> -/*
> - * Build the configuration closely match the EPYC hardware. Using the EPYC
> - * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE)
> - * right now. This could change in future.
> - * nr_cores : Total number of cores in the config
> - * core_id  : Core index of the current CPU
> - * topo     : Data structure to hold all the config info for this core index
> - */
> -static void build_core_topology(int nr_cores, int core_id,
> -                                struct core_topology *topo)
> -{
> -    int nodes, cores_in_ccx;
> -
> -    /* First get the number of nodes required */
> -    nodes = nodes_in_socket(nr_cores);
> -
> -    cores_in_ccx = cores_in_core_complex(nr_cores);
> -
> -    topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
> -    topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
> -    topo->core_id = core_id % cores_in_ccx;
> -    topo->num_nodes = nodes;
> -}
> -
>  /* Encode cache info for CPUID[8000001E] */
> -static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
> -                                       uint32_t *eax, uint32_t *ebx,
> -                                       uint32_t *ecx, uint32_t *edx)
> +static void encode_topo_cpuid8000001e(X86CPU *cpu, X86CPUTopoInfo *topo_info,
> +                                      uint32_t *eax, uint32_t *ebx,
> +                                      uint32_t *ecx, uint32_t *edx)
>  {
> -    struct core_topology topo = {0};
> -    unsigned long nodes;
> -    int shift;
> +    X86CPUTopoIDs topo_ids;
> +
> +    x86_topo_ids_from_apicid(cpu->apic_id, topo_info, &topo_ids);
>  
> -    build_core_topology(cs->nr_cores, cpu->core_id, &topo);
>      *eax = cpu->apic_id;
> +
>      /*
> -     * 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
> +     * 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];
> +     * 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.
>       */
> -    if (cs->nr_threads - 1) {
> -        *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> -                (topo.ccx_id << 2) | topo.core_id;
> -    } else {
> -        *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> -    }
> +    *ebx = ((topo_info->threads_per_core - 1) << 8) | (topo_ids.core_id & 0xFF);
> +
>      /*
> -     * 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
> +     * CPUID_Fn8000001E_ECX [Node Identifiers] (NodeId)
> +     * Read-only. Reset: 0000_0XXXh.
> +     * Core::X86::Cpuid::NodeId_lthree[1:0]_core[3:0]_thread[1:0];
> +     * 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 (topo.num_nodes <= 4) {
> -        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << 2) |
> -                topo.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 = topo.num_nodes - 1;
> -        shift = find_last_bit(&nodes, 8);
> -        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << (shift + 1)) |
> -                topo.node_id;
> -    }
> +    *ecx = ((topo_info->dies_per_pkg - 1) << 8) |
> +           ((cpu->apic_id >> apicid_die_offset(topo_info)) & 0xFF);
> +
>      *edx = 0;
>  }
>  
> @@ -6019,7 +5914,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          break;
>      case 0x8000001E:
>          assert(cpu->core_id <= 255);
> -        encode_topo_cpuid8000001e(cs, cpu,
> +        encode_topo_cpuid8000001e(cpu, &topo_info,
>                                    eax, ebx, ecx, edx);
>          break;
>      case 0xC0000000:
> 
> 

Re: [PATCH v6 01/10] Revert "i386: Fix pkg_id offset for EPYC cpu models"

[Igor Mammedov]

On Mon, 31 Aug 2020 13:42:11 -0500
Babu Moger <babu.moger@amd.com> wrote:

> This reverts commit 7b225762c8c05fd31d4c2be116aedfbc00383f8b.
> 
> Remove the EPYC specific apicid decoding and use the generic
> default decoding.
> 
> Also fix all the references of pkg_offset.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc.c      |    1 -
>  target/i386/cpu.c |    9 ++++-----
>  target/i386/cpu.h |    1 -
>  3 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5d8d5ef8b3..6b708f4341 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1502,7 +1502,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>  
>      env->nr_dies = x86ms->smp_dies;
>      env->nr_nodes = topo_info.nodes_per_pkg;
> -    env->pkg_offset = x86ms->apicid_pkg_offset(&topo_info);
>  
>      /*
>       * If APIC ID is not set,
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 588f32e136..173e6f4a07 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5678,7 +5678,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              *ecx |= CPUID_TOPOLOGY_LEVEL_SMT;
>              break;
>          case 1:
> -            *eax = env->pkg_offset;
> +            *eax = apicid_pkg_offset(&topo_info);
>              *ebx = cs->nr_cores * cs->nr_threads;
>              *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
>              break;
> @@ -5712,7 +5712,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
>              break;
>          case 2:
> -            *eax = env->pkg_offset;
> +            *eax = apicid_pkg_offset(&topo_info);
>              *ebx = env->nr_dies * cs->nr_cores * cs->nr_threads;
>              *ecx |= CPUID_TOPOLOGY_LEVEL_DIE;
>              break;
> @@ -5889,11 +5889,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              /*
>               * Bits 15:12 is "The number of bits in the initial
>               * Core::X86::Apic::ApicId[ApicId] value that indicate
> -             * thread ID within a package". This is already stored at
> -             * CPUX86State::pkg_offset.
> +             * thread ID within a package".
>               * Bits 7:0 is "The number of threads in the package is NC+1"
>               */
> -            *ecx = (env->pkg_offset << 12) |
> +            *ecx = (apicid_pkg_offset(&topo_info) << 12) |
>                     ((cs->nr_cores * cs->nr_threads) - 1);
>          } else {
>              *ecx = 0;
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index e1a5c174dc..d5ad42d694 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1630,7 +1630,6 @@ typedef struct CPUX86State {
>  
>      unsigned nr_dies;
>      unsigned nr_nodes;
> -    unsigned pkg_offset;
>  } CPUX86State;
>  
>  struct kvm_msrs;
> 

Re: [PATCH v6 02/10] Revert "target/i386: Enable new apic id encoding for EPYC based cpus models"

[Igor Mammedov]

On Mon, 31 Aug 2020 13:42:17 -0500
Babu Moger <babu.moger@amd.com> wrote:

> This reverts commit 247b18c593ec298446645af8d5d28911daf653b1.
> 
> Remove the EPYC specific apicid decoding and use the generic
> default decoding.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target/i386/cpu.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 173e6f4a07..c9c1e681c2 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3995,7 +3995,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .xlevel = 0x8000001E,
>          .model_id = "AMD EPYC Processor",
>          .cache_info = &epyc_cache_info,
> -        .use_epyc_apic_id_encoding = 1,
>          .versions = (X86CPUVersionDefinition[]) {
>              { .version = 1 },
>              {
> @@ -4123,7 +4122,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .xlevel = 0x8000001E,
>          .model_id = "AMD EPYC-Rome Processor",
>          .cache_info = &epyc_rome_cache_info,
> -        .use_epyc_apic_id_encoding = 1,
>      },
>  };
>  
> 

Re: [PATCH v6 03/10] Revert "hw/i386: Move arch_id decode inside x86_cpus_init"

[Igor Mammedov]

On Mon, 31 Aug 2020 13:42:23 -0500
Babu Moger <babu.moger@amd.com> wrote:

> This reverts commit 2e26f4ab3bf8390a2677d3afd9b1a04f015d7721.
> 
> Remove the EPYC specific apicid decoding and use the generic
> default decoding.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc.c  |    6 +++---
>  hw/i386/x86.c |   37 +++++++------------------------------
>  2 files changed, 10 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6b708f4341..0677d6a272 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1556,14 +1556,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>          topo_ids.die_id = cpu->die_id;
>          topo_ids.core_id = cpu->core_id;
>          topo_ids.smt_id = cpu->thread_id;
> -        cpu->apic_id = x86ms->apicid_from_topo_ids(&topo_info, &topo_ids);
> +        cpu->apic_id = x86_apicid_from_topo_ids(&topo_info, &topo_ids);
>      }
>  
>      cpu_slot = pc_find_cpu_slot(MACHINE(pcms), cpu->apic_id, &idx);
>      if (!cpu_slot) {
>          MachineState *ms = MACHINE(pcms);
>  
> -        x86ms->topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
> +        x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
>          error_setg(errp,
>              "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
>              " APIC ID %" PRIu32 ", valid index range 0:%d",
> @@ -1584,7 +1584,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn()
>       * once -smp refactoring is complete and there will be CPU private
>       * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */
> -    x86ms->topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
> +    x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
>      if (cpu->socket_id != -1 && cpu->socket_id != topo_ids.pkg_id) {
>          error_setg(errp, "property socket-id: %u doesn't match set apic-id:"
>              " 0x%x (socket-id: %u)", cpu->socket_id, cpu->apic_id,
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index cf384b9743..3cc2318212 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -68,22 +68,6 @@ inline void init_topo_info(X86CPUTopoInfo *topo_info,
>      topo_info->threads_per_core = ms->smp.threads;
>  }
>  
> -/*
> - * Set up with the new EPYC topology handlers
> - *
> - * AMD uses different apic id encoding for EPYC based cpus. Override
> - * the default topo handlers with EPYC encoding handlers.
> - */
> -static void x86_set_epyc_topo_handlers(MachineState *machine)
> -{
> -    X86MachineState *x86ms = X86_MACHINE(machine);
> -
> -    x86ms->apicid_from_cpu_idx = x86_apicid_from_cpu_idx_epyc;
> -    x86ms->topo_ids_from_apicid = x86_topo_ids_from_apicid_epyc;
> -    x86ms->apicid_from_topo_ids = x86_apicid_from_topo_ids_epyc;
> -    x86ms->apicid_pkg_offset = apicid_pkg_offset_epyc;
> -}
> -
>  /*
>   * Calculates initial APIC ID for a specific CPU index
>   *
> @@ -102,7 +86,7 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
>  
>      init_topo_info(&topo_info, x86ms);
>  
> -    correct_id = x86ms->apicid_from_cpu_idx(&topo_info, cpu_index);
> +    correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index);
>      if (x86mc->compat_apic_id_mode) {
>          if (cpu_index != correct_id && !warned && !qtest_enabled()) {
>              error_report("APIC IDs set in compatibility mode, "
> @@ -136,11 +120,6 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>      MachineState *ms = MACHINE(x86ms);
>      MachineClass *mc = MACHINE_GET_CLASS(x86ms);
>  
> -    /* Check for apicid encoding */
> -    if (cpu_x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
> -        x86_set_epyc_topo_handlers(ms);
> -    }
> -
>      x86_cpu_set_default_version(default_cpu_version);
>  
>      /*
> @@ -154,12 +133,6 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>      x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
>                                                        ms->smp.max_cpus - 1) + 1;
>      possible_cpus = mc->possible_cpu_arch_ids(ms);
> -
> -    for (i = 0; i < ms->possible_cpus->len; i++) {
> -        ms->possible_cpus->cpus[i].arch_id =
> -            x86_cpu_apic_id_from_index(x86ms, i);
> -    }
> -
>      for (i = 0; i < ms->smp.cpus; i++) {
>          x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal);
>      }
> @@ -184,7 +157,8 @@ int64_t x86_get_default_cpu_node_id(const MachineState *ms, int idx)
>     init_topo_info(&topo_info, x86ms);
>  
>     assert(idx < ms->possible_cpus->len);
> -   x86_topo_ids_from_idx(&topo_info, idx, &topo_ids);
> +   x86_topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id,
> +                            &topo_info, &topo_ids);
>     return topo_ids.pkg_id % ms->numa_state->num_nodes;
>  }
>  
> @@ -215,7 +189,10 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
>  
>          ms->possible_cpus->cpus[i].type = ms->cpu_type;
>          ms->possible_cpus->cpus[i].vcpus_count = 1;
> -        x86_topo_ids_from_idx(&topo_info, i, &topo_ids);
> +        ms->possible_cpus->cpus[i].arch_id =
> +            x86_cpu_apic_id_from_index(x86ms, i);
> +        x86_topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id,
> +                                 &topo_info, &topo_ids);
>          ms->possible_cpus->cpus[i].props.has_socket_id = true;
>          ms->possible_cpus->cpus[i].props.socket_id = topo_ids.pkg_id;
>          if (x86ms->smp_dies > 1) {
> 

Re: [PATCH v6 04/10] Revert "i386: Introduce use_epyc_apic_id_encoding in X86CPUDefinition"

[Igor Mammedov]

On Mon, 31 Aug 2020 13:42:30 -0500
Babu Moger <babu.moger@amd.com> wrote:

> This reverts commit 0c1538cb1a26287c072645f4759b9872b1596d79.
> 
> Remove the EPYC specific apicid decoding and use the generic
> default decoding.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target/i386/cpu.c |   16 ----------------
>  target/i386/cpu.h |    1 -
>  2 files changed, 17 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c9c1e681c2..b72b4b08ac 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1638,10 +1638,6 @@ typedef struct X86CPUDefinition {
>      FeatureWordArray features;
>      const char *model_id;
>      CPUCaches *cache_info;
> -
> -    /* Use AMD EPYC encoding for apic id */
> -    bool use_epyc_apic_id_encoding;
> -
>      /*
>       * Definitions for alternative versions of CPU model.
>       * List is terminated by item with version == 0.
> @@ -1683,18 +1679,6 @@ static const X86CPUVersionDefinition *x86_cpu_def_get_versions(X86CPUDefinition
>      return def->versions ?: default_version_list;
>  }
>  
> -bool cpu_x86_use_epyc_apic_id_encoding(const char *cpu_type)
> -{
> -    X86CPUClass *xcc = X86_CPU_CLASS(object_class_by_name(cpu_type));
> -
> -    assert(xcc);
> -    if (xcc->model && xcc->model->cpudef) {
> -        return xcc->model->cpudef->use_epyc_apic_id_encoding;
> -    } else {
> -        return false;
> -    }
> -}
> -
>  static CPUCaches epyc_cache_info = {
>      .l1d_cache = &(CPUCacheInfo) {
>          .type = DATA_CACHE,
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index d5ad42d694..5ff8ad8427 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1918,7 +1918,6 @@ void cpu_clear_apic_feature(CPUX86State *env);
>  void host_cpuid(uint32_t function, uint32_t count,
>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
>  void host_vendor_fms(char *vendor, int *family, int *model, int *stepping);
> -bool cpu_x86_use_epyc_apic_id_encoding(const char *cpu_type);
>  
>  /* helper.c */
>  bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> 

Re: [PATCH v6 05/10] Revert "hw/i386: Introduce apicid functions inside X86MachineState"

[Igor Mammedov]

On Mon, 31 Aug 2020 13:42:36 -0500
Babu Moger <babu.moger@amd.com> wrote:

> This reverts commit 6121c7fbfd98dbc3af1b00b56ff2eef66df87828.
> 
> Remove the EPYC specific apicid decoding and use the generic
> default decoding.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/x86.c         |    5 -----
>  include/hw/i386/x86.h |    9 ---------
>  2 files changed, 14 deletions(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 3cc2318212..727c4a0f1d 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -896,11 +896,6 @@ static void x86_machine_initfn(Object *obj)
>      x86ms->smm = ON_OFF_AUTO_AUTO;
>      x86ms->acpi = ON_OFF_AUTO_AUTO;
>      x86ms->smp_dies = 1;
> -
> -    x86ms->apicid_from_cpu_idx = x86_apicid_from_cpu_idx;
> -    x86ms->topo_ids_from_apicid = x86_topo_ids_from_apicid;
> -    x86ms->apicid_from_topo_ids = x86_apicid_from_topo_ids;
> -    x86ms->apicid_pkg_offset = apicid_pkg_offset;
>  }
>  
>  static void x86_machine_class_init(ObjectClass *oc, void *data)
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index b79f24e285..4d9a26326d 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -63,15 +63,6 @@ typedef struct {
>      OnOffAuto smm;
>      OnOffAuto acpi;
>  
> -    /* Apic id specific handlers */
> -    uint32_t (*apicid_from_cpu_idx)(X86CPUTopoInfo *topo_info,
> -                                    unsigned cpu_index);
> -    void (*topo_ids_from_apicid)(apic_id_t apicid, X86CPUTopoInfo *topo_info,
> -                                 X86CPUTopoIDs *topo_ids);
> -    apic_id_t (*apicid_from_topo_ids)(X86CPUTopoInfo *topo_info,
> -                                      const X86CPUTopoIDs *topo_ids);
> -    uint32_t (*apicid_pkg_offset)(X86CPUTopoInfo *topo_info);
> -
>      /*
>       * Address space used by IOAPIC device. All IOAPIC interrupts
>       * will be translated to MSI messages in the address space.
> 

Re: [PATCH v6 06/10] Revert "target/i386: Cleanup and use the EPYC mode topology functions"

[Igor Mammedov]

On Mon, 31 Aug 2020 13:42:42 -0500
Babu Moger <babu.moger@amd.com> wrote:

> This reverts commit dd08ef0318e2b61d14bc069590d174913f7f437a.
> 
> Remove the EPYC specific apicid decoding and use the generic
> default decoding.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target/i386/cpu.c |  161 ++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 127 insertions(+), 34 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b72b4b08ac..256bfa669f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -338,15 +338,68 @@ static void encode_cache_cpuid80000006(CPUCacheInfo *l2,
>      }
>  }
>  
> +/*
> + * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
> + * Please refer to the AMD64 Architecture Programmer’s Manual Volume 3.
> + * Define the constants to build the cpu topology. Right now, TOPOEXT
> + * feature is enabled only on EPYC. So, these constants are based on
> + * EPYC supported configurations. We may need to handle the cases if
> + * these values change in future.
> + */
> +/* Maximum core complexes in a node */
> +#define MAX_CCX 2
> +/* Maximum cores in a core complex */
> +#define MAX_CORES_IN_CCX 4
> +/* Maximum cores in a node */
> +#define MAX_CORES_IN_NODE 8
> +/* Maximum nodes in a socket */
> +#define MAX_NODES_PER_SOCKET 4
> +
> +/*
> + * Figure out the number of nodes required to build this config.
> + * Max cores in a node is 8
> + */
> +static int nodes_in_socket(int nr_cores)
> +{
> +    int nodes;
> +
> +    nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
> +
> +   /* Hardware does not support config with 3 nodes, return 4 in that case */
> +    return (nodes == 3) ? 4 : nodes;
> +}
> +
> +/*
> + * Decide the number of cores in a core complex with the given nr_cores using
> + * following set constants MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE and
> + * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
> + * L3 cache is shared across all cores in a core complex. So, this will also
> + * tell us how many cores are sharing the L3 cache.
> + */
> +static int cores_in_core_complex(int nr_cores)
> +{
> +    int nodes;
> +
> +    /* Check if we can fit all the cores in one core complex */
> +    if (nr_cores <= MAX_CORES_IN_CCX) {
> +        return nr_cores;
> +    }
> +    /* Get the number of nodes required to build this config */
> +    nodes = nodes_in_socket(nr_cores);
> +
> +    /*
> +     * Divide the cores accros all the core complexes
> +     * Return rounded up value
> +     */
> +    return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
> +}
> +
>  /* Encode cache info for CPUID[8000001D] */
> -static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
> -                                       X86CPUTopoInfo *topo_info,
> -                                       uint32_t *eax, uint32_t *ebx,
> -                                       uint32_t *ecx, uint32_t *edx)
> +static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
> +                                uint32_t *eax, uint32_t *ebx,
> +                                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,13 +408,10 @@ 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 *
> -                                 topo_info->threads_per_core),
> -                                 nodes);
> -        *eax |= (l3_cores - 1) << 14;
> +        l3_cores = cores_in_core_complex(cs->nr_cores);
> +        *eax |= ((l3_cores * cs->nr_threads) - 1) << 14;
>      } else {
> -        *eax |= ((topo_info->threads_per_core - 1) << 14);
> +        *eax |= ((cs->nr_threads - 1) << 14);
>      }
>  
>      assert(cache->line_size > 0);
> @@ -381,17 +431,55 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>             (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
>  }
>  
> +/* Data structure to hold the configuration info for a given core index */
> +struct core_topology {
> +    /* core complex id of the current core index */
> +    int ccx_id;
> +    /*
> +     * Adjusted core index for this core in the topology
> +     * This can be 0,1,2,3 with max 4 cores in a core complex
> +     */
> +    int core_id;
> +    /* Node id for this core index */
> +    int node_id;
> +    /* Number of nodes in this config */
> +    int num_nodes;
> +};
> +
> +/*
> + * Build the configuration closely match the EPYC hardware. Using the EPYC
> + * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE)
> + * right now. This could change in future.
> + * nr_cores : Total number of cores in the config
> + * core_id  : Core index of the current CPU
> + * topo     : Data structure to hold all the config info for this core index
> + */
> +static void build_core_topology(int nr_cores, int core_id,
> +                                struct core_topology *topo)
> +{
> +    int nodes, cores_in_ccx;
> +
> +    /* First get the number of nodes required */
> +    nodes = nodes_in_socket(nr_cores);
> +
> +    cores_in_ccx = cores_in_core_complex(nr_cores);
> +
> +    topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
> +    topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
> +    topo->core_id = core_id % cores_in_ccx;
> +    topo->num_nodes = nodes;
> +}
> +
>  /* Encode cache info for CPUID[8000001E] */
> -static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
> +static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
>                                         uint32_t *eax, uint32_t *ebx,
>                                         uint32_t *ecx, uint32_t *edx)
>  {
> -    X86CPUTopoIDs topo_ids = {0};
> -    unsigned long nodes = MAX(topo_info->nodes_per_pkg, 1);
> +    struct core_topology topo = {0};
> +    unsigned long nodes;
>      int shift;
>  
> -    x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
> -
> +    build_core_topology(cs->nr_cores, cpu->core_id, &topo);
>      *eax = cpu->apic_id;
>      /*
>       * CPUID_Fn8000001E_EBX
> @@ -408,8 +496,12 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>       *             3 Core complex id
>       *           1:0 Core id
>       */
> -    *ebx = ((topo_info->threads_per_core - 1) << 8) | (topo_ids.node_id << 3) |
> -            (topo_ids.core_id);
> +    if (cs->nr_threads - 1) {
> +        *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> +                (topo.ccx_id << 2) | topo.core_id;
> +    } else {
> +        *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> +    }
>      /*
>       * CPUID_Fn8000001E_ECX
>       * 31:11 Reserved
> @@ -418,8 +510,9 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>       *         2  Socket id
>       *       1:0  Node id
>       */
> -    if (nodes <= 4) {
> -        *ecx = ((nodes - 1) << 8) | (topo_ids.pkg_id << 2) | topo_ids.node_id;
> +    if (topo.num_nodes <= 4) {
> +        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << 2) |
> +                topo.node_id;
>      } else {
>          /*
>           * Node id fix up. Actual hardware supports up to 4 nodes. But with
> @@ -434,10 +527,10 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>           * 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;
> +        nodes = topo.num_nodes - 1;
>          shift = find_last_bit(&nodes, 8);
> -        *ecx = (nodes << 8) | (topo_ids.pkg_id << (shift + 1)) |
> -               topo_ids.node_id;
> +        *ecx = ((topo.num_nodes - 1) << 8) | (cpu->socket_id << (shift + 1)) |
> +                topo.node_id;
>      }
>      *edx = 0;
>  }
> @@ -5471,7 +5564,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;
> @@ -5902,20 +5994,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          }
>          switch (count) {
>          case 0: /* L1 dcache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache,
> -                                       &topo_info, eax, ebx, ecx, edx);
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
> +                                       eax, ebx, ecx, edx);
>              break;
>          case 1: /* L1 icache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache,
> -                                       &topo_info, eax, ebx, ecx, edx);
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
> +                                       eax, ebx, ecx, edx);
>              break;
>          case 2: /* L2 cache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache,
> -                                       &topo_info, eax, ebx, ecx, edx);
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
> +                                       eax, ebx, ecx, edx);
>              break;
>          case 3: /* L3 cache info */
> -            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache,
> -                                       &topo_info, eax, ebx, ecx, edx);
> +            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
> +                                       eax, ebx, ecx, edx);
>              break;
>          default: /* end of info */
>              *eax = *ebx = *ecx = *edx = 0;
> @@ -5924,7 +6016,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          break;
>      case 0x8000001E:
>          assert(cpu->core_id <= 255);
> -        encode_topo_cpuid8000001e(&topo_info, cpu, eax, ebx, ecx, edx);
> +        encode_topo_cpuid8000001e(cs, cpu,
> +                                  eax, ebx, ecx, edx);
>          break;
>      case 0xC0000000:
>          *eax = env->cpuid_xlevel2;
> 

Re: [PATCH v6 07/10] Revert "hw/386: Add EPYC mode topology decoding functions"

[Igor Mammedov]

On Mon, 31 Aug 2020 13:42:48 -0500
Babu Moger <babu.moger@amd.com> wrote:

> This reverts commit 7568b205555a6405042f62c64af3268f4330aed5.
> 
> Remove the EPYC specific apicid decoding and use the generic
> default decoding.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/hw/i386/topology.h |  100 --------------------------------------------
>  1 file changed, 100 deletions(-)
> 
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index 07239f95f4..b9593b9905 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -47,7 +47,6 @@ typedef uint32_t apic_id_t;
>  
>  typedef struct X86CPUTopoIDs {
>      unsigned pkg_id;
> -    unsigned node_id;
>      unsigned die_id;
>      unsigned core_id;
>      unsigned smt_id;
> @@ -89,11 +88,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,100 +108,6 @@ 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 */
> -
> -/*
> - * Bit offset of the node_id field
> - *
> - * Make sure nodes_per_pkg >  0 if numa configured else zero.
> - */
> -static inline unsigned apicid_node_offset_epyc(X86CPUTopoInfo *topo_info)
> -{
> -    unsigned offset = apicid_die_offset(topo_info) +
> -                      apicid_die_width(topo_info);
> -
> -    if (topo_info->nodes_per_pkg) {
> -        return MAX(NODE_ID_OFFSET, offset);
> -    } else {
> -        return 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);
> -}
> -
> -/*
> - * Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
> - *
> - * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
> - */
> -static inline apic_id_t
> -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->core_id << apicid_core_offset(topo_info)) |
> -           topo_ids->smt_id;
> -}
> -
> -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;
> -}
> -
> -/*
> - * Calculate thread/core/package IDs for a specific topology,
> - * based on APIC ID
> - */
> -static inline void x86_topo_ids_from_apicid_epyc(apic_id_t apicid,
> -                                            X86CPUTopoInfo *topo_info,
> -                                            X86CPUTopoIDs *topo_ids)
> -{
> -    topo_ids->smt_id = apicid &
> -            ~(0xFFFFFFFFUL << apicid_smt_width(topo_info));
> -    topo_ids->core_id =
> -            (apicid >> apicid_core_offset(topo_info)) &
> -            ~(0xFFFFFFFFUL << apicid_core_width(topo_info));
> -    topo_ids->die_id =
> -            (apicid >> apicid_die_offset(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);
> -}
> -
> -/*
> - * Make APIC ID for the CPU 'cpu_index'
> - *
> - * 'cpu_index' is a sequential, contiguous ID for the CPU.
> - */
> -static inline apic_id_t x86_apicid_from_cpu_idx_epyc(X86CPUTopoInfo *topo_info,
> -                                                     unsigned cpu_index)
> -{
> -    X86CPUTopoIDs topo_ids;
> -    x86_topo_ids_from_idx_epyc(topo_info, cpu_index, &topo_ids);
> -    return x86_apicid_from_topo_ids_epyc(topo_info, &topo_ids);
> -}
>  /* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
>   *
>   * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
> 

Re: [PATCH v6 08/10] Revert "hw/i386: Update structures to save the number of nodes per package"

[Igor Mammedov]

On Mon, 31 Aug 2020 13:42:54 -0500
Babu Moger <babu.moger@amd.com> wrote:

> This reverts commit c24a41bb53c0854d22c96b30d57cfcaa543c409d.
> 
> Remove the EPYC specific apicid decoding and use the generic
> default decoding.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc.c               |    1 -
>  hw/i386/x86.c              |    1 -
>  include/hw/i386/topology.h |    1 -
>  target/i386/cpu.c          |    1 -
>  target/i386/cpu.h          |    1 -
>  tests/test-x86-cpuid.c     |   40 ++++++++++++++++++++--------------------
>  6 files changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 0677d6a272..d11daacc23 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1501,7 +1501,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;
>  
>      /*
>       * If APIC ID is not set,
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 727c4a0f1d..c1954db152 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 b9593b9905..81573f6cfd 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -53,7 +53,6 @@ typedef struct X86CPUTopoIDs {
>  } X86CPUTopoIDs;
>  
>  typedef struct X86CPUTopoInfo {
> -    unsigned nodes_per_pkg;
>      unsigned dies_per_pkg;
>      unsigned cores_per_die;
>      unsigned threads_per_core;
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 256bfa669f..ba4667b33c 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7023,7 +7023,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 5ff8ad8427..d3097be6a5 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;
>  } CPUX86State;
>  
>  struct kvm_msrs;
> diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
> index 049030a50e..bfabc0403a 100644
> --- a/tests/test-x86-cpuid.c
> +++ b/tests/test-x86-cpuid.c
> @@ -31,12 +31,12 @@ static void test_topo_bits(void)
>      X86CPUTopoInfo topo_info = {0};
>  
>      /* simple tests for 1 thread per core, 1 core per die, 1 die per package */
> -    topo_info = (X86CPUTopoInfo) {0, 1, 1, 1};
> +    topo_info = (X86CPUTopoInfo) {1, 1, 1};
>      g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 0);
>      g_assert_cmpuint(apicid_core_width(&topo_info), ==, 0);
>      g_assert_cmpuint(apicid_die_width(&topo_info), ==, 0);
>  
> -    topo_info = (X86CPUTopoInfo) {0, 1, 1, 1};
> +    topo_info = (X86CPUTopoInfo) {1, 1, 1};
>      g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 0), ==, 0);
>      g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1), ==, 1);
>      g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2), ==, 2);
> @@ -45,39 +45,39 @@ static void test_topo_bits(void)
>  
>      /* Test field width calculation for multiple values
>       */
> -    topo_info = (X86CPUTopoInfo) {0, 1, 1, 2};
> +    topo_info = (X86CPUTopoInfo) {1, 1, 2};
>      g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 1);
> -    topo_info = (X86CPUTopoInfo) {0, 1, 1, 3};
> +    topo_info = (X86CPUTopoInfo) {1, 1, 3};
>      g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 2);
> -    topo_info = (X86CPUTopoInfo) {0, 1, 1, 4};
> +    topo_info = (X86CPUTopoInfo) {1, 1, 4};
>      g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 2);
>  
> -    topo_info = (X86CPUTopoInfo) {0, 1, 1, 14};
> +    topo_info = (X86CPUTopoInfo) {1, 1, 14};
>      g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 4);
> -    topo_info = (X86CPUTopoInfo) {0, 1, 1, 15};
> +    topo_info = (X86CPUTopoInfo) {1, 1, 15};
>      g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 4);
> -    topo_info = (X86CPUTopoInfo) {0, 1, 1, 16};
> +    topo_info = (X86CPUTopoInfo) {1, 1, 16};
>      g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 4);
> -    topo_info = (X86CPUTopoInfo) {0, 1, 1, 17};
> +    topo_info = (X86CPUTopoInfo) {1, 1, 17};
>      g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 5);
>  
>  
> -    topo_info = (X86CPUTopoInfo) {0, 1, 30, 2};
> +    topo_info = (X86CPUTopoInfo) {1, 30, 2};
>      g_assert_cmpuint(apicid_core_width(&topo_info), ==, 5);
> -    topo_info = (X86CPUTopoInfo) {0, 1, 31, 2};
> +    topo_info = (X86CPUTopoInfo) {1, 31, 2};
>      g_assert_cmpuint(apicid_core_width(&topo_info), ==, 5);
> -    topo_info = (X86CPUTopoInfo) {0, 1, 32, 2};
> +    topo_info = (X86CPUTopoInfo) {1, 32, 2};
>      g_assert_cmpuint(apicid_core_width(&topo_info), ==, 5);
> -    topo_info = (X86CPUTopoInfo) {0, 1, 33, 2};
> +    topo_info = (X86CPUTopoInfo) {1, 33, 2};
>      g_assert_cmpuint(apicid_core_width(&topo_info), ==, 6);
>  
> -    topo_info = (X86CPUTopoInfo) {0, 1, 30, 2};
> +    topo_info = (X86CPUTopoInfo) {1, 30, 2};
>      g_assert_cmpuint(apicid_die_width(&topo_info), ==, 0);
> -    topo_info = (X86CPUTopoInfo) {0, 2, 30, 2};
> +    topo_info = (X86CPUTopoInfo) {2, 30, 2};
>      g_assert_cmpuint(apicid_die_width(&topo_info), ==, 1);
> -    topo_info = (X86CPUTopoInfo) {0, 3, 30, 2};
> +    topo_info = (X86CPUTopoInfo) {3, 30, 2};
>      g_assert_cmpuint(apicid_die_width(&topo_info), ==, 2);
> -    topo_info = (X86CPUTopoInfo) {0, 4, 30, 2};
> +    topo_info = (X86CPUTopoInfo) {4, 30, 2};
>      g_assert_cmpuint(apicid_die_width(&topo_info), ==, 2);
>  
>      /* build a weird topology and see if IDs are calculated correctly
> @@ -85,18 +85,18 @@ static void test_topo_bits(void)
>  
>      /* This will use 2 bits for thread ID and 3 bits for core ID
>       */
> -    topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
> +    topo_info = (X86CPUTopoInfo) {1, 6, 3};
>      g_assert_cmpuint(apicid_smt_width(&topo_info), ==, 2);
>      g_assert_cmpuint(apicid_core_offset(&topo_info), ==, 2);
>      g_assert_cmpuint(apicid_die_offset(&topo_info), ==, 5);
>      g_assert_cmpuint(apicid_pkg_offset(&topo_info), ==, 5);
>  
> -    topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
> +    topo_info = (X86CPUTopoInfo) {1, 6, 3};
>      g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 0), ==, 0);
>      g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1), ==, 1);
>      g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 2), ==, 2);
>  
> -    topo_info = (X86CPUTopoInfo) {0, 1, 6, 3};
> +    topo_info = (X86CPUTopoInfo) {1, 6, 3};
>      g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1 * 3 + 0), ==,
>                       (1 << 2) | 0);
>      g_assert_cmpuint(x86_apicid_from_cpu_idx(&topo_info, 1 * 3 + 1), ==,
> 

Re: [PATCH v6 00/10] Remove EPYC mode apicid decode and use generic decode

[Eduardo Habkost]

On Mon, Aug 31, 2020 at 01:42:04PM -0500, Babu Moger wrote:
> To support some of the complex topology, we introduced EPYC mode apicid decode.
> But, EPYC mode decode is running into problems. Also it can become quite a
> maintenance problem in the future. So, it was decided to remove that code and
> use the generic decode which works for majority of the topology. Most of the
> SPECed configuration would work just fine. With some non-SPECed user inputs,
> it will create some sub-optimal configuration.
[...]
> 

Thank you, I'm queueing patches 1-8 on machine-next.

-- 
Eduardo

Re: [PATCH v6 09/10] i386: Simplify CPUID_8000_001E for AMD

[Babu Moger]

On 9/1/20 6:52 AM, Igor Mammedov wrote:
> On Mon, 31 Aug 2020 13:43:01 -0500
> Babu Moger <babu.moger@amd.com> wrote:
> 
>> Remove all the hardcoded values and replace with generalized
>> fields.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  target/i386/cpu.c |   31 +++++++++++++++++--------------
>>  1 file changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index ba4667b33c..d434c8545a 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -395,9 +395,10 @@ static int cores_in_core_complex(int nr_cores)
>>  }
>>  
>>  /* Encode cache info for CPUID[8000001D] */
>> -static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
>> -                                uint32_t *eax, uint32_t *ebx,
>> -                                uint32_t *ecx, uint32_t *edx)
>> +static void encode_cache_cpuid8000001d(CPUCacheInfo *cache,
>> +                                       X86CPUTopoInfo *topo_info,
>> +                                       uint32_t *eax, uint32_t *ebx,
>> +                                       uint32_t *ecx, uint32_t *edx)
>>  {
>>      uint32_t l3_cores;
>>      assert(cache->size == cache->line_size * cache->associativity *
>> @@ -408,10 +409,12 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
>>  
>>      /* L3 is shared among multiple cores */
>>      if (cache->level == 3) {
>> -        l3_cores = cores_in_core_complex(cs->nr_cores);
>> -        *eax |= ((l3_cores * cs->nr_threads) - 1) << 14;
>> +        l3_cores = DIV_ROUND_UP((topo_info->cores_per_die *
>> +                                 topo_info->threads_per_core),
>> +                                 topo_info->dies_per_pkg);
> 
> from spec:
> "
> NumSharingCache: number of '''logical processors''' sharing cache.
> "
> 
> s/l3_cores/l3_vcpus|l3_threads/
> 

Sure.

> Also why not use just:
> 
>   val = topo_info->cores_per_die * topo_info->threads_per_core

Yes. You are right. Will correct it. thanks

> 
> 
> 
>> +        *eax |= (l3_cores - 1) << 14;
>>      } else {
>> -        *eax |= ((cs->nr_threads - 1) << 14);
>> +        *eax |= ((topo_info->threads_per_core - 1) << 14);
>>      }
>>  
>>      assert(cache->line_size > 0);
>> @@ -5994,20 +5997,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>          }
>>          switch (count) {
>>          case 0: /* L1 dcache info */
>> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
>> -                                       eax, ebx, ecx, edx);
>> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache,
>> +                                       &topo_info, eax, ebx, ecx, edx);
>>              break;
>>          case 1: /* L1 icache info */
>> -            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
>> -                                       eax, ebx, ecx, edx);
>> +            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache,
>> +                                       &topo_info, eax, ebx, ecx, edx);
>>              break;
>>          case 2: /* L2 cache info */
>> -            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
>> -                                       eax, ebx, ecx, edx);
>> +            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache,
>> +                                       &topo_info, eax, ebx, ecx, edx);
>>              break;
>>          case 3: /* L3 cache info */
>> -            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
>> -                                       eax, ebx, ecx, edx);
>> +            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache,
>> +                                       &topo_info, eax, ebx, ecx, edx);
>>              break;
>>          default: /* end of info */
>>              *eax = *ebx = *ecx = *edx = 0;
>>
> 

Re: [PATCH v6 10/10] i386: Simplify CPUID_8000_001E for AMD

[Eduardo Habkost]

On Mon, Aug 31, 2020 at 01:43:07PM -0500, Babu Moger wrote:
> apic_id contains all the information required to build
> CPUID_8000_001E. core_id and node_id is already part of
> apic_id generated by x86_topo_ids_from_apicid.
> 
> 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.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
[...]
> @@ -6019,7 +5914,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          break;
>      case 0x8000001E:
>          assert(cpu->core_id <= 255);

I assume we still want to remove this assert()?  Do you plan to
redo and resubmit
  Subject: [PATCH] target/i386: Remove core_id assert check in CPUID 0x8000001E
  https://lore.kernel.org/qemu-devel/159257395689.52908.4409314503988289481.stgit@naples-babu.amd.com
?

> -        encode_topo_cpuid8000001e(cs, cpu,
> +        encode_topo_cpuid8000001e(cpu, &topo_info,
>                                    eax, ebx, ecx, edx);
>          break;
>      case 0xC0000000:
> 
> 

-- 
Eduardo

Re: [PATCH v6 10/10] i386: Simplify CPUID_8000_001E for AMD

[Babu Moger]

On 9/18/20 4:38 PM, Eduardo Habkost wrote:
> On Mon, Aug 31, 2020 at 01:43:07PM -0500, Babu Moger wrote:
>> apic_id contains all the information required to build
>> CPUID_8000_001E. core_id and node_id is already part of
>> apic_id generated by x86_topo_ids_from_apicid.
>>
>> 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.
>>
>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=02%7C01%7Cbabu.moger%40amd.com%7C3d768d58412043311f8208d85c1b2432%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360619193685138&sdata=NEWhmXx4QbtPv9XVFOqw1i3AbSm74qjC%2FqZXU9E0BJ0%3D&reserved=0
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> [...]
>> @@ -6019,7 +5914,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>          break;
>>      case 0x8000001E:
>>          assert(cpu->core_id <= 255);
> 
> I assume we still want to remove this assert()?  Do you plan to
> redo and resubmit

Sure. I will resubmit it.