Re: [Qemu-devel] [PATCH 02/13] PPC: Fix lsxw bounds checks

[David Gibson]

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 <address@hidden>
> >>>
> >>> 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 <address@hidden>
> >>> Signed-off-by: Mark Cave-Ayland <address@hidden>
> >>> ---
> >>>  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 <address@hidden>

-- 
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