[PATCH v6 0/4] hw/arm/virt: Fix CPU's default NUMA node ID

[PATCH v6 0/4] hw/arm/virt: Fix CPU's default NUMA node ID

[Gavin Shan]

When the CPU-to-NUMA association isn't provided by user, the default NUMA
node ID for the specific CPU is returned from virt_get_default_cpu_node_id().
Unfortunately, the default NUMA node ID breaks socket boundary and leads to
the broken CPU topology warning message in Linux guest. This series intends
to fix the issue.

  PATCH[1/4] Add cluster-id to CPU instance property
  PATCH[2/4] Uses SMP configuration to populate CPU topology
  PATCH[3/4] Fixes the broken CPU topology by considering the socket boundary
             when the default NUMA node ID is given
  PATCH[4/4] Uses the populated CPU topology to build PPTT table, instead of
             calculate it again

Changelog
=========
v6:
   * Fixed description for 'cluster-id'                         (Yanan)
   * Remove '% ms->smp.sockets' in socket ID calculation        (Yanan)
   * Fixed tests/qtest/numa-test/aarch64_numa_cpu()             (Yanan)
   * Initialized offset variables in build_pptt()               (Jonathan)
   * Added comments about the layout of cpus[n].props.*_id      (Igor)
v4/v5:
   * Split PATCH[v3 1/3] to PATCH[v5 1/4] and PATCH[v5 2/4].
     Verify or dump 'clsuter-id' in various spots               (Yanan)
   * s/within cluster/within cluster\/die/ for 'core-id' in
     qapi/machine.json                                          (Igor)
   * Apply '% ms->smp.{sockets, clusters, cores, threads} in
     virt_possible_cpu_arch_ids() as x86 does                   (Igor)
   * Use [0 - possible_cpus->len] as ACPI processor UID to
     build PPTT table and PATCH[v3 4/4] is dropped              (Igor)
   * Simplified build_pptt() to add all entries in one loop
     on ms->possible_cpus                                       (Igor)
v3:
   * Split PATCH[v2 1/3] to PATCH[v3 1/4] and PATCH[v3 2/4]     (Yanan)
   * Don't take account of die ID in CPU topology population
     and added assert(!mc->smp_props.dies_supported)            (Yanan/Igor)
   * Assign cluster_id and use it when building PPTT table      (Yanan/Igor)
v2:
   * Populate the CPU topology in virt_possible_cpu_arch_ids()
     so that it can be reused in virt_get_default_cpu_node_id() (Igor)
   * Added PATCH[2/3] to use the existing CPU topology when the
     PPTT table is built                                        (Igor)
   * Added PATCH[3/3] to take thread ID as ACPI processor ID
     in MADT and SRAT table                                     (Gavin)

Gavin Shan (4):
  qapi/machine.json: Add cluster-id
  hw/arm/virt: Consider SMP configuration in CPU topology
  hw/arm/virt: Fix CPU's default NUMA node ID
  hw/acpi/aml-build: Use existing CPU topology to build PPTT table

 hw/acpi/aml-build.c        | 105 +++++++++++++++----------------------
 hw/arm/virt.c              |  19 ++++++-
 hw/core/machine-hmp-cmds.c |   4 ++
 hw/core/machine.c          |  16 ++++++
 qapi/machine.json          |   6 ++-
 tests/qtest/numa-test.c    |   3 +-
 6 files changed, 86 insertions(+), 67 deletions(-)

-- 
2.23.0

[PATCH v6 1/4] qapi/machine.json: Add cluster-id

[Gavin Shan]

This adds cluster-id in CPU instance properties, which will be used
by arm/virt machine. Besides, the cluster-id is also verified or
dumped in various spots:

  * hw/core/machine.c::machine_set_cpu_numa_node() to associate
    CPU with its NUMA node.

  * hw/core/machine.c::machine_numa_finish_cpu_init() to associate
    CPU with NUMA node when no default association isn't provided.

  * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
    cluster-id.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine-hmp-cmds.c |  4 ++++
 hw/core/machine.c          | 16 ++++++++++++++++
 qapi/machine.json          |  6 ++++--
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
index 4e2f319aeb..5cb5eecbfc 100644
--- a/hw/core/machine-hmp-cmds.c
+++ b/hw/core/machine-hmp-cmds.c
@@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
         if (c->has_die_id) {
             monitor_printf(mon, "    die-id: \"%" PRIu64 "\"\n", c->die_id);
         }
+        if (c->has_cluster_id) {
+            monitor_printf(mon, "    cluster-id: \"%" PRIu64 "\"\n",
+                           c->cluster_id);
+        }
         if (c->has_core_id) {
             monitor_printf(mon, "    core-id: \"%" PRIu64 "\"\n", c->core_id);
         }
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1e23fdc14b..ac91dfd834 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -679,6 +679,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
             return;
         }
 
+        if (props->has_cluster_id && !slot->props.has_cluster_id) {
+            error_setg(errp, "cluster-id is not supported");
+            return;
+        }
+
         if (props->has_socket_id && !slot->props.has_socket_id) {
             error_setg(errp, "socket-id is not supported");
             return;
@@ -698,6 +703,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
                 continue;
         }
 
+        if (props->has_cluster_id &&
+            props->cluster_id != slot->props.cluster_id) {
+                continue;
+        }
+
         if (props->has_die_id && props->die_id != slot->props.die_id) {
                 continue;
         }
@@ -992,6 +1002,12 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
         }
         g_string_append_printf(s, "die-id: %"PRId64, cpu->props.die_id);
     }
