[Qemu-devel] [PATCH 1/9] target/ppc: Fix xvxsigdp

[Qemu-devel] [PATCH 1/9] target/ppc: Fix xvxsigdp

[Anton Blanchard]

Fix a typo in xvxsigdp where we put both results into the lower
doubleword.

Fixes: dd977e4f45cb ("target/ppc: Optimize x[sv]xsigdp using deposit_i64()")
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/translate/vsx-impl.inc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index 11d9b75d01..4d8ca7cf32 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -1820,7 +1820,7 @@ static void gen_xvxsigdp(DisasContext *ctx)
     tcg_gen_movi_i64(t0, 0x0010000000000000);
     tcg_gen_movcond_i64(TCG_COND_EQ, t0, exp, zr, zr, t0);
     tcg_gen_movcond_i64(TCG_COND_EQ, t0, exp, nan, zr, t0);
-    tcg_gen_deposit_i64(xth, t0, xbl, 0, 52);
+    tcg_gen_deposit_i64(xtl, t0, xbl, 0, 52);
     set_cpu_vsrl(xT(ctx->opcode), xtl);
 
     tcg_temp_free_i64(t0);
-- 
2.20.1

[Qemu-devel] [PATCH 2/9] target/ppc: Fix xxspltib

[Anton Blanchard]

xxspltib raises a VMX or a VSX exception depending on the register
set it is operating on. We had a check, but it was backwards.

Fixes: f113283525a4 ("target-ppc: add xxspltib instruction")
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/translate/vsx-impl.inc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index 4d8ca7cf32..d050cc03ed 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -1355,16 +1355,17 @@ static void gen_xxspltib(DisasContext *ctx)
     int rt = xT(ctx->opcode);
 
     if (rt < 32) {
-        if (unlikely(!ctx->altivec_enabled)) {
-            gen_exception(ctx, POWERPC_EXCP_VPU);
+        if (unlikely(!ctx->vsx_enabled)) {
+            gen_exception(ctx, POWERPC_EXCP_VSXU);
             return;
         }
     } else {
-        if (unlikely(!ctx->vsx_enabled)) {
-            gen_exception(ctx, POWERPC_EXCP_VSXU);
+        if (unlikely(!ctx->altivec_enabled)) {
+            gen_exception(ctx, POWERPC_EXCP_VPU);
             return;
         }
     }
+    printf("XT %x IMM8 %x\n", rt, uim8);
     tcg_gen_gvec_dup8i(vsr_full_offset(rt), 16, 16, uim8);
 }
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/9] target/ppc: Fix xxspltib

[David Gibson]

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

On Tue, May 07, 2019 at 10:48:04AM +1000, Anton Blanchard wrote:
> xxspltib raises a VMX or a VSX exception depending on the register
> set it is operating on. We had a check, but it was backwards.
> 
> Fixes: f113283525a4 ("target-ppc: add xxspltib instruction")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> ---
>  target/ppc/translate/vsx-impl.inc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index 4d8ca7cf32..d050cc03ed 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -1355,16 +1355,17 @@ static void gen_xxspltib(DisasContext *ctx)
>      int rt = xT(ctx->opcode);
>  
>      if (rt < 32) {
> -        if (unlikely(!ctx->altivec_enabled)) {
> -            gen_exception(ctx, POWERPC_EXCP_VPU);
> +        if (unlikely(!ctx->vsx_enabled)) {
> +            gen_exception(ctx, POWERPC_EXCP_VSXU);
>              return;
>          }
>      } else {
> -        if (unlikely(!ctx->vsx_enabled)) {
> -            gen_exception(ctx, POWERPC_EXCP_VSXU);
> +        if (unlikely(!ctx->altivec_enabled)) {
> +            gen_exception(ctx, POWERPC_EXCP_VPU);
>              return;
>          }
>      }
> +    printf("XT %x IMM8 %x\n", rt, uim8);

Looks like you have some leftover debugging here.

>      tcg_gen_gvec_dup8i(vsr_full_offset(rt), 16, 16, uim8);
>  }
>  

-- 
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: 833 bytes --]

[Qemu-devel] [PATCH v2] target/ppc: Fix xxspltib

[Anton Blanchard]

xxspltib raises a VMX or a VSX exception depending on the register
set it is operating on. We had a check, but it was backwards.

Fixes: f113283525a4 ("target-ppc: add xxspltib instruction")
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/translate/vsx-impl.inc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index 4d8ca7cf32..4812a374aa 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -1355,13 +1355,13 @@ static void gen_xxspltib(DisasContext *ctx)
     int rt = xT(ctx->opcode);
 
     if (rt < 32) {
-        if (unlikely(!ctx->altivec_enabled)) {
-            gen_exception(ctx, POWERPC_EXCP_VPU);
+        if (unlikely(!ctx->vsx_enabled)) {
+            gen_exception(ctx, POWERPC_EXCP_VSXU);
             return;
         }
     } else {
-        if (unlikely(!ctx->vsx_enabled)) {
-            gen_exception(ctx, POWERPC_EXCP_VSXU);
+        if (unlikely(!ctx->altivec_enabled)) {
+            gen_exception(ctx, POWERPC_EXCP_VPU);
             return;
         }
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2] target/ppc: Fix xxspltib

[David Gibson]

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

On Thu, May 09, 2019 at 06:17:33AM +1000, Anton Blanchard wrote:
> xxspltib raises a VMX or a VSX exception depending on the register
> set it is operating on. We had a check, but it was backwards.
> 
> Fixes: f113283525a4 ("target-ppc: add xxspltib instruction")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>

Applied, thanks.

> ---
>  target/ppc/translate/vsx-impl.inc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index 4d8ca7cf32..4812a374aa 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -1355,13 +1355,13 @@ static void gen_xxspltib(DisasContext *ctx)
>      int rt = xT(ctx->opcode);
>  
>      if (rt < 32) {
> -        if (unlikely(!ctx->altivec_enabled)) {
> -            gen_exception(ctx, POWERPC_EXCP_VPU);
> +        if (unlikely(!ctx->vsx_enabled)) {
> +            gen_exception(ctx, POWERPC_EXCP_VSXU);
>              return;
>          }
>      } else {
> -        if (unlikely(!ctx->vsx_enabled)) {
> -            gen_exception(ctx, POWERPC_EXCP_VSXU);
> +        if (unlikely(!ctx->altivec_enabled)) {
> +            gen_exception(ctx, POWERPC_EXCP_VPU);
>              return;
>          }
>      }

-- 
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: 833 bytes --]

[Qemu-devel] [PATCH 3/9] target/ppc: Fix xxbrq, xxbrw

[Anton Blanchard]

Fix a typo in xxbrq and xxbrw where we put both results into the lower
doubleword.

Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/translate/vsx-impl.inc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index d050cc03ed..05b75105be 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -1192,7 +1192,7 @@ static void gen_xxbrq(DisasContext *ctx)
     tcg_gen_bswap64_i64(xtl, xbh);
     set_cpu_vsrl(xT(ctx->opcode), xtl);
     tcg_gen_mov_i64(xth, t0);
-    set_cpu_vsrl(xT(ctx->opcode), xth);
+    set_cpu_vsrh(xT(ctx->opcode), xth);
 
     tcg_temp_free_i64(t0);
     tcg_temp_free_i64(xth);
@@ -1220,7 +1220,7 @@ static void gen_xxbrw(DisasContext *ctx)
     get_cpu_vsrl(xbl, xB(ctx->opcode));
 
     gen_bswap32x4(xth, xtl, xbh, xbl);
-    set_cpu_vsrl(xT(ctx->opcode), xth);
+    set_cpu_vsrh(xT(ctx->opcode), xth);
     set_cpu_vsrl(xT(ctx->opcode), xtl);
 
     tcg_temp_free_i64(xth);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 3/9] target/ppc: Fix xxbrq, xxbrw

[David Gibson]

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

On Tue, May 07, 2019 at 10:48:05AM +1000, Anton Blanchard wrote:
> Fix a typo in xxbrq and xxbrw where we put both results into the lower
> doubleword.
> 
> Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>

Applied, thanks.

> ---
>  target/ppc/translate/vsx-impl.inc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index d050cc03ed..05b75105be 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -1192,7 +1192,7 @@ static void gen_xxbrq(DisasContext *ctx)
>      tcg_gen_bswap64_i64(xtl, xbh);
>      set_cpu_vsrl(xT(ctx->opcode), xtl);
>      tcg_gen_mov_i64(xth, t0);
> -    set_cpu_vsrl(xT(ctx->opcode), xth);
> +    set_cpu_vsrh(xT(ctx->opcode), xth);
>  
>      tcg_temp_free_i64(t0);
>      tcg_temp_free_i64(xth);
> @@ -1220,7 +1220,7 @@ static void gen_xxbrw(DisasContext *ctx)
>      get_cpu_vsrl(xbl, xB(ctx->opcode));
>  
>      gen_bswap32x4(xth, xtl, xbh, xbl);
> -    set_cpu_vsrl(xT(ctx->opcode), xth);
> +    set_cpu_vsrh(xT(ctx->opcode), xth);
>      set_cpu_vsrl(xT(ctx->opcode), xtl);
>  
>      tcg_temp_free_i64(xth);

-- 
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: 833 bytes --]

[Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[Anton Blanchard]

During the conversion these instructions were incorrectly treated as
stores. We need to use set_cpu_vsr* and not get_cpu_vsr*.

Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/translate/vsx-impl.inc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index 05b75105be..c13f84e745 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -102,8 +102,7 @@ static void gen_lxvw4x(DisasContext *ctx)
     }
     xth = tcg_temp_new_i64();
     xtl = tcg_temp_new_i64();
-    get_cpu_vsrh(xth, xT(ctx->opcode));
-    get_cpu_vsrl(xtl, xT(ctx->opcode));
+
     gen_set_access_type(ctx, ACCESS_INT);
     EA = tcg_temp_new();
 
@@ -126,6 +125,8 @@ static void gen_lxvw4x(DisasContext *ctx)
         tcg_gen_addi_tl(EA, EA, 8);
         tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_BEQ);
     }
