[Qemu-devel] [PATCH v2 0/7] target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order

[Qemu-devel] [PATCH v2 0/7] target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order

[Mark Cave-Ayland]

After some investigation into Andrew's report of corruption in his ppc64le
tests at https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07234.html, I
discovered the underlying cause was that the first 32 VSX registers are not
stored in host endian order.

This is something that Richard and I had discussed before, but missed that with
VSX if you have source registers from different register sets then even logical
operations will give you the wrong result.

Rather than revert 7b8fe477e1 "target/ppc: convert VSX logical operations to
vector operations" let's keep the use of the accelerated vector instructions,
and instead fix the real problem which is to switch the first 32 VSX registers
to host endian order matching the VMX registers.

Patches 1-5 aim to consolidate the offset calculations for both CPUPPCState
and the associated _ptr() functions into one single place.

With this preliminary work complete, patch 6 switches the first 32 registers
into host endian order without too much difficulty.

Finally now that all VSX registers are stored in the same way, the vsr offset
functions and get_cpu_vsrh()/get_cpu_vsrl() can be simplified accordingly.

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

v2:
- Rebase onto master
- Rework patchset set based upon av64_offset()/vsr64_offset() as suggested by
  Richard, rather than using separate low/high accessors


Mark Cave-Ayland (7):
  target/ppc: introduce single fpr_offset() function
  target/ppc: introduce single vsrl_offset() function
  target/ppc: move Vsr* macros from internal.h to cpu.h
  target/ppc: introduce avr_full_offset() function
  target/ppc: improve avr64_offset() and use it to simplify
    get_avr64()/set_avr64()
  target/ppc: switch fpr/vsrl registers so all VSX registers are in host
    endian order
  target/ppc: introduce vsr64_offset() to simplify get_cpu_vsr{l,h}()
    and set_cpu_vsr{l,h}()

 target/ppc/cpu.h                    | 51 ++++++++++++++++++++++++++++++++++---
 target/ppc/internal.h               | 27 +++-----------------
 target/ppc/machine.c                |  8 +++---
 target/ppc/translate.c              | 20 +++------------
 target/ppc/translate/vmx-impl.inc.c | 27 ++++++++------------
 target/ppc/translate/vsx-impl.inc.c | 39 +++-------------------------
 6 files changed, 75 insertions(+), 97 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/7] target/ppc: introduce single fpr_offset() function

[Mark Cave-Ayland]

Instead of having multiple copies of the offset calculation logic, move it to a
single fpr_offset() function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/cpu.h       | 7 ++++++-
 target/ppc/translate.c | 4 ++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 26604ddf98..4bb4e42670 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2563,9 +2563,14 @@ static inline bool lsw_reg_in_range(int start, int nregs, int rx)
 }
 
 /* Accessors for FP, VMX and VSX registers */
+static inline int fpr_offset(int i)
+{
+    return offsetof(CPUPPCState, vsr[i].u64[0]);
+}
+
 static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
 {
-    return &env->vsr[i].u64[0];
+    return (uint64_t *)((uintptr_t)env + fpr_offset(i));
 }
 
 static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 819221f246..3b1992faf1 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6677,12 +6677,12 @@ GEN_TM_PRIV_NOOP(trechkpt);
 
 static inline void get_fpr(TCGv_i64 dst, int regno)
 {
-    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState, vsr[regno].u64[0]));
+    tcg_gen_ld_i64(dst, cpu_env, fpr_offset(regno));
 }
 
 static inline void set_fpr(int regno, TCGv_i64 src)
 {
-    tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState, vsr[regno].u64[0]));
+    tcg_gen_st_i64(src, cpu_env, fpr_offset(regno));
 }
 
 static inline void get_avr64(TCGv_i64 dst, int regno, bool high)
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 1/7] target/ppc: introduce single fpr_offset() function

[David Gibson]

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

On Thu, Mar 07, 2019 at 06:05:14PM +0000, Mark Cave-Ayland wrote:
> Instead of having multiple copies of the offset calculation logic, move it to a
> single fpr_offset() function.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Applied, thanks.

> ---
>  target/ppc/cpu.h       | 7 ++++++-
>  target/ppc/translate.c | 4 ++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 26604ddf98..4bb4e42670 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2563,9 +2563,14 @@ static inline bool lsw_reg_in_range(int start, int nregs, int rx)
>  }
>  
>  /* Accessors for FP, VMX and VSX registers */
> +static inline int fpr_offset(int i)
> +{
> +    return offsetof(CPUPPCState, vsr[i].u64[0]);
> +}
> +
>  static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
>  {
> -    return &env->vsr[i].u64[0];
> +    return (uint64_t *)((uintptr_t)env + fpr_offset(i));
>  }
>  
>  static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 819221f246..3b1992faf1 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -6677,12 +6677,12 @@ GEN_TM_PRIV_NOOP(trechkpt);
>  
>  static inline void get_fpr(TCGv_i64 dst, int regno)
>  {
> -    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState, vsr[regno].u64[0]));
> +    tcg_gen_ld_i64(dst, cpu_env, fpr_offset(regno));
>  }
>  
>  static inline void set_fpr(int regno, TCGv_i64 src)
>  {
> -    tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState, vsr[regno].u64[0]));
> +    tcg_gen_st_i64(src, cpu_env, fpr_offset(regno));
>  }
>  
>  static inline void get_avr64(TCGv_i64 dst, int regno, bool high)

-- 
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 2/7] target/ppc: introduce single vsrl_offset() function

[Mark Cave-Ayland]

Instead of having multiple copies of the offset calculation logic, move it to a
single vsrl_offset() function.

