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");
     }
 
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
 #
-- 
2.19.1

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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
.