| Message ID | 1456160797-832-8-git-send-email-mjrosato@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 22.02.2016 um 18:06 schrieb Matthew Rosato: > Implement cpu hotplug routine and add the machine hook. > > Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 33 +++++++++++++++++++++++++++++++++ > target-s390x/cpu.c | 7 +++++++ > 2 files changed, 40 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 3090e76..ea007e8 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -22,6 +22,7 @@ > #include "s390-pci-bus.h" > #include "hw/s390x/storage-keys.h" > #include "hw/compat.h" > +#include "qom/cpu.h" > > #define TYPE_S390_CCW_MACHINE "s390-ccw-machine" > > @@ -185,6 +186,37 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, > return NULL; > } > > +static void s390_hot_add_cpu(const int64_t id, Error **errp) > +{ > + MachineState *machine = MACHINE(qdev_get_machine()); > + > + if (id < 0) { > + error_setg(errp, "Invalid CPU id: %" PRIi64, id); > + return; > + } > + > + if (cpu_exists(id)) { > + error_setg(errp, "Unable to add CPU: %" PRIi64 > + ", it already exists", id); > + return; > + } > + > + if (id >= max_cpus) { > + error_setg(errp, "Unable to add CPU: %" PRIi64 > + ", max allowed: %d", id, max_cpus - 1); > + return; > + } > + > + if (id != (last_cpu->cpu_index + 1)) { This assumption about the order of the CPU list looks dangerous to me. Aren't there global int fields used by x86 already for the number of CPUs that you could use instead? That will allow you to drop the ugly preceding renaming patch. Please avoid messing with the CPU list directly from target code. > + error_setg(errp, "Unable to add CPU: %" PRIi64 > + ", The next available id is %d", id, > + (last_cpu->cpu_index + 1)); > + return; > + } > + > + cpu_s390x_init(machine->cpu_model); No proper error handling - that's why blindly reusing these old helpers is a bad idea. Regards, Andreas > +} > + > static void ccw_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > @@ -193,6 +225,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) > > mc->init = ccw_init; > mc->reset = s390_machine_reset; > + mc->hot_add_cpu = s390_hot_add_cpu; > mc->block_default_type = IF_VIRTIO; > mc->no_cdrom = 1; > mc->no_floppy = 1; > diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c > index 8dfd063..1a4977e 100644 > --- a/target-s390x/cpu.c > +++ b/target-s390x/cpu.c > @@ -32,6 +32,7 @@ > #include "trace.h" > #ifndef CONFIG_USER_ONLY > #include "sysemu/arch_init.h" > +#include "hw/s390x/sclp.h" > #endif > > #define CR0_RESET 0xE0UL > @@ -211,6 +212,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) > #endif > > scc->parent_realize(dev, errp); > + > +#if !defined(CONFIG_USER_ONLY) > + if (dev->hotplugged) { > + raise_irq_cpu_hotplug(); > + } > +#endif > } > > static void s390_cpu_initfn(Object *obj) >
On 02/22/2016 12:22 PM, Andreas Färber wrote: > Am 22.02.2016 um 18:06 schrieb Matthew Rosato: >> Implement cpu hotplug routine and add the machine hook. >> >> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 33 +++++++++++++++++++++++++++++++++ >> target-s390x/cpu.c | 7 +++++++ >> 2 files changed, 40 insertions(+) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 3090e76..ea007e8 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -22,6 +22,7 @@ >> #include "s390-pci-bus.h" >> #include "hw/s390x/storage-keys.h" >> #include "hw/compat.h" >> +#include "qom/cpu.h" >> >> #define TYPE_S390_CCW_MACHINE "s390-ccw-machine" >> >> @@ -185,6 +186,37 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, >> return NULL; >> } >> >> +static void s390_hot_add_cpu(const int64_t id, Error **errp) >> +{ >> + MachineState *machine = MACHINE(qdev_get_machine()); >> + >> + if (id < 0) { >> + error_setg(errp, "Invalid CPU id: %" PRIi64, id); >> + return; >> + } >> + >> + if (cpu_exists(id)) { >> + error_setg(errp, "Unable to add CPU: %" PRIi64 >> + ", it already exists", id); >> + return; >> + } >> + >> + if (id >= max_cpus) { >> + error_setg(errp, "Unable to add CPU: %" PRIi64 >> + ", max allowed: %d", id, max_cpus - 1); >> + return; >> + } >> + >> + if (id != (last_cpu->cpu_index + 1)) { > > This assumption about the order of the CPU list looks dangerous to me. > Aren't there global int fields used by x86 already for the number of > CPUs that you could use instead? That will allow you to drop the ugly > preceding renaming patch. Please avoid messing with the CPU list > directly from target code. > Not that I can find. We were keeping track of it with our own int for s390x, I just switched to this approach per review suggestion -- Sounds like you'd rather I bring back the int and inspect that, that's fine by me. >> + error_setg(errp, "Unable to add CPU: %" PRIi64 >> + ", The next available id is %d", id, >> + (last_cpu->cpu_index + 1)); >> + return; >> + } >> + >> + cpu_s390x_init(machine->cpu_model); > > No proper error handling - that's why blindly reusing these old helpers > is a bad idea. > Good point -- Let me re-work this into something more like f(model, id, local_err). Matt
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 3090e76..ea007e8 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -22,6 +22,7 @@ #include "s390-pci-bus.h" #include "hw/s390x/storage-keys.h" #include "hw/compat.h" +#include "qom/cpu.h" #define TYPE_S390_CCW_MACHINE "s390-ccw-machine" @@ -185,6 +186,37 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, return NULL; } +static void s390_hot_add_cpu(const int64_t id, Error **errp) +{ + MachineState *machine = MACHINE(qdev_get_machine()); + + if (id < 0) { + error_setg(errp, "Invalid CPU id: %" PRIi64, id); + return; + } + + if (cpu_exists(id)) { + error_setg(errp, "Unable to add CPU: %" PRIi64 + ", it already exists", id); + return; + } + + if (id >= max_cpus) { + error_setg(errp, "Unable to add CPU: %" PRIi64 + ", max allowed: %d", id, max_cpus - 1); + return; + } + + if (id != (last_cpu->cpu_index + 1)) { + error_setg(errp, "Unable to add CPU: %" PRIi64 + ", The next available id is %d", id, + (last_cpu->cpu_index + 1)); + return; + } + + cpu_s390x_init(machine->cpu_model); +} + static void ccw_machine_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); @@ -193,6 +225,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) mc->init = ccw_init; mc->reset = s390_machine_reset; + mc->hot_add_cpu = s390_hot_add_cpu; mc->block_default_type = IF_VIRTIO; mc->no_cdrom = 1; mc->no_floppy = 1; diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c index 8dfd063..1a4977e 100644 --- a/target-s390x/cpu.c +++ b/target-s390x/cpu.c @@ -32,6 +32,7 @@ #include "trace.h" #ifndef CONFIG_USER_ONLY #include "sysemu/arch_init.h" +#include "hw/s390x/sclp.h" #endif #define CR0_RESET 0xE0UL @@ -211,6 +212,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) #endif scc->parent_realize(dev, errp); + +#if !defined(CONFIG_USER_ONLY) + if (dev->hotplugged) { + raise_irq_cpu_hotplug(); + } +#endif } static void s390_cpu_initfn(Object *obj)
Implement cpu hotplug routine and add the machine hook. Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> --- hw/s390x/s390-virtio-ccw.c | 33 +++++++++++++++++++++++++++++++++ target-s390x/cpu.c | 7 +++++++ 2 files changed, 40 insertions(+)