From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40382) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZYSQ-0005Jo-6W for qemu-devel@nongnu.org; Wed, 09 Sep 2015 02:02:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZYSM-0007Ku-6s for qemu-devel@nongnu.org; Wed, 09 Sep 2015 02:02:10 -0400 Date: Wed, 9 Sep 2015 14:10:41 +1000 From: David Gibson Message-ID: <20150909041041.GA17641@voom.redhat.com> References: <1441755895-8920-1-git-send-email-mdroth@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="IJpNTDwzlM2Ie8A6" Content-Disposition: inline In-Reply-To: <1441755895-8920-1-git-send-email-mdroth@linux.vnet.ibm.com> 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 --IJpNTDwzlM2Ie8A6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 Several queries for clarification: * 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? * 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 > --- > hw/ppc/spapr_drc.c | 12 ++++++++++++ > hw/ppc/spapr_rtas.c | 9 +++++++-- > include/hw/ppc/spapr.h | 1 + > include/hw/ppc/spapr_drc.h | 2 ++ > 4 files changed, 22 insertions(+), 2 deletions(-) >=20 > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index 9ce844a..c1f664f 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -66,6 +66,18 @@ static int set_isolation_state(sPAPRDRConnector *drc, > =20 > DPRINTFN("drc: %x, set_isolation_state: %x", get_index(drc), state); > =20 > + if (state =3D=3D SPAPR_DR_ISOLATION_STATE_UNISOLATED) { > + /* cannot unisolate a non-existant resource. this generally > + * happens for logical resources where transitions from > + * allocation-state:UNUSABLE to allocation-state:USABLE are > + * unguarded, but instead rely on a subsequent > + * isolation-state:UNISOLATED transition to indicate failure > + */ > + if (!drc->dev) { > + return -1; > + } > + } > + > drc->isolation_state =3D state; > =20 > if (drc->isolation_state =3D=3D SPAPR_DR_ISOLATION_STATE_ISOLATED) { > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 3b7b20b..0ddedca 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -372,6 +372,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPR= MachineState *spapr, > uint32_t sensor_type; > uint32_t sensor_index; > uint32_t sensor_state; > + int drc_ret, ret =3D RTAS_OUT_SUCCESS; > sPAPRDRConnector *drc; > sPAPRDRConnectorClass *drck; > =20 > @@ -413,7 +414,11 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAP= RMachineState *spapr, > spapr_ccs_remove(spapr, ccs); > } > } > - drck->set_isolation_state(drc, sensor_state); > + drc_ret =3D drck->set_isolation_state(drc, sensor_state); > + if (drc_ret !=3D 0) { > + ret =3D (drc_ret =3D=3D -1) ? RTAS_OUT_NO_SUCH_INDICATOR > + : RTAS_OUT_HW_ERROR; > + } > break; > case RTAS_SENSOR_TYPE_DR: > drck->set_indicator_state(drc, sensor_state); > @@ -425,7 +430,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPR= MachineState *spapr, > goto out_unimplemented; > } > =20 > - rtas_st(rets, 0, RTAS_OUT_SUCCESS); > + rtas_st(rets, 0, ret); > return; > =20 > out_unimplemented: > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index c75cc5e..ffb108d 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -412,6 +412,7 @@ int spapr_allocate_irq_block(int num, bool lsi, bool = msi); > #define RTAS_OUT_BUSY -2 > #define RTAS_OUT_PARAM_ERROR -3 > #define RTAS_OUT_NOT_SUPPORTED -3 > +#define RTAS_OUT_NO_SUCH_INDICATOR -3 > #define RTAS_OUT_NOT_AUTHORIZED -9002 > =20 > /* RTAS tokens */ > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h > index 28ffeae..b2c1209 100644 > --- a/include/hw/ppc/spapr_drc.h > +++ b/include/hw/ppc/spapr_drc.h > @@ -165,6 +165,8 @@ typedef struct sPAPRDRConnectorClass { > /*< public >*/ > =20 > /* accessors for guest-visible (generally via RTAS) DR state */ > + > + /* returns -1 if DRC cannot be set to requested isolation state */ > int (*set_isolation_state)(sPAPRDRConnector *drc, > sPAPRDRIsolationState state); > int (*set_indicator_state)(sPAPRDRConnector *drc, --=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 --IJpNTDwzlM2Ie8A6 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJV77FAAAoJEGw4ysog2bOSlBIQAKnu2hYCCq7Xdz8WAoFOFOZ8 OOYMr03U0HadiR/t9eeNzx7/IDk3umOSZkoSlnh8hJMG0Cbbp/GzY7EeebMbq19u rrAaIILAoBzrklk7KdXhRmmaWKAJHd7GsWKJkiHXBvgSFlN9XaS6Wj1q2flpzPIw U/wgFyxdHnqiH3bWj6aQkzbIXHQIb6vba+dF989u9oCH/eXlncvgvfesqIFWpOi9 DMtkbX61O+nVbI4pWT7O/i5kTYEM0cE76rg3efpwNrnmxE7lqB39fyMRxa7D5aMU CfIkv8u41KXBRcxQDLyd2kDriObCzg2SePzDHQ6mxx399+EjSEonzGGENN6jZ2Rd QyBq/7Za4bd/wrGKBmqAzOaRqVHixkf+M9K9+D0WQD9yHcPhKEftt8osMJ3USwU7 UCvYQ5igJQRXfUsB8lWFunw11BkPxEw0/6p+93dxqPddiO/xgEe8C31hr+RZFVTI +izOLbQvH/JgbF6sgJXoKIYYXlxrkMqAtucJu/NmKc2b+M/OpFzCUxoadK/5ycuX tIwl6kYCf38zHAC1giFpsE50F+aSiGBBtytL+9mP5dFEwQ044+mCJkpOPTzMVkCW IpC+FfbhhEE7ArRt7ClYU/K5pMKg4HGAm3FHiwz6SiJ7BosoMn5u41VcP9eUHugq 9SKYSCBD3V0ZgYC+5e3f =/Cy6 -----END PGP SIGNATURE----- --IJpNTDwzlM2Ie8A6--