+    if (cpu->props.has_cluster_id) {
+        if (s->len) {
+            g_string_append_printf(s, ", ");
+        }
+        g_string_append_printf(s, "cluster-id: %"PRId64, cpu->props.cluster_id);
+    }
     if (cpu->props.has_core_id) {
         if (s->len) {
             g_string_append_printf(s, ", ");
diff --git a/qapi/machine.json b/qapi/machine.json
index d25a481ce4..fb855041ee 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -868,10 +868,11 @@
 # @node-id: NUMA node ID the CPU belongs to
 # @socket-id: socket number within node/board the CPU belongs to
 # @die-id: die number within socket the CPU belongs to (since 4.1)
-# @core-id: core number within die the CPU belongs to
+# @cluster-id: cluster number within socket the CPU belongs to (since 7.1)
+# @core-id: core number within cluster/die the CPU belongs to
 # @thread-id: thread number within core the CPU belongs to
 #
-# Note: currently there are 5 properties that could be present
+# Note: currently there are 6 properties that could be present
 #       but management should be prepared to pass through other
 #       properties with device_add command to allow for future
 #       interface extension. This also requires the filed names to be kept in
@@ -883,6 +884,7 @@
   'data': { '*node-id': 'int',
             '*socket-id': 'int',
             '*die-id': 'int',
+            '*cluster-id': 'int',
             '*core-id': 'int',
             '*thread-id': 'int'
   }
-- 
2.23.0

[PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Gavin Shan]

Currently, the SMP configuration isn't considered when the CPU
topology is populated. In this case, it's impossible to provide
the default CPU-to-NUMA mapping or association based on the socket
ID of the given CPU.

This takes account of SMP configuration when the CPU topology
is populated. The die ID for the given CPU isn't assigned since
it's not supported on arm/virt machine. Besides, the used SMP
configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted
to avoid testing failure

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c           | 15 ++++++++++++++-
 tests/qtest/numa-test.c |  3 ++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d2e5ecd234..5443ecae92 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2505,6 +2505,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
     int n;
     unsigned int max_cpus = ms->smp.max_cpus;
     VirtMachineState *vms = VIRT_MACHINE(ms);
+    MachineClass *mc = MACHINE_GET_CLASS(vms);
 
     if (ms->possible_cpus) {
         assert(ms->possible_cpus->len == max_cpus);
@@ -2518,8 +2519,20 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
         ms->possible_cpus->cpus[n].type = ms->cpu_type;
         ms->possible_cpus->cpus[n].arch_id =
             virt_cpu_mp_affinity(vms, n);
+
+        assert(!mc->smp_props.dies_supported);
+        ms->possible_cpus->cpus[n].props.has_socket_id = true;
+        ms->possible_cpus->cpus[n].props.socket_id =
+            (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads));
+        ms->possible_cpus->cpus[n].props.has_cluster_id = true;
+        ms->possible_cpus->cpus[n].props.cluster_id =
+            (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters;
+        ms->possible_cpus->cpus[n].props.has_core_id = true;
+        ms->possible_cpus->cpus[n].props.core_id =
+            (n / ms->smp.threads) % ms->smp.cores;
         ms->possible_cpus->cpus[n].props.has_thread_id = true;
-        ms->possible_cpus->cpus[n].props.thread_id = n;
+        ms->possible_cpus->cpus[n].props.thread_id =
+            n % ms->smp.threads;
     }
     return ms->possible_cpus;
 }
diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
index 90bf68a5b3..aeda8c774c 100644
--- a/tests/qtest/numa-test.c
+++ b/tests/qtest/numa-test.c
@@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data)
     QTestState *qts;
     g_autofree char *cli = NULL;
 
-    cli = make_cli(data, "-machine smp.cpus=2 "
+    cli = make_cli(data, "-machine "
+        "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 "
         "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
         "-numa cpu,node-id=1,thread-id=0 "
         "-numa cpu,node-id=0,thread-id=1");
-- 
2.23.0

Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Igor Mammedov]

On Mon, 18 Apr 2022 10:09:18 +0800
Gavin Shan <gshan@redhat.com> wrote:

