Re: [Qemu-devel] [PATCH RFC 05/16] hw/core/machine: add smp properites

[Andrew Jones]

On Thu, Jul 14, 2016 at 05:18:20PM -0300, Eduardo Habkost wrote:
> On Wed, Jun 15, 2016 at 09:11:13AM +0200, Andrew Jones wrote:
> > On Wed, Jun 15, 2016 at 10:37:59AM +1000, David Gibson wrote:
> > > On Tue, Jun 14, 2016 at 08:08:07AM +0200, Andrew Jones wrote:
> > > > On Tue, Jun 14, 2016 at 12:00:26PM +1000, David Gibson wrote:
> > > > > On Fri, Jun 10, 2016 at 07:40:16PM +0200, Andrew Jones wrote:
> > > > > > Signed-off-by: Andrew Jones 
> > > > > > ---
> > > > > >  hw/core/machine.c   | 81 
> > > > > > +
> > > > > >  include/hw/boards.h |  6 
> > > > > >  2 files changed, 87 insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > > index 3dce9020e510a..2625044002e57 100644
> > > > > > --- a/hw/core/machine.c
> > > > > > +++ b/hw/core/machine.c
> > > > > > @@ -172,6 +172,53 @@ static void machine_set_dumpdtb(Object *obj, 
> > > > > > const char *value, Error **errp)
> > > > > >  ms->dumpdtb = g_strdup(value);
> > > > > >  }
> > > > > >  
> > > > > > +static void machine_get_smp(Object *obj, Visitor *v, const char 
> > > > > > *name,
> > > > > > +void *opaque, Error **errp)
> > > > > > +{
> > > > > > +MachineState *ms = MACHINE(obj);
> > > > > > +int64_t value;
> > > > > > +
> > > > > > +if (strncmp(name, "sockets", 7) == 0) {
> > > > > > +value = ms->sockets;
> > > > > > +} else if (strncmp(name, "cores", 5) == 0) {
> > > > > > +value = ms->cores;
> > > > > > +} else if (strncmp(name, "threads", 7) == 0) {
> > > > > > +value = ms->threads;
> > > > > > +} else if (strncmp(name, "maxcpus", 7) == 0) {
> > > > > > +value = ms->maxcpus;
> > > > > > +} else if (strncmp(name, "cpus", 4) == 0) {
> > > > > > +value = ms->cpus;
> > > > > > +}
> > > > > > +
> > > > > > +visit_type_int(v, name, , errp);
> > > > > > +}
> > > > > 
> > > > > Any particular for multiplexing all the set / get, rather than having
> > > > > separate callbacks for each property?
> > > > 
> > > > Not really. This way just makes denser code. But I'll go whichever
> > > > direction people prefer.
> > > 
> > > I'd prefer not to have the multiplexer, but I don't care that much.
> > > 
> > > > Actually I should probably add an
> > > > else { error_report(...) } in either case, which means the multifunction
> > > > direction would still contain strncmps.
> > > 
> > > Hrm.. that would seem an odd choice to me if you didn't have the
> > > multiplex.  Not including the strncmp() means you can change the
> > > property name (or add aliases) in a single place, without changing the
> > > callback functions.
> > > 
> > > Note also that the current set of strncmp()s is only correct because
> > > there is a limited set of things bound to this callback.  Remember
> > > that strncmp(name, "sockets", 7) will match "socketsfoo".
> > 
> > Good point. I'll change to multiple functions and just drop the
> > strncmps.
> 
> Multiple functions that just get/set struct fields would be
> easier to convert to use a better property API (that don't
> require writing getter/setters) in the future.
> 
> [...]
> > > > > > +
> > > > > > +int sockets;
> > > > > > +int cores;
> > > > > > +int threads;
> > > > > > +int maxcpus;
> > > > > > +int cpus;
> > > > > 
> > > > > Hrm.. as the tests added in earlier patches highlight, essentially one
> > > > > of these properties is redundant.  Is there a reason to include them
> > > > > all, rather than to pick one to drop?
> > > > 
> > > > Well, IMO, only maxcpus could be dropped. I'd prefer not to though
> > > > because it's a convenient state to have pre-calculated, and possibly
> > > > error prone to leave to all users to re-calculate.
> > > 
> > > Sorry, to clarify.  I have no problem with having all 5 variables,
> > > with one precalculated from the others.  What I'm not convinced is a
> > > good idea is exposing all 5 as settable properties.
> > 
> > I see. I think we need them all, especially as we decide which ones
> > can be optional, if given others. For example we need cpus and maxcpus
> > for obvious reasons. threads can be optional (default =1), but if you
> > want to specify more than 1 then we need the property. Currently I
> > think both sockets and cores should be required since we don't know
> > which should be calculated form the other, and therefore can't set
> > a default for either. That's all five.
> 
> If we were designing a new interface, I would prefer to make it
> different, and add properties only to machine classes that really
> make this configurable. But as we are just moving existing data
> and logic from vl.c into MachineState fields, I agree we need all
> the struct fields, so the existing vl.c code can be easily moved
> to machine.c.
> 
> But:
> 
> About the new externally-visible interface: what about we
> register the properties only on the machine 