+    set_cpu_vsrh(xT(ctx->opcode), xth);
+    set_cpu_vsrl(xT(ctx->opcode), xtl);
     tcg_temp_free(EA);
     tcg_temp_free_i64(xth);
     tcg_temp_free_i64(xtl);
@@ -185,8 +186,6 @@ static void gen_lxvh8x(DisasContext *ctx)
     }
     xth = tcg_temp_new_i64();
     xtl = tcg_temp_new_i64();
-    get_cpu_vsrh(xth, xT(ctx->opcode));
-    get_cpu_vsrl(xtl, xT(ctx->opcode));
     gen_set_access_type(ctx, ACCESS_INT);
 
     EA = tcg_temp_new();
@@ -197,6 +196,8 @@ static void gen_lxvh8x(DisasContext *ctx)
     if (ctx->le_mode) {
         gen_bswap16x8(xth, xtl, xth, xtl);
     }
+    set_cpu_vsrh(xT(ctx->opcode), xth);
+    set_cpu_vsrl(xT(ctx->opcode), xtl);
     tcg_temp_free(EA);
     tcg_temp_free_i64(xth);
     tcg_temp_free_i64(xtl);
@@ -214,14 +215,14 @@ static void gen_lxvb16x(DisasContext *ctx)
     }
     xth = tcg_temp_new_i64();
     xtl = tcg_temp_new_i64();
-    get_cpu_vsrh(xth, xT(ctx->opcode));
-    get_cpu_vsrl(xtl, xT(ctx->opcode));
     gen_set_access_type(ctx, ACCESS_INT);
     EA = tcg_temp_new();
     gen_addr_reg_index(ctx, EA);
     tcg_gen_qemu_ld_i64(xth, EA, ctx->mem_idx, MO_BEQ);
     tcg_gen_addi_tl(EA, EA, 8);
     tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_BEQ);
+    set_cpu_vsrh(xT(ctx->opcode), xth);
+    set_cpu_vsrl(xT(ctx->opcode), xtl);
     tcg_temp_free(EA);
     tcg_temp_free_i64(xth);
     tcg_temp_free_i64(xtl);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[David Gibson]

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

On Tue, May 07, 2019 at 10:48:06AM +1000, Anton Blanchard wrote:
> During the conversion these instructions were incorrectly treated as
> stores. We need to use set_cpu_vsr* and not get_cpu_vsr*.
> 
> Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> ---
>  target/ppc/translate/vsx-impl.inc.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index 05b75105be..c13f84e745 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -102,8 +102,7 @@ static void gen_lxvw4x(DisasContext *ctx)
>      }
>      xth = tcg_temp_new_i64();
>      xtl = tcg_temp_new_i64();
> -    get_cpu_vsrh(xth, xT(ctx->opcode));
> -    get_cpu_vsrl(xtl, xT(ctx->opcode));
> +

Something seems amiss here.  Clearly we do need a set..() back to the
loaded register, but with the removal of these gets, it doesn't look
like the xth and xtl temporaries are initialized any more.

>      gen_set_access_type(ctx, ACCESS_INT);
>      EA = tcg_temp_new();
>  
> @@ -126,6 +125,8 @@ static void gen_lxvw4x(DisasContext *ctx)
>          tcg_gen_addi_tl(EA, EA, 8);
>          tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_BEQ);
>      }
> +    set_cpu_vsrh(xT(ctx->opcode), xth);
> +    set_cpu_vsrl(xT(ctx->opcode), xtl);
>      tcg_temp_free(EA);
>      tcg_temp_free_i64(xth);
>      tcg_temp_free_i64(xtl);
> @@ -185,8 +186,6 @@ static void gen_lxvh8x(DisasContext *ctx)
>      }
>      xth = tcg_temp_new_i64();
>      xtl = tcg_temp_new_i64();
> -    get_cpu_vsrh(xth, xT(ctx->opcode));
> -    get_cpu_vsrl(xtl, xT(ctx->opcode));
>      gen_set_access_type(ctx, ACCESS_INT);
>  
>      EA = tcg_temp_new();
> @@ -197,6 +196,8 @@ static void gen_lxvh8x(DisasContext *ctx)
>      if (ctx->le_mode) {
>          gen_bswap16x8(xth, xtl, xth, xtl);
>      }
> +    set_cpu_vsrh(xT(ctx->opcode), xth);
> +    set_cpu_vsrl(xT(ctx->opcode), xtl);
>      tcg_temp_free(EA);
>      tcg_temp_free_i64(xth);
>      tcg_temp_free_i64(xtl);
> @@ -214,14 +215,14 @@ static void gen_lxvb16x(DisasContext *ctx)
>      }
>      xth = tcg_temp_new_i64();
>      xtl = tcg_temp_new_i64();
> -    get_cpu_vsrh(xth, xT(ctx->opcode));
> -    get_cpu_vsrl(xtl, xT(ctx->opcode));
>      gen_set_access_type(ctx, ACCESS_INT);
>      EA = tcg_temp_new();
>      gen_addr_reg_index(ctx, EA);
>      tcg_gen_qemu_ld_i64(xth, EA, ctx->mem_idx, MO_BEQ);
>      tcg_gen_addi_tl(EA, EA, 8);
>      tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_BEQ);
> +    set_cpu_vsrh(xT(ctx->opcode), xth);
> +    set_cpu_vsrl(xT(ctx->opcode), xtl);
>      tcg_temp_free(EA);
>      tcg_temp_free_i64(xth);
>      tcg_temp_free_i64(xtl);

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[Mark Cave-Ayland]

On 07/05/2019 06:28, David Gibson wrote:

> On Tue, May 07, 2019 at 10:48:06AM +1000, Anton Blanchard wrote:
>> During the conversion these instructions were incorrectly treated as
>> stores. We need to use set_cpu_vsr* and not get_cpu_vsr*.
>>
>> Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
>> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
>> ---
>>  target/ppc/translate/vsx-impl.inc.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
>> index 05b75105be..c13f84e745 100644
>> --- a/target/ppc/translate/vsx-impl.inc.c
>> +++ b/target/ppc/translate/vsx-impl.inc.c
>> @@ -102,8 +102,7 @@ static void gen_lxvw4x(DisasContext *ctx)
>>      }
>>      xth = tcg_temp_new_i64();
>>      xtl = tcg_temp_new_i64();
>> -    get_cpu_vsrh(xth, xT(ctx->opcode));
>> -    get_cpu_vsrl(xtl, xT(ctx->opcode));
>> +
> 
> Something seems amiss here.  Clearly we do need a set..() back to the
> loaded register, but with the removal of these gets, it doesn't look
> like the xth and xtl temporaries are initialized any more.
> 
>>      gen_set_access_type(ctx, ACCESS_INT);
>>      EA = tcg_temp_new();
>>  
>> @@ -126,6 +125,8 @@ static void gen_lxvw4x(DisasContext *ctx)
>>          tcg_gen_addi_tl(EA, EA, 8);
>>          tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_BEQ);
>>      }
>> +    set_cpu_vsrh(xT(ctx->opcode), xth);
>> +    set_cpu_vsrl(xT(ctx->opcode), xtl);
>>      tcg_temp_free(EA);
>>      tcg_temp_free_i64(xth);
>>      tcg_temp_free_i64(xtl);
>> @@ -185,8 +186,6 @@ static void gen_lxvh8x(DisasContext *ctx)
>>      }
>>      xth = tcg_temp_new_i64();
>>      xtl = tcg_temp_new_i64();
>> -    get_cpu_vsrh(xth, xT(ctx->opcode));
>> -    get_cpu_vsrl(xtl, xT(ctx->opcode));
>>      gen_set_access_type(ctx, ACCESS_INT);
>>  
>>      EA = tcg_temp_new();
>> @@ -197,6 +196,8 @@ static void gen_lxvh8x(DisasContext *ctx)
>>      if (ctx->le_mode) {
>>          gen_bswap16x8(xth, xtl, xth, xtl);
>>      }
>> +    set_cpu_vsrh(xT(ctx->opcode), xth);
>> +    set_cpu_vsrl(xT(ctx->opcode), xtl);
>>      tcg_temp_free(EA);
>>      tcg_temp_free_i64(xth);
>>      tcg_temp_free_i64(xtl);
>> @@ -214,14 +215,14 @@ static void gen_lxvb16x(DisasContext *ctx)
>>      }
>>      xth = tcg_temp_new_i64();
>>      xtl = tcg_temp_new_i64();
>> -    get_cpu_vsrh(xth, xT(ctx->opcode));
>> -    get_cpu_vsrl(xtl, xT(ctx->opcode));
>>      gen_set_access_type(ctx, ACCESS_INT);
>>      EA = tcg_temp_new();
>>      gen_addr_reg_index(ctx, EA);
>>      tcg_gen_qemu_ld_i64(xth, EA, ctx->mem_idx, MO_BEQ);
>>      tcg_gen_addi_tl(EA, EA, 8);
>>      tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_BEQ);
>> +    set_cpu_vsrh(xT(ctx->opcode), xth);
>> +    set_cpu_vsrl(xT(ctx->opcode), xtl);
>>      tcg_temp_free(EA);
>>      tcg_temp_free_i64(xth);
>>      tcg_temp_free_i64(xtl);

