| Message ID | 20210719032043.25416-3-wangyanan55@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | machine: smp parsing fixes and improvement | expand |
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 <drjones@redhat.com> > Signed-off-by: Yanan Wang <wangyanan55@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
On Mon, Jul 19, 2021 at 06:28:46PM +0200, Andrew Jones wrote: > 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 <drjones@redhat.com> > > Signed-off-by: Yanan Wang <wangyanan55@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? Conceptually that is fine. Before the introduction of CPU sockets with 2+ dies, you can credibly say that all sockets had 1 die, so it is nreasonable for users to say -smp ....,dies=1,.... libvirt will unconditionally set dies=1 for all QEMU targets if the user didn't specify an explicit dies value > To not allow that, can we do > > if (!mc->smp_dies_supported && config->has_dies) > > instead? I don't see that this is benefitting apps/users. Regards, Daniel
On Mon, Jul 19, 2021 at 05:36:39PM +0100, Daniel P. Berrangé wrote: > On Mon, Jul 19, 2021 at 06:28:46PM +0200, Andrew Jones wrote: > > 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 <drjones@redhat.com> > > > Signed-off-by: Yanan Wang <wangyanan55@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? > > Conceptually that is fine. Before the introduction of CPU sockets > with 2+ dies, you can credibly say that all sockets had 1 die, so > it is nreasonable for users to say -smp ....,dies=1,.... > > libvirt will unconditionally set dies=1 for all QEMU targets if > the user didn't specify an explicit dies value > > > To not allow that, can we do > > > > if (!mc->smp_dies_supported && config->has_dies) > > > > instead? > > I don't see that this is benefitting apps/users. Other than maintaining consistency; erroring with "we don't support dies" for 2+, but not for 1, is inconsistent, then I agree there isn't much reason to disallow it, since we'll be using the value of 1 anyway internally. I don't really have a strong preference here, so I'm fine with allowing dies=1. Thanks, drew > > > 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, Jul 19, 2021 at 06:48:15PM +0200, Andrew Jones wrote: > On Mon, Jul 19, 2021 at 05:36:39PM +0100, Daniel P. Berrangé wrote: > > On Mon, Jul 19, 2021 at 06:28:46PM +0200, Andrew Jones wrote: > > > 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 <drjones@redhat.com> > > > > Signed-off-by: Yanan Wang <wangyanan55@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? > > > > Conceptually that is fine. Before the introduction of CPU sockets > > with 2+ dies, you can credibly say that all sockets had 1 die, so > > it is nreasonable for users to say -smp ....,dies=1,.... > > > > libvirt will unconditionally set dies=1 for all QEMU targets if > > the user didn't specify an explicit dies value > > > > > To not allow that, can we do > > > > > > if (!mc->smp_dies_supported && config->has_dies) > > > > > > instead? > > > > I don't see that this is benefitting apps/users. > > Other than maintaining consistency; erroring with "we don't support dies" > for 2+, but not for 1, is inconsistent, then I agree there isn't much > reason to disallow it, since we'll be using the value of 1 anyway > internally. I don't really have a strong preference here, so I'm fine with > allowing dies=1. How about we tweak the error message from "dies not supported by this machine's CPU topology" to "multiple dies not supported by this machine's CPU topology" Regards, Daniel
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 <drjones@redhat.com> > Signed-off-by: Yanan Wang <wangyanan55@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) { > 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); Since we're allowing dies=1 (but not greater), I'm not convinced we need the conditionally built error message, and could just include "* dies" all the time. If we do want it to be conditionally different though, I'd just sugest calling error_setg twice. Although this duplicates stuff, it'll be clearer to read which I think is a net win. Regards, Daniel
On Mon, Jul 19 2021, Yanan Wang <wangyanan55@huawei.com> 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. So I guess that should also allow us to include s390x books/drawers? > > No functional change intended. > > Suggested-by: Andrew Jones <drjones@redhat.com> > Signed-off-by: Yanan Wang <wangyanan55@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) { > error_setg(errp, "dies not supported by this machine's CPU topology"); > return; > } I'm wondering how we should handle parameters that are not supported by a certain machine type. E.g. if we add support for books/drawers, specifying them is unlikely to make sense on anything but s390x. Would we allow to specify books=1 on a non-s390x machine, even though that is quite bogus? Or do we want to disallow setting any parameters that are not supported by the machine type? > @@ -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); Similarily here; do we want to mention parameters that are not applicable for the machine type? That might be confusing, but a conditional error message may get too complex if we add some more parameters. > 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; We can probably remove smp_parse, and call the generic parser directly? > 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
On 2021/7/20 14:57, Cornelia Huck wrote: > On Mon, Jul 19 2021, Yanan Wang <wangyanan55@huawei.com> 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. > So I guess that should also allow us to include s390x books/drawers? I guess so. Ideally, we hope for a generic parser which is friendly to different arch-specific members. We now have included PC specific dies, then I think maybe we should also welcome s390 specific books/drawers in. Now only sockets/cores/threads will be automatically calculated if omitted, the later introduced members (e.g. dies) will directly default to 1 if omitted. So if we also treat books/drawers like dies, at least the calculation code will be easy to maintain when new members come in the generic parser. >> No functional change intended. >> >> Suggested-by: Andrew Jones <drjones@redhat.com> >> Signed-off-by: Yanan Wang <wangyanan55@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) { >> error_setg(errp, "dies not supported by this machine's CPU topology"); >> return; >> } > I'm wondering how we should handle parameters that are not supported by > a certain machine type. E.g. if we add support for books/drawers, > specifying them is unlikely to make sense on anything but s390x. Would > we allow to specify books=1 on a non-s390x machine, even though that is > quite bogus? Or do we want to disallow setting any parameters that are > not supported by the machine type? The "parameter = 1" specification for a machine type that doesn't support it will not affect the calculation and also the reporting. The value will be 1 in the calculation and we can chose to only report the supported parameters in the error message. The only problem is whether we should reject "parameter=1" for the unrelated machine types. It seems that libvirt now unconditionally default dies to 1, so if we reject "dies=1" unexpected errors may be caused. Now books and drawers are introduced, I also wonder how we plan to deal with these two members in libvirt, just like dies or different with it ? >> @@ -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); > Similarily here; do we want to mention parameters that are not > applicable for the machine type? That might be confusing, but a > conditional error message may get too complex if we add some more > parameters. I think we'd better not report all the parameters. It's ok to use the arch specific members in the internal calculation, but it's indeed confusing to report them to users if they are not supported. There may be a better implementation of the current conditional error message reporting, I'm thinking about it... >> 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; > We can probably remove smp_parse, and call the generic parser directly? Yes, I think we can. Thanks, Yanan . >> 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 > .
On 2021/7/20 0:53, Daniel P. Berrangé wrote: > 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 <drjones@redhat.com> >> Signed-off-by: Yanan Wang <wangyanan55@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) { >> 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); > Since we're allowing dies=1 (but not greater), I'm not convinced we > need the conditionally built error message, and could just include > "* dies" all the time. > > If we do want it to be conditionally different though, I'd just > sugest calling error_setg twice. Although this duplicates stuff, > it'll be clearer to read which I think is a net win. > The duplicates may increase quickly if more arch specific members are introduced, I think the conditional error reporting may still be necessary, but should be more clearer than current approach. :) Thanks, Yanan .
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) { 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; 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;
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 <drjones@redhat.com> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> --- hw/core/machine.c | 28 ++++++++++------- hw/i386/pc.c | 76 +-------------------------------------------- include/hw/boards.h | 1 + 3 files changed, 19 insertions(+), 86 deletions(-)