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 <ag...@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 <ag...@suse.de>
> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@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 <da...@gibson.dropbear.id.au>
--
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
signature.asc
Description: PGP signature