Re: [Qemu-devel] [PATCH RFC 05/16] hw/core/machine: add smp properites

[Eduardo Habkost]

On Wed, Jun 15, 2016 at 09:11:13AM +0200, Andrew Jones wrote:
> On Wed, Jun 15, 2016 at 10:37:59AM +1000, David Gibson wrote:
> > On Tue, Jun 14, 2016 at 08:08:07AM +0200, Andrew Jones wrote:
> > > On Tue, Jun 14, 2016 at 12:00:26PM +1000, David Gibson wrote:
> > > > On Fri, Jun 10, 2016 at 07:40:16PM +0200, Andrew Jones wrote:
> > > > > Signed-off-by: Andrew Jones 
> > > > > ---
> > > > >  hw/core/machine.c   | 81 
> > > > > +
> > > > >  include/hw/boards.h |  6 
> > > > >  2 files changed, 87 insertions(+)
> > > > > 
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index 3dce9020e510a..2625044002e57 100644
> > > > > --- a/hw/core/machine.c
> > > > > +++ b/hw/core/machine.c
> > > > > @@ -172,6 +172,53 @@ static void machine_set_dumpdtb(Object *obj, 
> > > > > const char *value, Error **errp)
> > > > >  ms->dumpdtb = g_strdup(value);
> > > > >  }
> > > > >  
> > > > > +static void machine_get_smp(Object *obj, Visitor *v, const char 
> > > > > *name,
> > > > > +void *opaque, Error **errp)
> > > > > +{
> > > > > +MachineState *ms = MACHINE(obj);
> > > > > +int64_t value;
> > > > > +
> > > > > +if (strncmp(name, "sockets", 7) == 0) {
> > > > > +value = ms->sockets;
> > > > > +} else if (strncmp(name, "cores", 5) == 0) {
> > > > > +value = ms->cores;
> > > > > +} else if (strncmp(name, "threads", 7) == 0) {
> > > > > +value = ms->threads;
> > > > > +} else if (strncmp(name, "maxcpus", 7) == 0) {
> > > > > +value = ms->maxcpus;
> > > > > +} else if (strncmp(name, "cpus", 4) == 0) {
> > > > > +value = ms->cpus;
> > > > > +}
> > > > > +
> > > > > +visit_type_int(v, name, , errp);
> > > > > +}
> > > > 
> > > > Any particular for multiplexing all the set / get, rather than having
> > > > separate callbacks for each property?
> > > 
> > > Not really. This way just makes denser code. But I'll go whichever
> > > direction people prefer.
> > 
> > I'd prefer not to have the multiplexer, but I don't care that much.
> > 
> > > Actually I should probably add an
> > > else { error_report(...) } in either case, which means the multifunction
> > > direction would still contain strncmps.
> > 
> > Hrm.. that would seem an odd choice to me if you didn't have the
> > multiplex.  Not including the strncmp() means you can change the
> > property name (or add aliases) in a single place, without changing the
> > callback functions.
> > 
> > Note also that the current set of strncmp()s is only correct because
> > there is a limited set of things bound to this callback.  Remember
> > that strncmp(name, "sockets", 7) will match "socketsfoo".
> 
> Good point. I'll change to multiple functions and just drop the
> strncmps.

Multiple functions that just get/set struct fields would be
easier to convert to use a better property API (that don't
require writing getter/setters) in the future.

