Re: [Qemu-devel] [PATCH 2/7] target-arm: Implement MDCR_EL3 and SDCR

[Peter Maydell <peter.maydell@linaro.org>]

On 5 February 2016 at 11:13, Alex Bennée <alex.bennee@linaro.org> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> Implement the MDCR_EL3 register (which is SDCR for AArch32).
>> For the moment we implement it as reads-as-written.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>> +/* Some secure-only AArch32 registers trap to EL3 if used from
>> + * Secure EL1 (but are just ordinary UNDEF in other non-EL3 contexts).
>> + * We assume that the .access field is set to PL1_RW.
>> + */
>> +static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
>> +                                            const ARMCPRegInfo *ri)
>> +{
>
> I wonder if we should assert the fact we are in AArch32 here in case the
> wrong access function gets added to a AArch64 register?

We mostly don't do that kind of checking in these access
functions. If you add the wrong access function to your regdef
then your register misbehaves anyway :-)

>> +    if (arm_current_el(env) == 3) {
>> +        return CP_ACCESS_OK;
>> +    }
>> +    if (arm_is_secure_below_el3(env)) {
>> +        return CP_ACCESS_TRAP_EL3;
>> +    }
>> +    /* This will be EL1 NS and EL2 NS, which just UNDEF */
>> +    return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +}
>> +
>>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>>  {
>>      ARMCPU *cpu = arm_env_get_cpu(env);
>> @@ -3532,6 +3549,13 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
>>        .access = PL3_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
>>        .writefn = scr_write },
>> +    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
>> +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
>> +    { .name = "SDCR", .type = ARM_CP_ALIAS,
>> +      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
>> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
>> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
>
> Does anything ensure the fields are reset to 0 on a warm reset?

Yes, the cpreg framework resets things. arm_cpu_reset() calls
cp_reg_reset() on every register the CPU knows about. Note that
.resetvalue is 0 as usual for unspecified fields in structure
initializers, but it would be clearer to specifically state it
(ie ".resetvalue = 0") as we do in most cases.

thanks
-- PMM