On 21 March 2018 at 22:26, Philippe Mathieu-Daudé <f4...@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é <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.
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