This commit also renames the existing get_vsr()/set_vsr() functions to
get_vsrl()/set_vsrl() which better describes their purpose.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/cpu.h                    |  7 ++++++-
 target/ppc/translate/vsx-impl.inc.c | 12 ++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 4bb4e42670..4a7df13c2d 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2573,9 +2573,14 @@ static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
     return (uint64_t *)((uintptr_t)env + fpr_offset(i));
 }
 
+static inline int vsrl_offset(int i)
+{
+    return offsetof(CPUPPCState, vsr[i].u64[1]);
+}
+
 static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
 {
-    return &env->vsr[i].u64[1];
+    return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
 }
 
 static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index e73197e717..381ae0f2e9 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -1,13 +1,13 @@
 /***                           VSX extension                               ***/
 
-static inline void get_vsr(TCGv_i64 dst, int n)
+static inline void get_vsrl(TCGv_i64 dst, int n)
 {
-    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState, vsr[n].u64[1]));
+    tcg_gen_ld_i64(dst, cpu_env, vsrl_offset(n));
 }
 
-static inline void set_vsr(int n, TCGv_i64 src)
+static inline void set_vsrl(int n, TCGv_i64 src)
 {
-    tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState, vsr[n].u64[1]));
+    tcg_gen_st_i64(src, cpu_env, vsrl_offset(n));
 }
 
 static inline int vsr_full_offset(int n)
@@ -27,7 +27,7 @@ static inline void get_cpu_vsrh(TCGv_i64 dst, int n)
 static inline void get_cpu_vsrl(TCGv_i64 dst, int n)
 {
     if (n < 32) {
-        get_vsr(dst, n);
+        get_vsrl(dst, n);
     } else {
         get_avr64(dst, n - 32, false);
     }
@@ -45,7 +45,7 @@ static inline void set_cpu_vsrh(int n, TCGv_i64 src)
 static inline void set_cpu_vsrl(int n, TCGv_i64 src)
 {
     if (n < 32) {
-        set_vsr(n, src);
+        set_vsrl(n, src);
     } else {
         set_avr64(n - 32, src, false);
     }
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 2/7] target/ppc: introduce single vsrl_offset() function

[David Gibson]

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

On Thu, Mar 07, 2019 at 06:05:15PM +0000, Mark Cave-Ayland wrote:
> Instead of having multiple copies of the offset calculation logic, move it to a
> single vsrl_offset() function.
> 
> This commit also renames the existing get_vsr()/set_vsr() functions to
> get_vsrl()/set_vsrl() which better describes their purpose.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Applied, thanks.

> ---
>  target/ppc/cpu.h                    |  7 ++++++-
>  target/ppc/translate/vsx-impl.inc.c | 12 ++++++------
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 4bb4e42670..4a7df13c2d 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2573,9 +2573,14 @@ static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
>      return (uint64_t *)((uintptr_t)env + fpr_offset(i));
>  }
>  
> +static inline int vsrl_offset(int i)
> +{
> +    return offsetof(CPUPPCState, vsr[i].u64[1]);
> +}
> +
>  static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
>  {
> -    return &env->vsr[i].u64[1];
> +    return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
>  }
>  
>  static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index e73197e717..381ae0f2e9 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -1,13 +1,13 @@
>  /***                           VSX extension                               ***/
>  
> -static inline void get_vsr(TCGv_i64 dst, int n)
> +static inline void get_vsrl(TCGv_i64 dst, int n)
>  {
> -    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState, vsr[n].u64[1]));
> +    tcg_gen_ld_i64(dst, cpu_env, vsrl_offset(n));
>  }
>  
> -static inline void set_vsr(int n, TCGv_i64 src)
> +static inline void set_vsrl(int n, TCGv_i64 src)
>  {
> -    tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState, vsr[n].u64[1]));
> +    tcg_gen_st_i64(src, cpu_env, vsrl_offset(n));
>  }
>  
>  static inline int vsr_full_offset(int n)
> @@ -27,7 +27,7 @@ static inline void get_cpu_vsrh(TCGv_i64 dst, int n)
>  static inline void get_cpu_vsrl(TCGv_i64 dst, int n)
>  {
>      if (n < 32) {
> -        get_vsr(dst, n);
> +        get_vsrl(dst, n);
>      } else {
>          get_avr64(dst, n - 32, false);
>      }
> @@ -45,7 +45,7 @@ static inline void set_cpu_vsrh(int n, TCGv_i64 src)
>  static inline void set_cpu_vsrl(int n, TCGv_i64 src)
>  {
>      if (n < 32) {
> -        set_vsr(n, src);
> +        set_vsrl(n, src);
>      } else {
>          set_avr64(n - 32, src, false);
>      }

-- 
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 3/7] target/ppc: move Vsr* macros from internal.h to cpu.h

[Mark Cave-Ayland]

It isn't possible to include internal.h from cpu.h so move the Vsr* macros
into cpu.h alongside the other VMX/VSX register access functions.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/ppc/cpu.h      | 20 ++++++++++++++++++++
 target/ppc/internal.h | 19 -------------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 4a7df13c2d..d0580c6b6d 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2563,6 +2563,26 @@ static inline bool lsw_reg_in_range(int start, int nregs, int rx)
 }
 
 /* Accessors for FP, VMX and VSX registers */