> Currently, the SMP configuration isn't considered when the CPU
> topology is populated. In this case, it's impossible to provide
> the default CPU-to-NUMA mapping or association based on the socket
> ID of the given CPU.
> 
> This takes account of SMP configuration when the CPU topology
> is populated. The die ID for the given CPU isn't assigned since
> it's not supported on arm/virt machine. Besides, the used SMP
> configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted
> to avoid testing failure
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt.c           | 15 ++++++++++++++-
>  tests/qtest/numa-test.c |  3 ++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d2e5ecd234..5443ecae92 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2505,6 +2505,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>      int n;
>      unsigned int max_cpus = ms->smp.max_cpus;
>      VirtMachineState *vms = VIRT_MACHINE(ms);
> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
>  
>      if (ms->possible_cpus) {
>          assert(ms->possible_cpus->len == max_cpus);
> @@ -2518,8 +2519,20 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>          ms->possible_cpus->cpus[n].type = ms->cpu_type;
>          ms->possible_cpus->cpus[n].arch_id =
>              virt_cpu_mp_affinity(vms, n);
> +
> +        assert(!mc->smp_props.dies_supported);
> +        ms->possible_cpus->cpus[n].props.has_socket_id = true;
> +        ms->possible_cpus->cpus[n].props.socket_id =
> +            (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads));
> +        ms->possible_cpus->cpus[n].props.has_cluster_id = true;
> +        ms->possible_cpus->cpus[n].props.cluster_id =
> +            (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters;
> +        ms->possible_cpus->cpus[n].props.has_core_id = true;
> +        ms->possible_cpus->cpus[n].props.core_id =
> +            (n / ms->smp.threads) % ms->smp.cores;
>          ms->possible_cpus->cpus[n].props.has_thread_id = true;
> -        ms->possible_cpus->cpus[n].props.thread_id = n;
> +        ms->possible_cpus->cpus[n].props.thread_id =
> +            n % ms->smp.threads;
>      }
>      return ms->possible_cpus;
>  }
> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> index 90bf68a5b3..aeda8c774c 100644
> --- a/tests/qtest/numa-test.c
> +++ b/tests/qtest/numa-test.c
> @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data)
>      QTestState *qts;
>      g_autofree char *cli = NULL;
>  
> -    cli = make_cli(data, "-machine smp.cpus=2 "
> +    cli = make_cli(data, "-machine "
> +        "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 "

Is cluster-less config possible?
(looks like it used to work before and it doesn't after this series)

>          "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
>          "-numa cpu,node-id=1,thread-id=0 "
>          "-numa cpu,node-id=0,thread-id=1");

Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Gavin Shan]

Hi Igor,

On 4/20/22 4:32 PM, Igor Mammedov wrote:
> On Mon, 18 Apr 2022 10:09:18 +0800
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> Currently, the SMP configuration isn't considered when the CPU
>> topology is populated. In this case, it's impossible to provide
>> the default CPU-to-NUMA mapping or association based on the socket
>> ID of the given CPU.
>>
>> This takes account of SMP configuration when the CPU topology
>> is populated. The die ID for the given CPU isn't assigned since
>> it's not supported on arm/virt machine. Besides, the used SMP
>> configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted
>> to avoid testing failure
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/arm/virt.c           | 15 ++++++++++++++-
>>   tests/qtest/numa-test.c |  3 ++-
>>   2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index d2e5ecd234..5443ecae92 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -2505,6 +2505,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>       int n;
>>       unsigned int max_cpus = ms->smp.max_cpus;
>>       VirtMachineState *vms = VIRT_MACHINE(ms);
>> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
>>   
>>       if (ms->possible_cpus) {
>>           assert(ms->possible_cpus->len == max_cpus);
>> @@ -2518,8 +2519,20 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>           ms->possible_cpus->cpus[n].type = ms->cpu_type;
>>           ms->possible_cpus->cpus[n].arch_id =
>>               virt_cpu_mp_affinity(vms, n);
>> +
>> +        assert(!mc->smp_props.dies_supported);
>> +        ms->possible_cpus->cpus[n].props.has_socket_id = true;
>> +        ms->possible_cpus->cpus[n].props.socket_id =
>> +            (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads));
>> +        ms->possible_cpus->cpus[n].props.has_cluster_id = true;
>> +        ms->possible_cpus->cpus[n].props.cluster_id =
>> +            (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters;
>> +        ms->possible_cpus->cpus[n].props.has_core_id = true;
>> +        ms->possible_cpus->cpus[n].props.core_id =
>> +            (n / ms->smp.threads) % ms->smp.cores;
>>           ms->possible_cpus->cpus[n].props.has_thread_id = true;
>> -        ms->possible_cpus->cpus[n].props.thread_id = n;
>> +        ms->possible_cpus->cpus[n].props.thread_id =
>> +            n % ms->smp.threads;
>>       }
>>       return ms->possible_cpus;
>>   }
>> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
>> index 90bf68a5b3..aeda8c774c 100644
>> --- a/tests/qtest/numa-test.c
>> +++ b/tests/qtest/numa-test.c
>> @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data)
>>       QTestState *qts;
>>       g_autofree char *cli = NULL;
>>   
>> -    cli = make_cli(data, "-machine smp.cpus=2 "
>> +    cli = make_cli(data, "-machine "
>> +        "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 "
> 
> Is cluster-less config possible?
> (looks like it used to work before and it doesn't after this series)
> 

Nope, it's impossible. This specific test case uses arm/virt machine
where cluster is always supported.mc->smp_props.clusters_supported
has been set to true in hw/arm/virt.c::virt_machine_class_init().

Exactly, the changes to virt_possible_cpu_arch_ids() included in this patch breaks
the test. It's why the fix to qtest/numa-test has been squashed to this patch, to
make it 'bit bisect' friendly as Yanan suggested.


>>           "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
>>           "-numa cpu,node-id=1,thread-id=0 "
>>           "-numa cpu,node-id=0,thread-id=1");

Thanks,
Gavin

Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Igor Mammedov]

On Wed, 20 Apr 2022 18:31:02 +0800
Gavin Shan <gshan@redhat.com> wrote:

> Hi Igor,
> 
> On 4/20/22 4:32 PM, Igor Mammedov wrote:
> > On Mon, 18 Apr 2022 10:09:18 +0800
> > Gavin Shan <gshan@redhat.com> wrote:
> >   
> >> Currently, the SMP configuration isn't considered when the CPU
> >> topology is populated. In this case, it's impossible to provide
> >> the default CPU-to-NUMA mapping or association based on the socket
> >> ID of the given CPU.
> >>
> >> This takes account of SMP configuration when the CPU topology
> >> is populated. The die ID for the given CPU isn't assigned since
> >> it's not supported on arm/virt machine. Besides, the used SMP
> >> configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted
> >> to avoid testing failure
> >>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> >> ---
> >>   hw/arm/virt.c           | 15 ++++++++++++++-
> >>   tests/qtest/numa-test.c |  3 ++-
> >>   2 files changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index d2e5ecd234..5443ecae92 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -2505,6 +2505,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >>       int n;
> >>       unsigned int max_cpus = ms->smp.max_cpus;
> >>       VirtMachineState *vms = VIRT_MACHINE(ms);
> >> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
> >>   
> >>       if (ms->possible_cpus) {
> >>           assert(ms->possible_cpus->len == max_cpus);
> >> @@ -2518,8 +2519,20 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >>           ms->possible_cpus->cpus[n].type = ms->cpu_type;
> >>           ms->possible_cpus->cpus[n].arch_id =
> >>               virt_cpu_mp_affinity(vms, n);
> >> +
> >> +        assert(!mc->smp_props.dies_supported);
> >> +        ms->possible_cpus->cpus[n].props.has_socket_id = true;
> >> +        ms->possible_cpus->cpus[n].props.socket_id =
> >> +            (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads));
> >> +        ms->possible_cpus->cpus[n].props.has_cluster_id = true;
> >> +        ms->possible_cpus->cpus[n].props.cluster_id =
> >> +            (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters;
> >> +        ms->possible_cpus->cpus[n].props.has_core_id = true;
> >> +        ms->possible_cpus->cpus[n].props.core_id =
> >> +            (n / ms->smp.threads) % ms->smp.cores;
> >>           ms->possible_cpus->cpus[n].props.has_thread_id = true;
> >> -        ms->possible_cpus->cpus[n].props.thread_id = n;
> >> +        ms->possible_cpus->cpus[n].props.thread_id =
> >> +            n % ms->smp.threads;
> >>       }
> >>       return ms->possible_cpus;
> >>   }
> >> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> >> index 90bf68a5b3..aeda8c774c 100644
> >> --- a/tests/qtest/numa-test.c
> >> +++ b/tests/qtest/numa-test.c
> >> @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data)
> >>       QTestState *qts;
> >>       g_autofree char *cli = NULL;
> >>   
> >> -    cli = make_cli(data, "-machine smp.cpus=2 "
> >> +    cli = make_cli(data, "-machine "
> >> +        "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 "  
> > 
> > Is cluster-less config possible?
> > (looks like it used to work before and it doesn't after this series)
> >   
> 
> Nope, it's impossible. This specific test case uses arm/virt machine
> where cluster is always supported.mc->smp_props.clusters_supported
> has been set to true in hw/arm/virt.c::virt_machine_class_init().
> 
> Exactly, the changes to virt_possible_cpu_arch_ids() included in this patch breaks
> the test. It's why the fix to qtest/numa-test has been squashed to this patch, to
> make it 'bit bisect' friendly as Yanan suggested.

so what was error that broke the test?
(probably should be mentioned in commit message)

(also is it possible to split out the test patch into
a separate one and put it before this one)


> 
> 
> >>           "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
> >>           "-numa cpu,node-id=1,thread-id=0 "
> >>           "-numa cpu,node-id=0,thread-id=1");  
> 
> Thanks,
> Gavin
> 

Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Gavin Shan]

Hi Igor,

On 4/20/22 7:50 PM, Igor Mammedov wrote:
> On Wed, 20 Apr 2022 18:31:02 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> On 4/20/22 4:32 PM, Igor Mammedov wrote:
>>> On Mon, 18 Apr 2022 10:09:18 +0800
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>    
>>>> Currently, the SMP configuration isn't considered when the CPU
>>>> topology is populated. In this case, it's impossible to provide
>>>> the default CPU-to-NUMA mapping or association based on the socket
>>>> ID of the given CPU.
>>>>
>>>> This takes account of SMP configuration when the CPU topology
>>>> is populated. The die ID for the given CPU isn't assigned since
>>>> it's not supported on arm/virt machine. Besides, the used SMP
>>>> configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted
>>>> to avoid testing failure
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>> ---
>>>>    hw/arm/virt.c           | 15 ++++++++++++++-
>>>>    tests/qtest/numa-test.c |  3 ++-
>>>>    2 files changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>> index d2e5ecd234..5443ecae92 100644
>>>> --- a/hw/arm/virt.c
>>>> +++ b/hw/arm/virt.c
>>>> @@ -2505,6 +2505,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>>        int n;
>>>>        unsigned int max_cpus = ms->smp.max_cpus;
>>>>        VirtMachineState *vms = VIRT_MACHINE(ms);
>>>> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
>>>>    
>>>>        if (ms->possible_cpus) {
>>>>            assert(ms->possible_cpus->len == max_cpus);
>>>> @@ -2518,8 +2519,20 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>>            ms->possible_cpus->cpus[n].type = ms->cpu_type;
>>>>            ms->possible_cpus->cpus[n].arch_id =
>>>>                virt_cpu_mp_affinity(vms, n);
>>>> +
>>>> +        assert(!mc->smp_props.dies_supported);
>>>> +        ms->possible_cpus->cpus[n].props.has_socket_id = true;
>>>> +        ms->possible_cpus->cpus[n].props.socket_id =
>>>> +            (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads));
>>>> +        ms->possible_cpus->cpus[n].props.has_cluster_id = true;
>>>> +        ms->possible_cpus->cpus[n].props.cluster_id =
>>>> +            (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters;
>>>> +        ms->possible_cpus->cpus[n].props.has_core_id = true;
>>>> +        ms->possible_cpus->cpus[n].props.core_id =
>>>> +            (n / ms->smp.threads) % ms->smp.cores;
>>>>            ms->possible_cpus->cpus[n].props.has_thread_id = true;
>>>> -        ms->possible_cpus->cpus[n].props.thread_id = n;
>>>> +        ms->possible_cpus->cpus[n].props.thread_id =
>>>> +            n % ms->smp.threads;
>>>>        }
>>>>        return ms->possible_cpus;
>>>>    }
>>>> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
>>>> index 90bf68a5b3..aeda8c774c 100644
>>>> --- a/tests/qtest/numa-test.c
>>>> +++ b/tests/qtest/numa-test.c
>>>> @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data)
>>>>        QTestState *qts;
>>>>        g_autofree char *cli = NULL;
>>>>    
>>>> -    cli = make_cli(data, "-machine smp.cpus=2 "
>>>> +    cli = make_cli(data, "-machine "
>>>> +        "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 "
>>>
>>> Is cluster-less config possible?
>>> (looks like it used to work before and it doesn't after this series)
>>>    
>>
>> Nope, it's impossible. This specific test case uses arm/virt machine
>> where cluster is always supported.mc->smp_props.clusters_supported
>> has been set to true in hw/arm/virt.c::virt_machine_class_init().
>>
>> Exactly, the changes to virt_possible_cpu_arch_ids() included in this patch breaks
>> the test. It's why the fix to qtest/numa-test has been squashed to this patch, to
>> make it 'bit bisect' friendly as Yanan suggested.
> 
> so what was error that broke the test?
> (probably should be mentioned in commit message)
> 
> (also is it possible to split out the test patch into
> a separate one and put it before this one)
> 

With amend to the command lines, the following one is used and below error
is raised from the test. The error is mentioned in the commit log in
PATCH[v7 2/4].

     -machine smp.cpus=2                                   \
     -numa node,nodeid=0,memdev=ram -numa node,nodeid=1    \
     -numa cpu,node-id=1,thread-id=0                       \
     -numa cpu,node-id=0,thread-id=1

     qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found
     (reported from hw/core/machine.c::machine_set_cpu_numa_node())

After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1"
isn't valid any more. The CPU topology becomes like below. Note that
mc->smp_props.prefer_sockets is true on arm/virt machine.

     index    socket   cluster    core    thread
     --------------------------------------------
       0        0        0         0        0
       1        1        0         0        0

With the amended command lines, the topology changes again so
that "thread-id=1" is valid:

     index    socket   cluster    core    thread
     --------------------------------------------
       0        0        0         0        0
       1        0        0         0        1

It should be ok to split the test/qtest/aarch64_numa_cpu() changes into
a separate patch and put it before this one. In that case, the specified
smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet,
and 'thread-id=2' should be still valid. Lets do this if I need post v8.
Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu()
changes into PATCH[2/4], as we're doing.

> 
>>
>>
>>>>            "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
>>>>            "-numa cpu,node-id=1,thread-id=0 "
>>>>            "-numa cpu,node-id=0,thread-id=1");

Thanks,
Gavin

Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Igor Mammedov]

On Wed, 20 Apr 2022 22:24:46 +0800
Gavin Shan <gshan@redhat.com> wrote:

> Hi Igor,
> 
> On 4/20/22 7:50 PM, Igor Mammedov wrote:
> > On Wed, 20 Apr 2022 18:31:02 +0800
> > Gavin Shan <gshan@redhat.com> wrote:  
> >> On 4/20/22 4:32 PM, Igor Mammedov wrote:  
> >>> On Mon, 18 Apr 2022 10:09:18 +0800
> >>> Gavin Shan <gshan@redhat.com> wrote:
> >>>      
> >>>> Currently, the SMP configuration isn't considered when the CPU
> >>>> topology is populated. In this case, it's impossible to provide
> >>>> the default CPU-to-NUMA mapping or association based on the socket
> >>>> ID of the given CPU.
> >>>>
> >>>> This takes account of SMP configuration when the CPU topology
> >>>> is populated. The die ID for the given CPU isn't assigned since
> >>>> it's not supported on arm/virt machine. Besides, the used SMP
> >>>> configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted
> >>>> to avoid testing failure
> >>>>
> >>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> >>>> ---
> >>>>    hw/arm/virt.c           | 15 ++++++++++++++-
> >>>>    tests/qtest/numa-test.c |  3 ++-
> >>>>    2 files changed, 16 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >>>> index d2e5ecd234..5443ecae92 100644
> >>>> --- a/hw/arm/virt.c
> >>>> +++ b/hw/arm/virt.c
> >>>> @@ -2505,6 +2505,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >>>>        int n;
> >>>>        unsigned int max_cpus = ms->smp.max_cpus;
> >>>>        VirtMachineState *vms = VIRT_MACHINE(ms);
> >>>> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
> >>>>    
> >>>>        if (ms->possible_cpus) {
> >>>>            assert(ms->possible_cpus->len == max_cpus);
> >>>> @@ -2518,8 +2519,20 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >>>>            ms->possible_cpus->cpus[n].type = ms->cpu_type;
> >>>>            ms->possible_cpus->cpus[n].arch_id =
> >>>>                virt_cpu_mp_affinity(vms, n);
> >>>> +
> >>>> +        assert(!mc->smp_props.dies_supported);
> >>>> +        ms->possible_cpus->cpus[n].props.has_socket_id = true;
> >>>> +        ms->possible_cpus->cpus[n].props.socket_id =
> >>>> +            (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads));
> >>>> +        ms->possible_cpus->cpus[n].props.has_cluster_id = true;
> >>>> +        ms->possible_cpus->cpus[n].props.cluster_id =
> >>>> +            (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters;
> >>>> +        ms->possible_cpus->cpus[n].props.has_core_id = true;
> >>>> +        ms->possible_cpus->cpus[n].props.core_id =
> >>>> +            (n / ms->smp.threads) % ms->smp.cores;
> >>>>            ms->possible_cpus->cpus[n].props.has_thread_id = true;
> >>>> -        ms->possible_cpus->cpus[n].props.thread_id = n;
> >>>> +        ms->possible_cpus->cpus[n].props.thread_id =
> >>>> +            n % ms->smp.threads;
> >>>>        }
> >>>>        return ms->possible_cpus;
> >>>>    }
> >>>> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> >>>> index 90bf68a5b3..aeda8c774c 100644
> >>>> --- a/tests/qtest/numa-test.c
> >>>> +++ b/tests/qtest/numa-test.c
> >>>> @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data)
> >>>>        QTestState *qts;
> >>>>        g_autofree char *cli = NULL;
> >>>>    
> >>>> -    cli = make_cli(data, "-machine smp.cpus=2 "
> >>>> +    cli = make_cli(data, "-machine "
> >>>> +        "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 "  
> >>>
> >>> Is cluster-less config possible?
> >>> (looks like it used to work before and it doesn't after this series)
> >>>      
> >>
> >> Nope, it's impossible. This specific test case uses arm/virt machine
> >> where cluster is always supported.mc->smp_props.clusters_supported
> >> has been set to true in hw/arm/virt.c::virt_machine_class_init().
> >>
> >> Exactly, the changes to virt_possible_cpu_arch_ids() included in this patch breaks
> >> the test. It's why the fix to qtest/numa-test has been squashed to this patch, to
> >> make it 'bit bisect' friendly as Yanan suggested.  
> > 
> > so what was error that broke the test?
> > (probably should be mentioned in commit message)
> > 
> > (also is it possible to split out the test patch into
> > a separate one and put it before this one)
> >   
> 
> With amend to the command lines, the following one is used and below error
> is raised from the test. The error is mentioned in the commit log in
> PATCH[v7 2/4].
> 
>      -machine smp.cpus=2                                   \
>      -numa node,nodeid=0,memdev=ram -numa node,nodeid=1    \
>      -numa cpu,node-id=1,thread-id=0                       \
>      -numa cpu,node-id=0,thread-id=1
> 
>      qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found
>      (reported from hw/core/machine.c::machine_set_cpu_numa_node())
> 
> After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1"
> isn't valid any more. The CPU topology becomes like below. Note that
> mc->smp_props.prefer_sockets is true on arm/virt machine.
> 
>      index    socket   cluster    core    thread
>      --------------------------------------------
>        0        0        0         0        0
>        1        1        0         0        0
> 
> With the amended command lines, the topology changes again so
> that "thread-id=1" is valid:
> 
>      index    socket   cluster    core    thread
>      --------------------------------------------
>        0        0        0         0        0
>        1        0        0         0        1
> 
> It should be ok to split the test/qtest/aarch64_numa_cpu() changes into
> a separate patch and put it before this one. In that case, the specified
> smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet,
> and 'thread-id=2' should be still valid. Lets do this if I need post v8.
> Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu()
> changes into PATCH[2/4], as we're doing.

if you need to respin v7. do it as separate patch with proper commit message
and maybe add an extra test that exercises fully specified topo.

> 
> >   
> >>
> >>  
> >>>>            "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
> >>>>            "-numa cpu,node-id=1,thread-id=0 "
> >>>>            "-numa cpu,node-id=0,thread-id=1");  
> 
> Thanks,
> Gavin
> 

Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Gavin Shan]

Hi Igor,

On 4/20/22 10:50 PM, Igor Mammedov wrote:
> On Wed, 20 Apr 2022 22:24:46 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> On 4/20/22 7:50 PM, Igor Mammedov wrote:
>>> On Wed, 20 Apr 2022 18:31:02 +0800
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>> On 4/20/22 4:32 PM, Igor Mammedov wrote:
>>>>> On Mon, 18 Apr 2022 10:09:18 +0800
>>>>> Gavin Shan <gshan@redhat.com> wrote:
>>>>>       
>>>>>> Currently, the SMP configuration isn't considered when the CPU
>>>>>> topology is populated. In this case, it's impossible to provide
>>>>>> the default CPU-to-NUMA mapping or association based on the socket
>>>>>> ID of the given CPU.
>>>>>>
>>>>>> This takes account of SMP configuration when the CPU topology
>>>>>> is populated. The die ID for the given CPU isn't assigned since
>>>>>> it's not supported on arm/virt machine. Besides, the used SMP
>>>>>> configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted
>>>>>> to avoid testing failure
>>>>>>
>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>>>> ---
>>>>>>     hw/arm/virt.c           | 15 ++++++++++++++-
>>>>>>     tests/qtest/numa-test.c |  3 ++-
>>>>>>     2 files changed, 16 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>>>> index d2e5ecd234..5443ecae92 100644
>>>>>> --- a/hw/arm/virt.c
>>>>>> +++ b/hw/arm/virt.c
>>>>>> @@ -2505,6 +2505,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>>>>         int n;
>>>>>>         unsigned int max_cpus = ms->smp.max_cpus;
>>>>>>         VirtMachineState *vms = VIRT_MACHINE(ms);
>>>>>> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
>>>>>>     
>>>>>>         if (ms->possible_cpus) {
>>>>>>             assert(ms->possible_cpus->len == max_cpus);
>>>>>> @@ -2518,8 +2519,20 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>>>>             ms->possible_cpus->cpus[n].type = ms->cpu_type;
>>>>>>             ms->possible_cpus->cpus[n].arch_id =
>>>>>>                 virt_cpu_mp_affinity(vms, n);
>>>>>> +
>>>>>> +        assert(!mc->smp_props.dies_supported);
>>>>>> +        ms->possible_cpus->cpus[n].props.has_socket_id = true;
>>>>>> +        ms->possible_cpus->cpus[n].props.socket_id =
>>>>>> +            (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads));
>>>>>> +        ms->possible_cpus->cpus[n].props.has_cluster_id = true;
>>>>>> +        ms->possible_cpus->cpus[n].props.cluster_id =
>>>>>> +            (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters;
>>>>>> +        ms->possible_cpus->cpus[n].props.has_core_id = true;
>>>>>> +        ms->possible_cpus->cpus[n].props.core_id =
>>>>>> +            (n / ms->smp.threads) % ms->smp.cores;
>>>>>>             ms->possible_cpus->cpus[n].props.has_thread_id = true;
>>>>>> -        ms->possible_cpus->cpus[n].props.thread_id = n;
>>>>>> +        ms->possible_cpus->cpus[n].props.thread_id =
>>>>>> +            n % ms->smp.threads;
>>>>>>         }
>>>>>>         return ms->possible_cpus;
>>>>>>     }
>>>>>> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
>>>>>> index 90bf68a5b3..aeda8c774c 100644
>>>>>> --- a/tests/qtest/numa-test.c
>>>>>> +++ b/tests/qtest/numa-test.c
>>>>>> @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data)
>>>>>>         QTestState *qts;
>>>>>>         g_autofree char *cli = NULL;
>>>>>>     
>>>>>> -    cli = make_cli(data, "-machine smp.cpus=2 "
>>>>>> +    cli = make_cli(data, "-machine "
>>>>>> +        "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 "
>>>>>
>>>>> Is cluster-less config possible?
>>>>> (looks like it used to work before and it doesn't after this series)
>>>>>       
>>>>
>>>> Nope, it's impossible. This specific test case uses arm/virt machine
>>>> where cluster is always supported.mc->smp_props.clusters_supported
>>>> has been set to true in hw/arm/virt.c::virt_machine_class_init().
>>>>
>>>> Exactly, the changes to virt_possible_cpu_arch_ids() included in this patch breaks
>>>> the test. It's why the fix to qtest/numa-test has been squashed to this patch, to
>>>> make it 'bit bisect' friendly as Yanan suggested.
>>>
>>> so what was error that broke the test?
>>> (probably should be mentioned in commit message)
>>>
>>> (also is it possible to split out the test patch into
>>> a separate one and put it before this one)
>>>    
>>
>> With amend to the command lines, the following one is used and below error
>> is raised from the test. The error is mentioned in the commit log in
>> PATCH[v7 2/4].
>>
>>       -machine smp.cpus=2                                   \
>>       -numa node,nodeid=0,memdev=ram -numa node,nodeid=1    \
>>       -numa cpu,node-id=1,thread-id=0                       \
>>       -numa cpu,node-id=0,thread-id=1
>>
>>       qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found
>>       (reported from hw/core/machine.c::machine_set_cpu_numa_node())
>>
>> After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1"
>> isn't valid any more. The CPU topology becomes like below. Note that
>> mc->smp_props.prefer_sockets is true on arm/virt machine.
>>
>>       index    socket   cluster    core    thread
>>       --------------------------------------------
>>         0        0        0         0        0
>>         1        1        0         0        0
>>
>> With the amended command lines, the topology changes again so
>> that "thread-id=1" is valid:
>>
>>       index    socket   cluster    core    thread
>>       --------------------------------------------
>>         0        0        0         0        0
>>         1        0        0         0        1
>>
>> It should be ok to split the test/qtest/aarch64_numa_cpu() changes into
>> a separate patch and put it before this one. In that case, the specified
>> smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet,
>> and 'thread-id=2' should be still valid. Lets do this if I need post v8.
>> Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu()
>> changes into PATCH[2/4], as we're doing.
> 
> if you need to respin v7. do it as separate patch with proper commit message
> and maybe add an extra test that exercises fully specified topo.
> 

Sure, I will split the fix for test/qtest/aarch64_numa_cpu() in v8 if it's
needed. For the additional test case to exercise the fully specific topology,
I rather to do it after this series is merged to v7.1 because our downstream
needs the fix :)

>>
>>>    
>>>>
>>>>   
>>>>>>             "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
>>>>>>             "-numa cpu,node-id=1,thread-id=0 "
>>>>>>             "-numa cpu,node-id=0,thread-id=1");

Thanks,
Gavin

Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Andrew Jones]

