| Message ID | 20180920103243.28474-8-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory-device: complete refactoring + virtio-pmem | expand |
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
>> >> - 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!
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.
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 :)
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.
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
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(-)