+#if defined(HOST_WORDS_BIGENDIAN)
+#define VsrB(i) u8[i]
+#define VsrSB(i) s8[i]
+#define VsrH(i) u16[i]
+#define VsrSH(i) s16[i]
+#define VsrW(i) u32[i]
+#define VsrSW(i) s32[i]
+#define VsrD(i) u64[i]
+#define VsrSD(i) s64[i]
+#else
+#define VsrB(i) u8[15 - (i)]
+#define VsrSB(i) s8[15 - (i)]
+#define VsrH(i) u16[7 - (i)]
+#define VsrSH(i) s16[7 - (i)]
+#define VsrW(i) u32[3 - (i)]
+#define VsrSW(i) s32[3 - (i)]
+#define VsrD(i) u64[1 - (i)]
+#define VsrSD(i) s64[1 - (i)]
+#endif
+
 static inline int fpr_offset(int i)
 {
     return offsetof(CPUPPCState, vsr[i].u64[0]);
diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index f26a71ffcf..3ebbdf4da4 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -204,25 +204,6 @@ EXTRACT_HELPER(IMM8, 11, 8);
 EXTRACT_HELPER(DCMX, 16, 7);
 EXTRACT_HELPER_SPLIT_3(DCMX_XV, 5, 16, 0, 1, 2, 5, 1, 6, 6);
 
-#if defined(HOST_WORDS_BIGENDIAN)
-#define VsrB(i) u8[i]
-#define VsrSB(i) s8[i]
-#define VsrH(i) u16[i]
-#define VsrSH(i) s16[i]
-#define VsrW(i) u32[i]
-#define VsrSW(i) s32[i]
-#define VsrD(i) u64[i]
-#define VsrSD(i) s64[i]
-#else
-#define VsrB(i) u8[15 - (i)]
-#define VsrSB(i) s8[15 - (i)]
-#define VsrH(i) u16[7 - (i)]
-#define VsrSH(i) s16[7 - (i)]
-#define VsrW(i) u32[3 - (i)]
-#define VsrSW(i) s32[3 - (i)]
-#define VsrD(i) u64[1 - (i)]
-#define VsrSD(i) s64[1 - (i)]
-#endif
 static inline void getVSR(int n, ppc_vsr_t *vsr, CPUPPCState *env)
 {
     vsr->VsrD(0) = env->vsr[n].u64[0];
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 3/7] target/ppc: move Vsr* macros from internal.h to cpu.h

[Richard Henderson]

On 3/7/19 10:05 AM, Mark Cave-Ayland wrote:
> It isn't possible to include internal.h from cpu.h so move the Vsr* macros
> into cpu.h alongside the other VMX/VSX register access functions.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  target/ppc/cpu.h      | 20 ++++++++++++++++++++
>  target/ppc/internal.h | 19 -------------------
>  2 files changed, 20 insertions(+), 19 deletions(-)

Although to some extent I think we should be minimizing the amount
of code within cpu.h, to avoid rebuilding the world.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 3/7] target/ppc: move Vsr* macros from internal.h to cpu.h

[Mark Cave-Ayland]

On 07/03/2019 18:23, Richard Henderson wrote:

> On 3/7/19 10:05 AM, Mark Cave-Ayland wrote:
>> It isn't possible to include internal.h from cpu.h so move the Vsr* macros
>> into cpu.h alongside the other VMX/VSX register access functions.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  target/ppc/cpu.h      | 20 ++++++++++++++++++++
>>  target/ppc/internal.h | 19 -------------------
>>  2 files changed, 20 insertions(+), 19 deletions(-)
> 
> Although to some extent I think we should be minimizing the amount
> of code within cpu.h, to avoid rebuilding the world.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Yeah - I did notice that quite a bit during testing, but when looking at cpu.h after
the final patch was reasonably convinced that these shouldn't need to be changed again...


ATB,

Mark.

Re: [Qemu-devel] [PATCH v2 3/7] target/ppc: move Vsr* macros from internal.h to cpu.h

[David Gibson]

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

On Thu, Mar 07, 2019 at 06:05:16PM +0000, Mark Cave-Ayland wrote:
> It isn't possible to include internal.h from cpu.h so move the Vsr* macros
> into cpu.h alongside the other VMX/VSX register access functions.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied, thanks.

> ---
>  target/ppc/cpu.h      | 20 ++++++++++++++++++++
>  target/ppc/internal.h | 19 -------------------
>  2 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 4a7df13c2d..d0580c6b6d 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2563,6 +2563,26 @@ static inline bool lsw_reg_in_range(int start, int nregs, int rx)
>  }
>  
>  /* Accessors for FP, VMX and VSX registers */
> +#if defined(HOST_WORDS_BIGENDIAN)
> +#define VsrB(i) u8[i]
> +#define VsrSB(i) s8[i]
> +#define VsrH(i) u16[i]
> +#define VsrSH(i) s16[i]
> +#define VsrW(i) u32[i]
> +#define VsrSW(i) s32[i]
> +#define VsrD(i) u64[i]
> +#define VsrSD(i) s64[i]
> +#else
> +#define VsrB(i) u8[15 - (i)]
> +#define VsrSB(i) s8[15 - (i)]
> +#define VsrH(i) u16[7 - (i)]
> +#define VsrSH(i) s16[7 - (i)]
> +#define VsrW(i) u32[3 - (i)]
> +#define VsrSW(i) s32[3 - (i)]
> +#define VsrD(i) u64[1 - (i)]
> +#define VsrSD(i) s64[1 - (i)]
> +#endif
> +
>  static inline int fpr_offset(int i)
>  {
>      return offsetof(CPUPPCState, vsr[i].u64[0]);
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index f26a71ffcf..3ebbdf4da4 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -204,25 +204,6 @@ EXTRACT_HELPER(IMM8, 11, 8);
>  EXTRACT_HELPER(DCMX, 16, 7);
>  EXTRACT_HELPER_SPLIT_3(DCMX_XV, 5, 16, 0, 1, 2, 5, 1, 6, 6);
>  
> -#if defined(HOST_WORDS_BIGENDIAN)
> -#define VsrB(i) u8[i]
> -#define VsrSB(i) s8[i]
> -#define VsrH(i) u16[i]
> -#define VsrSH(i) s16[i]
> -#define VsrW(i) u32[i]
> -#define VsrSW(i) s32[i]
> -#define VsrD(i) u64[i]
> -#define VsrSD(i) s64[i]
> -#else
> -#define VsrB(i) u8[15 - (i)]
> -#define VsrSB(i) s8[15 - (i)]
> -#define VsrH(i) u16[7 - (i)]
> -#define VsrSH(i) s16[7 - (i)]
> -#define VsrW(i) u32[3 - (i)]
> -#define VsrSW(i) s32[3 - (i)]
> -#define VsrD(i) u64[1 - (i)]
> -#define VsrSD(i) s64[1 - (i)]
> -#endif
>  static inline void getVSR(int n, ppc_vsr_t *vsr, CPUPPCState *env)
>  {
>      vsr->VsrD(0) = env->vsr[n].u64[0];

-- 
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 4/7] target/ppc: introduce avr_full_offset() function

[Mark Cave-Ayland]

All TCG vector operations require pointers to the base address of the vector
rather than separate access to the top and bottom 64-bits. Convert the VMX TCG
instructions to use a new avr_full_offset() function instead of avr64_offset()
which can then itself be written as a simple wrapper onto vsr_full_offset().

This same function can also reused in cpu_avr_ptr() to avoid having more than
one copy of the offset calculation logic.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/ppc/cpu.h                    | 12 +++++++++++-
 target/ppc/translate/vmx-impl.inc.c | 22 +++++++++++-----------
 target/ppc/translate/vsx-impl.inc.c |  5 -----
 3 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index d0580c6b6d..2a2792306f 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2598,14 +2598,24 @@ static inline int vsrl_offset(int i)
     return offsetof(CPUPPCState, vsr[i].u64[1]);
 }
 
