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

[Thomas Huth <thuth@redhat.com>]

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