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

[PATCH v5 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
=========
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        | 100 ++++++++++++++-----------------------
 hw/arm/virt.c              |  20 +++++++-
 hw/core/machine-hmp-cmds.c |   4 ++
 hw/core/machine.c          |  16 ++++++
 qapi/machine.json          |   6 ++-
 5 files changed, 80 insertions(+), 66 deletions(-)

-- 
2.23.0

[PATCH v5 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>
---
 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 d856485cb4..8748b64657 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -677,6 +677,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;
@@ -696,6 +701,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;
         }
@@ -990,6 +1000,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 9c460ec450..ea22b574b0 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 die the CPU belongs to
+# @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

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

[Daniel P. Berrangé]

On Sun, Apr 03, 2022 at 10:59:50PM +0800, Gavin Shan wrote:
> 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>
> ---
>  hw/core/machine-hmp-cmds.c |  4 ++++
>  hw/core/machine.c          | 16 ++++++++++++++++
>  qapi/machine.json          |  6 ++++--
>  3 files changed, 24 insertions(+), 2 deletions(-)

Missing changes to hw/core/machine-smp.c similar to 'dies' in that
file.

When 'dies' was added we added a 'dies_supported' flag, so we could
reject use of 'dies' when it was not supported - which is everywhere
except i386 target.

We need the same for 'clusters_supported' machine property since
AFAICT only the arm 'virt' machine is getting supported in this
series.

> 
> 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 d856485cb4..8748b64657 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -677,6 +677,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;
> @@ -696,6 +701,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;
>          }
> @@ -990,6 +1000,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 9c460ec450..ea22b574b0 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 die the CPU belongs to
> +# @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
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

On Mon, Apr 04, 2022 at 09:37:10AM +0100, Daniel P. Berrangé wrote:
> On Sun, Apr 03, 2022 at 10:59:50PM +0800, Gavin Shan wrote:
> > 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>
> > ---
> >  hw/core/machine-hmp-cmds.c |  4 ++++
> >  hw/core/machine.c          | 16 ++++++++++++++++
> >  qapi/machine.json          |  6 ++++--
> >  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> Missing changes to hw/core/machine-smp.c similar to 'dies' in that
> file.
> 
> When 'dies' was added we added a 'dies_supported' flag, so we could
> reject use of 'dies' when it was not supported - which is everywhere
> except i386 target.
> 
> We need the same for 'clusters_supported' machine property since
> AFAICT only the arm 'virt' machine is getting supported in this
> series.

Oh, actually I'm mixing up cluster-id and clusters - the latter is
already supported.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Gavin Shan]

Hi Daniel,

On 4/4/22 4:40 PM, Daniel P. Berrangé wrote:
> On Mon, Apr 04, 2022 at 09:37:10AM +0100, Daniel P. Berrangé wrote:
>> On Sun, Apr 03, 2022 at 10:59:50PM +0800, Gavin Shan wrote:
>>> 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>
>>> ---
>>>   hw/core/machine-hmp-cmds.c |  4 ++++
>>>   hw/core/machine.c          | 16 ++++++++++++++++
>>>   qapi/machine.json          |  6 ++++--
>>>   3 files changed, 24 insertions(+), 2 deletions(-)
>>
>> Missing changes to hw/core/machine-smp.c similar to 'dies' in that
>> file.
>>
>> When 'dies' was added we added a 'dies_supported' flag, so we could
>> reject use of 'dies' when it was not supported - which is everywhere
>> except i386 target.
>>
>> We need the same for 'clusters_supported' machine property since
>> AFAICT only the arm 'virt' machine is getting supported in this
>> series.
> 
> Oh, actually I'm mixing up cluster-id and clusters - the latter is
> already supported.
> 

Yeah, @clusters_supported has been existing for a while.

Thanks,
Gavin

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

[wangyanan (Y) via]

Hi Gavin,

On 2022/4/3 22:59, Gavin Shan wrote:
> 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>
> ---
>   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 d856485cb4..8748b64657 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -677,6 +677,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;
> @@ -696,6 +701,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;
>           }
> @@ -990,6 +1000,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 9c460ec450..ea22b574b0 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 die the CPU belongs to
I remember this should be "cluster number within socket..."
according to Igor's comments in v3 ?
> +# @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'
>     }
Otherwise, looks good to me:
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>

Please also keep the involved Maintainers on Cc list in next version,
an Ack from them is best. :)

Thanks,
Yanan

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

[Gavin Shan]

Hi Yanan,

On 4/13/22 7:49 PM, wangyanan (Y) wrote:
> On 2022/4/3 22:59, Gavin Shan wrote:
>> 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>
>> ---
>>   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 d856485cb4..8748b64657 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -677,6 +677,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;
>> @@ -696,6 +701,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;
>>           }
>> @@ -990,6 +1000,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 9c460ec450..ea22b574b0 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 die the CPU belongs to
> I remember this should be "cluster number within socket..."
> according to Igor's comments in v3 ?

Igor had suggestion to correct the description for 'core-id' like
below, but he didn't suggest anything for 'cluster-id'. The question
is clusters are sub-components of die, instead of socket, if die
is supported? You may want to me change it like below and please
confirm.

   @cluster-id: cluster number within die/socket the CPU belongs to

suggestion from Ignor in v3:

    > +# @core-id: core number within cluster the CPU belongs to

    s:cluster:cluster/die:


>> +# @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'
>>     }
> Otherwise, looks good to me:
> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> 
> Please also keep the involved Maintainers on Cc list in next version,
> an Ack from them is best. :)
> 

Thanks again for your time to review. Sure, I will do in next posting.

Thanks,
Gavin

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

[wangyanan (Y) via]

Hi Gavin,

Cc: Daniel and Markus
On 2022/4/14 8:06, Gavin Shan wrote:
> Hi Yanan,
>
> On 4/13/22 7:49 PM, wangyanan (Y) wrote:
>> On 2022/4/3 22:59, Gavin Shan wrote:
>>> 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>
>>> ---
>>>   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 d856485cb4..8748b64657 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -677,6 +677,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;
>>> @@ -696,6 +701,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;
>>>           }
>>> @@ -990,6 +1000,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 9c460ec450..ea22b574b0 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 die the CPU belongs to
We also need a "(since 7.1)" tag for cluster-id.
>> I remember this should be "cluster number within socket..."
>> according to Igor's comments in v3 ?
>
> Igor had suggestion to correct the description for 'core-id' like
> below, but he didn't suggest anything for 'cluster-id'. The question
> is clusters are sub-components of die, instead of socket, if die
> is supported? You may want to me change it like below and please
> confirm.
>
>   @cluster-id: cluster number within die/socket the CPU belongs to
>
> suggestion from Ignor in v3:
>
>    > +# @core-id: core number within cluster the CPU belongs to
>
>    s:cluster:cluster/die:
>
We want "within cluster/die" description for core-id because we
support both "cores in cluster" for ARM and "cores in die" for X86.
Base on this routine, we only need "within socket" for cluster-id
because we currently only support "clusters in socket". Does this
make sense?

Alternatively, the plainest documentation for the IDs is to simply
range **-id only to its next level topo like below. This may avoid
increasing complexity when more topo-ids are inserted middle.
But whether this way is acceptable is up to the Maintainers. :)

# @socket-id: socket number within node/board the CPU belongs to
# @die-id: die number within socket the CPU belongs to (since 4.1)
# @cluster-id: cluster number within die the CPU belongs to (since 7.1)
# @core-id: core number within cluster the CPU belongs to
# @thread-id: thread number within core the CPU belongs to

Thanks,
Yanan
>
>>> +# @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'
>>>     }
>> Otherwise, looks good to me:
>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>
>> Please also keep the involved Maintainers on Cc list in next version,
>> an Ack from them is best. :)
>>
>
> Thanks again for your time to review. Sure, I will do in next posting.
>
> Thanks,
> Gavin
>
> .

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

[Gavin Shan]

Hi Yanan,

