Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] target-ppc: mcrfs should always update FEX/VX and only clear exception bits

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4000 bytes --]

On Sun, Jan 24, 2016 at 03:41:26PM +0000, James Clarke wrote:
> Signed-off-by: James Clarke <jrtc27@jrtc27.com>

So, first, for a patch making a subtle behavioural change like this a
detailed commit message is absolutely essential.  In this case I can
take the description from 0/2, but in future please include
rationale's like that in the individual patches, so they'll appear in
the git history without extra work on my part.

But.. there's a more serious bug here, so I've backed this out of
ppc-for-2.6...

[snip]
> @@ -2501,17 +2501,24 @@ static void gen_mcrfs(DisasContext *ctx)
>  {
>      TCGv tmp = tcg_temp_new();
>      int bfa;
> +    int nibble;
> +    int shift;
>  
>      if (unlikely(!ctx->fpu_enabled)) {
>          gen_exception(ctx, POWERPC_EXCP_FPU);
>          return;
>      }
> -    bfa = 4 * (7 - crfS(ctx->opcode));
> -    tcg_gen_shri_tl(tmp, cpu_fpscr, bfa);
> +    bfa = crfS(ctx->opcode);
> +    nibble = 7 - bfa;
> +    shift = 4 * nibble;
> +    tcg_gen_shri_tl(tmp, cpu_fpscr, shift);
>      tcg_gen_trunc_tl_i32(cpu_crf[crfD(ctx->opcode)], tmp);
> -    tcg_temp_free(tmp);
>      tcg_gen_andi_i32(cpu_crf[crfD(ctx->opcode)], cpu_crf[crfD(ctx->opcode)], 0xf);
> -    tcg_gen_andi_tl(cpu_fpscr, cpu_fpscr, ~(0xF << bfa));
> +    /* Only the exception bits (including FX) should be cleared if read */
> +    tcg_gen_andi_tl(tmp, cpu_fpscr, ~((0xF << shift) & FP_EX_CLEAR_BITS));
> +    /* FEX and VX need to be updated, so don't set fpscr directly */
> +    gen_helper_store_fpscr(cpu_env, tmp, 1 << nibble);

This doesn't compile.  For 64-bit targets we get:

  CC    ppc64-softmmu/target-ppc/translate.o
/home/dwg/src/qemu/target-ppc/translate.c: In function ‘gen_mcrfs’:
/home/dwg/src/qemu/target-ppc/translate.c:2520:42: error: passing argument 3 of ‘gen_helper_store_fpscr’ makes pointe
r from integer without a cast [-Werror=int-conversion]
     gen_helper_store_fpscr(cpu_env, tmp, 1 << nibble);
                                          ^
In file included from /home/dwg/src/qemu/include/exec/helper-gen.h:59:0,
                 from /home/dwg/src/qemu/tcg/tcg-op.h:27,
                 from /home/dwg/src/qemu/target-ppc/translate.c:23:
/home/dwg/src/qemu/target-ppc/helper.h:56:58: note: expected ‘TCGv_i32 {aka struct TCGv_i32_d *}’ but argument is of 
type ‘int’

For 32-bit targets it's worse:

  CC    ppcemb-softmmu/target-ppc/translate.o
/home/dwg/src/qemu/target-ppc/translate.c: In function ‘gen_mcrfs’:
/home/dwg/src/qemu/target-ppc/translate.c:2520:37: error: passing argument 2 of ‘gen_helper_store_fpscr’ from incompa
tible pointer type [-Werror=incompatible-pointer-types]
     gen_helper_store_fpscr(cpu_env, tmp, 1 << nibble);
                                     ^
In file included from /home/dwg/src/qemu/include/exec/helper-gen.h:59:0,
                 from /home/dwg/src/qemu/tcg/tcg-op.h:27,
                 from /home/dwg/src/qemu/target-ppc/translate.c:23:
/home/dwg/src/qemu/target-ppc/helper.h:56:58: note: expected ‘TCGv_i64 {aka struct TCGv_i64_d *}’ but argument is of 
type ‘TCGv_i32 {aka struct TCGv_i32_d *}’
/home/dwg/src/qemu/target-ppc/translate.c:2520:42: error: passing argument 3 of ‘gen_helper_store_fpscr’ makes pointe
r from integer without a cast [-Werror=int-conversion]
     gen_helper_store_fpscr(cpu_env, tmp, 1 << nibble);
                                          ^
In file included from /home/dwg/src/qemu/include/exec/helper-gen.h:59:0,
                 from /home/dwg/src/qemu/tcg/tcg-op.h:27,
                 from /home/dwg/src/qemu/target-ppc/translate.c:23:
/home/dwg/src/qemu/target-ppc/helper.h:56:58: note: expected ‘TCGv_i32 {aka struct TCGv_i32_d *}’ but argument is of 
type ‘int’


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]