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.

Looks promising :) Will post something based on your qdev patch as soon
as I have it cleaned up.

Thanks for your help!

> 
>>
>>>
>>> Could you try something along these lines (I'll post a patch to
>>> override default hotplug handler as reply here for you to pick up,
>>> that should make following work):
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 100dfdc..c400c0c 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -393,12 +393,30 @@ static void s390_machine_reset(void)
>>>      s390_ipl_prepare_cpu(ipl_cpu);
>>>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
>>>  }
>>> +static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev,
>>> +                                         DeviceState *dev, Error **errp)
>>> +{
>>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) {
>>> +        /* do checks && set default values if it weren't set by user */
>>> +        /* possibly pass to device's bus pre_plug handler if need */
>>> +    }
>>> +}
>>>  
>>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>>>                                       DeviceState *dev, Error **errp)
>>>  {
>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>          s390_cpu_plug(hotplug_dev, dev, errp);
>>> +    } if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) {
>>> +        HotplugHandler *hotplug_ctrl = dev->parent_bus->hotplug_handler;
>>> +
>>> +        /* do machine specific wiring, i.e.
>>> +         * assign resources specified by user or by foo_pre_plug(),
>>> +         * like map region into machine address space at specified address 
>>> */
>>> +        if (OK) {
>>> +            /* pass control down to bus specific hotplug chain */
>>> +            hotplug_handler_plug(hotplug_ctrl, dev, errp);
>>> +        }
>>>      }
>>>  }
>>>  
>>> @@ -449,6 +467,8 @@ static HotplugHandler 
>>> *s390_get_hotplug_handler(MachineState *machine,
>>>  {
>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>          return HOTPLUG_HANDLER(machine);
>>> +    } (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM)) {
>>> +        return HOTPLUG_HANDLER(machine);
>>>      }
>>>      return NULL;
>>>  }
>>>   
>>
>>
> 


-- 

Thanks,

David / dhildenb