| Message ID | 20170904154316.4148-18-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x cleanups and CPU hotplug via device_add | expand |
On 09/04/2017 05:43 PM, David Hildenbrand wrote: > device_del on a CPU will currently do nothing. Let's emmit an error > telling that this is will never work (there is no architecture support > on s390x). Error message copied from ppc. > > (qemu) device_del cpu1 > device_del cpu1 > CPU hot unplug not supported on this machine Given the fact that I get the question about unplug _every_ time when I give a presentation about KVM on z, I will try to get some architecture folks look at this. Maybe we can define something very simple like "if the CPU is in the stopped state we can remove this and just piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification". So maybe add "currently" > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 22a8a1b45d..dd149567bb 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -338,6 +338,15 @@ static void s390_machine_device_plug(HotplugHandler *hotplug_dev, > } > } > > +static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev, > + DeviceState *dev, Error **errp) > +{ > + if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > + error_setg(errp, "CPU hot unplug not supported on this machine"); > + return; > + } > +} > + > static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, > DeviceState *dev) > { > @@ -387,6 +396,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) > mc->max_cpus = 248; > mc->get_hotplug_handler = s390_get_hotplug_handler; > hc->plug = s390_machine_device_plug; > + hc->unplug_request = s390_machine_device_unplug_request; > nc->nmi_monitor_handler = s390_nmi; > } >
On 05.09.2017 11:14, Christian Borntraeger wrote: > On 09/04/2017 05:43 PM, David Hildenbrand wrote: >> device_del on a CPU will currently do nothing. Let's emmit an error >> telling that this is will never work (there is no architecture support >> on s390x). Error message copied from ppc. >> >> (qemu) device_del cpu1 >> device_del cpu1 >> CPU hot unplug not supported on this machine > > Given the fact that I get the question about unplug _every_ time when I give a presentation > about KVM on z, I will try to get some architecture folks look at this. Maybe we can define > something very simple like "if the CPU is in the stopped state we can remove this and just > piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification". > > So maybe add "currently" Unfortunately it might not be that easy. We would have to find a way that existing OS's don't break. If a guest OS is not prepared for CPUs to get removed, we might run into inconsistencies when simply deleting CPUs that are in the STOPPED state. Especially, these CPUs would still show up in the guest as "offline". Wonder what would then happen trying to "online" these. But yes, I can add "currently".
On 09/05/2017 02:01 PM, David Hildenbrand wrote: > On 05.09.2017 11:14, Christian Borntraeger wrote: >> On 09/04/2017 05:43 PM, David Hildenbrand wrote: >>> device_del on a CPU will currently do nothing. Let's emmit an error >>> telling that this is will never work (there is no architecture support >>> on s390x). Error message copied from ppc. >>> >>> (qemu) device_del cpu1 >>> device_del cpu1 >>> CPU hot unplug not supported on this machine >> >> Given the fact that I get the question about unplug _every_ time when I give a presentation >> about KVM on z, I will try to get some architecture folks look at this. Maybe we can define >> something very simple like "if the CPU is in the stopped state we can remove this and just >> piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification". >> >> So maybe add "currently" > > Unfortunately it might not be that easy. > > We would have to find a way that existing OS's don't break. If a guest > OS is not prepared for CPUs to get removed, we might run into > inconsistencies when simply deleting CPUs that are in the STOPPED state. Yes, this needs to be validated across all things. > > Especially, these CPUs would still show up in the guest as "offline". > Wonder what would then happen trying to "online" these. The main use case seems to be, that the admin does not want to allow a guest to online back a guest CPU if it was taken away from the configuration. So maybe we could simply fail a SIGP START for those. Or we might go one level below and only allow an unplug if the CPU is in the deconfigured state and we would then have to forbid the configuration step. Right now SCLP_CMDW_(DE)CONFIGURE_CPU seems to be unimplemented in QEMU. Anyway, we need some architecture agreement here first (something that is also ok with LPAR and z/VM). > But yes, I can add "currently". >
On Tue, 5 Sep 2017 14:14:21 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 09/05/2017 02:01 PM, David Hildenbrand wrote: > > On 05.09.2017 11:14, Christian Borntraeger wrote: > >> On 09/04/2017 05:43 PM, David Hildenbrand wrote: > >>> device_del on a CPU will currently do nothing. Let's emmit an error > >>> telling that this is will never work (there is no architecture support > >>> on s390x). Error message copied from ppc. > >>> > >>> (qemu) device_del cpu1 > >>> device_del cpu1 > >>> CPU hot unplug not supported on this machine > >> > >> Given the fact that I get the question about unplug _every_ time when I give a presentation > >> about KVM on z, I will try to get some architecture folks look at this. Maybe we can define > >> something very simple like "if the CPU is in the stopped state we can remove this and just > >> piggy back on the existing sclp EVENT_QUAL_CPU_CHANGE notification". > >> > >> So maybe add "currently" > > > > Unfortunately it might not be that easy. > > > > We would have to find a way that existing OS's don't break. If a guest > > OS is not prepared for CPUs to get removed, we might run into > > inconsistencies when simply deleting CPUs that are in the STOPPED state. > > Yes, this needs to be validated across all things. > > > > Especially, these CPUs would still show up in the guest as "offline". > > Wonder what would then happen trying to "online" these. > > The main use case seems to be, that the admin does not want to allow a guest > to online back a guest CPU if it was taken away from the configuration. > So maybe we could simply fail a SIGP START for those. > > Or we might go one level below and only allow an unplug if the CPU is in > the deconfigured state and we would then have to forbid the configuration > step. Having the cpu in the unconfigured state as a requirement also makes this less likely to break for older OSs, I guess. > Right now SCLP_CMDW_(DE)CONFIGURE_CPU seems to be unimplemented in > QEMU. Sounds like something we'd like to have in the future? (Btw, what does cpuplugd trigger? Offline/online or deconfigure/configure?) > > Anyway, we need some architecture agreement here first (something that is > also ok with LPAR and z/VM). Certainly. > > But yes, I can add "currently". Yes, that makes sense.
On 09/05/2017 02:54 PM, Cornelia Huck wrote: [...] > Having the cpu in the unconfigured state as a requirement also makes > this less likely to break for older OSs, I guess. > >> Right now SCLP_CMDW_(DE)CONFIGURE_CPU seems to be unimplemented in >> QEMU. > > Sounds like something we'd like to have in the future? > > (Btw, what does cpuplugd trigger? Offline/online or > deconfigure/configure?) offline/online via sigp stop/start. When we hotplug we already provide the new cpu in the configured state (like z/VM).
On 09/04/2017 11:43 AM, David Hildenbrand wrote: > device_del on a CPU will currently do nothing. Let's emmit an error > telling that this is will never work (there is no architecture support > on s390x). Error message copied from ppc. > > (qemu) device_del cpu1 > device_del cpu1 > CPU hot unplug not supported on this machine > > Signed-off-by: David Hildenbrand <david@redhat.com> Architecture discussions aside, this code will work as-is to prevent CPU unplug. Add "currently" as Christian suggests and: Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 22a8a1b45d..dd149567bb 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -338,6 +338,15 @@ static void s390_machine_device_plug(HotplugHandler *hotplug_dev, > } > } > > +static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev, > + DeviceState *dev, Error **errp) > +{ > + if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > + error_setg(errp, "CPU hot unplug not supported on this machine"); > + return; > + } > +} > + > static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, > DeviceState *dev) > { > @@ -387,6 +396,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) > mc->max_cpus = 248; > mc->get_hotplug_handler = s390_get_hotplug_handler; > hc->plug = s390_machine_device_plug; > + hc->unplug_request = s390_machine_device_unplug_request; > nc->nmi_monitor_handler = s390_nmi; > } >
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 22a8a1b45d..dd149567bb 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -338,6 +338,15 @@ static void s390_machine_device_plug(HotplugHandler *hotplug_dev, } } +static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev, + DeviceState *dev, Error **errp) +{ + if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { + error_setg(errp, "CPU hot unplug not supported on this machine"); + return; + } +} + static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, DeviceState *dev) { @@ -387,6 +396,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) mc->max_cpus = 248; mc->get_hotplug_handler = s390_get_hotplug_handler; hc->plug = s390_machine_device_plug; + hc->unplug_request = s390_machine_device_unplug_request; nc->nmi_monitor_handler = s390_nmi; }
device_del on a CPU will currently do nothing. Let's emmit an error telling that this is will never work (there is no architecture support on s390x). Error message copied from ppc. (qemu) device_del cpu1 device_del cpu1 CPU hot unplug not supported on this machine Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/s390x/s390-virtio-ccw.c | 10 ++++++++++ 1 file changed, 10 insertions(+)