On 4/14/22 10:27 AM, wangyanan (Y) wrote:
> On 2022/4/14 8:06, Gavin Shan wrote:
>> On 4/13/22 7:49 PM, wangyanan (Y) wrote:
>>> On 2022/4/3 22:59, Gavin Shan wrote:
>>>> 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>
>>>> ---
>>>>   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 d856485cb4..8748b64657 100644
>>>> --- a/hw/core/machine.c
>>>> +++ b/hw/core/machine.c
>>>> @@ -677,6 +677,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;
>>>> @@ -696,6 +701,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;
>>>>           }
>>>> @@ -990,6 +1000,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 9c460ec450..ea22b574b0 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 die the CPU belongs to
> We also need a "(since 7.1)" tag for cluster-id.
>>> I remember this should be "cluster number within socket..."
>>> according to Igor's comments in v3 ?
>>
>> Igor had suggestion to correct the description for 'core-id' like
>> below, but he didn't suggest anything for 'cluster-id'. The question
>> is clusters are sub-components of die, instead of socket, if die
>> is supported? You may want to me change it like below and please
>> confirm.
>>
>>   @cluster-id: cluster number within die/socket the CPU belongs to
>>
>> suggestion from Ignor in v3:
>>
>>    > +# @core-id: core number within cluster the CPU belongs to
>>
>>    s:cluster:cluster/die:
>>
> We want "within cluster/die" description for core-id because we
> support both "cores in cluster" for ARM and "cores in die" for X86.
> Base on this routine, we only need "within socket" for cluster-id
> because we currently only support "clusters in socket". Does this
> make sense?
> 

Thanks for the explanation. So ARM64 doesn't have die and x86 doesn't
have cluster? If so, I need to adjust the description for 'cluster-id'
as you suggested in v6:

   @cluster-id: cluster number within socket the CPU belongs to (since 7.1)
    
> Alternatively, the plainest documentation for the IDs is to simply
> range **-id only to its next level topo like below. This may avoid
> increasing complexity when more topo-ids are inserted middle.
> But whether this way is acceptable is up to the Maintainers. :)
> 
> # @socket-id: socket number within node/board the CPU belongs to
> # @die-id: die number within socket the CPU belongs to (since 4.1)
> # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
> # @core-id: core number within cluster the CPU belongs to
> # @thread-id: thread number within core the CPU belongs to
> 

I like this scheme, but needs the confirms from Igor and maintainers.
Igor and maintainers, please let us know if the scheme is good to
have? :)

>>
>>>> +# @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'
>>>>     }
>>> Otherwise, looks good to me:
>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>
>>> Please also keep the involved Maintainers on Cc list in next version,
>>> an Ack from them is best. :)
>>>
>>
>> Thanks again for your time to review. Sure, I will do in next posting.
>>

Thanks,
Gavin

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

[wangyanan (Y) via]

On 2022/4/14 15:56, Gavin Shan wrote:
> Hi Yanan,
>
> On 4/14/22 10:27 AM, wangyanan (Y) wrote:
>> On 2022/4/14 8:06, Gavin Shan wrote:
>>> On 4/13/22 7:49 PM, wangyanan (Y) wrote:
>>>> On 2022/4/3 22:59, Gavin Shan wrote:
>>>>> 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>
>>>>> ---
>>>>>   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 d856485cb4..8748b64657 100644
>>>>> --- a/hw/core/machine.c
>>>>> +++ b/hw/core/machine.c
>>>>> @@ -677,6 +677,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;
>>>>> @@ -696,6 +701,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;
>>>>>           }
>>>>> @@ -990,6 +1000,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 9c460ec450..ea22b574b0 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 die the CPU belongs to
>> We also need a "(since 7.1)" tag for cluster-id.
>>>> I remember this should be "cluster number within socket..."
>>>> according to Igor's comments in v3 ?
>>>
>>> Igor had suggestion to correct the description for 'core-id' like
>>> below, but he didn't suggest anything for 'cluster-id'. The question
>>> is clusters are sub-components of die, instead of socket, if die
>>> is supported? You may want to me change it like below and please
>>> confirm.
>>>
>>>   @cluster-id: cluster number within die/socket the CPU belongs to
>>>
>>> suggestion from Ignor in v3:
>>>
>>>    > +# @core-id: core number within cluster the CPU belongs to
>>>
>>>    s:cluster:cluster/die:
>>>
>> We want "within cluster/die" description for core-id because we
>> support both "cores in cluster" for ARM and "cores in die" for X86.
>> Base on this routine, we only need "within socket" for cluster-id
>> because we currently only support "clusters in socket". Does this
>> make sense?
>>
>
> Thanks for the explanation. So ARM64 doesn't have die and x86 doesn't
> have cluster?
At least for now, yes. :)

Thanks,
Yanan
> If so, I need to adjust the description for 'cluster-id'
> as you suggested in v6:
>
>   @cluster-id: cluster number within socket the CPU belongs to (since 
> 7.1)
>> Alternatively, the plainest documentation for the IDs is to simply
>> range **-id only to its next level topo like below. This may avoid
>> increasing complexity when more topo-ids are inserted middle.
>> But whether this way is acceptable is up to the Maintainers. :)
>>
>> # @socket-id: socket number within node/board the CPU belongs to
>> # @die-id: die number within socket the CPU belongs to (since 4.1)
>> # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
>> # @core-id: core number within cluster the CPU belongs to
>> # @thread-id: thread number within core the CPU belongs to
>>
>
> I like this scheme, but needs the confirms from Igor and maintainers.
> Igor and maintainers, please let us know if the scheme is good to
> have? :)
>
>>>
>>>>> +# @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'
>>>>>     }
>>>> Otherwise, looks good to me:
>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>>
>>>> Please also keep the involved Maintainers on Cc list in next version,
>>>> an Ack from them is best. :)
>>>>
>>>
>>> Thanks again for your time to review. Sure, I will do in next posting.
>>>
>
> Thanks,
> Gavin
>
> .

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

[Daniel P. Berrangé]

On Thu, Apr 14, 2022 at 10:27:25AM +0800, wangyanan (Y) wrote:
> Hi Gavin,
> 
> Cc: Daniel and Markus
> On 2022/4/14 8:06, Gavin Shan wrote:
> > Hi Yanan,
> > 
> > On 4/13/22 7:49 PM, wangyanan (Y) wrote:
> > > On 2022/4/3 22:59, Gavin Shan wrote:
> > > > 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>
> > > > ---
> > > >   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 d856485cb4..8748b64657 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -677,6 +677,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;
> > > > @@ -696,6 +701,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;
> > > >           }
> > > > @@ -990,6 +1000,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 9c460ec450..ea22b574b0 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 die the CPU belongs to
> We also need a "(since 7.1)" tag for cluster-id.
> > > I remember this should be "cluster number within socket..."
> > > according to Igor's comments in v3 ?
> > 
> > Igor had suggestion to correct the description for 'core-id' like
> > below, but he didn't suggest anything for 'cluster-id'. The question
> > is clusters are sub-components of die, instead of socket, if die
> > is supported? You may want to me change it like below and please
> > confirm.
> > 
> >   @cluster-id: cluster number within die/socket the CPU belongs to
> > 
> > suggestion from Ignor in v3:
> > 
> >    > +# @core-id: core number within cluster the CPU belongs to
> > 
> >    s:cluster:cluster/die:
> > 
> We want "within cluster/die" description for core-id because we
> support both "cores in cluster" for ARM and "cores in die" for X86.
> Base on this routine, we only need "within socket" for cluster-id
> because we currently only support "clusters in socket". Does this
> make sense?
> 
> Alternatively, the plainest documentation for the IDs is to simply
> range **-id only to its next level topo like below. This may avoid
> increasing complexity when more topo-ids are inserted middle.
> But whether this way is acceptable is up to the Maintainers. :)

Rather saying we only support cluster on ARM and only dies on x86,
I tend to view it as, we only support greater than 1 cluster on
ARM, and greater than 1 die on x86.

IOW, x86 implicitly always has exactly 1-and-only-1 cluster,
and arm implicitly always has exactly 1-and-only-1 die.

With this POV, then we can keep the description simple, only
refering to the immediately above level in the hierarchy.

> 
> # @socket-id: socket number within node/board the CPU belongs to
> # @die-id: die number within socket the CPU belongs to (since 4.1)
> # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
> # @core-id: core number within cluster the CPU belongs to
> # @thread-id: thread number within core the CPU belongs to

So this suggested text is fine with me.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Gavin Shan]

Hi Daniel,

