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