From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54865) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXtVh-0002DW-6J for qemu-devel@nongnu.org; Fri, 04 Sep 2015 12:06:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZXtVc-0003x9-OC for qemu-devel@nongnu.org; Fri, 04 Sep 2015 12:06:41 -0400 Received: from e38.co.us.ibm.com ([32.97.110.159]:59068) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXtVc-0003wS-Ei for qemu-devel@nongnu.org; Fri, 04 Sep 2015 12:06:36 -0400 Received: from /spool/local by e38.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 4 Sep 2015 10:06:25 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20150826095750.GB11136@in.ibm.com> References: <1439967371-15870-1-git-send-email-bharata@linux.vnet.ibm.com> <1439967371-15870-4-git-send-email-bharata@linux.vnet.ibm.com> <20150825023931.11069.31241@loki> <20150826095750.GB11136@in.ibm.com> Message-ID: <20150904160600.10296.13169@loki> Date: Fri, 04 Sep 2015 11:06:00 -0500 Subject: Re: [Qemu-devel] [RFC PATCH v0 3/3] spapr: Memory hot-unplug support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: bharata@linux.vnet.ibm.comBharata B Rao Cc: qemu-devel@nongnu.org, agraf@suse.de, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, imammedo@redhat.com, david@gibson.dropbear.id.au Quoting Bharata B Rao (2015-08-26 04:57:51) > On Mon, Aug 24, 2015 at 09:39:31PM -0500, Michael Roth wrote: > > Quoting Bharata B Rao (2015-08-19 01:56:11) > > > Add support to hot remove pc-dimm memory devices. > > > = > > > Signed-off-by: Bharata B Rao > > > --- > > > hw/ppc/spapr.c | 114 +++++++++++++++++++++++++++++++++++++++= +++++++++- > > > hw/ppc/spapr_drc.c | 21 +++++++++ > > > include/hw/ppc/spapr.h | 2 + > > > 3 files changed, 136 insertions(+), 1 deletion(-) > > > = > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 06d000d..441012d 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -2110,6 +2110,109 @@ out: > > > error_propagate(errp, local_err); > > > } > > > = > > > +typedef struct sPAPRDIMMState { > > > + uint32_t nr_lmbs; > > > +} sPAPRDIMMState; > > > + > > > +/* > > > + * Called from spapr_drc.c: set_isolation_state(). > > > + * > > > + * If the drc is being marked as ISOLATED, ensure that the correspon= ding > > > + * LMB is part of the DIMM device which is being deleted. > > > + */ > > > +int spapr_lmb_in_removable_dimm(sPAPRDRConnector *drc, > > > + sPAPRDRIsolationState state) > > > +{ > > > + DeviceState *dev =3D drc->dev; > > > + PCDIMMDevice *dimm =3D PC_DIMM(dev); > > > + > > > + if (state !=3D SPAPR_DR_ISOLATION_STATE_ISOLATED) { > > > + return 0; > > > + } > > > + > > > + if (!dimm->delete_pending) { > > > + return -1; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static void spapr_lmb_release(DeviceState *dev, void *opaque) > > > +{ > > > + sPAPRDIMMState *ds =3D (sPAPRDIMMState *)opaque; > > > + HotplugHandler *hotplug_ctrl =3D NULL; > > > + Error *local_err =3D NULL; > > > + > > > + if (--ds->nr_lmbs) { > > > + return; > > > + } > > > + > > > + g_free(ds); > > > + > > > + /* > > > + * Now that all the LMBs have been removed by the guest, call the > > > + * pc-dimm unplug handler to cleanup up the pc-dimm device. > > > + */ > > > + hotplug_ctrl =3D qdev_get_hotplug_handler(dev); > > > + hotplug_handler_unplug(hotplug_ctrl, dev, &local_err); > > > +} > > > + > > > +static void spapr_del_lmbs(DeviceState *dev, uint64_t addr, uint64_t= size, > > > + Error **errp) > > > +{ > > > + sPAPRDRConnector *drc; > > > + sPAPRDRConnectorClass *drck; > > > + uint32_t nr_lmbs =3D size/SPAPR_MEMORY_BLOCK_SIZE; > > > + Error *local_err =3D NULL; > > > + int i; > > > + sPAPRDIMMState *ds =3D g_malloc0(sizeof(sPAPRDIMMState)); > > > + > > > + ds->nr_lmbs =3D nr_lmbs; > > > + for (i =3D 0; i < nr_lmbs; i++) { > > > + drc =3D spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, > > > + addr/SPAPR_MEMORY_BLOCK_SIZE); > > > + g_assert(drc); > > > + > > > + drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > > + drck->detach(drc, dev, spapr_lmb_release, ds, &local_err); > > > + addr +=3D SPAPR_MEMORY_BLOCK_SIZE; > > > + } > > > + spapr_hotplug_req_remove_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, n= r_lmbs); > > > +} > > > + > > > +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceS= tate *dev, > > > + Error **errp) > > > +{ > > > + sPAPRMachineState *ms =3D SPAPR_MACHINE(hotplug_dev); > > > + PCDIMMDevice *dimm =3D PC_DIMM(dev); > > > + PCDIMMDeviceClass *ddc =3D PC_DIMM_GET_CLASS(dimm); > > > + MemoryRegion *mr =3D ddc->get_memory_region(dimm); > > > + > > > + pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr); > > > + object_unparent(OBJECT(dev)); > > > +} > > = > > In the current code the unplug() and request_unplug() are mutually > > exclusive. Are the plans on making the unplug() do something in the > > prescence of request_unplug()? If so, I'd imagine it would've be a > > forced removal, except maybe as a fallback if the request is determined > > to fail somehow? > = > Like x86 memory hotremoval, our model too fits into async type of removal > where we first send removal notification to guest in ->unplug_request() a= nd > when the guest indeed removes the memory, we cleanup the pc-dimm device > in ->unplug(). > = > Since we implement both ->unplug() and ->unplug_request(), and given that > the removal works like above, I don't see why we would ever end up doing a > forced removal from ->unplug(). Ah, sorry, misunderstanding on my part: as far as *qdev* lifecycle is concerned, unplug() never gets called directly if unplug_request() is defined, so it seemed the 2 were mutually exclusive. But in the case of memory x86 unplug, unplug_request()'s implementation is such that the unplug() handler gets called explicitly via guest acknowledgement's callback, which is the same as what you've modelled here. It's a little wierd that the unplug() callback is needlessly exposed outside of the async unplug_request() implementation, but it does allow for future code where maybe a --force option could be introduced for certain situations where a guest isn't cooperative. > = > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > > > index 8cbcf4d..b9d7c71 100644 > > > --- a/hw/ppc/spapr_drc.c > > > +++ b/hw/ppc/spapr_drc.c > > > @@ -11,6 +11,7 @@ > > > */ > > > = > > > #include "hw/ppc/spapr_drc.h" > > > +#include "hw/ppc/spapr.h" > > > #include "qom/object.h" > > > #include "hw/qdev.h" > > > #include "qapi/visitor.h" > > > @@ -63,9 +64,29 @@ static int set_isolation_state(sPAPRDRConnector *d= rc, > > > sPAPRDRIsolationState state) > > > { > > > sPAPRDRConnectorClass *drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc= ); > > > + int ret; > > > = > > > DPRINTFN("drc: %x, set_isolation_state: %x", get_index(drc), sta= te); > > > = > > > + /* > > > + * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't > > > + * belong to a DIMM device that is marked for removal. > > > + * > > > + * Currently the guest userspace tool drmgr that drives the memo= ry > > > + * hotplug/unplug will just try to remove a set of 'removable' L= MBs > > > + * in response to a hot unplug request that is based on drc-coun= t. > > > + * If the LMB being removed doesn't belong to a DIMM device that= is > > > + * actually being unplugged, fail the isolation request here. > > > + * > > > + * TODO: Calling out from spapr_drc.c like this doesn't look goo= d. > > > + */ > > > + if (drc->type =3D=3D SPAPR_DR_CONNECTOR_TYPE_LMB) { > > > + ret =3D spapr_lmb_in_removable_dimm(drc, state); > > > + if (ret) { > > > + return RTAS_OUT_HW_ERROR; > > > + } > > > + } > = > I am not sure if this call out is the right way to do this. Do you have > suggestions here ? Since drc->awaiting_release is only set in cases where the corresponding dimm is pending delete, I think maybe we can rely on drc->awaiting_release instead and avoid the call out. > = > Regards, > Bharata. >=20