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é <f4...@amsat.org>
> and use the correct condition to select long or short format.
^ part 2 (fix) looks correct:
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>
> 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 <peter.mayd...@linaro.org>
> ---
> 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),
>
