[v2] RISC-V multi-socket support

[PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology

Posted by Anup Patel 5 years, 5 months ago

When "sockets" sub-option of "-smp" option is not specified, the
smp_parse() function will assume one CPU per-socket and set the
number of sockets equal to number of CPUs.

This is counter-intuitive and we should allow machine emulation to
decide default number of sockets when "sockets" sub-option is not
specified. To achieve this, we add boolean flag sockets_specified
in struct CpuTopology which tells machine emulation whether the
"sockets" sub-option was specified in command-line.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
---
 hw/core/machine.c   | 2 ++
 include/hw/boards.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index bb3a7b18b1..fd5ef5a4bb 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -706,6 +706,8 @@ static void smp_parse(MachineState *ms, QemuOpts *opts)
         unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
         unsigned threads = qemu_opt_get_number(opts, "threads", 0);
 
+         ms->smp.sockets_specified = (sockets == 0) ? false : true;
+
         /* compute missing values, prefer sockets over cores over threads */
         if (cpus == 0 || sockets == 0) {
             cores = cores > 0 ? cores : 1;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 18815d9be2..59b28ada65 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -244,6 +244,7 @@ typedef struct DeviceMemoryState {
  * @cores: the number of cores in one package
  * @threads: the number of threads in one core
  * @sockets: the number of sockets on the machine
+ * @sockets_specified: the number of sockets were specified for the machine
  * @max_cpus: the maximum number of logical processors on the machine
  */
 typedef struct CpuTopology {
@@ -251,6 +252,7 @@ typedef struct CpuTopology {
     unsigned int cores;
     unsigned int threads;
     unsigned int sockets;
+    bool sockets_specified;
     unsigned int max_cpus;
 } CpuTopology;
 
-- 
2.25.1

Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology

Posted by Daniel P. Berrangé 5 years, 5 months ago

On Wed, May 27, 2020 at 11:12:22AM +0530, Anup Patel wrote:
> When "sockets" sub-option of "-smp" option is not specified, the
> smp_parse() function will assume one CPU per-socket and set the
> number of sockets equal to number of CPUs.
> 
> This is counter-intuitive and we should allow machine emulation to
> decide default number of sockets when "sockets" sub-option is not
> specified.

I don't agree with this.  Having the semantics of the -smp option
be the same across all targets/machines *is* intuitive.  Changing
semantics of -smp per-machine will create a worse experiance for
people configuring QEMU as the configuration will mean different
things depending on the machine choce.


>           To achieve this, we add boolean flag sockets_specified
> in struct CpuTopology which tells machine emulation whether the
> "sockets" sub-option was specified in command-line.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> ---
>  hw/core/machine.c   | 2 ++
>  include/hw/boards.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index bb3a7b18b1..fd5ef5a4bb 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -706,6 +706,8 @@ static void smp_parse(MachineState *ms, QemuOpts *opts)
>          unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
>          unsigned threads = qemu_opt_get_number(opts, "threads", 0);
>  
> +         ms->smp.sockets_specified = (sockets == 0) ? false : true;
> +
>          /* compute missing values, prefer sockets over cores over threads */
>          if (cpus == 0 || sockets == 0) {
>              cores = cores > 0 ? cores : 1;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 18815d9be2..59b28ada65 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -244,6 +244,7 @@ typedef struct DeviceMemoryState {
>   * @cores: the number of cores in one package
>   * @threads: the number of threads in one core
>   * @sockets: the number of sockets on the machine
> + * @sockets_specified: the number of sockets were specified for the machine
>   * @max_cpus: the maximum number of logical processors on the machine
>   */
>  typedef struct CpuTopology {
> @@ -251,6 +252,7 @@ typedef struct CpuTopology {
>      unsigned int cores;
>      unsigned int threads;
>      unsigned int sockets;
> +    bool sockets_specified;
>      unsigned int max_cpus;
>  } CpuTopology;
>  
> -- 
> 2.25.1
> 
> 

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 :|

RE: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology

Posted by Anup Patel 5 years, 5 months ago


> -----Original Message-----
> From: Daniel P. Berrangé <berrange@redhat.com>
> Sent: 27 May 2020 14:16
> To: Anup Patel <Anup.Patel@wdc.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com>; Peter Maydell <peter.maydell@linaro.org>;
> Palmer Dabbelt <palmer@dabbelt.com>; Alistair Francis
> <Alistair.Francis@wdc.com>; Sagar Karandikar <sagark@eecs.berkeley.edu>;
> Atish Patra <Atish.Patra@wdc.com>; qemu-riscv@nongnu.org; qemu-
> devel@nongnu.org; Anup Patel <anup@brainfault.org>
> Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology
> 
> On Wed, May 27, 2020 at 11:12:22AM +0530, Anup Patel wrote:
> > When "sockets" sub-option of "-smp" option is not specified, the
> > smp_parse() function will assume one CPU per-socket and set the number
> > of sockets equal to number of CPUs.
> >
> > This is counter-intuitive and we should allow machine emulation to
> > decide default number of sockets when "sockets" sub-option is not
> > specified.
> 
> I don't agree with this.  Having the semantics of the -smp option be the same
> across all targets/machines *is* intuitive.  Changing semantics of -smp per-
> machine will create a worse experiance for people configuring QEMU as the
> configuration will mean different things depending on the machine choce.

Okay then why don't we default to sockets=1 in smp_parse() when "sockets"
sub-options is not specified ?? This will make it uniform across machines.

Is there a reason to by default have sockets=max_cpus ??

Regards,
Anup

> 
> 
> >           To achieve this, we add boolean flag sockets_specified in
> > struct CpuTopology which tells machine emulation whether the "sockets"
> > sub-option was specified in command-line.
> >
> > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > ---
> >  hw/core/machine.c   | 2 ++
> >  include/hw/boards.h | 2 ++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c index
> > bb3a7b18b1..fd5ef5a4bb 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -706,6 +706,8 @@ static void smp_parse(MachineState *ms, QemuOpts
> *opts)
> >          unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
> >          unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> >
> > +         ms->smp.sockets_specified = (sockets == 0) ? false : true;
> > +
> >          /* compute missing values, prefer sockets over cores over threads */
> >          if (cpus == 0 || sockets == 0) {
> >              cores = cores > 0 ? cores : 1; diff --git
> > a/include/hw/boards.h b/include/hw/boards.h index
> > 18815d9be2..59b28ada65 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -244,6 +244,7 @@ typedef struct DeviceMemoryState {
> >   * @cores: the number of cores in one package
> >   * @threads: the number of threads in one core
> >   * @sockets: the number of sockets on the machine
> > + * @sockets_specified: the number of sockets were specified for the
> > + machine
> >   * @max_cpus: the maximum number of logical processors on the machine
> >   */
> >  typedef struct CpuTopology {
> > @@ -251,6 +252,7 @@ typedef struct CpuTopology {
> >      unsigned int cores;
> >      unsigned int threads;
> >      unsigned int sockets;
> > +    bool sockets_specified;
> >      unsigned int max_cpus;
> >  } CpuTopology;
> >
> > --
> > 2.25.1
> >
> >
> 
> 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 :|

Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology

Posted by Daniel P. Berrangé 5 years, 5 months ago

On Wed, May 27, 2020 at 09:48:39AM +0000, Anup Patel wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel P. Berrangé <berrange@redhat.com>
> > Sent: 27 May 2020 14:16
> > To: Anup Patel <Anup.Patel@wdc.com>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum
> > <marcel.apfelbaum@gmail.com>; Peter Maydell <peter.maydell@linaro.org>;
> > Palmer Dabbelt <palmer@dabbelt.com>; Alistair Francis
> > <Alistair.Francis@wdc.com>; Sagar Karandikar <sagark@eecs.berkeley.edu>;
> > Atish Patra <Atish.Patra@wdc.com>; qemu-riscv@nongnu.org; qemu-
> > devel@nongnu.org; Anup Patel <anup@brainfault.org>
> > Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology
> > 
> > On Wed, May 27, 2020 at 11:12:22AM +0530, Anup Patel wrote:
> > > When "sockets" sub-option of "-smp" option is not specified, the
> > > smp_parse() function will assume one CPU per-socket and set the number
> > > of sockets equal to number of CPUs.
> > >
> > > This is counter-intuitive and we should allow machine emulation to
> > > decide default number of sockets when "sockets" sub-option is not
> > > specified.
> > 
> > I don't agree with this.  Having the semantics of the -smp option be the same
> > across all targets/machines *is* intuitive.  Changing semantics of -smp per-
> > machine will create a worse experiance for people configuring QEMU as the
> > configuration will mean different things depending on the machine choce.
> 
> Okay then why don't we default to sockets=1 in smp_parse() when "sockets"
> sub-options is not specified ?? This will make it uniform across machines.
> 
> Is there a reason to by default have sockets=max_cpus ??

IIUC both of these questions are due to backwards compatibility with
pre-existing QEMU versions.


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 :|

RE: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology

Posted by Anup Patel 5 years, 5 months ago


> -----Original Message-----
> From: Daniel P. Berrangé <berrange@redhat.com>
> Sent: 27 May 2020 15:21
> To: Anup Patel <Anup.Patel@wdc.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com>; Peter Maydell <peter.maydell@linaro.org>;
> Palmer Dabbelt <palmer@dabbelt.com>; Alistair Francis
> <Alistair.Francis@wdc.com>; Sagar Karandikar <sagark@eecs.berkeley.edu>;
> Atish Patra <Atish.Patra@wdc.com>; qemu-riscv@nongnu.org; qemu-
> devel@nongnu.org; Anup Patel <anup@brainfault.org>
> Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology
> 
> On Wed, May 27, 2020 at 09:48:39AM +0000, Anup Patel wrote:
> >
> >
> > > -----Original Message-----
> > > From: Daniel P. Berrangé <berrange@redhat.com>
> > > Sent: 27 May 2020 14:16
> > > To: Anup Patel <Anup.Patel@wdc.com>
> > > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum
> > > <marcel.apfelbaum@gmail.com>; Peter Maydell
> > > <peter.maydell@linaro.org>; Palmer Dabbelt <palmer@dabbelt.com>;
> > > Alistair Francis <Alistair.Francis@wdc.com>; Sagar Karandikar
> > > <sagark@eecs.berkeley.edu>; Atish Patra <Atish.Patra@wdc.com>;
> > > qemu-riscv@nongnu.org; qemu- devel@nongnu.org; Anup Patel
> > > <anup@brainfault.org>
> > > Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in
> > > CpuTopology
> > >
> > > On Wed, May 27, 2020 at 11:12:22AM +0530, Anup Patel wrote:
> > > > When "sockets" sub-option of "-smp" option is not specified, the
> > > > smp_parse() function will assume one CPU per-socket and set the
> > > > number of sockets equal to number of CPUs.
> > > >
> > > > This is counter-intuitive and we should allow machine emulation to
> > > > decide default number of sockets when "sockets" sub-option is not
> > > > specified.
> > >
> > > I don't agree with this.  Having the semantics of the -smp option be
> > > the same across all targets/machines *is* intuitive.  Changing
> > > semantics of -smp per- machine will create a worse experiance for
> > > people configuring QEMU as the configuration will mean different things
> depending on the machine choce.
> >
> > Okay then why don't we default to sockets=1 in smp_parse() when "sockets"
> > sub-options is not specified ?? This will make it uniform across machines.
> >
> > Is there a reason to by default have sockets=max_cpus ??
> 
> IIUC both of these questions are due to backwards compatibility with pre-
> existing QEMU versions.

I see that hw/x86/pc.c implements it's own smp_parse() callback.
Can we take that route ??

We need to have sockets=1 by default for RISC-V machines because
each socket has it's own interrupt controller and other peripherals.

Regards,
Anup

Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology

Posted by Daniel P. Berrangé 5 years, 5 months ago

On Wed, May 27, 2020 at 10:01:21AM +0000, Anup Patel wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel P. Berrangé <berrange@redhat.com>
> > Sent: 27 May 2020 15:21
> > To: Anup Patel <Anup.Patel@wdc.com>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum
> > <marcel.apfelbaum@gmail.com>; Peter Maydell <peter.maydell@linaro.org>;
> > Palmer Dabbelt <palmer@dabbelt.com>; Alistair Francis
> > <Alistair.Francis@wdc.com>; Sagar Karandikar <sagark@eecs.berkeley.edu>;
> > Atish Patra <Atish.Patra@wdc.com>; qemu-riscv@nongnu.org; qemu-
> > devel@nongnu.org; Anup Patel <anup@brainfault.org>
> > Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology
> > 
> > On Wed, May 27, 2020 at 09:48:39AM +0000, Anup Patel wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Daniel P. Berrangé <berrange@redhat.com>
> > > > Sent: 27 May 2020 14:16
> > > > To: Anup Patel <Anup.Patel@wdc.com>
> > > > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum
> > > > <marcel.apfelbaum@gmail.com>; Peter Maydell
> > > > <peter.maydell@linaro.org>; Palmer Dabbelt <palmer@dabbelt.com>;
> > > > Alistair Francis <Alistair.Francis@wdc.com>; Sagar Karandikar
> > > > <sagark@eecs.berkeley.edu>; Atish Patra <Atish.Patra@wdc.com>;
> > > > qemu-riscv@nongnu.org; qemu- devel@nongnu.org; Anup Patel
> > > > <anup@brainfault.org>
> > > > Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in
> > > > CpuTopology
> > > >
> > > > On Wed, May 27, 2020 at 11:12:22AM +0530, Anup Patel wrote:
> > > > > When "sockets" sub-option of "-smp" option is not specified, the
> > > > > smp_parse() function will assume one CPU per-socket and set the
> > > > > number of sockets equal to number of CPUs.
> > > > >
> > > > > This is counter-intuitive and we should allow machine emulation to
> > > > > decide default number of sockets when "sockets" sub-option is not
> > > > > specified.
> > > >
> > > > I don't agree with this.  Having the semantics of the -smp option be
> > > > the same across all targets/machines *is* intuitive.  Changing
> > > > semantics of -smp per- machine will create a worse experiance for
> > > > people configuring QEMU as the configuration will mean different things
> > depending on the machine choce.
> > >
> > > Okay then why don't we default to sockets=1 in smp_parse() when "sockets"
> > > sub-options is not specified ?? This will make it uniform across machines.
> > >
> > > Is there a reason to by default have sockets=max_cpus ??
> > 
> > IIUC both of these questions are due to backwards compatibility with pre-
> > existing QEMU versions.
> 
> I see that hw/x86/pc.c implements it's own smp_parse() callback.
> Can we take that route ??
> 
> We need to have sockets=1 by default for RISC-V machines because
> each socket has it's own interrupt controller and other peripherals.

I guess the fact that smp_parse() exists as a callback pretty much
says that allowing machine type overrides of default semantics is
permitted. So yeah, using a smp_parse callback seems reasonable.

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 :|

RE: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology

Posted by Anup Patel 5 years, 5 months ago


> -----Original Message-----
> From: Daniel P. Berrangé <berrange@redhat.com>
> Sent: 27 May 2020 16:12
> To: Anup Patel <Anup.Patel@wdc.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>; qemu-riscv@nongnu.org;
> Eduardo Habkost <ehabkost@redhat.com>; Sagar Karandikar
> <sagark@eecs.berkeley.edu>; Anup Patel <anup@brainfault.org>; qemu-
> devel@nongnu.org; Atish Patra <Atish.Patra@wdc.com>; Alistair Francis
> <Alistair.Francis@wdc.com>; Palmer Dabbelt <palmer@dabbelt.com>
> Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology
> 
> On Wed, May 27, 2020 at 10:01:21AM +0000, Anup Patel wrote:
> >
> >
> > > -----Original Message-----
> > > From: Daniel P. Berrangé <berrange@redhat.com>
> > > Sent: 27 May 2020 15:21
> > > To: Anup Patel <Anup.Patel@wdc.com>
> > > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum
> > > <marcel.apfelbaum@gmail.com>; Peter Maydell
> > > <peter.maydell@linaro.org>; Palmer Dabbelt <palmer@dabbelt.com>;
> > > Alistair Francis <Alistair.Francis@wdc.com>; Sagar Karandikar
> > > <sagark@eecs.berkeley.edu>; Atish Patra <Atish.Patra@wdc.com>;
> > > qemu-riscv@nongnu.org; qemu- devel@nongnu.org; Anup Patel
> > > <anup@brainfault.org>
> > > Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in
> > > CpuTopology
> > >
> > > On Wed, May 27, 2020 at 09:48:39AM +0000, Anup Patel wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Daniel P. Berrangé <berrange@redhat.com>
> > > > > Sent: 27 May 2020 14:16
> > > > > To: Anup Patel <Anup.Patel@wdc.com>
> > > > > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum
> > > > > <marcel.apfelbaum@gmail.com>; Peter Maydell
> > > > > <peter.maydell@linaro.org>; Palmer Dabbelt <palmer@dabbelt.com>;
> > > > > Alistair Francis <Alistair.Francis@wdc.com>; Sagar Karandikar
> > > > > <sagark@eecs.berkeley.edu>; Atish Patra <Atish.Patra@wdc.com>;
> > > > > qemu-riscv@nongnu.org; qemu- devel@nongnu.org; Anup Patel
> > > > > <anup@brainfault.org>
> > > > > Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in
> > > > > CpuTopology
> > > > >
> > > > > On Wed, May 27, 2020 at 11:12:22AM +0530, Anup Patel wrote:
> > > > > > When "sockets" sub-option of "-smp" option is not specified,
> > > > > > the
> > > > > > smp_parse() function will assume one CPU per-socket and set
> > > > > > the number of sockets equal to number of CPUs.
> > > > > >
> > > > > > This is counter-intuitive and we should allow machine
> > > > > > emulation to decide default number of sockets when "sockets"
> > > > > > sub-option is not specified.
> > > > >
> > > > > I don't agree with this.  Having the semantics of the -smp
> > > > > option be the same across all targets/machines *is* intuitive.
> > > > > Changing semantics of -smp per- machine will create a worse
> > > > > experiance for people configuring QEMU as the configuration will
> > > > > mean different things
> > > depending on the machine choce.
> > > >
> > > > Okay then why don't we default to sockets=1 in smp_parse() when
> "sockets"
> > > > sub-options is not specified ?? This will make it uniform across machines.
> > > >
> > > > Is there a reason to by default have sockets=max_cpus ??
> > >
> > > IIUC both of these questions are due to backwards compatibility with
> > > pre- existing QEMU versions.
> >
> > I see that hw/x86/pc.c implements it's own smp_parse() callback.
> > Can we take that route ??
> >
> > We need to have sockets=1 by default for RISC-V machines because each
> > socket has it's own interrupt controller and other peripherals.
> 
> I guess the fact that smp_parse() exists as a callback pretty much says that
> allowing machine type overrides of default semantics is permitted. So yeah,
> using a smp_parse callback seems reasonable.

Thanks Danie, I will drop this patch and send v3 with different approach.

Regards,
Anup