Re: [PATCH v3 7/9] target/arm: Add PMSAv8r registers

[Peter Maydell]

On Sat, 20 Aug 2022 at 15:19, <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>
> ---
>  target/arm/cpu.h    |  10 +++
>  target/arm/helper.c | 171 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 181 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 86e06116a9..632d0d13c6 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -726,8 +726,18 @@ typedef struct CPUArchState {
>           */
>          uint32_t *rbar[M_REG_NUM_BANKS];
>          uint32_t *rlar[M_REG_NUM_BANKS];
> +        uint32_t prbarn[255];
> +        uint32_t prlarn[255];
> +        uint32_t hprbarn[255];
> +        uint32_t hprlarn[255];

Don't use magic constants, please. In fact, don't use
fixed size arrays at all here. The v8R PRBAR/PRLAR
register arrays are exactly the same format as the v8M
pmsav8.rbar[] and pmsav8.rlar[], so please use the same
state fields that does. (You'll need to add equivalent
new arrays to handle the HPRBAR/HPRLAR.)

>          uint32_t mair0[M_REG_NUM_BANKS];
>          uint32_t mair1[M_REG_NUM_BANKS];
> +        uint32_t prbar;
> +        uint32_t prlar;

You should not need separate prbar/prlar fields, as those
registers only indirectly access the state for thecurrently
selected region. Similarly hprbar, hprlar below.

> +        uint32_t prselr;

PRSELR is just a renamed PMSAv7 RGNR, for which we already
have a state field, pmsav7.rnr[M_REG_NS] (and indeed a cpreg
struct).

> +        uint32_t hprbar;
> +        uint32_t hprlar;
> +        uint32_t hprselr;
>      } pmsav8;

Some of this new state must be handled for migration.
Where state is directly accessed via a coprocessor
register that will be migrated for you. However, where
there is state that is not directly accessible, i.e. for
the rbar/rlar arrays, you need to add code/vmstate structs
to target/arm/machine.c to migrate them. vmstate_pmsav8
already does this for rbar and rlar, but you'll need to
add something similar for the hyp versions. (Watch out that
you maintain migration compat for the existing CPUs -- you
can't just add new fields to existing VMStateDescription
structs. Ask if you need advice.)

The arrays will also need explicit handling in reset.
Again, look at how PMSAv7 is doing it.

>      /* v8M SAU */
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 23461397e0..1730383f28 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7422,6 +7422,78 @@ static CPAccessResult access_joscr_jmcr(CPUARMState 
> *env,
>      return CP_ACCESS_OK;
>  }
>
> +static void prbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                          uint64_t value)
> +{
> +    env->pmsav8.prbarn[env->pmsav8.prselr] = value;
> +}
> +
> +static void prlar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                          uint64_t value)
> +{
> +    env->pmsav8.prlarn[env->pmsav8.prselr] = value;
> +}

For writes you will need to do some kind of tlb flush.
Compare pmsav7_write().

