Re: [Qemu-devel] [Qemu-arm] [PATCH 6/7] target-arm: Implement NSACR trapping behaviour

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

On 5 February 2016 at 16:07, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Implement some corner cases of the behaviour of the NSACR
>> register on ARMv8:
>>  * if EL3 is AArch64 then accessing the NSACR from Secure EL1
>>    with AArch32 should trap to EL3
>>  * if EL3 is not present or is AArch64 then reads from NS EL1 and
>>    NS EL2 return constant 0xc00
>>
>> It would in theory be possible to implement all these with
>> a single reginfo definition, but for clarity we use three
>> separate definitions for the three cases and install the
>> right one based on the CPU feature flags.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target-arm/helper.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 8bc3ea3..34dc144 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3562,6 +3562,25 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>>      REGINFO_SENTINEL
>>  };
>>
>> +static CPAccessResult nsacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                                   bool isread)
>> +{
>> +    /* The NSACR is RW at EL3, and RO for NS EL1 and NS EL2.
>> +     * At Secure EL1 it traps to EL3.
>> +     */
>> +    if (arm_current_el(env) == 3) {
>> +        return CP_ACCESS_OK;
>> +    }
>> +    if (arm_is_secure_below_el3(env)) {
>> +        return CP_ACCESS_TRAP_EL3;
>> +    }
>> +    /* Accesses from EL1 NS and EL2 NS are UNDEF for write but allow reads. */
>> +    if (isread) {
>> +        return CP_ACCESS_OK;
>> +    }
>> +    return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +}
>> +
>>  static const ARMCPRegInfo el3_cp_reginfo[] = {
>>      { .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
>>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
>> @@ -3587,10 +3606,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 1,
>>        .access = PL3_RW, .resetvalue = 0,
>>        .fieldoffset = offsetoflow32(CPUARMState, cp15.sder) },
>> -      /* TODO: Implement NSACR trapping of secure EL1 accesses to EL3 */
>> -    { .name = "NSACR", .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
>> -      .access = PL3_W | PL1_R, .resetvalue = 0,
>> -      .fieldoffset = offsetof(CPUARMState, cp15.nsacr) },
>>      { .name = "MVBAR", .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
>>        .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
>>        .writefn = vbar_write, .resetvalue = 0,
>> @@ -4361,6 +4376,45 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>          };
>>          define_one_arm_cp_reg(cpu, &rvbar);
>>      }
>> +    /* The behaviour of NSACR is sufficiently various that we don't
>> +     * try to describe it in a single reginfo:
>> +     *  if EL3 is 64 bit, then trap to EL3 from S EL1,
>> +     *     reads as constant 0xc00 from NS EL1 and NS EL2
>> +     *  if EL3 is 32 bit, then RW at EL3, RO at NS EL1 and NS EL2
>> +     *  if v7 without EL3, register doesn't exist
>> +     *  if v8 without EL3, reads as constant 0xc00 from NS EL1 and NS EL2
>> +     */
>> +    if (arm_feature(env, ARM_FEATURE_EL3)) {
>> +        if (arm_feature(env, ARM_FEATURE_AARCH64)) {
>> +            ARMCPRegInfo nsacr = {
>> +                .name = "NSACR", .type = ARM_CP_CONST,
>
> I was thrown by the ARM_CP_CONST here as there is EL dependency. If
> nsacr_access says CP_ACCESS_OK where does the write go?

nsacr_access can never say OK for a write here (because we can't
be at EL3 since it's a 32-bit register and this is in the
"EL3 is 64-bit" arm of the if). The register is genuinely
constant.

>> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
>> +                .access = PL1_RW, .accessfn = nsacr_access,
>
> PL1_R?

No, because the .access checks happen before the access fn. If
we forbid writes in .access then a write from Secure EL1 will
fail UNDEF rather than trap to EL3. So instead we set a wider
permission set in .access and use .accessfn to get the exact
checks we need.

>> +                .resetvalue = 0xc00
>> +            };
>> +            define_one_arm_cp_reg(cpu, &nsacr);
>> +        } else {
>> +            ARMCPRegInfo nsacr = {
>> +                .name = "NSACR",
>> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
>> +                .access = PL3_RW | PL1_R,
>> +                .resetvalue = 0,
>> +                .fieldoffset = offsetof(CPUARMState, cp15.nsacr)
>> +            };
>> +            define_one_arm_cp_reg(cpu, &nsacr);
>> +        }
>> +    } else {
>> +        if (arm_feature(env, ARM_FEATURE_V8)) {
>> +            ARMCPRegInfo nsacr = {
>> +                .name = "NSACR", .type = ARM_CP_CONST,
>> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
>> +                .access = PL1_R,
>> +                .resetvalue = 0xc00
>> +            };
>> +            define_one_arm_cp_reg(cpu, &nsacr);
>> +        }
>> +    }
>> +
>>      if (arm_feature(env, ARM_FEATURE_MPU)) {
>>          if (arm_feature(env, ARM_FEATURE_V6)) {
>>              /* PMSAv6 not implemented */
>
> I don't know if a more compact definition would make this easier to
> follow?

>     /* The behaviour of NSACR is sufficiently various we tweak the reginfo:
>      *  if EL3 is 64 bit, then trap to EL3 from S EL1,
>      *     reads as constant 0xc00 from NS EL1 and NS EL2
>      *  if EL3 is 32 bit, then RW at EL3, RO at NS EL1 and NS EL2
>      *  if v7 without EL3, register doesn't exist
>      *  if v8 without EL3, reads as constant 0xc00 from NS EL1 and NS EL2
>      */
>     if (arm_feature(env, ARM_FEATURE_V8) || arm_feature(env, ARM_FEATURE_EL3)) {
>         ARMCPRegInfo nsacr = {
>             .name = "NSACR",
>             .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
>             .access = PL1_R,
>             .resetvalue = 0xc00
>         };
>
>         if (arm_feature(env, ARM_FEATURE_EL3)) {
>             if (arm_feature(env, ARM_FEATURE_AARCH64)) {
>                 nsacr.type = ARM_CP_CONST;      /* if no trap RO */
>                 nsacr.accessfn = nsacr_access;
>             } else {
>                 nsacr.access = PL3_RW | PL1_R;
>                 nsacr.resetvalue = 0;
>                 nsacr.fieldoffset = offsetof(CPUARMState, cp15.nsacr);
>             };
>         } else {
>             nsacr.type = ARM_CP_CONST;
>         }
>
>         define_one_arm_cp_reg(cpu, &nsacr);
>     }

I thought about this, but decided it was worse, because now you have
to read the whole dozen lines or so to figure out what the register
is defined as for each of the three interesting cases, and mentally
reassemble the regdef.

thanks
-- PMM