[...]
> > > > > +
> > > > > +int sockets;
> > > > > +int cores;
> > > > > +int threads;
> > > > > +int maxcpus;
> > > > > +int cpus;
> > > > 
> > > > Hrm.. as the tests added in earlier patches highlight, essentially one
> > > > of these properties is redundant.  Is there a reason to include them
> > > > all, rather than to pick one to drop?
> > > 
> > > Well, IMO, only maxcpus could be dropped. I'd prefer not to though
> > > because it's a convenient state to have pre-calculated, and possibly
> > > error prone to leave to all users to re-calculate.
> > 
> > Sorry, to clarify.  I have no problem with having all 5 variables,
> > with one precalculated from the others.  What I'm not convinced is a
> > good idea is exposing all 5 as settable properties.
> 
> I see. I think we need them all, especially as we decide which ones
> can be optional, if given others. For example we need cpus and maxcpus
> for obvious reasons. threads can be optional (default =1), but if you
> want to specify more than 1 then we need the property. Currently I
> think both sockets and cores should be required since we don't know
> which should be calculated form the other, and therefore can't set
> a default for either. That's all five.

If we were designing a new interface, I would prefer to make it
different, and add properties only to machine classes that really
make this configurable. But as we are just moving existing data
and logic from vl.c into MachineState fields, I agree we need all
the struct fields, so the existing vl.c code can be easily moved
to machine.c.

But:

About the new externally-visible interface: what about we
register the properties only on the machine subclasses that
really do something with those options? Then we won't need to
worry about keeping compatibility in machines that never
supported multi-core/multi-thread configurations in the first
place.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 05/16] hw/core/machine: add smp properites

[Andrew Jones]

On Wed, Jun 15, 2016 at 10:37:59AM +1000, David Gibson wrote:
> On Tue, Jun 14, 2016 at 08:08:07AM +0200, Andrew Jones wrote:
> > On Tue, Jun 14, 2016 at 12:00:26PM +1000, David Gibson wrote:
> > > On Fri, Jun 10, 2016 at 07:40:16PM +0200, Andrew Jones wrote:
> > > > Signed-off-by: Andrew Jones 
> > > > ---
> > > >  hw/core/machine.c   | 81 
> > > > +
> > > >  include/hw/boards.h |  6 
> > > >  2 files changed, 87 insertions(+)
> > > > 
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index 3dce9020e510a..2625044002e57 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -172,6 +172,53 @@ static void machine_set_dumpdtb(Object *obj, const 
> > > > char *value, Error **errp)
> > > >  ms->dumpdtb = g_strdup(value);
> > > >  }
> > > >  
> > > > +static void machine_get_smp(Object *obj, Visitor *v, const char *name,
> > > > +void *opaque, Error **errp)
> > > > +{
> > > > +MachineState *ms = MACHINE(obj);
> > > > +int64_t value;
> > > > +
> > > > +if (strncmp(name, "sockets", 7) == 0) {
> > > > +value = ms->sockets;
> > > > +} else if (strncmp(name, "cores", 5) == 0) {
> > > > +value = ms->cores;
> > > > +} else if (strncmp(name, "threads", 7) == 0) {
> > > > +value = ms->threads;
> > > > +} else if (strncmp(name, "maxcpus", 7) == 0) {
> > > > +value = ms->maxcpus;
> > > > +} else if (strncmp(name, "cpus", 4) == 0) {
> > > > +value = ms->cpus;
> > > > +}
> > > > +
> > > > +visit_type_int(v, name, , errp);
> > > > +}
> > > 
> > > Any particular for multiplexing all the set / get, rather than having
> > > separate callbacks for each property?
> > 
> > Not really. This way just makes denser code. But I'll go whichever
> > direction people prefer.
> 
> I'd prefer not to have the multiplexer, but I don't care that much.
> 
> > Actually I should probably add an
> > else { error_report(...) } in either case, which means the multifunction
> > direction would still contain strncmps.
> 
> Hrm.. that would seem an odd choice to me if you didn't have the
> multiplex.  Not including the strncmp() means you can change the
> property name (or add aliases) in a single place, without changing the
> callback functions.
> 
> Note also that the current set of strncmp()s is only correct because
> there is a limited set of things bound to this callback.  Remember
> that strncmp(name, "sockets", 7) will match "socketsfoo".

Good point. I'll change to multiple functions and just drop the
strncmps.

