[v4,06/21] target-arm: Provide syndrome information for MMU faults

From: Rob Herring <rob.herring@linaro.org>

Set up the required syndrome information when we detect an MMU fault.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
[PMM: split out from exception handling patch, tweaked to bring
 in line with how we create other kinds of syndrome information]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c    | 16 ++++++++++++++++
 target-arm/internals.h | 13 +++++++++++++
 2 files changed, 29 insertions(+)

On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> Set up the required syndrome information when we detect an MMU fault.
>
> Signed-off-by: Rob Herring <rob.herring@linaro.org>
> [PMM: split out from exception handling patch, tweaked to bring
>  in line with how we create other kinds of syndrome information]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c    | 16 ++++++++++++++++
>  target-arm/internals.h | 13 +++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 2fa01ae..d547a9d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3586,6 +3586,7 @@ int cpu_arm_handle_mmu_fault (CPUARMState *env, target_ulong address,
>      target_ulong page_size;
>      int prot;
>      int ret, is_user;
> +    uint32_t syn;
>
>      is_user = mmu_idx == MMU_USER_IDX;
>      ret = get_phys_addr(env, address, access_type, is_user, &phys_addr, &prot,
> @@ -3598,14 +3599,29 @@ int cpu_arm_handle_mmu_fault (CPUARMState *env, target_ulong address,
>          return 0;
>      }
>
> +    /* AArch64 syndrome does not have an LPAE bit */
> +    syn = ret & ~(1 << 9);
> +
> +    /* For insn and data aborts we assume there is no instruction syndrome
> +     * information; this is always true for exceptions reported to EL1.
> +     */
>      if (access_type == 2) {
> +        syn = syn_insn_abort(0, 0, syn);
>          env->exception_index = EXCP_PREFETCH_ABORT;
>      } else {
> +        syn = syn_data_abort(0, 0, 0, access_type == 1, syn);
>          if (access_type == 1 && arm_feature(env, ARM_FEATURE_V6)) {
>              ret |= (1 << 11);
>          }
>          env->exception_index = EXCP_DATA_ABORT;
>      }
> +
> +    /* Set bit 26 for exceptions with no change in EL */
> +    if (arm_current_pl(env)) {
> +        syn |= 1 << ARM_EL_EC_SHIFT;
> +    }
> +

Perhaps in internals.h:

#define ARM_EL_EC_SAME_LEVEL (1 << ARM_EL_EC_SHIFT)

Then this becomes:

syn |= ARM_EL_EC_SAME_LEVEL

Then in internals.h you can be more self documenting with:

EC_BREAKPOINT_SAME_EL = EC_BREAKPOINT | ARM_EL_EC_SAME_LEVEL

Regards,
Peter

> +    env->exception.syndrome = syn;
>      env->exception.vaddress = address;
>      env->exception.fsr = ret;
>      return 1;
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 9bec4e1..93f56a5 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -188,4 +188,17 @@ static inline uint32_t syn_cp15_rrt_trap(int cv, int cond, int opc1, int crm,
>          | (rt2 << 10) | (rt << 5) | (crm << 1) | isread;
>  }
>
> +static inline uint32_t syn_insn_abort(int ea, int s1ptw, int fsc)
> +{
> +    return (EC_INSNABORT << ARM_EL_EC_SHIFT) | (ea << 9)
> +        | (s1ptw << 7) | fsc;
> +}
> +
> +static inline uint32_t syn_data_abort(int ea, int cm, int s1ptw, int wnr,
> +                                      int fsc)
> +{
> +    return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (ea << 9)
> +        | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc;
> +}
> +
>  #endif
> --
> 1.9.0
>
>

On 17 March 2014 03:28, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> From: Rob Herring <rob.herring@linaro.org>

>> +    /* Set bit 26 for exceptions with no change in EL */
>> +    if (arm_current_pl(env)) {
>> +        syn |= 1 << ARM_EL_EC_SHIFT;
>> +    }
>> +
>
> Perhaps in internals.h:
>
> #define ARM_EL_EC_SAME_LEVEL (1 << ARM_EL_EC_SHIFT)
>
> Then this becomes:
>
> syn |= ARM_EL_EC_SAME_LEVEL
>
> Then in internals.h you can be more self documenting with:
>
> EC_BREAKPOINT_SAME_EL = EC_BREAKPOINT | ARM_EL_EC_SAME_LEVEL

Yeah, seems reasonable.

-- PMM

On 17 March 2014 12:41, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 March 2014 03:28, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> From: Rob Herring <rob.herring@linaro.org>
>
>>> +    /* Set bit 26 for exceptions with no change in EL */
>>> +    if (arm_current_pl(env)) {
>>> +        syn |= 1 << ARM_EL_EC_SHIFT;
>>> +    }
>>> +
>>
>> Perhaps in internals.h:
>>
>> #define ARM_EL_EC_SAME_LEVEL (1 << ARM_EL_EC_SHIFT)
>>
>> Then this becomes:
>>
>> syn |= ARM_EL_EC_SAME_LEVEL
>>
>> Then in internals.h you can be more self documenting with:
>>
>> EC_BREAKPOINT_SAME_EL = EC_BREAKPOINT | ARM_EL_EC_SAME_LEVEL
>
> Yeah, seems reasonable.

On the other hand you can't define EC_BREAKPOINT_SAME_EL
like that, because the EC_ enum values aren't shifted.

Perhaps it would be better to have the syn_* functions for
the EC values which have SAME_EL versions (currently just
insn abort and data abort, since we don't implement any
of the hardware debug exceptions) have an extra parameter
bool same_el, and have the syn_ function OR in the extra bit.

thanks
-- PMM