Re: [Qemu-devel] [Qemu-ppc] [RESEND PATCH v10 1/5] hw/ppc/spapr.c: adding pending_dimm_unplugs to sPAPRMachineState
On Fri, May 19, 2017 at 08:19:41AM -0300, Daniel Henrique Barboza wrote: > > > On 05/19/2017 01:26 AM, David Gibson wrote: > > On Thu, May 18, 2017 at 06:54:12PM -0300, Daniel Henrique Barboza wrote: > > > The LMB DRC release callback, spapr_lmb_release(), uses an opaque > > > parameter, a sPAPRDIMMState struct that stores the current LMBs that > > > are allocated to a DIMM (nr_lmbs). After each call to this callback, > > > the nr_lmbs is decremented by one and, when it reaches zero, the callback > > > proceeds with the qdev calls to hot unplug the LMB. > > > > > > Using drc->detach_cb_opaque is problematic because it can't be migrated in > > > the future DRC migration work. This patch makes the following changes to > > > eliminate the usage of this opaque callback inside spapr_lmb_release: > > > > > > - sPAPRDIMMState was moved from spapr.c and added to spapr.h. A new > > > attribute called 'addr' was added to it. This is used as an unique > > > identifier to associate a sPAPRDIMMState to a PCDIMM element. > > > > > > - sPAPRMachineState now hosts a new QTAILQ called 'pending_dimm_unplugs'. > > > This queue of sPAPRDIMMState elements will store the DIMM state of DIMMs > > > that are currently going under an unplug process. > > > > > > - spapr_lmb_release() will now retrieve the nr_lmbs value by getting the > > > correspondent sPAPRDIMMState. A helper function called > > > spapr_dimm_get_address > > > was created to fetch the address of a PCDIMM device inside > > > spapr_lmb_release. > > > When nr_lmbs reaches zero and the callback proceeds with the qdev hot > > > unplug > > > calls, the sPAPRDIMMState struct is removed from > > > spapr->pending_dimm_unplugs. > > > > > > After these changes, the opaque argument for spapr_lmb_release is now > > > unused and is passed as NULL inside spapr_del_lmbs. This and the other > > > opaque arguments can now be safely removed from the code. > > > > > > Signed-off-by: Daniel Henrique Barboza> > > --- > > > hw/ppc/spapr.c | 57 > > > +- > > > include/hw/ppc/spapr.h | 4 > > > 2 files changed, 56 insertions(+), 5 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 0980d73..b05abe5 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -2050,6 +2050,7 @@ static void ppc_spapr_init(MachineState *machine) > > > msi_nonbroken = true; > > > QLIST_INIT(>phbs); > > > +QTAILQ_INIT(>pending_dimm_unplugs); > > > /* Allocate RMA if necessary */ > > > rma_alloc_size = kvmppc_alloc_rma(); > > > @@ -2603,20 +2604,63 @@ out: > > > error_propagate(errp, local_err); > > > } > > > -typedef struct sPAPRDIMMState { > > > +struct sPAPRDIMMState { > > > +uint64_t addr; > > Since you're not trying to migrate this any more, you can index the > > list by an actual PCDIMMDevice *, rather than the base address. > > You're already passing the DeviceState * for the DIMM around, so this > > will actually remove the address parameter from some functions. > Good idea. > > > > > I think that could actually be done as a preliminary cleanup. It also > > probably makes sense to merge spapr_del_lmbs() with > > spapr_memory_unplug_request(), they're both very small. > > Ok. > > > > > > > > uint32_t nr_lmbs; > > > -} sPAPRDIMMState; > > > +QTAILQ_ENTRY(sPAPRDIMMState) next; > > > +}; > > > + > > > +static sPAPRDIMMState *spapr_pending_dimm_unplugs_find(sPAPRMachineState > > > *s, > > > + uint64_t addr) > > > +{ > > > +sPAPRDIMMState *dimm_state = NULL; > > > +QTAILQ_FOREACH(dimm_state, >pending_dimm_unplugs, next) { > > > +if (dimm_state->addr == addr) { > > > +break; > > > +} > > > +} > > > +return dimm_state; > > > +} > > > + > > > +static void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr, > > > + sPAPRDIMMState *dimm_state) > > > +{ > > > +g_assert(!spapr_pending_dimm_unplugs_find(spapr, dimm_state->addr)); > > > +QTAILQ_INSERT_HEAD(>pending_dimm_unplugs, dimm_state, next); > > > +} > > > + > > > +static void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr, > > > + sPAPRDIMMState *dimm_state) > > > +{ > > > +QTAILQ_REMOVE(>pending_dimm_unplugs, dimm_state, next); > > > +g_free(dimm_state); > > > +} > > > + > > > +static uint64_t spapr_dimm_get_address(PCDIMMDevice *dimm) > > > +{ > > > +Error *local_err = NULL; > > > +uint64_t addr; > > > +addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, > > > + _err); > > > +if (local_err) { > > > +error_propagate(_abort, local_err); > > > +return 0; > > > +} > > > +return addr; > > > +} > > > static void spapr_lmb_release(DeviceState *dev, void *opaque)
Re: [Qemu-devel] [Qemu-ppc] [RESEND PATCH v10 1/5] hw/ppc/spapr.c: adding pending_dimm_unplugs to sPAPRMachineState
On 05/19/2017 01:26 AM, David Gibson wrote: On Thu, May 18, 2017 at 06:54:12PM -0300, Daniel Henrique Barboza wrote: The LMB DRC release callback, spapr_lmb_release(), uses an opaque parameter, a sPAPRDIMMState struct that stores the current LMBs that are allocated to a DIMM (nr_lmbs). After each call to this callback, the nr_lmbs is decremented by one and, when it reaches zero, the callback proceeds with the qdev calls to hot unplug the LMB. Using drc->detach_cb_opaque is problematic because it can't be migrated in the future DRC migration work. This patch makes the following changes to eliminate the usage of this opaque callback inside spapr_lmb_release: - sPAPRDIMMState was moved from spapr.c and added to spapr.h. A new attribute called 'addr' was added to it. This is used as an unique identifier to associate a sPAPRDIMMState to a PCDIMM element. - sPAPRMachineState now hosts a new QTAILQ called 'pending_dimm_unplugs'. This queue of sPAPRDIMMState elements will store the DIMM state of DIMMs that are currently going under an unplug process. - spapr_lmb_release() will now retrieve the nr_lmbs value by getting the correspondent sPAPRDIMMState. A helper function called spapr_dimm_get_address was created to fetch the address of a PCDIMM device inside spapr_lmb_release. When nr_lmbs reaches zero and the callback proceeds with the qdev hot unplug calls, the sPAPRDIMMState struct is removed from spapr->pending_dimm_unplugs. After these changes, the opaque argument for spapr_lmb_release is now unused and is passed as NULL inside spapr_del_lmbs. This and the other opaque arguments can now be safely removed from the code. Signed-off-by: Daniel Henrique Barboza--- hw/ppc/spapr.c | 57 +- include/hw/ppc/spapr.h | 4 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 0980d73..b05abe5 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2050,6 +2050,7 @@ static void ppc_spapr_init(MachineState *machine) msi_nonbroken = true; QLIST_INIT(>phbs); +QTAILQ_INIT(>pending_dimm_unplugs); /* Allocate RMA if necessary */ rma_alloc_size = kvmppc_alloc_rma(); @@ -2603,20 +2604,63 @@ out: error_propagate(errp, local_err); } -typedef struct sPAPRDIMMState { +struct sPAPRDIMMState { +uint64_t addr; Since you're not trying to migrate this any more, you can index the list by an actual PCDIMMDevice *, rather than the base address. You're already passing the DeviceState * for the DIMM around, so this will actually remove the address parameter from some functions. Good idea. I think that could actually be done as a preliminary cleanup. It also probably makes sense to merge spapr_del_lmbs() with spapr_memory_unplug_request(), they're both very small. Ok. uint32_t nr_lmbs; -} sPAPRDIMMState; +QTAILQ_ENTRY(sPAPRDIMMState) next; +}; + +static sPAPRDIMMState *spapr_pending_dimm_unplugs_find(sPAPRMachineState *s, + uint64_t addr) +{ +sPAPRDIMMState *dimm_state = NULL; +QTAILQ_FOREACH(dimm_state, >pending_dimm_unplugs, next) { +if (dimm_state->addr == addr) { +break; +} +} +return dimm_state; +} + +static void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr, + sPAPRDIMMState *dimm_state) +{ +g_assert(!spapr_pending_dimm_unplugs_find(spapr, dimm_state->addr)); +QTAILQ_INSERT_HEAD(>pending_dimm_unplugs, dimm_state, next); +} + +static void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr, + sPAPRDIMMState *dimm_state) +{ +QTAILQ_REMOVE(>pending_dimm_unplugs, dimm_state, next); +g_free(dimm_state); +} + +static uint64_t spapr_dimm_get_address(PCDIMMDevice *dimm) +{ +Error *local_err = NULL; +uint64_t addr; +addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, + _err); +if (local_err) { +error_propagate(_abort, local_err); +return 0; +} +return addr; +} static void spapr_lmb_release(DeviceState *dev, void *opaque) { -sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque; HotplugHandler *hotplug_ctrl; +uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev)); +sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); I prefer not to access the machine as a global when possible. I think it's preferable to pass down the spapr object from above - unplug_request() itself can get it from hotplug_dev. I see that we have access to the hotplug_dev (HotplugHandler) in the end of spapr_lmb_release: hotplug_ctrl = qdev_get_hotplug_handler(dev); One alternative would be to move this call up in the function and then retrieve the machine as unplug_request() does: hotplug_ctrl =