+static inline int vsr_full_offset(int i)
+{
+    return offsetof(CPUPPCState, vsr[i].u64[0]);
+}
+
 static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
 {
     return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
 }
 
+static inline int avr_full_offset(int i)
+{
+    return vsr_full_offset(i + 32);
+}
+
 static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
 {
-    return &env->vsr[32 + i];
+    return (ppc_avr_t *)((uintptr_t)env + avr_full_offset(i));
 }
 
 void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env);
diff --git a/target/ppc/translate/vmx-impl.inc.c b/target/ppc/translate/vmx-impl.inc.c
index f1b15ae2cb..4e5d0bc0e0 100644
--- a/target/ppc/translate/vmx-impl.inc.c
+++ b/target/ppc/translate/vmx-impl.inc.c
@@ -10,7 +10,7 @@
 static inline TCGv_ptr gen_avr_ptr(int reg)
 {
     TCGv_ptr r = tcg_temp_new_ptr();
-    tcg_gen_addi_ptr(r, cpu_env, offsetof(CPUPPCState, vsr[32 + reg].u64[0]));
+    tcg_gen_addi_ptr(r, cpu_env, avr_full_offset(reg));
     return r;
 }
 
@@ -205,7 +205,7 @@ static void gen_mtvscr(DisasContext *ctx)
     }
 
     val = tcg_temp_new_i32();
-    bofs = avr64_offset(rB(ctx->opcode), true);
+    bofs = avr_full_offset(rB(ctx->opcode));
 #ifdef HOST_WORDS_BIGENDIAN
     bofs += 3 * 4;
 #endif
@@ -284,9 +284,9 @@ static void glue(gen_, name)(DisasContext *ctx)                         \
     }                                                                   \
                                                                         \
     tcg_op(vece,                                                        \
-           avr64_offset(rD(ctx->opcode), true),                         \
-           avr64_offset(rA(ctx->opcode), true),                         \
-           avr64_offset(rB(ctx->opcode), true),                         \
+           avr_full_offset(rD(ctx->opcode)),                            \
+           avr_full_offset(rA(ctx->opcode)),                            \
+           avr_full_offset(rB(ctx->opcode)),                            \
            16, 16);                                                     \
 }
 
@@ -578,10 +578,10 @@ static void glue(gen_, NAME)(DisasContext *ctx)                         \
         gen_exception(ctx, POWERPC_EXCP_VPU);                           \
         return;                                                         \
     }                                                                   \
-    tcg_gen_gvec_4(avr64_offset(rD(ctx->opcode), true),                 \
+    tcg_gen_gvec_4(avr_full_offset(rD(ctx->opcode)),                    \
                    offsetof(CPUPPCState, vscr_sat),                     \
-                   avr64_offset(rA(ctx->opcode), true),                 \
-                   avr64_offset(rB(ctx->opcode), true),                 \
+                   avr_full_offset(rA(ctx->opcode)),                    \
+                   avr_full_offset(rB(ctx->opcode)),                    \
                    16, 16, &g);                                         \
 }
 
@@ -755,7 +755,7 @@ static void glue(gen_, name)(DisasContext *ctx)                         \
             return;                                                     \
         }                                                               \
         simm = SIMM5(ctx->opcode);                                      \
-        tcg_op(avr64_offset(rD(ctx->opcode), true), 16, 16, simm);      \
+        tcg_op(avr_full_offset(rD(ctx->opcode)), 16, 16, simm);         \
     }
 
 GEN_VXFORM_DUPI(vspltisb, tcg_gen_gvec_dup8i, 6, 12);
@@ -850,8 +850,8 @@ static void gen_vsplt(DisasContext *ctx, int vece)
     }
 
     uimm = UIMM5(ctx->opcode);
-    bofs = avr64_offset(rB(ctx->opcode), true);
-    dofs = avr64_offset(rD(ctx->opcode), true);
+    bofs = avr_full_offset(rB(ctx->opcode));
+    dofs = avr_full_offset(rD(ctx->opcode));
 
     /* Experimental testing shows that hardware masks the immediate.  */
     bofs += (uimm << vece) & 15;
diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index 381ae0f2e9..7d02a235e7 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -10,11 +10,6 @@ static inline void set_vsrl(int n, TCGv_i64 src)
     tcg_gen_st_i64(src, cpu_env, vsrl_offset(n));
 }
 
-static inline int vsr_full_offset(int n)
-{
-    return offsetof(CPUPPCState, vsr[n].u64[0]);
-}
-
 static inline void get_cpu_vsrh(TCGv_i64 dst, int n)
 {
     if (n < 32) {
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 4/7] target/ppc: introduce avr_full_offset() function

[Richard Henderson]

On 3/7/19 10:05 AM, Mark Cave-Ayland wrote:
> All TCG vector operations require pointers to the base address of the vector
> rather than separate access to the top and bottom 64-bits. Convert the VMX TCG
> instructions to use a new avr_full_offset() function instead of avr64_offset()
> which can then itself be written as a simple wrapper onto vsr_full_offset().
> 
> This same function can also reused in cpu_avr_ptr() to avoid having more than
> one copy of the offset calculation logic.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  target/ppc/cpu.h                    | 12 +++++++++++-
>  target/ppc/translate/vmx-impl.inc.c | 22 +++++++++++-----------
>  target/ppc/translate/vsx-impl.inc.c |  5 -----
>  3 files changed, 22 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 4/7] target/ppc: introduce avr_full_offset() function

[David Gibson]

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

On Thu, Mar 07, 2019 at 06:05:17PM +0000, Mark Cave-Ayland wrote:
> All TCG vector operations require pointers to the base address of the vector
> rather than separate access to the top and bottom 64-bits. Convert the VMX TCG
> instructions to use a new avr_full_offset() function instead of avr64_offset()
> which can then itself be written as a simple wrapper onto vsr_full_offset().
> 
> This same function can also reused in cpu_avr_ptr() to avoid having more than
> one copy of the offset calculation logic.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied, thanks.

> ---
>  target/ppc/cpu.h                    | 12 +++++++++++-
>  target/ppc/translate/vmx-impl.inc.c | 22 +++++++++++-----------
>  target/ppc/translate/vsx-impl.inc.c |  5 -----
>  3 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index d0580c6b6d..2a2792306f 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2598,14 +2598,24 @@ static inline int vsrl_offset(int i)
>      return offsetof(CPUPPCState, vsr[i].u64[1]);
>  }
>  
> +static inline int vsr_full_offset(int i)
> +{
> +    return offsetof(CPUPPCState, vsr[i].u64[0]);
> +}
> +
>  static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
>  {
>      return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
>  }
>  
> +static inline int avr_full_offset(int i)
> +{
> +    return vsr_full_offset(i + 32);
> +}
> +
>  static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
>  {
> -    return &env->vsr[32 + i];
> +    return (ppc_avr_t *)((uintptr_t)env + avr_full_offset(i));
>  }
>  
>  void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env);
> diff --git a/target/ppc/translate/vmx-impl.inc.c b/target/ppc/translate/vmx-impl.inc.c
> index f1b15ae2cb..4e5d0bc0e0 100644
> --- a/target/ppc/translate/vmx-impl.inc.c
> +++ b/target/ppc/translate/vmx-impl.inc.c
> @@ -10,7 +10,7 @@
>  static inline TCGv_ptr gen_avr_ptr(int reg)
>  {
>      TCGv_ptr r = tcg_temp_new_ptr();
> -    tcg_gen_addi_ptr(r, cpu_env, offsetof(CPUPPCState, vsr[32 + reg].u64[0]));
> +    tcg_gen_addi_ptr(r, cpu_env, avr_full_offset(reg));
>      return r;
>  }
>  
> @@ -205,7 +205,7 @@ static void gen_mtvscr(DisasContext *ctx)
>      }
>  
>      val = tcg_temp_new_i32();
> -    bofs = avr64_offset(rB(ctx->opcode), true);
> +    bofs = avr_full_offset(rB(ctx->opcode));
>  #ifdef HOST_WORDS_BIGENDIAN
>      bofs += 3 * 4;
>  #endif
> @@ -284,9 +284,9 @@ static void glue(gen_, name)(DisasContext *ctx)                         \
>      }                                                                   \
>                                                                          \
>      tcg_op(vece,                                                        \
> -           avr64_offset(rD(ctx->opcode), true),                         \
> -           avr64_offset(rA(ctx->opcode), true),                         \
> -           avr64_offset(rB(ctx->opcode), true),                         \
> +           avr_full_offset(rD(ctx->opcode)),                            \
> +           avr_full_offset(rA(ctx->opcode)),                            \
> +           avr_full_offset(rB(ctx->opcode)),                            \
>             16, 16);                                                     \
>  }
>  
> @@ -578,10 +578,10 @@ static void glue(gen_, NAME)(DisasContext *ctx)                         \
>          gen_exception(ctx, POWERPC_EXCP_VPU);                           \
>          return;                                                         \
>      }                                                                   \
> -    tcg_gen_gvec_4(avr64_offset(rD(ctx->opcode), true),                 \
> +    tcg_gen_gvec_4(avr_full_offset(rD(ctx->opcode)),                    \
>                     offsetof(CPUPPCState, vscr_sat),                     \
> -                   avr64_offset(rA(ctx->opcode), true),                 \
> -                   avr64_offset(rB(ctx->opcode), true),                 \
> +                   avr_full_offset(rA(ctx->opcode)),                    \
> +                   avr_full_offset(rB(ctx->opcode)),                    \
>                     16, 16, &g);                                         \
>  }
>  
> @@ -755,7 +755,7 @@ static void glue(gen_, name)(DisasContext *ctx)                         \
>              return;                                                     \
>          }                                                               \
>          simm = SIMM5(ctx->opcode);                                      \
> -        tcg_op(avr64_offset(rD(ctx->opcode), true), 16, 16, simm);      \
> +        tcg_op(avr_full_offset(rD(ctx->opcode)), 16, 16, simm);         \
>      }
>  
>  GEN_VXFORM_DUPI(vspltisb, tcg_gen_gvec_dup8i, 6, 12);
> @@ -850,8 +850,8 @@ static void gen_vsplt(DisasContext *ctx, int vece)
>      }
>  
>      uimm = UIMM5(ctx->opcode);
> -    bofs = avr64_offset(rB(ctx->opcode), true);
> -    dofs = avr64_offset(rD(ctx->opcode), true);
> +    bofs = avr_full_offset(rB(ctx->opcode));
> +    dofs = avr_full_offset(rD(ctx->opcode));
>  
>      /* Experimental testing shows that hardware masks the immediate.  */
>      bofs += (uimm << vece) & 15;
> diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
> index 381ae0f2e9..7d02a235e7 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -10,11 +10,6 @@ static inline void set_vsrl(int n, TCGv_i64 src)
>      tcg_gen_st_i64(src, cpu_env, vsrl_offset(n));
>  }
>  
> -static inline int vsr_full_offset(int n)
> -{
> -    return offsetof(CPUPPCState, vsr[n].u64[0]);
> -}
> -
>  static inline void get_cpu_vsrh(TCGv_i64 dst, int n)
>  {
>      if (n < 32) {

-- 
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 5/7] target/ppc: improve avr64_offset() and use it to simplify get_avr64()/set_avr64()

[Mark Cave-Ayland]

By using the VsrD macro in avr64_offset() the same offset calculation can be
used regardless of the host endian. This allows get_avr64() and set_avr64() to
be simplified accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/ppc/cpu.h                    |  5 +++++
 target/ppc/translate.c              | 16 ++--------------
 target/ppc/translate/vmx-impl.inc.c |  5 -----
 3 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2a2792306f..aebb6c01ee 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2608,6 +2608,11 @@ static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
     return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
 }
 
