| Message ID | 20180510150206.1ff3ce22@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10.05.2018 15:02, Igor Mammedov wrote: > On Wed, 9 May 2018 16:13:14 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 03.05.2018 17:49, David Hildenbrand wrote: >>> Hotplug handlers usually have the following tasks: >>> 1. Allocate some resources for a new device >>> 2. Make the new device visible for the guest >>> 3. Notify the guest about the new device >>> >>> Hotplug handlers have right now one limitation: They handle their own >>> context and only care about resources they manage. >>> >>> We can have devices that need certain other resources that are e.g. >>> system resources managed by the machine. We need a clean way to assign >>> these resources (without violating layers as brought up by Igor). >>> >>> One example is virtio-mem/virtio-pmem. Both device types need to be >>> assigned some region in guest physical address space. This device memory >>> belongs to the machine and is managed by it. However, virito devices are >>> hotplugged using the hotplug handler their proxy device implements. So we >>> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW >>> hotplug handler for virtio-ccw. But definetly not the machine. >>> >>> So let's generalize the task of "assigning" resources and use it directly >>> for memory devices. We now have a clean way to support any kind of memory >>> device - independent of the underlying device type. Right now, only one >>> resource handler per device can be supported (in addition to the existing >>> hotplug handler). >>> >>> You can find more details in patch nr 2. >>> >>> This work is based on the already queued patch series >>> "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice" >>> >>> David Hildenbrand (8): >>> memory-device: always compile support for memory devices for SOFTMMU >>> qdev: introduce ResourceHandler as a first-stage hotplug handler >>> machine: provide default resource handler >>> memory-device: new functions to handle resource assignment >>> pc-dimm: implement new memory device functions >>> machine: introduce enforce_memory_device_align() and add it for pc >>> memory-device: factor out pre-assign into default resource handler >>> memory-device: factor out (un)assign into default resource handler >>> >>> hw/Makefile.objs | 2 +- >>> hw/core/Makefile.objs | 1 + >>> hw/core/machine.c | 70 +++++++++++++++++++++++ >>> hw/core/qdev.c | 41 +++++++++++++- >>> hw/core/resource-handler.c | 57 +++++++++++++++++++ >>> hw/i386/pc.c | 31 ++++++----- >>> hw/mem/Makefile.objs | 2 +- >>> hw/mem/memory-device.c | 122 +++++++++++++++++++++++++---------------- >>> hw/mem/pc-dimm.c | 53 ++++++++---------- >>> hw/mem/trace-events | 4 +- >>> hw/ppc/spapr.c | 5 +- >>> include/hw/boards.h | 17 ++++++ >>> include/hw/mem/memory-device.h | 17 ++++-- >>> include/hw/mem/pc-dimm.h | 3 +- >>> include/hw/resource-handler.h | 46 ++++++++++++++++ >>> stubs/Makefile.objs | 1 - >>> stubs/qmp_memory_device.c | 13 ----- >>> 17 files changed, 364 insertions(+), 121 deletions(-) >>> create mode 100644 hw/core/resource-handler.c >>> create mode 100644 include/hw/resource-handler.h >>> delete mode 100644 stubs/qmp_memory_device.c >>> >> >> If there are no further comments, I'll send a v2 by the end of this >> week. Thanks! > I couldn't convince myself that ResourceHandler is really necessary. > My main gripe with it, is that it imposes specific ordering wrt hotplug > handler that resources will be touched. Other issue is that it looks > a bit over-engineered with a lot of code fragmentation. Hence, > > I'd suggest use simple hotplug handler chaining instead, > which should take care of wiring up virtio-mem/virtio-pmem, > keeping code compact at the same time. I'll have a look tomorrow or next friday if that could work - not sure yet about unplug vs. unplug requests. Unplug requests might be tricky. Would be nice if it would work. Thanks! > > Could you try something along these lines (I'll post a patch to > override default hotplug handler as reply here for you to pick up, > that should make following work): > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 100dfdc..c400c0c 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -393,12 +393,30 @@ static void s390_machine_reset(void) > s390_ipl_prepare_cpu(ipl_cpu); > s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu); > } > +static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev, > + DeviceState *dev, Error **errp) > +{ > + if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) { > + /* do checks && set default values if it weren't set by user */ > + /* possibly pass to device's bus pre_plug handler if need */ > + } > +} > > static void s390_machine_device_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > s390_cpu_plug(hotplug_dev, dev, errp); > + } if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) { > + HotplugHandler *hotplug_ctrl = dev->parent_bus->hotplug_handler; > + > + /* do machine specific wiring, i.e. > + * assign resources specified by user or by foo_pre_plug(), > + * like map region into machine address space at specified address */ > + if (OK) { > + /* pass control down to bus specific hotplug chain */ > + hotplug_handler_plug(hotplug_ctrl, dev, errp); > + } > } > } > > @@ -449,6 +467,8 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, > { > if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > return HOTPLUG_HANDLER(machine); > + } (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM)) { > + return HOTPLUG_HANDLER(machine); > } > return NULL; > } >
On Thu, 10 May 2018 15:20:55 +0200 David Hildenbrand <david@redhat.com> wrote: > On 10.05.2018 15:02, Igor Mammedov wrote: > > On Wed, 9 May 2018 16:13:14 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 03.05.2018 17:49, David Hildenbrand wrote: > >>> Hotplug handlers usually have the following tasks: > >>> 1. Allocate some resources for a new device > >>> 2. Make the new device visible for the guest > >>> 3. Notify the guest about the new device > >>> > >>> Hotplug handlers have right now one limitation: They handle their own > >>> context and only care about resources they manage. > >>> > >>> We can have devices that need certain other resources that are e.g. > >>> system resources managed by the machine. We need a clean way to assign > >>> these resources (without violating layers as brought up by Igor). > >>> > >>> One example is virtio-mem/virtio-pmem. Both device types need to be > >>> assigned some region in guest physical address space. This device memory > >>> belongs to the machine and is managed by it. However, virito devices are > >>> hotplugged using the hotplug handler their proxy device implements. So we > >>> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW > >>> hotplug handler for virtio-ccw. But definetly not the machine. > >>> > >>> So let's generalize the task of "assigning" resources and use it directly > >>> for memory devices. We now have a clean way to support any kind of memory > >>> device - independent of the underlying device type. Right now, only one > >>> resource handler per device can be supported (in addition to the existing > >>> hotplug handler). > >>> > >>> You can find more details in patch nr 2. > >>> > >>> This work is based on the already queued patch series > >>> "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice" > >>> > >>> David Hildenbrand (8): > >>> memory-device: always compile support for memory devices for SOFTMMU > >>> qdev: introduce ResourceHandler as a first-stage hotplug handler > >>> machine: provide default resource handler > >>> memory-device: new functions to handle resource assignment > >>> pc-dimm: implement new memory device functions > >>> machine: introduce enforce_memory_device_align() and add it for pc > >>> memory-device: factor out pre-assign into default resource handler > >>> memory-device: factor out (un)assign into default resource handler > >>> > >>> hw/Makefile.objs | 2 +- > >>> hw/core/Makefile.objs | 1 + > >>> hw/core/machine.c | 70 +++++++++++++++++++++++ > >>> hw/core/qdev.c | 41 +++++++++++++- > >>> hw/core/resource-handler.c | 57 +++++++++++++++++++ > >>> hw/i386/pc.c | 31 ++++++----- > >>> hw/mem/Makefile.objs | 2 +- > >>> hw/mem/memory-device.c | 122 +++++++++++++++++++++++++---------------- > >>> hw/mem/pc-dimm.c | 53 ++++++++---------- > >>> hw/mem/trace-events | 4 +- > >>> hw/ppc/spapr.c | 5 +- > >>> include/hw/boards.h | 17 ++++++ > >>> include/hw/mem/memory-device.h | 17 ++++-- > >>> include/hw/mem/pc-dimm.h | 3 +- > >>> include/hw/resource-handler.h | 46 ++++++++++++++++ > >>> stubs/Makefile.objs | 1 - > >>> stubs/qmp_memory_device.c | 13 ----- > >>> 17 files changed, 364 insertions(+), 121 deletions(-) > >>> create mode 100644 hw/core/resource-handler.c > >>> create mode 100644 include/hw/resource-handler.h > >>> delete mode 100644 stubs/qmp_memory_device.c > >>> > >> > >> If there are no further comments, I'll send a v2 by the end of this > >> week. Thanks! > > I couldn't convince myself that ResourceHandler is really necessary. > > My main gripe with it, is that it imposes specific ordering wrt hotplug > > handler that resources will be touched. Other issue is that it looks > > a bit over-engineered with a lot of code fragmentation. Hence, > > > > I'd suggest use simple hotplug handler chaining instead, > > which should take care of wiring up virtio-mem/virtio-pmem, > > keeping code compact at the same time. > > I'll have a look tomorrow or next friday if that could work - not sure > yet about unplug vs. unplug requests. Unplug requests might be tricky. > Would be nice if it would work. Thanks! If you have issues with it, ping me, Maybe we'd figure out how to make it work together. > > > > > Could you try something along these lines (I'll post a patch to > > override default hotplug handler as reply here for you to pick up, > > that should make following work): > > > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > > index 100dfdc..c400c0c 100644 > > --- a/hw/s390x/s390-virtio-ccw.c > > +++ b/hw/s390x/s390-virtio-ccw.c > > @@ -393,12 +393,30 @@ static void s390_machine_reset(void) > > s390_ipl_prepare_cpu(ipl_cpu); > > s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu); > > } > > +static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev, > > + DeviceState *dev, Error **errp) > > +{ > > + if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) { > > + /* do checks && set default values if it weren't set by user */ > > + /* possibly pass to device's bus pre_plug handler if need */ > > + } > > +} > > > > static void s390_machine_device_plug(HotplugHandler *hotplug_dev, > > DeviceState *dev, Error **errp) > > { > > if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > > s390_cpu_plug(hotplug_dev, dev, errp); > > + } if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) { > > + HotplugHandler *hotplug_ctrl = dev->parent_bus->hotplug_handler; > > + > > + /* do machine specific wiring, i.e. > > + * assign resources specified by user or by foo_pre_plug(), > > + * like map region into machine address space at specified address */ > > + if (OK) { > > + /* pass control down to bus specific hotplug chain */ > > + hotplug_handler_plug(hotplug_ctrl, dev, errp); > > + } > > } > > } > > > > @@ -449,6 +467,8 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, > > { > > if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > > return HOTPLUG_HANDLER(machine); > > + } (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM)) { > > + return HOTPLUG_HANDLER(machine); > > } > > return NULL; > > } > > > >
On 10.05.2018 15:32, Igor Mammedov wrote: > On Thu, 10 May 2018 15:20:55 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 10.05.2018 15:02, Igor Mammedov wrote: >>> On Wed, 9 May 2018 16:13:14 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> On 03.05.2018 17:49, David Hildenbrand wrote: >>>>> Hotplug handlers usually have the following tasks: >>>>> 1. Allocate some resources for a new device >>>>> 2. Make the new device visible for the guest >>>>> 3. Notify the guest about the new device >>>>> >>>>> Hotplug handlers have right now one limitation: They handle their own >>>>> context and only care about resources they manage. >>>>> >>>>> We can have devices that need certain other resources that are e.g. >>>>> system resources managed by the machine. We need a clean way to assign >>>>> these resources (without violating layers as brought up by Igor). >>>>> >>>>> One example is virtio-mem/virtio-pmem. Both device types need to be >>>>> assigned some region in guest physical address space. This device memory >>>>> belongs to the machine and is managed by it. However, virito devices are >>>>> hotplugged using the hotplug handler their proxy device implements. So we >>>>> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW >>>>> hotplug handler for virtio-ccw. But definetly not the machine. >>>>> >>>>> So let's generalize the task of "assigning" resources and use it directly >>>>> for memory devices. We now have a clean way to support any kind of memory >>>>> device - independent of the underlying device type. Right now, only one >>>>> resource handler per device can be supported (in addition to the existing >>>>> hotplug handler). >>>>> >>>>> You can find more details in patch nr 2. >>>>> >>>>> This work is based on the already queued patch series >>>>> "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice" >>>>> >>>>> David Hildenbrand (8): >>>>> memory-device: always compile support for memory devices for SOFTMMU >>>>> qdev: introduce ResourceHandler as a first-stage hotplug handler >>>>> machine: provide default resource handler >>>>> memory-device: new functions to handle resource assignment >>>>> pc-dimm: implement new memory device functions >>>>> machine: introduce enforce_memory_device_align() and add it for pc >>>>> memory-device: factor out pre-assign into default resource handler >>>>> memory-device: factor out (un)assign into default resource handler >>>>> >>>>> hw/Makefile.objs | 2 +- >>>>> hw/core/Makefile.objs | 1 + >>>>> hw/core/machine.c | 70 +++++++++++++++++++++++ >>>>> hw/core/qdev.c | 41 +++++++++++++- >>>>> hw/core/resource-handler.c | 57 +++++++++++++++++++ >>>>> hw/i386/pc.c | 31 ++++++----- >>>>> hw/mem/Makefile.objs | 2 +- >>>>> hw/mem/memory-device.c | 122 +++++++++++++++++++++++++---------------- >>>>> hw/mem/pc-dimm.c | 53 ++++++++---------- >>>>> hw/mem/trace-events | 4 +- >>>>> hw/ppc/spapr.c | 5 +- >>>>> include/hw/boards.h | 17 ++++++ >>>>> include/hw/mem/memory-device.h | 17 ++++-- >>>>> include/hw/mem/pc-dimm.h | 3 +- >>>>> include/hw/resource-handler.h | 46 ++++++++++++++++ >>>>> stubs/Makefile.objs | 1 - >>>>> stubs/qmp_memory_device.c | 13 ----- >>>>> 17 files changed, 364 insertions(+), 121 deletions(-) >>>>> create mode 100644 hw/core/resource-handler.c >>>>> create mode 100644 include/hw/resource-handler.h >>>>> delete mode 100644 stubs/qmp_memory_device.c >>>>> >>>> >>>> If there are no further comments, I'll send a v2 by the end of this >>>> week. Thanks! >>> I couldn't convince myself that ResourceHandler is really necessary. >>> My main gripe with it, is that it imposes specific ordering wrt hotplug >>> handler that resources will be touched. Other issue is that it looks >>> a bit over-engineered with a lot of code fragmentation. Hence, >>> >>> I'd suggest use simple hotplug handler chaining instead, >>> which should take care of wiring up virtio-mem/virtio-pmem, >>> keeping code compact at the same time. >> >> I'll have a look tomorrow or next friday if that could work - not sure >> yet about unplug vs. unplug requests. Unplug requests might be tricky. >> Would be nice if it would work. Thanks! > If you have issues with it, ping me, Maybe we'd figure out how to make it > work together. Looks promising :) Will post something based on your qdev patch as soon as I have it cleaned up. Thanks for your help! > >> >>> >>> Could you try something along these lines (I'll post a patch to >>> override default hotplug handler as reply here for you to pick up, >>> that should make following work): >>> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>> index 100dfdc..c400c0c 100644 >>> --- a/hw/s390x/s390-virtio-ccw.c >>> +++ b/hw/s390x/s390-virtio-ccw.c >>> @@ -393,12 +393,30 @@ static void s390_machine_reset(void) >>> s390_ipl_prepare_cpu(ipl_cpu); >>> s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu); >>> } >>> +static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev, >>> + DeviceState *dev, Error **errp) >>> +{ >>> + if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) { >>> + /* do checks && set default values if it weren't set by user */ >>> + /* possibly pass to device's bus pre_plug handler if need */ >>> + } >>> +} >>> >>> static void s390_machine_device_plug(HotplugHandler *hotplug_dev, >>> DeviceState *dev, Error **errp) >>> { >>> if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { >>> s390_cpu_plug(hotplug_dev, dev, errp); >>> + } if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) { >>> + HotplugHandler *hotplug_ctrl = dev->parent_bus->hotplug_handler; >>> + >>> + /* do machine specific wiring, i.e. >>> + * assign resources specified by user or by foo_pre_plug(), >>> + * like map region into machine address space at specified address */ >>> + if (OK) { >>> + /* pass control down to bus specific hotplug chain */ >>> + hotplug_handler_plug(hotplug_ctrl, dev, errp); >>> + } >>> } >>> } >>> >>> @@ -449,6 +467,8 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, >>> { >>> if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { >>> return HOTPLUG_HANDLER(machine); >>> + } (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM)) { >>> + return HOTPLUG_HANDLER(machine); >>> } >>> return NULL; >>> } >>> >> >> >
On 10/05/2018 15:32, Igor Mammedov wrote: > On Thu, 10 May 2018 15:20:55 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 10.05.2018 15:02, Igor Mammedov wrote: >>> On Wed, 9 May 2018 16:13:14 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> On 03.05.2018 17:49, David Hildenbrand wrote: >>>>> Hotplug handlers usually have the following tasks: >>>>> 1. Allocate some resources for a new device >>>>> 2. Make the new device visible for the guest >>>>> 3. Notify the guest about the new device >>>>> >>>>> Hotplug handlers have right now one limitation: They handle their own >>>>> context and only care about resources they manage. >>>>> >>>>> We can have devices that need certain other resources that are e.g. >>>>> system resources managed by the machine. We need a clean way to assign >>>>> these resources (without violating layers as brought up by Igor). >>>>> >>>>> One example is virtio-mem/virtio-pmem. Both device types need to be >>>>> assigned some region in guest physical address space. This device memory >>>>> belongs to the machine and is managed by it. However, virito devices are >>>>> hotplugged using the hotplug handler their proxy device implements. So we >>>>> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW >>>>> hotplug handler for virtio-ccw. But definetly not the machine. >>>>> >>>>> So let's generalize the task of "assigning" resources and use it directly >>>>> for memory devices. We now have a clean way to support any kind of memory >>>>> device - independent of the underlying device type. Right now, only one >>>>> resource handler per device can be supported (in addition to the existing >>>>> hotplug handler). >>>>> >>>>> You can find more details in patch nr 2. >>>>> >>>>> This work is based on the already queued patch series >>>>> "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice" >>>>> >>>>> David Hildenbrand (8): >>>>> memory-device: always compile support for memory devices for SOFTMMU >>>>> qdev: introduce ResourceHandler as a first-stage hotplug handler >>>>> machine: provide default resource handler >>>>> memory-device: new functions to handle resource assignment >>>>> pc-dimm: implement new memory device functions >>>>> machine: introduce enforce_memory_device_align() and add it for pc >>>>> memory-device: factor out pre-assign into default resource handler >>>>> memory-device: factor out (un)assign into default resource handler >>>>> >>>>> hw/Makefile.objs | 2 +- >>>>> hw/core/Makefile.objs | 1 + >>>>> hw/core/machine.c | 70 +++++++++++++++++++++++ >>>>> hw/core/qdev.c | 41 +++++++++++++- >>>>> hw/core/resource-handler.c | 57 +++++++++++++++++++ >>>>> hw/i386/pc.c | 31 ++++++----- >>>>> hw/mem/Makefile.objs | 2 +- >>>>> hw/mem/memory-device.c | 122 +++++++++++++++++++++++++---------------- >>>>> hw/mem/pc-dimm.c | 53 ++++++++---------- >>>>> hw/mem/trace-events | 4 +- >>>>> hw/ppc/spapr.c | 5 +- >>>>> include/hw/boards.h | 17 ++++++ >>>>> include/hw/mem/memory-device.h | 17 ++++-- >>>>> include/hw/mem/pc-dimm.h | 3 +- >>>>> include/hw/resource-handler.h | 46 ++++++++++++++++ >>>>> stubs/Makefile.objs | 1 - >>>>> stubs/qmp_memory_device.c | 13 ----- >>>>> 17 files changed, 364 insertions(+), 121 deletions(-) >>>>> create mode 100644 hw/core/resource-handler.c >>>>> create mode 100644 include/hw/resource-handler.h >>>>> delete mode 100644 stubs/qmp_memory_device.c >>>>> >>>> >>>> If there are no further comments, I'll send a v2 by the end of this >>>> week. Thanks! >>> I couldn't convince myself that ResourceHandler is really necessary. >>> My main gripe with it, is that it imposes specific ordering wrt hotplug >>> handler that resources will be touched. Other issue is that it looks >>> a bit over-engineered with a lot of code fragmentation. Hence, >>> >>> I'd suggest use simple hotplug handler chaining instead, >>> which should take care of wiring up virtio-mem/virtio-pmem, >>> keeping code compact at the same time. >> >> I'll have a look tomorrow or next friday if that could work - not sure >> yet about unplug vs. unplug requests. Unplug requests might be tricky. >> Would be nice if it would work. Thanks! > If you have issues with it, ping me, Maybe we'd figure out how to make it > work together. I think I have to revive this thread. Let's have a look at virtio-pmem-pci: bus: pci.0 type PCI dev: virtio-pmem-pci, id "vp1" [...] bus: virtio-bus type virtio-pci-bus dev: virtio-pmem, id "" memaddr = 9663676416 (0x240000000) memdev = "/objects/mem1" [...] When creating virtio-pmem-pci (virtio proxy), virtio-pmem is created and attached to the virtio-bus of virtio-pmem-pci. When realizing virtio-pmem-pci, the child virtio-pmem device is realized. Now, hotplugging works by simply registering a new hotplug handler for virtio-pmem, because the virtio-bus does not have a hotplug handler (so we don't have multiple levels of hotplug handler calls). When realizing virtio-pmem, the pre_plug and plug handler will correctly be called. Works fine. Now, when unplugging virtio-pmem-pci, we will only get a hotplug handler call initially to start unplugging this device hierarchy for virtio-pmem-pci, effectively being some magiv followed by a object_unparent(). This will kick off unrealizing first virtio-pmem-pci, followed by virtio-bus and then virtio-pmem. For virtio-pmem, we won't get an unplug call to hotplug handlers. We would have to add a call to hotplug_handler_unplug() when unrealizing a device. However at that point we don't know if the hotplug handler has already been called (triggered initially by the user). Igor, any idea?
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 100dfdc..c400c0c 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -393,12 +393,30 @@ static void s390_machine_reset(void) s390_ipl_prepare_cpu(ipl_cpu); s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu); } +static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev, + DeviceState *dev, Error **errp) +{ + if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) { + /* do checks && set default values if it weren't set by user */ + /* possibly pass to device's bus pre_plug handler if need */ + } +} static void s390_machine_device_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { s390_cpu_plug(hotplug_dev, dev, errp); + } if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) { + HotplugHandler *hotplug_ctrl = dev->parent_bus->hotplug_handler; + + /* do machine specific wiring, i.e. + * assign resources specified by user or by foo_pre_plug(), + * like map region into machine address space at specified address */ + if (OK) { + /* pass control down to bus specific hotplug chain */ + hotplug_handler_plug(hotplug_ctrl, dev, errp); + } } } @@ -449,6 +467,8 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, { if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { return HOTPLUG_HANDLER(machine); + } (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM)) { + return HOTPLUG_HANDLER(machine); } return NULL; }