1. Patchew
  2. QEMU
  3. [PATCH 0/4] Fix incorrect accesses to XER
  4. View patch

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

matheus.ferst@eldorado.org.br posted 4 patches 4 years, 3 months ago
Maintainers: David Gibson <david@gibson.dropbear.id.au>, Laurent Vivier <laurent@vivier.eu>, Greg Kurz <groug@kaod.org>
[PATCH 1/4] linux-user/ppc: Fix XER access in save/restore_user_regs
Posted by matheus.ferst@eldorado.org.br 4 years, 3 months ago 
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
Re: [PATCH 1/4] linux-user/ppc: Fix XER access in save/restore_user_regs
Posted by Richard Henderson 4 years, 3 months ago 
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 1/4] linux-user/ppc: Fix XER access in save/restore_user_regs
Posted by Matheus K. Ferst 4 years, 3 months ago 
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
Posted by Richard Henderson 4 years, 3 months ago 
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~
Re: [PATCH 1/4] linux-user/ppc: Fix XER access in save/restore_user_regs
Posted by David Gibson 4 years, 3 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.