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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 13 Sep 2018 14:20:25 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> 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().
this part isn't clear and doesn't really answer "why" patch's been written
and how it will really help.

> > 
> > 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.
It might be better to document expectation as part of get_memory_region() doc comment.


I'm not really getting reasoning behind this patch.
 * Why get_region_size() doesn't work for you?
   benefits I see is that's one less get_foo_size() callback,
   so it becomes less confusing

 * I get we might need access to memory region at MemoryDeviceClass level,
   but why are you keeping PCDIMMDeviceClass::get_memory_region()?
   I'd expect the later being generalized (moved) to MemoryDeviceClass
   so we would end up with one level indirection that we have now.

> > 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;  
> 
>