We will factor out get_memory_region() from pc-dimm to memory device code
soon. Once that is done, get_region_size() can be implemented
generically and essentially be replaced by
memory_device_get_region_size (and work only on get_memory_region()).
We have some users of get_memory_region() (spapr and pc-dimm code) that are
only interested in the size. So let's rework them to use
memory_device_get_region_size() first, then we can factor out
get_memory_region() and eventually remove get_region_size() without
touching the same code multiple times.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/mem/memory-device.c | 13 ++++++++++---
hw/mem/pc-dimm.c | 10 ++++------
hw/ppc/spapr.c | 27 +++++++++------------------
include/hw/mem/memory-device.h | 2 ++
4 files changed, 25 insertions(+), 27 deletions(-)
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index bdcee6fd55..2fa68b3730 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -57,10 +57,9 @@ 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);
- const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
if (dev->realized) {
- *size += mdc->get_region_size(md, &error_abort);
+ *size += memory_device_get_region_size(md, &error_abort);
}
}
@@ -169,7 +168,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
uint64_t md_size, md_addr;
md_addr = mdc->get_addr(md);
- md_size = mdc->get_region_size(md, &error_abort);
+ md_size = memory_device_get_region_size(md, &error_abort);
if (ranges_overlap(md_addr, md_size, new_addr, size)) {
if (hint) {
@@ -268,6 +267,14 @@ void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr)
memory_region_del_subregion(&ms->device_memory->mr, mr);
}
+uint64_t memory_device_get_region_size(const MemoryDeviceState *md,
+ Error **errp)
+{
+ MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
+
+ return mdc->get_region_size(md, errp);
+}
+
static const TypeInfo memory_device_info = {
.name = TYPE_MEMORY_DEVICE,
.parent = TYPE_INTERFACE,
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 4bf1a0acc9..a06da7e08e 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -163,16 +163,14 @@ static Property pc_dimm_properties[] = {
static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
+ Error *local_err = NULL;
uint64_t value;
- MemoryRegion *mr;
- PCDIMMDevice *dimm = PC_DIMM(obj);
- PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
- mr = ddc->get_memory_region(dimm, errp);
- if (!mr) {
+ value = memory_device_get_region_size(MEMORY_DEVICE(obj), errp);
+ if (local_err) {
+ error_propagate(errp, local_err);
return;
}
- value = memory_region_size(mr);
visit_type_uint64(v, name, &value, errp);
}
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4edb6c7d16..b56ce6b7aa 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3124,20 +3124,17 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
{
Error *local_err = NULL;
sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
- PCDIMMDevice *dimm = PC_DIMM(dev);
- PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
uint64_t size, addr;
uint32_t node;
- size = memory_region_size(mr);
+ size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
pc_dimm_plug(dev, MACHINE(ms), &local_err);
if (local_err) {
goto out;
}
- addr = object_property_get_uint(OBJECT(dimm),
+ addr = object_property_get_uint(OBJECT(dev),
PC_DIMM_ADDR_PROP, &local_err);
if (local_err) {
goto out_unplug;
@@ -3165,10 +3162,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
{
const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
- PCDIMMDevice *dimm = PC_DIMM(dev);
- PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
Error *local_err = NULL;
- MemoryRegion *mr;
uint64_t size;
Object *memdev;
hwaddr pagesize;
@@ -3178,11 +3172,11 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
return;
}
- mr = ddc->get_memory_region(dimm, errp);
- if (!mr) {
+ size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
+ if (local_err) {
+ error_propagate(errp, local_err);
return;
}
- size = memory_region_size(mr);
if (size % SPAPR_MEMORY_BLOCK_SIZE) {
error_setg(errp, "Hotplugged memory size must be a multiple of "
@@ -3190,7 +3184,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
return;
}
- memdev = object_property_get_link(OBJECT(dimm), PC_DIMM_MEMDEV_PROP,
+ memdev = object_property_get_link(OBJECT(dev), PC_DIMM_MEMDEV_PROP,
&error_abort);
pagesize = host_memory_backend_pagesize(MEMORY_BACKEND(memdev));
spapr_check_pagesize(spapr, pagesize, &local_err);
@@ -3254,9 +3248,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
PCDIMMDevice *dimm)
{
sPAPRDRConnector *drc;
- PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
- uint64_t size = memory_region_size(mr);
+ uint64_t size = memory_device_get_region_size(MEMORY_DEVICE(dimm),
+ &error_abort);
uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
uint32_t avail_lmbs = 0;
uint64_t addr_start, addr;
@@ -3322,14 +3315,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
Error *local_err = NULL;
PCDIMMDevice *dimm = PC_DIMM(dev);
- PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
- MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
uint32_t nr_lmbs;
uint64_t size, addr_start, addr;
int i;
sPAPRDRConnector *drc;
- size = memory_region_size(mr);
+ size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index d6853156ff..12f8ca9eb1 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -60,5 +60,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
uint64_t addr);
void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr);
+uint64_t memory_device_get_region_size(const MemoryDeviceState *md,
+ Error **errp);
#endif
--
2.17.1
On Thu, Sep 20, 2018 at 12:32:28PM +0200, David Hildenbrand wrote:
> We will factor out get_memory_region() from pc-dimm to memory device code
> soon. Once that is done, get_region_size() can be implemented
> generically and essentially be replaced by
> memory_device_get_region_size (and work only on get_memory_region()).
>
> We have some users of get_memory_region() (spapr and pc-dimm code) that are
> only interested in the size. So let's rework them to use
> memory_device_get_region_size() first, then we can factor out
> get_memory_region() and eventually remove get_region_size() without
> touching the same code multiple times.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> hw/mem/memory-device.c | 13 ++++++++++---
> hw/mem/pc-dimm.c | 10 ++++------
> hw/ppc/spapr.c | 27 +++++++++------------------
> include/hw/mem/memory-device.h | 2 ++
> 4 files changed, 25 insertions(+), 27 deletions(-)
>
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index bdcee6fd55..2fa68b3730 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -57,10 +57,9 @@ 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);
> - const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
>
> if (dev->realized) {
> - *size += mdc->get_region_size(md, &error_abort);
> + *size += memory_device_get_region_size(md, &error_abort);
> }
> }
>
> @@ -169,7 +168,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
> uint64_t md_size, md_addr;
>
> md_addr = mdc->get_addr(md);
> - md_size = mdc->get_region_size(md, &error_abort);
> + md_size = memory_device_get_region_size(md, &error_abort);
>
> if (ranges_overlap(md_addr, md_size, new_addr, size)) {
> if (hint) {
> @@ -268,6 +267,14 @@ void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr)
> memory_region_del_subregion(&ms->device_memory->mr, mr);
> }
>
> +uint64_t memory_device_get_region_size(const MemoryDeviceState *md,
> + Error **errp)
> +{
> + MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
> +
> + return mdc->get_region_size(md, errp);
> +}
> +
> static const TypeInfo memory_device_info = {
> .name = TYPE_MEMORY_DEVICE,
> .parent = TYPE_INTERFACE,
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 4bf1a0acc9..a06da7e08e 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -163,16 +163,14 @@ static Property pc_dimm_properties[] = {
> static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> + Error *local_err = NULL;
> uint64_t value;
> - MemoryRegion *mr;
> - PCDIMMDevice *dimm = PC_DIMM(obj);
> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>
> - mr = ddc->get_memory_region(dimm, errp);
> - if (!mr) {
> + value = memory_device_get_region_size(MEMORY_DEVICE(obj), errp);
Given the below, that should be &local_err above, no?
> + if (local_err) {
> + error_propagate(errp, local_err);
> return;
> }
> - value = memory_region_size(mr);
>
> visit_type_uint64(v, name, &value, errp);
> }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4edb6c7d16..b56ce6b7aa 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3124,20 +3124,17 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> {
> Error *local_err = NULL;
> sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> - PCDIMMDevice *dimm = PC_DIMM(dev);
> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> uint64_t size, addr;
> uint32_t node;
>
> - size = memory_region_size(mr);
> + size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
>
> pc_dimm_plug(dev, MACHINE(ms), &local_err);
> if (local_err) {
> goto out;
> }
>
> - addr = object_property_get_uint(OBJECT(dimm),
> + addr = object_property_get_uint(OBJECT(dev),
> PC_DIMM_ADDR_PROP, &local_err);
It bothers me a bit that we're no longer explicitly forcing this to be
a DIMM object pointer, but we're still using PC_DIMM_ADDR_PROP.
> if (local_err) {
> goto out_unplug;
> @@ -3165,10 +3162,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> {
> const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> - PCDIMMDevice *dimm = PC_DIMM(dev);
> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> Error *local_err = NULL;
> - MemoryRegion *mr;
> uint64_t size;
> Object *memdev;
> hwaddr pagesize;
> @@ -3178,11 +3172,11 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> return;
> }
>
> - mr = ddc->get_memory_region(dimm, errp);
> - if (!mr) {
> + size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
And this should be &local_err above as well, no?
> + if (local_err) {
> + error_propagate(errp, local_err);
> return;
> }
> - size = memory_region_size(mr);
>
> if (size % SPAPR_MEMORY_BLOCK_SIZE) {
> error_setg(errp, "Hotplugged memory size must be a multiple of "
> @@ -3190,7 +3184,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> return;
> }
>
> - memdev = object_property_get_link(OBJECT(dimm), PC_DIMM_MEMDEV_PROP,
> + memdev = object_property_get_link(OBJECT(dev), PC_DIMM_MEMDEV_PROP,
> &error_abort);
> pagesize = host_memory_backend_pagesize(MEMORY_BACKEND(memdev));
> spapr_check_pagesize(spapr, pagesize, &local_err);
> @@ -3254,9 +3248,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> PCDIMMDevice *dimm)
> {
> sPAPRDRConnector *drc;
> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> - uint64_t size = memory_region_size(mr);
> + uint64_t size = memory_device_get_region_size(MEMORY_DEVICE(dimm),
> + &error_abort);
> uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> uint32_t avail_lmbs = 0;
> uint64_t addr_start, addr;
> @@ -3322,14 +3315,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> Error *local_err = NULL;
> PCDIMMDevice *dimm = PC_DIMM(dev);
> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> uint32_t nr_lmbs;
> uint64_t size, addr_start, addr;
> int i;
> sPAPRDRConnector *drc;
>
> - size = memory_region_size(mr);
> + size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
> nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>
> addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index d6853156ff..12f8ca9eb1 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -60,5 +60,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
> void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
> uint64_t addr);
> void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr);
> +uint64_t memory_device_get_region_size(const MemoryDeviceState *md,
> + Error **errp);
>
> #endif
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
>>
>> - mr = ddc->get_memory_region(dimm, errp);
>> - if (!mr) {
>> + value = memory_device_get_region_size(MEMORY_DEVICE(obj), errp);
>
> Given the below, that should be &local_err above, no?
Yes, very right!
>
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> return;
>> }
>> - value = memory_region_size(mr);
>>
>> visit_type_uint64(v, name, &value, errp);
>> }
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 4edb6c7d16..b56ce6b7aa 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3124,20 +3124,17 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> {
>> Error *local_err = NULL;
>> sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>> - PCDIMMDevice *dimm = PC_DIMM(dev);
>> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>> uint64_t size, addr;
>> uint32_t node;
>>
>> - size = memory_region_size(mr);
>> + size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
>>
>> pc_dimm_plug(dev, MACHINE(ms), &local_err);
>> if (local_err) {
>> goto out;
>> }
>>
>> - addr = object_property_get_uint(OBJECT(dimm),
>> + addr = object_property_get_uint(OBJECT(dev),
>> PC_DIMM_ADDR_PROP, &local_err);
>
> It bothers me a bit that we're no longer explicitly forcing this to be
> a DIMM object pointer, but we're still using PC_DIMM_ADDR_PROP.
Well, OBJECT(PC_DIMM(dev))) looks stange and should not be necessary.
We could do a g_assert(object_dynamic_cast(dimm, TYPE_PC_DIMM)) above
the function. What do you think?
We could also do a memory-device::get_addr() instead of going via the
property. What do you think?
>
>> if (local_err) {
>> goto out_unplug;
>> @@ -3165,10 +3162,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> {
>> const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
>> sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
>> - PCDIMMDevice *dimm = PC_DIMM(dev);
>> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>> Error *local_err = NULL;
>> - MemoryRegion *mr;
>> uint64_t size;
>> Object *memdev;
>> hwaddr pagesize;
>> @@ -3178,11 +3172,11 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> return;
>> }
>>
>> - mr = ddc->get_memory_region(dimm, errp);
>> - if (!mr) {
>> + size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
>
> And this should be &local_err above as well, no?
Yes, copy and paste error :(
Thanks!
--
Thanks,
David / dhildenb
On Fri, Sep 21, 2018 at 09:15:09AM +0200, David Hildenbrand wrote:
>
> >>
> >> - mr = ddc->get_memory_region(dimm, errp);
> >> - if (!mr) {
> >> + value = memory_device_get_region_size(MEMORY_DEVICE(obj), errp);
> >
> > Given the below, that should be &local_err above, no?
>
> Yes, very right!
>
> >
> >> + if (local_err) {
> >> + error_propagate(errp, local_err);
> >> return;
> >> }
> >> - value = memory_region_size(mr);
> >>
> >> visit_type_uint64(v, name, &value, errp);
> >> }
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 4edb6c7d16..b56ce6b7aa 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3124,20 +3124,17 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> {
> >> Error *local_err = NULL;
> >> sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> >> - PCDIMMDevice *dimm = PC_DIMM(dev);
> >> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> >> uint64_t size, addr;
> >> uint32_t node;
> >>
> >> - size = memory_region_size(mr);
> >> + size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
> >>
> >> pc_dimm_plug(dev, MACHINE(ms), &local_err);
> >> if (local_err) {
> >> goto out;
> >> }
> >>
> >> - addr = object_property_get_uint(OBJECT(dimm),
> >> + addr = object_property_get_uint(OBJECT(dev),
> >> PC_DIMM_ADDR_PROP, &local_err);
> >
> > It bothers me a bit that we're no longer explicitly forcing this to be
> > a DIMM object pointer, but we're still using PC_DIMM_ADDR_PROP.
>
> Well, OBJECT(PC_DIMM(dev))) looks stange and should not be necessary.
>
> We could do a g_assert(object_dynamic_cast(dimm, TYPE_PC_DIMM)) above
> the function. What do you think?
>
> We could also do a memory-device::get_addr() instead of going via the
> property. What do you think?
I think that's a better idea.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On 21/09/2018 10:38, David Gibson wrote:
> On Fri, Sep 21, 2018 at 09:15:09AM +0200, David Hildenbrand wrote:
>>
>>>>
>>>> - mr = ddc->get_memory_region(dimm, errp);
>>>> - if (!mr) {
>>>> + value = memory_device_get_region_size(MEMORY_DEVICE(obj), errp);
>>>
>>> Given the below, that should be &local_err above, no?
>>
>> Yes, very right!
>>
>>>
>>>> + if (local_err) {
>>>> + error_propagate(errp, local_err);
>>>> return;
>>>> }
>>>> - value = memory_region_size(mr);
>>>>
>>>> visit_type_uint64(v, name, &value, errp);
>>>> }
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 4edb6c7d16..b56ce6b7aa 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -3124,20 +3124,17 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>> {
>>>> Error *local_err = NULL;
>>>> sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>>>> - PCDIMMDevice *dimm = PC_DIMM(dev);
>>>> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>>>> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>>>> uint64_t size, addr;
>>>> uint32_t node;
>>>>
>>>> - size = memory_region_size(mr);
>>>> + size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
>>>>
>>>> pc_dimm_plug(dev, MACHINE(ms), &local_err);
>>>> if (local_err) {
>>>> goto out;
>>>> }
>>>>
>>>> - addr = object_property_get_uint(OBJECT(dimm),
>>>> + addr = object_property_get_uint(OBJECT(dev),
>>>> PC_DIMM_ADDR_PROP, &local_err);
>>>
>>> It bothers me a bit that we're no longer explicitly forcing this to be
>>> a DIMM object pointer, but we're still using PC_DIMM_ADDR_PROP.
>>
>> Well, OBJECT(PC_DIMM(dev))) looks stange and should not be necessary.
>>
>> We could do a g_assert(object_dynamic_cast(dimm, TYPE_PC_DIMM)) above
>> the function. What do you think?
>>
>> We could also do a memory-device::get_addr() instead of going via the
>> property. What do you think?
>
> I think that's a better idea.
>
Hmm, but looking at spapr_memory_plug(), we already get the node via
OBJECT(dev). And in spapr_memory_pre_plug(), we get the memdev. We are
not able to access these properties via the memory device interface.
So I guess adding assertions
g_assert(object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM));
Would be the right thing to do. On the other hand, these two static
functions are already called "if (object_dynamic_cast(OBJECT(dev),
TYPE_PC_DIMM))".
However, I guess I will go another path: Make pc_dimm_plug() and friends
eat a TYPE_PC_DIMM instead of a TYPE_DEVICE. Then this cast will not go
away. Problem solved :)
--
Thanks,
David / dhildenb
On Mon, 24 Sep 2018 11:03:35 +0200
David Hildenbrand <david@redhat.com> wrote:
> On 21/09/2018 10:38, David Gibson wrote:
> > On Fri, Sep 21, 2018 at 09:15:09AM +0200, David Hildenbrand wrote:
> >>
> >>>>
> >>>> - mr = ddc->get_memory_region(dimm, errp);
> >>>> - if (!mr) {
> >>>> + value = memory_device_get_region_size(MEMORY_DEVICE(obj), errp);
> >>>
> >>> Given the below, that should be &local_err above, no?
> >>
> >> Yes, very right!
> >>
> >>>
> >>>> + if (local_err) {
> >>>> + error_propagate(errp, local_err);
> >>>> return;
> >>>> }
> >>>> - value = memory_region_size(mr);
> >>>>
> >>>> visit_type_uint64(v, name, &value, errp);
> >>>> }
> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>> index 4edb6c7d16..b56ce6b7aa 100644
> >>>> --- a/hw/ppc/spapr.c
> >>>> +++ b/hw/ppc/spapr.c
> >>>> @@ -3124,20 +3124,17 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >>>> {
> >>>> Error *local_err = NULL;
> >>>> sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> >>>> - PCDIMMDevice *dimm = PC_DIMM(dev);
> >>>> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >>>> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> >>>> uint64_t size, addr;
> >>>> uint32_t node;
> >>>>
> >>>> - size = memory_region_size(mr);
> >>>> + size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
> >>>>
> >>>> pc_dimm_plug(dev, MACHINE(ms), &local_err);
> >>>> if (local_err) {
> >>>> goto out;
> >>>> }
> >>>>
> >>>> - addr = object_property_get_uint(OBJECT(dimm),
> >>>> + addr = object_property_get_uint(OBJECT(dev),
> >>>> PC_DIMM_ADDR_PROP, &local_err);
> >>>
> >>> It bothers me a bit that we're no longer explicitly forcing this to be
> >>> a DIMM object pointer, but we're still using PC_DIMM_ADDR_PROP.
> >>
> >> Well, OBJECT(PC_DIMM(dev))) looks stange and should not be necessary.
> >>
> >> We could do a g_assert(object_dynamic_cast(dimm, TYPE_PC_DIMM)) above
> >> the function. What do you think?
> >>
> >> We could also do a memory-device::get_addr() instead of going via the
> >> property. What do you think?
> >
> > I think that's a better idea.
> >
>
> Hmm, but looking at spapr_memory_plug(), we already get the node via
> OBJECT(dev). And in spapr_memory_pre_plug(), we get the memdev. We are
> not able to access these properties via the memory device interface.
>
> So I guess adding assertions
>
> g_assert(object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM));
>
> Would be the right thing to do. On the other hand, these two static
> functions are already called "if (object_dynamic_cast(OBJECT(dev),
> TYPE_PC_DIMM))".
Given that I guess assert wouldn't be necessary
>
> However, I guess I will go another path: Make pc_dimm_plug() and friends
> eat a TYPE_PC_DIMM instead of a TYPE_DEVICE. Then this cast will not go
> away. Problem solved :)
Look like a good idea
On Mon, Sep 24, 2018 at 11:03:35AM +0200, David Hildenbrand wrote:
> On 21/09/2018 10:38, David Gibson wrote:
> > On Fri, Sep 21, 2018 at 09:15:09AM +0200, David Hildenbrand wrote:
> >>
> >>>>
> >>>> - mr = ddc->get_memory_region(dimm, errp);
> >>>> - if (!mr) {
> >>>> + value = memory_device_get_region_size(MEMORY_DEVICE(obj), errp);
> >>>
> >>> Given the below, that should be &local_err above, no?
> >>
> >> Yes, very right!
> >>
> >>>
> >>>> + if (local_err) {
> >>>> + error_propagate(errp, local_err);
> >>>> return;
> >>>> }
> >>>> - value = memory_region_size(mr);
> >>>>
> >>>> visit_type_uint64(v, name, &value, errp);
> >>>> }
> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>> index 4edb6c7d16..b56ce6b7aa 100644
> >>>> --- a/hw/ppc/spapr.c
> >>>> +++ b/hw/ppc/spapr.c
> >>>> @@ -3124,20 +3124,17 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >>>> {
> >>>> Error *local_err = NULL;
> >>>> sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> >>>> - PCDIMMDevice *dimm = PC_DIMM(dev);
> >>>> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >>>> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> >>>> uint64_t size, addr;
> >>>> uint32_t node;
> >>>>
> >>>> - size = memory_region_size(mr);
> >>>> + size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
> >>>>
> >>>> pc_dimm_plug(dev, MACHINE(ms), &local_err);
> >>>> if (local_err) {
> >>>> goto out;
> >>>> }
> >>>>
> >>>> - addr = object_property_get_uint(OBJECT(dimm),
> >>>> + addr = object_property_get_uint(OBJECT(dev),
> >>>> PC_DIMM_ADDR_PROP, &local_err);
> >>>
> >>> It bothers me a bit that we're no longer explicitly forcing this to be
> >>> a DIMM object pointer, but we're still using PC_DIMM_ADDR_PROP.
> >>
> >> Well, OBJECT(PC_DIMM(dev))) looks stange and should not be necessary.
> >>
> >> We could do a g_assert(object_dynamic_cast(dimm, TYPE_PC_DIMM)) above
> >> the function. What do you think?
> >>
> >> We could also do a memory-device::get_addr() instead of going via the
> >> property. What do you think?
> >
> > I think that's a better idea.
> >
>
> Hmm, but looking at spapr_memory_plug(), we already get the node via
> OBJECT(dev). And in spapr_memory_pre_plug(), we get the memdev. We are
> not able to access these properties via the memory device interface.
>
> So I guess adding assertions
>
> g_assert(object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM));
>
> Would be the right thing to do. On the other hand, these two static
> functions are already called "if (object_dynamic_cast(OBJECT(dev),
> TYPE_PC_DIMM))".
>
> However, I guess I will go another path: Make pc_dimm_plug() and friends
> eat a TYPE_PC_DIMM instead of a TYPE_DEVICE. Then this cast will not go
> away. Problem solved :)
In the short term, yes. We will want to generalize the PAPR hotplug
code to memory devices other than PC DIMM in the fairly near future,
though.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
© 2016 - 2025 Red Hat, Inc.