On 4/19/22 11:59 PM, Daniel P. Berrangé wrote:
> On Thu, Apr 14, 2022 at 10:27:25AM +0800, wangyanan (Y) wrote:
>> Hi Gavin,
>>
>> Cc: Daniel and Markus
>> On 2022/4/14 8:06, Gavin Shan wrote:
>>> Hi Yanan,
>>>
>>> On 4/13/22 7:49 PM, wangyanan (Y) wrote:
>>>> On 2022/4/3 22:59, Gavin Shan wrote:
>>>>> 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>
>>>>> ---
>>>>>    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 d856485cb4..8748b64657 100644
>>>>> --- a/hw/core/machine.c
>>>>> +++ b/hw/core/machine.c
>>>>> @@ -677,6 +677,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;
>>>>> @@ -696,6 +701,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;
>>>>>            }
>>>>> @@ -990,6 +1000,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 9c460ec450..ea22b574b0 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 die the CPU belongs to
>> We also need a "(since 7.1)" tag for cluster-id.
>>>> I remember this should be "cluster number within socket..."
>>>> according to Igor's comments in v3 ?
>>>
>>> Igor had suggestion to correct the description for 'core-id' like
>>> below, but he didn't suggest anything for 'cluster-id'. The question
>>> is clusters are sub-components of die, instead of socket, if die
>>> is supported? You may want to me change it like below and please
>>> confirm.
>>>
>>>    @cluster-id: cluster number within die/socket the CPU belongs to
>>>
>>> suggestion from Ignor in v3:
>>>
>>>     > +# @core-id: core number within cluster the CPU belongs to
>>>
>>>     s:cluster:cluster/die:
>>>
>> We want "within cluster/die" description for core-id because we
>> support both "cores in cluster" for ARM and "cores in die" for X86.
>> Base on this routine, we only need "within socket" for cluster-id
>> because we currently only support "clusters in socket". Does this
>> make sense?
>>
>> Alternatively, the plainest documentation for the IDs is to simply
>> range **-id only to its next level topo like below. This may avoid
>> increasing complexity when more topo-ids are inserted middle.
>> But whether this way is acceptable is up to the Maintainers. :)
> 
> Rather saying we only support cluster on ARM and only dies on x86,
> I tend to view it as, we only support greater than 1 cluster on
> ARM, and greater than 1 die on x86.
> 
> IOW, x86 implicitly always has exactly 1-and-only-1 cluster,
> and arm implicitly always has exactly 1-and-only-1 die.
> 
> With this POV, then we can keep the description simple, only
> refering to the immediately above level in the hierarchy.
> 

Agreed and thanks a lot for the elaboration.

>>
>> # @socket-id: socket number within node/board the CPU belongs to
>> # @die-id: die number within socket the CPU belongs to (since 4.1)
>> # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
>> # @core-id: core number within cluster the CPU belongs to
>> # @thread-id: thread number within core the CPU belongs to
> 
> So this suggested text is fine with me.
> 

Ok. The description will be included into v7, as v6 was posted
two days ago.

Thanks,
Gavin

[PATCH v5 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 yet.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
+        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;
 }
-- 
2.23.0

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

[Daniel P. Berrangé]

On Sun, Apr 03, 2022 at 10:59:51PM +0800, Gavin Shan 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 yet.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/virt.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
> +        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;

Does this need to be conditionalized d behind a machine property, so that
we don't change behaviour of existing machine type versions ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Gavin Shan]

Hi Daniel,

On 4/4/22 4:39 PM, Daniel P. Berrangé wrote:
> On Sun, Apr 03, 2022 at 10:59:51PM +0800, Gavin Shan 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 yet.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/arm/virt.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
>> +        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;
> 
> Does this need to be conditionalized d behind a machine property, so that
> we don't change behaviour of existing machine type versions ?
> 

I think we probably needn't to do that because the default NUMA node
for the given CPU is returned based on the socket ID in next patch.
The socket ID is calculated in this patch. Otherwise, we will see
CPU topology broken warnings in Linux guest. I think we need to fix
this issue for all machine type versions.

Thanks,
Gavin

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

[Igor Mammedov]

On Mon, 4 Apr 2022 18:48:00 +0800
Gavin Shan <gshan@redhat.com> wrote:

> Hi Daniel,
> 
> On 4/4/22 4:39 PM, Daniel P. Berrangé wrote:
> > On Sun, Apr 03, 2022 at 10:59:51PM +0800, Gavin Shan 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 yet.
> >>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> ---
> >>   hw/arm/virt.c | 16 +++++++++++++++-
> >>   1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
> >> +        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;  
> > 
> > Does this need to be conditionalized d behind a machine property, so that
> > we don't change behaviour of existing machine type versions ?
> >   
> 
> I think we probably needn't to do that because the default NUMA node
> for the given CPU is returned based on the socket ID in next patch.
> The socket ID is calculated in this patch. Otherwise, we will see
> CPU topology broken warnings in Linux guest. I think we need to fix
> this issue for all machine type versions.

Agreed.
Also guest-wise it's ACPI only change, which is 'firmware' part of QEMU,
and by default we don't to version those changes unless we a pressed
into it (i.e the same policy that goes for bundled firmware)

> Thanks,
> Gavin
> 

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

[wangyanan (Y) via]

Hi Gavin,

On 2022/4/3 22:59, Gavin Shan 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 yet.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   hw/arm/virt.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
No need for "% ms->smp.sockets".
> +        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;
>   }
Otherwise, looks good to me:
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,
Yanan

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

[Gavin Shan]

Hi Yanan,

On 4/13/22 8:39 PM, wangyanan (Y) wrote:
> On 2022/4/3 22:59, Gavin Shan 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 yet.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/arm/virt.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
> No need for "% ms->smp.sockets".

Yeah, lets remove it in v6.

>> +        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;
>>   }
> Otherwise, looks good to me:
> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> 

Thanks for your time to review :)

Thanks,
Gavin

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

[wangyanan (Y) via]

On 2022/4/14 8:08, Gavin Shan wrote:
> Hi Yanan,
>
> On 4/13/22 8:39 PM, wangyanan (Y) wrote:
>> On 2022/4/3 22:59, Gavin Shan 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 yet.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>   hw/arm/virt.c | 16 +++++++++++++++-
>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
>> No need for "% ms->smp.sockets".
>
> Yeah, lets remove it in v6.
>
>>> + 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;
>>>   }
>> Otherwise, looks good to me:
>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>
>
> Thanks for your time to review :)
>
>
Oh, after further testing this patch breaks numa-test for aarch64,
which should be checked and fixed. I guess it's because we have
more IDs supported for ARM. We have to fully running the QEMU
tests before sending some patches to ensure that they are not
breaking anything. :)

Thanks,
Yanan
>
> .

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

[Gavin Shan]

Hi Yanan,

On 4/14/22 10:27 AM, wangyanan (Y) wrote:
> On 2022/4/14 8:08, Gavin Shan wrote:
>> On 4/13/22 8:39 PM, wangyanan (Y) wrote:
>>> On 2022/4/3 22:59, Gavin Shan 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 yet.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>>   hw/arm/virt.c | 16 +++++++++++++++-
>>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
>>> No need for "% ms->smp.sockets".
>>
>> Yeah, lets remove it in v6.
>>
>>>> + 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;
>>>>   }
>>> Otherwise, looks good to me:
>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>
>>
>> Thanks for your time to review :)
>>
>>
> Oh, after further testing this patch breaks numa-test for aarch64,
> which should be checked and fixed. I guess it's because we have
> more IDs supported for ARM. We have to fully running the QEMU
> tests before sending some patches to ensure that they are not
> breaking anything. :)
> 

Thanks for catching the failure and reporting back. I'm not
too much familar with QEMU's test workframe. Could you please
share the detailed commands to reproduce the failure? I will
fix in v6, which will be done in a separate patch :)

Thanks,
Gavin

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

[wangyanan (Y) via]

On 2022/4/14 10:37, Gavin Shan wrote:
> Hi Yanan,
>
> On 4/14/22 10:27 AM, wangyanan (Y) wrote:
>> On 2022/4/14 8:08, Gavin Shan wrote:
>>> On 4/13/22 8:39 PM, wangyanan (Y) wrote:
>>>> On 2022/4/3 22:59, Gavin Shan 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 yet.
>>>>>
>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>> ---
>>>>>   hw/arm/virt.c | 16 +++++++++++++++-
>>>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
>>>> No need for "% ms->smp.sockets".
>>>
>>> Yeah, lets remove it in v6.
>>>
>>>>> + 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;
>>>>>   }
>>>> Otherwise, looks good to me:
>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>>
>>>
>>> Thanks for your time to review :)
>>>
>>>
>> Oh, after further testing this patch breaks numa-test for aarch64,
>> which should be checked and fixed. I guess it's because we have
>> more IDs supported for ARM. We have to fully running the QEMU
>> tests before sending some patches to ensure that they are not
>> breaking anything. :)
>>
>
> Thanks for catching the failure and reporting back. I'm not
> too much familar with QEMU's test workframe. Could you please
> share the detailed commands to reproduce the failure? I will
> fix in v6, which will be done in a separate patch :)
>
There is a reference link: https://wiki.qemu.org/Testing
To catch the failure of this patch: "make check" will be enough.

Thanks,
Yanan
> Thanks,
> Gavin
>
>
> .

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

[Gavin Shan]

Hi Yanan,