> +
> +static uint64_t prbar_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    return env->pmsav8.prbarn[env->pmsav8.prselr];
> +}
> +
> +static uint64_t prlar_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    return env->pmsav8.prlarn[env->pmsav8.prselr];
> +}
> +
> +static void hprbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                          uint64_t value)
> +{
> +    env->pmsav8.hprbarn[env->pmsav8.hprselr] = value;
> +}
> +
> +static void hprlar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                          uint64_t value)
> +{
> +    env->pmsav8.hprlarn[env->pmsav8.hprselr] = value;
> +}
> +
> +static void hprenr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                          uint64_t value)
> +{
> +    uint32_t n;
> +    ARMCPU *cpu = env_archcpu(env);
> +    for (n = 0; n < (int)cpu->pmsav7_dregion; ++n) {

What's the cast for here ?

The architecture allows EL1 and EL2 MPUs to have different
numbers of regions, so this loop bound shouldn't be on
pmsav7_dregion, which is (I assume) the number of
EL1 MPU regions.

You need to also bound n to less than 32, to avoid
undefined behaviour.

> +        if (value & (1 << n)) {
> +            env->pmsav8.hprlarn[n] |= 0x1;
> +        } else {
> +            env->pmsav8.hprlarn[n] &= (~0x1);

Brackets unnecessary.

> +        }

Consider replacing this if() with

       bit = extract32(value, n, 1);
       env->pmsav8.hprlarn[n] = deposit32(env->pmsav8.hprlarn[n], 0, 1, bit);

> +    }
> +}
> +
> +static uint64_t hprbar_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    return env->pmsav8.hprbarn[env->pmsav8.hprselr];
> +}
> +
> +static uint64_t hprlar_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    return env->pmsav8.hprlarn[env->pmsav8.hprselr];
> +}
> +
> +static uint64_t hprenr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    uint32_t n;
> +    uint32_t result = 0x0;
> +    ARMCPU *cpu = env_archcpu(env);
> +
> +    for (n = 0; n < (int)cpu->pmsav7_dregion; ++n) {

> +        if (env->pmsav8.hprlarn[n] & 0x1) {
> +            result |= (0x1 << n);
> +        }
> +    }
> +    return result;
> +}
> +
>  static const ARMCPRegInfo jazelle_regs[] = {
>      { .name = "JIDR",
>        .cp = 14, .crn = 0, .crm = 0, .opc1 = 7, .opc2 = 0,
> @@ -8249,6 +8321,46 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .access = PL1_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->pmsav7_dregion << 8
>          };
> +        /* PMSAv8-R registers*/
> +        ARMCPRegInfo id_pmsav8_r_reginfo[] = {
> +            { .name = "HMPUIR",
> +              .cp = 15, .crn = 0, .crm = 0, .opc1 = 4, .opc2 = 4,
> +              .access = PL2_R, .type = ARM_CP_CONST,
> +              .resetvalue = cpu->pmsav7_dregion},
> +             /* PMSAv8-R registers */
> +            { .name = "PRBAR",
> +              .cp = 15, .opc1 = 0, .crn = 6, .crm = 3, .opc2 = 0,
> +              .access = PL1_RW, .resetvalue = 0,
> +              .readfn = prbar_read, .writefn = prbar_write,
> +              .fieldoffset = offsetof(CPUARMState, pmsav8.prbar)},
> +            { .name = "PRLAR",
> +              .cp = 15, .opc1 = 0, .crn = 6, .crm = 3, .opc2 = 1,
> +              .access = PL1_RW, .resetvalue = 0,
> +              .readfn = prlar_read, .writefn = prlar_write,
> +              .fieldoffset = offsetof(CPUARMState, pmsav8.prlar)},
> +            { .name = "PRSELR", .resetvalue = 0,
> +              .cp = 15, .opc1 = 0, .crn = 6, .crm = 2, .opc2 = 1,
> +              .access = PL1_RW, .accessfn = access_tvm_trvm,
> +              .fieldoffset = offsetof(CPUARMState, pmsav8.prselr)},
> +            { .name = "HPRBAR", .resetvalue = 0,
> +              .readfn = hprbar_read, .writefn = hprbar_write,
> +              .cp = 15, .opc1 = 4, .crn = 6, .crm = 3, .opc2 = 0,
> +              .access = PL2_RW, .resetvalue = 0,
> +              .fieldoffset = offsetof(CPUARMState, pmsav8.hprbar)},
> +            { .name = "HPRLAR",
> +              .cp = 15, .opc1 = 4, .crn = 6, .crm = 3, .opc2 = 1,
> +              .access = PL2_RW, .resetvalue = 0,
> +              .readfn = hprlar_read, .writefn = hprlar_write,
> +              .fieldoffset = offsetof(CPUARMState, pmsav8.hprlar)},
> +            { .name = "HPRSELR", .resetvalue = 0,
> +              .cp = 15, .opc1 = 4, .crn = 6, .crm = 2, .opc2 = 1,
> +              .access = PL2_RW, .accessfn = access_tvm_trvm,
> +              .fieldoffset = offsetof(CPUARMState, pmsav8.hprselr)},
> +            { .name = "HPRENR",
> +              .cp = 15, .opc1 = 4, .crn = 6, .crm = 1, .opc2 = 1,
> +              .access = PL2_RW, .resetvalue = 0,
> +              .readfn = hprenr_read, .writefn = hprenr_write},
> +        };

Unless you need to write into some of the fields of this array
at runtime, make it a static const file-level global. (Compare
others, like eg pmsav7_cp_reginfo[].)

I think you are missing the VSCTLR register.

>          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,
> @@ -8292,6 +8404,65 @@ 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_V8_R)) {
> +            uint32_t i = 0;
> +            char hprbar_string[] = "HPRBAR%u";
> +            char hprlar_string[] = "HPRLAR%u";
> +
> +            char prbar_string[] = "PRBAR%u";
> +            char prlar_string[] = "PRLAR%u";
> +            char tmp_string[50];

Don't use fixed arrays and sprintf(), please, and don't define
the format string in a variable either. Look at eg the handling
of RES_0_C0_C%d_X -- use of g_autofree and g_strdup_printf() is
usually the clearest approach.

> +            define_one_arm_cp_reg(cpu, &id_mpuir_reginfo);
> +            define_arm_cp_regs(cpu, id_pmsav8_r_reginfo);
> +            for (i = 0; i < cpu->pmsav7_dregion; ++i) {

This needs to be bounded so it doesn't go above 31, because
only the first 32 regions get these aliases.

> +                uint8_t crm = 0b1000 | ((i & 0b1110) >> 1);
> +                uint8_t opc2 = (i & 0x1) << 2;
> +
> +                sprintf(tmp_string, hprbar_string, i);
> +                ARMCPRegInfo tmp_hprbarn_reginfo = {
> +                    .name = tmp_string,
> +                    .cp = 15, .opc1 = 4, .crn = 6, .crm = crm, .opc2 = opc2,
> +                    .access = PL2_RW, .resetvalue = 0,
> +                    .accessfn = access_tvm_trvm,
> +                    .fieldoffset = offsetof(CPUARMState, pmsav8.hprbarn)
> +                    + i * sizeof(env->pmsav8.hprbarn[0])
> +                };
> +                define_one_arm_cp_reg(cpu, &tmp_hprbarn_reginfo);
> +
> +                sprintf(tmp_string, prbar_string, i);
> +                ARMCPRegInfo tmp_prbarn_reginfo = {
> +                    .name = tmp_string,
> +                    .cp = 15, .opc1 = 0, .crn = 6, .crm = crm, .opc2 = opc2,
> +                    .access = PL1_RW, .resetvalue = 0,
> +                    .accessfn = access_tvm_trvm,
> +                    .fieldoffset = offsetof(CPUARMState, pmsav8.prbarn)
> +                    + i * sizeof(env->pmsav8.prbarn[0])
> +                };
> +                define_one_arm_cp_reg(cpu, &tmp_prbarn_reginfo);
> +
> +                opc2 = (i & 0x1) << 2 | 0x1;
> +                sprintf(tmp_string, hprlar_string, i);
> +                ARMCPRegInfo tmp_hprlarn_reginfo = {
> +                    .name = tmp_string,
> +                    .cp = 15, .opc1 = 4, .crn = 6, .crm = crm, .opc2 = opc2,
> +                    .access = PL2_RW, .resetvalue = 0,
> +                    .accessfn = access_tvm_trvm,
> +                    .fieldoffset = offsetof(CPUARMState, pmsav8.hprlarn)
> +                    + i * sizeof(env->pmsav8.hprlarn[0])
> +                };
> +                define_one_arm_cp_reg(cpu, &tmp_hprlarn_reginfo);
> +
> +                sprintf(tmp_string, prlar_string, i);
> +                ARMCPRegInfo tmp_prlarn_reginfo = {
> +                    .name = tmp_string,
> +                    .cp = 15, .opc1 = 0, .crn = 6, .crm = crm, .opc2 = opc2,
> +                    .access = PL1_RW, .resetvalue = 0,
> +                    .accessfn = access_tvm_trvm,
> +                    .fieldoffset = offsetof(CPUARMState, pmsav8.prlarn)
> +                    + i * sizeof(env->pmsav8.prlarn[0])
> +                };

These registers all need to be marked as ARM_CP_ALIAS,
because they're just aliases into a different register which is
handling the migration and reset.

> +                define_one_arm_cp_reg(cpu, &tmp_prlarn_reginfo);
> +            }
>          } else if (arm_feature(env, ARM_FEATURE_V7)) {
>              define_one_arm_cp_reg(cpu, &id_mpuir_reginfo);
>          }
> --

thanks
-- PMM