Re: [Qemu-devel] [PATCH v4 2/5] target/mips: Fix MSA instructions ST. on big endian host

[Mateja Marjanovic]

On 20.5.19. 14:59, Aleksandar Markovic wrote:

On Apr 2, 2019 3:47 PM, "Mateja Marjanovic" <mateja.marjano...@rt-rk.com <mailto:mateja.marjano...@rt-rk.com>> wrote:

>

> From: Mateja Marjanovic <mateja.marjano...@rt-rk.com <mailto:mateja.marjano...@rt-rk.com>>

>
> Fix the case when the host is a big endian machine, and change
> the approach toward ST.<B|H|W|D> instruction helpers.
>

> Signed-off-by: Mateja Marjanovic <mateja.marjano...@rt-rk.com <mailto:mateja.marjano...@rt-rk.com>>

> ---

Hello, Mateja.

There is unfortunatelly still a slight problem with the new implementation: it looks like the invocations to ensure_writable_pages() in new helpers are missing. Or, perhaps, there is a reason you removed them. Please reanalyse and reexplain. But thanks for previous efforts.

I see, I deleted it by accident.

Thanks,
Mateja

Yours,
Aleksandar

>  target/mips/op_helper.c | 188 ++++++++++++++++++++++++++++++++++++++++++------

>  1 file changed, 168 insertions(+), 20 deletions(-)
>
> diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
> index 45be406..d94909a 100644
> --- a/target/mips/op_helper.c
> +++ b/target/mips/op_helper.c

> @@ -4565,31 +4565,179 @@ static inline void ensure_writable_pages(CPUMIPSState *env,

>  #endif
>  }
>
> -#define MSA_ST_DF(DF, TYPE, ST_INSN, ...)              \

> -void helper_msa_st_ ## TYPE(CPUMIPSState *env, uint32_t wd,             \

