On 01.06.2018 14:13, Igor Mammedov wrote: > On Fri, 25 May 2018 14:43:39 +0200 > David Hildenbrand <da...@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. -- Thanks, David / dhildenb
