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];
uint32_t mair0[M_REG_NUM_BANKS];
uint32_t mair1[M_REG_NUM_BANKS];
+ uint32_t prbar;
+ uint32_t prlar;
+ uint32_t prselr;
+ uint32_t hprbar;
+ uint32_t hprlar;
+ uint32_t hprselr;
} pmsav8;
/* 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;
+}
+
+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) {
+ if (value & (1 << n)) {
+ env->pmsav8.hprlarn[n] |= 0x1;
+ } else {
+ env->pmsav8.hprlarn[n] &= (~0x1);
+ }
+ }
+}
+
+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},
+ };
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];
+ 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) {
+ 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])
+ };
+ 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);
}
--
2.25.1
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
Thank you very much for the review!
I have a few questions:
On 27.09.22 15:50, Peter Maydell wrote:
> 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.)
Is there a way to find out whether I'm in secure mode while accessing a
co-processor register?
The banks for rbar etc. are used to model the secure non-secure banks.
The access mechanism
here is via a device which allows the use of the mmu index to find out
if we are in secure mode.
I'm struggling to find a good solution in the co-processor register case.
>
>> 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).
I changed it to use the rnr field, but I think I can't reuse the cpreg
since it has a different encoding.
>
>> + 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.)
I need some help here. Do I need new subsections?
>
> 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
On Sat, 15 Oct 2022 at 14:07, Tobias Roehmel
<tobias.roehmel@rwth-aachen.de> wrote:
>
> Thank you very much for the review!
>
> I have a few questions:
>
> On 27.09.22 15:50, Peter Maydell wrote:
> > 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.)
>
> Is there a way to find out whether I'm in secure mode while accessing a
> co-processor register?
> The banks for rbar etc. are used to model the secure non-secure banks.
> The access mechanism
> here is via a device which allows the use of the mmu index to find out
> if we are in secure mode.
> I'm struggling to find a good solution in the co-processor register case.
Well, for 32-bit v8 R-profile you know you're always NonSecure.
But more generally, the right way depends on where you are.
Code in ptw.c now gets passed a bool to tell it whether it
needs to do the address translation for a secure or nonsecure
translation regime (this is a change from a recent refactoring).
Otherwise there is the arm_is_secure() function.
For the coprocessor register access functions, just do what
pmsav7 does' and hardcode that the array element you want is
the M_REG_NS one.
> >
> >> 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).
> I changed it to use the rnr field, but I think I can't reuse the cpreg
> since it has a different encoding.
Oops, yes, I misread the opc2 value.
> >
> >> + 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.)
> I need some help here. Do I need new subsections?
That's generally the easiest way, yes. You can make it
a subsection of vmstate_pmsav8. the way subsections work is
that they appear in a .subsections field of some other
vmstate. They need a .needed function which returns true when
the new fields in the subsection must be migrated, and false
otherwise (such that any already existing CPU returns false
for the .needed function). That way the migration code will
migrate the new CPU struct fields for the new CPU, but won't
expect or send them for an old one.
As an example, look at vmstate_m_mve, which migrates a couple
of fields which are used only by M-profile CPUs with the MVE
feature. The vmstate_m_mve struct is listed in the .subsections
list of the vmstate_m struct (this is the VMStateDescription that
handles general M-profile CPU stuff), and the mve_needed() function
returns true only for CPUs with MVE. Note that the .name of
the subsection has to match up with the .name of its parent
section, so if the parent section's name is "parent/name"
then the subsection has to be "parent/name/whatever".
thanks
-- PMM
© 2016 - 2026 Red Hat, Inc.