AFAICT I think that this is correct since the patterns should be as follows:

Load instructions:
    tcg_gen_qemu_ld_i64(xth, ...);
    set_cpu_vsrh(n, xth);

Store instructions:
    get_cpu_vsrh(xth, n);
    tcg_gen_qemu_st_i64(xth, ...);

I remember that when I first started experimenting with the very first version of
this patchset last year, someone on IRC (maybe Richard?) pointed out that I had
inverted the load and store operations and so I went and reworked them all from
scratch. Unfortunately with this and Greg's patch for stxsdx I have a feeling that
something when wrong during a cherry-pick or rebase of the patchset :(

Following on from this I've just gone through the load/store operations once again
and spotted two things:


1) VSX_LOAD_SCALAR_DS has an extra get_cpu_vsrh() which can be removed

diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index 11d9b75d01..004ea56c4f 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -329,7 +329,6 @@ static void gen_##name(DisasContext *ctx)                         \
         return;                                                   \
     }                                                             \
     xth = tcg_temp_new_i64();                                     \
-    get_cpu_vsrh(xth, rD(ctx->opcode) + 32);                      \
     gen_set_access_type(ctx, ACCESS_INT);                         \
     EA = tcg_temp_new();                                          \
     gen_addr_imm_index(ctx, EA, 0x03);                            \


2) VSX_VECTOR_LOAD_STORE is confusing and should be split into separate
VSX_VECTOR_LOAD and VSX_VECTOR_STORE macros


Does that sound reasonable? I'm also thinking that we should consider adding a CC to
stable for patches 4, 5 and 9 in this series since these are genuine regressions.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[Anton Blanchard]

Hi Mark,

> Following on from this I've just gone through the load/store
> operations once again and spotted two things:
> 
> 
> 1) VSX_LOAD_SCALAR_DS has an extra get_cpu_vsrh() which can be removed
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c
> b/target/ppc/translate/vsx-impl.inc.c index 11d9b75d01..004ea56c4f
> 100644 --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -329,7 +329,6 @@ static void gen_##name(DisasContext
> *ctx)                         \
> return;
> \ }                                                             \ xth
> = tcg_temp_new_i64();                                     \
> -    get_cpu_vsrh(xth, rD(ctx->opcode) + 32);                      \
>      gen_set_access_type(ctx, ACCESS_INT);                         \
>      EA = tcg_temp_new();                                          \
>      gen_addr_imm_index(ctx, EA, 0x03);                            \

Looks good. I also noticed we had two stores that needed to be fixed:

VSX_LOAD_SCALAR_DS(stxsd, st64_i64)
VSX_LOAD_SCALAR_DS(stxssp, st32fs)

> 2) VSX_VECTOR_LOAD_STORE is confusing and should be split into
> separate VSX_VECTOR_LOAD and VSX_VECTOR_STORE macros

Good idea. I also removed (what I assume) are redundant set_cpu_vsr*
and get_cpu_vsr* calls.

> Does that sound reasonable? I'm also thinking that we should consider
> adding a CC to stable for patches 4, 5 and 9 in this series since
> these are genuine regressions.

Fine with me. If David agrees, I'm not sure if he can rebase them or
if I can send them manually if they have been already committed.

Thanks,
Anton

Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[David Gibson]

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

On Thu, May 09, 2019 at 10:33:24AM +1000, Anton Blanchard wrote:
> Hi Mark,
> 
> > Following on from this I've just gone through the load/store
> > operations once again and spotted two things:
> > 
> > 
> > 1) VSX_LOAD_SCALAR_DS has an extra get_cpu_vsrh() which can be removed
> > 
> > diff --git a/target/ppc/translate/vsx-impl.inc.c
> > b/target/ppc/translate/vsx-impl.inc.c index 11d9b75d01..004ea56c4f
> > 100644 --- a/target/ppc/translate/vsx-impl.inc.c
> > +++ b/target/ppc/translate/vsx-impl.inc.c
> > @@ -329,7 +329,6 @@ static void gen_##name(DisasContext
> > *ctx)                         \
> > return;
> > \ }                                                             \ xth
> > = tcg_temp_new_i64();                                     \
> > -    get_cpu_vsrh(xth, rD(ctx->opcode) + 32);                      \
> >      gen_set_access_type(ctx, ACCESS_INT);                         \
> >      EA = tcg_temp_new();                                          \
> >      gen_addr_imm_index(ctx, EA, 0x03);                            \
> 
> Looks good. I also noticed we had two stores that needed to be fixed:
> 
> VSX_LOAD_SCALAR_DS(stxsd, st64_i64)
> VSX_LOAD_SCALAR_DS(stxssp, st32fs)
> 
> > 2) VSX_VECTOR_LOAD_STORE is confusing and should be split into
> > separate VSX_VECTOR_LOAD and VSX_VECTOR_STORE macros
> 
> Good idea. I also removed (what I assume) are redundant set_cpu_vsr*
> and get_cpu_vsr* calls.
> 
> > Does that sound reasonable? I'm also thinking that we should consider
> > adding a CC to stable for patches 4, 5 and 9 in this series since
> > these are genuine regressions.
> 
> Fine with me. If David agrees, I'm not sure if he can rebase them or
> if I can send them manually if they have been already committed.

Usually going to stable is just a matter of pinging Mike Roth and
getting him to include it.  I can queue if somewhere if you like, but
the stable cadance is so slow it tends to get forgotten a bit.

-- 
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: 833 bytes --]

[Qemu-devel] [PATCH] target/ppc: Optimise VSX_LOAD_SCALAR_DS and VSX_VECTOR_LOAD_STORE

[Anton Blanchard]

A few small optimisations:

In VSX_LOAD_SCALAR_DS() we can don't need to read the VSR via
get_cpu_vsrh().

Split VSX_VECTOR_LOAD_STORE() into two functions. Loads only need to
write the VSRs (set_cpu_vsr*()) and stores only need to read the VSRs
(get_cpu_vsr*())

Thanks to Mark Cave-Ayland for the suggestions.

Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/translate/vsx-impl.inc.c | 68 ++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 10 deletions(-)

diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index 4b7627f53b..cdb44b8b70 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -228,7 +228,7 @@ static void gen_lxvb16x(DisasContext *ctx)
     tcg_temp_free_i64(xtl);
 }
 
-#define VSX_VECTOR_LOAD_STORE(name, op, indexed)            \
+#define VSX_VECTOR_LOAD(name, op, indexed)                  \
 static void gen_##name(DisasContext *ctx)                   \
 {                                                           \
     int xt;                                                 \
@@ -255,8 +255,6 @@ static void gen_##name(DisasContext *ctx)                   \
     }                                                       \
     xth = tcg_temp_new_i64();                               \
     xtl = tcg_temp_new_i64();                               \
-    get_cpu_vsrh(xth, xt);                                  \
-    get_cpu_vsrl(xtl, xt);                                  \
     gen_set_access_type(ctx, ACCESS_INT);                   \
     EA = tcg_temp_new();                                    \
     if (indexed) {                                          \
@@ -282,10 +280,61 @@ static void gen_##name(DisasContext *ctx)                   \
     tcg_temp_free_i64(xtl);                                 \
 }
 
