| Message ID | 1462192431-146342-21-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, May 02, 2016 at 02:33:29PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
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?
On Tue, 10 May 2016 17:27:18 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, May 02, 2016 at 02:33:29PM +0200, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > 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.
On Wed, May 11, 2016 at 04:00:19PM +0200, Igor Mammedov wrote: > On Tue, 10 May 2016 17:27:18 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, May 02, 2016 at 02:33:29PM +0200, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > 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); } }
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;
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/core/machine.c | 20 ++++++++++++++++++++ include/hw/boards.h | 1 + 2 files changed, 21 insertions(+)