Re: [PATCH 1/2] target/arm: Add Neoverse-N1 registers

[Chen Baozi]

Hi Peter,

> On Mar 6, 2023, at 19:33, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Fri, 3 Mar 2023 at 16:15, Chen Baozi <chenbaozi@phytium.com.cn> wrote:
>> 
>> Add implementation defined registers for neoverse-n1 which
>> would be accessed by TF-A.
>> 
>> Signed-off-by: Chen Baozi <chenbaozi@phytium.com.cn>
>> ---
>> target/arm/cpu64.c     |  2 ++
>> target/arm/cpu_tcg.c   | 62 ++++++++++++++++++++++++++++++++++++++++++
>> target/arm/internals.h |  2 ++
>> 3 files changed, 66 insertions(+)
>> 
>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>> index 4066950da1..a6ae7cafac 100644
>> --- a/target/arm/cpu64.c
>> +++ b/target/arm/cpu64.c
>> @@ -1094,6 +1094,8 @@ static void aarch64_neoverse_n1_initfn(Object *obj)
>> 
>>     /* From D5.1 AArch64 PMU register summary */
>>     cpu->isar.reset_pmcr_el0 = 0x410c3000;
>> +
>> +    define_neoverse_n1_cp_reginfo(cpu);
>> }
>> 
>> static void aarch64_host_initfn(Object *obj)
>> diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
>> index df0c45e523..6a1bb56b25 100644
>> --- a/target/arm/cpu_tcg.c
>> +++ b/target/arm/cpu_tcg.c
>> @@ -150,6 +150,68 @@ void define_cortex_a72_a57_a53_cp_reginfo(ARMCPU *cpu)
>> {
>>     define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
>> }
>> +
>> +static const ARMCPRegInfo neoverse_n1_cp_reginfo[] = {
>> +    { .name = "ATCR_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 7, .opc2 = 0,
>> +      .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "ATCR_EL2", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 4, .crn = 15, .crm = 7, .opc2 = 0,
>> +      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "ATCR_EL3", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 7, .opc2 = 0,
>> +      .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "ATCR_EL12", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 5, .crn = 15, .crm = 7, .opc2 = 0,
>> +      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "AVTCR_EL2", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 4, .crn = 15, .crm = 7, .opc2 = 1,
>> +      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "CPUACTLR_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 1, .opc2 = 0,
>> +      .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "CPUACTLR2_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 1, .opc2 = 1,
>> +      .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "CPUACTLR3_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 1, .opc2 = 2,
>> +      .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "CPUCFR_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 0, .opc2 = 0,
>> +      .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> 
> .PL1_R -- this register is read-only.
> 
> If we report bit 2 as 1 ("the DSU SCU is not present"), does
> TF-A pay attention to that and not try to access the DSU
> related registers you define in patch 2 ? If so, it would
> probably be nicer to say "no DSU" and not have to define
> dummy registers for it...

Aha! Yes, TF-A does have a function “is_scu_present_in_dsu" to check this bit.

I’ll work another version to fix them all (as well as the following issues).

Thanks.

Baozi.

> 
>> +    { .name = "CPUECTLR_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 1, .opc2 = 4,
>> +      .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0x961563010 },
>> +    { .name = "CPUPCR_EL3", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 8, .opc2 = 1,
>> +      .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "CPUPMR_EL3", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 8, .opc2 = 3,
>> +      .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "CPUPOR_EL3", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 8, .opc2 = 2,
>> +      .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "CPUPSELR_EL3", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 8, .opc2 = 0,
>> +      .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "CPUPWRCTLR_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 7,
>> +      .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "ERXPFGCDN_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 2,
>> +      .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> 
> Shouldn't this be PL1_RW ?
> 
>> +    { .name = "ERXPFGCTL_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 1,
>> +      .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> 
> Also PL1_RW.
> 
>> +    { .name = "ERXPFGF_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 0,
>> +      .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +};
>> +
>> +void define_neoverse_n1_cp_reginfo(ARMCPU *cpu)
>> +{
>> +    define_arm_cp_regs(cpu, neoverse_n1_cp_reginfo);
>> +}
>> #endif /* !CONFIG_USER_ONLY */
> 
> thanks
> -- PMM