Re: [PATCH v5 5/7] target/arm: Add PMSAv8r registers

[Peter Maydell]

On Sun, 27 Nov 2022 at 13:21, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>

Only some minor fixes needed in this one.

> ---
>  target/arm/cpu.c     |  24 +++-
>  target/arm/cpu.h     |   6 +
>  target/arm/helper.c  | 299 +++++++++++++++++++++++++++++++++++++++++++
>  target/arm/machine.c |  28 ++++
>  4 files changed, 356 insertions(+), 1 deletion(-)

> @@ -2001,8 +2009,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>       */
>      if (!cpu->has_mpu) {
>          cpu->pmsav7_dregion = 0;
> +        cpu->pmsav8r_hdregion = 0;
>      }
> -    if (cpu->pmsav7_dregion == 0) {
> +    if ((cpu->pmsav7_dregion == 0) && (cpu->pmsav8r_hdregion == 0)) {
>          cpu->has_mpu = false;
>      }

This leaves the door open for a CPU with 0 dregions but some hdregions.
That I think doesn't make sense, so we should just disallow it.
We can do that by combining these two if()s into one:

      if (!cpu->has_mpu || cpu->pmsav7_dregion == 0) {
          cpu->has_mpu = false;
          cpu->pmsav7_dregion = 0;
          cpu->pmsav8r_hdregion = 0;
      }

> @@ -2030,6 +2039,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>                  env->pmsav7.dracr = g_new0(uint32_t, nr);
>              }
>          }
> +
> +        if (cpu->pmsav8r_hdregion > 0xFF) {

Lowercase for hex digits, please (matches the check on pmsav7_dregion).

> +            error_setg(errp, "PMSAv8 MPU EL2 #regions invalid %" PRIu32,
> +                              cpu->pmsav8r_hdregion);
> +            return;
> +        }
> +
> +        if (cpu->pmsav8r_hdregion) {
> +            env->pmsav8.hprbar = g_new0(uint32_t,
> +                                        cpu->pmsav8r_hdregion);
> +            env->pmsav8.hprlar = g_new0(uint32_t,
> +                                        cpu->pmsav8r_hdregion);
> +        }
>      }
>
>      if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {



> +static const ARMCPRegInfo pmsav8r_cp_reginfo[] = {
> +    { .name = "PRBAR",
> +      .cp = 15, .opc1 = 0, .crn = 6, .crm = 3, .opc2 = 0,
> +      .access = PL1_RW, .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
> +      .accessfn = access_tvm_trvm,
> +      .readfn = prbar_read, .writefn = prbar_write },
> +    { .name = "PRLAR",
> +      .cp = 15, .opc1 = 0, .crn = 6, .crm = 3, .opc2 = 1,
> +      .access = PL1_RW, .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
> +      .accessfn = access_tvm_trvm,
> +      .readfn = prlar_read, .writefn = prlar_write },
> +    { .name = "PRSELR", .resetvalue = 0,
> +      .cp = 15, .opc1 = 0, .crn = 6, .crm = 2, .opc2 = 1,
> +      .access = PL1_RW, .accessfn = access_tvm_trvm,
> +      .writefn = prselr_write,
> +      .fieldoffset = offsetof(CPUARMState, pmsav7.rnr[M_REG_NS]) },
> +    { .name = "HPRBAR", .resetvalue = 0,
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 3, .opc2 = 0,
> +      .access = PL2_RW, .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
> +      .readfn = hprbar_read, .writefn = hprbar_write },
> +    { .name = "HPRLAR",
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 3, .opc2 = 1,
> +      .access = PL2_RW, .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
> +      .readfn = hprlar_read, .writefn = hprlar_write },

These 4 registers should be just ARM_CP_NO_RAW, not
ARM_CP_ALIAS | ARM_CP_NO_RAW.

> +    { .name = "HPRSELR", .resetvalue = 0,
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 2, .opc2 = 1,
> +      .access = PL2_RW,
> +      .writefn = hprselr_write,
> +      .fieldoffset = offsetof(CPUARMState, pmsav8.hprselr) },
> +    { .name = "HPRENR",
> +      .cp = 15, .opc1 = 4, .crn = 6, .crm = 1, .opc2 = 1,
> +      .access = PL2_RW, .type = ARM_CP_ALIAS,
> +      .readfn = hprenr_read, .writefn = hprenr_write },
> +};
> +
>  static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
>      /* Reset for all these registers is handled in arm_cpu_reset(),
>       * because the PMSAv7 is also used by M-profile CPUs, which do
> @@ -8166,6 +8382,13 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .access = PL1_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->pmsav7_dregion << 8
>          };
> +        /* HMPUIR is specific to PMSA V8 */
> +        ARMCPRegInfo id_hmpuir_reginfo = {
> +            .name = "HMPUIR",
> +            .cp = 15, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 4,
> +            .access = PL2_R, .type = ARM_CP_CONST,
> +            .resetvalue = cpu->pmsav8r_hdregion
> +        };
>          static const ARMCPRegInfo crn0_wi_reginfo = {
>              .name = "CRN0_WI", .cp = 15, .crn = 0, .crm = CP_ANY,
>              .opc1 = CP_ANY, .opc2 = CP_ANY, .access = PL1_W,
> @@ -8208,6 +8431,71 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_arm_cp_regs(cpu, id_cp_reginfo);
>          if (!arm_feature(env, ARM_FEATURE_PMSA)) {
>              define_one_arm_cp_reg(cpu, &id_tlbtr_reginfo);
> +        } else if (arm_feature(env, ARM_FEATURE_PMSA) &&
> +                   arm_feature(env, ARM_FEATURE_V8)) {
> +            uint32_t i = 0;
> +            g_autofree char *tmp_string_pr;
> +            g_autofree char *tmp_string_hpr;
> +
> +            define_one_arm_cp_reg(cpu, &id_mpuir_reginfo);
> +            define_one_arm_cp_reg(cpu, &id_hmpuir_reginfo);
> +            define_arm_cp_regs(cpu, pmsav8r_cp_reginfo);
> +
> +            /* Register alias is only valid for first 32 indexes */
> +            for (i = 0; i < MIN(cpu->pmsav7_dregion, 32); ++i) {
> +                uint8_t crm = 0b1000 | extract32(i, 1, 3);
> +                uint8_t opc1 = extract32(i, 4, 1);
> +                uint8_t opc2 = extract32(i, 0, 1) << 2;
> +
> +                tmp_string_pr = g_strdup_printf("PRBAR%u", i);
> +                ARMCPRegInfo tmp_prbarn_reginfo = {
> +                    .name = tmp_string_pr, .type = ARM_CP_ALIAS | 
> ARM_CP_NO_RAW,

All these registers hould be just ARM_CP_NO_RAW, not
ARM_CP_ALIAS | ARM_CP_NO_RAW.

> +                    .cp = 15, .opc1 = opc1, .crn = 6, .crm = crm, .opc2 = 
> opc2,
> +                    .access = PL1_RW, .resetvalue = 0,
> +                    .accessfn = access_tvm_trvm,
> +                    .writefn = pmsav8r_regn_write, .readfn = 
> pmsav8r_regn_read
> +                };
> +                define_one_arm_cp_reg(cpu, &tmp_prbarn_reginfo);

This is still leaking the name strings. If you don't want to
use g_autofree then you must free the strings yourself.

> +
> +                opc2 = extract32(i, 0, 1) << 2 | 0x1;
> +                tmp_string_pr = g_strdup_printf("PRLAR%u", i);
> +                ARMCPRegInfo tmp_prlarn_reginfo = {
> +                    .name = tmp_string_pr, .type = ARM_CP_ALIAS | 
> ARM_CP_NO_RAW,
> +                    .cp = 15, .opc1 = opc1, .crn = 6, .crm = crm, .opc2 = 
> opc2,
> +                    .access = PL1_RW, .resetvalue = 0,
> +                    .accessfn = access_tvm_trvm,
> +                    .writefn = pmsav8r_regn_write, .readfn = 
> pmsav8r_regn_read
> +                };
> +                define_one_arm_cp_reg(cpu, &tmp_prlarn_reginfo);
> +            }
> +
> +            /* Register alias is only valid for first 32 indexes */
> +            for (i = 0; i < MIN(cpu->pmsav8r_hdregion, 32); ++i) {
> +                uint8_t crm = 0b1000 | extract32(i, 1, 3);
> +                uint8_t opc1 = 0b100 | extract32(i, 4, 1);
> +                uint8_t opc2 = extract32(i, 0, 1) << 2;
> +
> +                tmp_string_hpr = g_strdup_printf("HPRBAR%u", i);
> +                ARMCPRegInfo tmp_hprbarn_reginfo = {
> +                    .name = tmp_string_hpr,
> +                    .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
> +                    .cp = 15, .opc1 = opc1, .crn = 6, .crm = crm, .opc2 = 
> opc2,
> +                    .access = PL2_RW, .resetvalue = 0,
> +                    .writefn = pmsav8r_regn_write, .readfn = 
> pmsav8r_regn_read
> +                };
> +                define_one_arm_cp_reg(cpu, &tmp_hprbarn_reginfo);
> +
> +                opc2 = extract32(i, 0, 1) << 2 | 0x1;
> +                tmp_string_hpr = g_strdup_printf("HPRLAR%u", i);
> +                ARMCPRegInfo tmp_hprlarn_reginfo = {
> +                    .name = tmp_string_hpr,
> +                    .type = ARM_CP_ALIAS | ARM_CP_NO_RAW,
> +                    .cp = 15, .opc1 = opc1, .crn = 6, .crm = crm, .opc2 = 
> opc2,
> +                    .access = PL2_RW, .resetvalue = 0,
> +                    .writefn = pmsav8r_regn_write, .readfn = 
> pmsav8r_regn_read
> +                };
> +                define_one_arm_cp_reg(cpu, &tmp_hprlarn_reginfo);
> +            }
>          } else if (arm_feature(env, ARM_FEATURE_V7)) {
>              define_one_arm_cp_reg(cpu, &id_mpuir_reginfo);
>          }

thanks
-- PMM