diff mbox series

[RFC,2,16/16] hw/core: Fix up the machine_set_cpu_numa_node for epyc

Message ID 156779720803.21957.8389712174989601936.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:13 p.m. UTC
Current topology id match will not work for epyc mode when setting
the node id. In epyc mode, ids like smt_id, thread_id, core_id,
ccx_id, socket_id can be same for more than one CPUs with across
two numa nodes.

For example, we can have two CPUs with following ids on two different node.
1. smt_id=0, thread_id=0, core_id=0, ccx_id=0, socket_id=0, node_id=0
2. smt_id=0, thread_id=0, core_id=0, ccx_id=0, socket_id=0, node_id=1

The function machine_set_cpu_numa_node will fail to find a match to assign
the node. Added new function machine_set_cpu_numa_node_epyc to set the node_id
directly in epyc mode.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 hw/core/machine.c   |   24 ++++++++++++++++++++++++
 hw/core/numa.c      |    6 +++++-
 include/hw/boards.h |    4 ++++
 3 files changed, 33 insertions(+), 1 deletion(-)

Comments

Eduardo Habkost Oct. 11, 2019, 4:10 a.m. UTC | #1
On Fri, Sep 06, 2019 at 07:13:29PM +0000, Moger, Babu wrote:
> Current topology id match will not work for epyc mode when setting
> the node id. In epyc mode, ids like smt_id, thread_id, core_id,
> ccx_id, socket_id can be same for more than one CPUs with across
> two numa nodes.
> 
> For example, we can have two CPUs with following ids on two different node.
> 1. smt_id=0, thread_id=0, core_id=0, ccx_id=0, socket_id=0, node_id=0
> 2. smt_id=0, thread_id=0, core_id=0, ccx_id=0, socket_id=0, node_id=1

I don't understand how that could happen.  If all IDs are the
same, how is it possible that those two cases should match two
different CPUs?

cpu_index_to_instance_props() must always return an unique
identifier for a single CPU.

