[3/3] target-mips: Misaligned Memory Accesses for MSA

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.

Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
---
 target-mips/op_helper.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

On 1 May 2015 at 16:24, Yongbok Kim <yongbok.kim@imgtec.com> 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.
>
> 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++) {
> +        if (!cpu_mips_validate_access(env, address + (i << df),
> +            address, (1 << df), rw)) {
> +            CPUState *cs = CPU(mips_env_get_cpu(env));
> +            helper_raise_exception_err(env, cs->exception_index,
> +                                       env->error_code);

I was wondering if this would get the correct PC in the exception
case, but we always call save_cpu_state() before calling the
msa_ld/st_df helpers, so it will.

-- PMM

On 01/05/2015 16:43, Peter Maydell wrote:
> On 1 May 2015 at 16:24, Yongbok Kim <yongbok.kim@imgtec.com> 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.
>>
>> 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++) {
>> +        if (!cpu_mips_validate_access(env, address + (i << df),
>> +            address, (1 << df), rw)) {
>> +            CPUState *cs = CPU(mips_env_get_cpu(env));
>> +            helper_raise_exception_err(env, cs->exception_index,
>> +                                       env->error_code);
> I was wondering if this would get the correct PC in the exception
> case, but we always call save_cpu_state() before calling the
> msa_ld/st_df helpers, so it will.
>
> -- PMM
Yes it does because of the save_cpu_state(). Actually I have considered to use
cpu_restore_state() with GETRA() but it looks like using save_cpu_state() is quite
common in the target-mips. It would be good such clean-up for all the cases in the future work.
But this patch I would follow existing style for the consistency.

Regards,
Yongbok

On 01/05/2015 16:43, Peter Maydell wrote:
>> +#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++) {
>> +        if (!cpu_mips_validate_access(env, address + (i << df),
>> +            address, (1 << df), rw)) {
>> +            CPUState *cs = CPU(mips_env_get_cpu(env));
>> +            helper_raise_exception_err(env, cs->exception_index,
>> +                                       env->error_code);
> 
> I was wondering if this would get the correct PC in the exception
> case, but we always call save_cpu_state() before calling the
> msa_ld/st_df helpers, so it will.

FYI, quite recently Richard suggested a clean-up for this (and all other
MIPS load/store instructions using helpers), so save_cpu_state()
wouldn’t be required. I haven’t got round to it yet but sounds like a
nice improvement.

Leon

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