Re: [PATCH v2 2/9] target/arm: Don't add all MIDR aliases for Cortex-R

[Peter Maydell]

On Mon, 18 Jul 2022 at 12:54, Tobias Roehmel <quic_trohmel@quicinc.com> wrote:
>
> From: Tobias Röhmel <quic_trohmel@quicinc.com>
>
> Cortex-R52 has the MPUIR register which has the same encoding
> has the MIDR alias with opc2=4. So we only add that alias
> when we are not realizing a Cortex-R.
>
> Signed-off-by: Tobias Röhmel <quic_trohmel@quicinc.com>
> ---
>  target/arm/helper.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 6457e6301c..03bdc3d149 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8189,9 +8189,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .fieldoffset = offsetof(CPUARMState, cp15.c0_cpuid),
>                .readfn = midr_read },
>              /* crn = 0 op1 = 0 crm = 0 op2 = 4,7 : AArch32 aliases of MIDR */

Moving the op2=4 register definition makes this comment out of date,
so you need to update it (ie remove the '4' here, and add a comment
similarly noting what id_v8_midr_alias_cp_reginfo[] is doing).

> -            { .name = "MIDR", .type = ARM_CP_ALIAS | ARM_CP_CONST,
> -              .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
> -              .access = PL1_R, .resetvalue = cpu->midr },
>              { .name = "MIDR", .type = ARM_CP_ALIAS | ARM_CP_CONST,
>                .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 7,
>                .access = PL1_R, .resetvalue = cpu->midr },
> @@ -8201,6 +8198,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .accessfn = access_aa64_tid1,
>                .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
>          };
> +        ARMCPRegInfo id_v8_midr_alias_cp_reginfo[] = {
> +            { .name = "MIDR", .type = ARM_CP_ALIAS | ARM_CP_CONST,
> +              .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
> +              .access = PL1_R, .resetvalue = cpu->midr },
> +        };

If there's only one register, you don't need to define a
single-element array, you can use define_one_arm_cp_reg()
and pass it a single ARMCPRegInfo struct.

>          ARMCPRegInfo id_cp_reginfo[] = {
>              /* These are common to v8 and pre-v8 */
>              { .name = "CTR",
> @@ -8264,8 +8266,12 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              id_mpuir_reginfo.access = PL1_RW;
>              id_tlbtr_reginfo.access = PL1_RW;
>          }
> +
>          if (arm_feature(env, ARM_FEATURE_V8)) {
>              define_arm_cp_regs(cpu, id_v8_midr_cp_reginfo);
> +            if (!arm_feature(env, ARM_FEATURE_V8_R)) {

You can use ARM_FEATURE_PMSA here.

> +                define_arm_cp_regs(cpu, id_v8_midr_alias_cp_reginfo);
> +            }
>          } else {
>              define_arm_cp_regs(cpu, id_pre_v8_midr_cp_reginfo);
>          }
> --

thanks
-- PMM