Re: [PATCH] target/hppa: Fix atomic_store_3 for STBY

[Helge Deller]

On 12/29/21 23:29, Richard Henderson wrote:
> The parallel version of STBY did not take host endianness into
> account, and also computed the incorrect address for STBY_E.
>
> Bswap twice to handle the merge and store.  Compute mask inside
> the function rather than as a parameter.  Force align the address,
> rather than subtracting one.
>
> Generalize the function to system mode by using probe_access().
>
> Reported-by: Helge Deller <deller@gmx.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Please add:
Tested-by: Helge Deller <deller@gmx.de>

I successfully tested the included stby test case on physical hardware and in 
qemu (on x86).
My original problem where gcc (on hppa) produces wrong assembly output is fixed 
too.
Please backport this patch to qemu v6.1.x and v6.0.x.

Thank you!
Helge


> ---
>  target/hppa/op_helper.c        | 27 ++++++-----
>  tests/tcg/hppa/stby.c          | 87 ++++++++++++++++++++++++++++++++++
>  tests/tcg/hppa/Makefile.target |  5 ++
>  3 files changed, 107 insertions(+), 12 deletions(-)
>  create mode 100644 tests/tcg/hppa/stby.c
>
> diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
> index 96d9391c39..1b86557d5d 100644
> --- a/target/hppa/op_helper.c
> +++ b/target/hppa/op_helper.c
> @@ -57,26 +57,29 @@ void HELPER(tcond)(CPUHPPAState *env, target_ureg cond)
>      }
>  }
>
> -static void atomic_store_3(CPUHPPAState *env, target_ulong addr, uint32_t 
> val,
> -                           uint32_t mask, uintptr_t ra)
> +static void atomic_store_3(CPUHPPAState *env, target_ulong addr,
> +                           uint32_t val, uintptr_t ra)
>  {
> -#ifdef CONFIG_USER_ONLY
> -    uint32_t old, new, cmp;
> +    int mmu_idx = cpu_mmu_index(env, 0);
> +    uint32_t old, new, cmp, mask, *haddr;
> +    void *vaddr;
> +
> +    vaddr = probe_access(env, addr, 3, MMU_DATA_STORE, mmu_idx, ra);
> +    if (vaddr == NULL) {
> +        cpu_loop_exit_atomic(env_cpu(env), ra);
> +    }
> +    haddr = (uint32_t *)((uintptr_t)vaddr & -4);
> +    mask = addr & 1 ? 0x00ffffffu : 0xffffff00u;
>
> -    uint32_t *haddr = g2h(env_cpu(env), addr - 1);
>      old = *haddr;
>      while (1) {
> -        new = (old & ~mask) | (val & mask);
> +        new = be32_to_cpu((cpu_to_be32(old) & ~mask) | (val & mask));
>          cmp = qatomic_cmpxchg(haddr, old, new);
>          if (cmp == old) {
>              return;
>          }
>          old = cmp;
>      }
> -#else
> -    /* FIXME -- we can do better.  */
> -    cpu_loop_exit_atomic(env_cpu(env), ra);
> -#endif
>  }
>
>  static void do_stby_b(CPUHPPAState *env, target_ulong addr, target_ureg val,
> @@ -92,7 +95,7 @@ static void do_stby_b(CPUHPPAState *env, target_ulong addr, 
> target_ureg val,
>      case 1:
>          /* The 3 byte store must appear atomic.  */
>          if (parallel) {
> -            atomic_store_3(env, addr, val, 0x00ffffffu, ra);
> +            atomic_store_3(env, addr, val, ra);
>          } else {
>              cpu_stb_data_ra(env, addr, val >> 16, ra);
>              cpu_stw_data_ra(env, addr + 1, val, ra);
> @@ -122,7 +125,7 @@ static void do_stby_e(CPUHPPAState *env, target_ulong 
> addr, target_ureg val,
>      case 3:
>          /* The 3 byte store must appear atomic.  */
>          if (parallel) {
> -            atomic_store_3(env, addr - 3, val, 0xffffff00u, ra);
> +            atomic_store_3(env, addr - 3, val, ra);
>          } else {
>              cpu_stw_data_ra(env, addr - 3, val >> 16, ra);
>              cpu_stb_data_ra(env, addr - 1, val >> 8, ra);
> diff --git a/tests/tcg/hppa/stby.c b/tests/tcg/hppa/stby.c
> new file mode 100644
> index 0000000000..36bd5f723c
> --- /dev/null
> +++ b/tests/tcg/hppa/stby.c
> @@ -0,0 +1,87 @@
> +/* Test STBY */
> +
> +#include <pthread.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +
> +struct S {
> +    unsigned a;
> +    unsigned b;
> +    unsigned c;
> +};
> +
> +static void check(const struct S *s, unsigned e,
> +                  const char *which, const char *insn, int ofs)
> +{
> +    int err = 0;
> +
> +    if (s->a != 0) {
> +        fprintf(stderr, "%s %s %d: garbage before word 0x%08x\n",
> +                which, insn, ofs, s->a);
> +        err = 1;
> +    }
> +    if (s->c != 0) {
> +        fprintf(stderr, "%s %s %d: garbage after word 0x%08x\n",
> +                which, insn, ofs, s->c);
> +        err = 1;
> +    }
> +    if (s->b != e) {
> +        fprintf(stderr, "%s %s %d: 0x%08x != 0x%08x\n",
> +                which, insn, ofs, s->b, e);
> +        err = 1;
> +    }
> +
> +    if (err) {
> +        exit(1);
> +    }
> +}
> +
> +#define TEST(INSN, OFS, E)                                         \
> +    do {                                                           \
> +        s.b = 0;                                                   \
> +        asm volatile(INSN " %1, " #OFS "(%0)"                      \
> +                     : : "r"(&s.b), "r" (0x11223344) : "memory");  \
> +        check(&s, E, which, INSN, OFS);                            \
> +    } while (0)
> +
> +static void test(const char *which)
> +{
> +    struct S s = { };
> +
> +    TEST("stby,b", 0, 0x11223344);
> +    TEST("stby,b", 1, 0x00223344);
> +    TEST("stby,b", 2, 0x00003344);
> +    TEST("stby,b", 3, 0x00000044);
> +
> +    TEST("stby,e", 0, 0x00000000);
> +    TEST("stby,e", 1, 0x11000000);
> +    TEST("stby,e", 2, 0x11220000);
> +    TEST("stby,e", 3, 0x11223300);
> +}
> +
> +static void *child(void *x)
> +{
> +    return NULL;
> +}
> +
> +int main()
> +{
> +    int err;
> +    pthread_t thr;
> +
> +    /* Run test in serial mode */
> +    test("serial");
> +
> +    /* Create a dummy thread to start parallel mode. */
> +    err = pthread_create(&thr, NULL, child, NULL);
> +    if (err != 0) {
> +        fprintf(stderr, "pthread_create: %s\n", strerror(err));
> +        return 2;
> +    }
> +
> +    /* Run test in parallel mode */
> +    test("parallel");
> +    return 0;
> +}
> diff --git a/tests/tcg/hppa/Makefile.target b/tests/tcg/hppa/Makefile.target
> index d0d5e0e257..b78e6b4849 100644
> --- a/tests/tcg/hppa/Makefile.target
> +++ b/tests/tcg/hppa/Makefile.target
> @@ -12,3 +12,8 @@ run-signals: signals
>       $(call skip-test, $<, "BROKEN awaiting vdso support")
>  run-plugin-signals-with-%:
>       $(call skip-test, $<, "BROKEN awaiting vdso support")
> +
> +VPATH += $(SRC_PATH)/tests/tcg/hppa
> +TESTS += stby
> +
> +stby: CFLAGS += -pthread
>