Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12 2/4] target/arm: Factor out code to calculate FSR for debug exceptions

[Philippe Mathieu-Daudé]

Hi Peter,

This patch might be split in 2.

On 03/20/2018 02:41 PM, Peter Maydell wrote:
> When a debug exception is taken to AArch32, it appears as a Prefetch
> Abort, and the Instruction Fault Status Register (IFSR) must be set.
> The IFSR has two possible formats, depending on whether LPAE is in

^ intro

> use. Factor out the code in arm_debug_excp_handler() which picks
> an FSR value into its own utility function, update it to use
> arm_fi_to_lfsc() and arm_fi_to_sfsc() rather than hard-coded constants,

^ part 1 (refactor):
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> and use the correct condition to select long or short format.

^ part 2 (fix) looks correct:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> 
> In particular this fixes a bug where we could select the short
> format because we're at EL0 and the EL1 translation regime is
> not using LPAE, but then route the debug exception to EL2 because
> of MDCR_EL2.TDE and hand EL2 the wrong format FSR.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target/arm/internals.h | 25 +++++++++++++++++++++++++
>  target/arm/op_helper.c | 12 ++----------
>  2 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 47cc224a46..8ce944b7a0 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -763,4 +763,29 @@ static inline bool regime_is_secure(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>      }
>  }
>  
> +/* Return the FSR value for a debug exception (watchpoint, hardware
> + * breakpoint or BKPT insn) targeting the specified exception level.
> + */
> +static inline uint32_t arm_debug_exception_fsr(CPUARMState *env)
> +{
> +    ARMMMUFaultInfo fi = { .type = ARMFault_Debug };
> +    int target_el = arm_debug_target_el(env);
> +    bool using_lpae = false;
> +
> +    if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
> +        using_lpae = true;
> +    } else {
> +        if (arm_feature(env, ARM_FEATURE_LPAE) &&
> +            (env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)) {
> +            using_lpae = true;
> +        }
> +    }

This looks pretty similar to regime_using_lpae_format() but for
ARMFault_Debug.

> +
> +    if (using_lpae) {
> +        return arm_fi_to_lfsc(&fi);
> +    } else {
> +        return arm_fi_to_sfsc(&fi);
> +    }
> +}
> +
>  #endif
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 4b123d2bd6..75efff9edf 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -1330,11 +1330,7 @@ void arm_debug_excp_handler(CPUState *cs)
>  
>              cs->watchpoint_hit = NULL;
>  
> -            if (extended_addresses_enabled(env)) {
> -                env->exception.fsr = (1 << 9) | 0x22;
> -            } else {
> -                env->exception.fsr = 0x2;
> -            }
> +            env->exception.fsr = arm_debug_exception_fsr(env);
>              env->exception.vaddress = wp_hit->hitaddr;
>              raise_exception(env, EXCP_DATA_ABORT,
>                      syn_watchpoint(same_el, 0, wnr),
> @@ -1354,11 +1350,7 @@ void arm_debug_excp_handler(CPUState *cs)
>              return;
>          }
>  
> -        if (extended_addresses_enabled(env)) {
> -            env->exception.fsr = (1 << 9) | 0x22;
> -        } else {
> -            env->exception.fsr = 0x2;
> -        }
> +        env->exception.fsr = arm_debug_exception_fsr(env);
>          /* FAR is UNKNOWN, so doesn't need setting */
>          raise_exception(env, EXCP_PREFETCH_ABORT,
>                          syn_breakpoint(same_el),
>