| Message ID | 20210719032043.25416-2-wangyanan55@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | machine: smp parsing fixes and improvement | expand |
On Mon, Jul 19, 2021 at 11:20:33AM +0800, Yanan Wang wrote: > In the SMP configuration, we should either specify a topology > parameter with a reasonable value (equal to or greater than 1) > or just leave it omitted and QEMU will calculate its value. > > Configurations which explicitly specify the topology parameters > as zero like "sockets=0" are meaningless, so disallow them. > > Suggested-by: Andrew Jones <drjones@redhat.com> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > --- > hw/core/machine.c | 31 +++++++++++++++++++++++-------- > hw/i386/pc.c | 29 +++++++++++++++++++++-------- > qapi/machine.json | 4 ++-- > 3 files changed, 46 insertions(+), 18 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 775add0795..d73daa10f4 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -745,11 +745,25 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > { > unsigned cpus = config->has_cpus ? config->cpus : 0; > unsigned sockets = config->has_sockets ? config->sockets : 0; > + unsigned dies = config->has_dies ? config->dies : 1; > unsigned cores = config->has_cores ? config->cores : 0; > unsigned threads = config->has_threads ? config->threads : 0; > + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; > + > + 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)) { > + error_setg(errp, "parameters must be equal to or greater than one" > + "if provided"); Missing a space between 'one' and 'if'. It's better to just put the whole string on one line too (ignore the 80 char thing) for error grepping. > + return; > + } > > - if (config->has_dies && config->dies != 0 && config->dies != 1) { > + if (dies > 1) { > error_setg(errp, "dies not supported by this machine's CPU topology"); > + return; > } > > /* compute missing values, prefer sockets over cores over threads */ > @@ -760,8 +774,8 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > sockets = sockets > 0 ? sockets : 1; > cpus = cores * threads * sockets; > } else { > - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; > - sockets = ms->smp.max_cpus / (cores * threads); > + maxcpus = maxcpus > 0 ? maxcpus : cpus; > + sockets = maxcpus / (cores * threads); > } > } else if (cores == 0) { > threads = threads > 0 ? threads : 1; > @@ -778,26 +792,27 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > return; > } > > - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; > + maxcpus = maxcpus > 0 ? maxcpus : cpus; > > - if (ms->smp.max_cpus < cpus) { > + if (maxcpus < cpus) { > error_setg(errp, "maxcpus must be equal to or greater than smp"); > return; > } > > - if (sockets * cores * threads != ms->smp.max_cpus) { > + if (sockets * cores * threads != maxcpus) { > error_setg(errp, "Invalid CPU topology: " > "sockets (%u) * cores (%u) * threads (%u) " > "!= maxcpus (%u)", > sockets, cores, threads, > - ms->smp.max_cpus); > + maxcpus); > return; > } > > ms->smp.cpus = cpus; > + ms->smp.sockets = sockets; > ms->smp.cores = cores; > ms->smp.threads = threads; > - ms->smp.sockets = sockets; > + ms->smp.max_cpus = maxcpus; > } > > 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 c2b9d62a35..c6b63c00a5 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -719,6 +719,18 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > unsigned dies = config->has_dies ? config->dies : 1; > unsigned cores = config->has_cores ? config->cores : 0; > unsigned threads = config->has_threads ? config->threads : 0; > + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; > + > + 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)) { > + error_setg(errp, "parameters must be equal to or greater than one" > + "if provided"); Same comment as above. > + return; > + } > > /* compute missing values, prefer sockets over cores over threads */ > if (cpus == 0 || sockets == 0) { > @@ -728,8 +740,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > sockets = sockets > 0 ? sockets : 1; > cpus = cores * threads * dies * sockets; > } else { > - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; > - sockets = ms->smp.max_cpus / (cores * threads * dies); > + maxcpus = maxcpus > 0 ? maxcpus : cpus; > + sockets = maxcpus / (cores * threads * dies); > } > } else if (cores == 0) { > threads = threads > 0 ? threads : 1; > @@ -746,27 +758,28 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err > return; > } > > - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; > + maxcpus = maxcpus > 0 ? maxcpus : cpus; > > - if (ms->smp.max_cpus < cpus) { > + if (maxcpus < cpus) { > error_setg(errp, "maxcpus must be equal to or greater than smp"); > return; > } > > - if (sockets * dies * cores * threads != ms->smp.max_cpus) { > + if (sockets * dies * cores * threads != maxcpus) { > error_setg(errp, "Invalid CPU topology deprecated: " > "sockets (%u) * dies (%u) * cores (%u) * threads (%u) " > "!= maxcpus (%u)", > sockets, dies, cores, threads, > - ms->smp.max_cpus); > + maxcpus); > return; > } > > ms->smp.cpus = cpus; > - ms->smp.cores = cores; > - ms->smp.threads = threads; > ms->smp.sockets = sockets; > ms->smp.dies = dies; > + ms->smp.cores = cores; > + ms->smp.threads = threads; > + ms->smp.max_cpus = maxcpus; > } > > static > diff --git a/qapi/machine.json b/qapi/machine.json > index c3210ee1fb..c11b2e6f73 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -1288,8 +1288,8 @@ > ## > # @SMPConfiguration: > # > -# Schema for CPU topology configuration. "0" or a missing value lets > -# QEMU figure out a suitable value based on the ones that are provided. > +# Schema for CPU topology configuration. A missing value lets QEMU > +# figure out a suitable value based on the ones that are provided. > # > # @cpus: number of virtual CPUs in the virtual machine > # > -- > 2.19.1 > Otherwise Reviewed-by: Andrew Jones <drjones@redhat.com>
On Mon, Jul 19, 2021 at 11:20:33AM +0800, Yanan Wang wrote: > In the SMP configuration, we should either specify a topology > parameter with a reasonable value (equal to or greater than 1) > or just leave it omitted and QEMU will calculate its value. > > Configurations which explicitly specify the topology parameters > as zero like "sockets=0" are meaningless, so disallow them. > > Suggested-by: Andrew Jones <drjones@redhat.com> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > --- > hw/core/machine.c | 31 +++++++++++++++++++++++-------- > hw/i386/pc.c | 29 +++++++++++++++++++++-------- > qapi/machine.json | 4 ++-- > 3 files changed, 46 insertions(+), 18 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 775add0795..d73daa10f4 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -745,11 +745,25 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) > { > unsigned cpus = config->has_cpus ? config->cpus : 0; > unsigned sockets = config->has_sockets ? config->sockets : 0; > + unsigned dies = config->has_dies ? config->dies : 1; It looks odd to set dies=1 by default at initially, when everything else is set to 0. I realize you're just copying existing pc_smp_parse code in this respect, but I feel like could benefit from a separate initialization with a comment to explain why we're hardcoding it to 1.... > unsigned cores = config->has_cores ? config->cores : 0; > unsigned threads = config->has_threads ? config->threads : 0; > + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; > + > + 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)) { > + error_setg(errp, "parameters must be equal to or greater than one" > + "if provided"); > + return; > + } > > - if (config->has_dies && config->dies != 0 && config->dies != 1) { > + if (dies > 1) { > error_setg(errp, "dies not supported by this machine's CPU topology"); > + return; > } .... eg how about here adding /* Never try to assign multiple dies when defaulting omitted topology */ if (dies == 0) { dies = 1; } > diff --git a/qapi/machine.json b/qapi/machine.json > index c3210ee1fb..c11b2e6f73 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -1288,8 +1288,8 @@ > ## > # @SMPConfiguration: > # > -# Schema for CPU topology configuration. "0" or a missing value lets > -# QEMU figure out a suitable value based on the ones that are provided. > +# Schema for CPU topology configuration. A missing value lets QEMU > +# figure out a suitable value based on the ones that are provided. Hmm, so we had actually documented that '0' had the same semantics as omitting a parameter. This was done in: commit 1e63fe685804dfadddd643bf3860b1a59702d4bf Author: Paolo Bonzini <pbonzini@redhat.com> Date: Thu Jun 17 17:53:06 2021 +0200 machine: pass QAPI struct to mc->smp_parse which hasn't been released yet. This was possible, but never documented, with the traditiaonl -smp impl before it was qapi-ified. I think that historical behaviour was simply a side effect of the QemuOpts impl rather than an intentional design, hence not documented. At the very least I think need to get rid of this bit of docs about "0" before this release, otherwise we'll have stronger need to consider a real deprecation process. Regards, Daniel
On 2021/7/20 0:11, Andrew Jones wrote: > On Mon, Jul 19, 2021 at 11:20:33AM +0800, Yanan Wang wrote: >> In the SMP configuration, we should either specify a topology >> parameter with a reasonable value (equal to or greater than 1) >> or just leave it omitted and QEMU will calculate its value. >> >> Configurations which explicitly specify the topology parameters >> as zero like "sockets=0" are meaningless, so disallow them. >> >> Suggested-by: Andrew Jones <drjones@redhat.com> >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >> --- >> hw/core/machine.c | 31 +++++++++++++++++++++++-------- >> hw/i386/pc.c | 29 +++++++++++++++++++++-------- >> qapi/machine.json | 4 ++-- >> 3 files changed, 46 insertions(+), 18 deletions(-) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index 775add0795..d73daa10f4 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -745,11 +745,25 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) >> { >> unsigned cpus = config->has_cpus ? config->cpus : 0; >> unsigned sockets = config->has_sockets ? config->sockets : 0; >> + unsigned dies = config->has_dies ? config->dies : 1; >> unsigned cores = config->has_cores ? config->cores : 0; >> unsigned threads = config->has_threads ? config->threads : 0; >> + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; >> + >> + 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)) { >> + error_setg(errp, "parameters must be equal to or greater than one" >> + "if provided"); > Missing a space between 'one' and 'if'. It's better to just put the whole > string on one line too (ignore the 80 char thing) for error grepping. > >> + return; >> + } >> >> - if (config->has_dies && config->dies != 0 && config->dies != 1) { >> + if (dies > 1) { >> error_setg(errp, "dies not supported by this machine's CPU topology"); >> + return; >> } >> >> /* compute missing values, prefer sockets over cores over threads */ >> @@ -760,8 +774,8 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) >> sockets = sockets > 0 ? sockets : 1; >> cpus = cores * threads * sockets; >> } else { >> - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; >> - sockets = ms->smp.max_cpus / (cores * threads); >> + maxcpus = maxcpus > 0 ? maxcpus : cpus; >> + sockets = maxcpus / (cores * threads); >> } >> } else if (cores == 0) { >> threads = threads > 0 ? threads : 1; >> @@ -778,26 +792,27 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) >> return; >> } >> >> - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; >> + maxcpus = maxcpus > 0 ? maxcpus : cpus; >> >> - if (ms->smp.max_cpus < cpus) { >> + if (maxcpus < cpus) { >> error_setg(errp, "maxcpus must be equal to or greater than smp"); >> return; >> } >> >> - if (sockets * cores * threads != ms->smp.max_cpus) { >> + if (sockets * cores * threads != maxcpus) { >> error_setg(errp, "Invalid CPU topology: " >> "sockets (%u) * cores (%u) * threads (%u) " >> "!= maxcpus (%u)", >> sockets, cores, threads, >> - ms->smp.max_cpus); >> + maxcpus); >> return; >> } >> >> ms->smp.cpus = cpus; >> + ms->smp.sockets = sockets; >> ms->smp.cores = cores; >> ms->smp.threads = threads; >> - ms->smp.sockets = sockets; >> + ms->smp.max_cpus = maxcpus; >> } >> >> 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 c2b9d62a35..c6b63c00a5 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -719,6 +719,18 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err >> unsigned dies = config->has_dies ? config->dies : 1; >> unsigned cores = config->has_cores ? config->cores : 0; >> unsigned threads = config->has_threads ? config->threads : 0; >> + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; >> + >> + 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)) { >> + error_setg(errp, "parameters must be equal to or greater than one" >> + "if provided"); > Same comment as above. Ok, I will fix them. >> + return; >> + } >> >> /* compute missing values, prefer sockets over cores over threads */ >> if (cpus == 0 || sockets == 0) { >> @@ -728,8 +740,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err >> sockets = sockets > 0 ? sockets : 1; >> cpus = cores * threads * dies * sockets; >> } else { >> - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; >> - sockets = ms->smp.max_cpus / (cores * threads * dies); >> + maxcpus = maxcpus > 0 ? maxcpus : cpus; >> + sockets = maxcpus / (cores * threads * dies); >> } >> } else if (cores == 0) { >> threads = threads > 0 ? threads : 1; >> @@ -746,27 +758,28 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err >> return; >> } >> >> - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; >> + maxcpus = maxcpus > 0 ? maxcpus : cpus; >> >> - if (ms->smp.max_cpus < cpus) { >> + if (maxcpus < cpus) { >> error_setg(errp, "maxcpus must be equal to or greater than smp"); >> return; >> } >> >> - if (sockets * dies * cores * threads != ms->smp.max_cpus) { >> + if (sockets * dies * cores * threads != maxcpus) { >> error_setg(errp, "Invalid CPU topology deprecated: " >> "sockets (%u) * dies (%u) * cores (%u) * threads (%u) " >> "!= maxcpus (%u)", >> sockets, dies, cores, threads, >> - ms->smp.max_cpus); >> + maxcpus); >> return; >> } >> >> ms->smp.cpus = cpus; >> - ms->smp.cores = cores; >> - ms->smp.threads = threads; >> ms->smp.sockets = sockets; >> ms->smp.dies = dies; >> + ms->smp.cores = cores; >> + ms->smp.threads = threads; >> + ms->smp.max_cpus = maxcpus; >> } >> >> static >> diff --git a/qapi/machine.json b/qapi/machine.json >> index c3210ee1fb..c11b2e6f73 100644 >> --- a/qapi/machine.json >> +++ b/qapi/machine.json >> @@ -1288,8 +1288,8 @@ >> ## >> # @SMPConfiguration: >> # >> -# Schema for CPU topology configuration. "0" or a missing value lets >> -# QEMU figure out a suitable value based on the ones that are provided. >> +# Schema for CPU topology configuration. A missing value lets QEMU >> +# figure out a suitable value based on the ones that are provided. >> # >> # @cpus: number of virtual CPUs in the virtual machine >> # >> -- >> 2.19.1 >> > Otherwise > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > . Thanks, Yanan .
On 2021/7/20 0:46, Daniel P. Berrangé wrote: > On Mon, Jul 19, 2021 at 11:20:33AM +0800, Yanan Wang wrote: >> In the SMP configuration, we should either specify a topology >> parameter with a reasonable value (equal to or greater than 1) >> or just leave it omitted and QEMU will calculate its value. >> >> Configurations which explicitly specify the topology parameters >> as zero like "sockets=0" are meaningless, so disallow them. >> >> Suggested-by: Andrew Jones <drjones@redhat.com> >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >> --- >> hw/core/machine.c | 31 +++++++++++++++++++++++-------- >> hw/i386/pc.c | 29 +++++++++++++++++++++-------- >> qapi/machine.json | 4 ++-- >> 3 files changed, 46 insertions(+), 18 deletions(-) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index 775add0795..d73daa10f4 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -745,11 +745,25 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) >> { >> unsigned cpus = config->has_cpus ? config->cpus : 0; >> unsigned sockets = config->has_sockets ? config->sockets : 0; >> + unsigned dies = config->has_dies ? config->dies : 1; > It looks odd to set dies=1 by default at initially, when everything > else is set to 0. I realize you're just copying existing pc_smp_parse > code in this respect, but I feel like could benefit from a separate > initialization with a comment to explain why we're hardcoding it > to 1.... > >> unsigned cores = config->has_cores ? config->cores : 0; >> unsigned threads = config->has_threads ? config->threads : 0; >> + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; >> + >> + 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)) { >> + error_setg(errp, "parameters must be equal to or greater than one" >> + "if provided"); >> + return; >> + } >> >> - if (config->has_dies && config->dies != 0 && config->dies != 1) { >> + if (dies > 1) { >> error_setg(errp, "dies not supported by this machine's CPU topology"); >> + return; >> } > .... eg how about here adding > > /* Never try to assign multiple dies when defaulting omitted topology */ > if (dies == 0) { > dies = 1; > } Yeah, I agree to default dies to 0 like the other parameters at initially and then explicitly assign it to 1 if omitted here. But I think this explicit assignment should be in pc_smp_parse, because dies is never used in the calculation in smp_parse yet except the front sanity-check. So I think what should be updated for this patch is: 1) default dies to 0 at initially both in smp_parse and pc_smp_parse 2) then explicitly assign dies to 1 if it's omitted in pc_smp_parse > > >> diff --git a/qapi/machine.json b/qapi/machine.json >> index c3210ee1fb..c11b2e6f73 100644 >> --- a/qapi/machine.json >> +++ b/qapi/machine.json >> @@ -1288,8 +1288,8 @@ >> ## >> # @SMPConfiguration: >> # >> -# Schema for CPU topology configuration. "0" or a missing value lets >> -# QEMU figure out a suitable value based on the ones that are provided. >> +# Schema for CPU topology configuration. A missing value lets QEMU >> +# figure out a suitable value based on the ones that are provided. > Hmm, so we had actually documented that '0' had the same semantics > as omitting a parameter. This was done in: > > commit 1e63fe685804dfadddd643bf3860b1a59702d4bf > Author: Paolo Bonzini <pbonzini@redhat.com> > Date: Thu Jun 17 17:53:06 2021 +0200 > > machine: pass QAPI struct to mc->smp_parse > > which hasn't been released yet. > > This was possible, but never documented, with the traditiaonl -smp > impl before it was qapi-ified. I think that historical behaviour > was simply a side effect of the QemuOpts impl rather than an > intentional design, hence not documented. Agreed. > At the very least I think need to get rid of this bit of docs about > "0" before this release, otherwise we'll have stronger need to > consider a real deprecation process. If the doc needs to be fixed right now, then I think we'd better resend this single patch separately for 6.1, including the doc fix and also the related sanity-check in the parsers. Right? Thanks, Yanan .
diff --git a/hw/core/machine.c b/hw/core/machine.c index 775add0795..d73daa10f4 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -745,11 +745,25 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) { unsigned cpus = config->has_cpus ? config->cpus : 0; unsigned sockets = config->has_sockets ? config->sockets : 0; + unsigned dies = config->has_dies ? config->dies : 1; unsigned cores = config->has_cores ? config->cores : 0; unsigned threads = config->has_threads ? config->threads : 0; + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; + + 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)) { + error_setg(errp, "parameters must be equal to or greater than one" + "if provided"); + return; + } - if (config->has_dies && config->dies != 0 && config->dies != 1) { + if (dies > 1) { error_setg(errp, "dies not supported by this machine's CPU topology"); + return; } /* compute missing values, prefer sockets over cores over threads */ @@ -760,8 +774,8 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) sockets = sockets > 0 ? sockets : 1; cpus = cores * threads * sockets; } else { - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; - sockets = ms->smp.max_cpus / (cores * threads); + maxcpus = maxcpus > 0 ? maxcpus : cpus; + sockets = maxcpus / (cores * threads); } } else if (cores == 0) { threads = threads > 0 ? threads : 1; @@ -778,26 +792,27 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) return; } - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; + maxcpus = maxcpus > 0 ? maxcpus : cpus; - if (ms->smp.max_cpus < cpus) { + if (maxcpus < cpus) { error_setg(errp, "maxcpus must be equal to or greater than smp"); return; } - if (sockets * cores * threads != ms->smp.max_cpus) { + if (sockets * cores * threads != maxcpus) { error_setg(errp, "Invalid CPU topology: " "sockets (%u) * cores (%u) * threads (%u) " "!= maxcpus (%u)", sockets, cores, threads, - ms->smp.max_cpus); + maxcpus); return; } ms->smp.cpus = cpus; + ms->smp.sockets = sockets; ms->smp.cores = cores; ms->smp.threads = threads; - ms->smp.sockets = sockets; + ms->smp.max_cpus = maxcpus; } 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 c2b9d62a35..c6b63c00a5 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -719,6 +719,18 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err unsigned dies = config->has_dies ? config->dies : 1; unsigned cores = config->has_cores ? config->cores : 0; unsigned threads = config->has_threads ? config->threads : 0; + unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; + + 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)) { + error_setg(errp, "parameters must be equal to or greater than one" + "if provided"); + return; + } /* compute missing values, prefer sockets over cores over threads */ if (cpus == 0 || sockets == 0) { @@ -728,8 +740,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err sockets = sockets > 0 ? sockets : 1; cpus = cores * threads * dies * sockets; } else { - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; - sockets = ms->smp.max_cpus / (cores * threads * dies); + maxcpus = maxcpus > 0 ? maxcpus : cpus; + sockets = maxcpus / (cores * threads * dies); } } else if (cores == 0) { threads = threads > 0 ? threads : 1; @@ -746,27 +758,28 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err return; } - ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus; + maxcpus = maxcpus > 0 ? maxcpus : cpus; - if (ms->smp.max_cpus < cpus) { + if (maxcpus < cpus) { error_setg(errp, "maxcpus must be equal to or greater than smp"); return; } - if (sockets * dies * cores * threads != ms->smp.max_cpus) { + if (sockets * dies * cores * threads != maxcpus) { error_setg(errp, "Invalid CPU topology deprecated: " "sockets (%u) * dies (%u) * cores (%u) * threads (%u) " "!= maxcpus (%u)", sockets, dies, cores, threads, - ms->smp.max_cpus); + maxcpus); return; } ms->smp.cpus = cpus; - ms->smp.cores = cores; - ms->smp.threads = threads; ms->smp.sockets = sockets; ms->smp.dies = dies; + ms->smp.cores = cores; + ms->smp.threads = threads; + ms->smp.max_cpus = maxcpus; } static diff --git a/qapi/machine.json b/qapi/machine.json index c3210ee1fb..c11b2e6f73 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -1288,8 +1288,8 @@ ## # @SMPConfiguration: # -# Schema for CPU topology configuration. "0" or a missing value lets -# QEMU figure out a suitable value based on the ones that are provided. +# Schema for CPU topology configuration. A missing value lets QEMU +# figure out a suitable value based on the ones that are provided. # # @cpus: number of virtual CPUs in the virtual machine #
In the SMP configuration, we should either specify a topology parameter with a reasonable value (equal to or greater than 1) or just leave it omitted and QEMU will calculate its value. Configurations which explicitly specify the topology parameters as zero like "sockets=0" are meaningless, so disallow them. Suggested-by: Andrew Jones <drjones@redhat.com> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> --- hw/core/machine.c | 31 +++++++++++++++++++++++-------- hw/i386/pc.c | 29 +++++++++++++++++++++-------- qapi/machine.json | 4 ++-- 3 files changed, 46 insertions(+), 18 deletions(-)