Re: [Qemu-devel] [PATCH 4/7] target-arm: Update arm_generate_debug_exceptions() to handle EL2/EL3

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> The arm_generate_debug_exceptions() function as originally implemented
> assumes no EL2 or EL3. Since we now have much more of an implementation
> of those now, fix this assumption.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h | 48 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cf2df50..0fb79d0 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1742,9 +1742,7 @@ typedef enum ARMASIdx {
>      ARMASIdx_S = 1,
>  } ARMASIdx;
>
> -/* Return the Exception Level targeted by debug exceptions;
> - * currently always EL1 since we don't implement EL2 or EL3.
> - */
> +/* Return the Exception Level targeted by debug exceptions. */
>  static inline int arm_debug_target_el(CPUARMState *env)
>  {
>      bool secure = arm_is_secure(env);
> @@ -1767,6 +1765,14 @@ static inline int arm_debug_target_el(CPUARMState *env)
>
>  static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>  {
> +    if (arm_is_secure(env)) {
> +        /* MDCR_EL3.SDD disables debug events from Secure state */

Is it worth commenting that BRK still works?

> +        if (extract32(env->cp15.mdcr_el3,ctct 16, 1) != 0

The != 0 is superfluous here.

> +            || arm_current_el(env) == 3) {
> +            return false;
> +        }
> +    }
> +
>      if (arm_current_el(env) == arm_debug_target_el(env)) {
>          if ((extract32(env->cp15.mdscr_el1, 13, 1) == 0)
>              || (env->daif & PSTATE_D)) {
> @@ -1778,10 +1784,42 @@ static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>
>  static inline bool aa32_generate_debug_exceptions(CPUARMState *env)
>  {
> -    if (arm_current_el(env) == 0 && arm_el_is_aa64(env, 1)) {
> +    int el = arm_current_el(env);
> +
> +    if (el == 0 && arm_el_is_aa64(env, 1)) {
>          return aa64_generate_debug_exceptions(env);
>      }
> -    return arm_current_el(env) != 2;
> +
> +    if (arm_is_secure(env)) {
> +        int spd;
> +
> +        if (el == 0 && (env->cp15.sder & 1)) {
> +            /* SDER.SUIDEN means debug exceptions from Secure EL0
> +             * are always enabled. Otherwise they are controlled by
> +             * SDCR.SPD like those from other Secure ELs.
> +             */
> +            return true;
> +        }
> +
> +        spd = extract32(env->cp15.mdcr_el3, 14, 2);
> +        switch (spd) {
> +        case 1:
> +            /* SPD == 0b01 is reserved, but behaves as 0b00. */
> +        case 0:
> +            /* For 0b00 we return true if external secure invasive debug
> +             * is enabled. On real hardware this is controlled by external
> +             * signals to the core. QEMU always permits debug, and behaves
> +             * as if DBGEN, SPIDEN, NIDEN and SPNIDEN are all tied high.
> +             */
> +            return true;
> +        case 2:
> +            return false;
> +        case 3:
> +            return true;
> +        }
> +    }
> +
> +    return el != 2;
>  }
>
>  /* Return true if debugging exceptions are currently enabled.

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée