| Message ID | 20180517081527.14410-1-david@redhat.com |
|---|---|
| Headers | show |
| Series | MemoryDevice: use multi stage hotplug handlers | expand |
On 17.05.2018 10:15, David Hildenbrand wrote: > 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. > > Now, we can route other devices through the machine hotplug handler, to > properly assign/unassign resources - like a portion in guest physical > address space. > > v3 -> v4: > - Removed the s390x bits, will send that out separately (was just a proof > that it works just fine with s390x) > - Fixed a typo and reworded a comment > > v2 -> v3: > - Added "memory-device: introduce separate config option" > - Dropped "parent_bus" check from hotplug handler lookup functions > - "Handly" -> "Handle" in patch description. > > v1 -> v2: > - Use multi stage hotplug handler instead of resource handler > - MemoryDevices only compiled if necessary (CONFIG_MEM_HOTPLUG) > - Prepare PC/SPAPR machines properly for multi stage hotplug handlers > - Route SPAPR unplug code via the hotunplug handler > - Directly include s390x support. But there are no usable memory devices > yet (well, only my virtio-mem prototype) > - Included "memory-device: drop assert related to align and start of address > space" > > David Hildenbrand (13): > memory-device: drop assert related to align and start of address space > memory-device: introduce separate config option > pc: prepare for multi stage hotplug handlers > pc: route all memory devices through the machine hotplug handler > spapr: prepare for multi stage hotplug handlers > spapr: route all memory devices through the machine hotplug handler > spapr: handle pc-dimm unplug via hotplug handler chain > spapr: handle cpu core unplug via hotplug handler chain > memory-device: new functions to handle plug/unplug > pc-dimm: implement new memory device functions > memory-device: factor out pre-plug into hotplug handler > memory-device: factor out unplug into hotplug handler > memory-device: factor out plug into hotplug handler > > Igor Mammedov (1): > qdev: let machine hotplug handler to override bus hotplug handler > > default-configs/i386-softmmu.mak | 3 +- > default-configs/ppc64-softmmu.mak | 3 +- > default-configs/x86_64-softmmu.mak | 3 +- > hw/Makefile.objs | 2 +- > hw/core/qdev.c | 6 +- > hw/i386/pc.c | 102 ++++++++++++++++++++++------- > hw/mem/Makefile.objs | 4 +- > hw/mem/memory-device.c | 129 +++++++++++++++++++++++-------------- > hw/mem/pc-dimm.c | 48 ++++++-------- > hw/mem/trace-events | 4 +- > hw/ppc/spapr.c | 129 +++++++++++++++++++++++++++++++------ > include/hw/mem/memory-device.h | 21 ++++-- > include/hw/mem/pc-dimm.h | 3 +- > include/hw/qdev-core.h | 11 ++++ > qapi/misc.json | 2 +- > 15 files changed, 330 insertions(+), 140 deletions(-) > As there was no negative feedback so far, I will go ahead and assume that this approach is the right thing to do.
On 25/05/2018 14:43, David Hildenbrand wrote: > On 17.05.2018 10:15, David Hildenbrand wrote: >> 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. >> >> Now, we can route other devices through the machine hotplug handler, to >> properly assign/unassign resources - like a portion in guest physical >> address space. >> >> v3 -> v4: >> - Removed the s390x bits, will send that out separately (was just a proof >> that it works just fine with s390x) >> - Fixed a typo and reworded a comment >> >> v2 -> v3: >> - Added "memory-device: introduce separate config option" >> - Dropped "parent_bus" check from hotplug handler lookup functions >> - "Handly" -> "Handle" in patch description. >> >> v1 -> v2: >> - Use multi stage hotplug handler instead of resource handler >> - MemoryDevices only compiled if necessary (CONFIG_MEM_HOTPLUG) >> - Prepare PC/SPAPR machines properly for multi stage hotplug handlers >> - Route SPAPR unplug code via the hotunplug handler >> - Directly include s390x support. But there are no usable memory devices >> yet (well, only my virtio-mem prototype) >> - Included "memory-device: drop assert related to align and start of address >> space" >> >> David Hildenbrand (13): >> memory-device: drop assert related to align and start of address space >> memory-device: introduce separate config option >> pc: prepare for multi stage hotplug handlers >> pc: route all memory devices through the machine hotplug handler >> spapr: prepare for multi stage hotplug handlers >> spapr: route all memory devices through the machine hotplug handler >> spapr: handle pc-dimm unplug via hotplug handler chain >> spapr: handle cpu core unplug via hotplug handler chain >> memory-device: new functions to handle plug/unplug >> pc-dimm: implement new memory device functions >> memory-device: factor out pre-plug into hotplug handler >> memory-device: factor out unplug into hotplug handler >> memory-device: factor out plug into hotplug handler >> >> Igor Mammedov (1): >> qdev: let machine hotplug handler to override bus hotplug handler >> >> default-configs/i386-softmmu.mak | 3 +- >> default-configs/ppc64-softmmu.mak | 3 +- >> default-configs/x86_64-softmmu.mak | 3 +- >> hw/Makefile.objs | 2 +- >> hw/core/qdev.c | 6 +- >> hw/i386/pc.c | 102 ++++++++++++++++++++++------- >> hw/mem/Makefile.objs | 4 +- >> hw/mem/memory-device.c | 129 +++++++++++++++++++++++-------------- >> hw/mem/pc-dimm.c | 48 ++++++-------- >> hw/mem/trace-events | 4 +- >> hw/ppc/spapr.c | 129 +++++++++++++++++++++++++++++++------ >> include/hw/mem/memory-device.h | 21 ++++-- >> include/hw/mem/pc-dimm.h | 3 +- >> include/hw/qdev-core.h | 11 ++++ >> qapi/misc.json | 2 +- >> 15 files changed, 330 insertions(+), 140 deletions(-) >> > > As there was no negative feedback so far, I will go ahead and assume > that this approach is the right thing to do. Ok, I'll queue this. Paolo
On Wed, 30 May 2018 16:03:29 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 25/05/2018 14:43, David Hildenbrand wrote: > > On 17.05.2018 10:15, David Hildenbrand wrote: > >> 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. > >> > >> Now, we can route other devices through the machine hotplug handler, to > >> properly assign/unassign resources - like a portion in guest physical > >> address space. > >> > >> v3 -> v4: > >> - Removed the s390x bits, will send that out separately (was just a proof > >> that it works just fine with s390x) > >> - Fixed a typo and reworded a comment > >> > >> v2 -> v3: > >> - Added "memory-device: introduce separate config option" > >> - Dropped "parent_bus" check from hotplug handler lookup functions > >> - "Handly" -> "Handle" in patch description. > >> > >> v1 -> v2: > >> - Use multi stage hotplug handler instead of resource handler > >> - MemoryDevices only compiled if necessary (CONFIG_MEM_HOTPLUG) > >> - Prepare PC/SPAPR machines properly for multi stage hotplug handlers > >> - Route SPAPR unplug code via the hotunplug handler > >> - Directly include s390x support. But there are no usable memory devices > >> yet (well, only my virtio-mem prototype) > >> - Included "memory-device: drop assert related to align and start of address > >> space" > >> > >> David Hildenbrand (13): > >> memory-device: drop assert related to align and start of address space > >> memory-device: introduce separate config option > >> pc: prepare for multi stage hotplug handlers > >> pc: route all memory devices through the machine hotplug handler > >> spapr: prepare for multi stage hotplug handlers > >> spapr: route all memory devices through the machine hotplug handler > >> spapr: handle pc-dimm unplug via hotplug handler chain > >> spapr: handle cpu core unplug via hotplug handler chain > >> memory-device: new functions to handle plug/unplug > >> pc-dimm: implement new memory device functions > >> memory-device: factor out pre-plug into hotplug handler > >> memory-device: factor out unplug into hotplug handler > >> memory-device: factor out plug into hotplug handler > >> > >> Igor Mammedov (1): > >> qdev: let machine hotplug handler to override bus hotplug handler > >> > >> default-configs/i386-softmmu.mak | 3 +- > >> default-configs/ppc64-softmmu.mak | 3 +- > >> default-configs/x86_64-softmmu.mak | 3 +- > >> hw/Makefile.objs | 2 +- > >> hw/core/qdev.c | 6 +- > >> hw/i386/pc.c | 102 ++++++++++++++++++++++------- > >> hw/mem/Makefile.objs | 4 +- > >> hw/mem/memory-device.c | 129 +++++++++++++++++++++++-------------- > >> hw/mem/pc-dimm.c | 48 ++++++-------- > >> hw/mem/trace-events | 4 +- > >> hw/ppc/spapr.c | 129 +++++++++++++++++++++++++++++++------ > >> include/hw/mem/memory-device.h | 21 ++++-- > >> include/hw/mem/pc-dimm.h | 3 +- > >> include/hw/qdev-core.h | 11 ++++ > >> qapi/misc.json | 2 +- > >> 15 files changed, 330 insertions(+), 140 deletions(-) > >> > > > > As there was no negative feedback so far, I will go ahead and assume > > that this approach is the right thing to do. > > Ok, I'll queue this. I think it's a bit premature. Series would need a respin and it should also include for completness at leas one actual user (virtio-mem) to see how new interfaces/wrappers would be used and if they actually needed. > Paolo > >
On 31/05/2018 13:47, Igor Mammedov wrote: > On Wed, 30 May 2018 16:03:29 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 25/05/2018 14:43, David Hildenbrand wrote: >>> On 17.05.2018 10:15, David Hildenbrand wrote: >>>> 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. >>>> >>>> Now, we can route other devices through the machine hotplug handler, to >>>> properly assign/unassign resources - like a portion in guest physical >>>> address space. >>>> >>>> v3 -> v4: >>>> - Removed the s390x bits, will send that out separately (was just a proof >>>> that it works just fine with s390x) >>>> - Fixed a typo and reworded a comment >>>> >>>> v2 -> v3: >>>> - Added "memory-device: introduce separate config option" >>>> - Dropped "parent_bus" check from hotplug handler lookup functions >>>> - "Handly" -> "Handle" in patch description. >>>> >>>> v1 -> v2: >>>> - Use multi stage hotplug handler instead of resource handler >>>> - MemoryDevices only compiled if necessary (CONFIG_MEM_HOTPLUG) >>>> - Prepare PC/SPAPR machines properly for multi stage hotplug handlers >>>> - Route SPAPR unplug code via the hotunplug handler >>>> - Directly include s390x support. But there are no usable memory devices >>>> yet (well, only my virtio-mem prototype) >>>> - Included "memory-device: drop assert related to align and start of address >>>> space" >>>> >>>> David Hildenbrand (13): >>>> memory-device: drop assert related to align and start of address space >>>> memory-device: introduce separate config option >>>> pc: prepare for multi stage hotplug handlers >>>> pc: route all memory devices through the machine hotplug handler >>>> spapr: prepare for multi stage hotplug handlers >>>> spapr: route all memory devices through the machine hotplug handler >>>> spapr: handle pc-dimm unplug via hotplug handler chain >>>> spapr: handle cpu core unplug via hotplug handler chain >>>> memory-device: new functions to handle plug/unplug >>>> pc-dimm: implement new memory device functions >>>> memory-device: factor out pre-plug into hotplug handler >>>> memory-device: factor out unplug into hotplug handler >>>> memory-device: factor out plug into hotplug handler >>>> >>>> Igor Mammedov (1): >>>> qdev: let machine hotplug handler to override bus hotplug handler >>>> >>>> default-configs/i386-softmmu.mak | 3 +- >>>> default-configs/ppc64-softmmu.mak | 3 +- >>>> default-configs/x86_64-softmmu.mak | 3 +- >>>> hw/Makefile.objs | 2 +- >>>> hw/core/qdev.c | 6 +- >>>> hw/i386/pc.c | 102 ++++++++++++++++++++++------- >>>> hw/mem/Makefile.objs | 4 +- >>>> hw/mem/memory-device.c | 129 +++++++++++++++++++++++-------------- >>>> hw/mem/pc-dimm.c | 48 ++++++-------- >>>> hw/mem/trace-events | 4 +- >>>> hw/ppc/spapr.c | 129 +++++++++++++++++++++++++++++++------ >>>> include/hw/mem/memory-device.h | 21 ++++-- >>>> include/hw/mem/pc-dimm.h | 3 +- >>>> include/hw/qdev-core.h | 11 ++++ >>>> qapi/misc.json | 2 +- >>>> 15 files changed, 330 insertions(+), 140 deletions(-) >>>> >>> >>> As there was no negative feedback so far, I will go ahead and assume >>> that this approach is the right thing to do. >> >> Ok, I'll queue this. > I think it's a bit premature. > Series would need a respin and it should also include > for completness at leas one actual user (virtio-mem) to see > how new interfaces/wrappers would be used and if they actually needed. Yeah, I noticed that a respin was needed after sending this. Thanks, Paolo
On Fri, 25 May 2018 14:43:39 +0200 David Hildenbrand <david@redhat.com> wrote: > On 17.05.2018 10:15, David Hildenbrand wrote: > > 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. > > > > Now, we can route other devices through the machine hotplug handler, to > > properly assign/unassign resources - like a portion in guest physical > > address space. To sum up review: Some comments apply to several patches even though I commented only once. I'd suggest to restructure and split series into several: * unplug cleanups 08/14 & co * generic _preplug refactoring so we won't have to go back to that question again * extending memory_device interface 11/14 + actual user for the sake of which interface is actually extended (virtio-mem) Also more descriptive commit messages describing why change is done, current ones look like "Lets do something for some vague reason" to unaware reviewers, having virtio-mem along with new extensions to memory_device would be useful here as it could have cross reference to parts that would need it. Try to keep patches smaller and doing one thing, we can always squash them later if it would be better. I'm sorry if some comments were a bit too much or insisting on things but I'm trying to keep hotplug infrastructure simple so that later when someone else comes with related patches, I could easily read it without studying it from ground up. PS: (I'm not a fan of idea to marry virtio device with its own bus plug logic into bus-less machine hotplug, but I don't have a better suggestion or time to explore alternatives, so lets do it but keep things manageable) > > > > v3 -> v4: > > - Removed the s390x bits, will send that out separately (was just a proof > > that it works just fine with s390x) > > - Fixed a typo and reworded a comment > > > > v2 -> v3: > > - Added "memory-device: introduce separate config option" > > - Dropped "parent_bus" check from hotplug handler lookup functions > > - "Handly" -> "Handle" in patch description. > > > > v1 -> v2: > > - Use multi stage hotplug handler instead of resource handler > > - MemoryDevices only compiled if necessary (CONFIG_MEM_HOTPLUG) > > - Prepare PC/SPAPR machines properly for multi stage hotplug handlers > > - Route SPAPR unplug code via the hotunplug handler > > - Directly include s390x support. But there are no usable memory devices > > yet (well, only my virtio-mem prototype) > > - Included "memory-device: drop assert related to align and start of address > > space" > > > > David Hildenbrand (13): > > memory-device: drop assert related to align and start of address space > > memory-device: introduce separate config option > > pc: prepare for multi stage hotplug handlers > > pc: route all memory devices through the machine hotplug handler > > spapr: prepare for multi stage hotplug handlers > > spapr: route all memory devices through the machine hotplug handler > > spapr: handle pc-dimm unplug via hotplug handler chain > > spapr: handle cpu core unplug via hotplug handler chain > > memory-device: new functions to handle plug/unplug > > pc-dimm: implement new memory device functions > > memory-device: factor out pre-plug into hotplug handler > > memory-device: factor out unplug into hotplug handler > > memory-device: factor out plug into hotplug handler > > > > Igor Mammedov (1): > > qdev: let machine hotplug handler to override bus hotplug handler > > > > default-configs/i386-softmmu.mak | 3 +- > > default-configs/ppc64-softmmu.mak | 3 +- > > default-configs/x86_64-softmmu.mak | 3 +- > > hw/Makefile.objs | 2 +- > > hw/core/qdev.c | 6 +- > > hw/i386/pc.c | 102 ++++++++++++++++++++++------- > > hw/mem/Makefile.objs | 4 +- > > hw/mem/memory-device.c | 129 +++++++++++++++++++++++-------------- > > hw/mem/pc-dimm.c | 48 ++++++-------- > > hw/mem/trace-events | 4 +- > > hw/ppc/spapr.c | 129 +++++++++++++++++++++++++++++++------ > > include/hw/mem/memory-device.h | 21 ++++-- > > include/hw/mem/pc-dimm.h | 3 +- > > include/hw/qdev-core.h | 11 ++++ > > qapi/misc.json | 2 +- > > 15 files changed, 330 insertions(+), 140 deletions(-) > > > > As there was no negative feedback so far, I will go ahead and assume > that this approach is the right thing to do. >
On 01.06.2018 14:13, Igor Mammedov wrote: > On Fri, 25 May 2018 14:43:39 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 17.05.2018 10:15, David Hildenbrand wrote: >>> 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. >>> >>> Now, we can route other devices through the machine hotplug handler, to >>> properly assign/unassign resources - like a portion in guest physical >>> address space. > > To sum up review: Thanks to the review! I'll look into the details and comment where I disagree (or where we need a third opinion). > Some comments apply to several patches even though I commented only once. > > I'd suggest to restructure and split series into several: > * unplug cleanups 08/14 & co > * generic _preplug refactoring so we won't have to go back to that question again > * extending memory_device interface 11/14 + actual user for the sake of which > interface is actually extended (virtio-mem) > > Also more descriptive commit messages describing why change is done, > current ones look like "Lets do something for some vague reason" to > unaware reviewers, having virtio-mem along with new extensions to > memory_device would be useful here as it could have cross reference > to parts that would need it. I'll try to be more verbose. > > Try to keep patches smaller and doing one thing, we can always squash > them later if it would be better. I can try to split them up even further where it makes sense. Please note that including virtio-mem in the same series won't be happening, but I'll soon share a virtio-mem protoype where we reviewers can then see the interfaces in action. (or maybe virtio-pmem is the faster one) (sending everything in one series will not make reviewers happy due to the high amount of code, trust me :) ) > > I'm sorry if some comments were a bit too much or insisting on things > but I'm trying to keep hotplug infrastructure simple so that later > when someone else comes with related patches, I could easily read it > without studying it from ground up. Nothing wrong about that, we can talk about the things where I disagree. > > PS: > (I'm not a fan of idea to marry virtio device with its own bus plug logic > into bus-less machine hotplug, but I don't have a better suggestion or > time to explore alternatives, so lets do it but keep things manageable) If it's stupid but it works, it's not stupid ;) No honestly, you challenged if it would even be possible and I think we found a way to make this fit in nicely.
On 01.06.2018 14:13, Igor Mammedov wrote: > On Fri, 25 May 2018 14:43:39 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 17.05.2018 10:15, David Hildenbrand wrote: >>> 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. >>> >>> Now, we can route other devices through the machine hotplug handler, to >>> properly assign/unassign resources - like a portion in guest physical >>> address space. > > To sum up review: > Some comments apply to several patches even though I commented only once. > > I'd suggest to restructure and split series into several: > * unplug cleanups 08/14 & co > * generic _preplug refactoring so we won't have to go back to that question again Looking into the details, I don't think this is possible and should be done for address asignment. It can be done for the node. Here is the reason why: In pre_plug(), we acccess a device before it has been realized. This is okay, as long as we are using "direct properties" like the "node", that won't be touched during realize. However, for address detection we need access to the memory region. This is a "derived property", as it is based on the "memdev" property. While we can easily verify the validity of "memdev" in the pc_dimm pre_plug handler (instead of doing that in pc_dimm_realize()), we cannot access the memory region itself before the device has been realized using get_memory_region() in all cases. While this works for PC_DIMM (pc_dimm_get_memory_region() is only based on dimm->hostmem, which we can verify as stated), we cannot do the same thing for NVDIMM, because nvdimm_get_memory_region() relies on "realize" already being called and setting up "nvdimm->nvdimm_mr". In short: we should call device functions only after realize(), therefore address assignment is not possible during pre_plug.