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@xxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx> 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
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