-VSX_VECTOR_LOAD_STORE(lxv, ld_i64, 0)
-VSX_VECTOR_LOAD_STORE(stxv, st_i64, 0)
-VSX_VECTOR_LOAD_STORE(lxvx, ld_i64, 1)
-VSX_VECTOR_LOAD_STORE(stxvx, st_i64, 1)
+VSX_VECTOR_LOAD(lxv, ld_i64, 0)
+VSX_VECTOR_LOAD(lxvx, ld_i64, 1)
+
+#define VSX_VECTOR_STORE(name, op, indexed)                 \
+static void gen_##name(DisasContext *ctx)                   \
+{                                                           \
+    int xt;                                                 \
+    TCGv EA;                                                \
+    TCGv_i64 xth;                                           \
+    TCGv_i64 xtl;                                           \
+                                                            \
+    if (indexed) {                                          \
+        xt = xT(ctx->opcode);                               \
+    } else {                                                \
+        xt = DQxT(ctx->opcode);                             \
+    }                                                       \
+                                                            \
+    if (xt < 32) {                                          \
+        if (unlikely(!ctx->vsx_enabled)) {                  \
+            gen_exception(ctx, POWERPC_EXCP_VSXU);          \
+            return;                                         \
+        }                                                   \
+    } else {                                                \
+        if (unlikely(!ctx->altivec_enabled)) {              \
+            gen_exception(ctx, POWERPC_EXCP_VPU);           \
+            return;                                         \
+        }                                                   \
+    }                                                       \
+    xth = tcg_temp_new_i64();                               \
+    xtl = tcg_temp_new_i64();                               \
+    get_cpu_vsrh(xth, xt);                                  \
+    get_cpu_vsrl(xtl, xt);                                  \
+    gen_set_access_type(ctx, ACCESS_INT);                   \
+    EA = tcg_temp_new();                                    \
+    if (indexed) {                                          \
+        gen_addr_reg_index(ctx, EA);                        \
+    } else {                                                \
+        gen_addr_imm_index(ctx, EA, 0x0F);                  \
+    }                                                       \
+    if (ctx->le_mode) {                                     \
+        tcg_gen_qemu_##op(xtl, EA, ctx->mem_idx, MO_LEQ);   \
+        tcg_gen_addi_tl(EA, EA, 8);                         \
+        tcg_gen_qemu_##op(xth, EA, ctx->mem_idx, MO_LEQ);   \
+    } else {                                                \
+        tcg_gen_qemu_##op(xth, EA, ctx->mem_idx, MO_BEQ);   \
+        tcg_gen_addi_tl(EA, EA, 8);                         \
+        tcg_gen_qemu_##op(xtl, EA, ctx->mem_idx, MO_BEQ);   \
+    }                                                       \
+    tcg_temp_free(EA);                                      \
+    tcg_temp_free_i64(xth);                                 \
+    tcg_temp_free_i64(xtl);                                 \
+}
+
+VSX_VECTOR_STORE(stxv, st_i64, 0)
+VSX_VECTOR_STORE(stxvx, st_i64, 1)
 
 #ifdef TARGET_PPC64
 #define VSX_VECTOR_LOAD_STORE_LENGTH(name)                      \
@@ -330,7 +379,6 @@ static void gen_##name(DisasContext *ctx)                         \
         return;                                                   \
     }                                                             \
     xth = tcg_temp_new_i64();                                     \
-    get_cpu_vsrh(xth, rD(ctx->opcode) + 32);                      \
     gen_set_access_type(ctx, ACCESS_INT);                         \
     EA = tcg_temp_new();                                          \
     gen_addr_imm_index(ctx, EA, 0x03);                            \
@@ -514,8 +562,8 @@ static void gen_##name(DisasContext *ctx)                         \
     tcg_temp_free_i64(xth);                                       \
 }
 
-VSX_LOAD_SCALAR_DS(stxsd, st64_i64)
-VSX_LOAD_SCALAR_DS(stxssp, st32fs)
+VSX_STORE_SCALAR_DS(stxsd, st64_i64)
+VSX_STORE_SCALAR_DS(stxssp, st32fs)
 
 static void gen_mfvsrwz(DisasContext *ctx)
 {
-- 
2.20.1

Re: [Qemu-devel] [PATCH] target/ppc: Optimise VSX_LOAD_SCALAR_DS and VSX_VECTOR_LOAD_STORE

[Mark Cave-Ayland]

On 09/05/2019 01:35, Anton Blanchard wrote:

> A few small optimisations:
> 
> In VSX_LOAD_SCALAR_DS() we can don't need to read the VSR via
> get_cpu_vsrh().
> 
> Split VSX_VECTOR_LOAD_STORE() into two functions. Loads only need to
> write the VSRs (set_cpu_vsr*()) and stores only need to read the VSRs
> (get_cpu_vsr*())
> 
> Thanks to Mark Cave-Ayland for the suggestions.
> 
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> ---
>  target/ppc/translate/vsx-impl.inc.c | 68 ++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index 4b7627f53b..cdb44b8b70 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -228,7 +228,7 @@ static void gen_lxvb16x(DisasContext *ctx)
>      tcg_temp_free_i64(xtl);
>  }
>  
> -#define VSX_VECTOR_LOAD_STORE(name, op, indexed)            \
> +#define VSX_VECTOR_LOAD(name, op, indexed)                  \
>  static void gen_##name(DisasContext *ctx)                   \
>  {                                                           \
>      int xt;                                                 \
> @@ -255,8 +255,6 @@ static void gen_##name(DisasContext *ctx)                   \
>      }                                                       \
>      xth = tcg_temp_new_i64();                               \
>      xtl = tcg_temp_new_i64();                               \
> -    get_cpu_vsrh(xth, xt);                                  \
> -    get_cpu_vsrl(xtl, xt);                                  \
>      gen_set_access_type(ctx, ACCESS_INT);                   \
>      EA = tcg_temp_new();                                    \
>      if (indexed) {                                          \
> @@ -282,10 +280,61 @@ static void gen_##name(DisasContext *ctx)                   \
>      tcg_temp_free_i64(xtl);                                 \
>  }
>  
> -VSX_VECTOR_LOAD_STORE(lxv, ld_i64, 0)
> -VSX_VECTOR_LOAD_STORE(stxv, st_i64, 0)
> -VSX_VECTOR_LOAD_STORE(lxvx, ld_i64, 1)
> -VSX_VECTOR_LOAD_STORE(stxvx, st_i64, 1)
> +VSX_VECTOR_LOAD(lxv, ld_i64, 0)
> +VSX_VECTOR_LOAD(lxvx, ld_i64, 1)
> +
> +#define VSX_VECTOR_STORE(name, op, indexed)                 \
> +static void gen_##name(DisasContext *ctx)                   \
> +{                                                           \
> +    int xt;                                                 \
> +    TCGv EA;                                                \
> +    TCGv_i64 xth;                                           \
> +    TCGv_i64 xtl;                                           \
> +                                                            \
> +    if (indexed) {                                          \
> +        xt = xT(ctx->opcode);                               \
> +    } else {                                                \
> +        xt = DQxT(ctx->opcode);                             \
> +    }                                                       \
> +                                                            \
> +    if (xt < 32) {                                          \
> +        if (unlikely(!ctx->vsx_enabled)) {                  \
> +            gen_exception(ctx, POWERPC_EXCP_VSXU);          \
> +            return;                                         \
> +        }                                                   \
> +    } else {                                                \
> +        if (unlikely(!ctx->altivec_enabled)) {              \
> +            gen_exception(ctx, POWERPC_EXCP_VPU);           \
> +            return;                                         \
> +        }                                                   \
> +    }                                                       \
> +    xth = tcg_temp_new_i64();                               \
> +    xtl = tcg_temp_new_i64();                               \
> +    get_cpu_vsrh(xth, xt);                                  \
> +    get_cpu_vsrl(xtl, xt);                                  \
> +    gen_set_access_type(ctx, ACCESS_INT);                   \
> +    EA = tcg_temp_new();                                    \
> +    if (indexed) {                                          \
> +        gen_addr_reg_index(ctx, EA);                        \
> +    } else {                                                \
> +        gen_addr_imm_index(ctx, EA, 0x0F);                  \
> +    }                                                       \
> +    if (ctx->le_mode) {                                     \
> +        tcg_gen_qemu_##op(xtl, EA, ctx->mem_idx, MO_LEQ);   \
> +        tcg_gen_addi_tl(EA, EA, 8);                         \
> +        tcg_gen_qemu_##op(xth, EA, ctx->mem_idx, MO_LEQ);   \
> +    } else {                                                \
> +        tcg_gen_qemu_##op(xth, EA, ctx->mem_idx, MO_BEQ);   \
> +        tcg_gen_addi_tl(EA, EA, 8);                         \
> +        tcg_gen_qemu_##op(xtl, EA, ctx->mem_idx, MO_BEQ);   \
> +    }                                                       \
> +    tcg_temp_free(EA);                                      \
> +    tcg_temp_free_i64(xth);                                 \
> +    tcg_temp_free_i64(xtl);                                 \
> +}
> +
> +VSX_VECTOR_STORE(stxv, st_i64, 0)
> +VSX_VECTOR_STORE(stxvx, st_i64, 1)
>  
>  #ifdef TARGET_PPC64
>  #define VSX_VECTOR_LOAD_STORE_LENGTH(name)                      \
> @@ -330,7 +379,6 @@ static void gen_##name(DisasContext *ctx)                         \
>          return;                                                   \
>      }                                                             \
>      xth = tcg_temp_new_i64();                                     \
> -    get_cpu_vsrh(xth, rD(ctx->opcode) + 32);                      \
>      gen_set_access_type(ctx, ACCESS_INT);                         \
>      EA = tcg_temp_new();                                          \
>      gen_addr_imm_index(ctx, EA, 0x03);                            \
> @@ -514,8 +562,8 @@ static void gen_##name(DisasContext *ctx)                         \
>      tcg_temp_free_i64(xth);                                       \
>  }
>  
> -VSX_LOAD_SCALAR_DS(stxsd, st64_i64)
> -VSX_LOAD_SCALAR_DS(stxssp, st32fs)
> +VSX_STORE_SCALAR_DS(stxsd, st64_i64)
> +VSX_STORE_SCALAR_DS(stxssp, st32fs)
>  
>  static void gen_mfvsrwz(DisasContext *ctx)
>  {

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[Mark Cave-Ayland]

On 07/05/2019 01:48, Anton Blanchard wrote:

> During the conversion these instructions were incorrectly treated as
> stores. We need to use set_cpu_vsr* and not get_cpu_vsr*.
> 
> Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> ---
>  target/ppc/translate/vsx-impl.inc.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index 05b75105be..c13f84e745 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -102,8 +102,7 @@ static void gen_lxvw4x(DisasContext *ctx)
>      }
>      xth = tcg_temp_new_i64();
>      xtl = tcg_temp_new_i64();
> -    get_cpu_vsrh(xth, xT(ctx->opcode));
> -    get_cpu_vsrl(xtl, xT(ctx->opcode));
> +
>      gen_set_access_type(ctx, ACCESS_INT);
>      EA = tcg_temp_new();
>  
> @@ -126,6 +125,8 @@ static void gen_lxvw4x(DisasContext *ctx)
>          tcg_gen_addi_tl(EA, EA, 8);
>          tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_BEQ);
>      }
> +    set_cpu_vsrh(xT(ctx->opcode), xth);
> +    set_cpu_vsrl(xT(ctx->opcode), xtl);
>      tcg_temp_free(EA);
>      tcg_temp_free_i64(xth);
>      tcg_temp_free_i64(xtl);
> @@ -185,8 +186,6 @@ static void gen_lxvh8x(DisasContext *ctx)
>      }
>      xth = tcg_temp_new_i64();
>      xtl = tcg_temp_new_i64();
> -    get_cpu_vsrh(xth, xT(ctx->opcode));
> -    get_cpu_vsrl(xtl, xT(ctx->opcode));
>      gen_set_access_type(ctx, ACCESS_INT);
>  
>      EA = tcg_temp_new();
> @@ -197,6 +196,8 @@ static void gen_lxvh8x(DisasContext *ctx)
>      if (ctx->le_mode) {
>          gen_bswap16x8(xth, xtl, xth, xtl);
>      }
> +    set_cpu_vsrh(xT(ctx->opcode), xth);
> +    set_cpu_vsrl(xT(ctx->opcode), xtl);
>      tcg_temp_free(EA);
>      tcg_temp_free_i64(xth);
>      tcg_temp_free_i64(xtl);
> @@ -214,14 +215,14 @@ static void gen_lxvb16x(DisasContext *ctx)
>      }
>      xth = tcg_temp_new_i64();
>      xtl = tcg_temp_new_i64();
> -    get_cpu_vsrh(xth, xT(ctx->opcode));
> -    get_cpu_vsrl(xtl, xT(ctx->opcode));
>      gen_set_access_type(ctx, ACCESS_INT);
>      EA = tcg_temp_new();
>      gen_addr_reg_index(ctx, EA);
>      tcg_gen_qemu_ld_i64(xth, EA, ctx->mem_idx, MO_BEQ);
>      tcg_gen_addi_tl(EA, EA, 8);
>      tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_BEQ);
> +    set_cpu_vsrh(xT(ctx->opcode), xth);
> +    set_cpu_vsrl(xT(ctx->opcode), xtl);
>      tcg_temp_free(EA);
>      tcg_temp_free_i64(xth);
>      tcg_temp_free_i64(xtl);