On Wed, Apr 20, 2022 at 10:24:46PM +0800, Gavin Shan wrote:
...
> With amend to the command lines, the following one is used and below error
> is raised from the test. The error is mentioned in the commit log in
> PATCH[v7 2/4].
> 
>     -machine smp.cpus=2                                   \
>     -numa node,nodeid=0,memdev=ram -numa node,nodeid=1    \
>     -numa cpu,node-id=1,thread-id=0                       \
>     -numa cpu,node-id=0,thread-id=1
> 
>     qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found
>     (reported from hw/core/machine.c::machine_set_cpu_numa_node())
> 
> After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1"
> isn't valid any more. The CPU topology becomes like below. Note that
> mc->smp_props.prefer_sockets is true on arm/virt machine.

prefer_sockets is only true for mach-virt 6.1 and older. It's false for
6.2 and later.

Thanks,
drew

> 
>     index    socket   cluster    core    thread
>     --------------------------------------------
>       0        0        0         0        0
>       1        1        0         0        0
> 
> With the amended command lines, the topology changes again so
> that "thread-id=1" is valid:
> 
>     index    socket   cluster    core    thread
>     --------------------------------------------
>       0        0        0         0        0
>       1        0        0         0        1
> 
> It should be ok to split the test/qtest/aarch64_numa_cpu() changes into
> a separate patch and put it before this one. In that case, the specified
> smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet,
> and 'thread-id=2' should be still valid. Lets do this if I need post v8.
> Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu()
> changes into PATCH[2/4], as we're doing.
> 
> > 
> > > 
> > > 
> > > > >            "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
> > > > >            "-numa cpu,node-id=1,thread-id=0 "
> > > > >            "-numa cpu,node-id=0,thread-id=1");
> 
> Thanks,
> Gavin
> 

Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Gavin Shan]

