This adds cluster-id in CPU instance properties, which will be used
by arm/virt machine. Besides, the cluster-id is also verified or
dumped in various spots:
* hw/core/machine.c::machine_set_cpu_numa_node() to associate
CPU with its NUMA node.
* hw/core/machine.c::machine_numa_finish_cpu_init() to associate
CPU with NUMA node when no default association isn't provided.
* hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
cluster-id.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/core/machine-hmp-cmds.c | 4 ++++
hw/core/machine.c | 16 ++++++++++++++++
qapi/machine.json | 6 ++++--
3 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
index 4e2f319aeb..5cb5eecbfc 100644
--- a/hw/core/machine-hmp-cmds.c
+++ b/hw/core/machine-hmp-cmds.c
@@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
if (c->has_die_id) {
monitor_printf(mon, " die-id: \"%" PRIu64 "\"\n", c->die_id);
}
+ if (c->has_cluster_id) {
+ monitor_printf(mon, " cluster-id: \"%" PRIu64 "\"\n",
+ c->cluster_id);
+ }
if (c->has_core_id) {
monitor_printf(mon, " core-id: \"%" PRIu64 "\"\n", c->core_id);
}
diff --git a/hw/core/machine.c b/hw/core/machine.c
index d856485cb4..8748b64657 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -677,6 +677,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
return;
}
+ if (props->has_cluster_id && !slot->props.has_cluster_id) {
+ error_setg(errp, "cluster-id is not supported");
+ return;
+ }
+
if (props->has_socket_id && !slot->props.has_socket_id) {
error_setg(errp, "socket-id is not supported");
return;
@@ -696,6 +701,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
continue;
}
+ if (props->has_cluster_id &&
+ props->cluster_id != slot->props.cluster_id) {
+ continue;
+ }
+
if (props->has_die_id && props->die_id != slot->props.die_id) {
continue;
}
@@ -990,6 +1000,12 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
}
g_string_append_printf(s, "die-id: %"PRId64, cpu->props.die_id);
}
+ if (cpu->props.has_cluster_id) {
+ if (s->len) {
+ g_string_append_printf(s, ", ");
+ }
+ g_string_append_printf(s, "cluster-id: %"PRId64, cpu->props.cluster_id);
+ }
if (cpu->props.has_core_id) {
if (s->len) {
g_string_append_printf(s, ", ");
diff --git a/qapi/machine.json b/qapi/machine.json
index 9c460ec450..ea22b574b0 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -868,10 +868,11 @@
# @node-id: NUMA node ID the CPU belongs to
# @socket-id: socket number within node/board the CPU belongs to
# @die-id: die number within socket the CPU belongs to (since 4.1)
-# @core-id: core number within die the CPU belongs to
+# @cluster-id: cluster number within die the CPU belongs to
+# @core-id: core number within cluster/die the CPU belongs to
# @thread-id: thread number within core the CPU belongs to
#
-# Note: currently there are 5 properties that could be present
+# Note: currently there are 6 properties that could be present
# but management should be prepared to pass through other
# properties with device_add command to allow for future
# interface extension. This also requires the filed names to be kept in
@@ -883,6 +884,7 @@
'data': { '*node-id': 'int',
'*socket-id': 'int',
'*die-id': 'int',
+ '*cluster-id': 'int',
'*core-id': 'int',
'*thread-id': 'int'
}
--
2.23.0Hi Gavin,
On 2022/4/3 22:59, Gavin Shan wrote:
> This adds cluster-id in CPU instance properties, which will be used
> by arm/virt machine. Besides, the cluster-id is also verified or
> dumped in various spots:
>
> * hw/core/machine.c::machine_set_cpu_numa_node() to associate
> CPU with its NUMA node.
>
> * hw/core/machine.c::machine_numa_finish_cpu_init() to associate
> CPU with NUMA node when no default association isn't provided.
>
> * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
> cluster-id.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> hw/core/machine-hmp-cmds.c | 4 ++++
> hw/core/machine.c | 16 ++++++++++++++++
> qapi/machine.json | 6 ++++--
> 3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
> index 4e2f319aeb..5cb5eecbfc 100644
> --- a/hw/core/machine-hmp-cmds.c
> +++ b/hw/core/machine-hmp-cmds.c
> @@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
> if (c->has_die_id) {
> monitor_printf(mon, " die-id: \"%" PRIu64 "\"\n", c->die_id);
> }
> + if (c->has_cluster_id) {
> + monitor_printf(mon, " cluster-id: \"%" PRIu64 "\"\n",
> + c->cluster_id);
> + }
> if (c->has_core_id) {
> monitor_printf(mon, " core-id: \"%" PRIu64 "\"\n", c->core_id);
> }
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d856485cb4..8748b64657 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -677,6 +677,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
> return;
> }
>
> + if (props->has_cluster_id && !slot->props.has_cluster_id) {
> + error_setg(errp, "cluster-id is not supported");
> + return;
> + }
> +
> if (props->has_socket_id && !slot->props.has_socket_id) {
> error_setg(errp, "socket-id is not supported");
> return;
> @@ -696,6 +701,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
> continue;
> }
>
> + if (props->has_cluster_id &&
> + props->cluster_id != slot->props.cluster_id) {
> + continue;
> + }
> +
> if (props->has_die_id && props->die_id != slot->props.die_id) {
> continue;
> }
> @@ -990,6 +1000,12 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
> }
> g_string_append_printf(s, "die-id: %"PRId64, cpu->props.die_id);
> }
> + if (cpu->props.has_cluster_id) {
> + if (s->len) {
> + g_string_append_printf(s, ", ");
> + }
> + g_string_append_printf(s, "cluster-id: %"PRId64, cpu->props.cluster_id);
> + }
> if (cpu->props.has_core_id) {
> if (s->len) {
> g_string_append_printf(s, ", ");
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 9c460ec450..ea22b574b0 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -868,10 +868,11 @@
> # @node-id: NUMA node ID the CPU belongs to
> # @socket-id: socket number within node/board the CPU belongs to
> # @die-id: die number within socket the CPU belongs to (since 4.1)
> -# @core-id: core number within die the CPU belongs to
> +# @cluster-id: cluster number within die the CPU belongs to
I remember this should be "cluster number within socket..."
according to Igor's comments in v3 ?
> +# @core-id: core number within cluster/die the CPU belongs to
> # @thread-id: thread number within core the CPU belongs to
> #
> -# Note: currently there are 5 properties that could be present
> +# Note: currently there are 6 properties that could be present
> # but management should be prepared to pass through other
> # properties with device_add command to allow for future
> # interface extension. This also requires the filed names to be kept in
> @@ -883,6 +884,7 @@
> 'data': { '*node-id': 'int',
> '*socket-id': 'int',
> '*die-id': 'int',
> + '*cluster-id': 'int',
> '*core-id': 'int',
> '*thread-id': 'int'
> }
Otherwise, looks good to me:
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Please also keep the involved Maintainers on Cc list in next version,
an Ack from them is best. :)
Thanks,
Yanan
Hi Yanan,
On 4/13/22 7:49 PM, wangyanan (Y) wrote:
> On 2022/4/3 22:59, Gavin Shan wrote:
>> This adds cluster-id in CPU instance properties, which will be used
>> by arm/virt machine. Besides, the cluster-id is also verified or
>> dumped in various spots:
>>
>> * hw/core/machine.c::machine_set_cpu_numa_node() to associate
>> CPU with its NUMA node.
>>
>> * hw/core/machine.c::machine_numa_finish_cpu_init() to associate
>> CPU with NUMA node when no default association isn't provided.
>>
>> * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
>> cluster-id.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> hw/core/machine-hmp-cmds.c | 4 ++++
>> hw/core/machine.c | 16 ++++++++++++++++
>> qapi/machine.json | 6 ++++--
>> 3 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
>> index 4e2f319aeb..5cb5eecbfc 100644
>> --- a/hw/core/machine-hmp-cmds.c
>> +++ b/hw/core/machine-hmp-cmds.c
>> @@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
>> if (c->has_die_id) {
>> monitor_printf(mon, " die-id: \"%" PRIu64 "\"\n", c->die_id);
>> }
>> + if (c->has_cluster_id) {
>> + monitor_printf(mon, " cluster-id: \"%" PRIu64 "\"\n",
>> + c->cluster_id);
>> + }
>> if (c->has_core_id) {
>> monitor_printf(mon, " core-id: \"%" PRIu64 "\"\n", c->core_id);
>> }
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index d856485cb4..8748b64657 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -677,6 +677,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
>> return;
>> }
>> + if (props->has_cluster_id && !slot->props.has_cluster_id) {
>> + error_setg(errp, "cluster-id is not supported");
>> + return;
>> + }
>> +
>> if (props->has_socket_id && !slot->props.has_socket_id) {
>> error_setg(errp, "socket-id is not supported");
>> return;
>> @@ -696,6 +701,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
>> continue;
>> }
>> + if (props->has_cluster_id &&
>> + props->cluster_id != slot->props.cluster_id) {
>> + continue;
>> + }
>> +
>> if (props->has_die_id && props->die_id != slot->props.die_id) {
>> continue;
>> }
>> @@ -990,6 +1000,12 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
>> }
>> g_string_append_printf(s, "die-id: %"PRId64, cpu->props.die_id);
>> }
>> + if (cpu->props.has_cluster_id) {
>> + if (s->len) {
>> + g_string_append_printf(s, ", ");
>> + }
>> + g_string_append_printf(s, "cluster-id: %"PRId64, cpu->props.cluster_id);
>> + }
>> if (cpu->props.has_core_id) {
>> if (s->len) {
>> g_string_append_printf(s, ", ");
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index 9c460ec450..ea22b574b0 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -868,10 +868,11 @@
>> # @node-id: NUMA node ID the CPU belongs to
>> # @socket-id: socket number within node/board the CPU belongs to
>> # @die-id: die number within socket the CPU belongs to (since 4.1)
>> -# @core-id: core number within die the CPU belongs to
>> +# @cluster-id: cluster number within die the CPU belongs to
> I remember this should be "cluster number within socket..."
> according to Igor's comments in v3 ?
Igor had suggestion to correct the description for 'core-id' like
below, but he didn't suggest anything for 'cluster-id'. The question
is clusters are sub-components of die, instead of socket, if die
is supported? You may want to me change it like below and please
confirm.
@cluster-id: cluster number within die/socket the CPU belongs to
suggestion from Ignor in v3:
> +# @core-id: core number within cluster the CPU belongs to
s:cluster:cluster/die:
>> +# @core-id: core number within cluster/die the CPU belongs to
>> # @thread-id: thread number within core the CPU belongs to
>> #
>> -# Note: currently there are 5 properties that could be present
>> +# Note: currently there are 6 properties that could be present
>> # but management should be prepared to pass through other
>> # properties with device_add command to allow for future
>> # interface extension. This also requires the filed names to be kept in
>> @@ -883,6 +884,7 @@
>> 'data': { '*node-id': 'int',
>> '*socket-id': 'int',
>> '*die-id': 'int',
>> + '*cluster-id': 'int',
>> '*core-id': 'int',
>> '*thread-id': 'int'
>> }
> Otherwise, looks good to me:
> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>
> Please also keep the involved Maintainers on Cc list in next version,
> an Ack from them is best. :)
>
Thanks again for your time to review. Sure, I will do in next posting.
Thanks,
Gavin
Hi Gavin,
Cc: Daniel and Markus
On 2022/4/14 8:06, Gavin Shan wrote:
> Hi Yanan,
>
> On 4/13/22 7:49 PM, wangyanan (Y) wrote:
>> On 2022/4/3 22:59, Gavin Shan wrote:
>>> This adds cluster-id in CPU instance properties, which will be used
>>> by arm/virt machine. Besides, the cluster-id is also verified or
>>> dumped in various spots:
>>>
>>> * hw/core/machine.c::machine_set_cpu_numa_node() to associate
>>> CPU with its NUMA node.
>>>
>>> * hw/core/machine.c::machine_numa_finish_cpu_init() to associate
>>> CPU with NUMA node when no default association isn't provided.
>>>
>>> * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
>>> cluster-id.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>> hw/core/machine-hmp-cmds.c | 4 ++++
>>> hw/core/machine.c | 16 ++++++++++++++++
>>> qapi/machine.json | 6 ++++--
>>> 3 files changed, 24 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
>>> index 4e2f319aeb..5cb5eecbfc 100644
>>> --- a/hw/core/machine-hmp-cmds.c
>>> +++ b/hw/core/machine-hmp-cmds.c
>>> @@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon, const
>>> QDict *qdict)
>>> if (c->has_die_id) {
>>> monitor_printf(mon, " die-id: \"%" PRIu64 "\"\n",
>>> c->die_id);
>>> }
>>> + if (c->has_cluster_id) {
>>> + monitor_printf(mon, " cluster-id: \"%" PRIu64 "\"\n",
>>> + c->cluster_id);
>>> + }
>>> if (c->has_core_id) {
>>> monitor_printf(mon, " core-id: \"%" PRIu64 "\"\n",
>>> c->core_id);
>>> }
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index d856485cb4..8748b64657 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -677,6 +677,11 @@ void machine_set_cpu_numa_node(MachineState
>>> *machine,
>>> return;
>>> }
>>> + if (props->has_cluster_id && !slot->props.has_cluster_id) {
>>> + error_setg(errp, "cluster-id is not supported");
>>> + return;
>>> + }
>>> +
>>> if (props->has_socket_id && !slot->props.has_socket_id) {
>>> error_setg(errp, "socket-id is not supported");
>>> return;
>>> @@ -696,6 +701,11 @@ void machine_set_cpu_numa_node(MachineState
>>> *machine,
>>> continue;
>>> }
>>> + if (props->has_cluster_id &&
>>> + props->cluster_id != slot->props.cluster_id) {
>>> + continue;
>>> + }
>>> +
>>> if (props->has_die_id && props->die_id !=
>>> slot->props.die_id) {
>>> continue;
>>> }
>>> @@ -990,6 +1000,12 @@ static char *cpu_slot_to_string(const
>>> CPUArchId *cpu)
>>> }
>>> g_string_append_printf(s, "die-id: %"PRId64,
>>> cpu->props.die_id);
>>> }
>>> + if (cpu->props.has_cluster_id) {
>>> + if (s->len) {
>>> + g_string_append_printf(s, ", ");
>>> + }
>>> + g_string_append_printf(s, "cluster-id: %"PRId64,
>>> cpu->props.cluster_id);
>>> + }
>>> if (cpu->props.has_core_id) {
>>> if (s->len) {
>>> g_string_append_printf(s, ", ");
>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>> index 9c460ec450..ea22b574b0 100644
>>> --- a/qapi/machine.json
>>> +++ b/qapi/machine.json
>>> @@ -868,10 +868,11 @@
>>> # @node-id: NUMA node ID the CPU belongs to
>>> # @socket-id: socket number within node/board the CPU belongs to
>>> # @die-id: die number within socket the CPU belongs to (since 4.1)
>>> -# @core-id: core number within die the CPU belongs to
>>> +# @cluster-id: cluster number within die the CPU belongs to
We also need a "(since 7.1)" tag for cluster-id.
>> I remember this should be "cluster number within socket..."
>> according to Igor's comments in v3 ?
>
> Igor had suggestion to correct the description for 'core-id' like
> below, but he didn't suggest anything for 'cluster-id'. The question
> is clusters are sub-components of die, instead of socket, if die
> is supported? You may want to me change it like below and please
> confirm.
>
> @cluster-id: cluster number within die/socket the CPU belongs to
>
> suggestion from Ignor in v3:
>
> > +# @core-id: core number within cluster the CPU belongs to
>
> s:cluster:cluster/die:
>
We want "within cluster/die" description for core-id because we
support both "cores in cluster" for ARM and "cores in die" for X86.
Base on this routine, we only need "within socket" for cluster-id
because we currently only support "clusters in socket". Does this
make sense?
Alternatively, the plainest documentation for the IDs is to simply
range **-id only to its next level topo like below. This may avoid
increasing complexity when more topo-ids are inserted middle.
But whether this way is acceptable is up to the Maintainers. :)
# @socket-id: socket number within node/board the CPU belongs to
# @die-id: die number within socket the CPU belongs to (since 4.1)
# @cluster-id: cluster number within die the CPU belongs to (since 7.1)
# @core-id: core number within cluster the CPU belongs to
# @thread-id: thread number within core the CPU belongs to
Thanks,
Yanan
>
>>> +# @core-id: core number within cluster/die the CPU belongs to
>>> # @thread-id: thread number within core the CPU belongs to
>>> #
>>> -# Note: currently there are 5 properties that could be present
>>> +# Note: currently there are 6 properties that could be present
>>> # but management should be prepared to pass through other
>>> # properties with device_add command to allow for future
>>> # interface extension. This also requires the filed names to
>>> be kept in
>>> @@ -883,6 +884,7 @@
>>> 'data': { '*node-id': 'int',
>>> '*socket-id': 'int',
>>> '*die-id': 'int',
>>> + '*cluster-id': 'int',
>>> '*core-id': 'int',
>>> '*thread-id': 'int'
>>> }
>> Otherwise, looks good to me:
>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>
>> Please also keep the involved Maintainers on Cc list in next version,
>> an Ack from them is best. :)
>>
>
> Thanks again for your time to review. Sure, I will do in next posting.
>
> Thanks,
> Gavin
>
> .
On Thu, Apr 14, 2022 at 10:27:25AM +0800, wangyanan (Y) wrote:
> Hi Gavin,
>
> Cc: Daniel and Markus
> On 2022/4/14 8:06, Gavin Shan wrote:
> > Hi Yanan,
> >
> > On 4/13/22 7:49 PM, wangyanan (Y) wrote:
> > > On 2022/4/3 22:59, Gavin Shan wrote:
> > > > This adds cluster-id in CPU instance properties, which will be used
> > > > by arm/virt machine. Besides, the cluster-id is also verified or
> > > > dumped in various spots:
> > > >
> > > > * hw/core/machine.c::machine_set_cpu_numa_node() to associate
> > > > CPU with its NUMA node.
> > > >
> > > > * hw/core/machine.c::machine_numa_finish_cpu_init() to associate
> > > > CPU with NUMA node when no default association isn't provided.
> > > >
> > > > * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
> > > > cluster-id.
> > > >
> > > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > > > ---
> > > > hw/core/machine-hmp-cmds.c | 4 ++++
> > > > hw/core/machine.c | 16 ++++++++++++++++
> > > > qapi/machine.json | 6 ++++--
> > > > 3 files changed, 24 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
> > > > index 4e2f319aeb..5cb5eecbfc 100644
> > > > --- a/hw/core/machine-hmp-cmds.c
> > > > +++ b/hw/core/machine-hmp-cmds.c
> > > > @@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon,
> > > > const QDict *qdict)
> > > > if (c->has_die_id) {
> > > > monitor_printf(mon, " die-id: \"%" PRIu64
> > > > "\"\n", c->die_id);
> > > > }
> > > > + if (c->has_cluster_id) {
> > > > + monitor_printf(mon, " cluster-id: \"%" PRIu64 "\"\n",
> > > > + c->cluster_id);
> > > > + }
> > > > if (c->has_core_id) {
> > > > monitor_printf(mon, " core-id: \"%" PRIu64
> > > > "\"\n", c->core_id);
> > > > }
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index d856485cb4..8748b64657 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -677,6 +677,11 @@ void machine_set_cpu_numa_node(MachineState
> > > > *machine,
> > > > return;
> > > > }
> > > > + if (props->has_cluster_id && !slot->props.has_cluster_id) {
> > > > + error_setg(errp, "cluster-id is not supported");
> > > > + return;
> > > > + }
> > > > +
> > > > if (props->has_socket_id && !slot->props.has_socket_id) {
> > > > error_setg(errp, "socket-id is not supported");
> > > > return;
> > > > @@ -696,6 +701,11 @@ void machine_set_cpu_numa_node(MachineState
> > > > *machine,
> > > > continue;
> > > > }
> > > > + if (props->has_cluster_id &&
> > > > + props->cluster_id != slot->props.cluster_id) {
> > > > + continue;
> > > > + }
> > > > +
> > > > if (props->has_die_id && props->die_id !=
> > > > slot->props.die_id) {
> > > > continue;
> > > > }
> > > > @@ -990,6 +1000,12 @@ static char *cpu_slot_to_string(const
> > > > CPUArchId *cpu)
> > > > }
> > > > g_string_append_printf(s, "die-id: %"PRId64,
> > > > cpu->props.die_id);
> > > > }
> > > > + if (cpu->props.has_cluster_id) {
> > > > + if (s->len) {
> > > > + g_string_append_printf(s, ", ");
> > > > + }
> > > > + g_string_append_printf(s, "cluster-id: %"PRId64,
> > > > cpu->props.cluster_id);
> > > > + }
> > > > if (cpu->props.has_core_id) {
> > > > if (s->len) {
> > > > g_string_append_printf(s, ", ");
> > > > diff --git a/qapi/machine.json b/qapi/machine.json
> > > > index 9c460ec450..ea22b574b0 100644
> > > > --- a/qapi/machine.json
> > > > +++ b/qapi/machine.json
> > > > @@ -868,10 +868,11 @@
> > > > # @node-id: NUMA node ID the CPU belongs to
> > > > # @socket-id: socket number within node/board the CPU belongs to
> > > > # @die-id: die number within socket the CPU belongs to (since 4.1)
> > > > -# @core-id: core number within die the CPU belongs to
> > > > +# @cluster-id: cluster number within die the CPU belongs to
> We also need a "(since 7.1)" tag for cluster-id.
> > > I remember this should be "cluster number within socket..."
> > > according to Igor's comments in v3 ?
> >
> > Igor had suggestion to correct the description for 'core-id' like
> > below, but he didn't suggest anything for 'cluster-id'. The question
> > is clusters are sub-components of die, instead of socket, if die
> > is supported? You may want to me change it like below and please
> > confirm.
> >
> > @cluster-id: cluster number within die/socket the CPU belongs to
> >
> > suggestion from Ignor in v3:
> >
> > > +# @core-id: core number within cluster the CPU belongs to
> >
> > s:cluster:cluster/die:
> >
> We want "within cluster/die" description for core-id because we
> support both "cores in cluster" for ARM and "cores in die" for X86.
> Base on this routine, we only need "within socket" for cluster-id
> because we currently only support "clusters in socket". Does this
> make sense?
>
> Alternatively, the plainest documentation for the IDs is to simply
> range **-id only to its next level topo like below. This may avoid
> increasing complexity when more topo-ids are inserted middle.
> But whether this way is acceptable is up to the Maintainers. :)
Rather saying we only support cluster on ARM and only dies on x86,
I tend to view it as, we only support greater than 1 cluster on
ARM, and greater than 1 die on x86.
IOW, x86 implicitly always has exactly 1-and-only-1 cluster,
and arm implicitly always has exactly 1-and-only-1 die.
With this POV, then we can keep the description simple, only
refering to the immediately above level in the hierarchy.
>
> # @socket-id: socket number within node/board the CPU belongs to
> # @die-id: die number within socket the CPU belongs to (since 4.1)
> # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
> # @core-id: core number within cluster the CPU belongs to
> # @thread-id: thread number within core the CPU belongs to
So this suggested text is fine with me.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hi Daniel,
On 4/19/22 11:59 PM, Daniel P. Berrangé wrote:
> On Thu, Apr 14, 2022 at 10:27:25AM +0800, wangyanan (Y) wrote:
>> Hi Gavin,
>>
>> Cc: Daniel and Markus
>> On 2022/4/14 8:06, Gavin Shan wrote:
>>> Hi Yanan,
>>>
>>> On 4/13/22 7:49 PM, wangyanan (Y) wrote:
>>>> On 2022/4/3 22:59, Gavin Shan wrote:
>>>>> This adds cluster-id in CPU instance properties, which will be used
>>>>> by arm/virt machine. Besides, the cluster-id is also verified or
>>>>> dumped in various spots:
>>>>>
>>>>> * hw/core/machine.c::machine_set_cpu_numa_node() to associate
>>>>> CPU with its NUMA node.
>>>>>
>>>>> * hw/core/machine.c::machine_numa_finish_cpu_init() to associate
>>>>> CPU with NUMA node when no default association isn't provided.
>>>>>
>>>>> * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
>>>>> cluster-id.
>>>>>
>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>> ---
>>>>> hw/core/machine-hmp-cmds.c | 4 ++++
>>>>> hw/core/machine.c | 16 ++++++++++++++++
>>>>> qapi/machine.json | 6 ++++--
>>>>> 3 files changed, 24 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
>>>>> index 4e2f319aeb..5cb5eecbfc 100644
>>>>> --- a/hw/core/machine-hmp-cmds.c
>>>>> +++ b/hw/core/machine-hmp-cmds.c
>>>>> @@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon,
>>>>> const QDict *qdict)
>>>>> if (c->has_die_id) {
>>>>> monitor_printf(mon, " die-id: \"%" PRIu64
>>>>> "\"\n", c->die_id);
>>>>> }
>>>>> + if (c->has_cluster_id) {
>>>>> + monitor_printf(mon, " cluster-id: \"%" PRIu64 "\"\n",
>>>>> + c->cluster_id);
>>>>> + }
>>>>> if (c->has_core_id) {
>>>>> monitor_printf(mon, " core-id: \"%" PRIu64
>>>>> "\"\n", c->core_id);
>>>>> }
>>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>>> index d856485cb4..8748b64657 100644
>>>>> --- a/hw/core/machine.c
>>>>> +++ b/hw/core/machine.c
>>>>> @@ -677,6 +677,11 @@ void machine_set_cpu_numa_node(MachineState
>>>>> *machine,
>>>>> return;
>>>>> }
>>>>> + if (props->has_cluster_id && !slot->props.has_cluster_id) {
>>>>> + error_setg(errp, "cluster-id is not supported");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> if (props->has_socket_id && !slot->props.has_socket_id) {
>>>>> error_setg(errp, "socket-id is not supported");
>>>>> return;
>>>>> @@ -696,6 +701,11 @@ void machine_set_cpu_numa_node(MachineState
>>>>> *machine,
>>>>> continue;
>>>>> }
>>>>> + if (props->has_cluster_id &&
>>>>> + props->cluster_id != slot->props.cluster_id) {
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> if (props->has_die_id && props->die_id !=
>>>>> slot->props.die_id) {
>>>>> continue;
>>>>> }
>>>>> @@ -990,6 +1000,12 @@ static char *cpu_slot_to_string(const
>>>>> CPUArchId *cpu)
>>>>> }
>>>>> g_string_append_printf(s, "die-id: %"PRId64,
>>>>> cpu->props.die_id);
>>>>> }
>>>>> + if (cpu->props.has_cluster_id) {
>>>>> + if (s->len) {
>>>>> + g_string_append_printf(s, ", ");
>>>>> + }
>>>>> + g_string_append_printf(s, "cluster-id: %"PRId64,
>>>>> cpu->props.cluster_id);
>>>>> + }
>>>>> if (cpu->props.has_core_id) {
>>>>> if (s->len) {
>>>>> g_string_append_printf(s, ", ");
>>>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>>>> index 9c460ec450..ea22b574b0 100644
>>>>> --- a/qapi/machine.json
>>>>> +++ b/qapi/machine.json
>>>>> @@ -868,10 +868,11 @@
>>>>> # @node-id: NUMA node ID the CPU belongs to
>>>>> # @socket-id: socket number within node/board the CPU belongs to
>>>>> # @die-id: die number within socket the CPU belongs to (since 4.1)
>>>>> -# @core-id: core number within die the CPU belongs to
>>>>> +# @cluster-id: cluster number within die the CPU belongs to
>> We also need a "(since 7.1)" tag for cluster-id.
>>>> I remember this should be "cluster number within socket..."
>>>> according to Igor's comments in v3 ?
>>>
>>> Igor had suggestion to correct the description for 'core-id' like
>>> below, but he didn't suggest anything for 'cluster-id'. The question
>>> is clusters are sub-components of die, instead of socket, if die
>>> is supported? You may want to me change it like below and please
>>> confirm.
>>>
>>> @cluster-id: cluster number within die/socket the CPU belongs to
>>>
>>> suggestion from Ignor in v3:
>>>
>>> > +# @core-id: core number within cluster the CPU belongs to
>>>
>>> s:cluster:cluster/die:
>>>
>> We want "within cluster/die" description for core-id because we
>> support both "cores in cluster" for ARM and "cores in die" for X86.
>> Base on this routine, we only need "within socket" for cluster-id
>> because we currently only support "clusters in socket". Does this
>> make sense?
>>
>> Alternatively, the plainest documentation for the IDs is to simply
>> range **-id only to its next level topo like below. This may avoid
>> increasing complexity when more topo-ids are inserted middle.
>> But whether this way is acceptable is up to the Maintainers. :)
>
> Rather saying we only support cluster on ARM and only dies on x86,
> I tend to view it as, we only support greater than 1 cluster on
> ARM, and greater than 1 die on x86.
>
> IOW, x86 implicitly always has exactly 1-and-only-1 cluster,
> and arm implicitly always has exactly 1-and-only-1 die.
>
> With this POV, then we can keep the description simple, only
> refering to the immediately above level in the hierarchy.
>
Agreed and thanks a lot for the elaboration.
>>
>> # @socket-id: socket number within node/board the CPU belongs to
>> # @die-id: die number within socket the CPU belongs to (since 4.1)
>> # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
>> # @core-id: core number within cluster the CPU belongs to
>> # @thread-id: thread number within core the CPU belongs to
>
> So this suggested text is fine with me.
>
Ok. The description will be included into v7, as v6 was posted
two days ago.
Thanks,
Gavin
Hi Yanan,
On 4/14/22 10:27 AM, wangyanan (Y) wrote:
> On 2022/4/14 8:06, Gavin Shan wrote:
>> On 4/13/22 7:49 PM, wangyanan (Y) wrote:
>>> On 2022/4/3 22:59, Gavin Shan wrote:
>>>> This adds cluster-id in CPU instance properties, which will be used
>>>> by arm/virt machine. Besides, the cluster-id is also verified or
>>>> dumped in various spots:
>>>>
>>>> * hw/core/machine.c::machine_set_cpu_numa_node() to associate
>>>> CPU with its NUMA node.
>>>>
>>>> * hw/core/machine.c::machine_numa_finish_cpu_init() to associate
>>>> CPU with NUMA node when no default association isn't provided.
>>>>
>>>> * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
>>>> cluster-id.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>> hw/core/machine-hmp-cmds.c | 4 ++++
>>>> hw/core/machine.c | 16 ++++++++++++++++
>>>> qapi/machine.json | 6 ++++--
>>>> 3 files changed, 24 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
>>>> index 4e2f319aeb..5cb5eecbfc 100644
>>>> --- a/hw/core/machine-hmp-cmds.c
>>>> +++ b/hw/core/machine-hmp-cmds.c
>>>> @@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
>>>> if (c->has_die_id) {
>>>> monitor_printf(mon, " die-id: \"%" PRIu64 "\"\n", c->die_id);
>>>> }
>>>> + if (c->has_cluster_id) {
>>>> + monitor_printf(mon, " cluster-id: \"%" PRIu64 "\"\n",
>>>> + c->cluster_id);
>>>> + }
>>>> if (c->has_core_id) {
>>>> monitor_printf(mon, " core-id: \"%" PRIu64 "\"\n", c->core_id);
>>>> }
>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>> index d856485cb4..8748b64657 100644
>>>> --- a/hw/core/machine.c
>>>> +++ b/hw/core/machine.c
>>>> @@ -677,6 +677,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>>> return;
>>>> }
>>>> + if (props->has_cluster_id && !slot->props.has_cluster_id) {
>>>> + error_setg(errp, "cluster-id is not supported");
>>>> + return;
>>>> + }
>>>> +
>>>> if (props->has_socket_id && !slot->props.has_socket_id) {
>>>> error_setg(errp, "socket-id is not supported");
>>>> return;
>>>> @@ -696,6 +701,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>>> continue;
>>>> }
>>>> + if (props->has_cluster_id &&
>>>> + props->cluster_id != slot->props.cluster_id) {
>>>> + continue;
>>>> + }
>>>> +
>>>> if (props->has_die_id && props->die_id != slot->props.die_id) {
>>>> continue;
>>>> }
>>>> @@ -990,6 +1000,12 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
>>>> }
>>>> g_string_append_printf(s, "die-id: %"PRId64, cpu->props.die_id);
>>>> }
>>>> + if (cpu->props.has_cluster_id) {
>>>> + if (s->len) {
>>>> + g_string_append_printf(s, ", ");
>>>> + }
>>>> + g_string_append_printf(s, "cluster-id: %"PRId64, cpu->props.cluster_id);
>>>> + }
>>>> if (cpu->props.has_core_id) {
>>>> if (s->len) {
>>>> g_string_append_printf(s, ", ");
>>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>>> index 9c460ec450..ea22b574b0 100644
>>>> --- a/qapi/machine.json
>>>> +++ b/qapi/machine.json
>>>> @@ -868,10 +868,11 @@
>>>> # @node-id: NUMA node ID the CPU belongs to
>>>> # @socket-id: socket number within node/board the CPU belongs to
>>>> # @die-id: die number within socket the CPU belongs to (since 4.1)
>>>> -# @core-id: core number within die the CPU belongs to
>>>> +# @cluster-id: cluster number within die the CPU belongs to
> We also need a "(since 7.1)" tag for cluster-id.
>>> I remember this should be "cluster number within socket..."
>>> according to Igor's comments in v3 ?
>>
>> Igor had suggestion to correct the description for 'core-id' like
>> below, but he didn't suggest anything for 'cluster-id'. The question
>> is clusters are sub-components of die, instead of socket, if die
>> is supported? You may want to me change it like below and please
>> confirm.
>>
>> @cluster-id: cluster number within die/socket the CPU belongs to
>>
>> suggestion from Ignor in v3:
>>
>> > +# @core-id: core number within cluster the CPU belongs to
>>
>> s:cluster:cluster/die:
>>
> We want "within cluster/die" description for core-id because we
> support both "cores in cluster" for ARM and "cores in die" for X86.
> Base on this routine, we only need "within socket" for cluster-id
> because we currently only support "clusters in socket". Does this
> make sense?
>
Thanks for the explanation. So ARM64 doesn't have die and x86 doesn't
have cluster? If so, I need to adjust the description for 'cluster-id'
as you suggested in v6:
@cluster-id: cluster number within socket the CPU belongs to (since 7.1)
> Alternatively, the plainest documentation for the IDs is to simply
> range **-id only to its next level topo like below. This may avoid
> increasing complexity when more topo-ids are inserted middle.
> But whether this way is acceptable is up to the Maintainers. :)
>
> # @socket-id: socket number within node/board the CPU belongs to
> # @die-id: die number within socket the CPU belongs to (since 4.1)
> # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
> # @core-id: core number within cluster the CPU belongs to
> # @thread-id: thread number within core the CPU belongs to
>
I like this scheme, but needs the confirms from Igor and maintainers.
Igor and maintainers, please let us know if the scheme is good to
have? :)
>>
>>>> +# @core-id: core number within cluster/die the CPU belongs to
>>>> # @thread-id: thread number within core the CPU belongs to
>>>> #
>>>> -# Note: currently there are 5 properties that could be present
>>>> +# Note: currently there are 6 properties that could be present
>>>> # but management should be prepared to pass through other
>>>> # properties with device_add command to allow for future
>>>> # interface extension. This also requires the filed names to be kept in
>>>> @@ -883,6 +884,7 @@
>>>> 'data': { '*node-id': 'int',
>>>> '*socket-id': 'int',
>>>> '*die-id': 'int',
>>>> + '*cluster-id': 'int',
>>>> '*core-id': 'int',
>>>> '*thread-id': 'int'
>>>> }
>>> Otherwise, looks good to me:
>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>
>>> Please also keep the involved Maintainers on Cc list in next version,
>>> an Ack from them is best. :)
>>>
>>
>> Thanks again for your time to review. Sure, I will do in next posting.
>>
Thanks,
Gavin
On 2022/4/14 15:56, Gavin Shan wrote:
> Hi Yanan,
>
> On 4/14/22 10:27 AM, wangyanan (Y) wrote:
>> On 2022/4/14 8:06, Gavin Shan wrote:
>>> On 4/13/22 7:49 PM, wangyanan (Y) wrote:
>>>> On 2022/4/3 22:59, Gavin Shan wrote:
>>>>> This adds cluster-id in CPU instance properties, which will be used
>>>>> by arm/virt machine. Besides, the cluster-id is also verified or
>>>>> dumped in various spots:
>>>>>
>>>>> * hw/core/machine.c::machine_set_cpu_numa_node() to associate
>>>>> CPU with its NUMA node.
>>>>>
>>>>> * hw/core/machine.c::machine_numa_finish_cpu_init() to associate
>>>>> CPU with NUMA node when no default association isn't provided.
>>>>>
>>>>> * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
>>>>> cluster-id.
>>>>>
>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>> ---
>>>>> hw/core/machine-hmp-cmds.c | 4 ++++
>>>>> hw/core/machine.c | 16 ++++++++++++++++
>>>>> qapi/machine.json | 6 ++++--
>>>>> 3 files changed, 24 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
>>>>> index 4e2f319aeb..5cb5eecbfc 100644
>>>>> --- a/hw/core/machine-hmp-cmds.c
>>>>> +++ b/hw/core/machine-hmp-cmds.c
>>>>> @@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon, const
>>>>> QDict *qdict)
>>>>> if (c->has_die_id) {
>>>>> monitor_printf(mon, " die-id: \"%" PRIu64 "\"\n",
>>>>> c->die_id);
>>>>> }
>>>>> + if (c->has_cluster_id) {
>>>>> + monitor_printf(mon, " cluster-id: \"%" PRIu64 "\"\n",
>>>>> + c->cluster_id);
>>>>> + }
>>>>> if (c->has_core_id) {
>>>>> monitor_printf(mon, " core-id: \"%" PRIu64
>>>>> "\"\n", c->core_id);
>>>>> }
>>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>>> index d856485cb4..8748b64657 100644
>>>>> --- a/hw/core/machine.c
>>>>> +++ b/hw/core/machine.c
>>>>> @@ -677,6 +677,11 @@ void machine_set_cpu_numa_node(MachineState
>>>>> *machine,
>>>>> return;
>>>>> }
>>>>> + if (props->has_cluster_id && !slot->props.has_cluster_id) {
>>>>> + error_setg(errp, "cluster-id is not supported");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> if (props->has_socket_id && !slot->props.has_socket_id) {
>>>>> error_setg(errp, "socket-id is not supported");
>>>>> return;
>>>>> @@ -696,6 +701,11 @@ void machine_set_cpu_numa_node(MachineState
>>>>> *machine,
>>>>> continue;
>>>>> }
>>>>> + if (props->has_cluster_id &&
>>>>> + props->cluster_id != slot->props.cluster_id) {
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> if (props->has_die_id && props->die_id !=
>>>>> slot->props.die_id) {
>>>>> continue;
>>>>> }
>>>>> @@ -990,6 +1000,12 @@ static char *cpu_slot_to_string(const
>>>>> CPUArchId *cpu)
>>>>> }
>>>>> g_string_append_printf(s, "die-id: %"PRId64,
>>>>> cpu->props.die_id);
>>>>> }
>>>>> + if (cpu->props.has_cluster_id) {
>>>>> + if (s->len) {
>>>>> + g_string_append_printf(s, ", ");
>>>>> + }
>>>>> + g_string_append_printf(s, "cluster-id: %"PRId64,
>>>>> cpu->props.cluster_id);
>>>>> + }
>>>>> if (cpu->props.has_core_id) {
>>>>> if (s->len) {
>>>>> g_string_append_printf(s, ", ");
>>>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>>>> index 9c460ec450..ea22b574b0 100644
>>>>> --- a/qapi/machine.json
>>>>> +++ b/qapi/machine.json
>>>>> @@ -868,10 +868,11 @@
>>>>> # @node-id: NUMA node ID the CPU belongs to
>>>>> # @socket-id: socket number within node/board the CPU belongs to
>>>>> # @die-id: die number within socket the CPU belongs to (since 4.1)
>>>>> -# @core-id: core number within die the CPU belongs to
>>>>> +# @cluster-id: cluster number within die the CPU belongs to
>> We also need a "(since 7.1)" tag for cluster-id.
>>>> I remember this should be "cluster number within socket..."
>>>> according to Igor's comments in v3 ?
>>>
>>> Igor had suggestion to correct the description for 'core-id' like
>>> below, but he didn't suggest anything for 'cluster-id'. The question
>>> is clusters are sub-components of die, instead of socket, if die
>>> is supported? You may want to me change it like below and please
>>> confirm.
>>>
>>> @cluster-id: cluster number within die/socket the CPU belongs to
>>>
>>> suggestion from Ignor in v3:
>>>
>>> > +# @core-id: core number within cluster the CPU belongs to
>>>
>>> s:cluster:cluster/die:
>>>
>> We want "within cluster/die" description for core-id because we
>> support both "cores in cluster" for ARM and "cores in die" for X86.
>> Base on this routine, we only need "within socket" for cluster-id
>> because we currently only support "clusters in socket". Does this
>> make sense?
>>
>
> Thanks for the explanation. So ARM64 doesn't have die and x86 doesn't
> have cluster?
At least for now, yes. :)
Thanks,
Yanan
> If so, I need to adjust the description for 'cluster-id'
> as you suggested in v6:
>
> @cluster-id: cluster number within socket the CPU belongs to (since
> 7.1)
>> Alternatively, the plainest documentation for the IDs is to simply
>> range **-id only to its next level topo like below. This may avoid
>> increasing complexity when more topo-ids are inserted middle.
>> But whether this way is acceptable is up to the Maintainers. :)
>>
>> # @socket-id: socket number within node/board the CPU belongs to
>> # @die-id: die number within socket the CPU belongs to (since 4.1)
>> # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
>> # @core-id: core number within cluster the CPU belongs to
>> # @thread-id: thread number within core the CPU belongs to
>>
>
> I like this scheme, but needs the confirms from Igor and maintainers.
> Igor and maintainers, please let us know if the scheme is good to
> have? :)
>
>>>
>>>>> +# @core-id: core number within cluster/die the CPU belongs to
>>>>> # @thread-id: thread number within core the CPU belongs to
>>>>> #
>>>>> -# Note: currently there are 5 properties that could be present
>>>>> +# Note: currently there are 6 properties that could be present
>>>>> # but management should be prepared to pass through other
>>>>> # properties with device_add command to allow for future
>>>>> # interface extension. This also requires the filed names
>>>>> to be kept in
>>>>> @@ -883,6 +884,7 @@
>>>>> 'data': { '*node-id': 'int',
>>>>> '*socket-id': 'int',
>>>>> '*die-id': 'int',
>>>>> + '*cluster-id': 'int',
>>>>> '*core-id': 'int',
>>>>> '*thread-id': 'int'
>>>>> }
>>>> Otherwise, looks good to me:
>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>>>>
>>>> Please also keep the involved Maintainers on Cc list in next version,
>>>> an Ack from them is best. :)
>>>>
>>>
>>> Thanks again for your time to review. Sure, I will do in next posting.
>>>
>
> Thanks,
> Gavin
>
> .
On Sun, Apr 03, 2022 at 10:59:50PM +0800, Gavin Shan wrote:
> This adds cluster-id in CPU instance properties, which will be used
> by arm/virt machine. Besides, the cluster-id is also verified or
> dumped in various spots:
>
> * hw/core/machine.c::machine_set_cpu_numa_node() to associate
> CPU with its NUMA node.
>
> * hw/core/machine.c::machine_numa_finish_cpu_init() to associate
> CPU with NUMA node when no default association isn't provided.
>
> * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
> cluster-id.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> hw/core/machine-hmp-cmds.c | 4 ++++
> hw/core/machine.c | 16 ++++++++++++++++
> qapi/machine.json | 6 ++++--
> 3 files changed, 24 insertions(+), 2 deletions(-)
Missing changes to hw/core/machine-smp.c similar to 'dies' in that
file.
When 'dies' was added we added a 'dies_supported' flag, so we could
reject use of 'dies' when it was not supported - which is everywhere
except i386 target.
We need the same for 'clusters_supported' machine property since
AFAICT only the arm 'virt' machine is getting supported in this
series.
>
> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
> index 4e2f319aeb..5cb5eecbfc 100644
> --- a/hw/core/machine-hmp-cmds.c
> +++ b/hw/core/machine-hmp-cmds.c
> @@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
> if (c->has_die_id) {
> monitor_printf(mon, " die-id: \"%" PRIu64 "\"\n", c->die_id);
> }
> + if (c->has_cluster_id) {
> + monitor_printf(mon, " cluster-id: \"%" PRIu64 "\"\n",
> + c->cluster_id);
> + }
> if (c->has_core_id) {
> monitor_printf(mon, " core-id: \"%" PRIu64 "\"\n", c->core_id);
> }
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d856485cb4..8748b64657 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -677,6 +677,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
> return;
> }
>
> + if (props->has_cluster_id && !slot->props.has_cluster_id) {
> + error_setg(errp, "cluster-id is not supported");
> + return;
> + }
> +
> if (props->has_socket_id && !slot->props.has_socket_id) {
> error_setg(errp, "socket-id is not supported");
> return;
> @@ -696,6 +701,11 @@ void machine_set_cpu_numa_node(MachineState *machine,
> continue;
> }
>
> + if (props->has_cluster_id &&
> + props->cluster_id != slot->props.cluster_id) {
> + continue;
> + }
> +
> if (props->has_die_id && props->die_id != slot->props.die_id) {
> continue;
> }
> @@ -990,6 +1000,12 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
> }
> g_string_append_printf(s, "die-id: %"PRId64, cpu->props.die_id);
> }
> + if (cpu->props.has_cluster_id) {
> + if (s->len) {
> + g_string_append_printf(s, ", ");
> + }
> + g_string_append_printf(s, "cluster-id: %"PRId64, cpu->props.cluster_id);
> + }
> if (cpu->props.has_core_id) {
> if (s->len) {
> g_string_append_printf(s, ", ");
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 9c460ec450..ea22b574b0 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -868,10 +868,11 @@
> # @node-id: NUMA node ID the CPU belongs to
> # @socket-id: socket number within node/board the CPU belongs to
> # @die-id: die number within socket the CPU belongs to (since 4.1)
> -# @core-id: core number within die the CPU belongs to
> +# @cluster-id: cluster number within die the CPU belongs to
> +# @core-id: core number within cluster/die the CPU belongs to
> # @thread-id: thread number within core the CPU belongs to
> #
> -# Note: currently there are 5 properties that could be present
> +# Note: currently there are 6 properties that could be present
> # but management should be prepared to pass through other
> # properties with device_add command to allow for future
> # interface extension. This also requires the filed names to be kept in
> @@ -883,6 +884,7 @@
> 'data': { '*node-id': 'int',
> '*socket-id': 'int',
> '*die-id': 'int',
> + '*cluster-id': 'int',
> '*core-id': 'int',
> '*thread-id': 'int'
> }
> --
> 2.23.0
>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Apr 04, 2022 at 09:37:10AM +0100, Daniel P. Berrangé wrote: > On Sun, Apr 03, 2022 at 10:59:50PM +0800, Gavin Shan wrote: > > This adds cluster-id in CPU instance properties, which will be used > > by arm/virt machine. Besides, the cluster-id is also verified or > > dumped in various spots: > > > > * hw/core/machine.c::machine_set_cpu_numa_node() to associate > > CPU with its NUMA node. > > > > * hw/core/machine.c::machine_numa_finish_cpu_init() to associate > > CPU with NUMA node when no default association isn't provided. > > > > * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump > > cluster-id. > > > > Signed-off-by: Gavin Shan <gshan@redhat.com> > > --- > > hw/core/machine-hmp-cmds.c | 4 ++++ > > hw/core/machine.c | 16 ++++++++++++++++ > > qapi/machine.json | 6 ++++-- > > 3 files changed, 24 insertions(+), 2 deletions(-) > > Missing changes to hw/core/machine-smp.c similar to 'dies' in that > file. > > When 'dies' was added we added a 'dies_supported' flag, so we could > reject use of 'dies' when it was not supported - which is everywhere > except i386 target. > > We need the same for 'clusters_supported' machine property since > AFAICT only the arm 'virt' machine is getting supported in this > series. Oh, actually I'm mixing up cluster-id and clusters - the latter is already supported. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hi Daniel, On 4/4/22 4:40 PM, Daniel P. Berrangé wrote: > On Mon, Apr 04, 2022 at 09:37:10AM +0100, Daniel P. Berrangé wrote: >> On Sun, Apr 03, 2022 at 10:59:50PM +0800, Gavin Shan wrote: >>> This adds cluster-id in CPU instance properties, which will be used >>> by arm/virt machine. Besides, the cluster-id is also verified or >>> dumped in various spots: >>> >>> * hw/core/machine.c::machine_set_cpu_numa_node() to associate >>> CPU with its NUMA node. >>> >>> * hw/core/machine.c::machine_numa_finish_cpu_init() to associate >>> CPU with NUMA node when no default association isn't provided. >>> >>> * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump >>> cluster-id. >>> >>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>> --- >>> hw/core/machine-hmp-cmds.c | 4 ++++ >>> hw/core/machine.c | 16 ++++++++++++++++ >>> qapi/machine.json | 6 ++++-- >>> 3 files changed, 24 insertions(+), 2 deletions(-) >> >> Missing changes to hw/core/machine-smp.c similar to 'dies' in that >> file. >> >> When 'dies' was added we added a 'dies_supported' flag, so we could >> reject use of 'dies' when it was not supported - which is everywhere >> except i386 target. >> >> We need the same for 'clusters_supported' machine property since >> AFAICT only the arm 'virt' machine is getting supported in this >> series. > > Oh, actually I'm mixing up cluster-id and clusters - the latter is > already supported. > Yeah, @clusters_supported has been existing for a while. Thanks, Gavin
© 2016 - 2026 Red Hat, Inc.