Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations
Hi Prasad,
On Mon, Mar 04, 2024 at 11:23:58AM +0530, Prasad Pandit wrote:
Date: Mon, 4 Mar 2024 11:23:58 +0530
From: Prasad Pandit <ppandit(a)redhat.com>
Subject: Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0"
SMP configurations
On Mon, 4 Mar 2024 at 10:02, Zhao Liu <zhao1.liu(a)linux.intel.com> wrote:
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index 25019c91ee36..96533886b14e 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms,
> (config->has_cores && config->cores == 0) ||
> (config->has_threads && config->threads == 0) ||
> (config->has_maxcpus && config->maxcpus == 0)) {
> - warn_report("Deprecated CPU topology (considered invalid): "
> - "CPU topology parameters must be greater than
zero");
> + error_setg(errp, "Invalid CPU topology: "
> + "CPU topology parameters must be greater than zero");
> + return;
> }
unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
This indicates the default maxcpus is initialized as 0 if user doesn't
specifies it.
For this case - no user configuration - maxcpus will be re-calculated
as:
maxcpus = maxcpus > 0 ? maxcpus : drawers * books * sockets * dies *
clusters * cores * threads; (*)
...
if (config->has_maxcpus && config->maxcpus == 0)
This check only wants to identify the case that user sets the 0.
* The check (has_maxcpus && maxcpus == 0) seems to be repeating above,
maybe we could check if (maxcpus == 0) error_setg().
If the default maxcpus is initialized as 0, then (maxcpus == 0) will
fail if user doesn't set maxcpus.
However, we could initialize maxcpus as other default value, e.g.,
maxcpus = config->has_maxcpus ? config->maxcpus : 1.
But it is still necessary to distinguish whether maxcpus is user-set or
auto-initialized.
If it is user-set, -smp should fail is there's invalid maxcpus/invalid
topology.
Otherwise, if it is auto-initialized, its value should be adjusted based
on other topology components as the above calculation in (*).
And same for
other topology parameters?
Other parameters also have the similar needs to distinguish if they're
set by user. So the check needs to also cover has_* fields.
* Also a check to ensure cpus <= maxcpus is required I think.
Yes, the valid topology needs this. This code block already covers this
case ;-):
if (maxcpus < cpus) {
g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
error_setg(errp, "Invalid CPU topology: "
"maxcpus must be equal to or greater than smp: "
"%s == maxcpus (%u) < smp_cpus (%u)",
topo_msg, maxcpus, cpus);
return;
}
Thanks,
Zhao