On 4/14/22 10:49 AM, wangyanan (Y) wrote:
> On 2022/4/14 10:37, Gavin Shan wrote:
>> On 4/14/22 10:27 AM, wangyanan (Y) wrote:
>>> On 2022/4/14 8:08, Gavin Shan wrote:
>>>> On 4/13/22 8:39 PM, wangyanan (Y) wrote:
>>>>> On 2022/4/3 22:59, Gavin Shan 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 yet.
>>>>>>
>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>>> ---
>>>>>>   hw/arm/virt.c | 16 +++++++++++++++-
>>>>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>>>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
>>>>> No need for "% ms->smp.sockets".
>>>>
>>>> Yeah, lets remove it in v6.
>>>>
>>>>>> + 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;
>>>>>>   }
>>>>> Otherwise, looks good to me:
>>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>>>
>>>>
>>>> Thanks for your time to review :)
>>>>
>>>>
>>> Oh, after further testing this patch breaks numa-test for aarch64,
>>> which should be checked and fixed. I guess it's because we have
>>> more IDs supported for ARM. We have to fully running the QEMU
>>> tests before sending some patches to ensure that they are not
>>> breaking anything. :)
>>>
>>
>> Thanks for catching the failure and reporting back. I'm not
>> too much familar with QEMU's test workframe. Could you please
>> share the detailed commands to reproduce the failure? I will
>> fix in v6, which will be done in a separate patch :)
>>
> There is a reference link: https://wiki.qemu.org/Testing
> To catch the failure of this patch: "make check" will be enough.
> 

Thanks for the pointer. The issue is caused by ms->possible_cpus->cpus[n].props.thread_id.
Before this patch, it's value of [0 ... smp.cpus]. However, it's always zero
after this patch is applied because '%' operation is applied for the thread
ID.

What we need to do is to specify SMP configuration for the test case. I will
add PATCH[v6 5/5] for it.

diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
index 90bf68a5b3..6178ac21a5 100644
--- a/tests/qtest/numa-test.c
+++ b/tests/qtest/numa-test.c
@@ -223,7 +223,7 @@ 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.cores=1,smp.threads=2 "

Thanks,
Gavin

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

[wangyanan (Y) via]

On 2022/4/14 15:35, Gavin Shan wrote:
> Hi Yanan,
>
> On 4/14/22 10:49 AM, wangyanan (Y) wrote:
>> On 2022/4/14 10:37, Gavin Shan wrote:
>>> On 4/14/22 10:27 AM, wangyanan (Y) wrote:
>>>> On 2022/4/14 8:08, Gavin Shan wrote:
>>>>> On 4/13/22 8:39 PM, wangyanan (Y) wrote:
>>>>>> On 2022/4/3 22:59, Gavin Shan 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 yet.
>>>>>>>
>>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>>>> ---
>>>>>>>   hw/arm/virt.c | 16 +++++++++++++++-
>>>>>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>>>>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
>>>>>> No need for "% ms->smp.sockets".
>>>>>
>>>>> Yeah, lets remove it in v6.
>>>>>
>>>>>>> + 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;
>>>>>>>   }
>>>>>> Otherwise, looks good to me:
>>>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>>>>
>>>>>
>>>>> Thanks for your time to review :)
>>>>>
>>>>>
>>>> Oh, after further testing this patch breaks numa-test for aarch64,
>>>> which should be checked and fixed. I guess it's because we have
>>>> more IDs supported for ARM. We have to fully running the QEMU
>>>> tests before sending some patches to ensure that they are not
>>>> breaking anything. :)
>>>>
>>>
>>> Thanks for catching the failure and reporting back. I'm not
>>> too much familar with QEMU's test workframe. Could you please
>>> share the detailed commands to reproduce the failure? I will
>>> fix in v6, which will be done in a separate patch :)
>>>
>> There is a reference link: https://wiki.qemu.org/Testing
>> To catch the failure of this patch: "make check" will be enough.
>>
>
> Thanks for the pointer. The issue is caused by 
> ms->possible_cpus->cpus[n].props.thread_id.
> Before this patch, it's value of [0 ... smp.cpus]. However, it's 
> always zero
> after this patch is applied because '%' operation is applied for the 
> thread
> ID.
>
> What we need to do is to specify SMP configuration for the test case. 
> I will
> add PATCH[v6 5/5] for it.
Better to keep the fix together with this patch for good bisection.
>
> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> index 90bf68a5b3..6178ac21a5 100644
> --- a/tests/qtest/numa-test.c
> +++ b/tests/qtest/numa-test.c
> @@ -223,7 +223,7 @@ 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.cores=1,smp.threads=2 "
>
Maybe it's better to extend aarch64_numa_cpu() by adding
"socket_id, cluster_id, core_id" configuration to the -numa cpu,
given that we have them for aarch64 now.

Thanks,
Yanan
>
>
>
> .

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

[Gavin Shan]

Hi Yanan,

On 4/14/22 5:29 PM, wangyanan (Y) wrote:
> On 2022/4/14 15:35, Gavin Shan wrote:
>> On 4/14/22 10:49 AM, wangyanan (Y) wrote:
>>> On 2022/4/14 10:37, Gavin Shan wrote:
>>>> On 4/14/22 10:27 AM, wangyanan (Y) wrote:
>>>>> On 2022/4/14 8:08, Gavin Shan wrote:
>>>>>> On 4/13/22 8:39 PM, wangyanan (Y) wrote:
>>>>>>> On 2022/4/3 22:59, Gavin Shan 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 yet.
>>>>>>>>
>>>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>>>>> ---
>>>>>>>>   hw/arm/virt.c | 16 +++++++++++++++-
>>>>>>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>>>>>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;
>>>>>>> No need for "% ms->smp.sockets".
>>>>>>
>>>>>> Yeah, lets remove it in v6.
>>>>>>
>>>>>>>> + 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;
>>>>>>>>   }
>>>>>>> Otherwise, looks good to me:
>>>>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>>>>>
>>>>>>
>>>>>> Thanks for your time to review :)
>>>>>>
>>>>>>
>>>>> Oh, after further testing this patch breaks numa-test for aarch64,
>>>>> which should be checked and fixed. I guess it's because we have
>>>>> more IDs supported for ARM. We have to fully running the QEMU
>>>>> tests before sending some patches to ensure that they are not
>>>>> breaking anything. :)
>>>>>
>>>>
>>>> Thanks for catching the failure and reporting back. I'm not
>>>> too much familar with QEMU's test workframe. Could you please
>>>> share the detailed commands to reproduce the failure? I will
>>>> fix in v6, which will be done in a separate patch :)
>>>>
>>> There is a reference link: https://wiki.qemu.org/Testing
>>> To catch the failure of this patch: "make check" will be enough.
>>>
>>
>> Thanks for the pointer. The issue is caused by ms->possible_cpus->cpus[n].props.thread_id.
>> Before this patch, it's value of [0 ... smp.cpus]. However, it's always zero
>> after this patch is applied because '%' operation is applied for the thread
>> ID.
>>
>> What we need to do is to specify SMP configuration for the test case. I will
>> add PATCH[v6 5/5] for it.
> Better to keep the fix together with this patch for good bisection.

Agreed, it will be part of PATCH[v6 02/04].

>>
>> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
>> index 90bf68a5b3..6178ac21a5 100644
>> --- a/tests/qtest/numa-test.c
>> +++ b/tests/qtest/numa-test.c
>> @@ -223,7 +223,7 @@ 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.cores=1,smp.threads=2 "
>>
> Maybe it's better to extend aarch64_numa_cpu() by adding
> "socket_id, cluster_id, core_id" configuration to the -numa cpu,
> given that we have them for aarch64 now.
> 

Exactly, I will use the following command in v6:

   -machine smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2

Thanks,
Gavin

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

[Jonathan Cameron via]

On Thu, 14 Apr 2022 15:35:41 +0800
Gavin Shan <gshan@redhat.com> wrote:

