* [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology
2022-03-23 7:24 [PATCH v3 0/4] hw/arm/virt: Fix CPU's default NUMA node ID Gavin Shan
@ 2022-03-23 7:24 ` Gavin Shan
2022-03-25 13:19 ` Igor Mammedov
` (2 more replies)
2022-03-23 7:24 ` [PATCH v3 2/4] hw/arm/virt: Fix CPU's default NUMA node ID Gavin Shan
` (2 subsequent siblings)
3 siblings, 3 replies; 23+ messages in thread
From: Gavin Shan @ 2022-03-23 7:24 UTC (permalink / raw)
To: qemu-arm
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, shan.gavin, imammedo
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. Besides, the cluster
ID for the given CPU is assigned because it has been supported
on arm/virt machine.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/arm/virt.c | 11 +++++++++++
qapi/machine.json | 6 ++++--
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d2e5ecd234..064eac42f7 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,6 +2519,16 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
ms->possible_cpus->cpus[n].type = ms->cpu_type;
ms->possible_cpus->cpus[n].arch_id =
virt_cpu_mp_affinity(vms, n);
+
+ assert(!mc->smp_props.dies_supported);
+ ms->possible_cpus->cpus[n].props.has_socket_id = true;
+ ms->possible_cpus->cpus[n].props.socket_id =
+ n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
+ ms->possible_cpus->cpus[n].props.has_cluster_id = true;
+ ms->possible_cpus->cpus[n].props.cluster_id =
+ n / (ms->smp.cores * ms->smp.threads);
+ ms->possible_cpus->cpus[n].props.has_core_id = true;
+ ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
ms->possible_cpus->cpus[n].props.has_thread_id = true;
ms->possible_cpus->cpus[n].props.thread_id = n;
}
diff --git a/qapi/machine.json b/qapi/machine.json
index 42fc68403d..99c945f258 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 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology
2022-03-23 7:24 ` [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology Gavin Shan
@ 2022-03-25 13:19 ` Igor Mammedov
2022-03-25 18:49 ` Gavin Shan
2022-03-30 13:18 ` Igor Mammedov
2022-04-02 2:17 ` wangyanan (Y) via
2 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2022-03-25 13:19 UTC (permalink / raw)
To: Gavin Shan
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
On Wed, 23 Mar 2022 15:24:35 +0800
Gavin Shan <gshan@redhat.com> wrote:
> Currently, the SMP configuration isn't considered when the CPU
> topology is populated. In this case, it's impossible to provide
> the default CPU-to-NUMA mapping or association based on the socket
> ID of the given CPU.
>
> This takes account of SMP configuration when the CPU topology
> is populated. The die ID for the given CPU isn't assigned since
> it's not supported on arm/virt machine yet. Besides, the cluster
> ID for the given CPU is assigned because it has been supported
> on arm/virt machine.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> hw/arm/virt.c | 11 +++++++++++
> qapi/machine.json | 6 ++++--
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d2e5ecd234..064eac42f7 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,6 +2519,16 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> ms->possible_cpus->cpus[n].type = ms->cpu_type;
> ms->possible_cpus->cpus[n].arch_id =
> virt_cpu_mp_affinity(vms, n);
> +
> + assert(!mc->smp_props.dies_supported);
> + ms->possible_cpus->cpus[n].props.has_socket_id = true;
> + ms->possible_cpus->cpus[n].props.socket_id =
> + n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
> + ms->possible_cpus->cpus[n].props.has_cluster_id = true;
> + ms->possible_cpus->cpus[n].props.cluster_id =
> + n / (ms->smp.cores * ms->smp.threads);
are there any relation cluster values here and number of clusters with
what virt_cpu_mp_affinity() calculates?
> + ms->possible_cpus->cpus[n].props.has_core_id = true;
> + ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
> ms->possible_cpus->cpus[n].props.has_thread_id = true;
> ms->possible_cpus->cpus[n].props.thread_id = n;
of cause target has the right to decide how to allocate IDs, and mgmt
is supposed to query these IDs before using them.
But:
* IDs within 'props' are supposed to be arch defined.
(on x86 IDs in range [0-smp.foo_id), on ppc it something different)
Question is what real hardware does here in ARM case (i.e.
how .../cores/threads are described on bare-metal)?
* maybe related: looks like build_pptt() and build_madt() diverge on
the meaning of 'ACPI Processor ID' and how it's generated.
My understanding of 'ACPI Processor ID' is that it should match
across all tables. So UIDs generated in build_pptt() look wrong to me.
* maybe related: build_pptt() looks broken wrt core/thread where it
may create at the same time a leaf core with a leaf thread underneath it,
is such description actually valid?
> }
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 42fc68403d..99c945f258 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 the CPU belongs to
s:cluster:cluster/die:
> # @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'
> }
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology
2022-03-25 13:19 ` Igor Mammedov
@ 2022-03-25 18:49 ` Gavin Shan
2022-03-30 12:50 ` Igor Mammedov
0 siblings, 1 reply; 23+ messages in thread
From: Gavin Shan @ 2022-03-25 18:49 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
Hi Igor,
On 3/25/22 9:19 PM, Igor Mammedov wrote:
> On Wed, 23 Mar 2022 15:24:35 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> Currently, the SMP configuration isn't considered when the CPU
>> topology is populated. In this case, it's impossible to provide
>> the default CPU-to-NUMA mapping or association based on the socket
>> ID of the given CPU.
>>
>> This takes account of SMP configuration when the CPU topology
>> is populated. The die ID for the given CPU isn't assigned since
>> it's not supported on arm/virt machine yet. Besides, the cluster
>> ID for the given CPU is assigned because it has been supported
>> on arm/virt machine.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> hw/arm/virt.c | 11 +++++++++++
>> qapi/machine.json | 6 ++++--
>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index d2e5ecd234..064eac42f7 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,6 +2519,16 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>> ms->possible_cpus->cpus[n].type = ms->cpu_type;
>> ms->possible_cpus->cpus[n].arch_id =
>> virt_cpu_mp_affinity(vms, n);
>> +
>> + assert(!mc->smp_props.dies_supported);
>> + ms->possible_cpus->cpus[n].props.has_socket_id = true;
>> + ms->possible_cpus->cpus[n].props.socket_id =
>> + n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
>> + ms->possible_cpus->cpus[n].props.has_cluster_id = true;
>> + ms->possible_cpus->cpus[n].props.cluster_id =
>> + n / (ms->smp.cores * ms->smp.threads);
>
> are there any relation cluster values here and number of clusters with
> what virt_cpu_mp_affinity() calculates?
>
They're different clusters. The cluster returned by virt_cpu_mp_affinity()
is reflected to MPIDR_EL1 system register, which is mainly used by VGIC2/3
interrupt controller to send send group interrupts to the CPU cluster. It's
notable that the value returned from virt_cpu_mp_affinity() is always
overrided by KVM. It means this value is only used by TCG for the emulated
GIC2/GIC3.
The cluster in 'ms->possible_cpus' is passed to ACPI PPTT table to populate
the CPU topology.
>> + ms->possible_cpus->cpus[n].props.has_core_id = true;
>> + ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
>
>> ms->possible_cpus->cpus[n].props.has_thread_id = true;
>> ms->possible_cpus->cpus[n].props.thread_id = n;
> of cause target has the right to decide how to allocate IDs, and mgmt
> is supposed to query these IDs before using them.
> But:
> * IDs within 'props' are supposed to be arch defined.
> (on x86 IDs in range [0-smp.foo_id), on ppc it something different)
> Question is what real hardware does here in ARM case (i.e.
> how .../cores/threads are described on bare-metal)?
>
On ARM64 bare-metal machine, the core/cluster ID assignment is pretty arbitrary.
I checked the CPU topology on my bare-metal machine, which has following SMP
configurations.
# lscpu
:
Thread(s) per core: 4
Core(s) per socket: 28
Socket(s): 2
smp.sockets = 2
smp.clusters = 1
smp.cores = 56 (28 per socket)
smp.threads = 4
// CPU0-111 belongs to socket0 or package0
// CPU112-223 belongs to socket1 or package1
# cat /sys/devices/system/cpu/cpu0/topology/package_cpus
00000000,00000000,00000000,0000ffff,ffffffff,ffffffff,ffffffff
# cat /sys/devices/system/cpu/cpu111/topology/package_cpus
00000000,00000000,00000000,0000ffff,ffffffff,ffffffff,ffffffff
# cat /sys/devices/system/cpu/cpu112/topology/package_cpus
ffffffff,ffffffff,ffffffff,ffff0000,00000000,00000000,00000000
# cat /sys/devices/system/cpu/cpu223/topology/package_cpus
ffffffff,ffffffff,ffffffff,ffff0000,00000000,00000000,00000000
// core/cluster ID spans from 0 to 27 on socket0
# for i in `seq 0 27`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
# for i in `seq 28 55`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
# for i in `seq 0 27`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
# for i in `seq 28 55`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
// However, core/cluster ID starts from 256 on socket1
# for i in `seq 112 139`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
256 257 258 259 260 261 262 263 264 265 266 267 268 269
270 271 272 273 274 275 276 277 278 279 280 281 282 283
# for i in `seq 140 167`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
256 257 258 259 260 261 262 263 264 265 266 267 268 269
270 271 272 273 274 275 276 277 278 279 280 281 282 283
# for i in `seq 112 139`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
256 257 258 259 260 261 262 263 264 265 266 267 268 269
270 271 272 273 274 275 276 277 278 279 280 281 282 283
# for i in `seq 140 167`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
256 257 258 259 260 261 262 263 264 265 266 267 268 269
270 271 272 273 274 275 276 277 278 279 280 281 282 283
> * maybe related: looks like build_pptt() and build_madt() diverge on
> the meaning of 'ACPI Processor ID' and how it's generated.
> My understanding of 'ACPI Processor ID' is that it should match
> across all tables. So UIDs generated in build_pptt() look wrong to me.
>
> * maybe related: build_pptt() looks broken wrt core/thread where it
> may create at the same time a leaf core with a leaf thread underneath it,
> is such description actually valid?
>
Yes, the UIDs in MADT/PPTT should match. I'm not sure if I missed anything here.
I don't see how the UID in MADT and PPTT table are diverged. In both functions,
'thread_id' is taken as UID.
In build_pptt(), when the entries for the cores becomes leaf, nothing will be
pushed into @list, @length becomes zero for the loop to create entries for
the threads. In this case, we won't have any entries created for threads.
>
>> }
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index 42fc68403d..99c945f258 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 the CPU belongs to
>
> s:cluster:cluster/die:
>
Ok. I will amend it like below in next respin:
# @core-id: core number within cluster/die the CPU belongs to
I'm not sure if we need make similar changes for 'cluster_id' like below?
# @cluster-id: cluster number within die/socket 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'
>> }
Thanks,
Gavin
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology
2022-03-25 18:49 ` Gavin Shan
@ 2022-03-30 12:50 ` Igor Mammedov
2022-04-02 2:27 ` wangyanan (Y) via
2022-04-03 10:46 ` Gavin Shan
0 siblings, 2 replies; 23+ messages in thread
From: Igor Mammedov @ 2022-03-30 12:50 UTC (permalink / raw)
To: Gavin Shan
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
On Sat, 26 Mar 2022 02:49:59 +0800
Gavin Shan <gshan@redhat.com> wrote:
> Hi Igor,
>
> On 3/25/22 9:19 PM, Igor Mammedov wrote:
> > On Wed, 23 Mar 2022 15:24:35 +0800
> > Gavin Shan <gshan@redhat.com> wrote:
> >> Currently, the SMP configuration isn't considered when the CPU
> >> topology is populated. In this case, it's impossible to provide
> >> the default CPU-to-NUMA mapping or association based on the socket
> >> ID of the given CPU.
> >>
> >> This takes account of SMP configuration when the CPU topology
> >> is populated. The die ID for the given CPU isn't assigned since
> >> it's not supported on arm/virt machine yet. Besides, the cluster
> >> ID for the given CPU is assigned because it has been supported
> >> on arm/virt machine.
> >>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> ---
> >> hw/arm/virt.c | 11 +++++++++++
> >> qapi/machine.json | 6 ++++--
> >> 2 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index d2e5ecd234..064eac42f7 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,6 +2519,16 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >> ms->possible_cpus->cpus[n].type = ms->cpu_type;
> >> ms->possible_cpus->cpus[n].arch_id =
> >> virt_cpu_mp_affinity(vms, n);
> >> +
> >> + assert(!mc->smp_props.dies_supported);
> >> + ms->possible_cpus->cpus[n].props.has_socket_id = true;
> >> + ms->possible_cpus->cpus[n].props.socket_id =
> >> + n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
> >> + ms->possible_cpus->cpus[n].props.has_cluster_id = true;
> >> + ms->possible_cpus->cpus[n].props.cluster_id =
> >> + n / (ms->smp.cores * ms->smp.threads);
> >
> > are there any relation cluster values here and number of clusters with
> > what virt_cpu_mp_affinity() calculates?
> >
>
> They're different clusters. The cluster returned by virt_cpu_mp_affinity()
> is reflected to MPIDR_EL1 system register, which is mainly used by VGIC2/3
> interrupt controller to send send group interrupts to the CPU cluster. It's
> notable that the value returned from virt_cpu_mp_affinity() is always
> overrided by KVM. It means this value is only used by TCG for the emulated
> GIC2/GIC3.
>
> The cluster in 'ms->possible_cpus' is passed to ACPI PPTT table to populate
> the CPU topology.
>
>
> >> + ms->possible_cpus->cpus[n].props.has_core_id = true;
> >> + ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
> >
> >> ms->possible_cpus->cpus[n].props.has_thread_id = true;
> >> ms->possible_cpus->cpus[n].props.thread_id = n;
> > of cause target has the right to decide how to allocate IDs, and mgmt
> > is supposed to query these IDs before using them.
> > But:
> > * IDs within 'props' are supposed to be arch defined.
> > (on x86 IDs in range [0-smp.foo_id), on ppc it something different)
> > Question is what real hardware does here in ARM case (i.e.
> > how .../cores/threads are described on bare-metal)?
> >
>
> On ARM64 bare-metal machine, the core/cluster ID assignment is pretty arbitrary.
> I checked the CPU topology on my bare-metal machine, which has following SMP
> configurations.
>
> # lscpu
> :
> Thread(s) per core: 4
> Core(s) per socket: 28
> Socket(s): 2
>
> smp.sockets = 2
> smp.clusters = 1
> smp.cores = 56 (28 per socket)
> smp.threads = 4
>
> // CPU0-111 belongs to socket0 or package0
> // CPU112-223 belongs to socket1 or package1
> # cat /sys/devices/system/cpu/cpu0/topology/package_cpus
> 00000000,00000000,00000000,0000ffff,ffffffff,ffffffff,ffffffff
> # cat /sys/devices/system/cpu/cpu111/topology/package_cpus
> 00000000,00000000,00000000,0000ffff,ffffffff,ffffffff,ffffffff
> # cat /sys/devices/system/cpu/cpu112/topology/package_cpus
> ffffffff,ffffffff,ffffffff,ffff0000,00000000,00000000,00000000
> # cat /sys/devices/system/cpu/cpu223/topology/package_cpus
> ffffffff,ffffffff,ffffffff,ffff0000,00000000,00000000,00000000
>
> // core/cluster ID spans from 0 to 27 on socket0
> # for i in `seq 0 27`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
> # for i in `seq 28 55`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
> # for i in `seq 0 27`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
> # for i in `seq 28 55`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
>
> // However, core/cluster ID starts from 256 on socket1
> # for i in `seq 112 139`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
> # for i in `seq 140 167`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
> # for i in `seq 112 139`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
> # for i in `seq 140 167`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
so it seems that IDs are repeatable within a socket.
If there no arch defined way or other objections it might be better
to stick to what x86 does for consistency reasons (i.e. socket/die/
cluster/core/thread are in range [0..x) including thread-id being
in range [0..threads) ) instead of inventing arm/virt specific scheme.
>
> > * maybe related: looks like build_pptt() and build_madt() diverge on
> > the meaning of 'ACPI Processor ID' and how it's generated.
> > My understanding of 'ACPI Processor ID' is that it should match
> > across all tables. So UIDs generated in build_pptt() look wrong to me.
> >
> > * maybe related: build_pptt() looks broken wrt core/thread where it
> > may create at the same time a leaf core with a leaf thread underneath it,
> > is such description actually valid?
> >
>
> Yes, the UIDs in MADT/PPTT should match. I'm not sure if I missed anything here.
> I don't see how the UID in MADT and PPTT table are diverged. In both functions,
> 'thread_id' is taken as UID.
>
> In build_pptt(), when the entries for the cores becomes leaf, nothing will be
> pushed into @list, @length becomes zero for the loop to create entries for
> the threads. In this case, we won't have any entries created for threads.
>
> >
> >> }
> >> diff --git a/qapi/machine.json b/qapi/machine.json
> >> index 42fc68403d..99c945f258 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 the CPU belongs to
> >
> > s:cluster:cluster/die:
> >
>
> Ok. I will amend it like below in next respin:
>
> # @core-id: core number within cluster/die the CPU belongs to
>
> I'm not sure if we need make similar changes for 'cluster_id' like below?
>
> # @cluster-id: cluster number within die/socket the CPU belongs to
> ^^^^^^^^^^
maybe postpone it till die is supported?
>
> >> # @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'
> >> }
>
> Thanks,
> Gavin
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology
2022-03-30 12:50 ` Igor Mammedov
@ 2022-04-02 2:27 ` wangyanan (Y) via
2022-04-03 10:46 ` Gavin Shan
1 sibling, 0 replies; 23+ messages in thread
From: wangyanan (Y) via @ 2022-04-02 2:27 UTC (permalink / raw)
To: Igor Mammedov, Gavin Shan
Cc: qemu-arm, qemu-devel, drjones, peter.maydell, richard.henderson,
zhenyzha, shan.gavin
On 2022/3/30 20:50, Igor Mammedov wrote:
> On Sat, 26 Mar 2022 02:49:59 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>
>> Hi Igor,
>>
>> On 3/25/22 9:19 PM, Igor Mammedov wrote:
>>> On Wed, 23 Mar 2022 15:24:35 +0800
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>> Currently, the SMP configuration isn't considered when the CPU
>>>> topology is populated. In this case, it's impossible to provide
>>>> the default CPU-to-NUMA mapping or association based on the socket
>>>> ID of the given CPU.
>>>>
>>>> This takes account of SMP configuration when the CPU topology
>>>> is populated. The die ID for the given CPU isn't assigned since
>>>> it's not supported on arm/virt machine yet. Besides, the cluster
>>>> ID for the given CPU is assigned because it has been supported
>>>> on arm/virt machine.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>> hw/arm/virt.c | 11 +++++++++++
>>>> qapi/machine.json | 6 ++++--
>>>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>> index d2e5ecd234..064eac42f7 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,6 +2519,16 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>> ms->possible_cpus->cpus[n].type = ms->cpu_type;
>>>> ms->possible_cpus->cpus[n].arch_id =
>>>> virt_cpu_mp_affinity(vms, n);
>>>> +
>>>> + assert(!mc->smp_props.dies_supported);
>>>> + ms->possible_cpus->cpus[n].props.has_socket_id = true;
>>>> + ms->possible_cpus->cpus[n].props.socket_id =
>>>> + n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
>>>> + ms->possible_cpus->cpus[n].props.has_cluster_id = true;
>>>> + ms->possible_cpus->cpus[n].props.cluster_id =
>>>> + n / (ms->smp.cores * ms->smp.threads);
>>> are there any relation cluster values here and number of clusters with
>>> what virt_cpu_mp_affinity() calculates?
>>>
>> They're different clusters. The cluster returned by virt_cpu_mp_affinity()
>> is reflected to MPIDR_EL1 system register, which is mainly used by VGIC2/3
>> interrupt controller to send send group interrupts to the CPU cluster. It's
>> notable that the value returned from virt_cpu_mp_affinity() is always
>> overrided by KVM. It means this value is only used by TCG for the emulated
>> GIC2/GIC3.
>>
>> The cluster in 'ms->possible_cpus' is passed to ACPI PPTT table to populate
>> the CPU topology.
>>
>>
>>>> + ms->possible_cpus->cpus[n].props.has_core_id = true;
>>>> + ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
>>>
>>>> ms->possible_cpus->cpus[n].props.has_thread_id = true;
>>>> ms->possible_cpus->cpus[n].props.thread_id = n;
>>> of cause target has the right to decide how to allocate IDs, and mgmt
>>> is supposed to query these IDs before using them.
>>> But:
>>> * IDs within 'props' are supposed to be arch defined.
>>> (on x86 IDs in range [0-smp.foo_id), on ppc it something different)
>>> Question is what real hardware does here in ARM case (i.e.
>>> how .../cores/threads are described on bare-metal)?
>>>
>> On ARM64 bare-metal machine, the core/cluster ID assignment is pretty arbitrary.
>> I checked the CPU topology on my bare-metal machine, which has following SMP
>> configurations.
>>
>> # lscpu
>> :
>> Thread(s) per core: 4
>> Core(s) per socket: 28
>> Socket(s): 2
>>
>> smp.sockets = 2
>> smp.clusters = 1
>> smp.cores = 56 (28 per socket)
>> smp.threads = 4
>>
>> // CPU0-111 belongs to socket0 or package0
>> // CPU112-223 belongs to socket1 or package1
>> # cat /sys/devices/system/cpu/cpu0/topology/package_cpus
>> 00000000,00000000,00000000,0000ffff,ffffffff,ffffffff,ffffffff
>> # cat /sys/devices/system/cpu/cpu111/topology/package_cpus
>> 00000000,00000000,00000000,0000ffff,ffffffff,ffffffff,ffffffff
>> # cat /sys/devices/system/cpu/cpu112/topology/package_cpus
>> ffffffff,ffffffff,ffffffff,ffff0000,00000000,00000000,00000000
>> # cat /sys/devices/system/cpu/cpu223/topology/package_cpus
>> ffffffff,ffffffff,ffffffff,ffff0000,00000000,00000000,00000000
>>
>> // core/cluster ID spans from 0 to 27 on socket0
>> # for i in `seq 0 27`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
>> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
>> # for i in `seq 28 55`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
>> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
>> # for i in `seq 0 27`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
>> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
>> # for i in `seq 28 55`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
>> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
>>
>> // However, core/cluster ID starts from 256 on socket1
>> # for i in `seq 112 139`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
>> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
>> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
>> # for i in `seq 140 167`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
>> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
>> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
>> # for i in `seq 112 139`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
>> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
>> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
>> # for i in `seq 140 167`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
>> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
>> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
> so it seems that IDs are repeatable within a socket.
> If there no arch defined way or other objections it might be better
> to stick to what x86 does for consistency reasons (i.e. socket/die/
> cluster/core/thread are in range [0..x) including thread-id being
> in range [0..threads) ) instead of inventing arm/virt specific scheme.
Agreed.
>>
>>> * maybe related: looks like build_pptt() and build_madt() diverge on
>>> the meaning of 'ACPI Processor ID' and how it's generated.
>>> My understanding of 'ACPI Processor ID' is that it should match
>>> across all tables. So UIDs generated in build_pptt() look wrong to me.
>>>
>>> * maybe related: build_pptt() looks broken wrt core/thread where it
>>> may create at the same time a leaf core with a leaf thread underneath it,
>>> is such description actually valid?
>>>
>> Yes, the UIDs in MADT/PPTT should match. I'm not sure if I missed anything here.
>> I don't see how the UID in MADT and PPTT table are diverged. In both functions,
>> 'thread_id' is taken as UID.
>>
>> In build_pptt(), when the entries for the cores becomes leaf, nothing will be
>> pushed into @list, @length becomes zero for the loop to create entries for
>> the threads. In this case, we won't have any entries created for threads.
>>
>>>
>>>> }
>>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>>> index 42fc68403d..99c945f258 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 the CPU belongs to
>>> s:cluster:cluster/die:
>>>
>> Ok. I will amend it like below in next respin:
>>
>> # @core-id: core number within cluster/die the CPU belongs to
>>
>> I'm not sure if we need make similar changes for 'cluster_id' like below?
>>
>> # @cluster-id: cluster number within die/socket the CPU belongs to
>> ^^^^^^^^^^
> maybe postpone it till die is supported?
>
>>>> # @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'
>>>> }
>> Thanks,
>> Gavin
>>
> .
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology
2022-03-30 12:50 ` Igor Mammedov
2022-04-02 2:27 ` wangyanan (Y) via
@ 2022-04-03 10:46 ` Gavin Shan
1 sibling, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2022-04-03 10:46 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
Hi Igor,
On 3/30/22 8:50 PM, Igor Mammedov wrote:
> On Sat, 26 Mar 2022 02:49:59 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> On 3/25/22 9:19 PM, Igor Mammedov wrote:
>>> On Wed, 23 Mar 2022 15:24:35 +0800
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>> Currently, the SMP configuration isn't considered when the CPU
>>>> topology is populated. In this case, it's impossible to provide
>>>> the default CPU-to-NUMA mapping or association based on the socket
>>>> ID of the given CPU.
>>>>
>>>> This takes account of SMP configuration when the CPU topology
>>>> is populated. The die ID for the given CPU isn't assigned since
>>>> it's not supported on arm/virt machine yet. Besides, the cluster
>>>> ID for the given CPU is assigned because it has been supported
>>>> on arm/virt machine.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>> hw/arm/virt.c | 11 +++++++++++
>>>> qapi/machine.json | 6 ++++--
>>>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>> index d2e5ecd234..064eac42f7 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,6 +2519,16 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>> ms->possible_cpus->cpus[n].type = ms->cpu_type;
>>>> ms->possible_cpus->cpus[n].arch_id =
>>>> virt_cpu_mp_affinity(vms, n);
>>>> +
>>>> + assert(!mc->smp_props.dies_supported);
>>>> + ms->possible_cpus->cpus[n].props.has_socket_id = true;
>>>> + ms->possible_cpus->cpus[n].props.socket_id =
>>>> + n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
>>>> + ms->possible_cpus->cpus[n].props.has_cluster_id = true;
>>>> + ms->possible_cpus->cpus[n].props.cluster_id =
>>>> + n / (ms->smp.cores * ms->smp.threads);
>>>
>>> are there any relation cluster values here and number of clusters with
>>> what virt_cpu_mp_affinity() calculates?
>>>
>>
>> They're different clusters. The cluster returned by virt_cpu_mp_affinity()
>> is reflected to MPIDR_EL1 system register, which is mainly used by VGIC2/3
>> interrupt controller to send send group interrupts to the CPU cluster. It's
>> notable that the value returned from virt_cpu_mp_affinity() is always
>> overrided by KVM. It means this value is only used by TCG for the emulated
>> GIC2/GIC3.
>>
>> The cluster in 'ms->possible_cpus' is passed to ACPI PPTT table to populate
>> the CPU topology.
>>
>>
>>>> + ms->possible_cpus->cpus[n].props.has_core_id = true;
>>>> + ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
>>>
>>>> ms->possible_cpus->cpus[n].props.has_thread_id = true;
>>>> ms->possible_cpus->cpus[n].props.thread_id = n;
>>> of cause target has the right to decide how to allocate IDs, and mgmt
>>> is supposed to query these IDs before using them.
>>> But:
>>> * IDs within 'props' are supposed to be arch defined.
>>> (on x86 IDs in range [0-smp.foo_id), on ppc it something different)
>>> Question is what real hardware does here in ARM case (i.e.
>>> how .../cores/threads are described on bare-metal)?
>>>
>>
>> On ARM64 bare-metal machine, the core/cluster ID assignment is pretty arbitrary.
>> I checked the CPU topology on my bare-metal machine, which has following SMP
>> configurations.
>>
>> # lscpu
>> :
>> Thread(s) per core: 4
>> Core(s) per socket: 28
>> Socket(s): 2
>>
>> smp.sockets = 2
>> smp.clusters = 1
>> smp.cores = 56 (28 per socket)
>> smp.threads = 4
>>
>> // CPU0-111 belongs to socket0 or package0
>> // CPU112-223 belongs to socket1 or package1
>> # cat /sys/devices/system/cpu/cpu0/topology/package_cpus
>> 00000000,00000000,00000000,0000ffff,ffffffff,ffffffff,ffffffff
>> # cat /sys/devices/system/cpu/cpu111/topology/package_cpus
>> 00000000,00000000,00000000,0000ffff,ffffffff,ffffffff,ffffffff
>> # cat /sys/devices/system/cpu/cpu112/topology/package_cpus
>> ffffffff,ffffffff,ffffffff,ffff0000,00000000,00000000,00000000
>> # cat /sys/devices/system/cpu/cpu223/topology/package_cpus
>> ffffffff,ffffffff,ffffffff,ffff0000,00000000,00000000,00000000
>>
>> // core/cluster ID spans from 0 to 27 on socket0
>> # for i in `seq 0 27`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
>> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
>> # for i in `seq 28 55`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
>> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
>> # for i in `seq 0 27`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
>> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
>> # for i in `seq 28 55`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
>> 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
>>
>> // However, core/cluster ID starts from 256 on socket1
>> # for i in `seq 112 139`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
>> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
>> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
>> # for i in `seq 140 167`; do cat /sys/devices/system/cpu/cpu$i/topology/core_id; done
>> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
>> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
>> # for i in `seq 112 139`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
>> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
>> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
>> # for i in `seq 140 167`; do cat /sys/devices/system/cpu/cpu$i/topology/cluster_id; done
>> 256 257 258 259 260 261 262 263 264 265 266 267 268 269
>> 270 271 272 273 274 275 276 277 278 279 280 281 282 283
>
> so it seems that IDs are repeatable within a socket.
> If there no arch defined way or other objections it might be better
> to stick to what x86 does for consistency reasons (i.e. socket/die/
> cluster/core/thread are in range [0..x) including thread-id being
> in range [0..threads) ) instead of inventing arm/virt specific scheme.
>
Agreed.
>>
>>> * maybe related: looks like build_pptt() and build_madt() diverge on
>>> the meaning of 'ACPI Processor ID' and how it's generated.
>>> My understanding of 'ACPI Processor ID' is that it should match
>>> across all tables. So UIDs generated in build_pptt() look wrong to me.
>>>
>>> * maybe related: build_pptt() looks broken wrt core/thread where it
>>> may create at the same time a leaf core with a leaf thread underneath it,
>>> is such description actually valid?
>>>
>>
>> Yes, the UIDs in MADT/PPTT should match. I'm not sure if I missed anything here.
>> I don't see how the UID in MADT and PPTT table are diverged. In both functions,
>> 'thread_id' is taken as UID.
>>
>> In build_pptt(), when the entries for the cores becomes leaf, nothing will be
>> pushed into @list, @length becomes zero for the loop to create entries for
>> the threads. In this case, we won't have any entries created for threads.
>>
>>>
>>>> }
>>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>>> index 42fc68403d..99c945f258 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 the CPU belongs to
>>>
>>> s:cluster:cluster/die:
>>>
>>
>> Ok. I will amend it like below in next respin:
>>
>> # @core-id: core number within cluster/die the CPU belongs to
>>
>> I'm not sure if we need make similar changes for 'cluster_id' like below?
>>
>> # @cluster-id: cluster number within die/socket the CPU belongs to
>> ^^^^^^^^^^
>
> maybe postpone it till die is supported?
>
Ok. Lets postpone to change description about 'cluster-id' until
die is supported. So only the description about 'core-id' will
be amended as you suggested in v4.
>>
>>>> # @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'
>>>> }
Thanks,
Gavin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology
2022-03-23 7:24 ` [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology Gavin Shan
2022-03-25 13:19 ` Igor Mammedov
@ 2022-03-30 13:18 ` Igor Mammedov
2022-04-03 10:48 ` Gavin Shan
2022-04-02 2:17 ` wangyanan (Y) via
2 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2022-03-30 13:18 UTC (permalink / raw)
To: Gavin Shan
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
On Wed, 23 Mar 2022 15:24:35 +0800
Gavin Shan <gshan@redhat.com> wrote:
> Currently, the SMP configuration isn't considered when the CPU
> topology is populated. In this case, it's impossible to provide
> the default CPU-to-NUMA mapping or association based on the socket
> ID of the given CPU.
>
> This takes account of SMP configuration when the CPU topology
> is populated. The die ID for the given CPU isn't assigned since
> it's not supported on arm/virt machine yet. Besides, the cluster
> ID for the given CPU is assigned because it has been supported
> on arm/virt machine.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> hw/arm/virt.c | 11 +++++++++++
> qapi/machine.json | 6 ++++--
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d2e5ecd234..064eac42f7 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,6 +2519,16 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> ms->possible_cpus->cpus[n].type = ms->cpu_type;
> ms->possible_cpus->cpus[n].arch_id =
> virt_cpu_mp_affinity(vms, n);
> +
> + assert(!mc->smp_props.dies_supported);
> + ms->possible_cpus->cpus[n].props.has_socket_id = true;
> + ms->possible_cpus->cpus[n].props.socket_id =
> + n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
> + ms->possible_cpus->cpus[n].props.has_cluster_id = true;
> + ms->possible_cpus->cpus[n].props.cluster_id =
> + n / (ms->smp.cores * ms->smp.threads);
> + ms->possible_cpus->cpus[n].props.has_core_id = true;
> + ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
> ms->possible_cpus->cpus[n].props.has_thread_id = true;
> ms->possible_cpus->cpus[n].props.thread_id = n;
shouldn't be above values calculated similar to the way they are
calculated in x86_topo_ids_from_idx()? /note '% foo' part/
> }
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 42fc68403d..99c945f258 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 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'
> }
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology
2022-03-30 13:18 ` Igor Mammedov
@ 2022-04-03 10:48 ` Gavin Shan
0 siblings, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2022-04-03 10:48 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
Hi Igor,
On 3/30/22 9:18 PM, Igor Mammedov wrote:
> On Wed, 23 Mar 2022 15:24:35 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>
>> Currently, the SMP configuration isn't considered when the CPU
>> topology is populated. In this case, it's impossible to provide
>> the default CPU-to-NUMA mapping or association based on the socket
>> ID of the given CPU.
>>
>> This takes account of SMP configuration when the CPU topology
>> is populated. The die ID for the given CPU isn't assigned since
>> it's not supported on arm/virt machine yet. Besides, the cluster
>> ID for the given CPU is assigned because it has been supported
>> on arm/virt machine.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> hw/arm/virt.c | 11 +++++++++++
>> qapi/machine.json | 6 ++++--
>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index d2e5ecd234..064eac42f7 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,6 +2519,16 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>> ms->possible_cpus->cpus[n].type = ms->cpu_type;
>> ms->possible_cpus->cpus[n].arch_id =
>> virt_cpu_mp_affinity(vms, n);
>> +
>> + assert(!mc->smp_props.dies_supported);
>> + ms->possible_cpus->cpus[n].props.has_socket_id = true;
>> + ms->possible_cpus->cpus[n].props.socket_id =
>> + n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
>> + ms->possible_cpus->cpus[n].props.has_cluster_id = true;
>> + ms->possible_cpus->cpus[n].props.cluster_id =
>> + n / (ms->smp.cores * ms->smp.threads);
>> + ms->possible_cpus->cpus[n].props.has_core_id = true;
>> + ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
>> ms->possible_cpus->cpus[n].props.has_thread_id = true;
>> ms->possible_cpus->cpus[n].props.thread_id = n;
>
> shouldn't be above values calculated similar to the way they are
> calculated in x86_topo_ids_from_idx()? /note '% foo' part/
>
I think it's fine not to have '% foo' here. However, I think it'd
better to have the similar '% foo' as x86 does. I will add this part
in v4.
>> }
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index 42fc68403d..99c945f258 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 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'
>> }
Thanks,
Gavin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology
2022-03-23 7:24 ` [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology Gavin Shan
2022-03-25 13:19 ` Igor Mammedov
2022-03-30 13:18 ` Igor Mammedov
@ 2022-04-02 2:17 ` wangyanan (Y) via
2022-04-03 11:55 ` Gavin Shan
2 siblings, 1 reply; 23+ messages in thread
From: wangyanan (Y) via @ 2022-04-02 2:17 UTC (permalink / raw)
To: Gavin Shan, qemu-arm
Cc: qemu-devel, imammedo, drjones, peter.maydell, richard.henderson,
zhenyzha, shan.gavin
Hi Gavin,
On 2022/3/23 15:24, 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. Besides, the cluster
> ID for the given CPU is assigned because it has been supported
> on arm/virt machine.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> hw/arm/virt.c | 11 +++++++++++
> qapi/machine.json | 6 ++++--
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d2e5ecd234..064eac42f7 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,6 +2519,16 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> ms->possible_cpus->cpus[n].type = ms->cpu_type;
> ms->possible_cpus->cpus[n].arch_id =
> virt_cpu_mp_affinity(vms, n);
> +
> + assert(!mc->smp_props.dies_supported);
> + ms->possible_cpus->cpus[n].props.has_socket_id = true;
> + ms->possible_cpus->cpus[n].props.socket_id =
> + n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
> + ms->possible_cpus->cpus[n].props.has_cluster_id = true;
> + ms->possible_cpus->cpus[n].props.cluster_id =
> + n / (ms->smp.cores * ms->smp.threads);
> + ms->possible_cpus->cpus[n].props.has_core_id = true;
> + ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
> ms->possible_cpus->cpus[n].props.has_thread_id = true;
> ms->possible_cpus->cpus[n].props.thread_id = n;
> }
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 42fc68403d..99c945f258 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 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'
> }
Since new cluster-id is introduced, you may want to check whether to
update machine_set_cpu_numa_node() and hmp_hotpluggable_cpus(),
accordingly, which both deal with topo-ids. If we need to update them,
it's easier to review to make the whole cluster-id introduction part
a separate patch.
Thanks,
Yanan
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology
2022-04-02 2:17 ` wangyanan (Y) via
@ 2022-04-03 11:55 ` Gavin Shan
0 siblings, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2022-04-03 11:55 UTC (permalink / raw)
To: wangyanan (Y), qemu-arm
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
shan.gavin, imammedo
Hi Yanan,
On 4/2/22 10:17 AM, wangyanan (Y) wrote:
> On 2022/3/23 15:24, 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. Besides, the cluster
>> ID for the given CPU is assigned because it has been supported
>> on arm/virt machine.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> hw/arm/virt.c | 11 +++++++++++
>> qapi/machine.json | 6 ++++--
>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index d2e5ecd234..064eac42f7 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,6 +2519,16 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>> ms->possible_cpus->cpus[n].type = ms->cpu_type;
>> ms->possible_cpus->cpus[n].arch_id =
>> virt_cpu_mp_affinity(vms, n);
>> +
>> + assert(!mc->smp_props.dies_supported);
>> + ms->possible_cpus->cpus[n].props.has_socket_id = true;
>> + ms->possible_cpus->cpus[n].props.socket_id =
>> + n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
>> + ms->possible_cpus->cpus[n].props.has_cluster_id = true;
>> + ms->possible_cpus->cpus[n].props.cluster_id =
>> + n / (ms->smp.cores * ms->smp.threads);
>> + ms->possible_cpus->cpus[n].props.has_core_id = true;
>> + ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
>> ms->possible_cpus->cpus[n].props.has_thread_id = true;
>> ms->possible_cpus->cpus[n].props.thread_id = n;
>> }
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index 42fc68403d..99c945f258 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 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'
>> }
> Since new cluster-id is introduced, you may want to check whether to
> update machine_set_cpu_numa_node() and hmp_hotpluggable_cpus(),
> accordingly, which both deal with topo-ids. If we need to update them,
> it's easier to review to make the whole cluster-id introduction part
> a separate patch.
>
Yes, I agree. hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() also
needs 'cluster-id' either. I will have the changes in v5 because I didn't
catch your comments before posting v4. Please go ahead to review v5
directly.
Thanks,
Gavin
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 2/4] hw/arm/virt: Fix CPU's default NUMA node ID
2022-03-23 7:24 [PATCH v3 0/4] hw/arm/virt: Fix CPU's default NUMA node ID Gavin Shan
2022-03-23 7:24 ` [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology Gavin Shan
@ 2022-03-23 7:24 ` Gavin Shan
2022-03-25 13:25 ` Igor Mammedov
2022-04-02 2:02 ` wangyanan (Y) via
2022-03-23 7:24 ` [PATCH v3 3/4] hw/acpi/aml-build: Use existing CPU topology to build PPTT table Gavin Shan
2022-03-23 7:24 ` [PATCH v3 4/4] hw/arm/virt: Unify ACPI processor ID in MADT and SRAT table Gavin Shan
3 siblings, 2 replies; 23+ messages in thread
From: Gavin Shan @ 2022-03-23 7:24 UTC (permalink / raw)
To: qemu-arm
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, shan.gavin, imammedo
When CPU-to-NUMA association isn't explicitly provided by users,
the default on 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>
---
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 064eac42f7..3286915229 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 2/4] hw/arm/virt: Fix CPU's default NUMA node ID
2022-03-23 7:24 ` [PATCH v3 2/4] hw/arm/virt: Fix CPU's default NUMA node ID Gavin Shan
@ 2022-03-25 13:25 ` Igor Mammedov
2022-04-02 2:02 ` wangyanan (Y) via
1 sibling, 0 replies; 23+ messages in thread
From: Igor Mammedov @ 2022-03-25 13:25 UTC (permalink / raw)
To: Gavin Shan
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
On Wed, 23 Mar 2022 15:24:36 +0800
Gavin Shan <gshan@redhat.com> wrote:
> When CPU-to-NUMA association isn't explicitly provided by users,
> the default on 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>
> ---
> 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 064eac42f7..3286915229 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)
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 2/4] hw/arm/virt: Fix CPU's default NUMA node ID
2022-03-23 7:24 ` [PATCH v3 2/4] hw/arm/virt: Fix CPU's default NUMA node ID Gavin Shan
2022-03-25 13:25 ` Igor Mammedov
@ 2022-04-02 2:02 ` wangyanan (Y) via
2022-04-03 11:57 ` Gavin Shan
1 sibling, 1 reply; 23+ messages in thread
From: wangyanan (Y) via @ 2022-04-02 2:02 UTC (permalink / raw)
To: Gavin Shan, qemu-arm
Cc: qemu-devel, imammedo, drjones, peter.maydell, richard.henderson,
zhenyzha, shan.gavin
On 2022/3/23 15:24, Gavin Shan wrote:
> When CPU-to-NUMA association isn't explicitly provided by users,
> the default on is given by mc->get_default_cpu_node_id(). However,
s/on/one
> 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>
> ---
> hw/arm/virt.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Thanks,
Yanan
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 064eac42f7..3286915229 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)
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 2/4] hw/arm/virt: Fix CPU's default NUMA node ID
2022-04-02 2:02 ` wangyanan (Y) via
@ 2022-04-03 11:57 ` Gavin Shan
0 siblings, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2022-04-03 11:57 UTC (permalink / raw)
To: wangyanan (Y), qemu-arm
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
shan.gavin, imammedo
Hi Yanan,
On 4/2/22 10:02 AM, wangyanan (Y) wrote:
> On 2022/3/23 15:24, Gavin Shan wrote:
>> When CPU-to-NUMA association isn't explicitly provided by users,
>> the default on is given by mc->get_default_cpu_node_id(). However,
> s/on/one
Will be corrected in v5.
>> 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>
>> ---
>> hw/arm/virt.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>
Thanks, will be included into v5 as I missed your comments in v4.
Please go ahead to review v5 directly.
Thanks,
Gavin
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 064eac42f7..3286915229 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)
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 3/4] hw/acpi/aml-build: Use existing CPU topology to build PPTT table
2022-03-23 7:24 [PATCH v3 0/4] hw/arm/virt: Fix CPU's default NUMA node ID Gavin Shan
2022-03-23 7:24 ` [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology Gavin Shan
2022-03-23 7:24 ` [PATCH v3 2/4] hw/arm/virt: Fix CPU's default NUMA node ID Gavin Shan
@ 2022-03-23 7:24 ` Gavin Shan
2022-03-30 14:10 ` Igor Mammedov
2022-03-23 7:24 ` [PATCH v3 4/4] hw/arm/virt: Unify ACPI processor ID in MADT and SRAT table Gavin Shan
3 siblings, 1 reply; 23+ messages in thread
From: Gavin Shan @ 2022-03-23 7:24 UTC (permalink / raw)
To: qemu-arm
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, shan.gavin, imammedo
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 avoids to re-calculate the CPU topology 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 | 96 +++++++++++++++++++++++++++++++++------------
1 file changed, 72 insertions(+), 24 deletions(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 4086879ebf..10a2d63b96 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2002,18 +2002,27 @@ 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);
+ CPUArchIdList *cpus = ms->possible_cpus;
+ GQueue *socket_list = g_queue_new();
+ GQueue *cluster_list = g_queue_new();
+ GQueue *core_list = g_queue_new();
GQueue *list = g_queue_new();
guint pptt_start = table_data->len;
guint parent_offset;
guint length, i;
- int uid = 0;
- int socket;
+ int n, socket_id, cluster_id, core_id, thread_id;
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++) {
+ for (n = 0; n < cpus->len; n++) {
+ socket_id = cpus->cpus[n].props.socket_id;
+ if (g_queue_find(socket_list, GUINT_TO_POINTER(socket_id))) {
+ continue;
+ }
+
+ g_queue_push_tail(socket_list, GUINT_TO_POINTER(socket_id));
g_queue_push_tail(list,
GUINT_TO_POINTER(table_data->len - pptt_start));
build_processor_hierarchy_node(
@@ -2023,65 +2032,104 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
* of a physical package
*/
(1 << 0),
- 0, socket, NULL, 0);
+ 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++) {
+ socket_id = GPOINTER_TO_UINT(g_queue_pop_head(socket_list));
+
+ for (n = 0; n < cpus->len; n++) {
+ if (cpus->cpus[n].props.socket_id != socket_id) {
+ continue;
+ }
+
+ cluster_id = cpus->cpus[n].props.cluster_id;
+ if (g_queue_find(cluster_list, GUINT_TO_POINTER(cluster_id))) {
+ continue;
+ }
+
+ g_queue_push_tail(cluster_list, GUINT_TO_POINTER(cluster_id));
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);
+ parent_offset, cluster_id, NULL, 0);
}
}
}
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,
- (0 << 0), /* not a physical package */
- parent_offset, core, NULL, 0);
- } else {
+ if (!mc->smp_props.clusters_supported) {
+ socket_id = GPOINTER_TO_UINT(g_queue_pop_head(socket_list));
+ } else {
+ cluster_id = GPOINTER_TO_UINT(g_queue_pop_head(cluster_list));
+ }
+
+ for (n = 0; n < cpus->len; n++) {
+ if (!mc->smp_props.clusters_supported &&
+ cpus->cpus[n].props.socket_id != socket_id) {
+ continue;
+ }
+
+ if (mc->smp_props.clusters_supported &&
+ cpus->cpus[n].props.cluster_id != cluster_id) {
+ continue;
+ }
+
+ core_id = cpus->cpus[n].props.core_id;
+ if (ms->smp.threads <= 1) {
build_processor_hierarchy_node(
table_data,
(1 << 1) | /* ACPI Processor ID valid */
(1 << 3), /* Node is a Leaf */
- parent_offset, uid++, NULL, 0);
+ parent_offset, core_id, NULL, 0);
+ continue;
+ }
+
+ if (g_queue_find(core_list, GUINT_TO_POINTER(core_id))) {
+ continue;
}
+
+ g_queue_push_tail(core_list, GUINT_TO_POINTER(core_id));
+ 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, 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++) {
+ core_id = GPOINTER_TO_UINT(g_queue_pop_head(core_list));
+
+ for (n = 0; n < cpus->len; n++) {
+ if (cpus->cpus[n].props.core_id != core_id) {
+ continue;
+ }
+
+ thread_id = cpus->cpus[n].props.thread_id;
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);
+ parent_offset, thread_id, NULL, 0);
}
}
g_queue_free(list);
+ g_queue_free(core_list);
+ g_queue_free(cluster_list);
+ g_queue_free(socket_list);
acpi_table_end(linker, &table);
}
--
2.23.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 3/4] hw/acpi/aml-build: Use existing CPU topology to build PPTT table
2022-03-23 7:24 ` [PATCH v3 3/4] hw/acpi/aml-build: Use existing CPU topology to build PPTT table Gavin Shan
@ 2022-03-30 14:10 ` Igor Mammedov
2022-04-03 14:40 ` Gavin Shan
0 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2022-03-30 14:10 UTC (permalink / raw)
To: Gavin Shan
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
On Wed, 23 Mar 2022 15:24:37 +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 avoids to re-calculate the CPU topology 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 | 96 +++++++++++++++++++++++++++++++++------------
> 1 file changed, 72 insertions(+), 24 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 4086879ebf..10a2d63b96 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2002,18 +2002,27 @@ 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);
> + CPUArchIdList *cpus = ms->possible_cpus;
> + GQueue *socket_list = g_queue_new();
> + GQueue *cluster_list = g_queue_new();
> + GQueue *core_list = g_queue_new();
> GQueue *list = g_queue_new();
> guint pptt_start = table_data->len;
> guint parent_offset;
> guint length, i;
> - int uid = 0;
> - int socket;
> + int n, socket_id, cluster_id, core_id, thread_id;
> 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++) {
> + for (n = 0; n < cpus->len; n++) {
> + socket_id = cpus->cpus[n].props.socket_id;
> + if (g_queue_find(socket_list, GUINT_TO_POINTER(socket_id))) {
> + continue;
> + }
maybe instead of scanning cpus[n] every time for each topology level
and trying to keep code flattened (which mimics PPTT fattened tree
table for not much of the reason, spec doesn't require entries
from the same level to e described contiguously),
try to rebuild hierarchy tree from flat cpus[n] in 1 pass first
and then use nested loops or recursion to build PPTT table,
something like:
sockets = cpus_to_topo(possible)
build_pptt_level(items = sockets, parent_ref = 0)
for item in items
level_ref = table_data->len - pptt_start
build_processor_hierarchy_node(item {id, flags, ...}, parent_ref)
if not leaf:
build_pptt_level(item, level_ref)
which is much more compact and easier to read compared to
unrolled impl. it's now with all push/pop stack emulation.
> +
> + g_queue_push_tail(socket_list, GUINT_TO_POINTER(socket_id));
> g_queue_push_tail(list,
> GUINT_TO_POINTER(table_data->len - pptt_start));
> build_processor_hierarchy_node(
> @@ -2023,65 +2032,104 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> * of a physical package
> */
> (1 << 0),
> - 0, socket, NULL, 0);
> + 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++) {
> + socket_id = GPOINTER_TO_UINT(g_queue_pop_head(socket_list));
> +
> + for (n = 0; n < cpus->len; n++) {
> + if (cpus->cpus[n].props.socket_id != socket_id) {
> + continue;
> + }
> +
> + cluster_id = cpus->cpus[n].props.cluster_id;
> + if (g_queue_find(cluster_list, GUINT_TO_POINTER(cluster_id))) {
> + continue;
> + }
> +
> + g_queue_push_tail(cluster_list, GUINT_TO_POINTER(cluster_id));
> 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);
> + parent_offset, cluster_id, NULL, 0);
> }
> }
> }
>
> 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,
> - (0 << 0), /* not a physical package */
> - parent_offset, core, NULL, 0);
> - } else {
> + if (!mc->smp_props.clusters_supported) {
> + socket_id = GPOINTER_TO_UINT(g_queue_pop_head(socket_list));
> + } else {
> + cluster_id = GPOINTER_TO_UINT(g_queue_pop_head(cluster_list));
> + }
> +
> + for (n = 0; n < cpus->len; n++) {
> + if (!mc->smp_props.clusters_supported &&
> + cpus->cpus[n].props.socket_id != socket_id) {
> + continue;
> + }
> +
> + if (mc->smp_props.clusters_supported &&
> + cpus->cpus[n].props.cluster_id != cluster_id) {
> + continue;
> + }
> +
> + core_id = cpus->cpus[n].props.core_id;
> + if (ms->smp.threads <= 1) {
> build_processor_hierarchy_node(
> table_data,
> (1 << 1) | /* ACPI Processor ID valid */
> (1 << 3), /* Node is a Leaf */
> - parent_offset, uid++, NULL, 0);
> + parent_offset, core_id, NULL, 0);
> + continue;
> + }
> +
> + if (g_queue_find(core_list, GUINT_TO_POINTER(core_id))) {
> + continue;
> }
> +
> + g_queue_push_tail(core_list, GUINT_TO_POINTER(core_id));
> + 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, 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++) {
> + core_id = GPOINTER_TO_UINT(g_queue_pop_head(core_list));
> +
> + for (n = 0; n < cpus->len; n++) {
> + if (cpus->cpus[n].props.core_id != core_id) {
> + continue;
> + }
> +
> + thread_id = cpus->cpus[n].props.thread_id;
> 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);
> + parent_offset, thread_id, NULL, 0);
> }
> }
>
> g_queue_free(list);
> + g_queue_free(core_list);
> + g_queue_free(cluster_list);
> + g_queue_free(socket_list);
> acpi_table_end(linker, &table);
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 3/4] hw/acpi/aml-build: Use existing CPU topology to build PPTT table
2022-03-30 14:10 ` Igor Mammedov
@ 2022-04-03 14:40 ` Gavin Shan
0 siblings, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2022-04-03 14:40 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
Hi Igor,
On 3/30/22 10:10 PM, Igor Mammedov wrote:
> On Wed, 23 Mar 2022 15:24:37 +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 avoids to re-calculate the CPU topology 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 | 96 +++++++++++++++++++++++++++++++++------------
>> 1 file changed, 72 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 4086879ebf..10a2d63b96 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2002,18 +2002,27 @@ 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);
>> + CPUArchIdList *cpus = ms->possible_cpus;
>> + GQueue *socket_list = g_queue_new();
>> + GQueue *cluster_list = g_queue_new();
>> + GQueue *core_list = g_queue_new();
>> GQueue *list = g_queue_new();
>> guint pptt_start = table_data->len;
>> guint parent_offset;
>> guint length, i;
>> - int uid = 0;
>> - int socket;
>> + int n, socket_id, cluster_id, core_id, thread_id;
>> 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++) {
>> + for (n = 0; n < cpus->len; n++) {
>> + socket_id = cpus->cpus[n].props.socket_id;
>> + if (g_queue_find(socket_list, GUINT_TO_POINTER(socket_id))) {
>> + continue;
>> + }
>
> maybe instead of scanning cpus[n] every time for each topology level
> and trying to keep code flattened (which mimics PPTT fattened tree
> table for not much of the reason, spec doesn't require entries
> from the same level to e described contiguously),
> try to rebuild hierarchy tree from flat cpus[n] in 1 pass first
> and then use nested loops or recursion to build PPTT table,
> something like:
>
> sockets = cpus_to_topo(possible)
> build_pptt_level(items = sockets, parent_ref = 0)
> for item in items
> level_ref = table_data->len - pptt_start
> build_processor_hierarchy_node(item {id, flags, ...}, parent_ref)
> if not leaf:
> build_pptt_level(item, level_ref)
>
> which is much more compact and easier to read compared to
> unrolled impl. it's now with all push/pop stack emulation.
>
I missed your comments when v4 was posted. Sorry about this. I'm using
thunderbird mail client and have some filters running to put incoming
mails into the corresponding folders, but this reply has been put into
wrong folder. It's why I always copy my private email while sending
patches and emails. Please ignore v4 and review v5 directly.
Thanks for the suggestion, but it's going to introduce duplicate entries
for same socket/cluster/core, or I missed something. Otherwise, the
CPUs need to be iterated to check if they're in the corresponding level.
In order to make it simplified and remove the stack emulation stuff,
I will introduce variables to track the socket/cluster/core IDs whose
ACPI table entries have been added. Once the socket, cluster or core ID
changes while iterating 'ms->possible_cpus', the corresponding ACPI table
entry is added and the IDs for child levels are invalidated. With this,
all needed ACPI table entries will be created in one loop on 'ms->possible_cpus'
The changes will be included to v5, which will be posted shortly.
Thanks,
Gavin
>
>> +
>> + g_queue_push_tail(socket_list, GUINT_TO_POINTER(socket_id));
>> g_queue_push_tail(list,
>> GUINT_TO_POINTER(table_data->len - pptt_start));
>> build_processor_hierarchy_node(
>> @@ -2023,65 +2032,104 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>> * of a physical package
>> */
>> (1 << 0),
>> - 0, socket, NULL, 0);
>> + 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++) {
>> + socket_id = GPOINTER_TO_UINT(g_queue_pop_head(socket_list));
>> +
>> + for (n = 0; n < cpus->len; n++) {
>> + if (cpus->cpus[n].props.socket_id != socket_id) {
>> + continue;
>> + }
>> +
>> + cluster_id = cpus->cpus[n].props.cluster_id;
>> + if (g_queue_find(cluster_list, GUINT_TO_POINTER(cluster_id))) {
>> + continue;
>> + }
>> +
>> + g_queue_push_tail(cluster_list, GUINT_TO_POINTER(cluster_id));
>> 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);
>> + parent_offset, cluster_id, NULL, 0);
>> }
>> }
>> }
>>
>> 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,
>> - (0 << 0), /* not a physical package */
>> - parent_offset, core, NULL, 0);
>> - } else {
>> + if (!mc->smp_props.clusters_supported) {
>> + socket_id = GPOINTER_TO_UINT(g_queue_pop_head(socket_list));
>> + } else {
>> + cluster_id = GPOINTER_TO_UINT(g_queue_pop_head(cluster_list));
>> + }
>> +
>> + for (n = 0; n < cpus->len; n++) {
>> + if (!mc->smp_props.clusters_supported &&
>> + cpus->cpus[n].props.socket_id != socket_id) {
>> + continue;
>> + }
>> +
>> + if (mc->smp_props.clusters_supported &&
>> + cpus->cpus[n].props.cluster_id != cluster_id) {
>> + continue;
>> + }
>> +
>> + core_id = cpus->cpus[n].props.core_id;
>> + if (ms->smp.threads <= 1) {
>> build_processor_hierarchy_node(
>> table_data,
>> (1 << 1) | /* ACPI Processor ID valid */
>> (1 << 3), /* Node is a Leaf */
>> - parent_offset, uid++, NULL, 0);
>> + parent_offset, core_id, NULL, 0);
>> + continue;
>> + }
>> +
>> + if (g_queue_find(core_list, GUINT_TO_POINTER(core_id))) {
>> + continue;
>> }
>> +
>> + g_queue_push_tail(core_list, GUINT_TO_POINTER(core_id));
>> + 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, 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++) {
>> + core_id = GPOINTER_TO_UINT(g_queue_pop_head(core_list));
>> +
>> + for (n = 0; n < cpus->len; n++) {
>> + if (cpus->cpus[n].props.core_id != core_id) {
>> + continue;
>> + }
>> +
>> + thread_id = cpus->cpus[n].props.thread_id;
>> 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);
>> + parent_offset, thread_id, NULL, 0);
>> }
>> }
>>
>> g_queue_free(list);
>> + g_queue_free(core_list);
>> + g_queue_free(cluster_list);
>> + g_queue_free(socket_list);
>> acpi_table_end(linker, &table);
>> }
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 4/4] hw/arm/virt: Unify ACPI processor ID in MADT and SRAT table
2022-03-23 7:24 [PATCH v3 0/4] hw/arm/virt: Fix CPU's default NUMA node ID Gavin Shan
` (2 preceding siblings ...)
2022-03-23 7:24 ` [PATCH v3 3/4] hw/acpi/aml-build: Use existing CPU topology to build PPTT table Gavin Shan
@ 2022-03-23 7:24 ` Gavin Shan
2022-03-25 14:00 ` Igor Mammedov
3 siblings, 1 reply; 23+ messages in thread
From: Gavin Shan @ 2022-03-23 7:24 UTC (permalink / raw)
To: qemu-arm
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, shan.gavin, imammedo
The value of the following field has been used in ACPI PPTT table
to identify the corresponding processor. This takes the same field
as the ACPI processor ID in MADT and SRAT tables.
ms->possible_cpus->cpus[i].props.thread_id
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/arm/virt-acpi-build.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 449fab0080..7fedb56eea 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -534,13 +534,16 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
for (i = 0; i < cpu_list->len; ++i) {
uint32_t nodeid = cpu_list->cpus[i].props.node_id;
+ uint32_t thread_id = cpu_list->cpus[i].props.thread_id;
+
/*
* 5.2.16.4 GICC Affinity Structure
*/
build_append_int_noprefix(table_data, 3, 1); /* Type */
build_append_int_noprefix(table_data, 18, 1); /* Length */
build_append_int_noprefix(table_data, nodeid, 4); /* Proximity Domain */
- build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
+ build_append_int_noprefix(table_data,
+ thread_id, 4); /* ACPI Processor UID */
/* Flags, Table 5-76 */
build_append_int_noprefix(table_data, 1 /* Enabled */, 4);
build_append_int_noprefix(table_data, 0, 4); /* Clock Domain */
@@ -704,6 +707,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
{
int i;
VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
+ MachineState *ms = MACHINE(vms);
const MemMapEntry *memmap = vms->memmap;
AcpiTable table = { .sig = "APIC", .rev = 3, .oem_id = vms->oem_id,
.oem_table_id = vms->oem_table_id };
@@ -725,8 +729,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
build_append_int_noprefix(table_data, vms->gic_version, 1);
build_append_int_noprefix(table_data, 0, 3); /* Reserved */
- for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
+ for (i = 0; i < ms->smp.cpus; i++) {
ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
+ uint32_t thread_id = ms->possible_cpus->cpus[i].props.thread_id;
uint64_t physical_base_address = 0, gich = 0, gicv = 0;
uint32_t vgic_interrupt = vms->virt ? PPI(ARCH_GIC_MAINT_IRQ) : 0;
uint32_t pmu_interrupt = arm_feature(&armcpu->env, ARM_FEATURE_PMU) ?
@@ -743,7 +748,8 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
build_append_int_noprefix(table_data, 76, 1); /* Length */
build_append_int_noprefix(table_data, 0, 2); /* Reserved */
build_append_int_noprefix(table_data, i, 4); /* GIC ID */
- build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
+ build_append_int_noprefix(table_data,
+ thread_id, 4); /* ACPI Processor UID */
/* Flags */
build_append_int_noprefix(table_data, 1, 4); /* Enabled */
/* Parking Protocol Version */
--
2.23.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 4/4] hw/arm/virt: Unify ACPI processor ID in MADT and SRAT table
2022-03-23 7:24 ` [PATCH v3 4/4] hw/arm/virt: Unify ACPI processor ID in MADT and SRAT table Gavin Shan
@ 2022-03-25 14:00 ` Igor Mammedov
2022-03-25 19:08 ` Gavin Shan
0 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2022-03-25 14:00 UTC (permalink / raw)
To: Gavin Shan
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
On Wed, 23 Mar 2022 15:24:38 +0800
Gavin Shan <gshan@redhat.com> wrote:
> The value of the following field has been used in ACPI PPTT table
> to identify the corresponding processor. This takes the same field
> as the ACPI processor ID in MADT and SRAT tables.
>
> ms->possible_cpus->cpus[i].props.thread_id
thread-id could be something different than ACPI processor ID
(to be discussed in the first patch).
I'd prefer to keep both decoupled, i.e. use [0 - possible_cpus->len)
for ACPI processor ID as it's done with x86 madt/srat and ACPI CPU
hotplug code. So we could reuse at least the later when implementing
CPU hotplug for arm/virt and it's good from consistency point of view.
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> hw/arm/virt-acpi-build.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 449fab0080..7fedb56eea 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -534,13 +534,16 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>
> for (i = 0; i < cpu_list->len; ++i) {
> uint32_t nodeid = cpu_list->cpus[i].props.node_id;
> + uint32_t thread_id = cpu_list->cpus[i].props.thread_id;
> +
> /*
> * 5.2.16.4 GICC Affinity Structure
> */
> build_append_int_noprefix(table_data, 3, 1); /* Type */
> build_append_int_noprefix(table_data, 18, 1); /* Length */
> build_append_int_noprefix(table_data, nodeid, 4); /* Proximity Domain */
> - build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
> + build_append_int_noprefix(table_data,
> + thread_id, 4); /* ACPI Processor UID */
> /* Flags, Table 5-76 */
> build_append_int_noprefix(table_data, 1 /* Enabled */, 4);
> build_append_int_noprefix(table_data, 0, 4); /* Clock Domain */
> @@ -704,6 +707,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> {
> int i;
> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> + MachineState *ms = MACHINE(vms);
> const MemMapEntry *memmap = vms->memmap;
> AcpiTable table = { .sig = "APIC", .rev = 3, .oem_id = vms->oem_id,
> .oem_table_id = vms->oem_table_id };
> @@ -725,8 +729,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> build_append_int_noprefix(table_data, vms->gic_version, 1);
> build_append_int_noprefix(table_data, 0, 3); /* Reserved */
>
> - for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
> + for (i = 0; i < ms->smp.cpus; i++) {
> ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
> + uint32_t thread_id = ms->possible_cpus->cpus[i].props.thread_id;
> uint64_t physical_base_address = 0, gich = 0, gicv = 0;
> uint32_t vgic_interrupt = vms->virt ? PPI(ARCH_GIC_MAINT_IRQ) : 0;
> uint32_t pmu_interrupt = arm_feature(&armcpu->env, ARM_FEATURE_PMU) ?
> @@ -743,7 +748,8 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> build_append_int_noprefix(table_data, 76, 1); /* Length */
> build_append_int_noprefix(table_data, 0, 2); /* Reserved */
> build_append_int_noprefix(table_data, i, 4); /* GIC ID */
> - build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
> + build_append_int_noprefix(table_data,
> + thread_id, 4); /* ACPI Processor UID */
> /* Flags */
> build_append_int_noprefix(table_data, 1, 4); /* Enabled */
> /* Parking Protocol Version */
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 4/4] hw/arm/virt: Unify ACPI processor ID in MADT and SRAT table
2022-03-25 14:00 ` Igor Mammedov
@ 2022-03-25 19:08 ` Gavin Shan
2022-03-30 12:52 ` Igor Mammedov
0 siblings, 1 reply; 23+ messages in thread
From: Gavin Shan @ 2022-03-25 19:08 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
Hi Igor,
On 3/25/22 10:00 PM, Igor Mammedov wrote:
> On Wed, 23 Mar 2022 15:24:38 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>
>> The value of the following field has been used in ACPI PPTT table
>> to identify the corresponding processor. This takes the same field
>> as the ACPI processor ID in MADT and SRAT tables.
>>
>> ms->possible_cpus->cpus[i].props.thread_id
>
> thread-id could be something different than ACPI processor ID
> (to be discussed in the first patch).
>
> I'd prefer to keep both decoupled, i.e. use [0 - possible_cpus->len)
> for ACPI processor ID as it's done with x86 madt/srat and ACPI CPU
> hotplug code. So we could reuse at least the later when implementing
> CPU hotplug for arm/virt and it's good from consistency point of view.
>
Yeah, I think so, thread-id and ACPI processor UID could be different.
thread IDs could be overlapped on multiple CPUs, who belongs to different
socket/die/core. However, ACPI processor UID should be unique for the
CPU in the whole system.
If you agree, Lets use [0 - possible_cpus->len] in next respin. What I
need to do is dropping PATCH[4/4] and replacing @thread_id with @n in
build_pptt() of PATCH[3/4].
Thanks,
Gavin
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> hw/arm/virt-acpi-build.c | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 449fab0080..7fedb56eea 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -534,13 +534,16 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>
>> for (i = 0; i < cpu_list->len; ++i) {
>> uint32_t nodeid = cpu_list->cpus[i].props.node_id;
>> + uint32_t thread_id = cpu_list->cpus[i].props.thread_id;
>> +
>> /*
>> * 5.2.16.4 GICC Affinity Structure
>> */
>> build_append_int_noprefix(table_data, 3, 1); /* Type */
>> build_append_int_noprefix(table_data, 18, 1); /* Length */
>> build_append_int_noprefix(table_data, nodeid, 4); /* Proximity Domain */
>> - build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
>> + build_append_int_noprefix(table_data,
>> + thread_id, 4); /* ACPI Processor UID */
>> /* Flags, Table 5-76 */
>> build_append_int_noprefix(table_data, 1 /* Enabled */, 4);
>> build_append_int_noprefix(table_data, 0, 4); /* Clock Domain */
>> @@ -704,6 +707,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> {
>> int i;
>> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>> + MachineState *ms = MACHINE(vms);
>> const MemMapEntry *memmap = vms->memmap;
>> AcpiTable table = { .sig = "APIC", .rev = 3, .oem_id = vms->oem_id,
>> .oem_table_id = vms->oem_table_id };
>> @@ -725,8 +729,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> build_append_int_noprefix(table_data, vms->gic_version, 1);
>> build_append_int_noprefix(table_data, 0, 3); /* Reserved */
>>
>> - for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
>> + for (i = 0; i < ms->smp.cpus; i++) {
>> ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
>> + uint32_t thread_id = ms->possible_cpus->cpus[i].props.thread_id;
>> uint64_t physical_base_address = 0, gich = 0, gicv = 0;
>> uint32_t vgic_interrupt = vms->virt ? PPI(ARCH_GIC_MAINT_IRQ) : 0;
>> uint32_t pmu_interrupt = arm_feature(&armcpu->env, ARM_FEATURE_PMU) ?
>> @@ -743,7 +748,8 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> build_append_int_noprefix(table_data, 76, 1); /* Length */
>> build_append_int_noprefix(table_data, 0, 2); /* Reserved */
>> build_append_int_noprefix(table_data, i, 4); /* GIC ID */
>> - build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
>> + build_append_int_noprefix(table_data,
>> + thread_id, 4); /* ACPI Processor UID */
>> /* Flags */
>> build_append_int_noprefix(table_data, 1, 4); /* Enabled */
>> /* Parking Protocol Version */
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 4/4] hw/arm/virt: Unify ACPI processor ID in MADT and SRAT table
2022-03-25 19:08 ` Gavin Shan
@ 2022-03-30 12:52 ` Igor Mammedov
2022-04-03 10:43 ` Gavin Shan
0 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2022-03-30 12:52 UTC (permalink / raw)
To: Gavin Shan
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
On Sat, 26 Mar 2022 03:08:19 +0800
Gavin Shan <gshan@redhat.com> wrote:
> Hi Igor,
>
> On 3/25/22 10:00 PM, Igor Mammedov wrote:
> > On Wed, 23 Mar 2022 15:24:38 +0800
> > Gavin Shan <gshan@redhat.com> wrote:
> >
> >> The value of the following field has been used in ACPI PPTT table
> >> to identify the corresponding processor. This takes the same field
> >> as the ACPI processor ID in MADT and SRAT tables.
> >>
> >> ms->possible_cpus->cpus[i].props.thread_id
> >
> > thread-id could be something different than ACPI processor ID
> > (to be discussed in the first patch).
> >
> > I'd prefer to keep both decoupled, i.e. use [0 - possible_cpus->len)
> > for ACPI processor ID as it's done with x86 madt/srat and ACPI CPU
> > hotplug code. So we could reuse at least the later when implementing
> > CPU hotplug for arm/virt and it's good from consistency point of view.
> >
>
> Yeah, I think so, thread-id and ACPI processor UID could be different.
> thread IDs could be overlapped on multiple CPUs, who belongs to different
> socket/die/core. However, ACPI processor UID should be unique for the
> CPU in the whole system.
>
> If you agree, Lets use [0 - possible_cpus->len] in next respin.
Agreed.
> What I
> need to do is dropping PATCH[4/4] and replacing @thread_id with @n in
> build_pptt() of PATCH[3/4].
>
> Thanks,
> Gavin
>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> ---
> >> hw/arm/virt-acpi-build.c | 12 +++++++++---
> >> 1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index 449fab0080..7fedb56eea 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -534,13 +534,16 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >>
> >> for (i = 0; i < cpu_list->len; ++i) {
> >> uint32_t nodeid = cpu_list->cpus[i].props.node_id;
> >> + uint32_t thread_id = cpu_list->cpus[i].props.thread_id;
> >> +
> >> /*
> >> * 5.2.16.4 GICC Affinity Structure
> >> */
> >> build_append_int_noprefix(table_data, 3, 1); /* Type */
> >> build_append_int_noprefix(table_data, 18, 1); /* Length */
> >> build_append_int_noprefix(table_data, nodeid, 4); /* Proximity Domain */
> >> - build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
> >> + build_append_int_noprefix(table_data,
> >> + thread_id, 4); /* ACPI Processor UID */
> >> /* Flags, Table 5-76 */
> >> build_append_int_noprefix(table_data, 1 /* Enabled */, 4);
> >> build_append_int_noprefix(table_data, 0, 4); /* Clock Domain */
> >> @@ -704,6 +707,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >> {
> >> int i;
> >> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> >> + MachineState *ms = MACHINE(vms);
> >> const MemMapEntry *memmap = vms->memmap;
> >> AcpiTable table = { .sig = "APIC", .rev = 3, .oem_id = vms->oem_id,
> >> .oem_table_id = vms->oem_table_id };
> >> @@ -725,8 +729,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >> build_append_int_noprefix(table_data, vms->gic_version, 1);
> >> build_append_int_noprefix(table_data, 0, 3); /* Reserved */
> >>
> >> - for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
> >> + for (i = 0; i < ms->smp.cpus; i++) {
> >> ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
> >> + uint32_t thread_id = ms->possible_cpus->cpus[i].props.thread_id;
> >> uint64_t physical_base_address = 0, gich = 0, gicv = 0;
> >> uint32_t vgic_interrupt = vms->virt ? PPI(ARCH_GIC_MAINT_IRQ) : 0;
> >> uint32_t pmu_interrupt = arm_feature(&armcpu->env, ARM_FEATURE_PMU) ?
> >> @@ -743,7 +748,8 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >> build_append_int_noprefix(table_data, 76, 1); /* Length */
> >> build_append_int_noprefix(table_data, 0, 2); /* Reserved */
> >> build_append_int_noprefix(table_data, i, 4); /* GIC ID */
> >> - build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
> >> + build_append_int_noprefix(table_data,
> >> + thread_id, 4); /* ACPI Processor UID */
> >> /* Flags */
> >> build_append_int_noprefix(table_data, 1, 4); /* Enabled */
> >> /* Parking Protocol Version */
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 4/4] hw/arm/virt: Unify ACPI processor ID in MADT and SRAT table
2022-03-30 12:52 ` Igor Mammedov
@ 2022-04-03 10:43 ` Gavin Shan
0 siblings, 0 replies; 23+ messages in thread
From: Gavin Shan @ 2022-04-03 10:43 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, drjones, richard.henderson, qemu-devel, zhenyzha,
wangyanan55, qemu-arm, shan.gavin
Hi Igor,
On 3/30/22 8:52 PM, Igor Mammedov wrote:
> On Sat, 26 Mar 2022 03:08:19 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> On 3/25/22 10:00 PM, Igor Mammedov wrote:
>>> On Wed, 23 Mar 2022 15:24:38 +0800
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>>> The value of the following field has been used in ACPI PPTT table
>>>> to identify the corresponding processor. This takes the same field
>>>> as the ACPI processor ID in MADT and SRAT tables.
>>>>
>>>> ms->possible_cpus->cpus[i].props.thread_id
>>>
>>> thread-id could be something different than ACPI processor ID
>>> (to be discussed in the first patch).
>>>
>>> I'd prefer to keep both decoupled, i.e. use [0 - possible_cpus->len)
>>> for ACPI processor ID as it's done with x86 madt/srat and ACPI CPU
>>> hotplug code. So we could reuse at least the later when implementing
>>> CPU hotplug for arm/virt and it's good from consistency point of view.
>>>
>>
>> Yeah, I think so, thread-id and ACPI processor UID could be different.
>> thread IDs could be overlapped on multiple CPUs, who belongs to different
>> socket/die/core. However, ACPI processor UID should be unique for the
>> CPU in the whole system.
>>
>> If you agree, Lets use [0 - possible_cpus->len] in next respin.
>
> Agreed.
>
Thanks for your confirm and the changes, as I mentioned before, will be
included in v4.
>> What I
>> need to do is dropping PATCH[4/4] and replacing @thread_id with @n in
>> build_pptt() of PATCH[3/4].
>>
Thanks,
Gavin
>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>> hw/arm/virt-acpi-build.c | 12 +++++++++---
>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>> index 449fab0080..7fedb56eea 100644
>>>> --- a/hw/arm/virt-acpi-build.c
>>>> +++ b/hw/arm/virt-acpi-build.c
>>>> @@ -534,13 +534,16 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>>
>>>> for (i = 0; i < cpu_list->len; ++i) {
>>>> uint32_t nodeid = cpu_list->cpus[i].props.node_id;
>>>> + uint32_t thread_id = cpu_list->cpus[i].props.thread_id;
>>>> +
>>>> /*
>>>> * 5.2.16.4 GICC Affinity Structure
>>>> */
>>>> build_append_int_noprefix(table_data, 3, 1); /* Type */
>>>> build_append_int_noprefix(table_data, 18, 1); /* Length */
>>>> build_append_int_noprefix(table_data, nodeid, 4); /* Proximity Domain */
>>>> - build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
>>>> + build_append_int_noprefix(table_data,
>>>> + thread_id, 4); /* ACPI Processor UID */
>>>> /* Flags, Table 5-76 */
>>>> build_append_int_noprefix(table_data, 1 /* Enabled */, 4);
>>>> build_append_int_noprefix(table_data, 0, 4); /* Clock Domain */
>>>> @@ -704,6 +707,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>> {
>>>> int i;
>>>> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>>> + MachineState *ms = MACHINE(vms);
>>>> const MemMapEntry *memmap = vms->memmap;
>>>> AcpiTable table = { .sig = "APIC", .rev = 3, .oem_id = vms->oem_id,
>>>> .oem_table_id = vms->oem_table_id };
>>>> @@ -725,8 +729,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>> build_append_int_noprefix(table_data, vms->gic_version, 1);
>>>> build_append_int_noprefix(table_data, 0, 3); /* Reserved */
>>>>
>>>> - for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
>>>> + for (i = 0; i < ms->smp.cpus; i++) {
>>>> ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
>>>> + uint32_t thread_id = ms->possible_cpus->cpus[i].props.thread_id;
>>>> uint64_t physical_base_address = 0, gich = 0, gicv = 0;
>>>> uint32_t vgic_interrupt = vms->virt ? PPI(ARCH_GIC_MAINT_IRQ) : 0;
>>>> uint32_t pmu_interrupt = arm_feature(&armcpu->env, ARM_FEATURE_PMU) ?
>>>> @@ -743,7 +748,8 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>> build_append_int_noprefix(table_data, 76, 1); /* Length */
>>>> build_append_int_noprefix(table_data, 0, 2); /* Reserved */
>>>> build_append_int_noprefix(table_data, i, 4); /* GIC ID */
>>>> - build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
>>>> + build_append_int_noprefix(table_data,
>>>> + thread_id, 4); /* ACPI Processor UID */
>>>> /* Flags */
>>>> build_append_int_noprefix(table_data, 1, 4); /* Enabled */
>>>> /* Parking Protocol Version */
>>>
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread