It's possible that dies parameter is explicitly specified as "dies=0"
in the cmdline, if so we will wrongly calculate the other ommited
parameters such as "sockets = maxcpus / (dies * cores * threads);"
with a zeroed dies value.

So perform zero-check (default the value to 1 if zeroed) for -smp dies
before using it to calculate other parameters.

Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc)
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/i386/pc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a44511c937..93d1f12a49 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
 {
     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 dies    = config->has_dies ? config->dies : 0;
     unsigned cores   = config->has_cores ? config->cores : 0;
     unsigned threads = config->has_threads ? config->threads : 0;
     unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
 
     /* compute missing values, prefer sockets over cores over threads */
+    dies = dies > 0 ? dies : 1;
+
     if (cpus == 0 || sockets == 0) {
         cores = cores > 0 ? cores : 1;
         threads = threads > 0 ? threads : 1;
-- 
2.19.1

On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote:
> It's possible that dies parameter is explicitly specified as "dies=0"
> in the cmdline, if so we will wrongly calculate the other ommited
> parameters such as "sockets = maxcpus / (dies * cores * threads);"
> with a zeroed dies value.
> 
> So perform zero-check (default the value to 1 if zeroed) for -smp dies
> before using it to calculate other parameters.

OK, dies=0 may make some sense for a user that doesn't want to describe
dies.

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc)
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/i386/pc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a44511c937..93d1f12a49 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>  {
>      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 dies    = config->has_dies ? config->dies : 0;
>      unsigned cores   = config->has_cores ? config->cores : 0;
>      unsigned threads = config->has_threads ? config->threads : 0;
>      unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>  
>      /* compute missing values, prefer sockets over cores over threads */
> +    dies = dies > 0 ? dies : 1;
> +
>      if (cpus == 0 || sockets == 0) {
>          cores = cores > 0 ? cores : 1;
>          threads = threads > 0 ? threads : 1;
> -- 
> 2.19.1
> 

On Mon, Jul 12, 2021 at 05:04:04PM +0200, Andrew Jones wrote:
> On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote:
> > It's possible that dies parameter is explicitly specified as "dies=0"
> > in the cmdline, if so we will wrongly calculate the other ommited
> > parameters such as "sockets = maxcpus / (dies * cores * threads);"
> > with a zeroed dies value.
> > 
> > So perform zero-check (default the value to 1 if zeroed) for -smp dies
> > before using it to calculate other parameters.
> 
> OK, dies=0 may make some sense for a user that doesn't want to describe
> dies.
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

But... this is the pc smp parse function, not the general one, so maybe my
comment from patch 1 should apply here as well, which is, that dies=0
should be an error rather than silently changed to dies=1.

> 
> > 
> > Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc)
> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > ---
> >  hw/i386/pc.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index a44511c937..93d1f12a49 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
> >  {
> >      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 dies    = config->has_dies ? config->dies : 0;
> >      unsigned cores   = config->has_cores ? config->cores : 0;
> >      unsigned threads = config->has_threads ? config->threads : 0;
> >      unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
> >  
> >      /* compute missing values, prefer sockets over cores over threads */
> > +    dies = dies > 0 ? dies : 1;
> > +
> >      if (cpus == 0 || sockets == 0) {
> >          cores = cores > 0 ? cores : 1;
> >          threads = threads > 0 ? threads : 1;
> > -- 
> > 2.19.1
> > 

On Mon, Jul 12, 2021 at 05:05:43PM +0200, Andrew Jones wrote:
> On Mon, Jul 12, 2021 at 05:04:04PM +0200, Andrew Jones wrote:
> > On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote:
> > > It's possible that dies parameter is explicitly specified as "dies=0"
> > > in the cmdline, if so we will wrongly calculate the other ommited
> > > parameters such as "sockets = maxcpus / (dies * cores * threads);"
> > > with a zeroed dies value.
> > > 
> > > So perform zero-check (default the value to 1 if zeroed) for -smp dies
> > > before using it to calculate other parameters.
> > 
> > OK, dies=0 may make some sense for a user that doesn't want to describe
> > dies.
> > 
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> But... this is the pc smp parse function, not the general one, so maybe my
> comment from patch 1 should apply here as well, which is, that dies=0
> should be an error rather than silently changed to dies=1.

Also, after reading Daniel's comment on a later patch, I think anything=0
should just be an error, even for the general parser.

Thanks,
drew

> 
> > 
> > > 
> > > Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc)
> > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > > ---
> > >  hw/i386/pc.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index a44511c937..93d1f12a49 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
> > >  {
> > >      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 dies    = config->has_dies ? config->dies : 0;
> > >      unsigned cores   = config->has_cores ? config->cores : 0;
> > >      unsigned threads = config->has_threads ? config->threads : 0;
> > >      unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
> > >  
> > >      /* compute missing values, prefer sockets over cores over threads */
> > > +    dies = dies > 0 ? dies : 1;
> > > +
> > >      if (cpus == 0 || sockets == 0) {
> > >          cores = cores > 0 ? cores : 1;
> > >          threads = threads > 0 ? threads : 1;
> > > -- 
> > > 2.19.1
> > > 

On 2021/7/12 23:27, Andrew Jones wrote:
> On Mon, Jul 12, 2021 at 05:05:43PM +0200, Andrew Jones wrote:
>> On Mon, Jul 12, 2021 at 05:04:04PM +0200, Andrew Jones wrote:
>>> On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote:
>>>> It's possible that dies parameter is explicitly specified as "dies=0"
>>>> in the cmdline, if so we will wrongly calculate the other ommited
>>>> parameters such as "sockets = maxcpus / (dies * cores * threads);"
>>>> with a zeroed dies value.
>>>>
>>>> So perform zero-check (default the value to 1 if zeroed) for -smp dies
>>>> before using it to calculate other parameters.
>>> OK, dies=0 may make some sense for a user that doesn't want to describe
>>> dies.
>>>
>>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> But... this is the pc smp parse function, not the general one, so maybe my
>> comment from patch 1 should apply here as well, which is, that dies=0
>> should be an error rather than silently changed to dies=1.
> Also, after reading Daniel's comment on a later patch, I think anything=0
> should just be an error, even for the general parser.
>
Ok, after reading comments in patch #1, I also agree with this solution.
We can reject the "anything = 0" configuration in advance in function
machine_smp_parse(). I will do it in v2.

BTW, I have converted smp_parse() into a generic helper allowing future
extension for new arch-specific parameters, as you have suggested.
So that we don't need to maintain duplicated parsing code any more.

Thanks,
Yanan
.
>
>>>> Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc)
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> ---
>>>>   hw/i386/pc.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index a44511c937..93d1f12a49 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>>>>   {
>>>>       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 dies    = config->has_dies ? config->dies : 0;
>>>>       unsigned cores   = config->has_cores ? config->cores : 0;
>>>>       unsigned threads = config->has_threads ? config->threads : 0;
>>>>       unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>>>>   
>>>>       /* compute missing values, prefer sockets over cores over threads */
>>>> +    dies = dies > 0 ? dies : 1;
>>>> +
>>>>       if (cpus == 0 || sockets == 0) {
>>>>           cores = cores > 0 ? cores : 1;
>>>>           threads = threads > 0 ? threads : 1;
>>>> -- 
>>>> 2.19.1
>>>>
> .