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);
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;
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;
--
2.17.1
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;
Am 13.09.18 um 14:20 schrieb Igor Mammedov:
> 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?
The MemoryRegion pointer that is returned is not const, so this getter
can never take a const pointer (as the MemoryRegion is e.g. part of the
MemoryDeviceState for NVDIMM) - unless we cast away the const
internally, which also sounds bad.
We could keep get_region_size() (along with the const pointer) and
add/factor out get_memory_region() in addition. What do you prefer?
Thanks!
--
Thanks,
David / dhildenb
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;
>
>
Am 14.09.18 um 16:34 schrieb Igor Mammedov: > 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. > Sure, I can add more details. >>> >>> 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. Yes, I will extend that, too. > > > 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 get_region_size() size is no longer necessary when factoring pre_plug/plug_unplug out. Especially we need in addition of the size: 1. the alignment of the region (patch #9) 2. the region for memory_region_add_subregion() to plug it (patch #10) 3. the region for memory_region_del_subregion() to unplug it (patch #11) > > * 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. > Yes, this is the main reason behind it. I guess getting rid of PCDIMMDeviceClass::get_memory_region() makes it clear that this is the next step of factoring out. I could turn this patch into said "factor get_memory_region() out" patch and keep get_region_size() for now. Thanks! -- Thanks, David / dhildenb
© 2016 - 2026 Red Hat, Inc.