I've now had a bit of time to look through this and I believe it is correct, so:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[Anton Blanchard]

Hi,

> I've now had a bit of time to look through this and I believe it is
> correct, so:
> 
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks Mark. David: any chance we could get this merged? I can't run a
recent Ubuntu image successfully without it. sshd hangs when I try to
ssh into it.

Thanks,
Anton

Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[David Gibson]

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

On Wed, May 22, 2019 at 06:11:12AM +1000, Anton Blanchard wrote:
> Hi,
> 
> > I've now had a bit of time to look through this and I believe it is
> > correct, so:
> > 
> > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Thanks Mark. David: any chance we could get this merged? I can't run a
> recent Ubuntu image successfully without it. sshd hangs when I try to
> ssh into it.

I had a comment that was never addressed - it didn't look like the xth
and xtl temporaries were initialized after the patch.

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[Mark Cave-Ayland]

On 22/05/2019 01:49, David Gibson wrote:

> On Wed, May 22, 2019 at 06:11:12AM +1000, Anton Blanchard wrote:
>> Hi,
>>
>>> I've now had a bit of time to look through this and I believe it is
>>> correct, so:
>>>
>>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> Thanks Mark. David: any chance we could get this merged? I can't run a
>> recent Ubuntu image successfully without it. sshd hangs when I try to
>> ssh into it.
> 
> I had a comment that was never addressed - it didn't look like the xth
> and xtl temporaries were initialized after the patch.

If it helps, here was my analysis at the time (looks like you were also included on
the reply?): https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01515.html.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[David Gibson]

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

On Wed, May 22, 2019 at 05:37:47AM +0100, Mark Cave-Ayland wrote:
> On 22/05/2019 01:49, David Gibson wrote:
> 
> > On Wed, May 22, 2019 at 06:11:12AM +1000, Anton Blanchard wrote:
> >> Hi,
> >>
> >>> I've now had a bit of time to look through this and I believe it is
> >>> correct, so:
> >>>
> >>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>
> >> Thanks Mark. David: any chance we could get this merged? I can't run a
> >> recent Ubuntu image successfully without it. sshd hangs when I try to
> >> ssh into it.
> > 
> > I had a comment that was never addressed - it didn't look like the xth
> > and xtl temporaries were initialized after the patch.
> 
> If it helps, here was my analysis at the time (looks like you were also included on
> the reply?): https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01515.html.

Sorry, I missed that.  Looks reasonable, I think I failed to spot the
generated load instructions which effectively initialize the temps.

This is all at some remove now, can you resend the patch on top of the
latest tree please and I'll apply.  It's missed the pull request I
sent today, obviously, but I know I have some other stuff I want to
get in pretty soon, so I expect to send another one relatively soon.

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[Mark Cave-Ayland]

On 22/05/2019 07:10, David Gibson wrote:

> On Wed, May 22, 2019 at 05:37:47AM +0100, Mark Cave-Ayland wrote:
>> On 22/05/2019 01:49, David Gibson wrote:
>>
>>> On Wed, May 22, 2019 at 06:11:12AM +1000, Anton Blanchard wrote:
>>>> Hi,
>>>>
>>>>> I've now had a bit of time to look through this and I believe it is
>>>>> correct, so:
>>>>>
>>>>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>
>>>> Thanks Mark. David: any chance we could get this merged? I can't run a
>>>> recent Ubuntu image successfully without it. sshd hangs when I try to
>>>> ssh into it.
>>>
>>> I had a comment that was never addressed - it didn't look like the xth
>>> and xtl temporaries were initialized after the patch.
>>
>> If it helps, here was my analysis at the time (looks like you were also included on
>> the reply?): https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01515.html.
> 
> Sorry, I missed that.  Looks reasonable, I think I failed to spot the
> generated load instructions which effectively initialize the temps.
> 
> This is all at some remove now, can you resend the patch on top of the
> latest tree please and I'll apply.  It's missed the pull request I
> sent today, obviously, but I know I have some other stuff I want to
> get in pretty soon, so I expect to send another one relatively soon.

All done - I've just sent a v2 rebased upon your ppc-for-4.1 branch with R-B and T-B
tags included.


ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x

[Greg Kurz]

On Wed, 22 May 2019 06:11:12 +1000
Anton Blanchard <anton@ozlabs.org> wrote:

> Hi,
> 
> > I've now had a bit of time to look through this and I believe it is
> > correct, so:
> > 
> > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>  
> 
> Thanks Mark. David: any chance we could get this merged? I can't run a
> recent Ubuntu image successfully without it. sshd hangs when I try to
> ssh into it.
> 

Ha ! I also had troubles ssh'ing into a fedora guest, but couldn't find
time to investigate:

$ ssh 192.168.122.76
ssh_dispatch_run_fatal: Connection to 192.168.122.76 port 22: incorrect signature

It doesn't happen anymore with this patch. Maybe worth mentioning it in the
changelog, and Cc stable of course since this is a regression in QEMU 4.0.

Tested-by: Greg Kurz <groug@kaod.org>

Also, as Mark mentioned in another mail, this looks very much like the other
bug I had fixed with 3e5365b7aa6c "target/ppc: Fix QEMU crash with stxsdx".
The patch looks correct:

