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 <address@hidden>
> > ---
> >  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, &value, 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, &value, &error);
> > +    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;
> > +    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.

Thanks,
drew

> 
> >  };
> >  
> >  #define DEFINE_MACHINE(namestr, machine_initfn) \
> 
> -- 
> David Gibson                  | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au        | minimalist, thank you.  NOT _the_ 
> _other_
>                               | _way_ _around_!
> http://www.ozlabs.org/~dgibson