* [PATCH-for-6.1? v2 0/3] hw/core: fix error checkig in smp_parse
@ 2021-08-13 11:26 Philippe Mathieu-Daudé
2021-08-13 11:26 ` [PATCH-for-6.1? v2 1/3] hw/core: Add missing return on error Philippe Mathieu-Daudé
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-13 11:26 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P . Berrangé,
Eduardo Habkost, Michael S. Tsirkin, Richard Henderson,
Paolo Bonzini, Philippe Mathieu-Daudé
Respin of Daniel's series checking MachineClass::smp_parse()
return value instead of *errp.
Daniel P. Berrangé (1):
hw/core: fix error checkig in smp_parse
Philippe Mathieu-Daudé (2):
hw/core: Add missing return on error
hw/core: Have MachineClass::smp_parse() return boolean on error
include/hw/boards.h | 2 +-
hw/core/machine.c | 14 ++++++++------
hw/i386/pc.c | 10 ++++++----
3 files changed, 15 insertions(+), 11 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH-for-6.1? v2 1/3] hw/core: Add missing return on error 2021-08-13 11:26 [PATCH-for-6.1? v2 0/3] hw/core: fix error checkig in smp_parse Philippe Mathieu-Daudé @ 2021-08-13 11:26 ` Philippe Mathieu-Daudé 2021-08-13 12:29 ` Paolo Bonzini 2021-08-13 11:26 ` [PATCH-for-6.1? v2 2/3] hw/core: Have MachineClass::smp_parse() return boolean " Philippe Mathieu-Daudé 2021-08-13 11:26 ` [PATCH-for-6.1? v2 3/3] hw/core: fix error checkig in smp_parse Philippe Mathieu-Daudé 2 siblings, 1 reply; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2021-08-13 11:26 UTC (permalink / raw) To: qemu-devel Cc: Daniel P . Berrangé, Eduardo Habkost, Michael S. Tsirkin, Richard Henderson, Paolo Bonzini, Philippe Mathieu-Daudé If dies is not supported by this machine's CPU topology, don't keep processing options and return directly. Fixes: 0aebebb561c ("machine: reject -smp dies!=1 for non-PC machines") Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/core/machine.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/core/machine.c b/hw/core/machine.c index 943974d411c..abaeda589b7 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -752,6 +752,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) if (config->has_dies && config->dies != 0 && config->dies != 1) { error_setg(errp, "dies not supported by this machine's CPU topology"); + return; } /* compute missing values, prefer sockets over cores over threads */ -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH-for-6.1? v2 1/3] hw/core: Add missing return on error 2021-08-13 11:26 ` [PATCH-for-6.1? v2 1/3] hw/core: Add missing return on error Philippe Mathieu-Daudé @ 2021-08-13 12:29 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2021-08-13 12:29 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Richard Henderson, Daniel P . Berrangé, Eduardo Habkost, Michael S. Tsirkin On 13/08/21 13:26, Philippe Mathieu-Daudé wrote: > If dies is not supported by this machine's CPU topology, don't > keep processing options and return directly. > > Fixes: 0aebebb561c ("machine: reject -smp dies!=1 for non-PC machines") > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/core/machine.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 943974d411c..abaeda589b7 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -752,6 +752,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > > if (config->has_dies && config->dies != 0 && config->dies != 1) { > error_setg(errp, "dies not supported by this machine's CPU topology"); > + return; > } > > /* compute missing values, prefer sockets over cores over threads */ > Queued, thanks. Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH-for-6.1? v2 2/3] hw/core: Have MachineClass::smp_parse() return boolean on error 2021-08-13 11:26 [PATCH-for-6.1? v2 0/3] hw/core: fix error checkig in smp_parse Philippe Mathieu-Daudé 2021-08-13 11:26 ` [PATCH-for-6.1? v2 1/3] hw/core: Add missing return on error Philippe Mathieu-Daudé @ 2021-08-13 11:26 ` Philippe Mathieu-Daudé 2021-08-13 12:26 ` Paolo Bonzini 2021-08-13 11:26 ` [PATCH-for-6.1? v2 3/3] hw/core: fix error checkig in smp_parse Philippe Mathieu-Daudé 2 siblings, 1 reply; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2021-08-13 11:26 UTC (permalink / raw) To: qemu-devel Cc: Daniel P . Berrangé, Eduardo Habkost, Michael S. Tsirkin, Richard Henderson, Paolo Bonzini, Philippe Mathieu-Daudé Just for consistency, following the example documented since commit e3fe3988d7 ("error: Document Error API usage rules"), return a boolean value indicating an error is set or not. Directly pass errp as the local_err is not requested in our case. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/hw/boards.h | 2 +- hw/core/machine.c | 12 +++++++----- hw/i386/pc.c | 10 ++++++---- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/include/hw/boards.h b/include/hw/boards.h index accd6eff35a..d5b7058c2e2 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -209,7 +209,7 @@ struct MachineClass { void (*reset)(MachineState *state); void (*wakeup)(MachineState *state); int (*kvm_type)(MachineState *machine, const char *arg); - void (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp); + bool (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp); BlockInterfaceType block_default_type; int units_per_default_bus; diff --git a/hw/core/machine.c b/hw/core/machine.c index abaeda589b7..159c6b098e2 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -743,7 +743,7 @@ void machine_set_cpu_numa_node(MachineState *machine, } } -static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) +static bool smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) { unsigned cpus = config->has_cpus ? config->cpus : 0; unsigned sockets = config->has_sockets ? config->sockets : 0; @@ -752,7 +752,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) if (config->has_dies && config->dies != 0 && config->dies != 1) { error_setg(errp, "dies not supported by this machine's CPU topology"); - return; + return true; } /* compute missing values, prefer sockets over cores over threads */ @@ -778,14 +778,14 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) "sockets (%u) * cores (%u) * threads (%u) < " "smp_cpus (%u)", sockets, cores, threads, cpus); - return; + return true; } ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; if (ms->smp.max_cpus < cpus) { error_setg(errp, "maxcpus must be equal to or greater than smp"); - return; + return true; } if (sockets * cores * threads != ms->smp.max_cpus) { @@ -794,13 +794,15 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) "!= maxcpus (%u)", sockets, cores, threads, ms->smp.max_cpus); - return; + return true; } ms->smp.cpus = cpus; ms->smp.cores = cores; ms->smp.threads = threads; ms->smp.sockets = sockets; + + return false; } static void machine_get_smp(Object *obj, Visitor *v, const char *name, diff --git a/hw/i386/pc.c b/hw/i386/pc.c index c2b9d62a358..84138a8bfd2 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -712,7 +712,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) * This function is very similar to smp_parse() * in hw/core/machine.c but includes CPU die support. */ -static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) +static bool pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) { unsigned cpus = config->has_cpus ? config->cpus : 0; unsigned sockets = config->has_sockets ? config->sockets : 0; @@ -743,14 +743,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < " "smp_cpus (%u)", sockets, dies, cores, threads, cpus); - return; + return true; } ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; if (ms->smp.max_cpus < cpus) { error_setg(errp, "maxcpus must be equal to or greater than smp"); - return; + return true; } if (sockets * dies * cores * threads != ms->smp.max_cpus) { @@ -759,7 +759,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err "!= maxcpus (%u)", sockets, dies, cores, threads, ms->smp.max_cpus); - return; + return true; } ms->smp.cpus = cpus; @@ -767,6 +767,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err ms->smp.threads = threads; ms->smp.sockets = sockets; ms->smp.dies = dies; + + return false; } static -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH-for-6.1? v2 2/3] hw/core: Have MachineClass::smp_parse() return boolean on error 2021-08-13 11:26 ` [PATCH-for-6.1? v2 2/3] hw/core: Have MachineClass::smp_parse() return boolean " Philippe Mathieu-Daudé @ 2021-08-13 12:26 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2021-08-13 12:26 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Richard Henderson, Daniel P . Berrangé, Eduardo Habkost, Michael S. Tsirkin On 13/08/21 13:26, Philippe Mathieu-Daudé wrote: > Just for consistency, following the example documented since > commit e3fe3988d7 ("error: Document Error API usage rules"), > return a boolean value indicating an error is set or not. > Directly pass errp as the local_err is not requested in our > case. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> This was intentional; no one should invoke mc->smp_parse directly except for machine_set_smp, therefore if mc->smp_parse returns bool there are more places where things can go wrong (instead of just the one that is fixed by patch 3). Paolo > --- > include/hw/boards.h | 2 +- > hw/core/machine.c | 12 +++++++----- > hw/i386/pc.c | 10 ++++++---- > 3 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index accd6eff35a..d5b7058c2e2 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -209,7 +209,7 @@ struct MachineClass { > void (*reset)(MachineState *state); > void (*wakeup)(MachineState *state); > int (*kvm_type)(MachineState *machine, const char *arg); > - void (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp); > + bool (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp); > > BlockInterfaceType block_default_type; > int units_per_default_bus; > diff --git a/hw/core/machine.c b/hw/core/machine.c > index abaeda589b7..159c6b098e2 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -743,7 +743,7 @@ void machine_set_cpu_numa_node(MachineState *machine, > } > } > > -static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > +static bool smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > { > unsigned cpus = config->has_cpus ? config->cpus : 0; > unsigned sockets = config->has_sockets ? config->sockets : 0; > @@ -752,7 +752,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > > if (config->has_dies && config->dies != 0 && config->dies != 1) { > error_setg(errp, "dies not supported by this machine's CPU topology"); > - return; > + return true; > } > > /* compute missing values, prefer sockets over cores over threads */ > @@ -778,14 +778,14 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > "sockets (%u) * cores (%u) * threads (%u) < " > "smp_cpus (%u)", > sockets, cores, threads, cpus); > - return; > + return true; > } > > ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; > > if (ms->smp.max_cpus < cpus) { > error_setg(errp, "maxcpus must be equal to or greater than smp"); > - return; > + return true; > } > > if (sockets * cores * threads != ms->smp.max_cpus) { > @@ -794,13 +794,15 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > "!= maxcpus (%u)", > sockets, cores, threads, > ms->smp.max_cpus); > - return; > + return true; > } > > ms->smp.cpus = cpus; > ms->smp.cores = cores; > ms->smp.threads = threads; > ms->smp.sockets = sockets; > + > + return false; > } > > static void machine_get_smp(Object *obj, Visitor *v, const char *name, > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index c2b9d62a358..84138a8bfd2 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -712,7 +712,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) > * This function is very similar to smp_parse() > * in hw/core/machine.c but includes CPU die support. > */ > -static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > +static bool pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > { > unsigned cpus = config->has_cpus ? config->cpus : 0; > unsigned sockets = config->has_sockets ? config->sockets : 0; > @@ -743,14 +743,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < " > "smp_cpus (%u)", > sockets, dies, cores, threads, cpus); > - return; > + return true; > } > > ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; > > if (ms->smp.max_cpus < cpus) { > error_setg(errp, "maxcpus must be equal to or greater than smp"); > - return; > + return true; > } > > if (sockets * dies * cores * threads != ms->smp.max_cpus) { > @@ -759,7 +759,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > "!= maxcpus (%u)", > sockets, dies, cores, threads, > ms->smp.max_cpus); > - return; > + return true; > } > > ms->smp.cpus = cpus; > @@ -767,6 +767,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > ms->smp.threads = threads; > ms->smp.sockets = sockets; > ms->smp.dies = dies; > + > + return false; > } > > static > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH-for-6.1? v2 3/3] hw/core: fix error checkig in smp_parse 2021-08-13 11:26 [PATCH-for-6.1? v2 0/3] hw/core: fix error checkig in smp_parse Philippe Mathieu-Daudé 2021-08-13 11:26 ` [PATCH-for-6.1? v2 1/3] hw/core: Add missing return on error Philippe Mathieu-Daudé 2021-08-13 11:26 ` [PATCH-for-6.1? v2 2/3] hw/core: Have MachineClass::smp_parse() return boolean " Philippe Mathieu-Daudé @ 2021-08-13 11:26 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2021-08-13 11:26 UTC (permalink / raw) To: qemu-devel Cc: Daniel P . Berrangé, Eduardo Habkost, Michael S. Tsirkin, Richard Henderson, Paolo Bonzini, Philippe Mathieu-Daudé From: Daniel P. Berrangé <berrange@redhat.com> The machine_set_smp() mistakenly checks 'errp' not '*errp', and so thinks there is an error every single time it runs. This causes it to jump to the end of the method, skipping the max CPUs checks. The caller meanwhile sees no error and so carries on execution. The result of all this is: $ qemu-system-x86_64 -smp -1 qemu-system-x86_64: GLib: ../glib/gmem.c:142: failed to allocate 481036337048 bytes instead of $ qemu-system-x86_64 -smp -1 qemu-system-x86_64: Invalid SMP CPUs -1. The max CPUs supported by machine 'pc-i440fx-6.1' is 255 This is a regression from commit fe68090e8fbd6e831aaf3fc3bb0459c5cccf14cf Author: Paolo Bonzini <pbonzini@redhat.com> Date: Thu May 13 09:03:48 2021 -0400 machine: add smp compound property Closes: https://gitlab.com/qemu-project/qemu/-/issues/524 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Message-Id: <20210812175353.4128471-1-berrange@redhat.com> [PMD: Check MachineClass::smp_parse() returned value] Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/core/machine.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/core/machine.c b/hw/core/machine.c index 159c6b098e2..29edf655140 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -834,8 +834,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, return; } - mc->smp_parse(ms, config, errp); - if (errp) { + if (mc->smp_parse(ms, config, errp)) { goto out_free; } -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-13 12:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-13 11:26 [PATCH-for-6.1? v2 0/3] hw/core: fix error checkig in smp_parse Philippe Mathieu-Daudé 2021-08-13 11:26 ` [PATCH-for-6.1? v2 1/3] hw/core: Add missing return on error Philippe Mathieu-Daudé 2021-08-13 12:29 ` Paolo Bonzini 2021-08-13 11:26 ` [PATCH-for-6.1? v2 2/3] hw/core: Have MachineClass::smp_parse() return boolean " Philippe Mathieu-Daudé 2021-08-13 12:26 ` Paolo Bonzini 2021-08-13 11:26 ` [PATCH-for-6.1? v2 3/3] hw/core: fix error checkig in smp_parse Philippe Mathieu-Daudé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.