Re: [qemu-s390x] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[David Hildenbrand]

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