> Hi Yanan,
> 
> On 4/14/22 10:49 AM, wangyanan (Y) wrote:
> > On 2022/4/14 10:37, Gavin Shan wrote:  
> >> On 4/14/22 10:27 AM, wangyanan (Y) wrote:  
> >>> On 2022/4/14 8:08, Gavin Shan wrote:  
> >>>> On 4/13/22 8:39 PM, wangyanan (Y) wrote:  
> >>>>> On 2022/4/3 22:59, Gavin Shan 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 yet.
> >>>>>>
> >>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >>>>>> ---
> >>>>>>   hw/arm/virt.c | 16 +++++++++++++++-
> >>>>>>   1 file changed, 15 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >>>>>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets;  
> >>>>> No need for "% ms->smp.sockets".  
> >>>>
> >>>> Yeah, lets remove it in v6.
> >>>>  
> >>>>>> + 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;
> >>>>>>   }  
> >>>>> Otherwise, looks good to me:
> >>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> >>>>>  
> >>>>
> >>>> Thanks for your time to review :)
> >>>>
> >>>>  
> >>> Oh, after further testing this patch breaks numa-test for aarch64,
> >>> which should be checked and fixed. I guess it's because we have
> >>> more IDs supported for ARM. We have to fully running the QEMU
> >>> tests before sending some patches to ensure that they are not
> >>> breaking anything. :)
> >>>  
> >>
> >> Thanks for catching the failure and reporting back. I'm not
> >> too much familar with QEMU's test workframe. Could you please
> >> share the detailed commands to reproduce the failure? I will
> >> fix in v6, which will be done in a separate patch :)
> >>  
> > There is a reference link: https://wiki.qemu.org/Testing
> > To catch the failure of this patch: "make check" will be enough.
> >   

Speaking from experience, best bet is also upload to a gitlab repo
and let the CI hit things. It will catch this plus any weirdness
elsewhere without you having to figure out too much unless you see
a failure.

The CI is pretty good though more tests always needed!

Jonathan

> 
> Thanks for the pointer. The issue is caused by ms->possible_cpus->cpus[n].props.thread_id.
> Before this patch, it's value of [0 ... smp.cpus]. However, it's always zero
> after this patch is applied because '%' operation is applied for the thread
> ID.
> 
> What we need to do is to specify SMP configuration for the test case. I will
> add PATCH[v6 5/5] for it.
> 
> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> index 90bf68a5b3..6178ac21a5 100644
> --- a/tests/qtest/numa-test.c
> +++ b/tests/qtest/numa-test.c
> @@ -223,7 +223,7 @@ 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.cores=1,smp.threads=2 "
> 
> Thanks,
> Gavin
> 
> 
> 

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

[Gavin Shan]

Hi Jonathan,

On 4/14/22 5:33 PM, Jonathan Cameron wrote:
> On Thu, 14 Apr 2022 15:35:41 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> On 4/14/22 10:49 AM, wangyanan (Y) wrote:
>>> On 2022/4/14 10:37, Gavin Shan wrote:
>>>> On 4/14/22 10:27 AM, wangyanan (Y) wrote:
>>>>> On 2022/4/14 8:08, Gavin Shan wrote:
>>>>>> On 4/13/22 8:39 PM, wangyanan (Y) wrote:
>>>>>>> On 2022/4/3 22:59, Gavin Shan wrote:

[...]

>>>>> Oh, after further testing this patch breaks numa-test for aarch64,
>>>>> which should be checked and fixed. I guess it's because we have
>>>>> more IDs supported for ARM. We have to fully running the QEMU
>>>>> tests before sending some patches to ensure that they are not
>>>>> breaking anything. :)
>>>>>   
>>>>
>>>> Thanks for catching the failure and reporting back. I'm not
>>>> too much familar with QEMU's test workframe. Could you please
>>>> share the detailed commands to reproduce the failure? I will
>>>> fix in v6, which will be done in a separate patch :)
>>>>   
>>> There is a reference link: https://wiki.qemu.org/Testing
>>> To catch the failure of this patch: "make check" will be enough.
>>>    
> 
> Speaking from experience, best bet is also upload to a gitlab repo
> and let the CI hit things. It will catch this plus any weirdness
> elsewhere without you having to figure out too much unless you see
> a failure.
> 
> The CI is pretty good though more tests always needed!
> 

Thanks a lot for the hint. I usually use github to host my code.
I will setup gitlab repositories so that the verification and
tests can be automated. Not sure if there is any document on
how to trigger the automatic verification and testing from
gitlab?

[...]

Thanks,
Gavin

[PATCH v5 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 3174526730..d8b621ef79 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 v5 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 | 100 +++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 62 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 4086879ebf..4b0f9df3e3 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
+    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);
-    }
+    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

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

[Jonathan Cameron via]

On Sun,  3 Apr 2022 22:59:53 +0800
Gavin Shan <gshan@redhat.com> wrote:

> 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>

Hi Gavin,

My compiler isn't being very smart today and gives a bunch of
maybe used uninitialized for socket_offset, cluster_offset and core_offset.

They probably are initialized in all real paths, but I think you need to
set them to something at instantiation to keep the compiler happy.

Thanks,

Jonathan

> ---
>  hw/acpi/aml-build.c | 100 +++++++++++++++++---------------------------
>  1 file changed, 38 insertions(+), 62 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 4086879ebf..4b0f9df3e3 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
> +    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);
> -    }
> +    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);
>  }
>  

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

[Gavin Shan]

Hi Jonathan,

On 4/12/22 11:40 PM, Jonathan Cameron wrote:
> On Sun,  3 Apr 2022 22:59:53 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> 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>
> 
> My compiler isn't being very smart today and gives a bunch of
> maybe used uninitialized for socket_offset, cluster_offset and core_offset.
> 
> They probably are initialized in all real paths, but I think you need to
> set them to something at instantiation to keep the compiler happy.
> 

Thanks for reporting the warning raised from the compiler. I think
your compiler may be smarter than mine :)

Yeah, they're initialized in all real paths after checking the code
again. So lets initialize them with zeroes in v6, but I would hold
for Igor and Yanan's reviews on this (v5) series.

Thanks,
Gavin

> 
>> ---
>>   hw/acpi/aml-build.c | 100 +++++++++++++++++---------------------------
>>   1 file changed, 38 insertions(+), 62 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 4086879ebf..4b0f9df3e3 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
>> +    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);
>> -    }
>> +    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);
>>   }
>>   
> 

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

[Igor Mammedov]

On Sun,  3 Apr 2022 22:59:53 +0800
Gavin Shan <gshan@redhat.com> wrote:

> 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 | 100 +++++++++++++++++---------------------------
>  1 file changed, 38 insertions(+), 62 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 4086879ebf..4b0f9df3e3 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
> +    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);
> -    }
> +    for (n = 0; n < cpus->len; n++) {

> +        if (cpus->cpus[n].props.socket_id != socket_id) {
> +            socket_id = cpus->cpus[n].props.socket_id;

this relies on cpus->cpus[n].props.*_id being sorted form top to down levels
I'd add here and for other container_id an assert() that checks for that
specific ID goes in only one direction, to be able to detect when rule is broken.

otherwise on may end up with duplicate containers silently.

> +            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) {

why <= instead of < is used here?

> +            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);
>  }
>  

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

[Gavin Shan]

Hi Igor,

On 4/13/22 9:52 PM, Igor Mammedov wrote:
> On Sun,  3 Apr 2022 22:59:53 +0800
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> 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 | 100 +++++++++++++++++---------------------------
>>   1 file changed, 38 insertions(+), 62 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 4086879ebf..4b0f9df3e3 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
>> +    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);
>> -    }
>> +    for (n = 0; n < cpus->len; n++) {
> 
>> +        if (cpus->cpus[n].props.socket_id != socket_id) {
>> +            socket_id = cpus->cpus[n].props.socket_id;
> 
> this relies on cpus->cpus[n].props.*_id being sorted form top to down levels
> I'd add here and for other container_id an assert() that checks for that
> specific ID goes in only one direction, to be able to detect when rule is broken.
> 
> otherwise on may end up with duplicate containers silently.
> 

Exactly. cpus->cpus[n].props.*_id is sorted as you said in virt_possible_cpu_arch_ids().
The only user of build_pptt() is arm/virt machine. So it's fine. However, I think I
may need add comments for this in v6.

     /*
      * 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.
      */

The implementation in v3 looks complicated, but comprehensive. The one
in this revision (v6) looks simple, but the we're losing flexibility :)


>> +            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) {
> 
> why <= instead of < is used here?
> 

It's the counterpart to the one in the original implementation,
which is "if (ms->smp.threads > 1)". the nodes for threads won't
be created until ms->smp.threads is bigger than 1. If we want
to use "<" here, then the code will be changed like below in v6.
However, I really don't think it's necessary.

            if (ms->smp.threads < 2) {
                :
            }


>> +            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);
>>   }
>>   

Thanks,
Gavin

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

[wangyanan (Y) via]