Reviewed-by: Greg Kurz <groug@kaod.org>

> Thanks,
> Anton
> 

[Qemu-devel] [PATCH 5/9] target/ppc: Fix xvabs[sd]p, xvnabs[sd]p, xvneg[sd]p, xvcpsgn[sd]p

[Anton Blanchard]

We were using set_cpu_vsr* when we should have used set_cpu_vsrl*

Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/translate/vsx-impl.inc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index c13f84e745..0a48020e3b 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -859,8 +859,8 @@ static void glue(gen_, name)(DisasContext *ctx)                  \
         xbh = tcg_temp_new_i64();                                \
         xbl = tcg_temp_new_i64();                                \
         sgm = tcg_temp_new_i64();                                \
-        set_cpu_vsrh(xB(ctx->opcode), xbh);                      \
-        set_cpu_vsrl(xB(ctx->opcode), xbl);                      \
+        get_cpu_vsrh(xbh, xB(ctx->opcode));                      \
+        get_cpu_vsrl(xbl, xB(ctx->opcode));                      \
         tcg_gen_movi_i64(sgm, sgn_mask);                         \
         switch (op) {                                            \
             case OP_ABS: {                                       \
-- 
2.20.1

Re: [Qemu-devel] [PATCH 5/9] target/ppc: Fix xvabs[sd]p, xvnabs[sd]p, xvneg[sd]p, xvcpsgn[sd]p

[David Gibson]

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

On Tue, May 07, 2019 at 10:48:07AM +1000, Anton Blanchard wrote:
> We were using set_cpu_vsr* when we should have used set_cpu_vsrl*

Commit message doesn't appear to match the code.

> 
> Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> ---
>  target/ppc/translate/vsx-impl.inc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index c13f84e745..0a48020e3b 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -859,8 +859,8 @@ static void glue(gen_, name)(DisasContext *ctx)                  \
>          xbh = tcg_temp_new_i64();                                \
>          xbl = tcg_temp_new_i64();                                \
>          sgm = tcg_temp_new_i64();                                \
> -        set_cpu_vsrh(xB(ctx->opcode), xbh);                      \
> -        set_cpu_vsrl(xB(ctx->opcode), xbl);                      \
> +        get_cpu_vsrh(xbh, xB(ctx->opcode));                      \
> +        get_cpu_vsrl(xbl, xB(ctx->opcode));                      \
>          tcg_gen_movi_i64(sgm, sgn_mask);                         \
>          switch (op) {                                            \
>              case OP_ABS: {                                       \

-- 
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: 833 bytes --]

[Qemu-devel] [PATCH v2] target/ppc: Fix xvabs[sd]p, xvnabs[sd]p, xvneg[sd]p, xvcpsgn[sd]p

[Anton Blanchard]

We were using set_cpu_vsr*() when we should have used get_cpu_vsr*().

Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/translate/vsx-impl.inc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index b487136d52..4b7627f53b 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -859,8 +859,8 @@ static void glue(gen_, name)(DisasContext *ctx)                  \
         xbh = tcg_temp_new_i64();                                \
         xbl = tcg_temp_new_i64();                                \
         sgm = tcg_temp_new_i64();                                \
-        set_cpu_vsrh(xB(ctx->opcode), xbh);                      \
-        set_cpu_vsrl(xB(ctx->opcode), xbl);                      \
+        get_cpu_vsrh(xbh, xB(ctx->opcode));                      \
+        get_cpu_vsrl(xbl, xB(ctx->opcode));                      \
         tcg_gen_movi_i64(sgm, sgn_mask);                         \
         switch (op) {                                            \
             case OP_ABS: {                                       \
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2] target/ppc: Fix xvabs[sd]p, xvnabs[sd]p, xvneg[sd]p, xvcpsgn[sd]p

[Mark Cave-Ayland]

On 09/05/2019 01:49, Anton Blanchard wrote:

> We were using set_cpu_vsr*() when we should have used get_cpu_vsr*().
> 
> Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> ---
>  target/ppc/translate/vsx-impl.inc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index b487136d52..4b7627f53b 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -859,8 +859,8 @@ static void glue(gen_, name)(DisasContext *ctx)                  \
>          xbh = tcg_temp_new_i64();                                \
>          xbl = tcg_temp_new_i64();                                \
>          sgm = tcg_temp_new_i64();                                \
> -        set_cpu_vsrh(xB(ctx->opcode), xbh);                      \
> -        set_cpu_vsrl(xB(ctx->opcode), xbl);                      \
> +        get_cpu_vsrh(xbh, xB(ctx->opcode));                      \
> +        get_cpu_vsrl(xbl, xB(ctx->opcode));                      \
>          tcg_gen_movi_i64(sgm, sgn_mask);                         \
>          switch (op) {                                            \
>              case OP_ABS: {                                       \

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH v2] target/ppc: Fix xvabs[sd]p, xvnabs[sd]p, xvneg[sd]p, xvcpsgn[sd]p

[David Gibson]

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

On Fri, May 10, 2019 at 04:02:56PM +0100, Mark Cave-Ayland wrote:
> On 09/05/2019 01:49, Anton Blanchard wrote:
> 
> > We were using set_cpu_vsr*() when we should have used get_cpu_vsr*().
> > 
> > Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
> > Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> > ---
> >  target/ppc/translate/vsx-impl.inc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> > index b487136d52..4b7627f53b 100644
> > --- a/target/ppc/translate/vsx-impl.inc.c
> > +++ b/target/ppc/translate/vsx-impl.inc.c
> > @@ -859,8 +859,8 @@ static void glue(gen_, name)(DisasContext *ctx)                  \
> >          xbh = tcg_temp_new_i64();                                \
> >          xbl = tcg_temp_new_i64();                                \
> >          sgm = tcg_temp_new_i64();                                \
> > -        set_cpu_vsrh(xB(ctx->opcode), xbh);                      \
> > -        set_cpu_vsrl(xB(ctx->opcode), xbl);                      \
> > +        get_cpu_vsrh(xbh, xB(ctx->opcode));                      \
> > +        get_cpu_vsrl(xbl, xB(ctx->opcode));                      \
> >          tcg_gen_movi_i64(sgm, sgn_mask);                         \
> >          switch (op) {                                            \
> >              case OP_ABS: {                                       \
> 
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied, thanks.

> 
> 
> ATB,
> 
> Mark.
> 

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 5/9] target/ppc: Fix xvabs[sd]p, xvnabs[sd]p, xvneg[sd]p, xvcpsgn[sd]p

[Mark Cave-Ayland]

On 07/05/2019 01:48, Anton Blanchard wrote:

> We were using set_cpu_vsr* when we should have used set_cpu_vsrl*
> 
> Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}() helpers for VSR register access")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> ---
>  target/ppc/translate/vsx-impl.inc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index c13f84e745..0a48020e3b 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -859,8 +859,8 @@ static void glue(gen_, name)(DisasContext *ctx)                  \
>          xbh = tcg_temp_new_i64();                                \
>          xbl = tcg_temp_new_i64();                                \
>          sgm = tcg_temp_new_i64();                                \
> -        set_cpu_vsrh(xB(ctx->opcode), xbh);                      \
> -        set_cpu_vsrl(xB(ctx->opcode), xbl);                      \
> +        get_cpu_vsrh(xbh, xB(ctx->opcode));                      \
> +        get_cpu_vsrl(xbl, xB(ctx->opcode));                      \
>          tcg_gen_movi_i64(sgm, sgn_mask);                         \
>          switch (op) {                                            \
>              case OP_ABS: {                                       \

With the typo in the subject line fixed:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[Qemu-devel] [PATCH 6/9] target/ppc: Fix vslv and vsrv

[Anton Blanchard]

vslv and vsrv are broken on little endian, we append 00 to the
high byte not the low byte. Fix it by using the VsrB() accessor.

Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/int_helper.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
index f6a088ac08..fd715b4076 100644
--- a/target/ppc/int_helper.c
+++ b/target/ppc/int_helper.c
@@ -1800,10 +1800,10 @@ void helper_vslv(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
 
     size = ARRAY_SIZE(r->u8);
     for (i = 0; i < size; i++) {
-        shift = b->u8[i] & 0x7;             /* extract shift value */
-        bytes = (a->u8[i] << 8) +             /* extract adjacent bytes */
-            (((i + 1) < size) ? a->u8[i + 1] : 0);
-        r->u8[i] = (bytes << shift) >> 8;   /* shift and store result */
+        shift = b->VsrB(i) & 0x7;             /* extract shift value */
+        bytes = (a->VsrB(i) << 8) +           /* extract adjacent bytes */
+            (((i + 1) < size) ? a->VsrB(i + 1) : 0);
+        r->VsrB(i) = (bytes << shift) >> 8;   /* shift and store result */
     }
 }
 
@@ -1818,10 +1818,10 @@ void helper_vsrv(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
      * order will guarantee that computed result is not fed back.
      */
     for (i = ARRAY_SIZE(r->u8) - 1; i >= 0; i--) {
-        shift = b->u8[i] & 0x7;                 /* extract shift value */
-        bytes = ((i ? a->u8[i - 1] : 0) << 8) + a->u8[i];
+        shift = b->VsrB(i) & 0x7;               /* extract shift value */
+        bytes = ((i ? a->VsrB(i - 1) : 0) << 8) + a->VsrB(i);
                                                 /* extract adjacent bytes */
-        r->u8[i] = (bytes >> shift) & 0xFF;     /* shift and store result */
+        r->VsrB(i) = (bytes >> shift) & 0xFF;   /* shift and store result */
     }
 }
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH 6/9] target/ppc: Fix vslv and vsrv

[David Gibson]

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

On Tue, May 07, 2019 at 10:48:08AM +1000, Anton Blanchard wrote:
> vslv and vsrv are broken on little endian, we append 00 to the
> high byte not the low byte. Fix it by using the VsrB() accessor.
> 
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>

Applied, thanks.

> ---
>  target/ppc/int_helper.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
> index f6a088ac08..fd715b4076 100644
> --- a/target/ppc/int_helper.c
> +++ b/target/ppc/int_helper.c
> @@ -1800,10 +1800,10 @@ void helper_vslv(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
>  
>      size = ARRAY_SIZE(r->u8);
>      for (i = 0; i < size; i++) {
> -        shift = b->u8[i] & 0x7;             /* extract shift value */
> -        bytes = (a->u8[i] << 8) +             /* extract adjacent bytes */
> -            (((i + 1) < size) ? a->u8[i + 1] : 0);
> -        r->u8[i] = (bytes << shift) >> 8;   /* shift and store result */
> +        shift = b->VsrB(i) & 0x7;             /* extract shift value */
> +        bytes = (a->VsrB(i) << 8) +           /* extract adjacent bytes */
> +            (((i + 1) < size) ? a->VsrB(i + 1) : 0);
> +        r->VsrB(i) = (bytes << shift) >> 8;   /* shift and store result */
>      }
>  }
>  
> @@ -1818,10 +1818,10 @@ void helper_vsrv(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
>       * order will guarantee that computed result is not fed back.
>       */
>      for (i = ARRAY_SIZE(r->u8) - 1; i >= 0; i--) {
> -        shift = b->u8[i] & 0x7;                 /* extract shift value */
> -        bytes = ((i ? a->u8[i - 1] : 0) << 8) + a->u8[i];
> +        shift = b->VsrB(i) & 0x7;               /* extract shift value */
> +        bytes = ((i ? a->VsrB(i - 1) : 0) << 8) + a->VsrB(i);
>                                                  /* extract adjacent bytes */
> -        r->u8[i] = (bytes >> shift) & 0xFF;     /* shift and store result */
> +        r->VsrB(i) = (bytes >> shift) & 0xFF;   /* shift and store result */
>      }
>  }
>  

-- 
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: 833 bytes --]

[Qemu-devel] [PATCH 7/9] target/ppc: Fix vrlwmi and vrlwnm

[Anton Blanchard]

We should only look at 5 bits of each byte, not 6.

Fixes: 3e00884f4e9f ("target-ppc: add vrldnmi and vrlwmi instructions")
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/int_helper.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
index fd715b4076..111586c981 100644
--- a/target/ppc/int_helper.c
+++ b/target/ppc/int_helper.c
@@ -1652,7 +1652,7 @@ void helper_vrsqrtefp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *b)
     }
 }
 
-#define VRLMI(name, size, element, insert)                            \
+#define VRLMI(name, size, element, insert, modifier_bits)             \
 void helper_##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)          \
 {                                                                     \
     int i;                                                            \
@@ -1662,9 +1662,9 @@ void helper_##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)          \
         uint##size##_t src3 = r->element[i];                          \
         uint##size##_t begin, end, shift, mask, rot_val;              \
                                                                       \
-        shift = extract##size(src2, 0, 6);                            \
-        end   = extract##size(src2, 8, 6);                            \
-        begin = extract##size(src2, 16, 6);                           \
+        shift = extract##size(src2, 0, modifier_bits);                \
+        end   = extract##size(src2, 8, modifier_bits);                \
+        begin = extract##size(src2, 16, modifier_bits);               \
         rot_val = rol##size(src1, shift);                             \
         mask = mask_u##size(begin, end);                              \
         if (insert) {                                                 \
@@ -1675,10 +1675,10 @@ void helper_##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)          \
     }                                                                 \
 }
 
-VRLMI(vrldmi, 64, u64, 1);
-VRLMI(vrlwmi, 32, u32, 1);
-VRLMI(vrldnm, 64, u64, 0);
-VRLMI(vrlwnm, 32, u32, 0);
+VRLMI(vrldmi, 64, u64, 1, 6);
+VRLMI(vrlwmi, 32, u32, 1, 5);
+VRLMI(vrldnm, 64, u64, 0, 6);
+VRLMI(vrlwnm, 32, u32, 0, 5);
 
 void helper_vsel(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b,
                  ppc_avr_t *c)
-- 
2.20.1

Re: [Qemu-devel] [PATCH 7/9] target/ppc: Fix vrlwmi and vrlwnm

[David Gibson]

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

On Tue, May 07, 2019 at 10:48:09AM +1000, Anton Blanchard wrote:
> We should only look at 5 bits of each byte, not 6.
> 
> Fixes: 3e00884f4e9f ("target-ppc: add vrldnmi and vrlwmi instructions")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>

Hrm.  So, what lives in those extra bits in the 'w' instructions?  Is
ignoring it correct?  Should we throw an exception?  Does it mean
something else?

> ---
>  target/ppc/int_helper.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
> index fd715b4076..111586c981 100644
> --- a/target/ppc/int_helper.c
> +++ b/target/ppc/int_helper.c
> @@ -1652,7 +1652,7 @@ void helper_vrsqrtefp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *b)
>      }
>  }
>  
> -#define VRLMI(name, size, element, insert)                            \
> +#define VRLMI(name, size, element, insert, modifier_bits)             \
>  void helper_##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)          \
>  {                                                                     \
>      int i;                                                            \
> @@ -1662,9 +1662,9 @@ void helper_##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)          \
>          uint##size##_t src3 = r->element[i];                          \
>          uint##size##_t begin, end, shift, mask, rot_val;              \
>                                                                        \
> -        shift = extract##size(src2, 0, 6);                            \
> -        end   = extract##size(src2, 8, 6);                            \
> -        begin = extract##size(src2, 16, 6);                           \
> +        shift = extract##size(src2, 0, modifier_bits);                \
> +        end   = extract##size(src2, 8, modifier_bits);                \
> +        begin = extract##size(src2, 16, modifier_bits);               \
>          rot_val = rol##size(src1, shift);                             \
>          mask = mask_u##size(begin, end);                              \
>          if (insert) {                                                 \
> @@ -1675,10 +1675,10 @@ void helper_##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)          \
>      }                                                                 \
>  }
>  
> -VRLMI(vrldmi, 64, u64, 1);
> -VRLMI(vrlwmi, 32, u32, 1);
> -VRLMI(vrldnm, 64, u64, 0);
> -VRLMI(vrlwnm, 32, u32, 0);
> +VRLMI(vrldmi, 64, u64, 1, 6);
> +VRLMI(vrlwmi, 32, u32, 1, 5);
> +VRLMI(vrldnm, 64, u64, 0, 6);
> +VRLMI(vrlwnm, 32, u32, 0, 5);
>  
>  void helper_vsel(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b,
>                   ppc_avr_t *c)