+static inline long avr64_offset(int i, bool high)
+{
+    return offsetof(CPUPPCState, vsr[32 + i].VsrD(high ? 0 : 1));
+}
+
 static inline int avr_full_offset(int i)
 {
     return vsr_full_offset(i + 32);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 3b1992faf1..a6cff20daf 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6687,24 +6687,12 @@ static inline void set_fpr(int regno, TCGv_i64 src)
 
 static inline void get_avr64(TCGv_i64 dst, int regno, bool high)
 {
-#ifdef HOST_WORDS_BIGENDIAN
-    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
-                                          vsr[32 + regno].u64[(high ? 0 : 1)]));
-#else
-    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
-                                          vsr[32 + regno].u64[(high ? 1 : 0)]));
-#endif
+    tcg_gen_ld_i64(dst, cpu_env, avr64_offset(regno, high));
 }
 
 static inline void set_avr64(int regno, TCGv_i64 src, bool high)
 {
-#ifdef HOST_WORDS_BIGENDIAN
-    tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState,
-                                          vsr[32 + regno].u64[(high ? 0 : 1)]));
-#else
-    tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState,
-                                          vsr[32 + regno].u64[(high ? 1 : 0)]));
-#endif
+    tcg_gen_st_i64(src, cpu_env, avr64_offset(regno, high));
 }
 
 #include "translate/fp-impl.inc.c"
diff --git a/target/ppc/translate/vmx-impl.inc.c b/target/ppc/translate/vmx-impl.inc.c
index 4e5d0bc0e0..eb10c533ca 100644
--- a/target/ppc/translate/vmx-impl.inc.c
+++ b/target/ppc/translate/vmx-impl.inc.c
@@ -14,11 +14,6 @@ static inline TCGv_ptr gen_avr_ptr(int reg)
     return r;
 }
 
-static inline long avr64_offset(int reg, bool high)
-{
-    return offsetof(CPUPPCState, vsr[32 + reg].u64[(high ? 0 : 1)]);
-}
-
 #define GEN_VR_LDX(name, opc2, opc3)                                          \
 static void glue(gen_, name)(DisasContext *ctx)                                       \
 {                                                                             \
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 5/7] target/ppc: improve avr64_offset() and use it to simplify get_avr64()/set_avr64()

[Richard Henderson]

On 3/7/19 10:05 AM, Mark Cave-Ayland wrote:
> By using the VsrD macro in avr64_offset() the same offset calculation can be
> used regardless of the host endian. This allows get_avr64() and set_avr64() to
> be simplified accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  target/ppc/cpu.h                    |  5 +++++
>  target/ppc/translate.c              | 16 ++--------------
>  target/ppc/translate/vmx-impl.inc.c |  5 -----
>  3 files changed, 7 insertions(+), 19 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v2 6/7] target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order

[Mark Cave-Ayland]

When VSX support was initially added, the fpr registers were added at
offset 0 of the VSR register and the vsrl registers were added at offset
1. This is in contrast to the VMX registers (the last 32 VSX registers) which
are stored in host-endian order.

Switch the fpr/vsrl registers so that the lower 32 VSX registers are now also
stored in host endian order to match the VMX registers. This ensures that TCG
vector operations involving mixed VMX and VSX registers will function
correctly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/cpu.h      | 4 ++--
 target/ppc/internal.h | 8 ++++----
 target/ppc/machine.c  | 8 ++++----
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index aebb6c01ee..704e595014 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2585,7 +2585,7 @@ static inline bool lsw_reg_in_range(int start, int nregs, int rx)
 
 static inline int fpr_offset(int i)
 {
-    return offsetof(CPUPPCState, vsr[i].u64[0]);
+    return offsetof(CPUPPCState, vsr[i].VsrD(0));
 }
 
 static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
