From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57226) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtozH-0004Nh-OU for qemu-devel@nongnu.org; Tue, 03 Nov 2015 22:43:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtozD-0002Ol-Si for qemu-devel@nongnu.org; Tue, 03 Nov 2015 22:43:51 -0500 Date: Wed, 4 Nov 2015 14:01:21 +1100 From: David Gibson Message-ID: <20151104030121.GC21954@voom.redhat.com> References: <1445608598-24485-1-git-send-email-mark.cave-ayland@ilande.co.uk> <1445608598-24485-3-git-send-email-mark.cave-ayland@ilande.co.uk> <5638D18E.8010809@redhat.com> <56390930.7060409@ilande.co.uk> <56392119.50200@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="OBd5C1Lgu00Gd/Tn" Content-Disposition: inline In-Reply-To: <56392119.50200@redhat.com> Subject: Re: [Qemu-devel] [PATCH 02/13] PPC: Fix lsxw bounds checks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: qemu-ppc@nongnu.org, cormac@c-obrien.org, Mark Cave-Ayland , qemu-devel@nongnu.org, agraf@suse.de --OBd5C1Lgu00Gd/Tn Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 03, 2015 at 10:03:21PM +0100, Thomas Huth wrote: > On 03/11/15 20:21, Mark Cave-Ayland wrote: > > On 03/11/15 15:23, Thomas Huth wrote: > >=20 > >> On 23/10/15 15:56, Mark Cave-Ayland wrote: > >>> From: Alexander Graf > >>> > >>> The lsxw instruction checks whether the desired string actually fits > >>> into all defined registers. Unfortunately it does the calculation wro= ng, > >>> resulting in illegal instruction traps for loads that really should f= it. > >> > >> s/lsxw/lswx/ in the above text and in the title ... but I guess this > >> could also be done when this patch gets picked up. > >> > >>> Fix it up, making Mac OS happier. > >>> > >>> Signed-off-by: Alexander Graf > >>> Signed-off-by: Mark Cave-Ayland > >>> --- > >>> target-ppc/mem_helper.c | 5 +++-- > >>> 1 file changed, 3 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c > >>> index 6d37dae..7e1f234 100644 > >>> --- a/target-ppc/mem_helper.c > >>> +++ b/target-ppc/mem_helper.c > >>> @@ -100,8 +100,9 @@ void helper_lswx(CPUPPCState *env, target_ulong a= ddr, uint32_t reg, > >>> uint32_t ra, uint32_t rb) > >>> { > >>> if (likely(xer_bc !=3D 0)) { > >>> - if (unlikely((ra !=3D 0 && reg < ra && (reg + xer_bc) > ra) = || > >>> - (reg < rb && (reg + xer_bc) > rb))) { > >>> + int num_used_regs =3D (xer_bc + 3) / 4; > >>> + if (unlikely((ra !=3D 0 && reg < ra && (reg + num_used_regs)= > ra) || > >>> + (reg < rb && (reg + num_used_regs) > rb))) { > >>> helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM, > >>> POWERPC_EXCP_INVAL | > >>> POWERPC_EXCP_INVAL_LSWX); > >> > >> The calculation of num_used_regs looks fine to me ... but is the > >> remaining part of the condition really right already? > >> > >> According to the PowerISA: > >> > >> If RA or RB is in the range of registers to be loaded, > >> including the case in which RA=3D0, the instruction is > >> treated as if the instruction form were invalid. If RT=3DRA > >> or RT=3DRB, the instruction form is invalid. > >> > >> So I wonder whether the check for "ra !=3D 0" is really necessary here? > >> Also, shouldn't the code rather check for "reg <=3D ra" instead of "re= g < > >> ra"? And "reg <=3D rb", too, of course? > >> > >> Also this code seems to completely ignore the case of the register > >> wrap-around, where the sequence of registers jumps back to r0 ... > >> > >> So I'm basically fine with the num_used_regs fix for now, but I think > >> this needs a big "FIXME" comment so that the remaining issues get > >> cleaned up later? > >=20 > > This was one of Alex's patches that was originally queued for ppc-next > > before being dropped for missing the SoB, so I was expecting review to > > find issues with other patches in the set rather than this one... > >=20 > > Having said that, I'm not sure whether this was deliberate for > > compatibility reasons or just an oversight. Unless David has any ideas > > it might be that we have to wait for Alex to return before this series > > can be included, but thanks for the review anyhow :) >=20 > Well, as I said, I'm basically fine with the patch, since it fixes one > bug and the fix itself also looks fine. It's just that the surrounding > code looks like it contains some more bugs... but these could also be > fixed with a later patch, I guess. Right. It does look like the ra !=3D 0 check is unnnecessary, according to the ISA. I think it's clearer to change that in a separate patch, though (and it will making things easier to deal with if we discover some implementations *do* allow RT=3D=3DRA=3D=3D0 and there's software that relies on it). With the trivial fix to the title, Reviewed-by: David Gibson --=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 --OBd5C1Lgu00Gd/Tn Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIbBAEBAgAGBQJWOXUBAAoJEGw4ysog2bOSopIP9RmGUIMJUxwr2NinjB4CZgtj N0g6g2eihpBteXEHsxvDLIEW9V5Mi9YkIxzJkOG6e2qqA93yKQtLre1peZaOPE28 S/R5PwbOuhqdK2ckbU9t77HS4zlvtYOqzvjlKdiIZ5BTp+IT1FLQjV4s0H7eJfdO hwrdPLw5qzD76WeKahyTZdWcwFYjz6oRfAbZ2SHfggFDIczt+Kuo6yaBuc+HTQhJ 220ChRX/skdX8JcZlQPlr1dSZwxXQyghaWzf5ubN/bgT+PtUH0ssZn4XMm616PkF Gq3h1rDp+T1hMjrv8bBdwRrdHlcFeHGeOO3y+8ENVm+gmK5kwnX/0Moa/hJqaRuf x/CaEHt3odgIfjB9Wb1w5EIXX+7U7W9RDrPtsbs73aTCVv/5j+1DBagX6UaPVjMa S0kJqspujug+PwPzdDjGU7UCI+cK2tNpjVRIBpJz9EmJQa6o2BoTqaVCTwEVg2XS UbtlvC+vgXQuKTIHmYCZxwlSl9R8qCZJqgMtUyNRxo9hC+/DrbuHAt0+Qd4HMhn8 BW8nW8lS8lbRD44aslQiH0k5VPEO3oM/csmeaqDQoJn1hxhmCi1qIkx8jWCvpIv4 ENnOx6rdeqx2S9noFkvUrjMsRn6fPIZdAUqBYc8phc4QMbwCsDGpCTRVWnDom+M2 3dCqBw5ZSqtbtlzPh0g= =Z6j5 -----END PGP SIGNATURE----- --OBd5C1Lgu00Gd/Tn--