From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43864) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZqUj-0002tS-DX for qemu-devel@nongnu.org; Wed, 09 Sep 2015 21:17:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZqUi-0007bk-5t for qemu-devel@nongnu.org; Wed, 09 Sep 2015 21:17:45 -0400 Date: Thu, 10 Sep 2015 11:18:01 +1000 From: David Gibson Message-ID: <20150910011800.GF17641@voom.redhat.com> References: <1441755895-8920-1-git-send-email-mdroth@linux.vnet.ibm.com> <20150909041041.GA17641@voom.redhat.com> <20150909171922.3885.44908@loki> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="IvGM3kKqwtniy32b" Content-Disposition: inline In-Reply-To: <20150909171922.3885.44908@loki> Subject: Re: [Qemu-devel] [PATCH v2] spapr_drc: don't allow 'empty' DRCs to be unisolated 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 --IvGM3kKqwtniy32b Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 09, 2015 at 12:19:22PM -0500, Michael Roth wrote: > Quoting David Gibson (2015-09-08 23:10:41) > > On Tue, Sep 08, 2015 at 06:44:55PM -0500, Michael Roth wrote: > > > Logical resources start with allocation-state:UNUSABLE / > > > isolation-state:ISOLATED. During hotplug, guests will transition > > > them to allocate-state:USABLE, and then to isolate-state:UNISOLATED. > > > The former transition does not seem to have any failure path for > > > cases where a DRC does not have any resources associated with it to > > > allocate for guest, but instead relies on the subsequent > > > isolation-state:UNISOLATED transition to indicate failure in this > > > situation. > > >=20 > > > Currently DRC code does not implement this logic, but instead > > > tries to indicate failure by refusing the allocation-state:USABLE > > > transition. Unfortunately, since that's not a documented failure > > > path, guests continue undeterred, causing undefined behavior in > > > QEMU and guest code. > > >=20 > > > Fix this by handling things as PAPR defines (13.7 and 13.7.3.1). > > >=20 > > > Cc: qemu-ppc@nongnu.org > > > Cc: David Gibson > > > Cc: Bharata B Rao > > > Signed-off-by: Michael Roth > > > --- > > > v2: > > > - actually include the full changeset in the patch > >=20 > > Several queries for clarification: >=20 > Looks like you've already picked this up, but just in case: >=20 > >=20 > > * Is this intended to replace Bharata's patch "spapr_drc: > > Return correct state for logical DR in entity_sense()" or to apply > > on top of it? >=20 > Yes this should replace that patch. >=20 > >=20 > > * If I'm understanding correctly, the problem here is that although > > the guest is supposed to check for failures to set the allocation > > state, it's actually not? This patch is to make qemu gracefully > > handle the guest's failure to do this? Is that right? >=20 > The root issue is that allocation state setting doesn't actually have > a documented failure path. Instead, the subsequent isolation state > setting is where we surface an error if we're unable to actually > provide the device to the guest (due to it not being attached > to the DRC in question). Um.. 13.7.3.1 (step 2) implies checking for failures on setting allocation state, and 13.5.3.5 Table 177 says "If the transition to the usable state is not possible the -3 (no such indicator implemented) status is returned." How is that not a documented failure path? > We weren't surfacing that error in the isolation state setting, so the > guest was continuing on with configuring devices that aren't actually > present. This patch should correct that. So, to be clear, I think the check in set isolation-state is a good idea as well, to properly handle a misbehaving guest. > Personally it seems like allocation state setting is where that failure > should occur, since that's the earliest point to surface the error, but > that's how PAPR has it documented. =2E. but I'm not seeing how PAPR is documenting it as happening in set isolation state rather than set allocation-state. --=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 --IvGM3kKqwtniy32b Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJV8NpIAAoJEGw4ysog2bOS1skP/it5Zc2seLXtVirWkaLY91vr bCsWFF3V+xt5cUPGG5yiGuoG1ZujqTERmox6OmCrRY2DOD8o1FlMvP46K62Szm+I WrcekaLubAcVGJk3cnPWCAAnJh7B87fUoax+DDmsNB8AfMCq894IcvcXiHu3sxiH NaTR2bAArbJQCesDwuGDiTdIYFiRtlRpQAHiglz86/cepLYW6IbWD9f0D8tlJe6p 0TjvLKGfVTwo4ykET5Jtztu/+/YewLULH1W88AV8AsYtR3If+QP43Ix+BBJfginx m4FixZ7txFcPDFo1qTSguOPq5yB5kbVnnLA4+a3I3mlEaKuIF/N28DcevfwRWfjT 93nSXMegqgvxnnDNlBDEfbg2gOu/w7BASRnBHGRvkXJ2m38IzShJL0euzn/xJc10 ZJPInMlS5GmwuAAnSA80lGl6u0k+hemtMmaLa/YOKcJbvTi1PlylTl+LjNEUExXK WlSw1Zh8tn4kL6KOFUCh0urDt+lxgkO/IeBPS8GPLMI4ZC9njWI/ZzKlrl1l4a/8 JiZQLuopKnQpu+kGj5t5xBp1YthQTUzJtgRCdT/yM6Y8mOKqe3xNJTd7z4OxfuoL ADRxA97Gmvpcwo6FnDdoL2MfOXvUcsItmtBlUqlXl7EdyVH+grWGJ+B/AC91H/Ig STu0kHV315sRRLKEgsW3 =13Mj -----END PGP SIGNATURE----- --IvGM3kKqwtniy32b--