From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40383) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZYSQ-0005Jp-6u for qemu-devel@nongnu.org; Wed, 09 Sep 2015 02:02:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZYSM-0007Kp-6n for qemu-devel@nongnu.org; Wed, 09 Sep 2015 02:02:10 -0400 Date: Wed, 9 Sep 2015 14:13:28 +1000 From: David Gibson Message-ID: <20150909041328.GB17641@voom.redhat.com> References: <1441606024-1238-1-git-send-email-bharata@linux.vnet.ibm.com> <20150908012250.GG6537@voom.redhat.com> <20150908210356.10296.36458@loki> <20150908220325.10296.24993@loki> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="+pHx0qQiF2pBVqBT" Content-Disposition: inline In-Reply-To: <20150908220325.10296.24993@loki> 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: Michael Roth Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Bharata B Rao --+pHx0qQiF2pBVqBT Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 08, 2015 at 05:03:25PM -0500, Michael Roth wrote: > 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 i= ssued > > > > only if get-sensor-state call returns PRESENT state. However curren= tly for > > > > a logical DR, entity_sense() would unconditinally return UNUSABLE > > > > state only. This prevents any subsequent hotplug of the device with > > > > that DRC. > >=20 > > 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/unattac= hed > > 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. > >=20 > > According to PAPR+ 2.7: > >=20 > > 13.7.3.1 Acquire Logical Resource from Resource Pool: > >=20 > > If the state is =E2=80=9Cunusable=E2=80=9D the OS issues set-indicato= r (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 = resource. > >=20 > > and > >=20 > > 13.7 Logical Resource Dynamic Reconfiguration (LRDR): > >=20 > > The OS may use the get-sensor-state RTAS call with the dr-entity-sens= e token to deter- > > mine if a given drc-index refers to a connector that is currently usabl= e 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 i= n a =E2=80=9CNo such indicator implemented=E2=80=9D return sta- > > tus. =20 > >=20 > > 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. >=20 > Patch incoming: >=20 > spapr_drc: don't allow 'empty' DRCs to be unisolated >=20 > applies to spapr-next but requires revert of this patch. >=20 > Bharata, can you give it a spin with CPU hotplug and see if it fixes the > issue you hit? >=20 > >=20 > > 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. >=20 > 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. To my mind the set_indicator path is pretty much inherently specific to PAPR's weird way of doing things. For that reason I think it's just going to be simplest for it to return PAPR defined errors - i.e. RTAS errors. The whole thing is so tied to PAPR, I don't think it makes sense to try to use generic error codes for it. Trying to separate "DRC" error codes from the RTAS codes they result in sounds like a translation layer for no real gain. > Just FYI: original author names appear to have gotten lost in recent > spapr-next rebase. Ah, thanks for catching that. Should be fixed now. --=20 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 --+pHx0qQiF2pBVqBT Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJV77HoAAoJEGw4ysog2bOSvn0QALksU3z5bN7ItCdDRMxV02lz i2UIFKKPk62cOv5BsHMEJorNoe9kYm+s51+gKorVX+lNKX3rnnlIGFKsfFPqXSla VIqGZnRd4049e/AC73PundN7t6+k+EcE8Lao0GCOzppK/X40WID/pYDGbZvMvBju FGai9vA8crfpzsrFTCpxWlZvDkY1tgmF3ioba6HbDtazu6nDVWzj2sxoiK4WIFbR ZVsZ8a6ZQvaApSIQ+pTQDpKq4f77XI3/l56UXHNS7U0G2R8RKIWFRyt+1kxeFKfh Bn0iECsEWafouJ5d6v7otAb17RHH88a6GNnbxlQWsGwt5OZejcln2rXmAepXGHqR VDOwWyAkVnAqy+/NnxA+QJpTB7wNkeyFblD79w4Qk9iIZUuslpqflGq6+Qi1kNHi E6l/2f8KKbqeSnguZB2fttsJ8RioV6IdagaH2LJulGZrOY0F108b1940NY7D6Ayu 9l8cgBEXE8hKCtvrT4o/bMZ5sBpSWwW3WHB7sZ9vsv9CZo3M1LTPw3XFPc3mydhx sxXv3kdLLjs4dclTrhzRrleZ1mqbQAf/yhIYVFVYc15toUnAcFO4x3/1+5YhJKG1 ZW+DJMiuwRQPaRS1CHg9hjiMlvZ1m5jvJGK7hCAvP3abSeHRgzd8CAkHDXP9guYC QqQixQi5NPcdcqoisNXZ =+tCi -----END PGP SIGNATURE----- --+pHx0qQiF2pBVqBT--