Hi Drew,

On 4/21/22 5:02 PM, Andrew Jones wrote:
> On Wed, Apr 20, 2022 at 10:24:46PM +0800, Gavin Shan wrote:
> ...
>> With amend to the command lines, the following one is used and below error
>> is raised from the test. The error is mentioned in the commit log in
>> PATCH[v7 2/4].
>>
>>      -machine smp.cpus=2                                   \
>>      -numa node,nodeid=0,memdev=ram -numa node,nodeid=1    \
>>      -numa cpu,node-id=1,thread-id=0                       \
>>      -numa cpu,node-id=0,thread-id=1
>>
>>      qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found
>>      (reported from hw/core/machine.c::machine_set_cpu_numa_node())
>>
>> After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1"
>> isn't valid any more. The CPU topology becomes like below. Note that
>> mc->smp_props.prefer_sockets is true on arm/virt machine.
> 
> prefer_sockets is only true for mach-virt 6.1 and older. It's false for
> 6.2 and later.
> 

Yeah, @perfer_socket is false in last mach-virt-7.0 , which is used in
this test case. So we prefer CPU core over sockets, as explained in
hw/core/machine.c::machine_parse_smp_config(). The CPU topology
becomes like below instead, but 'thread-id=1' is still invalid.

       index    socket   cluster    core    thread
       --------------------------------------------
         0        0        0         0        0
         1        0        0         1        0

