On Thu, 13 Sep 2018 14:20:25 +0200
Igor Mammedov <imamm...@redhat.com> wrote:
> On Wed, 29 Aug 2018 17:36:09 +0200
> David Hildenbrand <da...@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 <da...@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;
>
>