On 2022/4/14 8:33, Gavin Shan wrote:
> Hi Igor,
>
> On 4/13/22 9:52 PM, Igor Mammedov wrote:
>> On Sun,  3 Apr 2022 22:59:53 +0800
>> Gavin Shan <gshan@redhat.com> wrote:
>>
>>> 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 | 100 
>>> +++++++++++++++++---------------------------
>>>   1 file changed, 38 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>> index 4086879ebf..4b0f9df3e3 100644
>>> --- a/hw/acpi/aml-build.c
>>> +++ b/hw/acpi/aml-build.c
>>> @@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
>>> +    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);
>>> -    }
>>> +    for (n = 0; n < cpus->len; n++) {
>>
>>> +        if (cpus->cpus[n].props.socket_id != socket_id) {
>>> +            socket_id = cpus->cpus[n].props.socket_id;
>>
>> this relies on cpus->cpus[n].props.*_id being sorted form top to down 
>> levels
>> I'd add here and for other container_id an assert() that checks for that
>> specific ID goes in only one direction, to be able to detect when 
>> rule is broken.
>>
>> otherwise on may end up with duplicate containers silently.
>>
>
> Exactly. cpus->cpus[n].props.*_id is sorted as you said in 
> virt_possible_cpu_arch_ids().
> The only user of build_pptt() is arm/virt machine. So it's fine. 
> However, I think I
> may need add comments for this in v6.
>
>     /*
>      * 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.
>      */
>
> The implementation in v3 looks complicated, but comprehensive. The one
> in this revision (v6) looks simple, but the we're losing flexibility :)
>
>
>>> +            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) {
>>
>> why <= instead of < is used here?
>>
>
> It's the counterpart to the one in the original implementation,
> which is "if (ms->smp.threads > 1)". the nodes for threads won't
> be created until ms->smp.threads is bigger than 1. If we want
> to use "<" here, then the code will be changed like below in v6.
> However, I really don't think it's necessary.
>
>            if (ms->smp.threads < 2) {
>                :
>            }
>
The smp_parse() guarantees that we either have "one threads" or
"multi threads" in ms->smp. So I think there are two proper choices:
if (ms->smp.threads == 1) {
...
} else {
...
}

or

if (ms->smp.threads > 1) {
...
} else {
...
}

Thanks,
Yanan
>
>>> + 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);
>>>   }
>
> Thanks,
> Gavin
>
> .

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

[Gavin Shan]

Hi Yanan,

On 4/14/22 10:56 AM, wangyanan (Y) wrote:
> On 2022/4/14 8:33, Gavin Shan wrote:
>> On 4/13/22 9:52 PM, Igor Mammedov wrote:
>>> On Sun,  3 Apr 2022 22:59:53 +0800
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>>> 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 | 100 +++++++++++++++++---------------------------
>>>>   1 file changed, 38 insertions(+), 62 deletions(-)
>>>>
>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>> index 4086879ebf..4b0f9df3e3 100644
>>>> --- a/hw/acpi/aml-build.c
>>>> +++ b/hw/acpi/aml-build.c
>>>> @@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
>>>> +    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);
>>>> -    }
>>>> +    for (n = 0; n < cpus->len; n++) {
>>>
>>>> +        if (cpus->cpus[n].props.socket_id != socket_id) {
>>>> +            socket_id = cpus->cpus[n].props.socket_id;
>>>
>>> this relies on cpus->cpus[n].props.*_id being sorted form top to down levels
>>> I'd add here and for other container_id an assert() that checks for that
>>> specific ID goes in only one direction, to be able to detect when rule is broken.
>>>
>>> otherwise on may end up with duplicate containers silently.
>>>
>>
>> Exactly. cpus->cpus[n].props.*_id is sorted as you said in virt_possible_cpu_arch_ids().
>> The only user of build_pptt() is arm/virt machine. So it's fine. However, I think I
>> may need add comments for this in v6.
>>
>>     /*
>>      * 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.
>>      */
>>
>> The implementation in v3 looks complicated, but comprehensive. The one
>> in this revision (v6) looks simple, but the we're losing flexibility :)
>>
>>
>>>> +            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) {
>>>
>>> why <= instead of < is used here?
>>>
>>
>> It's the counterpart to the one in the original implementation,
>> which is "if (ms->smp.threads > 1)". the nodes for threads won't
>> be created until ms->smp.threads is bigger than 1. If we want
>> to use "<" here, then the code will be changed like below in v6.
>> However, I really don't think it's necessary.
>>
>>            if (ms->smp.threads < 2) {
>>                :
>>            }
>>
> The smp_parse() guarantees that we either have "one threads" or
> "multi threads" in ms->smp. So I think there are two proper choices:
> if (ms->smp.threads == 1) {
> ...
> } else {
> ...
> }
> 
> or
> 
> if (ms->smp.threads > 1) {
> ...
> } else {
> ...
> }
> 
Yes, it's guaranteed ms->smp.threads is equal to or larger than 1.
I will use the pattern of 'if (ms->smp.threads == 1)' in v6.

>>>> + 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);
>>>>   }

Thanks,
Gavin

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

[Igor Mammedov]

On Thu, 14 Apr 2022 08:33:29 +0800
Gavin Shan <gshan@redhat.com> wrote:

