| Message ID | 20210928035755.11684-15-wangyanan55@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | machine: smp parsing fixes and improvement | expand |
On Tue, Sep 28, 2021 at 11:57:55AM +0800, Yanan Wang wrote: > Put both sanity-check of the input SMP configuration and sanity-check > of the output SMP configuration uniformly in the generic parser. Then > machine_set_smp() will become cleaner, also all the invalid scenarios > can be tested only by calling the parser. > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > Reviewed-by: Andrew Jones <drjones@redhat.com> > Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com> > --- > hw/core/machine.c | 63 +++++++++++++++++++++++------------------------ > 1 file changed, 31 insertions(+), 32 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On 9/28/21 05:57, Yanan Wang wrote: > Put both sanity-check of the input SMP configuration and sanity-check > of the output SMP configuration uniformly in the generic parser. Then > machine_set_smp() will become cleaner, also all the invalid scenarios > can be tested only by calling the parser. > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > Reviewed-by: Andrew Jones <drjones@redhat.com> > Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com> > --- > hw/core/machine.c | 63 +++++++++++++++++++++++------------------------ > 1 file changed, 31 insertions(+), 32 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index e2a48aa18c..637acd8d42 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -798,6 +798,20 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > unsigned threads = config->has_threads ? config->threads : 0; > unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; > > + /* > + * Specified CPU topology parameters must be greater than zero, > + * explicit configuration like "cpus=0" is not allowed. > + */ > + 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)) { > + warn_report("Invalid CPU topology deprecated: " Maybe: "Deprecated CPU topology: " or "Deprecated CPU topology (considered invalid): " > + "CPU topology parameters must be greater than zero"); > + }
On 2021/9/28 19:01, Philippe Mathieu-Daudé wrote: > On 9/28/21 05:57, Yanan Wang wrote: >> Put both sanity-check of the input SMP configuration and sanity-check >> of the output SMP configuration uniformly in the generic parser. Then >> machine_set_smp() will become cleaner, also all the invalid scenarios >> can be tested only by calling the parser. >> >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >> Reviewed-by: Andrew Jones <drjones@redhat.com> >> Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com> >> --- >> hw/core/machine.c | 63 +++++++++++++++++++++++------------------------ >> 1 file changed, 31 insertions(+), 32 deletions(-) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index e2a48aa18c..637acd8d42 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -798,6 +798,20 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) >> unsigned threads = config->has_threads ? config->threads : 0; >> unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; >> >> + /* >> + * Specified CPU topology parameters must be greater than zero, >> + * explicit configuration like "cpus=0" is not allowed. >> + */ >> + 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)) { >> + warn_report("Invalid CPU topology deprecated: " > Maybe: > > "Deprecated CPU topology: " > or > > "Deprecated CPU topology (considered invalid): " Ok, I will chose the second one which I think is clearer. Thanks, Yanan >> + "CPU topology parameters must be greater than zero"); >> + } > .
diff --git a/hw/core/machine.c b/hw/core/machine.c index e2a48aa18c..637acd8d42 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -798,6 +798,20 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) unsigned threads = config->has_threads ? config->threads : 0; unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; + /* + * Specified CPU topology parameters must be greater than zero, + * explicit configuration like "cpus=0" is not allowed. + */ + 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)) { + warn_report("Invalid CPU topology deprecated: " + "CPU topology parameters must be greater than zero"); + } + /* * If not supported by the machine, a topology parameter must be * omitted or specified equal to 1. @@ -873,6 +887,22 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) topo_msg, maxcpus, cpus); return; } + + if (ms->smp.cpus < mc->min_cpus) { + error_setg(errp, "Invalid SMP CPUs %d. The min CPUs " + "supported by machine '%s' is %d", + ms->smp.cpus, + mc->name, mc->min_cpus); + return; + } + + if (ms->smp.max_cpus > mc->max_cpus) { + error_setg(errp, "Invalid SMP CPUs %d. The max CPUs " + "supported by machine '%s' is %d", + ms->smp.max_cpus, + mc->name, mc->max_cpus); + return; + } } static void machine_get_smp(Object *obj, Visitor *v, const char *name, @@ -895,7 +925,6 @@ static void machine_get_smp(Object *obj, Visitor *v, const char *name, static void machine_set_smp(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { - MachineClass *mc = MACHINE_GET_CLASS(obj); MachineState *ms = MACHINE(obj); SMPConfiguration *config; ERRP_GUARD(); @@ -904,40 +933,10 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, return; } - /* - * Specified CPU topology parameters must be greater than zero, - * explicit configuration like "cpus=0" is not allowed. - */ - 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)) { - warn_report("Invalid CPU topology deprecated: " - "CPU topology parameters must be greater than zero"); - } - smp_parse(ms, config, errp); if (*errp) { - goto out_free; - } - - /* sanity-check smp_cpus and max_cpus against mc */ - if (ms->smp.cpus < mc->min_cpus) { - error_setg(errp, "Invalid SMP CPUs %d. The min CPUs " - "supported by machine '%s' is %d", - ms->smp.cpus, - mc->name, mc->min_cpus); - } else if (ms->smp.max_cpus > mc->max_cpus) { - error_setg(errp, "Invalid SMP CPUs %d. The max CPUs " - "supported by machine '%s' is %d", - ms->smp.max_cpus, - mc->name, mc->max_cpus); + qapi_free_SMPConfiguration(config); } - -out_free: - qapi_free_SMPConfiguration(config); } static void machine_class_init(ObjectClass *oc, void *data)