>>
>>      index    socket   cluster    core    thread
>>      --------------------------------------------
>>        0        0        0         0        0
>>        1        1        0         0        0
>>
>> With the amended command lines, the topology changes again so
>> that "thread-id=1" is valid:
>>
>>      index    socket   cluster    core    thread
>>      --------------------------------------------
>>        0        0        0         0        0
>>        1        0        0         0        1
>>
>> It should be ok to split the test/qtest/aarch64_numa_cpu() changes into
>> a separate patch and put it before this one. In that case, the specified
>> smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet,
>> and 'thread-id=2' should be still valid. Lets do this if I need post v8.
>> Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu()
>> changes into PATCH[2/4], as we're doing.
>>
>>>
>>>>
>>>>
>>>>>>             "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
>>>>>>             "-numa cpu,node-id=1,thread-id=0 "
>>>>>>             "-numa cpu,node-id=0,thread-id=1");

Thanks,
Gavin

[PATCH v6 3/4] hw/arm/virt: Fix CPU's default NUMA node ID

[Gavin Shan]

When CPU-to-NUMA association isn't explicitly provided by users,
the default one is given by mc->get_default_cpu_node_id(). However,
the CPU topology isn't fully considered in the default association
and this causes CPU topology broken warnings on booting Linux guest.

