| Message ID | 1445608598-24485-3-git-send-email-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
On 23/10/15 15:56, Mark Cave-Ayland wrote: > From: Alexander Graf <agraf@suse.de> > > 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 <agraf@suse.de> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > 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? Thomas
On 03/11/15 15:23, Thomas Huth wrote: > On 23/10/15 15:56, Mark Cave-Ayland wrote: >> From: Alexander Graf <agraf@suse.de> >> >> 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 <agraf@suse.de> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> 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.
On 03/11/15 20:21, Mark Cave-Ayland wrote: > On 03/11/15 15:23, Thomas Huth wrote: > >> On 23/10/15 15:56, Mark Cave-Ayland wrote: >>> From: Alexander Graf <agraf@suse.de> >>> >>> 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 <agraf@suse.de> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >>> 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 :) 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. Thomas
On 03/11/15 21:03, Thomas Huth wrote: > On 03/11/15 20:21, Mark Cave-Ayland wrote: >> On 03/11/15 15:23, Thomas Huth wrote: >> >>> On 23/10/15 15:56, Mark Cave-Ayland wrote: >>>> From: Alexander Graf <agraf@suse.de> >>>> >>>> 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 <agraf@suse.de> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> --- >>>> 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 :) > > 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. Okay, great. I've fixed the comments in my local branch and will resubmit on-list if David is happy with the changes. Many thanks, Mark.
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: > > > >> On 23/10/15 15:56, Mark Cave-Ayland wrote: > >>> From: Alexander Graf <agraf@suse.de> > >>> > >>> 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 <agraf@suse.de> > >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > >>> --- > >>> 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 :) > > 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 != 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==RA==0 and there's software that relies on it). With the trivial fix to the title, Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
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);