Re: [PATCH for-6.2 v2 10/11] machine: Split out the smp parsing code

["wangyanan (Y)" <wangyanan55@huawei.com>]

On 2021/7/20 1:20, Andrew Jones wrote:
> On Mon, Jul 19, 2021 at 11:20:42AM +0800, Yanan Wang wrote:
>> We are going to introduce an unit test for the parser smp_parse()
>> in hw/core/machine.c, but now machine.c is only built in softmmu.
>>
>> In order to solve the build dependency on the smp parsing code and
>> avoid building unrelated stuff for the unit tests, move the related
>> code from machine.c into a new common file, i.e., machine-smp.c.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   MAINTAINERS           |   1 +
>>   hw/core/machine-smp.c | 124 ++++++++++++++++++++++++++++++++++++++++++
>>   hw/core/machine.c     | 109 -------------------------------------
>>   hw/core/meson.build   |   1 +
>>   include/hw/boards.h   |   1 +
>>   5 files changed, 127 insertions(+), 109 deletions(-)
>>   create mode 100644 hw/core/machine-smp.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9100f9a043..70633e3bf4 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1626,6 +1626,7 @@ F: cpu.c
>>   F: hw/core/cpu.c
>>   F: hw/core/machine-qmp-cmds.c
>>   F: hw/core/machine.c
>> +F: hw/core/machine-smp.c
>>   F: hw/core/null-machine.c
>>   F: hw/core/numa.c
>>   F: hw/cpu/cluster.c
>> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
>> new file mode 100644
>> index 0000000000..6a00cfe44a
>> --- /dev/null
>> +++ b/hw/core/machine-smp.c
>> @@ -0,0 +1,124 @@
>> +/*
>> + * QEMU Machine (related to SMP configuration)
>> + *
>> + * Copyright (C) 2014 Red Hat Inc
>> + *
>> + * Authors:
>> + *   Marcel Apfelbaum <marcel.a@redhat.com>
> This header was obviously copy+pasted without being updated.
Yes, the header was kept unchanged.

But actually I'm not completely sure which field should be updated. :)
Should I add "Copyright (C) 2021, Huawei, Inc." and also the authorship
"Yanan Wang <wangyanan55@huawei.com>" behind the existing ones
or just replace them?
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/boards.h"
>> +#include "qapi/error.h"
>> +
>> +/*
>> + * smp_parse - Generic function used to parse the given SMP configuration
>> + *
>> + * The topology parameters must be specified equal to or great than one
>> + * or just omitted, explicit configuration like "cpus=0" is not allowed.
>> + * The omitted parameters will be calculated based on the provided ones.
>> + *
>> + * maxcpus will default to the value of cpus if omitted and will be used
>> + * to compute the missing sockets/cores/threads. cpus will be calculated
>> + * from the computed parametrs if omitted.
>> + *
>> + * In calculation of omitted arch-netural sockets/cores/threads, we prefer
>> + * sockets over cores over threads before 6.2, while prefer cores over
>> + * sockets over threads since 6.2 on. The arch-specific dies will directly
>> + * default to 1 if omitted.
>> + */
>> +void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>> +{
>> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>> +    unsigned cpus    = config->has_cpus ? config->cpus : 0;
>> +    unsigned sockets = config->has_sockets ? config->sockets : 0;
>> +    unsigned dies    = config->has_dies ? config->dies : 1;
>> +    unsigned cores   = config->has_cores ? config->cores : 0;
>> +    unsigned threads = config->has_threads ? config->threads : 0;
>> +    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>> +
>> +    if ((config->has_cpus && config->cpus == 0) ||
>> +        (config->has_sockets && config->sockets == 0) ||
>> +        (config->has_dies && config->dies == 0) ||
>> +        (config->has_cores && config->cores == 0) ||
>> +        (config->has_threads && config->threads == 0) ||
>> +        (config->has_maxcpus && config->maxcpus == 0)) {
>> +        error_setg(errp, "parameters must be equal to or greater than one"
>> +                   "if provided");
>> +        return;
>> +    }
>> +
>> +    if (!mc->smp_dies_supported && dies > 1) {
>> +        error_setg(errp, "dies not supported by this machine's CPU topology");
>> +        return;
>> +    }
>> +
>> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> +
>> +    /* prefer sockets over cores over threads before 6.2 */
>> +    if (mc->smp_prefer_sockets) {
>> +        if (sockets == 0) {
>> +            cores = cores > 0 ? cores : 1;
>> +            threads = threads > 0 ? threads : 1;
>> +            sockets = maxcpus / (dies * cores * threads);
>> +            sockets = sockets > 0 ? sockets : 1;
>> +        } else if (cores == 0) {
>> +            threads = threads > 0 ? threads : 1;
>> +            cores = maxcpus / (sockets * dies * threads);
>> +            cores = cores > 0 ? cores : 1;
>> +        } else if (threads == 0) {
>> +            threads = maxcpus / (sockets * dies * cores);
>> +            threads = threads > 0 ? threads : 1;
>> +        }
>> +    /* prefer cores over sockets over threads since 6.2 */
>> +    } else {
>> +        if (cores == 0) {
>> +            sockets = sockets > 0 ? sockets : 1;
>> +            threads = threads > 0 ? threads : 1;
>> +            cores = maxcpus / (sockets * dies * threads);
>> +            cores = cores > 0 ? cores : 1;
>> +        } else if (sockets == 0) {
>> +            threads = threads > 0 ? threads : 1;
>> +            sockets = maxcpus / (dies * cores * threads);
>> +            sockets = sockets > 0 ? sockets : 1;
>> +        } else if (threads == 0) {
>> +            threads = maxcpus / (sockets * dies * cores);
>> +            threads = threads > 0 ? threads : 1;
>> +        }
>> +    }
>> +
>> +    /* use the computed parameters to calculate the omitted cpus */
>> +    cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
>> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> +
>> +    if (sockets * dies * cores * threads != maxcpus) {
>> +        g_autofree char *dies_msg = g_strdup_printf(
>> +            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
>> +        error_setg(errp, "Invalid CPU topology: "
>> +                   "sockets (%u)%s * cores (%u) * threads (%u) "
>> +                   "!= maxcpus (%u)",
>> +                   sockets, dies_msg, cores, threads,
>> +                   maxcpus);
>> +        return;
>> +    }
>> +
>> +    if (sockets * dies * cores * threads < cpus) {
>> +        g_autofree char *dies_msg = g_strdup_printf(
>> +            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
>> +        error_setg(errp, "Invalid CPU topology: "
>> +                   "sockets (%u)%s * cores (%u) * threads (%u) < "
>> +                   "smp_cpus (%u)",
>> +                   sockets, dies_msg, cores, threads, cpus);
>> +        return;
>> +    }
>> +
>> +    ms->smp.cpus = cpus;
>> +    ms->smp.sockets = sockets;
>> +    ms->smp.dies = dies;
>> +    ms->smp.cores = cores;
>> +    ms->smp.threads = threads;
>> +    ms->smp.max_cpus = maxcpus;
>> +}
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 9d24b67ef3..61be266b6c 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -744,115 +744,6 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>       }
>>   }
>>   
>> -/*
>> - * smp_parse - Generic function used to parse the given SMP configuration
>> - *
>> - * The topology parameters must be specified equal to or great than one
>> - * or just omitted, explicit configuration like "cpus=0" is not allowed.
>> - * The omitted parameters will be calculated based on the provided ones.
>> - *
>> - * maxcpus will default to the value of cpus if omitted and will be used
>> - * to compute the missing sockets/cores/threads. cpus will be calculated
>> - * from the computed parametrs if omitted.
>> - *
>> - * In calculation of omitted arch-netural sockets/cores/threads, we prefer
>> - * sockets over cores over threads before 6.2, while prefer cores over
>> - * sockets over threads since 6.2 on. The arch-specific dies will directly
>> - * default to 1 if omitted.
>> - */
>> -static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>> -{
>> -    MachineClass *mc = MACHINE_GET_CLASS(ms);
>> -    unsigned cpus    = config->has_cpus ? config->cpus : 0;
>> -    unsigned sockets = config->has_sockets ? config->sockets : 0;
>> -    unsigned dies    = config->has_dies ? config->dies : 1;
>> -    unsigned cores   = config->has_cores ? config->cores : 0;
>> -    unsigned threads = config->has_threads ? config->threads : 0;
>> -    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>> -
>> -    if ((config->has_cpus && config->cpus == 0) ||
>> -        (config->has_sockets && config->sockets == 0) ||
>> -        (config->has_dies && config->dies == 0) ||
>> -        (config->has_cores && config->cores == 0) ||
>> -        (config->has_threads && config->threads == 0) ||
>> -        (config->has_maxcpus && config->maxcpus == 0)) {
>> -        error_setg(errp, "parameters must be equal to or greater than one"
>> -                   "if provided");
>> -        return;
>> -    }
>> -
>> -    if (!mc->smp_dies_supported && dies > 1) {
>> -        error_setg(errp, "dies not supported by this machine's CPU topology");
>> -        return;
>> -    }
>> -
>> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> -
>> -    /* prefer sockets over cores over threads before 6.2 */
>> -    if (mc->smp_prefer_sockets) {
>> -        if (sockets == 0) {
>> -            cores = cores > 0 ? cores : 1;
>> -            threads = threads > 0 ? threads : 1;
>> -            sockets = maxcpus / (dies * cores * threads);
>> -            sockets = sockets > 0 ? sockets : 1;
>> -        } else if (cores == 0) {
>> -            threads = threads > 0 ? threads : 1;
>> -            cores = maxcpus / (sockets * dies * threads);
>> -            cores = cores > 0 ? cores : 1;
>> -        } else if (threads == 0) {
>> -            threads = maxcpus / (sockets * dies * cores);
>> -            threads = threads > 0 ? threads : 1;
>> -        }
>> -    /* prefer cores over sockets over threads since 6.2 */
>> -    } else {
>> -        if (cores == 0) {
>> -            sockets = sockets > 0 ? sockets : 1;
>> -            threads = threads > 0 ? threads : 1;
>> -            cores = maxcpus / (sockets * dies * threads);
>> -            cores = cores > 0 ? cores : 1;
>> -        } else if (sockets == 0) {
>> -            threads = threads > 0 ? threads : 1;
>> -            sockets = maxcpus / (dies * cores * threads);
>> -            sockets = sockets > 0 ? sockets : 1;
>> -        } else if (threads == 0) {
>> -            threads = maxcpus / (sockets * dies * cores);
>> -            threads = threads > 0 ? threads : 1;
>> -        }
>> -    }
>> -
>> -    /* use the computed parameters to calculate the omitted cpus */
>> -    cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
>> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> -
>> -    if (sockets * dies * cores * threads != maxcpus) {
>> -        g_autofree char *dies_msg = g_strdup_printf(
>> -            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
>> -        error_setg(errp, "Invalid CPU topology: "
>> -                   "sockets (%u)%s * cores (%u) * threads (%u) "
>> -                   "!= maxcpus (%u)",
>> -                   sockets, dies_msg, cores, threads,
>> -                   maxcpus);
>> -        return;
>> -    }
>> -
>> -    if (sockets * dies * cores * threads < cpus) {
>> -        g_autofree char *dies_msg = g_strdup_printf(
>> -            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
>> -        error_setg(errp, "Invalid CPU topology: "
>> -                   "sockets (%u)%s * cores (%u) * threads (%u) < "
>> -                   "smp_cpus (%u)",
>> -                   sockets, dies_msg, cores, threads, cpus);
>> -        return;
>> -    }
>> -
>> -    ms->smp.cpus = cpus;
>> -    ms->smp.sockets = sockets;
>> -    ms->smp.dies = dies;
>> -    ms->smp.cores = cores;
>> -    ms->smp.threads = threads;
>> -    ms->smp.max_cpus = maxcpus;
>> -}
>> -
>>   static void machine_get_smp(Object *obj, Visitor *v, const char *name,
>>                               void *opaque, Error **errp)
>>   {
>> diff --git a/hw/core/meson.build b/hw/core/meson.build
>> index 18f44fb7c2..6d727c7742 100644
>> --- a/hw/core/meson.build
>> +++ b/hw/core/meson.build
>> @@ -14,6 +14,7 @@ hwcore_files = files(
>>   )
>>   
>>   common_ss.add(files('cpu-common.c'))
>> +common_ss.add(files('machine-smp.c'))
>>   common_ss.add(when: 'CONFIG_FITLOADER', if_true: files('loader-fit.c'))
>>   common_ss.add(when: 'CONFIG_GENERIC_LOADER', if_true: files('generic-loader.c'))
>>   common_ss.add(when: ['CONFIG_GUEST_LOADER', fdt], if_true: files('guest-loader.c'))
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 12ab0f5968..071eec1e74 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -34,6 +34,7 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
>>   void machine_set_cpu_numa_node(MachineState *machine,
>>                                  const CpuInstanceProperties *props,
>>                                  Error **errp);
>> +void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp);
>>   
>>   /**
>>    * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
>> -- 
>> 2.19.1
>>
> Otherwise
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>
Thanks,
Yanan
.