For example, the following warning messages are observed when the
Linux guest is booted with the following command lines.

  /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
  -accel kvm -machine virt,gic-version=host               \
  -cpu host                                               \
  -smp 6,sockets=2,cores=3,threads=1                      \
  -m 1024M,slots=16,maxmem=64G                            \
  -object memory-backend-ram,id=mem0,size=128M            \
  -object memory-backend-ram,id=mem1,size=128M            \
  -object memory-backend-ram,id=mem2,size=128M            \
  -object memory-backend-ram,id=mem3,size=128M            \
  -object memory-backend-ram,id=mem4,size=128M            \
  -object memory-backend-ram,id=mem4,size=384M            \
  -numa node,nodeid=0,memdev=mem0                         \
  -numa node,nodeid=1,memdev=mem1                         \
  -numa node,nodeid=2,memdev=mem2                         \
  -numa node,nodeid=3,memdev=mem3                         \
  -numa node,nodeid=4,memdev=mem4                         \
  -numa node,nodeid=5,memdev=mem5
         :
  alternatives: patching kernel code
  BUG: arch topology borken
  the CLS domain not a subset of the MC domain
  <the above error log repeats>
  BUG: arch topology borken
  the DIE domain not a subset of the NODE domain

With current implementation of mc->get_default_cpu_node_id(),
CPU#0 to CPU#5 are associated with NODE#0 to NODE#5 separately.
That's incorrect because CPU#0/1/2 should be associated with same
NUMA node because they're seated in same socket.

