On 10/05/2018 15:32, Igor Mammedov wrote:
> On Thu, 10 May 2018 15:20:55 +0200
> David Hildenbrand <da...@redhat.com> wrote:
>
>> On 10.05.2018 15:02, Igor Mammedov wrote:
>>> On Wed, 9 May 2018 16:13:14 +0200
>>> David Hildenbrand <da...@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?
--
Thanks,
David / dhildenb
