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

[Andrew Jones]

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.

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