> -                            target_ulong addr)               \
> -{              \
> -    wr_t *pwd = &(env->active_fpu.fpr[wd].wr);                     \
> -    int mmu_idx = cpu_mmu_index(env, false);              \
> -    int i;               \
> -    MEMOP_IDX(DF)              \
> -    ensure_writable_pages(env, addr, mmu_idx, GETPC());              \
> -    for (i = 0; i < DF_ELEMENTS(DF); i++) {                  \

> -        ST_INSN(env, addr + (i << DF), pwd->TYPE[i], ##__VA_ARGS__);    \

> -    }              \
> +void helper_msa_st_b(CPUMIPSState *env, uint32_t wd,
> +                     target_ulong addr)
> +{
> +    wr_t *pwd = &(env->active_fpu.fpr[wd].wr);
> +    MEMOP_IDX(DF_BYTE)
> +#if !defined(CONFIG_USER_ONLY)
> +#if !defined(HOST_WORDS_BIGENDIAN)

> +    helper_ret_stb_mmu(env, addr + (0  << DF_BYTE), pwd->b[0],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (1  << DF_BYTE), pwd->b[1],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (2  << DF_BYTE), pwd->b[2],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (3  << DF_BYTE), pwd->b[3],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (4  << DF_BYTE), pwd->b[4],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (5  << DF_BYTE), pwd->b[5],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (6  << DF_BYTE), pwd->b[6],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (7  << DF_BYTE), pwd->b[7],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (8  << DF_BYTE), pwd->b[8],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (9  << DF_BYTE), pwd->b[9],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (10 << DF_BYTE), pwd->b[10], oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (11 << DF_BYTE), pwd->b[11], oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (12 << DF_BYTE), pwd->b[12], oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (13 << DF_BYTE), pwd->b[13], oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (14 << DF_BYTE), pwd->b[14], oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (15 << DF_BYTE), pwd->b[15], oi, GETPC());

> +#else

> +    helper_ret_stb_mmu(env, addr + (7  << DF_BYTE), pwd->b[0],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (6  << DF_BYTE), pwd->b[1],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (5  << DF_BYTE), pwd->b[2],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (4  << DF_BYTE), pwd->b[3],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (3  << DF_BYTE), pwd->b[4],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (2  << DF_BYTE), pwd->b[5],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (1  << DF_BYTE), pwd->b[6],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (0  << DF_BYTE), pwd->b[7],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (15 << DF_BYTE), pwd->b[8],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (14 << DF_BYTE), pwd->b[9],  oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (13 << DF_BYTE), pwd->b[10], oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (12 << DF_BYTE), pwd->b[11], oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (11 << DF_BYTE), pwd->b[12], oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (10 << DF_BYTE), pwd->b[13], oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (9  << DF_BYTE), pwd->b[14], oi, GETPC()); > +    helper_ret_stb_mmu(env, addr + (8  << DF_BYTE), pwd->b[15], oi, GETPC());

> +#endif
> +#else
> +#if !defined(HOST_WORDS_BIGENDIAN)
> +    cpu_stb_data(env, addr + (0  << DF_BYTE), pwd->b[0]);
> +    cpu_stb_data(env, addr + (1  << DF_BYTE), pwd->b[1]);
> +    cpu_stb_data(env, addr + (2  << DF_BYTE), pwd->b[2]);
> +    cpu_stb_data(env, addr + (3  << DF_BYTE), pwd->b[3]);
> +    cpu_stb_data(env, addr + (4  << DF_BYTE), pwd->b[4]);
> +    cpu_stb_data(env, addr + (5  << DF_BYTE), pwd->b[5]);
> +    cpu_stb_data(env, addr + (6  << DF_BYTE), pwd->b[6]);
> +    cpu_stb_data(env, addr + (7  << DF_BYTE), pwd->b[7]);
> +    cpu_stb_data(env, addr + (8  << DF_BYTE), pwd->b[8]);
> +    cpu_stb_data(env, addr + (9  << DF_BYTE), pwd->b[9]);
> +    cpu_stb_data(env, addr + (10 << DF_BYTE), pwd->b[10]);
> +    cpu_stb_data(env, addr + (11 << DF_BYTE), pwd->b[11]);
> +    cpu_stb_data(env, addr + (12 << DF_BYTE), pwd->b[12]);
> +    cpu_stb_data(env, addr + (13 << DF_BYTE), pwd->b[13]);
> +    cpu_stb_data(env, addr + (14 << DF_BYTE), pwd->b[14]);
> +    cpu_stb_data(env, addr + (15 << DF_BYTE), pwd->b[15]);
> +#else
> +    cpu_stb_data(env, addr + (7  << DF_BYTE), pwd->b[0]);
> +    cpu_stb_data(env, addr + (6  << DF_BYTE), pwd->b[1]);
> +    cpu_stb_data(env, addr + (5  << DF_BYTE), pwd->b[2]);
> +    cpu_stb_data(env, addr + (4  << DF_BYTE), pwd->b[3]);
> +    cpu_stb_data(env, addr + (3  << DF_BYTE), pwd->b[4]);
> +    cpu_stb_data(env, addr + (2  << DF_BYTE), pwd->b[5]);
> +    cpu_stb_data(env, addr + (1  << DF_BYTE), pwd->b[6]);
> +    cpu_stb_data(env, addr + (0  << DF_BYTE), pwd->b[7]);
> +    cpu_stb_data(env, addr + (15 << DF_BYTE), pwd->b[8]);
> +    cpu_stb_data(env, addr + (14 << DF_BYTE), pwd->b[9]);
> +    cpu_stb_data(env, addr + (13 << DF_BYTE), pwd->b[10]);
> +    cpu_stb_data(env, addr + (12 << DF_BYTE), pwd->b[11]);
> +    cpu_stb_data(env, addr + (11 << DF_BYTE), pwd->b[12]);
> +    cpu_stb_data(env, addr + (10 << DF_BYTE), pwd->b[13]);
> +    cpu_stb_data(env, addr + (9  << DF_BYTE), pwd->b[14]);
> +    cpu_stb_data(env, addr + (8  << DF_BYTE), pwd->b[15]);
> +#endif
> +#endif
> +}
> +
> +void helper_msa_st_h(CPUMIPSState *env, uint32_t wd,
> +                     target_ulong addr)
> +{
> +    wr_t *pwd = &(env->active_fpu.fpr[wd].wr);
> +    MEMOP_IDX(DF_HALF)
> +#if !defined(CONFIG_USER_ONLY)
> +#if !defined(HOST_WORDS_BIGENDIAN)

> +    helper_ret_stw_mmu(env, addr + (0 << DF_HALF), pwd->h[0], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (1 << DF_HALF), pwd->h[1], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (2 << DF_HALF), pwd->h[2], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (3 << DF_HALF), pwd->h[3], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (4 << DF_HALF), pwd->h[4], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (5 << DF_HALF), pwd->h[5], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (6 << DF_HALF), pwd->h[6], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (7 << DF_HALF), pwd->h[7], oi, GETPC());

> +#else

> +    helper_ret_stw_mmu(env, addr + (3 << DF_HALF), pwd->h[0], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (2 << DF_HALF), pwd->h[1], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (1 << DF_HALF), pwd->h[2], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (0 << DF_HALF), pwd->h[3], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (7 << DF_HALF), pwd->h[4], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (6 << DF_HALF), pwd->h[5], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (5 << DF_HALF), pwd->h[6], oi, GETPC()); > +    helper_ret_stw_mmu(env, addr + (4 << DF_HALF), pwd->h[7], oi, GETPC());

> +#endif
> +#else
> +#if !defined(HOST_WORDS_BIGENDIAN)
> +    cpu_stw_data(env, addr + (0 << DF_HALF), pwd->h[0]);
> +    cpu_stw_data(env, addr + (1 << DF_HALF), pwd->h[1]);
> +    cpu_stw_data(env, addr + (2 << DF_HALF), pwd->h[2]);
> +    cpu_stw_data(env, addr + (3 << DF_HALF), pwd->h[3]);
> +    cpu_stw_data(env, addr + (4 << DF_HALF), pwd->h[4]);
> +    cpu_stw_data(env, addr + (5 << DF_HALF), pwd->h[5]);
> +    cpu_stw_data(env, addr + (6 << DF_HALF), pwd->h[6]);
> +    cpu_stw_data(env, addr + (7 << DF_HALF), pwd->h[7]);
> +#else
> +    cpu_stw_data(env, addr + (3 << DF_HALF), pwd->h[0]);
> +    cpu_stw_data(env, addr + (2 << DF_HALF), pwd->h[1]);
> +    cpu_stw_data(env, addr + (1 << DF_HALF), pwd->h[2]);
> +    cpu_stw_data(env, addr + (0 << DF_HALF), pwd->h[3]);
> +    cpu_stw_data(env, addr + (7 << DF_HALF), pwd->h[4]);
> +    cpu_stw_data(env, addr + (6 << DF_HALF), pwd->h[5]);
> +    cpu_stw_data(env, addr + (5 << DF_HALF), pwd->h[6]);
> +    cpu_stw_data(env, addr + (4 << DF_HALF), pwd->h[7]);
> +#endif
> +#endif
>  }
>
> +void helper_msa_st_w(CPUMIPSState *env, uint32_t wd,
> +                     target_ulong addr)
> +{
> +    wr_t *pwd = &(env->active_fpu.fpr[wd].wr);
> +    MEMOP_IDX(DF_WORD)
>  #if !defined(CONFIG_USER_ONLY)
> -MSA_ST_DF(DF_BYTE,   b, helper_ret_stb_mmu, oi, GETPC())
> -MSA_ST_DF(DF_HALF,   h, helper_ret_stw_mmu, oi, GETPC())
> -MSA_ST_DF(DF_WORD,   w, helper_ret_stl_mmu, oi, GETPC())
> -MSA_ST_DF(DF_DOUBLE, d, helper_ret_stq_mmu, oi, GETPC())
> +#if !defined(HOST_WORDS_BIGENDIAN)

> +    helper_ret_stl_mmu(env, addr + (0 << DF_WORD), oi, GETPC(), pwd->w[0]); > +    helper_ret_stl_mmu(env, addr + (1 << DF_WORD), oi, GETPC(), pwd->w[1]); > +    helper_ret_stl_mmu(env, addr + (2 << DF_WORD), oi, GETPC(), pwd->w[2]); > +    helper_ret_stl_mmu(env, addr + (3 << DF_WORD), oi, GETPC(), pwd->w[3]);

>  #else
> -MSA_ST_DF(DF_BYTE,   b, cpu_stb_data)
> -MSA_ST_DF(DF_HALF,   h, cpu_stw_data)
> -MSA_ST_DF(DF_WORD,   w, cpu_stl_data)
> -MSA_ST_DF(DF_DOUBLE, d, cpu_stq_data)

> +    helper_ret_stl_mmu(env, addr + (1 << DF_WORD), oi, GETPC(), pwd->w[0]); > +    helper_ret_stl_mmu(env, addr + (0 << DF_WORD), oi, GETPC(), pwd->w[1]); > +    helper_ret_stl_mmu(env, addr + (3 << DF_WORD), oi, GETPC(), pwd->w[2]); > +    helper_ret_stl_mmu(env, addr + (2 << DF_WORD), oi, GETPC(), pwd->w[3]);

>  #endif
> +#else
> +#if !defined(HOST_WORDS_BIGENDIAN)
> +    cpu_stl_data(env, addr + (0 << DF_WORD), pwd->w[0]);
> +    cpu_stl_data(env, addr + (1 << DF_WORD), pwd->w[1]);
> +    cpu_stl_data(env, addr + (2 << DF_WORD), pwd->w[2]);
> +    cpu_stl_data(env, addr + (3 << DF_WORD), pwd->w[3]);
> +#else
> +    cpu_stl_data(env, addr + (1 << DF_WORD), pwd->w[0]);
> +    cpu_stl_data(env, addr + (0 << DF_WORD), pwd->w[1]);
> +    cpu_stl_data(env, addr + (3 << DF_WORD), pwd->w[2]);
> +    cpu_stl_data(env, addr + (2 << DF_WORD), pwd->w[3]);
> +#endif
> +#endif
> +}
> +
> +void helper_msa_st_d(CPUMIPSState *env, uint32_t wd,
> +                     target_ulong addr)
> +{
> +    wr_t *pwd = &(env->active_fpu.fpr[wd].wr);
> +    MEMOP_IDX(DF_DOUBLE)
> +#if !defined(CONFIG_USER_ONLY)

> +    helper_ret_stq_mmu(env, addr + (0 << DF_DOUBLE), pwd->d[0], oi, GETPC()); > +    helper_ret_stq_mmu(env, addr + (1 << DF_DOUBLE), pwd->d[1], oi, GETPC());

> +#else
> +    cpu_stq_data(env, addr + (0 << DF_DOUBLE), pwd->d[0]);
> +    cpu_stq_data(env, addr + (1 << DF_DOUBLE), pwd->d[1]);
> +#endif
> +}
>
>  void helper_cache(CPUMIPSState *env, target_ulong addr, uint32_t op)
>  {
> --
> 2.7.4
>
>