diff mbox series

[RFC,2,03/16] hw/i386: Introduce X86CPUTopoInfo to contain topology info

Message ID 156779711572.21957.10722611828264773686.stgit@localhost.localdomain (mailing list archive)
State New, archived
Headers show
Series APIC ID fixes for AMD EPYC CPU models | expand

Commit Message

Babu Moger Sept. 6, 2019, 7:11 p.m. UTC 
This is an effort to re-arrange few data structure for better
readability. Add X86CPUTopoInfo which will have all the topology
informations required to build the cpu topology. There is no
functional changes.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 hw/i386/pc.c               |   40 +++++++++++++++++++++++++++-------------
 include/hw/i386/topology.h |   40 ++++++++++++++++++++++++++--------------
 2 files changed, 53 insertions(+), 27 deletions(-)

Comments

Eduardo Habkost Oct. 11, 2019, 2:29 a.m. UTC | #1 
On Fri, Sep 06, 2019 at 07:11:57PM +0000, Moger, Babu wrote:
> This is an effort to re-arrange few data structure for better
> readability. Add X86CPUTopoInfo which will have all the topology
> informations required to build the cpu topology. There is no
> functional changes.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  hw/i386/pc.c               |   40 +++++++++++++++++++++++++++-------------
>  include/hw/i386/topology.h |   40 ++++++++++++++++++++++++++--------------
>  2 files changed, 53 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ada445f8f3..95aab8e5e7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -930,11 +930,15 @@ static uint32_t x86_cpu_apic_id_from_index(PCMachineState *pcms,
>  {
>      MachineState *ms = MACHINE(pcms);
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    X86CPUTopoInfo topo_info;
>      uint32_t correct_id;
>      static bool warned;
>  
> -    correct_id = x86_apicid_from_cpu_idx(pcms->smp_dies, ms->smp.cores,
> -                                         ms->smp.threads, cpu_index);
> +    topo_info.nr_dies = pcms->smp_dies;
> +    topo_info.nr_cores = ms->smp.cores;
> +    topo_info.nr_threads = ms->smp.threads;
> +
> +    correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index);

If you are using the struct in function calls, please make sure
all fields are filled correctly, so we won't introduce bugs
accidentally if we start using the new fields inside the topology
functions.

Alternatively, you can leave the struct without the numa_nodes
and nr_sockets fields by now (because they are unused), and add
the fields in another patch.

Except for this, the patch looks good.  However, I don't
understand yet the use case for the `numa_nodes` field yet.  I
will probably comment on the `numa_nodes` field once I see the
patches that use the new field.
Eduardo Habkost Oct. 11, 2019, 3:54 a.m. UTC | #2 
On Fri, Sep 06, 2019 at 07:11:57PM +0000, Moger, Babu wrote:
> This is an effort to re-arrange few data structure for better
> readability. Add X86CPUTopoInfo which will have all the topology
> informations required to build the cpu topology. There is no
> functional changes.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
[...]
> +typedef struct X86CPUTopoInfo {
> +    unsigned numa_nodes;
> +    unsigned nr_sockets;
> +    unsigned nr_dies;
> +    unsigned nr_cores;
> +    unsigned nr_threads;
> +} X86CPUTopoInfo;

With more complex topologies, the meaning of each of those fields
may be ambiguous.  e.g.: is nr_cores cores per die, cores per
ccx, or cores per socket?  Maybe we should use this opportunity
to use more explicit names like threads_per_core, cores_per_die,
dies_per_socket.
Babu Moger Dec. 2, 2019, 8:23 p.m. UTC | #3 
On 10/10/19 9:29 PM, Eduardo Habkost wrote:
> On Fri, Sep 06, 2019 at 07:11:57PM +0000, Moger, Babu wrote:
>> This is an effort to re-arrange few data structure for better
>> readability. Add X86CPUTopoInfo which will have all the topology
>> informations required to build the cpu topology. There is no
>> functional changes.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  hw/i386/pc.c               |   40 +++++++++++++++++++++++++++-------------
>>  include/hw/i386/topology.h |   40 ++++++++++++++++++++++++++--------------
>>  2 files changed, 53 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index ada445f8f3..95aab8e5e7 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -930,11 +930,15 @@ static uint32_t x86_cpu_apic_id_from_index(PCMachineState *pcms,
>>  {
>>      MachineState *ms = MACHINE(pcms);
>>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> +    X86CPUTopoInfo topo_info;
>>      uint32_t correct_id;
>>      static bool warned;
>>  
>> -    correct_id = x86_apicid_from_cpu_idx(pcms->smp_dies, ms->smp.cores,
>> -                                         ms->smp.threads, cpu_index);
>> +    topo_info.nr_dies = pcms->smp_dies;
>> +    topo_info.nr_cores = ms->smp.cores;
>> +    topo_info.nr_threads = ms->smp.threads;
>> +
>> +    correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index);
> 
> If you are using the struct in function calls, please make sure
> all fields are filled correctly, so we won't introduce bugs
> accidentally if we start using the new fields inside the topology
> functions.
> 
> Alternatively, you can leave the struct without the numa_nodes
> and nr_sockets fields by now (because they are unused), and add
> the fields in another patch.

Yes. Separated the patches and added the new fields separately.

> 
> Except for this, the patch looks good.  However, I don't
> understand yet the use case for the `numa_nodes` field yet.  I
> will probably comment on the `numa_nodes` field once I see the
> patches that use the new field.
>
Babu Moger Dec. 2, 2019, 8:25 p.m. UTC | #4 
On 10/10/19 10:54 PM, Eduardo Habkost wrote:
> On Fri, Sep 06, 2019 at 07:11:57PM +0000, Moger, Babu wrote:
>> This is an effort to re-arrange few data structure for better
>> readability. Add X86CPUTopoInfo which will have all the topology
>> informations required to build the cpu topology. There is no
>> functional changes.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> [...]
>> +typedef struct X86CPUTopoInfo {
>> +    unsigned numa_nodes;
>> +    unsigned nr_sockets;
>> +    unsigned nr_dies;
>> +    unsigned nr_cores;
>> +    unsigned nr_threads;
>> +} X86CPUTopoInfo;
> 
> With more complex topologies, the meaning of each of those fields
> may be ambiguous.  e.g.: is nr_cores cores per die, cores per
> ccx, or cores per socket?  Maybe we should use this opportunity
> to use more explicit names like threads_per_core, cores_per_die,
> dies_per_socket.

Yes. Changed it to

typedef struct X86CPUTopoInfo {
    unsigned nodes_per_pkg;
    unsigned dies_per_pkg;
    unsigned cores_per_die;
    unsigned threads_per_core;
} X86CPUTopoInfo;
diff mbox series

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ada445f8f3..95aab8e5e7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -930,11 +930,15 @@  static uint32_t x86_cpu_apic_id_from_index(PCMachineState *pcms,
 {
     MachineState *ms = MACHINE(pcms);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    X86CPUTopoInfo topo_info;
     uint32_t correct_id;
     static bool warned;
 
-    correct_id = x86_apicid_from_cpu_idx(pcms->smp_dies, ms->smp.cores,
-                                         ms->smp.threads, cpu_index);
+    topo_info.nr_dies = pcms->smp_dies;
+    topo_info.nr_cores = ms->smp.cores;
+    topo_info.nr_threads = ms->smp.threads;
+
+    correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index);
     if (pcmc->compat_apic_id_mode) {
         if (cpu_index != correct_id && !warned && !qtest_enabled()) {
             error_report("APIC IDs set in compatibility mode, "
@@ -2386,6 +2390,7 @@  static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
     unsigned int smp_cores = ms->smp.cores;
     unsigned int smp_threads = ms->smp.threads;
+    X86CPUTopoInfo topo_info;
 
     if(!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
         error_setg(errp, "Invalid CPU type, expected cpu type: '%s'",
@@ -2393,6 +2398,10 @@  static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
         return;
     }
 
+    topo_info.nr_dies = pcms->smp_dies;
+    topo_info.nr_cores = smp_cores;
+    topo_info.nr_threads = smp_threads;
+
     env->nr_dies = pcms->smp_dies;
 
     /*
@@ -2436,16 +2445,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 = apicid_from_topo_ids(pcms->smp_dies, smp_cores,
-                                            smp_threads, &topo_ids);
+        cpu->apic_id = 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);
 
-        x86_topo_ids_from_apicid(cpu->apic_id, pcms->smp_dies,
-                                 smp_cores, smp_threads, &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",
@@ -2466,8 +2473,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 */
-    x86_topo_ids_from_apicid(cpu->apic_id, pcms->smp_dies,
-                             smp_cores, smp_threads, &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, topo_ids.pkg_id);
@@ -2842,19 +2848,28 @@  static int64_t pc_get_default_cpu_node_id(const MachineState *ms, int idx)
 {
    X86CPUTopoIDs topo_ids;
    PCMachineState *pcms = PC_MACHINE(ms);
+   X86CPUTopoInfo topo_info;
+
+   topo_info.nr_dies = pcms->smp_dies;
+   topo_info.nr_cores = ms->smp.cores;
+   topo_info.nr_threads = ms->smp.threads;
 
    assert(idx < ms->possible_cpus->len);
    x86_topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id,
-                            pcms->smp_dies, ms->smp.cores,
-                            ms->smp.threads, &topo_ids);
+                            &topo_info, &topo_ids);
    return topo_ids.pkg_id % nb_numa_nodes;
 }
 
 static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *ms)
 {
     PCMachineState *pcms = PC_MACHINE(ms);
-    int i;
     unsigned int max_cpus = ms->smp.max_cpus;
+    X86CPUTopoInfo topo_info;
+    int i;
+
+    topo_info.nr_dies = pcms->smp_dies;
+    topo_info.nr_cores = ms->smp.cores;
+    topo_info.nr_threads = ms->smp.threads;
 
     if (ms->possible_cpus) {
         /*
@@ -2875,8 +2890,7 @@  static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *ms)
         ms->possible_cpus->cpus[i].vcpus_count = 1;
         ms->possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(pcms, i);
         x86_topo_ids_from_apicid(ms->possible_cpus->cpus[i].arch_id,
-                                 pcms->smp_dies, ms->smp.cores,
-                                 ms->smp.threads, &topo_ids);
+                                 &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;
         ms->possible_cpus->cpus[i].props.has_die_id = true;
diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index 0637743cdf..906017e8e3 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -54,6 +54,14 @@  typedef struct X86CPUTopoIDs {
     unsigned ccx_id;
 } X86CPUTopoIDs;
 
+typedef struct X86CPUTopoInfo {
+    unsigned numa_nodes;
+    unsigned nr_sockets;
+    unsigned nr_dies;
+    unsigned nr_cores;
+    unsigned nr_threads;
+} X86CPUTopoInfo;
+
 /* Return the bit width needed for 'count' IDs
  */
 static unsigned apicid_bitwidth_for_count(unsigned count)
@@ -121,11 +129,13 @@  static inline unsigned apicid_pkg_offset(unsigned nr_dies,
  *
  * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
  */
-static inline apic_id_t apicid_from_topo_ids(unsigned nr_dies,
-                                             unsigned nr_cores,
-                                             unsigned nr_threads,
+static inline apic_id_t apicid_from_topo_ids(X86CPUTopoInfo *topo_info,
                                              const X86CPUTopoIDs *topo_ids)
 {
+    unsigned nr_dies = topo_info->nr_dies;
+    unsigned nr_cores = topo_info->nr_cores;
+    unsigned nr_threads = topo_info->nr_threads;
+
     return (topo_ids->pkg_id  << apicid_pkg_offset(nr_dies, nr_cores, nr_threads)) |
            (topo_ids->die_id  << apicid_die_offset(nr_dies, nr_cores, nr_threads)) |
            (topo_ids->core_id << apicid_core_offset(nr_dies, nr_cores, nr_threads)) |
@@ -135,12 +145,14 @@  static inline apic_id_t apicid_from_topo_ids(unsigned nr_dies,
 /* Calculate thread/core/package IDs for a specific topology,
  * based on (contiguous) CPU index
  */
-static inline void x86_topo_ids_from_idx(unsigned nr_dies,
-                                         unsigned nr_cores,
-                                         unsigned nr_threads,
+static inline void x86_topo_ids_from_idx(X86CPUTopoInfo *topo_info,
                                          unsigned cpu_index,
                                          X86CPUTopoIDs *topo_ids)
 {
+    unsigned nr_dies = topo_info->nr_dies;
+    unsigned nr_cores = topo_info->nr_cores;
+    unsigned nr_threads = topo_info->nr_threads;
+
     topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads);
     topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies;
     topo_ids->core_id = cpu_index / nr_threads % nr_cores;
@@ -151,11 +163,13 @@  static inline void x86_topo_ids_from_idx(unsigned nr_dies,
  * based on APIC ID
  */
 static inline void x86_topo_ids_from_apicid(apic_id_t apicid,
-                                            unsigned nr_dies,
-                                            unsigned nr_cores,
-                                            unsigned nr_threads,
+                                            X86CPUTopoInfo *topo_info,
                                             X86CPUTopoIDs *topo_ids)
 {
+    unsigned nr_dies = topo_info->nr_dies;
+    unsigned nr_cores = topo_info->nr_cores;
+    unsigned nr_threads = topo_info->nr_threads;
+
     topo_ids->smt_id = apicid &
             ~(0xFFFFFFFFUL << apicid_smt_width(nr_dies, nr_cores, nr_threads));
     topo_ids->core_id =
@@ -171,14 +185,12 @@  static inline void x86_topo_ids_from_apicid(apic_id_t apicid,
  *
  * 'cpu_index' is a sequential, contiguous ID for the CPU.
  */
-static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_dies,
-                                                unsigned nr_cores,
-                                                unsigned nr_threads,
+static inline apic_id_t x86_apicid_from_cpu_idx(X86CPUTopoInfo *topo_info,
                                                 unsigned cpu_index)
 {
     X86CPUTopoIDs topo_ids;
-    x86_topo_ids_from_idx(nr_dies, nr_cores, nr_threads, cpu_index, &topo_ids);
-    return apicid_from_topo_ids(nr_dies, nr_cores, nr_threads, &topo_ids);
+    x86_topo_ids_from_idx(topo_info, cpu_index, &topo_ids);
+    return apicid_from_topo_ids(topo_info, &topo_ids);
 }
 
 #endif /* HW_I386_TOPOLOGY_H */