From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37529) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zth9e-0001h1-5w for qemu-devel@nongnu.org; Tue, 03 Nov 2015 14:22:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zth9d-0008El-5d for qemu-devel@nongnu.org; Tue, 03 Nov 2015 14:22:02 -0500 Message-ID: <56390930.7060409@ilande.co.uk> Date: Tue, 03 Nov 2015 19:21:20 +0000 From: Mark Cave-Ayland MIME-Version: 1.0 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> In-Reply-To: <5638D18E.8010809@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de, david@gibson.dropbear.id.au, cormac@c-obrien.org On 03/11/15 15:23, Thomas Huth wrote: > 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 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. >> >> 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 addr, uint32_t reg, >> uint32_t ra, uint32_t rb) >> { >> if (likely(xer_bc != 0)) { >> - if (unlikely((ra != 0 && reg < ra && (reg + xer_bc) > ra) || >> - (reg < rb && (reg + xer_bc) > rb))) { >> + int num_used_regs = (xer_bc + 3) / 4; >> + if (unlikely((ra != 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=0, the instruction is > treated as if the instruction form were invalid. If RT=RA > or RT=RB, the instruction form is invalid. > > So I wonder whether the check for "ra != 0" is really necessary here? > Also, shouldn't the code rather check for "reg <= ra" instead of "reg < > ra"? And "reg <= 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? 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... 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 :) ATB, Mark.