> 
> > > > +
> > > > +static void machine_set_smp(Object *obj, Visitor *v, const char *name,
> > > > +void *opaque, Error **errp)
> > > > +{
> > > > +MachineState *ms = MACHINE(obj);
> > > > +Error *error = NULL;
> > > > +int64_t value;
> > > > +
> > > > +visit_type_int(v, name, , );
> > > > +if (error) {
> > > > +error_propagate(errp, error);
> > > > +return;
> > > > +}
> > > > +
> > > > +if (strncmp(name, "sockets", 7) == 0) {
> > > > +ms->sockets = value;
> > > > +} else if (strncmp(name, "cores", 5) == 0) {
> > > > +ms->cores = value;;
> > > > +} else if (strncmp(name, "threads", 7) == 0) {
> > > > +ms->threads = value;
> > > > +} else if (strncmp(name, "maxcpus", 7) == 0) {
> > > > +ms->maxcpus = value;
> > > > +} else if (strncmp(name, "cpus", 4) == 0) {
> > > > +ms->cpus = value;
> > > > +}
> > > > +}
> > > > +
> > > >  static void machine_get_phandle_start(Object *obj, Visitor *v,
> > > >const char *name, void *opaque,
> > > >Error **errp)
> > > > @@ -368,8 +415,18 @@ static void machine_init_notify(Notifier 
> > > > *notifier, void *data)
> > > >  foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
> > > >  }
> > > >  
> > > > +static void machine_set_smp_parameters(MachineState *ms)
> > > > +{
> > > > +if (ms->sockets != -1 || ms->cores != -1 || ms->threads != -1 ||
> > > > +ms->maxcpus != -1 || ms->cpus != -1) {
> > > > +error_report("warning: cpu topology: "
> > > > + "machine properties currently ignored");
> > > > +}
> > > > +}
> > > > +
> > > >  static void machine_pre_init(MachineState *ms)
> > > >  {
> > > > +machine_set_smp_parameters(ms);
> > > >  }
> > > >  
> > > >  static void machine_class_init(ObjectClass *oc, void *data)
> > > > @@ -403,6 +460,11 @@ static void machine_initfn(Object *obj)
> > > >  ms->dump_guest_core = true;
> > > >  ms->mem_merge = true;
> > > >  ms->enable_graphics = true;
> > > > +ms->sockets = -1;
> > > > +ms->cores = -1;
> > > > +ms->threads = -1;
> > > > +ms->maxcpus = -1;
> > > > +ms->cpus = -1;
> > > >  
> > > >  object_property_add_str(obj, "accel",
> > > >

Re: [Qemu-devel] [PATCH RFC 05/16] hw/core/machine: add smp properites

[David Gibson]

On Tue, Jun 14, 2016 at 08:08:07AM +0200, Andrew Jones wrote:
> On Tue, Jun 14, 2016 at 12:00:26PM +1000, David Gibson wrote:
> > On Fri, Jun 10, 2016 at 07:40:16PM +0200, Andrew Jones wrote:
> > > Signed-off-by: Andrew Jones 
> > > ---
> > >  hw/core/machine.c   | 81 
> > > +
> > >  include/hw/boards.h |  6 
> > >  2 files changed, 87 insertions(+)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index 3dce9020e510a..2625044002e57 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -172,6 +172,53 @@ static void machine_set_dumpdtb(Object *obj, const 
> > > char *value, Error **errp)
> > >  ms->dumpdtb = g_strdup(value);
> > >  }
> > >  
> > > +static void machine_get_smp(Object *obj, Visitor *v, const char *name,
> > > +void *opaque, Error **errp)
> > > +{
> > > +MachineState *ms = MACHINE(obj);
> > > +int64_t value;
> > > +
> > > +if (strncmp(name, "sockets", 7) == 0) {
> > > +value = ms->sockets;
> > > +} else if (strncmp(name, "cores", 5) == 0) {
> > > +value = ms->cores;
> > > +} else if (strncmp(name, "threads", 7) == 0) {
> > > +value = ms->threads;
> > > +} else if (strncmp(name, "maxcpus", 7) == 0) {
> > > +value = ms->maxcpus;
> > > +} else if (strncmp(name, "cpus", 4) == 0) {
> > > +value = ms->cpus;
> > > +}
> > > +
> > > +visit_type_int(v, name, , errp);
> > > +}
> > 
> > Any particular for multiplexing all the set / get, rather than having
> > separate callbacks for each property?
> 
> Not really. This way just makes denser code. But I'll go whichever
> direction people prefer.

I'd prefer not to have the multiplexer, but I don't care that much.

> Actually I should probably add an
> else { error_report(...) } in either case, which means the multifunction
> direction would still contain strncmps.

Hrm.. that would seem an odd choice to me if you didn't have the
multiplex.  Not including the strncmp() means you can change the
property name (or add aliases) in a single place, without changing the
callback functions.