-- 
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: 833 bytes --]

[Qemu-devel] [PATCH 8/9] target/ppc: Fix dtstsfi and dtstsfiq

[Anton Blanchard]

The immediate field is 6 bits, not 5.

Fixes: 217f6b88058f ("target-ppc: add dtstsfi[q] instructions")
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/internal.h               | 2 ++
 target/ppc/translate/dfp-impl.inc.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index fb6f64ed1e..4719369cc5 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -123,6 +123,8 @@ static inline uint32_t SPR(uint32_t opcode)
 EXTRACT_SHELPER(SIMM, 0, 16);
 /* 16 bits unsigned immediate value */
 EXTRACT_HELPER(UIMM, 0, 16);
+/* 6 bits unsigned immediate value */
+EXTRACT_HELPER(UIMM6, 16, 6);
 /* 5 bits signed immediate value */
 EXTRACT_SHELPER(SIMM5, 16, 5);
 /* 5 bits signed immediate value */
diff --git a/target/ppc/translate/dfp-impl.inc.c b/target/ppc/translate/dfp-impl.inc.c
index 6c556dc2e1..5b01c9239d 100644
--- a/target/ppc/translate/dfp-impl.inc.c
+++ b/target/ppc/translate/dfp-impl.inc.c
@@ -55,7 +55,7 @@ static void gen_##name(DisasContext *ctx)         \
         return;                                   \
     }                                             \
     gen_update_nip(ctx, ctx->base.pc_next - 4);            \
