[PATCH 0/4] Fix incorrect accesses to XER

[PATCH 0/4] Fix incorrect accesses to XER

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

This patch series fixes some uses of env->xer that were left behind when
SO, OV, and CA bits were separated into other fields of CPUPPCState. At
the time, all of them weren't in target-specific files/directories,
which probably made it harder to track what needed to be changed.
Currently, this still is the case for linux-user/elfload.c. Is there
any particular reason for that? I can try to provide a patch to split
it.

Matheus Ferst (4):
  linux-user/ppc: Fix XER access in save/restore_user_regs
  target/ppc: Fix XER access in gdbstub
  linux-user: Fix XER access in ppc version of elf_core_copy_regs
  target/ppc: Fix XER access in monitor

 linux-user/elfload.c                        |  2 +-
 linux-user/ppc/signal.c                     |  9 +++--
 target/ppc/cpu.c                            |  2 +-
 target/ppc/cpu.h                            |  2 +-
 target/ppc/gdbstub.c                        |  8 ++--
 target/ppc/monitor.c                        |  9 ++++-
 tests/tcg/ppc64/Makefile.target             |  2 +
 tests/tcg/ppc64le/Makefile.target           |  2 +
 tests/tcg/ppc64le/signal_save_restore_xer.c | 42 +++++++++++++++++++++
 9 files changed, 67 insertions(+), 11 deletions(-)
 create mode 100644 tests/tcg/ppc64le/signal_save_restore_xer.c

-- 
2.25.1

[PATCH 1/4] linux-user/ppc: Fix XER access in save/restore_user_regs

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

We should use cpu_read_xer/cpu_write_xer to save/restore the complete
register since some of its bits are in other fields of CPUPPCState. A
test is added to prevent future regressions.

Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 linux-user/ppc/signal.c                     |  9 +++--
 tests/tcg/ppc64/Makefile.target             |  2 +
 tests/tcg/ppc64le/Makefile.target           |  2 +
 tests/tcg/ppc64le/signal_save_restore_xer.c | 42 +++++++++++++++++++++
 4 files changed, 52 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/ppc64le/signal_save_restore_xer.c

diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
index edfad28a37..4413a4a28f 100644
--- a/linux-user/ppc/signal.c
+++ b/linux-user/ppc/signal.c
@@ -244,7 +244,7 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
     __put_user(env->nip, &frame->mc_gregs[TARGET_PT_NIP]);
     __put_user(env->ctr, &frame->mc_gregs[TARGET_PT_CTR]);
     __put_user(env->lr, &frame->mc_gregs[TARGET_PT_LNK]);
