From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58443) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtdRQ-0006gX-El for qemu-devel@nongnu.org; Tue, 03 Nov 2015 10:24:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtdRM-0006A0-FG for qemu-devel@nongnu.org; Tue, 03 Nov 2015 10:24:08 -0500 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> From: Thomas Huth Message-ID: <5638D18E.8010809@redhat.com> Date: Tue, 3 Nov 2015 16:23:58 +0100 MIME-Version: 1.0 In-Reply-To: <1445608598-24485-3-git-send-email-mark.cave-ayland@ilande.co.uk> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: Mark Cave-Ayland , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de, david@gibson.dropbear.id.au, cormac@c-obrien.org On 23/10/15 15:56, Mark Cave-Ayland wrote: > From: Alexander Graf >=20 > The lsxw instruction checks whether the desired string actually fits > into all defined registers. Unfortunately it does the calculation wrong= , > resulting in illegal instruction traps for loads that really should fit= . 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. >=20 > Signed-off-by: Alexander Graf > Signed-off-by: Mark Cave-Ayland > --- > target-ppc/mem_helper.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) >=20 > 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 add= r, 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 "reg < 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? Thomas