-    uim = tcg_const_i32(UIMM5(ctx->opcode));      \
+    uim = tcg_const_i32(UIMM6(ctx->opcode));      \
     rb = gen_fprp_ptr(rB(ctx->opcode));           \
     gen_helper_##name(cpu_crf[crfD(ctx->opcode)], \
                       cpu_env, uim, rb);          \
-- 
2.20.1

[Qemu-devel] [PATCH 9/9] target/ppc: Fix vsum2sws

[Anton Blanchard]

A recent cleanup changed the pre zeroing of the result from 64 bit
to 32 bit operations:

-        result.u64[i] = 0;
+        result.VsrW(i) = 0;

This corrupts the result.

Fixes: 60594fea298d ("target/ppc: remove various HOST_WORDS_BIGENDIAN hacks in int_helper.c")
Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 target/ppc/int_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
index 111586c981..b8b3279f71 100644
--- a/target/ppc/int_helper.c
+++ b/target/ppc/int_helper.c
@@ -2038,7 +2038,7 @@ void helper_vsum2sws(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
     for (i = 0; i < ARRAY_SIZE(r->u64); i++) {
         int64_t t = (int64_t)b->VsrSW(upper + i * 2);
 
-        result.VsrW(i) = 0;
+        result.VsrD(i) = 0;
         for (j = 0; j < ARRAY_SIZE(r->u64); j++) {
             t += a->VsrSW(2 * i + j);
         }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 9/9] target/ppc: Fix vsum2sws

[David Gibson]

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

On Tue, May 07, 2019 at 10:48:11AM +1000, Anton Blanchard wrote:
> A recent cleanup changed the pre zeroing of the result from 64 bit
> to 32 bit operations:
> 
> -        result.u64[i] = 0;
> +        result.VsrW(i) = 0;
> 
> This corrupts the result.
> 
> Fixes: 60594fea298d ("target/ppc: remove various HOST_WORDS_BIGENDIAN hacks in int_helper.c")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>

Applied, thanks.

> ---
>  target/ppc/int_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
> index 111586c981..b8b3279f71 100644
> --- a/target/ppc/int_helper.c
> +++ b/target/ppc/int_helper.c
> @@ -2038,7 +2038,7 @@ void helper_vsum2sws(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
>      for (i = 0; i < ARRAY_SIZE(r->u64); i++) {
>          int64_t t = (int64_t)b->VsrSW(upper + i * 2);
>  
> -        result.VsrW(i) = 0;
> +        result.VsrD(i) = 0;
>          for (j = 0; j < ARRAY_SIZE(r->u64); j++) {
>              t += a->VsrSW(2 * i + j);
>          }

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 9/9] target/ppc: Fix vsum2sws

[Mark Cave-Ayland]

On 07/05/2019 01:48, Anton Blanchard wrote:

> A recent cleanup changed the pre zeroing of the result from 64 bit
> to 32 bit operations:
> 
> -        result.u64[i] = 0;
> +        result.VsrW(i) = 0;
> 
> This corrupts the result.
> 
> Fixes: 60594fea298d ("target/ppc: remove various HOST_WORDS_BIGENDIAN hacks in int_helper.c")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> ---
>  target/ppc/int_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
> index 111586c981..b8b3279f71 100644
> --- a/target/ppc/int_helper.c
> +++ b/target/ppc/int_helper.c
> @@ -2038,7 +2038,7 @@ void helper_vsum2sws(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
>      for (i = 0; i < ARRAY_SIZE(r->u64); i++) {
>          int64_t t = (int64_t)b->VsrSW(upper + i * 2);
>  
> -        result.VsrW(i) = 0;
> +        result.VsrD(i) = 0;
>          for (j = 0; j < ARRAY_SIZE(r->u64); j++) {
>              t += a->VsrSW(2 * i + j);
>          }

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH 1/9] target/ppc: Fix xvxsigdp

[Alexey Kardashevskiy]

On 07/05/2019 10:48, Anton Blanchard wrote:
> Fix a typo in xvxsigdp where we put both results into the lower
> doubleword.
> 
> Fixes: dd977e4f45cb ("target/ppc: Optimize x[sv]xsigdp using deposit_i64()")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> ---
>  target/ppc/translate/vsx-impl.inc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index 11d9b75d01..4d8ca7cf32 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -1820,7 +1820,7 @@ static void gen_xvxsigdp(DisasContext *ctx)
>      tcg_gen_movi_i64(t0, 0x0010000000000000);
>      tcg_gen_movcond_i64(TCG_COND_EQ, t0, exp, zr, zr, t0);
>      tcg_gen_movcond_i64(TCG_COND_EQ, t0, exp, nan, zr, t0);
> -    tcg_gen_deposit_i64(xth, t0, xbl, 0, 52);
> +    tcg_gen_deposit_i64(xtl, t0, xbl, 0, 52);


Out of curiosity - how did you find this one and (especially) the next
one - "Fix xxspltib"? Is there some testsuite, or by just looking at the
code? Thanks,


>      set_cpu_vsrl(xT(ctx->opcode), xtl);
>  
>      tcg_temp_free_i64(t0);
> 

-- 
Alexey

Re: [Qemu-devel] [PATCH 1/9] target/ppc: Fix xvxsigdp

[Anton Blanchard]

Hi Alexey,

> Out of curiosity - how did you find this one and (especially) the next
> one - "Fix xxspltib"? Is there some testsuite, or by just looking at
> the code? Thanks,

I'm running test cases and comparing results between QEMU and real
hardware.

Thanks,
Anton

Re: [Qemu-devel] [PATCH 1/9] target/ppc: Fix xvxsigdp

[Mark Cave-Ayland]

On 07/05/2019 04:48, Anton Blanchard wrote:

> Hi Alexey,
> 
>> Out of curiosity - how did you find this one and (especially) the next
>> one - "Fix xxspltib"? Is there some testsuite, or by just looking at
>> the code? Thanks,
> 
> I'm running test cases and comparing results between QEMU and real
> hardware.

Nice - thanks for looking into these issues and reporting them. There is likely some
other low hanging fruit e.g. converting more accesses of the form .uX[i] into VsrX(i)
which can potentially be done, but without much in the way of test coverage against
real hardware I've been reluctant to go ahead with more changes.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 1/9] target/ppc: Fix xvxsigdp

[David Gibson]

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

On Tue, May 07, 2019 at 10:48:03AM +1000, Anton Blanchard wrote:
> Fix a typo in xvxsigdp where we put both results into the lower
> doubleword.
> 
> Fixes: dd977e4f45cb ("target/ppc: Optimize x[sv]xsigdp using deposit_i64()")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>

Applied, thanks.

> ---
>  target/ppc/translate/vsx-impl.inc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index 11d9b75d01..4d8ca7cf32 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -1820,7 +1820,7 @@ static void gen_xvxsigdp(DisasContext *ctx)
>      tcg_gen_movi_i64(t0, 0x0010000000000000);
>      tcg_gen_movcond_i64(TCG_COND_EQ, t0, exp, zr, zr, t0);
>      tcg_gen_movcond_i64(TCG_COND_EQ, t0, exp, nan, zr, t0);
> -    tcg_gen_deposit_i64(xth, t0, xbl, 0, 52);
> +    tcg_gen_deposit_i64(xtl, t0, xbl, 0, 52);
>      set_cpu_vsrl(xT(ctx->opcode), xtl);
>  
>      tcg_temp_free_i64(t0);

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/9] target/ppc: Fix xvxsigdp

[Philippe Mathieu-Daudé]

On 5/7/19 2:48 AM, Anton Blanchard wrote:
> Fix a typo in xvxsigdp where we put both results into the lower
> doubleword.
> 
> Fixes: dd977e4f45cb ("target/ppc: Optimize x[sv]xsigdp using deposit_i64()")
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>
> ---
>  target/ppc/translate/vsx-impl.inc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index 11d9b75d01..4d8ca7cf32 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -1820,7 +1820,7 @@ static void gen_xvxsigdp(DisasContext *ctx)
>      tcg_gen_movi_i64(t0, 0x0010000000000000);
>      tcg_gen_movcond_i64(TCG_COND_EQ, t0, exp, zr, zr, t0);
>      tcg_gen_movcond_i64(TCG_COND_EQ, t0, exp, nan, zr, t0);
> -    tcg_gen_deposit_i64(xth, t0, xbl, 0, 52);
> +    tcg_gen_deposit_i64(xtl, t0, xbl, 0, 52);

Argh I'm very sorry. I obviously failed while copy/pasting

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>      set_cpu_vsrl(xT(ctx->opcode), xtl);
>  
>      tcg_temp_free_i64(t0);
> 

Re: [Qemu-devel] [PATCH 1/9] target/ppc: Fix xvxsigdp

[Eric Blake]

On 5/6/19 7:48 PM, Anton Blanchard wrote:
> Fix a typo in xvxsigdp where we put both results into the lower
> doubleword.
> 

Just a reminder - when sending a series, it's best to have a 0/9 cover
letter with all the other patches In-Reply-To the cover. More patch
submission tips at:
https://wiki.qemu.org/Contribute/SubmitAPatch

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org