Re: [Qemu-devel] [PATCH 3/3] target-mips: Misaligned Memory Accesses for MSA

[Leon Alrae <leon.alrae@imgtec.com>]

On 01/05/15 16:24, Yongbok Kim wrote:
> MIPS SIMD Architecture vector loads and stores require misalignment support.
> MSA Memory access should work as an atomic operation. Therefore, it has to
> check validity of all the addresses for the operation.

As far as I can tell mips_cpu_do_unaligned_access() will be still
generating AdE exceptions for MSA loads/stores in MIPS R5 which doesn't
seem to be correct.

> 
> Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
> ---
>  target-mips/op_helper.c |   30 ++++++++++++++++++++++++++++++
>  1 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index dacc92b..89a7de6 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -3571,6 +3571,24 @@ FOP_CONDN_S(sne,  (float32_lt(fst1, fst0, &env->active_fpu.fp_status)
>  /* Element-by-element access macros */
>  #define DF_ELEMENTS(df) (MSA_WRLEN / DF_BITS(df))
>  
> +#if !defined(CONFIG_USER_ONLY)
> +static bool cpu_mips_validate_msa_block_access(CPUMIPSState *env,
> +                                        target_ulong address, int df, int rw)
> +{
> +    int i;
> +    for (i = 0; i < DF_ELEMENTS(df); i++) {

Do we really need to check each element, wouldn't byte 0 and byte
(MSA_WRLEN/8 - 1) be enough?

> +        if (!cpu_mips_validate_access(env, address + (i << df),
> +            address, (1 << df), rw)) {

I believe this would look better if this line was aligned with the first
argument of the function (and would be consistent with the helper below).

> +            CPUState *cs = CPU(mips_env_get_cpu(env));
> +            helper_raise_exception_err(env, cs->exception_index,
> +                                       env->error_code);
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +#endif
> +
>  void helper_msa_ld_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs,
>                       int32_t s10)
>  {
> @@ -3578,6 +3596,12 @@ void helper_msa_ld_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs,
>      target_ulong addr = env->active_tc.gpr[rs] + (s10 << df);
>      int i;
>  
> +#if !defined(CONFIG_USER_ONLY)
> +    if (!cpu_mips_validate_msa_block_access(env, addr, df, MMU_DATA_LOAD)) {
> +        return;
> +    }

Shouldn't this be called only for cases where page boundary is crossed?
Otherwise I don't think this validation is needed.

> +#endif
> +
>      switch (df) {
>      case DF_BYTE:
>          for (i = 0; i < DF_ELEMENTS(DF_BYTE); i++) {
> @@ -3613,6 +3637,12 @@ void helper_msa_st_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs,
>      target_ulong addr = env->active_tc.gpr[rs] + (s10 << df);
>      int i;
>  
> +#if !defined(CONFIG_USER_ONLY)
> +    if (!cpu_mips_validate_msa_block_access(env, addr, df, MMU_DATA_STORE)) {
> +        return;
> +    }

Same.

Thanks,
Leon