From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48940) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byroj-0002gG-3S for qemu-devel@nongnu.org; Mon, 24 Oct 2016 22:50:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byrog-0006lg-1c for qemu-devel@nongnu.org; Mon, 24 Oct 2016 22:50:21 -0400 Date: Tue, 25 Oct 2016 12:23:42 +1100 From: David Gibson Message-ID: <20161025012342.GL11052@umbus.fritz.box> References: <20161020065912.16132-1-npiggin@gmail.com> <20161020065912.16132-3-npiggin@gmail.com> <20161021004058.074a7769@roar.ozlabs.ibm.com> <20161021010954.GY11140@umbus.fritz.box> <20161021153543.294dfa9d@roar.ozlabs.ibm.com> <20161024011619.GE19629@umbus.fritz.box> <20161024175622.22076067@roar.ozlabs.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xjamM5M9kpPM/bcu" Content-Disposition: inline In-Reply-To: <20161024175622.22076067@roar.ozlabs.ibm.com> Subject: Re: [Qemu-devel] [PATCH v2] ppc: allow certain HV interrupts to be delivered to guests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nicholas Piggin Cc: =?iso-8859-1?Q?C=E9dric?= Le Goater , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Benjamin Herrenschmidt --xjamM5M9kpPM/bcu Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Oct 24, 2016 at 05:56:22PM +1100, Nicholas Piggin wrote: > On Mon, 24 Oct 2016 12:16:19 +1100 > David Gibson wrote: >=20 > > On Fri, Oct 21, 2016 at 03:35:43PM +1100, Nicholas Piggin wrote: > > > On Fri, 21 Oct 2016 12:09:54 +1100 > > > David Gibson wrote: > > > =20 > > > > On Fri, Oct 21, 2016 at 12:40:58AM +1100, Nicholas Piggin wrote: = =20 > > > > > On Thu, 20 Oct 2016 15:08:07 +0200 > > > > > C=E9dric Le Goater wrote: > > > > > =20 > > > > > > On 10/20/2016 08:59 AM, Nicholas Piggin wrote: =20 > > > > > > > Signed-off-by: Nicholas Piggin > > > > > > > --- > > > > > > > target-ppc/excp_helper.c | 8 ++++++-- > > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > >=20 > > > > > > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helpe= r.c > > > > > > > index 53c4075..477af10 100644 > > > > > > > --- a/target-ppc/excp_helper.c > > > > > > > +++ b/target-ppc/excp_helper.c > > > > > > > @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCC= PU *cpu, int excp_model, int excp) > > > > > > > /* indicate that we resumed from power save mode= */ > > > > > > > msr |=3D 0x10000; > > > > > > > new_msr |=3D ((target_ulong)1 << MSR_ME); > > > > > > > + new_msr |=3D (target_ulong)MSR_HVB; > > > > > > > + } else { > > > > > > > + /* The ISA specifies the HV bit is set when the hardwar= e interrupt > > > > > > > + * is raised, however when hypervisors deliver the exce= ption to > > > > > > > + * guests, it should not be set. > > > > > > > + */ > > > > > > > } > > > > > > > - > > > > > > > - new_msr |=3D (target_ulong)MSR_HVB; > > > > > > > ail =3D 0; > > > > > > > break; > > > > > > > case POWERPC_EXCP_DSEG: /* Data segment exception = */ > > > > > > > =20 > > > > > >=20 > > > > > > should not that be cleared later on in powerpc_excp() by : > > > > > >=20 > > > > > > env->msr =3D new_msr & env->msr_mask; > > > > > >=20 > > > > > > ? but the routine is rather long so I might be missing a branch= =2E =20 > > > > >=20 > > > > > No you're right, so it can't leak into the guest, phew! > > > > >=20 > > > > > The problem I get is the interrupt code doing some things differe= ntly > > > > > depending on on the HV bit. For example what I noticed is the gue= st > > > > > losing its LE bit upon entry. > > > > >=20 > > > > > Perhaps a cleaner way is for the system reset case to set new_msr > > > > > according to the ISA, and then apply the msr_mask (or at least ma= sk > > > > > out HV) before calculating the exception model? Any preference? = =20 > > > >=20 > > > > I think the proposed revision makes sense. > > > > =20 > > >=20 > > > What do you think of this version? This fixes up machine check guest > > > delivery as well. I'm sending this ahead of the new hcall patch, beca= use > > > it's a bugfix for existing code. I'll get around to the hcall again n= ext > > > week. > > >=20 > > > Thanks, > > > Nick > > >=20 > > >=20 > > > ppc hypervisors have delivered system reset and machine check excepti= on > > > interrupts to guests in some situations (e.g., see FWNMI feature of L= oPAPR, > > > or NMI injection in QEMU). > > >=20 > > > These exceptions are architected to set the HV bit in hardware, howev= er > > > when injected into a guest, the HV bit should be cleared. Current code > > > masks off the HV bit before setting the new MSR, however this happens= after > > > the interrupt delivery model has calculated delivery mode for the exc= eption. > > > This can result in the guest's MSR LE bit being lost. > > >=20 > > > Provide a new flag for HV exceptions to allow delivery to guests. The > > > exception model masks out the HV bit. > > >=20 > > > Also add another sanity check to ensure other such exceptions don't t= ry > > > to set HV in guest without setting guest_hv_excp > > >=20 > > > Signed-off-by: Nicholas Piggin > > > --- > > > target-ppc/excp_helper.c | 25 ++++++++++++++++++++++--- > > > 1 file changed, 22 insertions(+), 3 deletions(-) > > >=20 > > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c > > > index 53c4075..1b18433 100644 > > > --- a/target-ppc/excp_helper.c > > > +++ b/target-ppc/excp_helper.c > > > @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, in= t excp_model, int excp) > > > CPUState *cs =3D CPU(cpu); > > > CPUPPCState *env =3D &cpu->env; > > > target_ulong msr, new_msr, vector; > > > - int srr0, srr1, asrr0, asrr1, lev, ail; > > > + int srr0, srr1, asrr0, asrr1, lev, ail, guest_hv_excp; =20 > >=20 > > So, to clarify my understanding of this. > >=20 > > The guest_hv_excp flag indicates that this is a normally-HV exception > > which *could* be delivered to a guest with HV clear, *not* that we're > > actually doing so in this instance. Yes? >=20 > Correct. Ok. Hmm. Could you please do a minor respin and: - change the flag to a bool - add a comment explaining this meaning - if you can think of a name which makes the meaning more obvious (I can't, quickly), change that too --=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 --xjamM5M9kpPM/bcu Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYDrQeAAoJEGw4ysog2bOSqP0P+wUGC+AKa9YvirD6lwmIA4x4 FsL9kZRP9ip6HSa39yz9A5euYHdAcXIjzZDpPfYAfA6E7hUc6ctfRjY5cOZfUV0i hMi9I1HKFNbFVmkXkFXrWUUZYBAoPRXOHuDs+ulLhgWSjCvwLJGAF4OANxAvQqEM ARaZhFIWGHbT02H+iMvPV1REF2FMwwg312wrZAJFMx006w+1fBQVlAh6UMRd7U7g SMGw3ZB34ETmTzUEvkfahrW+2JIMK7dTLM8zzwOXyUY9f+yR6huXOK1VH6cClmlq TaYc2rX0z7ggdr5IkS0zmJ2L9GL2jJ9FsgSFcUBIjtm9H6srAAtV9Bq9F3Jsn4Co yBEiqQp2x+rmQdHn+choWKj41hk8DDWl5uj8JAlTp90dN0EVDPCTKNB0VWNopNwo PAsQ1/9gBzASX06okncykHahwNEVItivh/FXifT5nSyZK87/0Cx7PF/2KkB4v1PH e1ZC5gAx9TTXHco6FS7hN2t/X6F9Ad4xRAuBPcXdSNO9AGSLhpTweleUNBwsQ9Vo J092v1xqIwBBIoA2WfV2NsXEfCW22eaphRoFK2izZr6DdAMVjhHIGpcYYL8j5wYx B/K7kLfbn/DU8sxsGwYf8z8DdaIft9ZBF+UCMmz4WlXF+LpoA7OUEszFnrRxz2iU 3gtXDjqUG2jgmYw7CrhJ =eDz6 -----END PGP SIGNATURE----- --xjamM5M9kpPM/bcu--