> Hi Igor,
> 
> On 4/13/22 9:52 PM, Igor Mammedov wrote:
> > On Sun,  3 Apr 2022 22:59:53 +0800
> > Gavin Shan <gshan@redhat.com> wrote:
> >   
> >> 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 | 100 +++++++++++++++++---------------------------
> >>   1 file changed, 38 insertions(+), 62 deletions(-)
> >>
> >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> >> index 4086879ebf..4b0f9df3e3 100644
> >> --- a/hw/acpi/aml-build.c
> >> +++ b/hw/acpi/aml-build.c
> >> @@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
> >> +    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);
> >> -    }
> >> +    for (n = 0; n < cpus->len; n++) {  
> >   
> >> +        if (cpus->cpus[n].props.socket_id != socket_id) {
> >> +            socket_id = cpus->cpus[n].props.socket_id;  
> > 
> > this relies on cpus->cpus[n].props.*_id being sorted form top to down levels
> > I'd add here and for other container_id an assert() that checks for that
> > specific ID goes in only one direction, to be able to detect when rule is broken.
> > 
> > otherwise on may end up with duplicate containers silently.
> >   
> 
> Exactly. cpus->cpus[n].props.*_id is sorted as you said in virt_possible_cpu_arch_ids().
> The only user of build_pptt() is arm/virt machine. So it's fine. However, I think I
> may need add comments for this in v6.
> 
>      /*
>       * 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.
>       */
> 
> The implementation in v3 looks complicated, but comprehensive. The one
> in this revision (v6) looks simple, but the we're losing flexibility :)


comment is not enough, as it will break silently that's why I suggested
sprinkling asserts() here.

[...]
> Thanks,
> Gavin
> 

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

[Gavin Shan]

[-- Attachment #1: Type: text/plain, Size: 5249 bytes --]

Hi Igor,

On 4/19/22 4:54 PM, Igor Mammedov wrote:
> On Thu, 14 Apr 2022 08:33:29 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> On 4/13/22 9:52 PM, Igor Mammedov wrote:
>>> On Sun,  3 Apr 2022 22:59:53 +0800
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>    
>>>> 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 | 100 +++++++++++++++++---------------------------
>>>>    1 file changed, 38 insertions(+), 62 deletions(-)
>>>>
>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>> index 4086879ebf..4b0f9df3e3 100644
>>>> --- a/hw/acpi/aml-build.c
>>>> +++ b/hw/acpi/aml-build.c
>>>> @@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
>>>> +    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);
>>>> -    }
>>>> +    for (n = 0; n < cpus->len; n++) {
>>>    
>>>> +        if (cpus->cpus[n].props.socket_id != socket_id) {
>>>> +            socket_id = cpus->cpus[n].props.socket_id;
>>>
>>> this relies on cpus->cpus[n].props.*_id being sorted form top to down levels
>>> I'd add here and for other container_id an assert() that checks for that
>>> specific ID goes in only one direction, to be able to detect when rule is broken.
>>>
>>> otherwise on may end up with duplicate containers silently.
>>>    
>>
>> Exactly. cpus->cpus[n].props.*_id is sorted as you said in virt_possible_cpu_arch_ids().
>> The only user of build_pptt() is arm/virt machine. So it's fine. However, I think I
>> may need add comments for this in v6.
>>
>>       /*
>>        * 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.
>>        */
>>
>> The implementation in v3 looks complicated, but comprehensive. The one
>> in this revision (v6) looks simple, but the we're losing flexibility :)
> 
> 
> comment is not enough, as it will break silently that's why I suggested
> sprinkling asserts() here.
> 

I don't think it breaks anything. Duplicated PPTT entries are allowed in
linux at least. The IDs in the duplicated PPTT entries should be same.
Otherwise, the exposed CPU topology is really broken.

I don't think it's harmful to add the check and assert, so I will introduce
a helper function like below in v7. Sadly that v6 was posted before I received
your confirm. Igor, could you please the changes, to be included into v7,
looks good to you? The complete patch is also attached :)

+static bool pptt_entry_exists(MachineState *ms, int n, bool check_socket_id,
+                              bool check_cluster_id, bool check_core_id)
+{
+    CPUArchId *cpus = ms->possible_cpus->cpus;
+    CpuInstanceProperties *t = &cpus[n].props;
+    CpuInstanceProperties *s;
+    bool match;
+    int i;
+
+    for (i = 0; i < n; i++) {
+        match = true;
+        s = &cpus[i].props;
+
+        if (check_socket_id && s->socket_id != t->socket_id) {
+            match = false;
+        }
+
+        if (match && check_cluster_id && s->cluster_id != t->cluster_id) {
+            match = false;
+        }
+
+        if (match && check_core_id && s->core_id != t->core_id) {
+            match = false;
+        }
+
+        if (match) {
+            return true;
+        }
+    }
+
+    return false;
+}

The following assert() will be applied in build_pptt():

assert(!pptt_entry_exists(ms, n, true, false, false));           /* socket  */
assert(!pptt_entry_exists(ms, n, true, true, false));            /* cluster */
assert(!pptt_entry_exists(ms, n, true,
            mc->smp_props.clusters_supported, true));             /* core    */

Thanks,
Gavin


[-- Attachment #2: 0001-hw-acpi-aml-build-Use-existing-CPU-topology-to-build.patch --]
[-- Type: text/x-patch, Size: 7417 bytes --]

From cb26c277478a7415b8f51fd8f495bac7a2f42f0e Mon Sep 17 00:00:00 2001
From: Gavin Shan <gshan@redhat.com>
Date: Mon, 18 Apr 2022 09:47:27 +0800
Subject: [PATCH v7] hw/acpi/aml-build: Use existing CPU topology to build PPTT
 table
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 | 143 +++++++++++++++++++++++++-------------------
 1 file changed, 81 insertions(+), 62 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 4086879ebf..4a9ecb84b0 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1998,90 +1998,109 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
  * ACPI spec, Revision 6.3
  * 5.2.29 Processor Properties Topology Table (PPTT)
  */
+static bool pptt_entry_exists(MachineState *ms, int n, bool check_socket_id,
+                              bool check_cluster_id, bool check_core_id)
+{
+    CPUArchId *cpus = ms->possible_cpus->cpus;
+    CpuInstanceProperties *t = &cpus[n].props;
+    CpuInstanceProperties *s;
+    bool match;
+    int i;
+
+    for (i = 0; i < n; i++) {
+        match = true;
+        s = &cpus[i].props;
+
+        if (check_socket_id && s->socket_id != t->socket_id) {
+            match = false;
+        }
+
+        if (match && check_cluster_id && s->cluster_id != t->cluster_id) {
+            match = false;
+        }
+
+        if (match && check_core_id && s->core_id != t->core_id) {
+            match = false;
+        }
+
+        if (match) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 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. The check is done by pptt_entry_exists().
+     */
+    for (n = 0; n < cpus->len; n++) {
+        if (cpus->cpus[n].props.socket_id != socket_id) {
+            assert(!pptt_entry_exists(ms, n, true, false, false));
+            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) {
+                assert(!pptt_entry_exists(ms, n, true, true, false));
+                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) {
+                assert(!pptt_entry_exists(ms, n, true,
+                           mc->smp_props.clusters_supported, true));
+                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

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

[Igor Mammedov]

On Wed, 20 Apr 2022 13:19:34 +0800
Gavin Shan <gshan@redhat.com> wrote:

> Hi Igor,
> 
> On 4/19/22 4:54 PM, Igor Mammedov wrote:
> > On Thu, 14 Apr 2022 08:33:29 +0800
> > Gavin Shan <gshan@redhat.com> wrote:  
> >> On 4/13/22 9:52 PM, Igor Mammedov wrote:  
> >>> On Sun,  3 Apr 2022 22:59:53 +0800
> >>> Gavin Shan <gshan@redhat.com> wrote:
> >>>      
> >>>> 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 | 100 +++++++++++++++++---------------------------
> >>>>    1 file changed, 38 insertions(+), 62 deletions(-)
> >>>>
> >>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> >>>> index 4086879ebf..4b0f9df3e3 100644
> >>>> --- a/hw/acpi/aml-build.c
> >>>> +++ b/hw/acpi/aml-build.c
> >>>> @@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
> >>>> +    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);
> >>>> -    }
> >>>> +    for (n = 0; n < cpus->len; n++) {  
> >>>      
> >>>> +        if (cpus->cpus[n].props.socket_id != socket_id) {
> >>>> +            socket_id = cpus->cpus[n].props.socket_id;  
> >>>
> >>> this relies on cpus->cpus[n].props.*_id being sorted form top to down levels
> >>> I'd add here and for other container_id an assert() that checks for that
> >>> specific ID goes in only one direction, to be able to detect when rule is broken.
> >>>
> >>> otherwise on may end up with duplicate containers silently.
> >>>      
> >>
> >> Exactly. cpus->cpus[n].props.*_id is sorted as you said in virt_possible_cpu_arch_ids().
> >> The only user of build_pptt() is arm/virt machine. So it's fine. However, I think I
> >> may need add comments for this in v6.
> >>
> >>       /*
> >>        * 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.
> >>        */
> >>
> >> The implementation in v3 looks complicated, but comprehensive. The one
> >> in this revision (v6) looks simple, but the we're losing flexibility :)  
> > 
> > 
> > comment is not enough, as it will break silently that's why I suggested
> > sprinkling asserts() here.
> >   
> 
> I don't think it breaks anything. Duplicated PPTT entries are allowed in
> linux at least. The IDs in the duplicated PPTT entries should be same.
> Otherwise, the exposed CPU topology is really broken.

Spec doesn't say anything about allowing duplicate entries so I'd rather
avoid that (if you find a such provision in spec then put a reference
in this commit message to end discussion on duplicates).


> 
> I don't think it's harmful to add the check and assert, so I will introduce
> a helper function like below in v7. Sadly that v6 was posted before I received
> your confirm. Igor, could you please the changes, to be included into v7,
> looks good to you? The complete patch is also attached :)
> 
> +static bool pptt_entry_exists(MachineState *ms, int n, bool check_socket_id,
> +                              bool check_cluster_id, bool check_core_id)
> +{
> +    CPUArchId *cpus = ms->possible_cpus->cpus;
> +    CpuInstanceProperties *t = &cpus[n].props;
> +    CpuInstanceProperties *s;
> +    bool match;
> +    int i;
> +
> +    for (i = 0; i < n; i++) {

Wouldn't it make whole thing O(n^2) in worst case?

I suggest put asserts directly into build_pptt() and considering that
it relies on ids being sorted, do something like this:
   assert(foo_id_val > previous_id)
which will ensure that id doesn't jump back unexpectedly


> +        match = true;
> +        s = &cpus[i].props;
> +
> +        if (check_socket_id && s->socket_id != t->socket_id) {
> +            match = false;
> +        }
> +
> +        if (match && check_cluster_id && s->cluster_id != t->cluster_id) {
> +            match = false;
> +        }
> +
> +        if (match && check_core_id && s->core_id != t->core_id) {
> +            match = false;
> +        }
> +
> +        if (match) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> 
> The following assert() will be applied in build_pptt():
> 
> assert(!pptt_entry_exists(ms, n, true, false, false));           /* socket  */
> assert(!pptt_entry_exists(ms, n, true, true, false));            /* cluster */
> assert(!pptt_entry_exists(ms, n, true,
>             mc->smp_props.clusters_supported, true));             /* core    */
> 
> Thanks,
> Gavin
> 

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

[Gavin Shan]

Hi Igor,

On 4/20/22 4:10 PM, Igor Mammedov wrote:
> On Wed, 20 Apr 2022 13:19:34 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> On 4/19/22 4:54 PM, Igor Mammedov wrote:
>>> On Thu, 14 Apr 2022 08:33:29 +0800
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>> On 4/13/22 9:52 PM, Igor Mammedov wrote:
>>>>> On Sun,  3 Apr 2022 22:59:53 +0800
>>>>> Gavin Shan <gshan@redhat.com> wrote:
>>>>>       
>>>>>> 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 | 100 +++++++++++++++++---------------------------
>>>>>>     1 file changed, 38 insertions(+), 62 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>>>> index 4086879ebf..4b0f9df3e3 100644
>>>>>> --- a/hw/acpi/aml-build.c
>>>>>> +++ b/hw/acpi/aml-build.c
>>>>>> @@ -2002,86 +2002,62 @@ 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, cluster_offset, core_offset;
>>>>>> +    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);
>>>>>> -    }
>>>>>> +    for (n = 0; n < cpus->len; n++) {
>>>>>       
>>>>>> +        if (cpus->cpus[n].props.socket_id != socket_id) {
>>>>>> +            socket_id = cpus->cpus[n].props.socket_id;
>>>>>
>>>>> this relies on cpus->cpus[n].props.*_id being sorted form top to down levels
>>>>> I'd add here and for other container_id an assert() that checks for that
>>>>> specific ID goes in only one direction, to be able to detect when rule is broken.
>>>>>
>>>>> otherwise on may end up with duplicate containers silently.
>>>>>       
>>>>
>>>> Exactly. cpus->cpus[n].props.*_id is sorted as you said in virt_possible_cpu_arch_ids().
>>>> The only user of build_pptt() is arm/virt machine. So it's fine. However, I think I
>>>> may need add comments for this in v6.
>>>>
>>>>        /*
>>>>         * 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.
>>>>         */
>>>>
>>>> The implementation in v3 looks complicated, but comprehensive. The one
>>>> in this revision (v6) looks simple, but the we're losing flexibility :)
>>>
>>>
>>> comment is not enough, as it will break silently that's why I suggested
>>> sprinkling asserts() here.
>>>    
>>
>> I don't think it breaks anything. Duplicated PPTT entries are allowed in
>> linux at least. The IDs in the duplicated PPTT entries should be same.
>> Otherwise, the exposed CPU topology is really broken.
> 
> Spec doesn't say anything about allowing duplicate entries so I'd rather
> avoid that (if you find a such provision in spec then put a reference
> in this commit message to end discussion on duplicates).
> 

Yes, Spec doesn't say it's allowed. I checked linux implementation on
how PPTT table is parsed. Duplicate entries won't break anything actually
in Linux. I'm not sure about other OS. So I think it's still needed.

> 
>>
>> I don't think it's harmful to add the check and assert, so I will introduce
>> a helper function like below in v7. Sadly that v6 was posted before I received
>> your confirm. Igor, could you please the changes, to be included into v7,
>> looks good to you? The complete patch is also attached :)
>>
>> +static bool pptt_entry_exists(MachineState *ms, int n, bool check_socket_id,
>> +                              bool check_cluster_id, bool check_core_id)
>> +{
>> +    CPUArchId *cpus = ms->possible_cpus->cpus;
>> +    CpuInstanceProperties *t = &cpus[n].props;
>> +    CpuInstanceProperties *s;
>> +    bool match;
>> +    int i;
>> +
>> +    for (i = 0; i < n; i++) {
> 
> Wouldn't it make whole thing O(n^2) in worst case?
> 
> I suggest put asserts directly into build_pptt() and considering that
> it relies on ids being sorted, do something like this:
>     assert(foo_id_val > previous_id)
> which will ensure that id doesn't jump back unexpectedly
> 

Yes, your suggested method is much simpler and more efficient. I will
include it in v7. By the way, please skip v6 and go ahead to review
v7 directly. v7 should be posted shortly after some tests :)

> 
>> +        match = true;
>> +        s = &cpus[i].props;
>> +
>> +        if (check_socket_id && s->socket_id != t->socket_id) {
>> +            match = false;
>> +        }
>> +
>> +        if (match && check_cluster_id && s->cluster_id != t->cluster_id) {
>> +            match = false;
>> +        }
>> +
>> +        if (match && check_core_id && s->core_id != t->core_id) {
>> +            match = false;
>> +        }
>> +
>> +        if (match) {
>> +            return true;
>> +        }
>> +    }
>> +
>> +    return false;
>> +}
>>
>> The following assert() will be applied in build_pptt():
>>
>> assert(!pptt_entry_exists(ms, n, true, false, false));           /* socket  */
>> assert(!pptt_entry_exists(ms, n, true, true, false));            /* cluster */
>> assert(!pptt_entry_exists(ms, n, true,
>>              mc->smp_props.clusters_supported, true));             /* core    */
>>

Thanks,
Gavin

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

[wangyanan (Y) via]

Hi Gavin,

On 2022/4/3 22:59, Gavin Shan wrote:
> 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 | 100 +++++++++++++++++---------------------------
>   1 file changed, 38 insertions(+), 62 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 4086879ebf..4b0f9df3e3 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2002,86 +2002,62 @@ 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;
nit: why not using "int" for the ID variables? (to be consistent with how
the IDs are stored in CpuInstanceProperties).

Thanks,
Yanan
> +    uint32_t socket_offset, cluster_offset, core_offset;
> +    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);
> -    }
> +    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);
>   }
>   

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

[Gavin Shan]

Hi Yanan,

On 4/14/22 11:09 AM, wangyanan (Y) wrote:
> On 2022/4/3 22:59, Gavin Shan wrote:
>> 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 | 100 +++++++++++++++++---------------------------
>>   1 file changed, 38 insertions(+), 62 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 4086879ebf..4b0f9df3e3 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2002,86 +2002,62 @@ 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;
> nit: why not using "int" for the ID variables? (to be consistent with how
> the IDs are stored in CpuInstanceProperties).
> 

They're actually "int64_t". CPUInstanceProperty is declared in
qemu/build/qapi/qapi-types-machine.h like below:

struct CpuInstanceProperties {
     bool has_node_id;
     int64_t node_id;
     bool has_socket_id;
     int64_t socket_id;
     bool has_die_id;
     int64_t die_id;
     bool has_cluster_id;
     int64_t cluster_id;
     bool has_core_id;
     int64_t core_id;
     bool has_thread_id;
     int64_t thread_id;
};

I doubt if we're using same configurations to build QEMU. I use
the following one:

configure_qemu() {
    ./configure --target-list=aarch64-softmmu --enable-numa    \
    --enable-debug --disable-werror --enable-fdt --enable-kvm  \
    --disable-mpath --disable-xen --disable-vnc --disable-docs \
    --python=/usr/bin/python3 $@
}

>> +    uint32_t socket_offset, cluster_offset, core_offset;
>> +    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);
>> -    }
>> +    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);
>>   }

