Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/5] hw/ppc: removing spapr_drc_detach_cb opaques

[Daniel Henrique Barboza]

On 05/03/2017 11:33 AM, Laurent Vivier wrote:
> On 30/04/2017 19:25, Daniel Henrique Barboza wrote:
> > Following up the previous detach_cb change, this patch removes the
> > detach_cb_opaque entirely from the code.
> >
> > The reason is that the drc->detach_cb_opaque object can't be
> > restored in the post load of the upcoming DRC migration and no detach
> > callbacks actually need this opaque. 'spapr_core_release' is
> > receiving it as NULL, 'spapr_phb_remove_pci_device_cb' is receiving
> > a phb object as opaque but is't using it. These were trivial removal
> > cases.
> >
> > However, the LM removal callback 'spapr_lmb_release' is receiving
> > and using the opaque object, a 'sPAPRDIMMState' struct. This struct
> > holds the number of LMBs the DIMM object contains and the callback
> > was using this counter as a countdown to check if all LMB DRCs were
> > release before proceeding to the DIMM unplug. To remove the need of
> > this callback we have choices such as:
> >
> > - migrate the 'sPAPRDIMMState' struct. This would require creating a
> > QTAILQ to store all DIMMStates and an additional 'dimm_id' field to
> > associate the DIMMState with the DIMM object. We could attach this
> > QTAILQ to the 'sPAPRPHBState' and retrieve it later in the callback.
> Did you think about adding the nr_lmbs into PCDIMMDevice structure?
> Or to create a SPAPRPCDIMMDevice with PCDIMMDevice parent_obj and
> nr_lmbs field we can retrieve from the DeviceState pointer?

Haven't thought about the first option. I've checked here in the code and
it seems like PCDIMMDevice is a common structure used by other archs
as well. I am a bit skeptical about adding nr_lmbs there just because we 
have
a pseries specific behavior (DRCs) that requires that a DIMM unplug must
be done in chunks of SPAPR_MEMORY_BLOCK_SIZE.

Your second option sounds like the suggestion I gave in the commit message.
We can use the existent sPAPRDIMMState like your SPAPRPCDIMMDevice, 
parent_obj
would be some sort of unique ID that would identify the owner of this 
DIMMState
(the DIMM address for example).

The latter approach most likely will be taken if we can't get rid of the 
LMB callback
and the scanning function inside the callback is too much of a burden. 
Or course that
migrating extra information is also a burden of its own and should be 
considered. Let's
see how the discussion goes.


Thanks,


Daniel

> I don't know if it is a good idea or an acceptable way to do that, but
> I'd like to know if you have thought about that.
>
> Thanks,
> Laurent