Note also that the current set of strncmp()s is only correct because
there is a limited set of things bound to this callback.  Remember
that strncmp(name, "sockets", 7) will match "socketsfoo".

> > > +
> > > +static void machine_set_smp(Object *obj, Visitor *v, const char *name,
> > > +void *opaque, Error **errp)
> > > +{
> > > +MachineState *ms = MACHINE(obj);
> > > +Error *error = NULL;
> > > +int64_t value;
> > > +
> > > +visit_type_int(v, name, , );
> > > +if (error) {
> > > +error_propagate(errp, error);
> > > +return;
> > > +}
> > > +
> > > +if (strncmp(name, "sockets", 7) == 0) {
> > > +ms->sockets = value;
> > > +} else if (strncmp(name, "cores", 5) == 0) {
> > > +ms->cores = value;;
> > > +} else if (strncmp(name, "threads", 7) == 0) {
> > > +ms->threads = value;
> > > +} else if (strncmp(name, "maxcpus", 7) == 0) {
> > > +ms->maxcpus = value;
> > > +} else if (strncmp(name, "cpus", 4) == 0) {
> > > +ms->cpus = value;
> > > +}
> > > +}
> > > +
> > >  static void machine_get_phandle_start(Object *obj, Visitor *v,
> > >const char *name, void *opaque,
> > >Error **errp)
> > > @@ -368,8 +415,18 @@ static void machine_init_notify(Notifier *notifier, 
> > > void *data)
> > >  foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
> > >  }
> > >  
> > > +static void machine_set_smp_parameters(MachineState *ms)
> > > +{
> > > +if (ms->sockets != -1 || ms->cores != -1 || ms->threads != -1 ||
> > > +ms->maxcpus != -1 || ms->cpus != -1) {
> > > +error_report("warning: cpu topology: "
> > > + "machine properties currently ignored");
> > > +}
> > > +}
> > > +
> > >  static void machine_pre_init(MachineState *ms)
> > >  {
> > > +machine_set_smp_parameters(ms);
> > >  }
> > >  
> > >  static void machine_class_init(ObjectClass *oc, void *data)
> > > @@ -403,6 +460,11 @@ static void machine_initfn(Object *obj)
> > >  ms->dump_guest_core = true;
> > >  ms->mem_merge = true;
> > >  ms->enable_graphics = true;
> > > +ms->sockets = -1;
> > > +ms->cores = -1;
> > > +ms->threads = -1;
> > > +ms->maxcpus = -1;
> > > +ms->cpus = -1;
> > >  
> > >  object_property_add_str(obj, "accel",
> > >  machine_get_accel, machine_set_accel, NULL);
> > > @@ -462,6 +524,25 @@ static void machine_initfn(Object *obj)
> > >  object_property_set_description(obj, "dt-compatible",
> > >  "Overrides the \"compatible\" 
> > > property of the dt root node",
> > >  NULL);
> > > +

Re: [Qemu-devel] [PATCH RFC 05/16] hw/core/machine: add smp properites

[Andrew Jones]

On Tue, Jun 14, 2016 at 12:00:26PM +1000, David Gibson wrote:
> On Fri, Jun 10, 2016 at 07:40:16PM +0200, Andrew Jones wrote:
> > Signed-off-by: Andrew Jones 
> > ---
> >  hw/core/machine.c   | 81 
> > +
> >  include/hw/boards.h |  6 
> >  2 files changed, 87 insertions(+)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 3dce9020e510a..2625044002e57 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -172,6 +172,53 @@ static void machine_set_dumpdtb(Object *obj, const 
> > char *value, Error **errp)
> >  ms->dumpdtb = g_strdup(value);
> >  }
> >  
> > +static void machine_get_smp(Object *obj, Visitor *v, const char *name,
> > +void *opaque, Error **errp)
> > +{
> > +MachineState *ms = MACHINE(obj);
> > +int64_t value;
> > +
> > +if (strncmp(name, "sockets", 7) == 0) {
> > +value = ms->sockets;
> > +} else if (strncmp(name, "cores", 5) == 0) {
> > +value = ms->cores;
> > +} else if (strncmp(name, "threads", 7) == 0) {
> > +value = ms->threads;
> > +} else if (strncmp(name, "maxcpus", 7) == 0) {
> > +value = ms->maxcpus;
> > +} else if (strncmp(name, "cpus", 4) == 0) {
> > +value = ms->cpus;
> > +}
> > +
> > +visit_type_int(v, name, , errp);
> > +}
> 
> Any particular for multiplexing all the set / get, rather than having
> separate callbacks for each property?

