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) {
> 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
--
|: 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 :|
