Re: [Qemu-devel] [qemu-s390x] [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers

[David Hildenbrand]

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