[RFC,20/42] machine: add cpu-hotplug machine option

Commit Message

Igor Mammedov May 2, 2016, 12:33 p.m. UTC

Signed-off-by: Igor Mammedov <[e-mail]>
---
 hw/core/machine.c   | 20 ++++++++++++++++++++
 include/hw/boards.h |  1 +
 2 files changed, 21 insertions(+)

Comments

Eduardo Habkost May 10, 2016, 8:27 p.m. UTC | #1

On Mon, May 02, 2016 at 02:33:29PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <[e-mail]>

If a machine class simply doesn't support CPU hotplug at all, is
silently ignoring "cpu-hotplug=on" the right thing to do?

Shouldn't we exit with an error if the machine class doesn't
support CPU hotplug and the user tries to enable it?

Igor Mammedov May 11, 2016, 2 p.m. UTC | #2

On Tue, 10 May 2016 17:27:18 -0300
Eduardo Habkost <[e-mail]> wrote:

> On Mon, May 02, 2016 at 02:33:29PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <[e-mail]>  
> 
> If a machine class simply doesn't support CPU hotplug at all, is
> silently ignoring "cpu-hotplug=on" the right thing to do?
> 
> Shouldn't we exit with an error if the machine class doesn't
> support CPU hotplug and the user tries to enable it?
We have a bunch of such options in generic MachineClass
and it was upto concrete machine to implement such checks.

So I hadn't even considered to make such check in generic code
nor think that's a right thing to do, if that's what you are asking.

But maybe I've misunderstood question.

Eduardo Habkost May 11, 2016, 3 p.m. UTC | #3

On Wed, May 11, 2016 at 04:00:19PM +0200, Igor Mammedov wrote:
> On Tue, 10 May 2016 17:27:18 -0300
> Eduardo Habkost <[e-mail]> wrote:
> 
> > On Mon, May 02, 2016 at 02:33:29PM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <[e-mail]>  
> > 
> > If a machine class simply doesn't support CPU hotplug at all, is
> > silently ignoring "cpu-hotplug=on" the right thing to do?
> > 
> > Shouldn't we exit with an error if the machine class doesn't
> > support CPU hotplug and the user tries to enable it?
> We have a bunch of such options in generic MachineClass
> and it was upto concrete machine to implement such checks.

Yes, and my proposal is to make this more robust from now on, and
make machines stop silently ignoring unsupported options.

> 
> So I hadn't even considered to make such check in generic code
> nor think that's a right thing to do, if that's what you are asking.

If only 1 or 2 machines support CPU hotplug, I don't think it
would be reasonable to have the option available at every machine
class and require most classes to add an extra check like:

    if (machine->cpu_hotplug)
        error_setg(errp, "CPU hotplug not supported");


A check like the following, in common code:

    if (!machine_class->cpu_hotplug_supported && machine->cpu_hotplug)
        error_setg(errp, "CPU hotplug not supported")

would avoid duplicating code in every machine class, and only
require the following extra line in the machine classes that
really support CPU hotplug:

    machine_class->cpu_hotplug_supported = true;


Or, even better: we can avoid registering the cpu-hotplug
property at all if the subclass doesn't support CPU hotplug. You
can do that at machine_class_base_init, e.g.:

    static void machine_class_base_init(ObjectClass *oc, void *data)
    {
        /* [...] */
        MachineClass *mc = MACHINE_CLASS(oc);
        if (mc->cpu_hotplug_supported) {
            object_class_property_add_str(oc, "cpu-hotplug",
                                          machine_get_cpu_hotplug,
                                          machine_set_cpu_hotplug, NULL);
            object_class_property_set_description(oc, "cpu-hotplug",
                                                  "Set on to enable CPU hotplug",
                                                  NULL);
        }
    }

Patch

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6dbbc85..498230a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -329,6 +329,20 @@  static bool machine_get_enforce_config_section(Object *obj, Error **errp)
     return ms->enforce_config_section;
 }
 
+static bool machine_get_cpu_hotplug(Object *obj, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    return ms->cpu_hotplug;
+}
+
+static void machine_set_cpu_hotplug(Object *obj, bool value, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    ms->cpu_hotplug = value;
+}
+
 static int error_on_sysbus_device(SysBusDevice *sbdev, void *opaque)
 {
     error_report("Option '-device %s' cannot be handled by this machine",
@@ -490,6 +504,12 @@  static void machine_initfn(Object *obj)
     object_property_set_description(obj, "enforce-config-section",
                                     "Set on to enforce configuration section migration",
                                     NULL);
+    object_property_add_bool(obj, "cpu-hotplug",
+                             machine_get_cpu_hotplug,
+                             machine_set_cpu_hotplug, NULL);
+    object_property_set_description(obj, "cpu-hotplug",
+                                   "Set on to enable CPU hotplug",
+                                    NULL);
 
     /* Register notifier when init is done for sysbus sanity checks */
     ms->sysbus_notifier.notify = machine_init_notify;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 8d4fe56..d388f96 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -154,6 +154,7 @@  struct MachineState {
     bool iommu;
     bool suppress_vmdesc;
     bool enforce_config_section;
+    bool cpu_hotplug;
 
     ram_addr_t ram_size;
     ram_addr_t maxram_size;