Not really. This way just makes denser code. But I'll go whichever
direction people prefer. Actually I should probably add an
else { error_report(...) } in either case, which means the multifunction
direction would still contain strncmps.

> 
> > +
> > +static void machine_set_smp(Object *obj, Visitor *v, const char *name,
> > +void *opaque, Error **errp)
> > +{
> > +MachineState *ms = MACHINE(obj);
> > +Error *error = NULL;
> > +int64_t value;
> > +
> > +visit_type_int(v, name, , );
> > +if (error) {
> > +error_propagate(errp, error);
> > +return;
> > +}
> > +
> > +if (strncmp(name, "sockets", 7) == 0) {
> > +ms->sockets = value;
> > +} else if (strncmp(name, "cores", 5) == 0) {
> > +ms->cores = value;;
> > +} else if (strncmp(name, "threads", 7) == 0) {
> > +ms->threads = value;
> > +} else if (strncmp(name, "maxcpus", 7) == 0) {
> > +ms->maxcpus = value;
> > +} else if (strncmp(name, "cpus", 4) == 0) {
> > +ms->cpus = value;
> > +}
> > +}
> > +
> >  static void machine_get_phandle_start(Object *obj, Visitor *v,
> >const char *name, void *opaque,
> >Error **errp)
> > @@ -368,8 +415,18 @@ static void machine_init_notify(Notifier *notifier, 
> > void *data)
> >  foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
> >  }
> >  
> > +static void machine_set_smp_parameters(MachineState *ms)
> > +{
> > +if (ms->sockets != -1 || ms->cores != -1 || ms->threads != -1 ||
> > +ms->maxcpus != -1 || ms->cpus != -1) {
> > +error_report("warning: cpu topology: "
> > + "machine properties currently ignored");
> > +}
> > +}
> > +
> >  static void machine_pre_init(MachineState *ms)
> >  {
> > +machine_set_smp_parameters(ms);
> >  }
> >  
> >  static void machine_class_init(ObjectClass *oc, void *data)
> > @@ -403,6 +460,11 @@ static void machine_initfn(Object *obj)
> >  ms->dump_guest_core = true;
> >  ms->mem_merge = true;
> >  ms->enable_graphics = true;
> > +ms->sockets = -1;
> > +ms->cores = -1;
> > +ms->threads = -1;
> > +ms->maxcpus = -1;
> > +ms->cpus = -1;
> >  
> >  object_property_add_str(obj, "accel",
> >  machine_get_accel, machine_set_accel, NULL);
> > @@ -462,6 +524,25 @@ static void machine_initfn(Object *obj)
> >  object_property_set_description(obj, "dt-compatible",
> >  "Overrides the \"compatible\" property 
> > of the dt root node",
> >  NULL);
> > +object_property_add(obj, "sockets", "int", machine_get_smp,
> > +machine_set_smp, NULL, NULL, NULL);
> > +object_property_set_description(obj, "sockets", "Number of sockets", 
> > NULL);
> > +object_property_add(obj, "cores", "int", machine_get_smp,
> > +machine_set_smp, NULL, NULL, NULL);
> > +object_property_set_description(obj, "cores",
> > +"Number of cores per socket", NULL);
> > +object_property_add(obj, "threads", "int", machine_get_smp,
> > +machine_set_smp, NULL, NULL, NULL);
> > +object_property_set_description(obj, "threads",
> > +"Number of threads per core", NULL);
> > +object_property_add(obj, 

Re: [Qemu-devel] [PATCH RFC 05/16] hw/core/machine: add smp properites

[David Gibson]

On Fri, Jun 10, 2016 at 07:40:16PM +0200, Andrew Jones wrote:
> Signed-off-by: Andrew Jones 
> ---
>  hw/core/machine.c   | 81 
> +
>  include/hw/boards.h |  6 
>  2 files changed, 87 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 3dce9020e510a..2625044002e57 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -172,6 +172,53 @@ static void machine_set_dumpdtb(Object *obj, const char 
> *value, Error **errp)
>  ms->dumpdtb = g_strdup(value);
>  }
>  
> +static void machine_get_smp(Object *obj, Visitor *v, const char *name,
> +void *opaque, Error **errp)
> +{
> +MachineState *ms = MACHINE(obj);
> +int64_t value;
> +
> +if (strncmp(name, "sockets", 7) == 0) {
> +value = ms->sockets;
> +} else if (strncmp(name, "cores", 5) == 0) {
> +value = ms->cores;
> +} else if (strncmp(name, "threads", 7) == 0) {
> +value = ms->threads;
> +} else if (strncmp(name, "maxcpus", 7) == 0) {
> +value = ms->maxcpus;
> +} else if (strncmp(name, "cpus", 4) == 0) {
> +value = ms->cpus;
> +}
> +
> +visit_type_int(v, name, , errp);
> +}

