Re: [Qemu-ppc] [PATCH v3 07/22] memory-device: add and use memory_device_get_region_size()

[David Hildenbrand]

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