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