| Message ID | 20210702100739.13672-6-wangyanan55@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | machine: smp parsing fixes and improvement | expand |
On Fri, Jul 02, 2021 at 06:07:38PM +0800, Yanan Wang wrote: > Since a machine type does not support topology parameter of dies, > it's probably more reasonable to reject any explicit specification > to avoid possible confuse, including "dies=0" and "dies=1" although > they won't affect the calculation of non-PC machines. > > Also a comment of struct SMPConfiguration is fixed. > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > --- > hw/core/machine.c | 2 +- > qapi/machine.json | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 58882835be..55785feee2 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -747,7 +747,7 @@ 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; > > - if (config->has_dies && config->dies != 0 && config->dies != 1) { > + if (config->has_dies) { > error_setg(errp, "dies not supported by this machine's CPU topology"); > } This will cause a functional regression. Libvirt always specifies dies=1 if QEMU supports it. I don't see a need to reject it, since conceptually it is reasonable to say that all existing CPUs have a single die. It simply that 99% of CPUs don't support more than 1 die. > diff --git a/qapi/machine.json b/qapi/machine.json > index c3210ee1fb..253f84abf6 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -1297,7 +1297,7 @@ > # > # @dies: number of dies per socket in the CPU topology > # > -# @cores: number of cores per thread in the CPU topology > +# @cores: number of cores per die in the CPU topology > # > # @threads: number of threads per core in the CPU topology > # This is a simple docs fix and ok Regards, Daniel
On 2021/7/2 18:18, Daniel P. Berrangé wrote: > On Fri, Jul 02, 2021 at 06:07:38PM +0800, Yanan Wang wrote: >> Since a machine type does not support topology parameter of dies, >> it's probably more reasonable to reject any explicit specification >> to avoid possible confuse, including "dies=0" and "dies=1" although >> they won't affect the calculation of non-PC machines. >> >> Also a comment of struct SMPConfiguration is fixed. >> >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >> --- >> hw/core/machine.c | 2 +- >> qapi/machine.json | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index 58882835be..55785feee2 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -747,7 +747,7 @@ 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; >> >> - if (config->has_dies && config->dies != 0 && config->dies != 1) { >> + if (config->has_dies) { >> error_setg(errp, "dies not supported by this machine's CPU topology"); >> } > This will cause a functional regression. Libvirt always specifies > dies=1 if QEMU supports it. I don't see a need to reject it, > since conceptually it is reasonable to say that all existing > CPUs have a single die. It simply that 99% of CPUs don't support > more than 1 die. Ok, I agree. This is a sincere suggestion, but clearly i didn't consider how Libvirt deals with configuration of dies. I will drop this part and the single comment fix can be merged into patch #6. >> diff --git a/qapi/machine.json b/qapi/machine.json >> index c3210ee1fb..253f84abf6 100644 >> --- a/qapi/machine.json >> +++ b/qapi/machine.json >> @@ -1297,7 +1297,7 @@ >> # >> # @dies: number of dies per socket in the CPU topology >> # >> -# @cores: number of cores per thread in the CPU topology >> +# @cores: number of cores per die in the CPU topology >> # >> # @threads: number of threads per core in the CPU topology >> # > This is a simple docs fix and ok > Thanks, Yanan .
diff --git a/hw/core/machine.c b/hw/core/machine.c index 58882835be..55785feee2 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -747,7 +747,7 @@ 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; - if (config->has_dies && config->dies != 0 && config->dies != 1) { + if (config->has_dies) { error_setg(errp, "dies not supported by this machine's CPU topology"); } diff --git a/qapi/machine.json b/qapi/machine.json index c3210ee1fb..253f84abf6 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -1297,7 +1297,7 @@ # # @dies: number of dies per socket in the CPU topology # -# @cores: number of cores per thread in the CPU topology +# @cores: number of cores per die in the CPU topology # # @threads: number of threads per core in the CPU topology #
Since a machine type does not support topology parameter of dies, it's probably more reasonable to reject any explicit specification to avoid possible confuse, including "dies=0" and "dies=1" although they won't affect the calculation of non-PC machines. Also a comment of struct SMPConfiguration is fixed. Signed-off-by: Yanan Wang <wangyanan55@huawei.com> --- hw/core/machine.c | 2 +- qapi/machine.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)