On Mon, Jul 19, 2021 at 11:20:34AM +0800, Yanan Wang wrote:
> Currently the only difference between smp_parse and pc_smp_parse
> is the support of multi-dies and the related error reporting code.
> With an arch compat variable "bool smp_dies_supported", we can
> easily make smp_parse generic enough for all arches and the PC
> specific one can be removed.
>
> Making smp_parse() generic enough can reduce code duplication and
> ease the code maintenance, and also allows extending the topology
> with more arch specific members (e.g., clusters) in the future.
>
> No functional change intended.
>
> Suggested-by: Andrew Jones <drjo...@redhat.com>
> Signed-off-by: Yanan Wang <wangyana...@huawei.com>
> ---
> hw/core/machine.c | 28 ++++++++++-------
> hw/i386/pc.c | 76 +--------------------------------------------
> include/hw/boards.h | 1 +
> 3 files changed, 19 insertions(+), 86 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d73daa10f4..ed6712e964 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -743,6 +743,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
>
> 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;
> @@ -761,7 +762,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration
> *config, Error **errp)
> return;
> }
>
> - if (dies > 1) {
> + if (!mc->smp_dies_supported && dies > 1) {
Won't this allow a user on an arch with !mc->smp_dies_supported to specify
dies=1? To not allow that, can we do
if (!mc->smp_dies_supported && config->has_dies)
instead?
> error_setg(errp, "dies not supported by this machine's CPU
> topology");
> return;
> }
> @@ -772,23 +773,25 @@ static void smp_parse(MachineState *ms,
> SMPConfiguration *config, Error **errp)
> threads = threads > 0 ? threads : 1;
> if (cpus == 0) {
> sockets = sockets > 0 ? sockets : 1;
> - cpus = cores * threads * sockets;
> + cpus = sockets * dies * cores * threads;
> } else {
> maxcpus = maxcpus > 0 ? maxcpus : cpus;
> - sockets = maxcpus / (cores * threads);
> + sockets = maxcpus / (dies * cores * threads);
> }
> } else if (cores == 0) {
> threads = threads > 0 ? threads : 1;
> - cores = cpus / (sockets * threads);
> + cores = cpus / (sockets * dies * threads);
> cores = cores > 0 ? cores : 1;
> } else if (threads == 0) {
> - threads = cpus / (cores * sockets);
> + threads = cpus / (sockets * dies * cores);
> threads = threads > 0 ? threads : 1;
> - } else if (sockets * cores * threads < cpus) {
> + } else if (sockets * dies * cores * threads < cpus) {
> + g_autofree char *dies_msg = g_strdup_printf(
> + mc->smp_dies_supported ? " * dies (%u)" : "", dies);
> error_setg(errp, "cpu topology: "
> - "sockets (%u) * cores (%u) * threads (%u) < "
> + "sockets (%u)%s * cores (%u) * threads (%u) < "
> "smp_cpus (%u)",
> - sockets, cores, threads, cpus);
> + sockets, dies_msg, cores, threads, cpus);
> return;
> }
>
> @@ -799,17 +802,20 @@ static void smp_parse(MachineState *ms,
> SMPConfiguration *config, Error **errp)
> return;
> }
>
> - if (sockets * cores * threads != maxcpus) {
> + 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) * cores (%u) * threads (%u) "
> + "sockets (%u)%s * cores (%u) * threads (%u) "
> "!= maxcpus (%u)",
> - sockets, cores, threads,
> + sockets, dies_msg, cores, threads,
> maxcpus);
> 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/i386/pc.c b/hw/i386/pc.c
> index c6b63c00a5..d94ef582b5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -708,80 +708,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> level)
> }
> }
>
> -/*
> - * This function is very similar to smp_parse()
> - * in hw/core/machine.c but includes CPU die support.
> - */
> -static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error
> **errp)
> -{
> - 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;
> - }
> -
> - /* compute missing values, prefer sockets over cores over threads */
> - if (cpus == 0 || sockets == 0) {
> - cores = cores > 0 ? cores : 1;
> - threads = threads > 0 ? threads : 1;
> - if (cpus == 0) {
> - sockets = sockets > 0 ? sockets : 1;
> - cpus = cores * threads * dies * sockets;
> - } else {
> - maxcpus = maxcpus > 0 ? maxcpus : cpus;
> - sockets = maxcpus / (cores * threads * dies);
> - }
> - } else if (cores == 0) {
> - threads = threads > 0 ? threads : 1;
> - cores = cpus / (sockets * dies * threads);
> - cores = cores > 0 ? cores : 1;
> - } else if (threads == 0) {
> - threads = cpus / (cores * dies * sockets);
> - threads = threads > 0 ? threads : 1;
> - } else if (sockets * dies * cores * threads < cpus) {
> - error_setg(errp, "cpu topology: "
> - "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
> - "smp_cpus (%u)",
> - sockets, dies, cores, threads, cpus);
> - return;
> - }
> -
> - maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
> - if (maxcpus < cpus) {
> - error_setg(errp, "maxcpus must be equal to or greater than smp");
> - return;
> - }
> -
> - if (sockets * dies * cores * threads != maxcpus) {
> - error_setg(errp, "Invalid CPU topology deprecated: "
> - "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
> - "!= maxcpus (%u)",
> - sockets, dies, cores, threads,
> - maxcpus);
> - 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 pc_machine_done(Notifier *notifier, void *data)
> {
> @@ -1735,7 +1661,6 @@ static void pc_machine_class_init(ObjectClass *oc, void
> *data)
> mc->auto_enable_numa_with_memdev = true;
> mc->has_hotpluggable_cpus = true;
> mc->default_boot_order = "cad";
> - mc->smp_parse = pc_smp_parse;
Can we also get rid of MachineClass::smp_parse now?
> mc->block_default_type = IF_IDE;
> mc->max_cpus = 255;
> mc->reset = pc_machine_reset;
> @@ -1746,6 +1671,7 @@ static void pc_machine_class_init(ObjectClass *oc, void
> *data)
> hc->unplug = pc_machine_device_unplug_cb;
> mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
> mc->nvdimm_supported = true;
> + mc->smp_dies_supported = true;
> mc->default_ram_id = "pc.ram";
>
> object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index accd6eff35..b6161cee88 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -246,6 +246,7 @@ struct MachineClass {
> bool smbus_no_migration_support;
> bool nvdimm_supported;
> bool numa_mem_supported;
> + bool smp_dies_supported;
> bool auto_enable_numa;
> const char *default_ram_id;
>
> --
> 2.19.1
>
Thanks,
drew