@@ -2595,7 +2595,7 @@ static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
 
 static inline int vsrl_offset(int i)
 {
-    return offsetof(CPUPPCState, vsr[i].u64[1]);
+    return offsetof(CPUPPCState, vsr[i].VsrD(1));
 }
 
 static inline int vsr_full_offset(int i)
diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 3ebbdf4da4..fb6f64ed1e 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -206,14 +206,14 @@ EXTRACT_HELPER_SPLIT_3(DCMX_XV, 5, 16, 0, 1, 2, 5, 1, 6, 6);
 
 static inline void getVSR(int n, ppc_vsr_t *vsr, CPUPPCState *env)
 {
-    vsr->VsrD(0) = env->vsr[n].u64[0];
-    vsr->VsrD(1) = env->vsr[n].u64[1];
+    vsr->VsrD(0) = env->vsr[n].VsrD(0);
+    vsr->VsrD(1) = env->vsr[n].VsrD(1);
 }
 
 static inline void putVSR(int n, ppc_vsr_t *vsr, CPUPPCState *env)
 {
-    env->vsr[n].u64[0] = vsr->VsrD(0);
-    env->vsr[n].u64[1] = vsr->VsrD(1);
+    env->vsr[n].VsrD(0) = vsr->VsrD(0);
+    env->vsr[n].VsrD(1) = vsr->VsrD(1);
 }
 
 void helper_compute_fprf_float16(CPUPPCState *env, float16 arg);
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 756b6d2971..a92d0ad3a3 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -150,7 +150,7 @@ static int get_fpr(QEMUFile *f, void *pv, size_t size,
 {
     ppc_vsr_t *v = pv;
 
-    v->u64[0] = qemu_get_be64(f);
+    v->VsrD(0) = qemu_get_be64(f);
 
     return 0;
 }
@@ -160,7 +160,7 @@ static int put_fpr(QEMUFile *f, void *pv, size_t size,
 {
     ppc_vsr_t *v = pv;
 
-    qemu_put_be64(f, v->u64[0]);
+    qemu_put_be64(f, v->VsrD(0));
     return 0;
 }
 
@@ -181,7 +181,7 @@ static int get_vsr(QEMUFile *f, void *pv, size_t size,
 {
     ppc_vsr_t *v = pv;
 
-    v->u64[1] = qemu_get_be64(f);
+    v->VsrD(1) = qemu_get_be64(f);
 
     return 0;
 }
@@ -191,7 +191,7 @@ static int put_vsr(QEMUFile *f, void *pv, size_t size,
 {
     ppc_vsr_t *v = pv;
 
-    qemu_put_be64(f, v->u64[1]);
+    qemu_put_be64(f, v->VsrD(1));
     return 0;
 }
 
-- 
2.11.0

[Qemu-devel] [PATCH v2 7/7] target/ppc: introduce vsr64_offset() to simplify get_cpu_vsr{l, h}() and set_cpu_vsr{l, h}()

[Mark Cave-Ayland]

Now that all VSX registers are stored in host endian order, there is no need
to go via different accessors depending upon the register number. Instead we
introduce vsr64_offset() and use it directly from within get_cpu_vsr{l,h}() and
set_cpu_vsr{l,h}().

This also allows us to rewrite avr64_offset() and fpr_offset() in terms of the
new vsr64_offset() function to more clearly express the relationship between the
VSX, FPR and VMX registers, and also remove vsrl_offset() which is no longer
required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/ppc/cpu.h                    | 20 ++++++++++----------
 target/ppc/translate/vsx-impl.inc.c | 34 ++++------------------------------
 2 files changed, 14 insertions(+), 40 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 704e595014..5754281cfe 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2583,34 +2583,34 @@ static inline bool lsw_reg_in_range(int start, int nregs, int rx)
 #define VsrSD(i) s64[1 - (i)]
 #endif
 
-static inline int fpr_offset(int i)
+static inline int vsr64_offset(int i, bool high)
 {
-    return offsetof(CPUPPCState, vsr[i].VsrD(0));
+    return offsetof(CPUPPCState, vsr[i].VsrD(high ? 0 : 1));
 }
 
-static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
+static inline int vsr_full_offset(int i)
 {
-    return (uint64_t *)((uintptr_t)env + fpr_offset(i));
+    return offsetof(CPUPPCState, vsr[i].u64[0]);
 }
 
-static inline int vsrl_offset(int i)
+static inline int fpr_offset(int i)
 {
-    return offsetof(CPUPPCState, vsr[i].VsrD(1));
+    return vsr64_offset(i, true);
 }
 
-static inline int vsr_full_offset(int i)
+static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
 {
-    return offsetof(CPUPPCState, vsr[i].u64[0]);
+    return (uint64_t *)((uintptr_t)env + fpr_offset(i));
 }
 
 static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
 {
-    return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
+    return (uint64_t *)((uintptr_t)env + vsr64_offset(i, false));
 }
 
 static inline long avr64_offset(int i, bool high)
 {
-    return offsetof(CPUPPCState, vsr[32 + i].VsrD(high ? 0 : 1));
+    return vsr64_offset(i + 32, high);
 }
 
 static inline int avr_full_offset(int i)
diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c
index 7d02a235e7..95a269fff0 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -1,49 +1,23 @@
 /***                           VSX extension                               ***/
 
-static inline void get_vsrl(TCGv_i64 dst, int n)
-{
-    tcg_gen_ld_i64(dst, cpu_env, vsrl_offset(n));
-}
-
-static inline void set_vsrl(int n, TCGv_i64 src)
-{
-    tcg_gen_st_i64(src, cpu_env, vsrl_offset(n));
-}
-
 static inline void get_cpu_vsrh(TCGv_i64 dst, int n)
 {
-    if (n < 32) {
-        get_fpr(dst, n);
-    } else {
-        get_avr64(dst, n - 32, true);
-    }
+    tcg_gen_ld_i64(dst, cpu_env, vsr64_offset(n, true));
 }
 
 static inline void get_cpu_vsrl(TCGv_i64 dst, int n)
 {
-    if (n < 32) {
-        get_vsrl(dst, n);
-    } else {
-        get_avr64(dst, n - 32, false);
-    }
+    tcg_gen_ld_i64(dst, cpu_env, vsr64_offset(n, false));
 }
 
 static inline void set_cpu_vsrh(int n, TCGv_i64 src)
 {
-    if (n < 32) {
-        set_fpr(n, src);
-    } else {
-        set_avr64(n - 32, src, true);
-    }
+    tcg_gen_st_i64(src, cpu_env, vsr64_offset(n, true));
 }
 
 static inline void set_cpu_vsrl(int n, TCGv_i64 src)
 {
-    if (n < 32) {
-        set_vsrl(n, src);
-    } else {
-        set_avr64(n - 32, src, false);
-    }
+    tcg_gen_st_i64(src, cpu_env, vsr64_offset(n, false));
 }
 
 #define VSX_LOAD_SCALAR(name, operation)                      \
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 7/7] target/ppc: introduce vsr64_offset() to simplify get_cpu_vsr{l, h}() and set_cpu_vsr{l, h}()

[Richard Henderson]

On 3/7/19 10:05 AM, Mark Cave-Ayland wrote:
> Now that all VSX registers are stored in host endian order, there is no need
> to go via different accessors depending upon the register number. Instead we
> introduce vsr64_offset() and use it directly from within get_cpu_vsr{l,h}() and
> set_cpu_vsr{l,h}().
> 
> This also allows us to rewrite avr64_offset() and fpr_offset() in terms of the
> new vsr64_offset() function to more clearly express the relationship between the
> VSX, FPR and VMX registers, and also remove vsrl_offset() which is no longer
> required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  target/ppc/cpu.h                    | 20 ++++++++++----------
>  target/ppc/translate/vsx-impl.inc.c | 34 ++++------------------------------
>  2 files changed, 14 insertions(+), 40 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 0/7] target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order

[Richard Henderson]

On 3/7/19 10:05 AM, Mark Cave-Ayland wrote:
> Finally now that all VSX registers are stored in the same way, the vsr offset
> functions and get_cpu_vsrh()/get_cpu_vsrl() can be simplified accordingly.

For the todo list for 4.1 should be getting rid of getVSR and setVSR.


r`

Re: [Qemu-devel] [PATCH v2 0/7] target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order

[Mark Cave-Ayland]

On 07/03/2019 21:00, Richard Henderson wrote:

> On 3/7/19 10:05 AM, Mark Cave-Ayland wrote:
>> Finally now that all VSX registers are stored in the same way, the vsr offset
>> functions and get_cpu_vsrh()/get_cpu_vsrl() can be simplified accordingly.
> 
> For the todo list for 4.1 should be getting rid of getVSR and setVSR.

That's something that certainly crossed my mind, although I was conscious that freeze
was coming up and wanted to get the fix for the existing VSX vector ops merged. I
think it should be a simple mechanical change, but I'm not real set up right now for
much in the way of ppc64 testing.


ATB,

Mark.

Re: [Qemu-devel] [PATCH v2 0/7] target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order

[David Gibson]

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

On Thu, Mar 07, 2019 at 06:05:13PM +0000, Mark Cave-Ayland wrote:
> After some investigation into Andrew's report of corruption in his ppc64le
> tests at https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07234.html, I
> discovered the underlying cause was that the first 32 VSX registers are not
> stored in host endian order.
> 
> This is something that Richard and I had discussed before, but missed that with
> VSX if you have source registers from different register sets then even logical
> operations will give you the wrong result.
> 
> Rather than revert 7b8fe477e1 "target/ppc: convert VSX logical operations to
> vector operations" let's keep the use of the accelerated vector instructions,
> and instead fix the real problem which is to switch the first 32 VSX registers
> to host endian order matching the VMX registers.
> 
> Patches 1-5 aim to consolidate the offset calculations for both CPUPPCState
> and the associated _ptr() functions into one single place.
> 
> With this preliminary work complete, patch 6 switches the first 32 registers
> into host endian order without too much difficulty.
> 
> Finally now that all VSX registers are stored in the same way, the vsr offset
> functions and get_cpu_vsrh()/get_cpu_vsrl() can be simplified accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Series applied to ppc-for-4.0, thanks.

> 
> v2:
> - Rebase onto master
> - Rework patchset set based upon av64_offset()/vsr64_offset() as suggested by
>   Richard, rather than using separate low/high accessors
> 
> 
> Mark Cave-Ayland (7):
>   target/ppc: introduce single fpr_offset() function
>   target/ppc: introduce single vsrl_offset() function
>   target/ppc: move Vsr* macros from internal.h to cpu.h
>   target/ppc: introduce avr_full_offset() function
>   target/ppc: improve avr64_offset() and use it to simplify
>     get_avr64()/set_avr64()
>   target/ppc: switch fpr/vsrl registers so all VSX registers are in host
>     endian order
>   target/ppc: introduce vsr64_offset() to simplify get_cpu_vsr{l,h}()
>     and set_cpu_vsr{l,h}()
> 
>  target/ppc/cpu.h                    | 51 ++++++++++++++++++++++++++++++++++---
>  target/ppc/internal.h               | 27 +++-----------------
>  target/ppc/machine.c                |  8 +++---
>  target/ppc/translate.c              | 20 +++------------
>  target/ppc/translate/vmx-impl.inc.c | 27 ++++++++------------
>  target/ppc/translate/vsx-impl.inc.c | 39 +++-------------------------
>  6 files changed, 75 insertions(+), 97 deletions(-)
> 

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