> 
> The function machine_set_cpu_numa_node will fail to find a match to assign
> the node. Added new function machine_set_cpu_numa_node_epyc to set the node_id
> directly in epyc mode.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  hw/core/machine.c   |   24 ++++++++++++++++++++++++
>  hw/core/numa.c      |    6 +++++-
>  include/hw/boards.h |    4 ++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 9a8586cf30..6bceefc6f3 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -741,6 +741,30 @@ void machine_set_cpu_numa_node(MachineState *machine,
>      }
>  }
>  
> +void machine_set_cpu_numa_node_epyc(MachineState *machine,
> +                                    const CpuInstanceProperties *props,
> +                                    unsigned index,
> +                                    Error **errp)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    CPUArchId *slot;
> +
> +    if (!mc->possible_cpu_arch_ids) {
> +        error_setg(errp, "mapping of CPUs to NUMA node is not supported");
> +        return;
> +    }
> +
> +    /* disabling node mapping is not supported, forbid it */
> +    assert(props->has_node_id);
> +
> +    /* force board to initialize possible_cpus if it hasn't been done yet */
> +    mc->possible_cpu_arch_ids(machine);
> +
> +    slot = &machine->possible_cpus->cpus[index];
> +    slot->props.node_id = props->node_id;
> +    slot->props.has_node_id = props->has_node_id;
> +}
> +
>  static void smp_parse(MachineState *ms, QemuOpts *opts)
>  {
>      if (opts) {
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index 27fa6b5e1d..a9e835aea6 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -247,7 +247,11 @@ void set_numa_node_options(MachineState *ms, NumaOptions *object, Error **errp)
>           props = mc->cpu_index_to_instance_props(ms, cpus->value);
>           props.node_id = nodenr;
>           props.has_node_id = true;
> -         machine_set_cpu_numa_node(ms, &props, &err);
> +         if (ms->epyc) {
> +             machine_set_cpu_numa_node_epyc(ms, &props, cpus->value, &err);
> +         } else {
> +             machine_set_cpu_numa_node(ms, &props, &err);
> +         }
>           if (err) {
>              error_propagate(errp, err);
>              return;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 0001d42e50..ec1b1c5a85 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -74,6 +74,10 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
>  void machine_set_cpu_numa_node(MachineState *machine,
>                                 const CpuInstanceProperties *props,
>                                 Error **errp);
> +void machine_set_cpu_numa_node_epyc(MachineState *machine,
> +                                    const CpuInstanceProperties *props,
> +                                    unsigned index,
> +                                    Error **errp);
>  
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
>  
>
Babu Moger Dec. 2, 2019, 8:44 p.m. UTC | #2
On 10/10/19 11:10 PM, Eduardo Habkost wrote:
> On Fri, Sep 06, 2019 at 07:13:29PM +0000, Moger, Babu wrote:
>> Current topology id match will not work for epyc mode when setting
>> the node id. In epyc mode, ids like smt_id, thread_id, core_id,
>> ccx_id, socket_id can be same for more than one CPUs with across
>> two numa nodes.
>>
>> For example, we can have two CPUs with following ids on two different node.
>> 1. smt_id=0, thread_id=0, core_id=0, ccx_id=0, socket_id=0, node_id=0
>> 2. smt_id=0, thread_id=0, core_id=0, ccx_id=0, socket_id=0, node_id=1
> 
> I don't understand how that could happen.  If all IDs are the
> same, how is it possible that those two cases should match two
> different CPUs?
> 
> cpu_index_to_instance_props() must always return an unique
> identifier for a single CPU.

Simplified apic id decoding in v3 version. We don't need these changes
anymore.

> 
>>
>> The function machine_set_cpu_numa_node will fail to find a match to assign
>> the node. Added new function machine_set_cpu_numa_node_epyc to set the node_id
>> directly in epyc mode.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  hw/core/machine.c   |   24 ++++++++++++++++++++++++
>>  hw/core/numa.c      |    6 +++++-
>>  include/hw/boards.h |    4 ++++
>>  3 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 9a8586cf30..6bceefc6f3 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -741,6 +741,30 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>      }
>>  }
>>  
>> +void machine_set_cpu_numa_node_epyc(MachineState *machine,
>> +                                    const CpuInstanceProperties *props,
>> +                                    unsigned index,
>> +                                    Error **errp)
>> +{
>> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
>> +    CPUArchId *slot;
>> +
>> +    if (!mc->possible_cpu_arch_ids) {
>> +        error_setg(errp, "mapping of CPUs to NUMA node is not supported");
>> +        return;
>> +    }
>> +
>> +    /* disabling node mapping is not supported, forbid it */
>> +    assert(props->has_node_id);
>> +
>> +    /* force board to initialize possible_cpus if it hasn't been done yet */
>> +    mc->possible_cpu_arch_ids(machine);
>> +
>> +    slot = &machine->possible_cpus->cpus[index];
>> +    slot->props.node_id = props->node_id;
>> +    slot->props.has_node_id = props->has_node_id;
>> +}
>> +
>>  static void smp_parse(MachineState *ms, QemuOpts *opts)
>>  {
>>      if (opts) {
>> diff --git a/hw/core/numa.c b/hw/core/numa.c
>> index 27fa6b5e1d..a9e835aea6 100644
>> --- a/hw/core/numa.c
>> +++ b/hw/core/numa.c
>> @@ -247,7 +247,11 @@ void set_numa_node_options(MachineState *ms, NumaOptions *object, Error **errp)
>>           props = mc->cpu_index_to_instance_props(ms, cpus->value);
>>           props.node_id = nodenr;
>>           props.has_node_id = true;
>> -         machine_set_cpu_numa_node(ms, &props, &err);
>> +         if (ms->epyc) {
>> +             machine_set_cpu_numa_node_epyc(ms, &props, cpus->value, &err);
>> +         } else {
>> +             machine_set_cpu_numa_node(ms, &props, &err);
>> +         }
>>           if (err) {
>>              error_propagate(errp, err);
>>              return;
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 0001d42e50..ec1b1c5a85 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -74,6 +74,10 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
>>  void machine_set_cpu_numa_node(MachineState *machine,
>>                                 const CpuInstanceProperties *props,
>>                                 Error **errp);
>> +void machine_set_cpu_numa_node_epyc(MachineState *machine,
>> +                                    const CpuInstanceProperties *props,
>> +                                    unsigned index,
>> +                                    Error **errp);
>>  
>>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
>>  
>>
>
diff mbox series

Patch

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 9a8586cf30..6bceefc6f3 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -741,6 +741,30 @@  void machine_set_cpu_numa_node(MachineState *machine,
     }
 }
 
+void machine_set_cpu_numa_node_epyc(MachineState *machine,
+                                    const CpuInstanceProperties *props,
+                                    unsigned index,
+                                    Error **errp)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    CPUArchId *slot;
+
+    if (!mc->possible_cpu_arch_ids) {
+        error_setg(errp, "mapping of CPUs to NUMA node is not supported");
+        return;
+    }
+
+    /* disabling node mapping is not supported, forbid it */
+    assert(props->has_node_id);
+
+    /* force board to initialize possible_cpus if it hasn't been done yet */
+    mc->possible_cpu_arch_ids(machine);
+
+    slot = &machine->possible_cpus->cpus[index];
+    slot->props.node_id = props->node_id;
+    slot->props.has_node_id = props->has_node_id;
+}
+
 static void smp_parse(MachineState *ms, QemuOpts *opts)
 {
     if (opts) {
diff --git a/hw/core/numa.c b/hw/core/numa.c
index 27fa6b5e1d..a9e835aea6 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -247,7 +247,11 @@  void set_numa_node_options(MachineState *ms, NumaOptions *object, Error **errp)
          props = mc->cpu_index_to_instance_props(ms, cpus->value);
          props.node_id = nodenr;
          props.has_node_id = true;
-         machine_set_cpu_numa_node(ms, &props, &err);
+         if (ms->epyc) {
+             machine_set_cpu_numa_node_epyc(ms, &props, cpus->value, &err);
+         } else {
+             machine_set_cpu_numa_node(ms, &props, &err);
+         }
          if (err) {
             error_propagate(errp, err);
             return;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 0001d42e50..ec1b1c5a85 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -74,6 +74,10 @@  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
 void machine_set_cpu_numa_node(MachineState *machine,
                                const CpuInstanceProperties *props,
                                Error **errp);
+void machine_set_cpu_numa_node_epyc(MachineState *machine,
+                                    const CpuInstanceProperties *props,
+                                    unsigned index,
+                                    Error **errp);
 
 void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);