-    __put_user(env->xer, &frame->mc_gregs[TARGET_PT_XER]);
+    __put_user(cpu_read_xer(env), &frame->mc_gregs[TARGET_PT_XER]);
 
     for (i = 0; i < ARRAY_SIZE(env->crf); i++) {
         ccr |= env->crf[i] << (32 - ((i + 1) * 4));
@@ -319,6 +319,7 @@ static void restore_user_regs(CPUPPCState *env,
 {
     target_ulong save_r2 = 0;
     target_ulong msr;
+    target_ulong xer;
     target_ulong ccr;
 
     int i;
@@ -334,9 +335,11 @@ static void restore_user_regs(CPUPPCState *env,
     __get_user(env->nip, &frame->mc_gregs[TARGET_PT_NIP]);
     __get_user(env->ctr, &frame->mc_gregs[TARGET_PT_CTR]);
     __get_user(env->lr, &frame->mc_gregs[TARGET_PT_LNK]);
-    __get_user(env->xer, &frame->mc_gregs[TARGET_PT_XER]);
+
+    __get_user(xer, &frame->mc_gregs[TARGET_PT_XER]);
+    cpu_write_xer(env, xer);
+
     __get_user(ccr, &frame->mc_gregs[TARGET_PT_CCR]);
-
     for (i = 0; i < ARRAY_SIZE(env->crf); i++) {
         env->crf[i] = (ccr >> (32 - ((i + 1) * 4))) & 0xf;
     }
diff --git a/tests/tcg/ppc64/Makefile.target b/tests/tcg/ppc64/Makefile.target
index a6a4ddaeca..6ab7934fdf 100644
--- a/tests/tcg/ppc64/Makefile.target
+++ b/tests/tcg/ppc64/Makefile.target
@@ -23,4 +23,6 @@ run-plugin-byte_reverse-with-%:
 	$(call skip-test, "RUN of byte_reverse ($*)", "not built")
 endif
 
+PPC64_TESTS += signal_save_restore_xer
+
 TESTS += $(PPC64_TESTS)
diff --git a/tests/tcg/ppc64le/Makefile.target b/tests/tcg/ppc64le/Makefile.target
index c0c14ffbad..5e65b1590d 100644
--- a/tests/tcg/ppc64le/Makefile.target
+++ b/tests/tcg/ppc64le/Makefile.target
@@ -22,4 +22,6 @@ run-plugin-byte_reverse-with-%:
 	$(call skip-test, "RUN of byte_reverse ($*)", "not built")
 endif
 
+PPC64LE_TESTS += signal_save_restore_xer
+
 TESTS += $(PPC64LE_TESTS)
diff --git a/tests/tcg/ppc64le/signal_save_restore_xer.c b/tests/tcg/ppc64le/signal_save_restore_xer.c
new file mode 100644
index 0000000000..e4f8a07dd7
--- /dev/null
+++ b/tests/tcg/ppc64le/signal_save_restore_xer.c
@@ -0,0 +1,42 @@
+#include <assert.h>
+#include <stdint.h>
+#include <signal.h>
+#include <sys/user.h>
+
+#define XER_SO   (1 << 31)
+#define XER_OV   (1 << 30)
+#define XER_CA   (1 << 29)
+#define XER_OV32 (1 << 19)
+#define XER_CA32 (1 << 18)
+
+uint64_t saved;
+
+void sigill_handler(int sig, siginfo_t *si, void *ucontext)
+{
+    ucontext_t *uc = ucontext;
+    uc->uc_mcontext.regs->nip += 4;
+    saved = uc->uc_mcontext.regs->xer;
+    uc->uc_mcontext.regs->xer |= XER_OV | XER_OV32;
+}
+
+int main(void)
+{
+    uint64_t initial = XER_CA | XER_CA32, restored;
+    struct sigaction sa = {
+        .sa_sigaction = sigill_handler,
+        .sa_flags = SA_SIGINFO
+    };
+
+    sigaction(SIGILL, &sa, NULL);
+
+    asm("mtspr 1, %1\n\t"
+        ".long 0x0\n\t"
+        "mfspr %0, 1\n\t"
+        : "=r" (restored)
+        : "r" (initial));
+
+    assert(saved == initial);
+    assert(restored == (XER_OV | XER_OV32 | XER_CA | XER_CA32));
+
+    return 0;
+}
-- 
2.25.1

[PATCH 2/4] target/ppc: Fix XER access in gdbstub

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

The value of XER is split in multiple fields of CPUPPCState, like
env->xer and env->so. To get/set the whole register from gdb, we should
use cpu_read_xer/cpu_write_xer.

Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 target/ppc/gdbstub.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 1808a150e4..105c2f7dd1 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -159,7 +159,7 @@ int ppc_cpu_gdb_read_register(CPUState *cs, GByteArray *buf, int n)
             gdb_get_regl(buf, env->ctr);
             break;
         case 69:
-            gdb_get_reg32(buf, env->xer);
+            gdb_get_reg32(buf, cpu_read_xer(env));
             break;
         case 70:
             gdb_get_reg32(buf, env->fpscr);
@@ -217,7 +217,7 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, GByteArray *buf, int n)
             gdb_get_reg64(buf, env->ctr);
             break;
         case 69 + 32:
-            gdb_get_reg32(buf, env->xer);
+            gdb_get_reg32(buf, cpu_read_xer(env));
             break;
         case 70 + 32:
             gdb_get_reg64(buf, env->fpscr);
@@ -269,7 +269,7 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
             env->ctr = ldtul_p(mem_buf);
             break;
         case 69:
-            env->xer = ldl_p(mem_buf);
+            cpu_write_xer(env, ldl_p(mem_buf));
             break;
         case 70:
             /* fpscr */
@@ -319,7 +319,7 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
             env->ctr = ldq_p(mem_buf);
             break;
         case 69 + 32:
-            env->xer = ldl_p(mem_buf);
+            cpu_write_xer(env, ldl_p(mem_buf));
             break;
         case 70 + 32:
             /* fpscr */
-- 
2.25.1

[PATCH 3/4] linux-user: Fix XER access in ppc version of elf_core_copy_regs

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

env->xer doesn't hold some bits of XER, like OV and CA. To write the
complete register in the core dump we should read XER value with
cpu_read_xer.

Reported-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 linux-user/elfload.c | 2 +-
 target/ppc/cpu.c     | 2 +-
 target/ppc/cpu.h     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 01e9a833fb..e760786a98 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -896,7 +896,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUPPCState *en
     (*regs)[33] = tswapreg(env->msr);
     (*regs)[35] = tswapreg(env->ctr);
     (*regs)[36] = tswapreg(env->lr);
-    (*regs)[37] = tswapreg(env->xer);
+    (*regs)[37] = tswapreg(cpu_read_xer(env));
 
     for (i = 0; i < ARRAY_SIZE(env->crf); i++) {
         ccr |= env->crf[i] << (32 - ((i + 1) * 4));
diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index 7ad9bd6044..f933d9f2bd 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -27,7 +27,7 @@
 #include "helper_regs.h"
 #include "sysemu/tcg.h"
 
-target_ulong cpu_read_xer(CPUPPCState *env)
+target_ulong cpu_read_xer(const CPUPPCState *env)
 {
     if (is_isa300(env)) {
         return env->xer | (env->so << XER_SO) |
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 500205229c..67a143941f 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2418,7 +2418,7 @@ enum {
 /*****************************************************************************/
 
 #define is_isa300(ctx) (!!(ctx->insns_flags2 & PPC2_ISA300))
-target_ulong cpu_read_xer(CPUPPCState *env);
+target_ulong cpu_read_xer(const CPUPPCState *env);
 void cpu_write_xer(CPUPPCState *env, target_ulong xer);
 
 /*
-- 
2.25.1

[PATCH 4/4] target/ppc: Fix XER access in monitor

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

We can't read env->xer directly, as it does not contain some bits of
XER. Instead, we should have a callback that uses cpu_read_xer to read
the complete register.

Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 target/ppc/monitor.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
index a475108b2d..0b805ef6e9 100644
--- a/target/ppc/monitor.c
+++ b/target/ppc/monitor.c
@@ -44,6 +44,13 @@ static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
     return u;
 }
 
+static target_long monitor_get_xer(Monitor *mon, const struct MonitorDef *md,
+                                   int val)
+{
+    CPUArchState *env = mon_get_cpu_env(mon);
+    return cpu_read_xer(env);
+}
+
 static target_long monitor_get_decr(Monitor *mon, const struct MonitorDef *md,
                                     int val)
 {
@@ -85,7 +92,7 @@ const MonitorDef monitor_defs[] = {
     { "decr", 0, &monitor_get_decr, },
     { "ccr|cr", 0, &monitor_get_ccr, },
     /* Machine state register */
-    { "xer", offsetof(CPUPPCState, xer) },
+    { "xer", 0, &monitor_get_xer },
     { "msr", offsetof(CPUPPCState, msr) },
     { "tbu", 0, &monitor_get_tbu, },
     { "tbl", 0, &monitor_get_tbl, },
-- 
2.25.1

Re: [PATCH 1/4] linux-user/ppc: Fix XER access in save/restore_user_regs

[Richard Henderson]

On 10/14/21 3:32 PM, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> We should use cpu_read_xer/cpu_write_xer to save/restore the complete
> register since some of its bits are in other fields of CPUPPCState. A
> test is added to prevent future regressions.
> 
> Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
>   linux-user/ppc/signal.c                     |  9 +++--
>   tests/tcg/ppc64/Makefile.target             |  2 +
>   tests/tcg/ppc64le/Makefile.target           |  2 +
>   tests/tcg/ppc64le/signal_save_restore_xer.c | 42 +++++++++++++++++++++
>   4 files changed, 52 insertions(+), 3 deletions(-)
>   create mode 100644 tests/tcg/ppc64le/signal_save_restore_xer.c

The code is good so,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> +    sigaction(SIGILL, &sa, NULL);
> +
> +    asm("mtspr 1, %1\n\t"
> +        ".long 0x0\n\t"

While Appendix B does guarantee that "0" is and always will be an invalid instruction, I 
wonder if the test itself would be clearer (i.e. self-documenting the intent) using 
SIGTRAP and "trap".


r~

Re: [PATCH 2/4] target/ppc: Fix XER access in gdbstub

[Richard Henderson]

On 10/14/21 3:32 PM, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst<matheus.ferst@eldorado.org.br>
> 
> The value of XER is split in multiple fields of CPUPPCState, like
> env->xer and env->so. To get/set the whole register from gdb, we should
> use cpu_read_xer/cpu_write_xer.
> 
> Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
> Signed-off-by: Matheus Ferst<matheus.ferst@eldorado.org.br>
> ---
>   target/ppc/gdbstub.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

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

r~

Re: [PATCH 3/4] linux-user: Fix XER access in ppc version of elf_core_copy_regs

[Richard Henderson]

On 10/14/21 3:32 PM, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst<matheus.ferst@eldorado.org.br>
> 
> env->xer doesn't hold some bits of XER, like OV and CA. To write the
> complete register in the core dump we should read XER value with
> cpu_read_xer.
> 
> Reported-by: Lucas Mateus Castro (alqotel)<lucas.araujo@eldorado.org.br>
> Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
> Signed-off-by: Matheus Ferst<matheus.ferst@eldorado.org.br>
> ---
>   linux-user/elfload.c | 2 +-
>   target/ppc/cpu.c     | 2 +-
>   target/ppc/cpu.h     | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)

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

r~

Re: [PATCH 4/4] target/ppc: Fix XER access in monitor

[Richard Henderson]

On 10/14/21 3:32 PM, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst<matheus.ferst@eldorado.org.br>
> 
> We can't read env->xer directly, as it does not contain some bits of
> XER. Instead, we should have a callback that uses cpu_read_xer to read
> the complete register.
> 
> Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
> Signed-off-by: Matheus Ferst<matheus.ferst@eldorado.org.br>
> ---
>   target/ppc/monitor.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

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

r~

Re: [PATCH 1/4] linux-user/ppc: Fix XER access in save/restore_user_regs

[David Gibson]

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

On Thu, Oct 14, 2021 at 07:32:31PM -0300, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> We should use cpu_read_xer/cpu_write_xer to save/restore the complete
> register since some of its bits are in other fields of CPUPPCState. A
> test is added to prevent future regressions.
> 
> Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

Applied to ppc-for-6.2, thanks.

> ---
>  linux-user/ppc/signal.c                     |  9 +++--
>  tests/tcg/ppc64/Makefile.target             |  2 +
>  tests/tcg/ppc64le/Makefile.target           |  2 +
>  tests/tcg/ppc64le/signal_save_restore_xer.c | 42 +++++++++++++++++++++
>  4 files changed, 52 insertions(+), 3 deletions(-)
>  create mode 100644 tests/tcg/ppc64le/signal_save_restore_xer.c
> 
> diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> index edfad28a37..4413a4a28f 100644
> --- a/linux-user/ppc/signal.c
> +++ b/linux-user/ppc/signal.c
> @@ -244,7 +244,7 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
>      __put_user(env->nip, &frame->mc_gregs[TARGET_PT_NIP]);
>      __put_user(env->ctr, &frame->mc_gregs[TARGET_PT_CTR]);
>      __put_user(env->lr, &frame->mc_gregs[TARGET_PT_LNK]);
> -    __put_user(env->xer, &frame->mc_gregs[TARGET_PT_XER]);
> +    __put_user(cpu_read_xer(env), &frame->mc_gregs[TARGET_PT_XER]);
>  
>      for (i = 0; i < ARRAY_SIZE(env->crf); i++) {
>          ccr |= env->crf[i] << (32 - ((i + 1) * 4));
> @@ -319,6 +319,7 @@ static void restore_user_regs(CPUPPCState *env,
>  {
>      target_ulong save_r2 = 0;
>      target_ulong msr;
> +    target_ulong xer;
>      target_ulong ccr;
>  
>      int i;
> @@ -334,9 +335,11 @@ static void restore_user_regs(CPUPPCState *env,
>      __get_user(env->nip, &frame->mc_gregs[TARGET_PT_NIP]);
>      __get_user(env->ctr, &frame->mc_gregs[TARGET_PT_CTR]);
>      __get_user(env->lr, &frame->mc_gregs[TARGET_PT_LNK]);
> -    __get_user(env->xer, &frame->mc_gregs[TARGET_PT_XER]);
> +
> +    __get_user(xer, &frame->mc_gregs[TARGET_PT_XER]);
> +    cpu_write_xer(env, xer);
> +
>      __get_user(ccr, &frame->mc_gregs[TARGET_PT_CCR]);
> -
>      for (i = 0; i < ARRAY_SIZE(env->crf); i++) {
>          env->crf[i] = (ccr >> (32 - ((i + 1) * 4))) & 0xf;
>      }
> diff --git a/tests/tcg/ppc64/Makefile.target b/tests/tcg/ppc64/Makefile.target
> index a6a4ddaeca..6ab7934fdf 100644
> --- a/tests/tcg/ppc64/Makefile.target
> +++ b/tests/tcg/ppc64/Makefile.target
> @@ -23,4 +23,6 @@ run-plugin-byte_reverse-with-%:
>  	$(call skip-test, "RUN of byte_reverse ($*)", "not built")
>  endif
>  
> +PPC64_TESTS += signal_save_restore_xer
> +
>  TESTS += $(PPC64_TESTS)
> diff --git a/tests/tcg/ppc64le/Makefile.target b/tests/tcg/ppc64le/Makefile.target
> index c0c14ffbad..5e65b1590d 100644
> --- a/tests/tcg/ppc64le/Makefile.target
> +++ b/tests/tcg/ppc64le/Makefile.target
> @@ -22,4 +22,6 @@ run-plugin-byte_reverse-with-%:
>  	$(call skip-test, "RUN of byte_reverse ($*)", "not built")
>  endif
>  
> +PPC64LE_TESTS += signal_save_restore_xer
> +
>  TESTS += $(PPC64LE_TESTS)
> diff --git a/tests/tcg/ppc64le/signal_save_restore_xer.c b/tests/tcg/ppc64le/signal_save_restore_xer.c
> new file mode 100644
> index 0000000000..e4f8a07dd7
> --- /dev/null
> +++ b/tests/tcg/ppc64le/signal_save_restore_xer.c
> @@ -0,0 +1,42 @@
> +#include <assert.h>
> +#include <stdint.h>
> +#include <signal.h>
> +#include <sys/user.h>
> +
> +#define XER_SO   (1 << 31)
> +#define XER_OV   (1 << 30)
> +#define XER_CA   (1 << 29)
> +#define XER_OV32 (1 << 19)
> +#define XER_CA32 (1 << 18)
> +
> +uint64_t saved;
> +
> +void sigill_handler(int sig, siginfo_t *si, void *ucontext)
> +{
> +    ucontext_t *uc = ucontext;
> +    uc->uc_mcontext.regs->nip += 4;
> +    saved = uc->uc_mcontext.regs->xer;
> +    uc->uc_mcontext.regs->xer |= XER_OV | XER_OV32;
> +}
> +
> +int main(void)
> +{
> +    uint64_t initial = XER_CA | XER_CA32, restored;
> +    struct sigaction sa = {
> +        .sa_sigaction = sigill_handler,
> +        .sa_flags = SA_SIGINFO
> +    };
> +
> +    sigaction(SIGILL, &sa, NULL);
> +
> +    asm("mtspr 1, %1\n\t"
> +        ".long 0x0\n\t"
> +        "mfspr %0, 1\n\t"
> +        : "=r" (restored)
> +        : "r" (initial));
> +
> +    assert(saved == initial);
> +    assert(restored == (XER_OV | XER_OV32 | XER_CA | XER_CA32));
> +
> +    return 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 --]

Re: [PATCH 2/4] target/ppc: Fix XER access in gdbstub

[David Gibson]

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

On Thu, Oct 14, 2021 at 07:32:32PM -0300, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> The value of XER is split in multiple fields of CPUPPCState, like
> env->xer and env->so. To get/set the whole register from gdb, we should
> use cpu_read_xer/cpu_write_xer.
> 
> Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

Applied to ppc-for-6.2.

> ---
>  target/ppc/gdbstub.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 1808a150e4..105c2f7dd1 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -159,7 +159,7 @@ int ppc_cpu_gdb_read_register(CPUState *cs, GByteArray *buf, int n)
>              gdb_get_regl(buf, env->ctr);
>              break;
>          case 69:
> -            gdb_get_reg32(buf, env->xer);
> +            gdb_get_reg32(buf, cpu_read_xer(env));
>              break;
>          case 70:
>              gdb_get_reg32(buf, env->fpscr);
> @@ -217,7 +217,7 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, GByteArray *buf, int n)
>              gdb_get_reg64(buf, env->ctr);
>              break;
>          case 69 + 32:
> -            gdb_get_reg32(buf, env->xer);
> +            gdb_get_reg32(buf, cpu_read_xer(env));
>              break;
>          case 70 + 32:
>              gdb_get_reg64(buf, env->fpscr);
> @@ -269,7 +269,7 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>              env->ctr = ldtul_p(mem_buf);
>              break;
>          case 69:
> -            env->xer = ldl_p(mem_buf);
> +            cpu_write_xer(env, ldl_p(mem_buf));
>              break;
>          case 70:
>              /* fpscr */
> @@ -319,7 +319,7 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
>              env->ctr = ldq_p(mem_buf);
>              break;
>          case 69 + 32:
> -            env->xer = ldl_p(mem_buf);
> +            cpu_write_xer(env, ldl_p(mem_buf));
>              break;
>          case 70 + 32:
>              /* fpscr */

-- 
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: [PATCH 1/4] linux-user/ppc: Fix XER access in save/restore_user_regs

[Matheus K. Ferst]

On 14/10/2021 20:43, Richard Henderson wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você 
> possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de 
> e-mail suspeito entre imediatamente em contato com o DTI.
> 
> On 10/14/21 3:32 PM, matheus.ferst@eldorado.org.br wrote:
>> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>
>> We should use cpu_read_xer/cpu_write_xer to save/restore the complete
>> register since some of its bits are in other fields of CPUPPCState. A
>> test is added to prevent future regressions.
>>
>> Fixes: da91a00f191f ("target-ppc: Split out SO, OV, CA fields from XER")
>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>> ---
>>   linux-user/ppc/signal.c                     |  9 +++--
>>   tests/tcg/ppc64/Makefile.target             |  2 +
>>   tests/tcg/ppc64le/Makefile.target           |  2 +
>>   tests/tcg/ppc64le/signal_save_restore_xer.c | 42 +++++++++++++++++++++
>>   4 files changed, 52 insertions(+), 3 deletions(-)
>>   create mode 100644 tests/tcg/ppc64le/signal_save_restore_xer.c
> 
> The code is good so,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
>> +    sigaction(SIGILL, &sa, NULL);
>> +
>> +    asm("mtspr 1, %1\n\t"
>> +        ".long 0x0\n\t"
> 
> While Appendix B does guarantee that "0" is and always will be an 
> invalid instruction, I
> wonder if the test itself would be clearer (i.e. self-documenting the 
> intent) using
> SIGTRAP and "trap".
> 
> 
> r~

It would be better, but cpu_loop is currently calling cpu_abort for 
POWERPC_EXCP_TRAP, so the test would fail. I'll see if I can fix that in 
another patch, and then we can change the test to use trap.

-- 
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH 1/4] linux-user/ppc: Fix XER access in save/restore_user_regs

[Richard Henderson]

On 10/15/21 7:54 AM, Matheus K. Ferst wrote:
>> While Appendix B does guarantee that "0" is and always will be an invalid instruction, I
>> wonder if the test itself would be clearer (i.e. self-documenting the intent) using
>> SIGTRAP and "trap".
>>
>>
>> r~
> 
> It would be better, but cpu_loop is currently calling cpu_abort for POWERPC_EXCP_TRAP, so 
> the test would fail. I'll see if I can fix that in another patch, and then we can change 
> the test to use trap.

Fair enough.


r~