Re: [qemu-s390x] [Qemu-devel] [PATCH v3 3/3] pc-dimm: factor out address space logic into MemoryDevice code

[David Hildenbrand]

On 24.04.2018 15:28, Igor Mammedov wrote:
> On Mon, 23 Apr 2018 14:44:34 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> On 23.04.2018 14:19, Igor Mammedov wrote:
>>> On Fri, 20 Apr 2018 14:34:56 +0200
>>> David Hildenbrand <address@hidden> wrote:
> considering v4 queued, I'm dropping mostly nor relevant points at this point.
> wrt, virtio I'll elaborate more in reply to Pankaj
> 
> [...]
> 
>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>>>> index b860c9c582..b96efa3bf4 100644
>>>> --- a/hw/mem/memory-device.c
>>>> +++ b/hw/mem/memory-device.c
>>>> @@ -15,6 +15,8 @@
>>>>  #include "qapi/error.h"
>>>>  #include "hw/boards.h"
>>>>  #include "qemu/range.h"
>>>> +#include "hw/virtio/vhost.h"
>>>> +#include "sysemu/kvm.h"
>>>>  
>>>>  static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
>>>>  {
>>>> @@ -106,6 +108,166 @@ uint64_t get_plugged_memory_size(void)
>>>>      return size;
>>>>  }
>>>>  
>>>> +static int memory_device_used_region_size_internal(Object *obj, void 
>>>> *opaque)
>>>> +{
>>>> +    uint64_t *size = opaque;
>>>> +
>>>> +    if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
>>>> +        DeviceState *dev = DEVICE(obj);
>>>> +        MemoryDeviceState *md = MEMORY_DEVICE(obj);
>>>> +        MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
>>>> +
>>>> +        if (dev->realized) {
>>>> +            *size += mdc->get_region_size(md, &error_abort);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    object_child_foreach(obj, memory_device_used_region_size_internal, 
>>>> opaque);
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static uint64_t memory_device_used_region_size(void)
>>>> +{
>>>> +    uint64_t size = 0;
>>>> +
>>>> +    memory_device_used_region_size_internal(qdev_get_machine(), &size);
>>>> +
>>>> +    return size;
>>>> +}
>>>> +
>>>> +uint64_t memory_device_get_free_addr(uint64_t *hint, uint64_t align,
>>>> +                                     uint64_t size, Error **errp)  
>>> I'd suggest to pc_dimm_memory_plug/pc_dimm_get_free_addr first,
>>> namely most of the stuff it does like checks and assigning default
>>> values should go to pre_plug (pre realize) handler and then only
>>> actual mapping is left for plug (after realize) handler to deal with:
>>>   
>>
>> Can you elaborate what you mean by pre-plug? If this is about pre plug
>> handler of the (machine) hotplug handler, it might be problematic for
>> virtio devices.
> yes, something along these lines: c871bc70b
> 
> 

Yes, we can factor that out (at least) for pc-dimm later on easily.
Seems to be just about moving a couple of calls.

-- 

Thanks,

David / dhildenb