| Message ID | 20180503154936.18946-1-david@redhat.com |
|---|---|
| Headers | show |
| Series | MemoryDevice: introduce and use ResourceHandler | expand |
On Thu, 3 May 2018 17:49:28 +0200 David Hildenbrand <david@redhat.com> 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). So far I've just skimmed through series and still don't get clear picture if new interface is really needed. I'd like very much to see patches for how virtio-[p]mem in CCW and PCI case would utilize this. > 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 >
On 04.05.2018 10:49, Igor Mammedov wrote: > On Thu, 3 May 2018 17:49:28 +0200 > David Hildenbrand <david@redhat.com> 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). > So far I've just skimmed through series and still don't get clear > picture if new interface is really needed. > I'd like very much to see patches for how virtio-[p]mem in CCW and PCI > case would utilize this. Sure, I think you've seen the problematic parts in the latest virtio-pmem prototype (https://www.spinics.net/lists/linux-mm/msg151081.html). There, we do the assignment from the realize function, which you described as layer violation. I will ask Pankaj to rebase his work on this series. Until then, I can point you at the current QEMU side prototype of virtio-mem. I won't be posting these as patches as there are still many things to sort out and clean up. The prototype currently works on x86 and s390x. You can find the latest prototype on github, including patches of this series at https://github.com/davidhildenbrand/qemu/tree/virtio-mem Interesting patches are: - "s390x: inititalize memory region for memory devices" -- Provide a region for memory devices just like x86 - "virtio-mem: paravirtualized memory" -- Prototype of virtio-mem. - "virtio-ccw: add proxy for virtio-mem" -- CCW proxy device for virtio-mem (using the CSS/CCW hotplug handler) - "virtio-mem: paravirtualized memory" -- PCI proxy device for virtio-mem (using PCI hotplug handler) Note how virtio-mem implements the MemoryDevice interface and how resource assignments happens without any layer violations (and no modifications to any hotplug handler).
On Fri, 4 May 2018 11:19:25 +0200 David Hildenbrand <david@redhat.com> wrote: > On 04.05.2018 10:49, Igor Mammedov wrote: > > On Thu, 3 May 2018 17:49:28 +0200 > > David Hildenbrand <david@redhat.com> 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). > > So far I've just skimmed through series and still don't get clear > > picture if new interface is really needed. > > I'd like very much to see patches for how virtio-[p]mem in CCW and PCI > > case would utilize this. > > Sure, I think you've seen the problematic parts in the latest > virtio-pmem prototype > (https://www.spinics.net/lists/linux-mm/msg151081.html). > > There, we do the assignment from the realize function, which you > described as layer violation. I will ask Pankaj to rebase his work on > this series. That's too premature at the moment. > Until then, I can point you at the current QEMU side prototype of > virtio-mem. I won't be posting these as patches as there are still many > things to sort out and clean up. The prototype currently works on x86 > and s390x. > > You can find the latest prototype on github, including patches of this > series at https://github.com/davidhildenbrand/qemu/tree/virtio-mem > > Interesting patches are: > - "s390x: inititalize memory region for memory devices" > -- Provide a region for memory devices just like x86 > - "virtio-mem: paravirtualized memory" > -- Prototype of virtio-mem. > - "virtio-ccw: add proxy for virtio-mem" > -- CCW proxy device for virtio-mem (using the CSS/CCW hotplug handler) > - "virtio-mem: paravirtualized memory" > -- PCI proxy device for virtio-mem (using PCI hotplug handler) > > Note how virtio-mem implements the MemoryDevice interface and how > resource assignments happens without any layer violations (and no > modifications to any hotplug handler). Thanks, that should be sufficient to get idea. I'll look into it and come back here with concrete comments or suggesting alternative.
>> Note how virtio-mem implements the MemoryDevice interface and how >> resource assignments happens without any layer violations (and no >> modifications to any hotplug handler). > Thanks, that should be sufficient to get idea. > I'll look into it and come back here with concrete comments or > suggesting alternative. > Thanks, looking forward to your reply. If there are any questions in the meantime, please let me know!
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!
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. 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: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?
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