On Thu, Jul 22, 2021 at 04:10:32PM +0800, wangyanan (Y) wrote: > On 2021/7/20 0:53, Andrew Jones wrote: > > On Mon, Jul 19, 2021 at 11:20:37AM +0800, Yanan Wang wrote: > > > We totally have two requirements for a valid SMP configuration: > > s/totally// > > > > > the sum of "sockets * dies * cores * threads" must represent all > > the product > > > > > the possible cpus, i.e., max_cpus, and must include the initial > > > present cpus, i.e., smp_cpus. > > > > > > We only need to ensure "sockets * dies * cores * threads == maxcpus" > > > at first and then ensure "sockets * dies * cores * threads >= cpus". > > Or, "maxcpus >= cpus" > > > > > With a reasonable order of the sanity-check, we can simplify the > > > error reporting code. > > > > > > Signed-off-by: Yanan Wang <wangyana...@huawei.com> > > > --- > > > hw/core/machine.c | 25 ++++++++++--------------- > > > 1 file changed, 10 insertions(+), 15 deletions(-) > > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > > index 668f0a1553..8b4d07d3fc 100644 > > > --- a/hw/core/machine.c > > > +++ b/hw/core/machine.c > > > @@ -788,21 +788,6 @@ static void smp_parse(MachineState *ms, > > > SMPConfiguration *config, Error **errp) > > > cpus = cpus > 0 ? cpus : sockets * dies * cores * threads; > > > maxcpus = maxcpus > 0 ? maxcpus : cpus; > > > - 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)%s * cores (%u) * threads (%u) < " > > > - "smp_cpus (%u)", > > > - sockets, dies_msg, cores, threads, cpus); > > > - return; > > > - } > > > - > > > - if (maxcpus < cpus) { > > > - error_setg(errp, "maxcpus must be equal to or greater than smp"); > > > - return; > > > - } > > This may be redundant when determining a valid config, but by checking it > > separately we can provide a more useful error message. > Yes, this message is more useful. Can we also report the exact values of the > parameters within this error message ?
sure > How about the following: > > if (sockets * cores * threads != maxcpus) { > error_setg("product of the topology must be equal to maxcpus" > "sockets (%u) * cores (%u) * threads (%u)" > "!= maxcpus (%u)", > sockets, cores, threads, maxcpus); > return; > } > > if (maxcpus < cpus) { > error_setg("maxcpus must be equal to or greater than smp:" > "sockets (%u) * cores (%u) * threads (%u)" > "== maxcpus (%u) < smp_cpus (%u)", > sockets, cores, threads, maxcpus, cpus); > return; > } OK by me Thanks, drew > > Thanks, > Yanan > . > > > - > > > if (sockets * dies * cores * threads != maxcpus) { > > > g_autofree char *dies_msg = g_strdup_printf( > > > mc->smp_dies_supported ? " * dies (%u)" : "", dies); > > > @@ -814,6 +799,16 @@ static void smp_parse(MachineState *ms, > > > SMPConfiguration *config, Error **errp) > > > return; > > > } > > > + if (sockets * dies * cores * threads < cpus) { > > > + g_autofree char *dies_msg = g_strdup_printf( > > > + mc->smp_dies_supported ? " * dies (%u)" : "", dies); > > > + error_setg(errp, "Invalid CPU topology: " > > > + "sockets (%u)%s * cores (%u) * threads (%u) < " > > > + "smp_cpus (%u)", > > > + sockets, dies_msg, cores, threads, cpus); > > > + return; > > > + } > > > + > > > ms->smp.cpus = cpus; > > > ms->smp.sockets = sockets; > > > ms->smp.dies = dies; > > > -- > > > 2.19.1 > > > > > I'm not sure we need this patch. > > > > Thanks, > > drew > > > > . >