| Message ID | 20180320134114.30418-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix various BRK/BKPT related bugs | expand |
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é <f4bug@amsat.org> > and use the correct condition to select long or short format. ^ part 2 (fix) looks correct: Reviewed-by: Philippe Mathieu-Daudé <f4bug@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.maydell@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), >
On 21 March 2018 at 22:26, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi Peter, > > This patch might be split in 2. For a +27-10 patch, it doesn't really seem necessary. > 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é <f4bug@amsat.org> > >> and use the correct condition to select long or short format. > > ^ part 2 (fix) looks correct: > Reviewed-by: Philippe Mathieu-Daudé <f4bug@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.maydell@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. Yeah, it's basically similar logic. I'm a bit confused overall -- are you giving a reviewed-by for this patch, or do you want changes? thanks -- PMM
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; + } + } + + 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),
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 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, and use the correct condition to select long or short format. 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.maydell@linaro.org> --- target/arm/internals.h | 25 +++++++++++++++++++++++++ target/arm/op_helper.c | 12 ++---------- 2 files changed, 27 insertions(+), 10 deletions(-) -- 2.16.2