From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZQzQ-0001U2-JD for qemu-devel@nongnu.org; Tue, 08 Sep 2015 18:03:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZQzM-00079s-Ed for qemu-devel@nongnu.org; Tue, 08 Sep 2015 18:03:44 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:60691) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZQzM-00079G-9M for qemu-devel@nongnu.org; Tue, 08 Sep 2015 18:03:40 -0400 Received: from /spool/local by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 8 Sep 2015 16:03:39 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20150908210356.10296.36458@loki> References: <1441606024-1238-1-git-send-email-bharata@linux.vnet.ibm.com> <20150908012250.GG6537@voom.redhat.com> <20150908210356.10296.36458@loki> Message-ID: <20150908220325.10296.24993@loki> Date: Tue, 08 Sep 2015 17:03:25 -0500 Subject: Re: [Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson , Bharata B Rao Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org Quoting Michael Roth (2015-09-08 16:03:56) > Quoting David Gibson (2015-09-07 20:22:50) > > On Mon, Sep 07, 2015 at 11:37:04AM +0530, Bharata B Rao wrote: > > > When drmgr is run in the guest to add a device for which device_add > > > hasn't been issued in QEMU, configure-connector call fails. > > > When configure-connector call fails, the guest would release (*) > > > the previously acquired DRC by setting back the DRC isolation state > > > to ISOLATED and allocation state to UNUSABLE. These calls will be iss= ued > > > only if get-sensor-state call returns PRESENT state. However currentl= y for > > > a logical DR, entity_sense() would unconditinally return UNUSABLE > > > state only. This prevents any subsequent hotplug of the device with > > > that DRC. > = > This seems a little odd. I think we default to ALLOCATION_STATE_UNUSABLE > for logical DR, and it's up the guest to transition to USABLE, which > probably happens prior to the configure-connector calls. So I think the > net effect of this fix is that guest will see these unallocated/unattached > resources the same way they would a resource that was actually attached > via device_add, and all we're really doing is working around the > eventual configuration failure that that will lead to by pretending a > resource was actually there. > = > According to PAPR+ 2.7: > = > 13.7.3.1 Acquire Logical Resource from Resource Pool: > = > If the state is =E2=80=9Cunusable=E2=80=9D the OS issues set-indicator = (allocation-state, usable) to attempt to allocate the re- > source. Similarly, if the state is =E2=80=9Cavailable for exchange=E2= =80=9D the OS issues set-indicator (allocation-state, ex- > change) to attempt to allocate the resource, and if the state is =E2=80= =9Cavailable for recovery=E2=80=9D the OS issues > set-indicator (allocation-state, recover) to attempt to allocate the re= source. > = > and > = > 13.7 Logical Resource Dynamic Reconfiguration (LRDR): > = > The OS may use the get-sensor-state RTAS call with the dr-entity-sense = token to deter- > mine if a given drc-index refers to a connector that is currently usable = for DR operations. If the connector is not > currently usable the return state is =E2=80=9CDR entity unusable=E2=80=9D= (2). A set-indicator (isolation state) RTAS call to an unusable > connector or (dr-indicator) to any logical resource connector results in = a =E2=80=9CNo such indicator implemented=E2=80=9D return sta- > tus. = > = > So I think maybe the proper fix is to make sure that > drc->set_indicator_state() fails with an error that indicates to RTAS to > return NO_SENSOR (-3) for cases where we haven't attached a resource > to the DRC via device_add. Patch incoming: spapr_drc: don't allow 'empty' DRCs to be unisolated applies to spapr-next but requires revert of this patch. Bharata, can you give it a spin with CPU hotplug and see if it fixes the issue you hit? > = > Which also kind of re-opens the discussion of whether or not > drc->set_indicator_state() should return RTAS errors directly. I'd > still stray away from that for now but maybe if we get more cases > like this it'll start becoming more practical. I'm starting to second-guess myself on this. I'm trying to maintain separation between RTAS/DRC but result is a bit pathological. Feel free to comment in the above patch. Just FYI: original author names appear to have gotten lost in recent spapr-next rebase. > = > > > = > > > Fix this by returning the right state in entity_sense() by checking > > > the allocation_state of DRC. > > > = > > > (*) https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-September/13= 3430.html > > > = > > > Signed-off-by: Bharata B Rao > > > Cc: Michael Roth > > = > > Reviewed-by: David Gibson > > = > > and applied to my tree. > > = > > > --- > > > hw/ppc/spapr_drc.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > = > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > > > index 9ce844a..2586065 100644 > > > --- a/hw/ppc/spapr_drc.c > > > +++ b/hw/ppc/spapr_drc.c > > > @@ -186,7 +186,11 @@ static sPAPRDREntitySense entity_sense(sPAPRDRCo= nnector *drc) > > > */ > > > state =3D SPAPR_DR_ENTITY_SENSE_EMPTY; > > > } else { > > > - state =3D SPAPR_DR_ENTITY_SENSE_UNUSABLE; > > > + if (drc->allocation_state =3D=3D SPAPR_DR_ALLOCATION_STA= TE_UNUSABLE) { > > > + state =3D SPAPR_DR_ENTITY_SENSE_UNUSABLE; > > > + } else { > > > + state =3D SPAPR_DR_ENTITY_SENSE_PRESENT; > > > + } > > > } > > > } > > > = > > = > > -- = > > David Gibson | I'll have my music baroque, and my co= de > > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _ot= her_ > > | _way_ _around_! > > http://www.ozlabs.org/~dgibson > = >=20