This fixes the issue by considering the socket ID when the default
CPU-to-NUMA association is provided in virt_possible_cpu_arch_ids().
With this applied, no more CPU topology broken warnings are seen
from the Linux guest. The 6 CPUs are associated with NODE#0/1, but
there are no CPUs associated with NODE#2/3/4/5.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 5443ecae92..c3477a8a17 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2497,7 +2497,9 @@ virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
 
 static int64_t virt_get_default_cpu_node_id(const MachineState *ms, int idx)
 {
-    return idx % ms->numa_state->num_nodes;
+    int64_t socket_id = ms->possible_cpus->cpus[idx].props.socket_id;
+
+    return socket_id % ms->numa_state->num_nodes;
 }
 
 static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
-- 
2.23.0

[PATCH v6 4/4] hw/acpi/aml-build: Use existing CPU topology to build PPTT table

[Gavin Shan]

When the PPTT table is built, the CPU topology is re-calculated, but
it's unecessary because the CPU topology has been populated in
virt_possible_cpu_arch_ids() on arm/virt machine.

This reworks build_pptt() to avoid by reusing the existing one in
ms->possible_cpus. Currently, the only user of build_pptt() is
arm/virt machine.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/acpi/aml-build.c | 105 ++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 62 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 4086879ebf..2bd062a52b 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2002,86 +2002,67 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
                 const char *oem_id, const char *oem_table_id)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
-    GQueue *list = g_queue_new();
-    guint pptt_start = table_data->len;
-    guint parent_offset;
-    guint length, i;
-    int uid = 0;
-    int socket;
+    CPUArchIdList *cpus = ms->possible_cpus;
+    int64_t socket_id = -1, cluster_id = -1, core_id = -1;
+    uint32_t socket_offset = 0, cluster_offset = 0, core_offset = 0;
+    uint32_t pptt_start = table_data->len;
+    int n;
     AcpiTable table = { .sig = "PPTT", .rev = 2,
                         .oem_id = oem_id, .oem_table_id = oem_table_id };
 
     acpi_table_begin(&table, table_data);
 
-    for (socket = 0; socket < ms->smp.sockets; socket++) {
-        g_queue_push_tail(list,
-            GUINT_TO_POINTER(table_data->len - pptt_start));
-        build_processor_hierarchy_node(
-            table_data,
-            /*
-             * Physical package - represents the boundary
-             * of a physical package
-             */
-            (1 << 0),
-            0, socket, NULL, 0);
-    }
+    /*
+     * This works with the assumption that cpus[n].props.*_id has been
+     * sorted from top to down levels in mc->possible_cpu_arch_ids().
+     * Otherwise, the unexpected and duplicate containers will be created.
+     */
+    for (n = 0; n < cpus->len; n++) {
+        if (cpus->cpus[n].props.socket_id != socket_id) {
+            socket_id = cpus->cpus[n].props.socket_id;
+            cluster_id = -1;
+            core_id = -1;
+            socket_offset = table_data->len - pptt_start;
+            build_processor_hierarchy_node(table_data,
+                (1 << 0), /* Physical package */
+                0, socket_id, NULL, 0);
+        }
 
-    if (mc->smp_props.clusters_supported) {
-        length = g_queue_get_length(list);
-        for (i = 0; i < length; i++) {
-            int cluster;
-
-            parent_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
-            for (cluster = 0; cluster < ms->smp.clusters; cluster++) {
-                g_queue_push_tail(list,
-                    GUINT_TO_POINTER(table_data->len - pptt_start));
-                build_processor_hierarchy_node(
-                    table_data,
-                    (0 << 0), /* not a physical package */
-                    parent_offset, cluster, NULL, 0);
+        if (mc->smp_props.clusters_supported) {
+            if (cpus->cpus[n].props.cluster_id != cluster_id) {
+                cluster_id = cpus->cpus[n].props.cluster_id;
+                core_id = -1;
+                cluster_offset = table_data->len - pptt_start;
+                build_processor_hierarchy_node(table_data,
+                    (0 << 0), /* Not a physical package */
+                    socket_offset, cluster_id, NULL, 0);
             }
+        } else {
+            cluster_offset = socket_offset;
         }
-    }
 
-    length = g_queue_get_length(list);
-    for (i = 0; i < length; i++) {
-        int core;
-
-        parent_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
-        for (core = 0; core < ms->smp.cores; core++) {
-            if (ms->smp.threads > 1) {
-                g_queue_push_tail(list,
-                    GUINT_TO_POINTER(table_data->len - pptt_start));
-                build_processor_hierarchy_node(
-                    table_data,
+        if (ms->smp.threads <= 1) {
+            build_processor_hierarchy_node(table_data,
+                (1 << 1) | /* ACPI Processor ID valid */
+                (1 << 3),  /* Node is a Leaf */
+                cluster_offset, n, NULL, 0);
+        } else {
+            if (cpus->cpus[n].props.core_id != core_id) {
+                core_id = cpus->cpus[n].props.core_id;
+                core_offset = table_data->len - pptt_start;
+                build_processor_hierarchy_node(table_data,
                     (0 << 0), /* not a physical package */
-                    parent_offset, core, NULL, 0);
-            } else {
-                build_processor_hierarchy_node(
-                    table_data,
-                    (1 << 1) | /* ACPI Processor ID valid */
-                    (1 << 3),  /* Node is a Leaf */
-                    parent_offset, uid++, NULL, 0);
+                    cluster_offset, core_id, NULL, 0);
             }
-        }
-    }
-
-    length = g_queue_get_length(list);
-    for (i = 0; i < length; i++) {
-        int thread;
 
-        parent_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
-        for (thread = 0; thread < ms->smp.threads; thread++) {
-            build_processor_hierarchy_node(
-                table_data,
+            build_processor_hierarchy_node(table_data,
                 (1 << 1) | /* ACPI Processor ID valid */
                 (1 << 2) | /* Processor is a Thread */
                 (1 << 3),  /* Node is a Leaf */
-                parent_offset, uid++, NULL, 0);
+                core_offset, n, NULL, 0);
         }
     }
 
-    g_queue_free(list);
     acpi_table_end(linker, &table);
 }
 
-- 
2.23.0