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

[Peter Maydell]

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

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