On 2022/10/7 21:48, Michael S. Tsirkin wrote:
> On Thu, Sep 22, 2022 at 09:11:40PM +0800, Yicong Yang wrote:
>> From: Yicong Yang <yangyic...@hisilicon.com>
>>
>> Currently we'll always generate a cluster node no matter user has
>> specified '-smp clusters=X' or not. Cluster is an optional level
>> and it's unncessary to build it if user don't need. So only generate
>> it when user specify explicitly.
>>
>> Also update the test ACPI tables.
>>
>> Signed-off-by: Yicong Yang <yangyic...@hisilicon.com>
>
> This is an example of a commit log repeating what the patch does.
> Which is ok but the important thing is to explain the motivation -
> why is it a bug to generate a cluster node without '-smp clusters'?
>
It may not be a bug but may build the unneeded topology unconsciously
and doesn't provide a way to inhibit this. So I thought the policy
can be improved.
Thanks.
>
>> ---
>> hw/acpi/aml-build.c | 2 +-
>> hw/core/machine-smp.c | 3 +++
>> include/hw/boards.h | 2 ++
>> 3 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index e6bfac95c7..aab73af66d 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2030,7 +2030,7 @@ void build_pptt(GArray *table_data, BIOSLinker
>> *linker, MachineState *ms,
>> 0, socket_id, NULL, 0);
>> }
>>
>> - if (mc->smp_props.clusters_supported) {
>> + if (mc->smp_props.clusters_supported && ms->smp.build_cluster) {
>> if (cpus->cpus[n].props.cluster_id != cluster_id) {
>> assert(cpus->cpus[n].props.cluster_id > cluster_id);
>> cluster_id = cpus->cpus[n].props.cluster_id;
>> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
>> index b39ed21e65..5d37e8d07a 100644
>> --- a/hw/core/machine-smp.c
>> +++ b/hw/core/machine-smp.c
>> @@ -158,6 +158,9 @@ void machine_parse_smp_config(MachineState *ms,
>> ms->smp.threads = threads;
>> ms->smp.max_cpus = maxcpus;
>>
>> + if (config->has_clusters)
>> + ms->smp.build_cluster = true;
>> +
>> /* sanity-check of the computed topology */
>> if (sockets * dies * clusters * cores * threads != maxcpus) {
>> g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 7b416c9787..24aafc213d 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -305,6 +305,7 @@ typedef struct DeviceMemoryState {
>> * @cores: the number of cores in one cluster
>> * @threads: the number of threads in one core
>> * @max_cpus: the maximum number of logical processors on the machine
>> + * @build_cluster: build cluster topology or not
>> */
>> typedef struct CpuTopology {
>> unsigned int cpus;
>> @@ -314,6 +315,7 @@ typedef struct CpuTopology {
>> unsigned int cores;
>> unsigned int threads;
>> unsigned int max_cpus;
>> + bool build_cluster;
>> } CpuTopology;
>>
>> /**
>> --
>> 2.24.0
>
> .
>
