Re: [Qemu-devel] [PATCH v2 05/20] memory-device: convert get_region_size() to get_memory_region()

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 29 Aug 2018 17:36:09 +0200
David Hildenbrand <david@redhat.com> wrote:

> To factor out plugging and unplugging of memory device we need access to
> the memory region. So let's replace get_region_size() by
> get_memory_region().
> 
> If any memory device will in the future have multiple memory regions
> that make up the total region size, it can simply use a wrapping memory
> region to embed the other ones.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/memory-device.c         | 10 ++++++----
>  hw/mem/pc-dimm.c               | 19 ++++++++++++++-----
>  include/hw/mem/memory-device.h |  2 +-
>  3 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index d87599c280..3bd30d98bb 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -56,11 +56,13 @@ static int memory_device_used_region_size(Object *obj, void *opaque)
>  
>      if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
>          const DeviceState *dev = DEVICE(obj);
> -        const MemoryDeviceState *md = MEMORY_DEVICE(obj);
> +        MemoryDeviceState *md = MEMORY_DEVICE(obj);
it's a getter, why do we loose 'const' here?

>          const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
>  
>          if (dev->realized) {
> -            *size += mdc->get_region_size(md, &error_abort);
> +            MemoryRegion *mr = mdc->get_memory_region(md, &error_abort);
> +
> +            *size += memory_region_size(mr);
>          }
>      }
>  
> @@ -162,12 +164,12 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
>      /* find address range that will fit new memory device */
>      object_child_foreach(OBJECT(ms), memory_device_build_list, &list);
>      for (item = list; item; item = g_slist_next(item)) {
> -        const MemoryDeviceState *md = item->data;
> +        MemoryDeviceState *md = item->data;
ditto

>          const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
>          uint64_t md_size, md_addr;
>  
>          md_addr = mdc->get_addr(md);
> -        md_size = mdc->get_region_size(md, &error_abort);
> +        md_size = memory_region_size(mdc->get_memory_region(md, &error_abort));
>          if (*errp) {
>              goto out;
>          }
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 4bf1a0acc9..a208b17c64 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -236,8 +236,8 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
>      return dimm->addr;
>  }
>  
> -static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md,
> -                                           Error **errp)
> +static uint64_t pc_dimm_md_get_plugged_size(const MemoryDeviceState *md,
> +                                            Error **errp)
>  {
>      /* dropping const here is fine as we don't touch the memory region */
>      PCDIMMDevice *dimm = PC_DIMM(md);
> @@ -249,9 +249,19 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md,
>          return 0;
>      }
>  
> +    /* for a dimm plugged_size == region_size */
>      return memory_region_size(mr);
>  }
>  
> +static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
> +                                                  Error **errp)
> +{
> +    PCDIMMDevice *dimm = PC_DIMM(md);
> +    const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
> +
> +    return ddc->get_memory_region(dimm, errp);
> +}
> +
>  static void pc_dimm_md_fill_device_info(const MemoryDeviceState *md,
>                                          MemoryDeviceInfo *info)
>  {
> @@ -297,9 +307,8 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>      ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
>  
>      mdc->get_addr = pc_dimm_md_get_addr;
> -    /* for a dimm plugged_size == region_size */
> -    mdc->get_plugged_size = pc_dimm_md_get_region_size;
> -    mdc->get_region_size = pc_dimm_md_get_region_size;
> +    mdc->get_plugged_size = pc_dimm_md_get_plugged_size;
> +    mdc->get_memory_region = pc_dimm_md_get_memory_region;
>      mdc->fill_device_info = pc_dimm_md_fill_device_info;
>  }
>  
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index f02b229837..852fd8f98a 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -34,7 +34,7 @@ typedef struct MemoryDeviceClass {
>  
>      uint64_t (*get_addr)(const MemoryDeviceState *md);
>      uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp);
> -    uint64_t (*get_region_size)(const MemoryDeviceState *md, Error **errp);
> +    MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
>      void (*fill_device_info)(const MemoryDeviceState *md,
>                               MemoryDeviceInfo *info);
>  } MemoryDeviceClass;