Any particular for multiplexing all the set / get, rather than having
separate callbacks for each property?

> +
> +static void machine_set_smp(Object *obj, Visitor *v, const char *name,
> +void *opaque, Error **errp)
> +{
> +MachineState *ms = MACHINE(obj);
> +Error *error = NULL;
> +int64_t value;
> +
> +visit_type_int(v, name, , );
> +if (error) {
> +error_propagate(errp, error);
> +return;
> +}
> +
> +if (strncmp(name, "sockets", 7) == 0) {
> +ms->sockets = value;
> +} else if (strncmp(name, "cores", 5) == 0) {
> +ms->cores = value;;
> +} else if (strncmp(name, "threads", 7) == 0) {
> +ms->threads = value;
> +} else if (strncmp(name, "maxcpus", 7) == 0) {
> +ms->maxcpus = value;
> +} else if (strncmp(name, "cpus", 4) == 0) {
> +ms->cpus = value;
> +}
> +}
> +
>  static void machine_get_phandle_start(Object *obj, Visitor *v,
>const char *name, void *opaque,
>Error **errp)
> @@ -368,8 +415,18 @@ static void machine_init_notify(Notifier *notifier, void 
> *data)
>  foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
>  }
>  
> +static void machine_set_smp_parameters(MachineState *ms)
> +{
> +if (ms->sockets != -1 || ms->cores != -1 || ms->threads != -1 ||
> +ms->maxcpus != -1 || ms->cpus != -1) {
> +error_report("warning: cpu topology: "
> + "machine properties currently ignored");
> +}
> +}
> +
>  static void machine_pre_init(MachineState *ms)
>  {
> +machine_set_smp_parameters(ms);
>  }
>  
>  static void machine_class_init(ObjectClass *oc, void *data)
> @@ -403,6 +460,11 @@ static void machine_initfn(Object *obj)
>  ms->dump_guest_core = true;
>  ms->mem_merge = true;
>  ms->enable_graphics = true;
> +ms->sockets = -1;
> +ms->cores = -1;
> +ms->threads = -1;
> +ms->maxcpus = -1;
> +ms->cpus = -1;
>  
>  object_property_add_str(obj, "accel",
>  machine_get_accel, machine_set_accel, NULL);
> @@ -462,6 +524,25 @@ static void machine_initfn(Object *obj)
>  object_property_set_description(obj, "dt-compatible",
>  "Overrides the \"compatible\" property 
> of the dt root node",
>  NULL);
> +object_property_add(obj, "sockets", "int", machine_get_smp,
> +machine_set_smp, NULL, NULL, NULL);
> +object_property_set_description(obj, "sockets", "Number of sockets", 
> NULL);
> +object_property_add(obj, "cores", "int", machine_get_smp,
> +machine_set_smp, NULL, NULL, NULL);
> +object_property_set_description(obj, "cores",
> +"Number of cores per socket", NULL);
> +object_property_add(obj, "threads", "int", machine_get_smp,
> +machine_set_smp, NULL, NULL, NULL);
> +object_property_set_description(obj, "threads",
> +"Number of threads per core", NULL);
> +object_property_add(obj, "maxcpus", "int", machine_get_smp,
> +machine_set_smp, NULL, NULL, NULL);
> +object_property_set_description(obj, "maxcpus", "Maximum number of cpus",
> +NULL);
> +object_property_add(obj, "cpus", "int", machine_get_smp,
> +machine_set_smp, NULL, NULL, NULL);
> +object_property_set_description(obj, "cpus", "Number of online cpus",
> +NULL);
>  object_property_add_bool(obj, "dump-guest-core",
>   

[Qemu-devel] [PATCH RFC 05/16] hw/core/machine: add smp properites

[Andrew Jones]

Signed-off-by: Andrew Jones 
---
 hw/core/machine.c   | 81 +
 include/hw/boards.h |  6 
 2 files changed, 87 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 3dce9020e510a..2625044002e57 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -172,6 +172,53 @@ static void machine_set_dumpdtb(Object *obj, const char 
*value, Error **errp)
 ms->dumpdtb = g_strdup(value);
 }
 
