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

[David Gibson]

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

signature.asc
Description: PGP signature