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