+static void machine_get_smp(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+MachineState *ms = MACHINE(obj);
+int64_t value;
+
+if (strncmp(name, "sockets", 7) == 0) {
+value = ms->sockets;
+} else if (strncmp(name, "cores", 5) == 0) {
+value = ms->cores;
+} else if (strncmp(name, "threads", 7) == 0) {
+value = ms->threads;
+} else if (strncmp(name, "maxcpus", 7) == 0) {
+value = ms->maxcpus;
+} else if (strncmp(name, "cpus", 4) == 0) {
+value = ms->cpus;
+}
+
+visit_type_int(v, name, , errp);
+}
+
+static void machine_set_smp(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+MachineState *ms = MACHINE(obj);
+Error *error = NULL;
+int64_t value;
+
+visit_type_int(v, name, , );
+if (error) {
+error_propagate(errp, error);
+return;
+}
+
+if (strncmp(name, "sockets", 7) == 0) {
+ms->sockets = value;
+} else if (strncmp(name, "cores", 5) == 0) {
+ms->cores = value;;
+} else if (strncmp(name, "threads", 7) == 0) {
+ms->threads = value;
+} else if (strncmp(name, "maxcpus", 7) == 0) {
+ms->maxcpus = value;
+} else if (strncmp(name, "cpus", 4) == 0) {
+ms->cpus = value;
+}
+}
+
 static void machine_get_phandle_start(Object *obj, Visitor *v,
   const char *name, void *opaque,
   Error **errp)
@@ -368,8 +415,18 @@ static void machine_init_notify(Notifier *notifier, void 
*data)
 foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
 }
 
+static void machine_set_smp_parameters(MachineState *ms)
+{
+if (ms->sockets != -1 || ms->cores != -1 || ms->threads != -1 ||
+ms->maxcpus != -1 || ms->cpus != -1) {
+error_report("warning: cpu topology: "
+ "machine properties currently ignored");
+}
+}
+
 static void machine_pre_init(MachineState *ms)
 {
+machine_set_smp_parameters(ms);
 }
 
 static void machine_class_init(ObjectClass *oc, void *data)
@@ -403,6 +460,11 @@ static void machine_initfn(Object *obj)
 ms->dump_guest_core = true;
 ms->mem_merge = true;
 ms->enable_graphics = true;
+ms->sockets = -1;
+ms->cores = -1;
+ms->threads = -1;
+ms->maxcpus = -1;
+ms->cpus = -1;
 
 object_property_add_str(obj, "accel",
 machine_get_accel, machine_set_accel, NULL);
@@ -462,6 +524,25 @@ static void machine_initfn(Object *obj)
 object_property_set_description(obj, "dt-compatible",
 "Overrides the \"compatible\" property of 
the dt root node",
 NULL);
+object_property_add(obj, "sockets", "int", machine_get_smp,
+machine_set_smp, NULL, NULL, NULL);
+object_property_set_description(obj, "sockets", "Number of sockets", NULL);
+object_property_add(obj, "cores", "int", machine_get_smp,
+machine_set_smp, NULL, NULL, NULL);
+object_property_set_description(obj, "cores",
+"Number of cores per socket", NULL);
+object_property_add(obj, "threads", "int", machine_get_smp,
+machine_set_smp, NULL, NULL, NULL);
+object_property_set_description(obj, "threads",
+"Number of threads per core", NULL);
+object_property_add(obj, "maxcpus", "int", machine_get_smp,
+machine_set_smp, NULL, NULL, NULL);
+object_property_set_description(obj, "maxcpus", "Maximum number of cpus",
+NULL);
+object_property_add(obj, "cpus", "int", machine_get_smp,
+machine_set_smp, NULL, NULL, NULL);
+object_property_set_description(obj, "cpus", "Number of online cpus",
+NULL);
 object_property_add_bool(obj, "dump-guest-core",
  machine_get_dump_guest_core,
  machine_set_dump_guest_core,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 4e8dc68b07a24..53adbfe2a3099 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -166,6 +166,12 @@ struct MachineState {
 char *initrd_filename;
 const char *cpu_model;
 AccelState *accelerator;
+
+int sockets;
+int cores;
+