Thanks,
Gavin

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

[wangyanan (Y) via]

On 2022/4/14 15:45, Gavin Shan wrote:
> Hi Yanan,
>
> On 4/14/22 11:09 AM, wangyanan (Y) wrote:
>> On 2022/4/3 22:59, Gavin Shan wrote:
>>> 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 | 100 
>>> +++++++++++++++++---------------------------
>>>   1 file changed, 38 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>> index 4086879ebf..4b0f9df3e3 100644
>>> --- a/hw/acpi/aml-build.c
>>> +++ b/hw/acpi/aml-build.c
>>> @@ -2002,86 +2002,62 @@ 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;
>> nit: why not using "int" for the ID variables? (to be consistent with 
>> how
>> the IDs are stored in CpuInstanceProperties).
>>
>
> They're actually "int64_t". CPUInstanceProperty is declared in
> qemu/build/qapi/qapi-types-machine.h like below:
>
> struct CpuInstanceProperties {
>     bool has_node_id;
>     int64_t node_id;
>     bool has_socket_id;
>     int64_t socket_id;
>     bool has_die_id;
>     int64_t die_id;
>     bool has_cluster_id;
>     int64_t cluster_id;
>     bool has_core_id;
>     int64_t core_id;
>     bool has_thread_id;
>     int64_t thread_id;
> };
>
Yes, you are right.
> I doubt if we're using same configurations to build QEMU. I use
> the following one:
>
> configure_qemu() {
>    ./configure --target-list=aarch64-softmmu --enable-numa    \
>    --enable-debug --disable-werror --enable-fdt --enable-kvm  \
>    --disable-mpath --disable-xen --disable-vnc --disable-docs \
>    --python=/usr/bin/python3 $@
> }
>
Thanks,
Yanan
>>> +    uint32_t socket_offset, cluster_offset, core_offset;
>>> +    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);
>>> -    }
>>> +    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);
>>>   }
>
> Thanks,
> Gavin
>
>
> .

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

[Gavin Shan]

Hi Igor and Yanan,

On 4/3/22 10:59 PM, Gavin Shan wrote:
> 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
> 

Please let me know if you have more comments for this series, thanks
in advance.

Thanks,
Gavin

> Changelog
> =========
> 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        | 100 ++++++++++++++-----------------------
>   hw/arm/virt.c              |  20 +++++++-
>   hw/core/machine-hmp-cmds.c |   4 ++
>   hw/core/machine.c          |  16 ++++++
>   qapi/machine.json          |   6 ++-
>   5 files changed, 80 insertions(+), 66 deletions(-)
>