* [PATCH v4 1/7] target/arm: Don't add all MIDR aliases for cores that immplement PMSA
2022-10-23 15:36 [PATCH v4 0/7] Add ARM Cortex-R52 CPU tobias.roehmel
@ 2022-10-23 15:36 ` tobias.roehmel
2022-10-23 23:06 ` Richard Henderson
2022-10-23 15:36 ` [PATCH v4 2/7] target/arm: Make RVBAR available for all ARMv8 CPUs tobias.roehmel
` (6 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: tobias.roehmel @ 2022-10-23 15:36 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel
From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
Cores with PMSA have the MPUIR register which has the
same encoding as the MIDR alias with opc2=4. So we only
add that alias if we are not realizing a core that
implements PMSA.
Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
---
target/arm/helper.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index db3b1ea72d..3c517356e1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8025,10 +8025,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
.access = PL1_R, .type = ARM_CP_NO_RAW, .resetvalue = cpu->midr,
.fieldoffset = offsetof(CPUARMState, cp15.c0_cpuid),
.readfn = midr_read },
- /* crn = 0 op1 = 0 crm = 0 op2 = 4,7 : AArch32 aliases of MIDR */
- { .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 },
+ /* crn = 0 op1 = 0 crm = 0 op2 = 7 : AArch32 aliases of 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 },
@@ -8038,6 +8035,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
+ };
ARMCPRegInfo id_cp_reginfo[] = {
/* These are common to v8 and pre-v8 */
{ .name = "CTR",
@@ -8101,8 +8103,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_PMSA)) {
+ define_one_arm_cp_reg(cpu, &id_v8_midr_alias_cp_reginfo);
+ }
} else {
define_arm_cp_regs(cpu, id_pre_v8_midr_cp_reginfo);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 1/7] target/arm: Don't add all MIDR aliases for cores that immplement PMSA
2022-10-23 15:36 ` [PATCH v4 1/7] target/arm: Don't add all MIDR aliases for cores that immplement PMSA tobias.roehmel
@ 2022-10-23 23:06 ` Richard Henderson
0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2022-10-23 23:06 UTC (permalink / raw)
To: tobias.roehmel, qemu-devel; +Cc: peter.maydell
On 10/24/22 01:36, tobias.roehmel@rwth-aachen.de wrote:
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
Typo "implement" in subject.
> @@ -8038,6 +8035,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
> + };
Indentation is off: 6 spaces instead of 4 at this level.
Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 2/7] target/arm: Make RVBAR available for all ARMv8 CPUs
2022-10-23 15:36 [PATCH v4 0/7] Add ARM Cortex-R52 CPU tobias.roehmel
2022-10-23 15:36 ` [PATCH v4 1/7] target/arm: Don't add all MIDR aliases for cores that immplement PMSA tobias.roehmel
@ 2022-10-23 15:36 ` tobias.roehmel
2022-11-14 17:04 ` Peter Maydell
2022-10-23 15:36 ` [PATCH v4 3/7] target/arm: Make stage_2_format for cache attributes optional tobias.roehmel
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: tobias.roehmel @ 2022-10-23 15:36 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel
From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
RVBAR shadows RVBAR_ELx where x is the highest exception
level if the highest EL is not EL3. This patch also allows
ARMv8 CPUs to change the reset address with
the rvbar property.
Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
---
target/arm/cpu.c | 6 +++++-
target/arm/helper.c | 23 +++++++++++++++--------
2 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 94ca6f163f..b642749d6d 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -281,6 +281,10 @@ static void arm_cpu_reset(DeviceState *dev)
env->cp15.cpacr_el1 = FIELD_DP64(env->cp15.cpacr_el1,
CPACR, CP11, 3);
#endif
+ if (arm_feature(env, ARM_FEATURE_V8)) {
+ env->cp15.rvbar = cpu->rvbar_prop;
+ env->regs[15] = cpu->rvbar_prop;
+ }
}
#if defined(CONFIG_USER_ONLY)
@@ -1306,7 +1310,7 @@ void arm_cpu_post_init(Object *obj)
qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_hivecs_property);
}
- if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+ if (arm_feature(&cpu->env, ARM_FEATURE_V8)) {
object_property_add_uint64_ptr(obj, "rvbar",
&cpu->rvbar_prop,
OBJ_PROP_FLAG_READWRITE);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 3c517356e1..2e9e420d4e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7768,8 +7768,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
if (!arm_feature(env, ARM_FEATURE_EL3) &&
!arm_feature(env, ARM_FEATURE_EL2)) {
ARMCPRegInfo rvbar = {
- .name = "RVBAR_EL1", .state = ARM_CP_STATE_AA64,
- .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
+ .name = "RVBAR_EL1", .state = ARM_CP_STATE_BOTH,
+ .opc0 = 3, .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
.access = PL1_R,
.fieldoffset = offsetof(CPUARMState, cp15.rvbar),
};
@@ -7859,13 +7859,20 @@ void register_cp_regs_for_features(ARMCPU *cpu)
}
/* RVBAR_EL2 is only implemented if EL2 is the highest EL */
if (!arm_feature(env, ARM_FEATURE_EL3)) {
- ARMCPRegInfo rvbar = {
- .name = "RVBAR_EL2", .state = ARM_CP_STATE_AA64,
- .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 1,
- .access = PL2_R,
- .fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+ ARMCPRegInfo rvbar[] = {
+ {
+ .name = "RVBAR_EL2", .state = ARM_CP_STATE_AA64,
+ .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 1,
+ .access = PL2_R,
+ .fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+ },
+ { .name = "RVBAR", .type = ARM_CP_ALIAS,
+ .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
+ .access = PL2_R,
+ .fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+ },
};
- define_one_arm_cp_reg(cpu, &rvbar);
+ define_arm_cp_regs(cpu, rvbar);
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/7] target/arm: Make RVBAR available for all ARMv8 CPUs
2022-10-23 15:36 ` [PATCH v4 2/7] target/arm: Make RVBAR available for all ARMv8 CPUs tobias.roehmel
@ 2022-11-14 17:04 ` Peter Maydell
0 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2022-11-14 17:04 UTC (permalink / raw)
To: tobias.roehmel; +Cc: qemu-devel
On Sun, 23 Oct 2022 at 16:37, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> RVBAR shadows RVBAR_ELx where x is the highest exception
> level if the highest EL is not EL3. This patch also allows
> ARMv8 CPUs to change the reset address with
> the rvbar property.
>
> Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 3c517356e1..2e9e420d4e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7768,8 +7768,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> if (!arm_feature(env, ARM_FEATURE_EL3) &&
> !arm_feature(env, ARM_FEATURE_EL2)) {
> ARMCPRegInfo rvbar = {
> - .name = "RVBAR_EL1", .state = ARM_CP_STATE_AA64,
> - .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
> + .name = "RVBAR_EL1", .state = ARM_CP_STATE_BOTH,
> + .opc0 = 3, .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
You don't need to specify .cp for a STATE_BOTH register: 15
is the default.
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 3/7] target/arm: Make stage_2_format for cache attributes optional
2022-10-23 15:36 [PATCH v4 0/7] Add ARM Cortex-R52 CPU tobias.roehmel
2022-10-23 15:36 ` [PATCH v4 1/7] target/arm: Don't add all MIDR aliases for cores that immplement PMSA tobias.roehmel
2022-10-23 15:36 ` [PATCH v4 2/7] target/arm: Make RVBAR available for all ARMv8 CPUs tobias.roehmel
@ 2022-10-23 15:36 ` tobias.roehmel
2022-11-14 17:13 ` Peter Maydell
2022-10-23 15:36 ` [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32 tobias.roehmel
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: tobias.roehmel @ 2022-10-23 15:36 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel
From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
The v8R PMSAv8 has a two-stage MPU translation process, but, unlike
VMSAv8, the stage 2 attributes are in the same format as the stage 1
attributes (8-bit MAIR format). Rather than converting the MAIR
format to the format used for VMSA stage 2 (bits [5:2] of a VMSA
stage 2 descriptor) and then converting back to do the attribute
combination, allow combined_attrs_nofwb() to accept s2 attributes
that are already in the MAIR format.
We move the assert() to combined_attrs_fwb(), because that function
really does require a VMSA stage 2 attribute format. (We will never
get there for v8R, because PMSAv8 does not implement FEAT_S2FWB.)
Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
---
target/arm/ptw.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 2ddfc028ab..db50715fa7 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2105,7 +2105,11 @@ static uint8_t combined_attrs_nofwb(CPUARMState *env,
{
uint8_t s1lo, s2lo, s1hi, s2hi, s2_mair_attrs, ret_attrs;
- s2_mair_attrs = convert_stage2_attrs(env, s2.attrs);
+ if (s2.is_s2_format) {
+ s2_mair_attrs = convert_stage2_attrs(env, s2.attrs);
+ } else {
+ s2_mair_attrs = s2.attrs;
+ }
s1lo = extract32(s1.attrs, 0, 4);
s2lo = extract32(s2_mair_attrs, 0, 4);
@@ -2163,6 +2167,8 @@ static uint8_t force_cacheattr_nibble_wb(uint8_t attr)
static uint8_t combined_attrs_fwb(CPUARMState *env,
ARMCacheAttrs s1, ARMCacheAttrs s2)
{
+ assert(s2.is_s2_format && !s1.is_s2_format);
+
switch (s2.attrs) {
case 7:
/* Use stage 1 attributes */
@@ -2212,7 +2218,6 @@ static ARMCacheAttrs combine_cacheattrs(CPUARMState *env,
ARMCacheAttrs ret;
bool tagged = false;
- assert(s2.is_s2_format && !s1.is_s2_format);
ret.is_s2_format = false;
if (s1.attrs == 0xf0) {
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 3/7] target/arm: Make stage_2_format for cache attributes optional
2022-10-23 15:36 ` [PATCH v4 3/7] target/arm: Make stage_2_format for cache attributes optional tobias.roehmel
@ 2022-11-14 17:13 ` Peter Maydell
0 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2022-11-14 17:13 UTC (permalink / raw)
To: tobias.roehmel; +Cc: qemu-devel
On Sun, 23 Oct 2022 at 16:37, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> The v8R PMSAv8 has a two-stage MPU translation process, but, unlike
> VMSAv8, the stage 2 attributes are in the same format as the stage 1
> attributes (8-bit MAIR format). Rather than converting the MAIR
> format to the format used for VMSA stage 2 (bits [5:2] of a VMSA
> stage 2 descriptor) and then converting back to do the attribute
> combination, allow combined_attrs_nofwb() to accept s2 attributes
> that are already in the MAIR format.
>
> We move the assert() to combined_attrs_fwb(), because that function
> really does require a VMSA stage 2 attribute format. (We will never
> get there for v8R, because PMSAv8 does not implement FEAT_S2FWB.)
>
> Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
> ---
> target/arm/ptw.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 2ddfc028ab..db50715fa7 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2105,7 +2105,11 @@ static uint8_t combined_attrs_nofwb(CPUARMState *env,
> {
> uint8_t s1lo, s2lo, s1hi, s2hi, s2_mair_attrs, ret_attrs;
>
> - s2_mair_attrs = convert_stage2_attrs(env, s2.attrs);
> + if (s2.is_s2_format) {
> + s2_mair_attrs = convert_stage2_attrs(env, s2.attrs);
> + } else {
> + s2_mair_attrs = s2.attrs;
> + }
You'll find when you next rebase that this needs adjustment,
because after a recent refactoring, this function no longer gets
passed the env but instead is passed the effective HCR_EL2 value.
>
> s1lo = extract32(s1.attrs, 0, 4);
> s2lo = extract32(s2_mair_attrs, 0, 4);
> @@ -2163,6 +2167,8 @@ static uint8_t force_cacheattr_nibble_wb(uint8_t attr)
> static uint8_t combined_attrs_fwb(CPUARMState *env,
> ARMCacheAttrs s1, ARMCacheAttrs s2)
> {
> + assert(s2.is_s2_format && !s1.is_s2_format);
> +
> switch (s2.attrs) {
> case 7:
> /* Use stage 1 attributes */
> @@ -2212,7 +2218,6 @@ static ARMCacheAttrs combine_cacheattrs(CPUARMState *env,
> ARMCacheAttrs ret;
> bool tagged = false;
>
> - assert(s2.is_s2_format && !s1.is_s2_format);
I think we should still assert(!s1.is_s2_format) here.
> ret.is_s2_format = false;
>
> if (s1.attrs == 0xf0) {
> --
> 2.34.1
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
2022-10-23 15:36 [PATCH v4 0/7] Add ARM Cortex-R52 CPU tobias.roehmel
` (2 preceding siblings ...)
2022-10-23 15:36 ` [PATCH v4 3/7] target/arm: Make stage_2_format for cache attributes optional tobias.roehmel
@ 2022-10-23 15:36 ` tobias.roehmel
2022-11-14 17:19 ` Peter Maydell
2022-10-23 15:36 ` [PATCH v4 5/7] target/arm: Add PMSAv8r registers tobias.roehmel
` (3 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: tobias.roehmel @ 2022-10-23 15:36 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel
From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
ARMv8-R AArch32 CPUs behave as if TTBCR.EAE is always 1 even
tough they don't have the TTBCR register.
See ARM Architecture Reference Manual Supplement - ARMv8, for the ARMv8-R
AArch32 architecture profile Version:A.c section C1.2.
Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
---
target/arm/debug_helper.c | 3 +++
target/arm/internals.h | 4 ++++
target/arm/tlb_helper.c | 3 +++
3 files changed, 10 insertions(+)
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index c21739242c..73665f988b 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -437,6 +437,9 @@ static uint32_t arm_debug_exception_fsr(CPUARMState *env)
if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
using_lpae = true;
+ } else if (arm_feature(env, ARM_FEATURE_PMSA)
+ && arm_feature(env, ARM_FEATURE_V8)) {
+ using_lpae = true;
} else {
if (arm_feature(env, ARM_FEATURE_LPAE) &&
(env->cp15.tcr_el[target_el] & TTBCR_EAE)) {
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 307a596505..e3699421b0 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -253,6 +253,10 @@ unsigned int arm_pamax(ARMCPU *cpu);
static inline bool extended_addresses_enabled(CPUARMState *env)
{
uint64_t tcr = env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
+ if (arm_feature(env, ARM_FEATURE_PMSA)
+ && arm_feature(env, ARM_FEATURE_V8)) {
+ return true;
+ }
return arm_el_is_aa64(env, 1) ||
(arm_feature(env, ARM_FEATURE_LPAE) && (tcr & TTBCR_EAE));
}
diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
index ad225b1cb2..a2047b0bc6 100644
--- a/target/arm/tlb_helper.c
+++ b/target/arm/tlb_helper.c
@@ -18,6 +18,9 @@ bool regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx)
int el = regime_el(env, mmu_idx);
if (el == 2 || arm_el_is_aa64(env, el)) {
return true;
+ } else if (arm_feature(env, ARM_FEATURE_PMSA)
+ && arm_feature(env, ARM_FEATURE_V8)) {
+ return true;
}
if (arm_feature(env, ARM_FEATURE_LPAE)
&& (regime_tcr(env, mmu_idx) & TTBCR_EAE)) {
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
2022-10-23 15:36 ` [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32 tobias.roehmel
@ 2022-11-14 17:19 ` Peter Maydell
2022-11-15 11:37 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2022-11-14 17:19 UTC (permalink / raw)
To: tobias.roehmel; +Cc: qemu-devel
On Sun, 23 Oct 2022 at 16:37, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> ARMv8-R AArch32 CPUs behave as if TTBCR.EAE is always 1 even
> tough they don't have the TTBCR register.
> See ARM Architecture Reference Manual Supplement - ARMv8, for the ARMv8-R
> AArch32 architecture profile Version:A.c section C1.2.
>
> Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
> ---
> target/arm/debug_helper.c | 3 +++
> target/arm/internals.h | 4 ++++
> target/arm/tlb_helper.c | 3 +++
> 3 files changed, 10 insertions(+)
>
> diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
> index c21739242c..73665f988b 100644
> --- a/target/arm/debug_helper.c
> +++ b/target/arm/debug_helper.c
> @@ -437,6 +437,9 @@ static uint32_t arm_debug_exception_fsr(CPUARMState *env)
>
> if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
> using_lpae = true;
> + } else if (arm_feature(env, ARM_FEATURE_PMSA)
> + && arm_feature(env, ARM_FEATURE_V8)) {
Indentation looks wrong here. Generally the second line of a
multiline if (...) condition starts in the column after the '(',
so it lines up with the first part of the condition.
> + using_lpae = true;
> } else {
> if (arm_feature(env, ARM_FEATURE_LPAE) &&
> (env->cp15.tcr_el[target_el] & TTBCR_EAE)) {
For instance this is an example in the existing code.
We are inconsistent about whether we put operators like '&&' at
the end of the first line or beginning of the second line, so
pick whichever you like best, I guess.
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 307a596505..e3699421b0 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -253,6 +253,10 @@ unsigned int arm_pamax(ARMCPU *cpu);
> static inline bool extended_addresses_enabled(CPUARMState *env)
> {
> uint64_t tcr = env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
> + if (arm_feature(env, ARM_FEATURE_PMSA)
> + && arm_feature(env, ARM_FEATURE_V8)) {
Indentation is a bit odd here too.
> + return true;
> + }
> return arm_el_is_aa64(env, 1) ||
> (arm_feature(env, ARM_FEATURE_LPAE) && (tcr & TTBCR_EAE));
> }
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index ad225b1cb2..a2047b0bc6 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -18,6 +18,9 @@ bool regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx)
> int el = regime_el(env, mmu_idx);
> if (el == 2 || arm_el_is_aa64(env, el)) {
> return true;
> + } else if (arm_feature(env, ARM_FEATURE_PMSA)
> + && arm_feature(env, ARM_FEATURE_V8)) {
> + return true;
> }
Use an "if ()..." not an "else if" here to match the style
of the other check in this function.
> if (arm_feature(env, ARM_FEATURE_LPAE)
> && (regime_tcr(env, mmu_idx) & TTBCR_EAE)) {
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
2022-11-14 17:19 ` Peter Maydell
@ 2022-11-15 11:37 ` Philippe Mathieu-Daudé
2022-11-15 12:01 ` Daniel P. Berrangé
0 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-15 11:37 UTC (permalink / raw)
To: Peter Maydell, tobias.roehmel, Eric Blake
Cc: qemu-devel, Daniel P. Berrangé
On 14/11/22 18:19, Peter Maydell wrote:
> On Sun, 23 Oct 2022 at 16:37, <tobias.roehmel@rwth-aachen.de> wrote:
>>
>> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>>
>> ARMv8-R AArch32 CPUs behave as if TTBCR.EAE is always 1 even
>> tough they don't have the TTBCR register.
>> See ARM Architecture Reference Manual Supplement - ARMv8, for the ARMv8-R
>> AArch32 architecture profile Version:A.c section C1.2.
>>
>> Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>> ---
>> target/arm/debug_helper.c | 3 +++
>> target/arm/internals.h | 4 ++++
>> target/arm/tlb_helper.c | 3 +++
>> 3 files changed, 10 insertions(+)
>>
>> diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
>> index c21739242c..73665f988b 100644
>> --- a/target/arm/debug_helper.c
>> +++ b/target/arm/debug_helper.c
>> @@ -437,6 +437,9 @@ static uint32_t arm_debug_exception_fsr(CPUARMState *env)
>>
>> if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
>> using_lpae = true;
>> + } else if (arm_feature(env, ARM_FEATURE_PMSA)
>> + && arm_feature(env, ARM_FEATURE_V8)) {
>
> Indentation looks wrong here. Generally the second line of a
> multiline if (...) condition starts in the column after the '(',
> so it lines up with the first part of the condition.
>
>> + using_lpae = true;
>> } else {
>> if (arm_feature(env, ARM_FEATURE_LPAE) &&
>> (env->cp15.tcr_el[target_el] & TTBCR_EAE)) {
>
> For instance this is an example in the existing code.
>
> We are inconsistent about whether we put operators like '&&' at
> the end of the first line or beginning of the second line, so
> pick whichever you like best, I guess.
Personally I find the operator at the end aesthetically nicer, but
few years ago Eric Blake reasoned that moving it at the beginning
was more explicit (to reviewers) thus safer, and I now I tend to
place it at the beginning.
Maybe part of the justification was cases where copy/pasting new
conditions in pre-existing could introduce a bug when the operator
is at the end?
+Eric/Daniel who usually give such good style advises :)
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
2022-11-15 11:37 ` Philippe Mathieu-Daudé
@ 2022-11-15 12:01 ` Daniel P. Berrangé
0 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2022-11-15 12:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, tobias.roehmel, Eric Blake, qemu-devel
On Tue, Nov 15, 2022 at 12:37:45PM +0100, Philippe Mathieu-Daudé wrote:
> On 14/11/22 18:19, Peter Maydell wrote:
> > On Sun, 23 Oct 2022 at 16:37, <tobias.roehmel@rwth-aachen.de> wrote:
> > >
> > > From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
> > >
> > > ARMv8-R AArch32 CPUs behave as if TTBCR.EAE is always 1 even
> > > tough they don't have the TTBCR register.
> > > See ARM Architecture Reference Manual Supplement - ARMv8, for the ARMv8-R
> > > AArch32 architecture profile Version:A.c section C1.2.
> > >
> > > Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
> > > ---
> > > target/arm/debug_helper.c | 3 +++
> > > target/arm/internals.h | 4 ++++
> > > target/arm/tlb_helper.c | 3 +++
> > > 3 files changed, 10 insertions(+)
> > >
> > > diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
> > > index c21739242c..73665f988b 100644
> > > --- a/target/arm/debug_helper.c
> > > +++ b/target/arm/debug_helper.c
> > > @@ -437,6 +437,9 @@ static uint32_t arm_debug_exception_fsr(CPUARMState *env)
> > >
> > > if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
> > > using_lpae = true;
> > > + } else if (arm_feature(env, ARM_FEATURE_PMSA)
> > > + && arm_feature(env, ARM_FEATURE_V8)) {
> >
> > Indentation looks wrong here. Generally the second line of a
> > multiline if (...) condition starts in the column after the '(',
> > so it lines up with the first part of the condition.
This illustrates the problem with putting '&&' at start of line.
It harms the vertical alignment of the expressions, leading to
the desire to un-indent the block by 3 spaces to get 'arm_feature'
to line up. Understandable, but no editor/code indentors will
preserve this kind of indentation/alignment, so it shouldn't
be done.
Both ways you can choose to line up the indent for operator at
start of line are unpleasant - it is a no-win scenario IMHO.
> > > + using_lpae = true;
> > > } else {
> > > if (arm_feature(env, ARM_FEATURE_LPAE) &&
> > > (env->cp15.tcr_el[target_el] & TTBCR_EAE)) {
> >
> > For instance this is an example in the existing code.
> >
> > We are inconsistent about whether we put operators like '&&' at
> > the end of the first line or beginning of the second line, so
> > pick whichever you like best, I guess.
> Personally I find the operator at the end aesthetically nicer, but
> few years ago Eric Blake reasoned that moving it at the beginning
> was more explicit (to reviewers) thus safer, and I now I tend to
> place it at the beginning.
I'm not convinced that operator at start of line makes any
difference at all to reviewability, and as above it leads
to undesirable indentation choices.
> Maybe part of the justification was cases where copy/pasting new
> conditions in pre-existing could introduce a bug when the operator
> is at the end?
The QEMU defacto standard is operators at end of line, given what
appears as the usage ratio of 6:1
$ git grep -E '^\s*(&&|&|\||\|\|)\s' '*.c' | wc -l
1692
$ git grep -E '\s(&&|&|\||\|\|)\s*$' '*.c' | wc -l
10289
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 5/7] target/arm: Add PMSAv8r registers
2022-10-23 15:36 [PATCH v4 0/7] Add ARM Cortex-R52 CPU tobias.roehmel
` (3 preceding siblings ...)
2022-10-23 15:36 ` [PATCH v4 4/7] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32 tobias.roehmel
@ 2022-10-23 15:36 ` tobias.roehmel
2022-11-18 13:52 ` Peter Maydell
2022-10-23 15:36 ` [PATCH v4 6/7] target/arm: Add PMSAv8r functionality tobias.roehmel
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: tobias.roehmel @ 2022-10-23 15:36 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel
From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
---
target/arm/cpu.c | 26 +++-
target/arm/cpu.h | 12 ++
target/arm/helper.c | 290 +++++++++++++++++++++++++++++++++++++++++++
target/arm/machine.c | 28 +++++
target/arm/ptw.c | 9 +-
5 files changed, 363 insertions(+), 2 deletions(-)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b642749d6d..468150ad6c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -463,6 +463,16 @@ static void arm_cpu_reset(DeviceState *dev)
sizeof(*env->pmsav7.dracr) * cpu->pmsav7_dregion);
}
}
+
+ if (cpu->pmsav8r_hdregion > 0) {
+ memset(env->pmsav8.hprbar[M_REG_NS], 0,
+ sizeof(*env->pmsav8.hprbar[M_REG_NS])
+ * cpu->pmsav8r_hdregion);
+ memset(env->pmsav8.hprlar[M_REG_NS], 0,
+ sizeof(*env->pmsav8.hprlar[M_REG_NS])
+ * cpu->pmsav8r_hdregion);
+ }
+
env->pmsav7.rnr[M_REG_NS] = 0;
env->pmsav7.rnr[M_REG_S] = 0;
env->pmsav8.mair0[M_REG_NS] = 0;
@@ -1965,8 +1975,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;
}
@@ -1994,6 +2005,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
env->pmsav7.dracr = g_new0(uint32_t, nr);
}
}
+
+ if (cpu->pmsav8r_hdregion > 0xFF) {
+ error_setg(errp, "PMSAv8 MPU EL2 #regions invalid %" PRIu32,
+ cpu->pmsav8r_hdregion);
+ return;
+ }
+
+ if (cpu->pmsav8r_hdregion) {
+ env->pmsav8.hprbar[M_REG_NS] = g_new0(uint32_t,
+ cpu->pmsav8r_hdregion);
+ env->pmsav8.hprlar[M_REG_NS] = g_new0(uint32_t,
+ cpu->pmsav8r_hdregion);
+ }
}
if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 429ed42eec..1bb3c24db1 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -307,6 +307,13 @@ typedef struct CPUArchState {
};
uint64_t sctlr_el[4];
};
+ union { /* Virtualization System control register. */
+ struct {
+ uint32_t vsctlr_ns;
+ uint32_t vsctlr_s;
+ };
+ uint32_t vsctlr_el[2];
+ };
uint64_t cpacr_el1; /* Architectural feature access control register */
uint64_t cptr_el[4]; /* ARMv8 feature trap registers */
uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */
@@ -740,8 +747,11 @@ typedef struct CPUArchState {
*/
uint32_t *rbar[M_REG_NUM_BANKS];
uint32_t *rlar[M_REG_NUM_BANKS];
+ uint32_t *hprbar[M_REG_NUM_BANKS];
+ uint32_t *hprlar[M_REG_NUM_BANKS];
uint32_t mair0[M_REG_NUM_BANKS];
uint32_t mair1[M_REG_NUM_BANKS];
+ uint32_t hprselr[M_REG_NUM_BANKS];
} pmsav8;
/* v8M SAU */
@@ -901,6 +911,8 @@ struct ArchCPU {
bool has_mpu;
/* PMSAv7 MPU number of supported regions */
uint32_t pmsav7_dregion;
+ /* PMSAv8 MPU number of supported hyp regions */
+ uint32_t pmsav8r_hdregion;
/* v8M SAU number of supported regions */
uint32_t sau_sregion;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2e9e420d4e..6a27a618bc 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3607,6 +3607,215 @@ static void pmsav7_rgnr_write(CPUARMState *env, const ARMCPRegInfo *ri,
raw_write(env, ri, value);
}
+static void prbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+ ARMCPU *cpu = env_archcpu(env);
+
+ tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
+ env->pmsav8.rbar[M_REG_NS][env->pmsav7.rnr[M_REG_NS]] = value;
+}
+
+static uint64_t prbar_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+ return env->pmsav8.rbar[M_REG_NS][env->pmsav7.rnr[M_REG_NS]];
+}
+
+static void prlar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+ ARMCPU *cpu = env_archcpu(env);
+
+ tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
+ env->pmsav8.rlar[M_REG_NS][env->pmsav7.rnr[M_REG_NS]] = value;
+}
+
+static uint64_t prlar_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+ return env->pmsav8.rlar[M_REG_NS][env->pmsav7.rnr[M_REG_NS]];
+}
+
+static void prselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+ ARMCPU *cpu = env_archcpu(env);
+
+ /* Ignore writes that would select not implemented region */
+ if (value >= cpu->pmsav7_dregion) {
+ return;
+ }
+
+ env->pmsav7.rnr[M_REG_NS] = value;
+}
+
+static void hprbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+ ARMCPU *cpu = env_archcpu(env);
+
+ tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
+ env->pmsav8.hprbar[M_REG_NS][env->pmsav8.hprselr[M_REG_NS]] = value;
+}
+
+static uint64_t hprbar_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+ return env->pmsav8.hprbar[M_REG_NS][env->pmsav8.hprselr[M_REG_NS]];
+}
+
+static void hprlar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+ ARMCPU *cpu = env_archcpu(env);
+
+ tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
+ env->pmsav8.hprlar[M_REG_NS][env->pmsav8.hprselr[M_REG_NS]] = value;
+}
+
+static uint64_t hprlar_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+ return env->pmsav8.hprlar[M_REG_NS][env->pmsav8.hprselr[M_REG_NS]];
+}
+
+static void hprenr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+ uint32_t n;
+ uint32_t bit;
+ ARMCPU *cpu = env_archcpu(env);
+
+ /* Ignore writes to unimplemented regions */
+ value &= (1 << cpu->pmsav8r_hdregion) - 1;
+
+ tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
+
+ /* Register alias is only valid for first 32 indexes */
+ for (n = 0; n < (cpu->pmsav8r_hdregion & 0x1F); ++n) {
+ bit = extract32(value, n, 1);
+ env->pmsav8.hprlar[M_REG_NS][n] = deposit32(
+ env->pmsav8.hprlar[M_REG_NS][n], 0, 1, bit);
+ }
+}
+
+static uint64_t hprenr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+ uint32_t n;
+ uint32_t result = 0x0;
+ ARMCPU *cpu = env_archcpu(env);
+
+ /* Register alias is only valid for first 32 indexes */
+ for (n = 0; n < (cpu->pmsav8r_hdregion & 0x1F); ++n) {
+ if (env->pmsav8.hprlar[M_REG_NS][n] & 0x1) {
+ result |= (0x1 << n);
+ }
+ }
+ return result;
+}
+
+static void hprselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+ ARMCPU *cpu = env_archcpu(env);
+
+ /* Ignore writes that would select not implemented region */
+ if (value >= cpu->pmsav8r_hdregion) {
+ return;
+ }
+
+ env->pmsav8.hprselr[M_REG_NS] = value;
+}
+
+static void pmsav8r_regn_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+ ARMCPU *cpu = env_archcpu(env);
+ uint8_t index = (ri->crm & 0b111) << 1;
+ index |= (ri->opc2 & 1 << 2) >> 2;
+
+ tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
+
+ if (ri->opc1 == 4) {
+ if (index >= cpu->pmsav8r_hdregion) {
+ return;
+ }
+ if (ri->opc2 & 0x1) {
+ env->pmsav8.hprlar[M_REG_NS][index] = value;
+ } else {
+ env->pmsav8.hprbar[M_REG_NS][index] = value;
+ }
+ } else {
+ if (index >= cpu->pmsav7_dregion) {
+ return;
+ }
+ if (ri->opc2 & 0x1) {
+ env->pmsav8.rlar[M_REG_NS][index] = value;
+ } else {
+ env->pmsav8.rbar[M_REG_NS][index] = value;
+ }
+ }
+}
+
+static uint64_t pmsav8r_regn_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+ ARMCPU *cpu = env_archcpu(env);
+ uint8_t index = (ri->crm & 0b111) << 1;
+ index |= (ri->opc2 & 1 << 2) >> 2;
+
+ if (ri->opc1 == 4) {
+ if (index >= cpu->pmsav8r_hdregion) {
+ return 0x0;
+ }
+ if (ri->opc2 & 0x1) {
+ return env->pmsav8.hprlar[M_REG_NS][index];
+ } else {
+ return env->pmsav8.hprbar[M_REG_NS][index];
+ }
+ } else {
+ if (index >= cpu->pmsav7_dregion) {
+ return 0x0;
+ }
+ if (ri->opc2 & 0x1) {
+ return env->pmsav8.rlar[M_REG_NS][index];
+ } else {
+ return env->pmsav8.rbar[M_REG_NS][index];
+ }
+ }
+}
+
+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,
+ .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,
+ .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,
+ .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,
+ .readfn = hprlar_read, .writefn = hprlar_write},
+ { .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[M_REG_NS])},
+ { .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
@@ -8079,6 +8288,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, .crn = 0, .crm = 0, .opc1 = 4, .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,
@@ -8122,6 +8338,67 @@ 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_M)
+ && arm_feature(env, ARM_FEATURE_V8)) {
+ uint32_t i = 0;
+ g_autofree char *tmp_string;
+
+ 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 < (cpu->pmsav7_dregion & 0x1F); ++i) {
+ uint8_t crm = 0b1000 | ((i & 0b1110) >> 1);
+ uint8_t opc2 = (i & 0x1) << 2;
+
+ tmp_string = g_strdup_printf("PRBAR%u", i);
+ ARMCPRegInfo tmp_prbarn_reginfo = {
+ .name = tmp_string, .type = ARM_CP_ALIAS,
+ .cp = 15, .opc1 = 0, .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);
+
+ opc2 = (i & 0x1) << 2 | 0x1;
+ tmp_string = g_strdup_printf("PRLAR%u", i);
+ ARMCPRegInfo tmp_prlarn_reginfo = {
+ .name = tmp_string, .type = ARM_CP_ALIAS,
+ .cp = 15, .opc1 = 0, .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 < (cpu->pmsav8r_hdregion & 0x1F); ++i) {
+ uint8_t crm = 0b1000 | ((i & 0b1110) >> 1);
+ uint8_t opc2 = (i & 0x1) << 2;
+
+ tmp_string = g_strdup_printf("HPRBAR%u", i);
+ ARMCPRegInfo tmp_hprbarn_reginfo = {
+ .name = tmp_string, .type = ARM_CP_ALIAS,
+ .cp = 15, .opc1 = 4, .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 = (i & 0x1) << 2 | 0x1;
+ tmp_string = g_strdup_printf("HPRLAR%u", i);
+ ARMCPRegInfo tmp_hprlarn_reginfo = {
+ .name = tmp_string, .type = ARM_CP_ALIAS,
+ .cp = 15, .opc1 = 4, .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);
}
@@ -8243,6 +8520,19 @@ void register_cp_regs_for_features(ARMCPU *cpu)
sctlr.type |= ARM_CP_SUPPRESS_TB_END;
}
define_one_arm_cp_reg(cpu, &sctlr);
+
+ if (arm_feature(env, ARM_FEATURE_PMSA)
+ && !arm_feature(env, ARM_FEATURE_M)
+ && arm_feature(env, ARM_FEATURE_V8)) {
+ ARMCPRegInfo vsctlr = {
+ .name = "VSCTLR", .state = ARM_CP_STATE_AA32,
+ .cp = 15, .opc1 = 4, .crn = 2, .crm = 0, .opc2 = 0,
+ .access = PL2_RW, .resetvalue = 0x0,
+ .bank_fieldoffsets = { offsetof(CPUARMState, cp15.vsctlr_s),
+ offsetof(CPUARMState, cp15.vsctlr_ns) },
+ };
+ define_one_arm_cp_reg(cpu, &vsctlr);
+ }
}
if (cpu_isar_feature(aa64_lor, cpu)) {
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 54c5c62433..923da8d0bc 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -487,6 +487,30 @@ static bool pmsav8_needed(void *opaque)
arm_feature(env, ARM_FEATURE_V8);
}
+static bool pmsav8r_needed(void *opaque)
+{
+ ARMCPU *cpu = opaque;
+ CPUARMState *env = &cpu->env;
+
+ return arm_feature(env, ARM_FEATURE_PMSA) &&
+ arm_feature(env, ARM_FEATURE_V8) &&
+ !arm_feature(env, ARM_FEATURE_M);
+}
+
+static const VMStateDescription vmstate_pmsav8r = {
+ .name = "cpu/pmsav8/pmsav8r",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = pmsav8r_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_VARRAY_UINT32(env.pmsav8.hprbar[M_REG_NS], ARMCPU,
+ pmsav8r_hdregion, 0, vmstate_info_uint32, uint32_t),
+ VMSTATE_VARRAY_UINT32(env.pmsav8.hprlar[M_REG_NS], ARMCPU,
+ pmsav8r_hdregion, 0, vmstate_info_uint32, uint32_t),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
static const VMStateDescription vmstate_pmsav8 = {
.name = "cpu/pmsav8",
.version_id = 1,
@@ -500,6 +524,10 @@ static const VMStateDescription vmstate_pmsav8 = {
VMSTATE_UINT32(env.pmsav8.mair0[M_REG_NS], ARMCPU),
VMSTATE_UINT32(env.pmsav8.mair1[M_REG_NS], ARMCPU),
VMSTATE_END_OF_LIST()
+ },
+ .subsections = (const VMStateDescription * []) {
+ &vmstate_pmsav8r,
+ NULL
}
};
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index db50715fa7..4bd7389fa9 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1718,6 +1718,13 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
bool hit = false;
uint32_t addr_page_base = address & TARGET_PAGE_MASK;
uint32_t addr_page_limit = addr_page_base + (TARGET_PAGE_SIZE - 1);
+ int region_counter;
+
+ if (regime_el(env, mmu_idx) == 2) {
+ region_counter = cpu->pmsav8r_hdregion;
+ } else {
+ region_counter = cpu->pmsav7_dregion;
+ }
result->page_size = TARGET_PAGE_SIZE;
result->phys = address;
@@ -1742,7 +1749,7 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
hit = true;
}
- for (n = (int)cpu->pmsav7_dregion - 1; n >= 0; n--) {
+ for (n = region_counter - 1; n >= 0; n--) {
/* region search */
/*
* Note that the base address is bits [31:5] from the register
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 5/7] target/arm: Add PMSAv8r registers
2022-10-23 15:36 ` [PATCH v4 5/7] target/arm: Add PMSAv8r registers tobias.roehmel
@ 2022-11-18 13:52 ` Peter Maydell
0 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2022-11-18 13:52 UTC (permalink / raw)
To: tobias.roehmel; +Cc: qemu-devel
On Sun, 23 Oct 2022 at 16:37, <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>
This patch is basically the right shape, but there's a big
simplification you can make and then a bunch of minor tweaks.
> ---
> target/arm/cpu.c | 26 +++-
> target/arm/cpu.h | 12 ++
> target/arm/helper.c | 290 +++++++++++++++++++++++++++++++++++++++++++
> target/arm/machine.c | 28 +++++
> target/arm/ptw.c | 9 +-
> 5 files changed, 363 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index b642749d6d..468150ad6c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -463,6 +463,16 @@ static void arm_cpu_reset(DeviceState *dev)
> sizeof(*env->pmsav7.dracr) * cpu->pmsav7_dregion);
> }
> }
> +
> + if (cpu->pmsav8r_hdregion > 0) {
> + memset(env->pmsav8.hprbar[M_REG_NS], 0,
> + sizeof(*env->pmsav8.hprbar[M_REG_NS])
> + * cpu->pmsav8r_hdregion);
> + memset(env->pmsav8.hprlar[M_REG_NS], 0,
> + sizeof(*env->pmsav8.hprlar[M_REG_NS])
> + * cpu->pmsav8r_hdregion);
> + }
> +
> env->pmsav7.rnr[M_REG_NS] = 0;
> env->pmsav7.rnr[M_REG_S] = 0;
> env->pmsav8.mair0[M_REG_NS] = 0;
> @@ -1965,8 +1975,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;
> }
>
> @@ -1994,6 +2005,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> env->pmsav7.dracr = g_new0(uint32_t, nr);
> }
> }
> +
> + if (cpu->pmsav8r_hdregion > 0xFF) {
> + error_setg(errp, "PMSAv8 MPU EL2 #regions invalid %" PRIu32,
> + cpu->pmsav8r_hdregion);
> + return;
> + }
> +
> + if (cpu->pmsav8r_hdregion) {
> + env->pmsav8.hprbar[M_REG_NS] = g_new0(uint32_t,
> + cpu->pmsav8r_hdregion);
> + env->pmsav8.hprlar[M_REG_NS] = g_new0(uint32_t,
> + cpu->pmsav8r_hdregion);
> + }
> }
>
> if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 429ed42eec..1bb3c24db1 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -307,6 +307,13 @@ typedef struct CPUArchState {
> };
> uint64_t sctlr_el[4];
> };
> + union { /* Virtualization System control register. */
> + struct {
> + uint32_t vsctlr_ns;
> + uint32_t vsctlr_s;
> + };
> + uint32_t vsctlr_el[2];
> + };
v8R only has a single security state, so you don't need to make this
a union or have _ns and _s versions, you can just have a single field.
We should make the field a uint64_t because this register in
PMSAv8-64 is 64 bits, and having the field be 64 bits to start with will
make life slightly easier if we need to implement that in future. So
uint64_t vsctlr;
> uint64_t cpacr_el1; /* Architectural feature access control register */
> uint64_t cptr_el[4]; /* ARMv8 feature trap registers */
> uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */
> @@ -740,8 +747,11 @@ typedef struct CPUArchState {
> */
> uint32_t *rbar[M_REG_NUM_BANKS];
> uint32_t *rlar[M_REG_NUM_BANKS];
> + uint32_t *hprbar[M_REG_NUM_BANKS];
> + uint32_t *hprlar[M_REG_NUM_BANKS];
These don't need to be arrays, so just
uint32_t *hprbar;
uint32_t *hprlar;
(PMSAv8-64 also has these as 64-bit registers, but that is also true
of the existing rbar/rlar. So I think on balance it's better to
keep hprbar/hprlar the same length as rbar/rlar, and if we ever
implement PMSAv8-64 we'll have to sort out extending the length
of both sets of registers at the same time.)
> uint32_t mair0[M_REG_NUM_BANKS];
> uint32_t mair1[M_REG_NUM_BANKS];
> + uint32_t hprselr[M_REG_NUM_BANKS];
Similarly this can just be
uint32_t hprselr;
> } pmsav8;
>
> /* v8M SAU */
> @@ -901,6 +911,8 @@ struct ArchCPU {
> bool has_mpu;
> /* PMSAv7 MPU number of supported regions */
> uint32_t pmsav7_dregion;
> + /* PMSAv8 MPU number of supported hyp regions */
> + uint32_t pmsav8r_hdregion;
> /* v8M SAU number of supported regions */
> uint32_t sau_sregion;
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2e9e420d4e..6a27a618bc 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3607,6 +3607,215 @@ static void pmsav7_rgnr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> raw_write(env, ri, value);
> }
>
> +static void prbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + ARMCPU *cpu = env_archcpu(env);
> +
> + tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
> + env->pmsav8.rbar[M_REG_NS][env->pmsav7.rnr[M_REG_NS]] = value;
> +}
> +
> +static uint64_t prbar_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> + return env->pmsav8.rbar[M_REG_NS][env->pmsav7.rnr[M_REG_NS]];
> +}
> +
> +static void prlar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + ARMCPU *cpu = env_archcpu(env);
> +
> + tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
> + env->pmsav8.rlar[M_REG_NS][env->pmsav7.rnr[M_REG_NS]] = value;
> +}
> +
> +static uint64_t prlar_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> + return env->pmsav8.rlar[M_REG_NS][env->pmsav7.rnr[M_REG_NS]];
> +}
> +
> +static void prselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + ARMCPU *cpu = env_archcpu(env);
> +
> + /* Ignore writes that would select not implemented region */
It's worth mentioning that this is architecturally UNPREDICTABLE.
> + if (value >= cpu->pmsav7_dregion) {
> + return;
> + }
> +
> + env->pmsav7.rnr[M_REG_NS] = value;
> +}
> +
> +static void hprbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + ARMCPU *cpu = env_archcpu(env);
> +
> + tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
> + env->pmsav8.hprbar[M_REG_NS][env->pmsav8.hprselr[M_REG_NS]] = value;
> +}
> +
> +static uint64_t hprbar_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> + return env->pmsav8.hprbar[M_REG_NS][env->pmsav8.hprselr[M_REG_NS]];
> +}
> +
> +static void hprlar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + ARMCPU *cpu = env_archcpu(env);
> +
> + tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
> + env->pmsav8.hprlar[M_REG_NS][env->pmsav8.hprselr[M_REG_NS]] = value;
> +}
> +
> +static uint64_t hprlar_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> + return env->pmsav8.hprlar[M_REG_NS][env->pmsav8.hprselr[M_REG_NS]];
> +}
> +
> +static void hprenr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + uint32_t n;
> + uint32_t bit;
> + ARMCPU *cpu = env_archcpu(env);
> +
> + /* Ignore writes to unimplemented regions */
> + value &= (1 << cpu->pmsav8r_hdregion) - 1;
This is undefined behaviour if cpu->pmsav8r_hdregion is greater than 31.
I suggest defining a local variable
int rmax = MIN(cpu->pmsav8r_hdregion, 32);
Then you can do
value &= MAKE_64BIT_MASK(0, rmax);
> +
> + tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
> +
> + /* Register alias is only valid for first 32 indexes */
> + for (n = 0; n < (cpu->pmsav8r_hdregion & 0x1F); ++n) {
This isn't the right boundary -- for instance if pmsav8r_hdregion
is 33 then this will only set bit 0. If you take my suggestion
above then you can just use 'rmax' as the upper bound.
> + bit = extract32(value, n, 1);
> + env->pmsav8.hprlar[M_REG_NS][n] = deposit32(
> + env->pmsav8.hprlar[M_REG_NS][n], 0, 1, bit);
> + }
> +}
> +
> +static uint64_t hprenr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> + uint32_t n;
> + uint32_t result = 0x0;
> + ARMCPU *cpu = env_archcpu(env);
> +
> + /* Register alias is only valid for first 32 indexes */
> + for (n = 0; n < (cpu->pmsav8r_hdregion & 0x1F); ++n) {
Same remark as above about the upper bound.
> + if (env->pmsav8.hprlar[M_REG_NS][n] & 0x1) {
> + result |= (0x1 << n);
> + }
> + }
> + return result;
> +}
> +
> +static void hprselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + ARMCPU *cpu = env_archcpu(env);
> +
> + /* Ignore writes that would select not implemented region */
> + if (value >= cpu->pmsav8r_hdregion) {
> + return;
> + }
> +
> + env->pmsav8.hprselr[M_REG_NS] = value;
> +}
> +
> +static void pmsav8r_regn_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + ARMCPU *cpu = env_archcpu(env);
> + uint8_t index = (ri->crm & 0b111) << 1;
> + index |= (ri->opc2 & 1 << 2) >> 2;
This is missing the 5th bit of index, which is in opc0 bit 0.
I would suggest
index = (extract32(ri->opc0, 0, 1) << 4) | (extract32(ri->crm, 0, 3)
<< 1) | extract32(ri->opc2, 2, 1);
> + tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
> +
> + if (ri->opc1 == 4) {
This should be checking (ri->opc1 & 2).
> + if (index >= cpu->pmsav8r_hdregion) {
> + return;
> + }
> + if (ri->opc2 & 0x1) {
> + env->pmsav8.hprlar[M_REG_NS][index] = value;
> + } else {
> + env->pmsav8.hprbar[M_REG_NS][index] = value;
> + }
> + } else {
> + if (index >= cpu->pmsav7_dregion) {
> + return;
> + }
> + if (ri->opc2 & 0x1) {
> + env->pmsav8.rlar[M_REG_NS][index] = value;
> + } else {
> + env->pmsav8.rbar[M_REG_NS][index] = value;
> + }
> + }
> +}
> +
> +static uint64_t pmsav8r_regn_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> + ARMCPU *cpu = env_archcpu(env);
> + uint8_t index = (ri->crm & 0b111) << 1;
> + index |= (ri->opc2 & 1 << 2) >> 2;
Same remarks apply to this function as above.
> +
> + if (ri->opc1 == 4) {
> + if (index >= cpu->pmsav8r_hdregion) {
> + return 0x0;
> + }
> + if (ri->opc2 & 0x1) {
> + return env->pmsav8.hprlar[M_REG_NS][index];
> + } else {
> + return env->pmsav8.hprbar[M_REG_NS][index];
> + }
> + } else {
> + if (index >= cpu->pmsav7_dregion) {
> + return 0x0;
> + }
> + if (ri->opc2 & 0x1) {
> + return env->pmsav8.rlar[M_REG_NS][index];
> + } else {
> + return env->pmsav8.rbar[M_REG_NS][index];
> + }
> + }
> +}
> +
> +static const ARMCPRegInfo pmsav8r_cp_reginfo[] = {
> + { .name = "PRBAR",
> + .cp = 15, .opc1 = 0, .crn = 6, .crm = 3, .opc2 = 0,
Indent here seems odd (even by the ad-hoc standards we apply
to cpreginfo array declarations): generally we make all the '.'s line up.
> + .access = PL1_RW, .type = ARM_CP_ALIAS,
> + .accessfn = access_tvm_trvm,
> + .readfn = prbar_read, .writefn = prbar_write},
We generally put a space before the closing } on this kind of line.
> + { .name = "PRLAR",
> + .cp = 15, .opc1 = 0, .crn = 6, .crm = 3, .opc2 = 1,
> + .access = PL1_RW, .type = ARM_CP_ALIAS,
> + .accessfn = access_tvm_trvm,
> + .readfn = prlar_read, .writefn = prlar_write},
These two should be .type = ARM_CP_NO_RAW
> + { .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,
> + .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,
> + .readfn = hprlar_read, .writefn = hprlar_write},
These two also should be 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[M_REG_NS])},
> + { .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
> @@ -8079,6 +8288,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, .crn = 0, .crm = 0, .opc1 = 4, .opc2 = 4,
Prefer the order cp, opc1, crn, crm, opc2
> + .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,
> @@ -8122,6 +8338,67 @@ 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_M)
M-profile is checked for at the top of the function and it returns
early and never gets to this code, so you can skip this test.
> + && arm_feature(env, ARM_FEATURE_V8)) {
> + uint32_t i = 0;
> + g_autofree char *tmp_string;
> +
> + 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 < (cpu->pmsav7_dregion & 0x1F); ++i) {
Bad upper index again.
> + uint8_t crm = 0b1000 | ((i & 0b1110) >> 1);
> + uint8_t opc2 = (i & 0x1) << 2;
Doesn't handle the 5th bit in the index (the one that ends up in opc1).
> +
> + tmp_string = g_strdup_printf("PRBAR%u", i);
> + ARMCPRegInfo tmp_prbarn_reginfo = {
> + .name = tmp_string, .type = ARM_CP_ALIAS,
ARM_CP_NO_RAW
> + .cp = 15, .opc1 = 0, .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);
> +
> + opc2 = (i & 0x1) << 2 | 0x1;
> + tmp_string = g_strdup_printf("PRLAR%u", i);
If you're going to use g_autofree, you can't reuse the same variable
for a new string allocation -- this leaks the first string when we
assign to tmp_string again. You need to use separate variables so
that both allocations are auto-freed when they go out of scope.
> + ARMCPRegInfo tmp_prlarn_reginfo = {
> + .name = tmp_string, .type = ARM_CP_ALIAS,
ARM_CP_NO_RAW
> + .cp = 15, .opc1 = 0, .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 < (cpu->pmsav8r_hdregion & 0x1F); ++i) {
> + uint8_t crm = 0b1000 | ((i & 0b1110) >> 1);
> + uint8_t opc2 = (i & 0x1) << 2;
Same comments for the first loop all apply to this loop.
> +
> + tmp_string = g_strdup_printf("HPRBAR%u", i);
> + ARMCPRegInfo tmp_hprbarn_reginfo = {
> + .name = tmp_string, .type = ARM_CP_ALIAS,
> + .cp = 15, .opc1 = 4, .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 = (i & 0x1) << 2 | 0x1;
> + tmp_string = g_strdup_printf("HPRLAR%u", i);
> + ARMCPRegInfo tmp_hprlarn_reginfo = {
> + .name = tmp_string, .type = ARM_CP_ALIAS,
> + .cp = 15, .opc1 = 4, .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);
> }
> @@ -8243,6 +8520,19 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> sctlr.type |= ARM_CP_SUPPRESS_TB_END;
> }
> define_one_arm_cp_reg(cpu, &sctlr);
> +
> + if (arm_feature(env, ARM_FEATURE_PMSA)
> + && !arm_feature(env, ARM_FEATURE_M)
Don't need to check for not-M.
> + && arm_feature(env, ARM_FEATURE_V8)) {
> + ARMCPRegInfo vsctlr = {
> + .name = "VSCTLR", .state = ARM_CP_STATE_AA32,
> + .cp = 15, .opc1 = 4, .crn = 2, .crm = 0, .opc2 = 0,
> + .access = PL2_RW, .resetvalue = 0x0,
> + .bank_fieldoffsets = { offsetof(CPUARMState, cp15.vsctlr_s),
> + offsetof(CPUARMState, cp15.vsctlr_ns) },
This will get simpler when you aren't trying to describe it as
a banked register. Note that if you make the field a uint64_t
as I suggest above then you will want to use offsetoflow32()
rather than plain offsetof() (so that on a big-endian host the
field offset is pointed to the high half of the uint64_t.)
> + };
> + define_one_arm_cp_reg(cpu, &vsctlr);
> + }
> }
>
> if (cpu_isar_feature(aa64_lor, cpu)) {
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 54c5c62433..923da8d0bc 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -487,6 +487,30 @@ static bool pmsav8_needed(void *opaque)
> arm_feature(env, ARM_FEATURE_V8);
> }
>
> +static bool pmsav8r_needed(void *opaque)
> +{
> + ARMCPU *cpu = opaque;
> + CPUARMState *env = &cpu->env;
> +
> + return arm_feature(env, ARM_FEATURE_PMSA) &&
> + arm_feature(env, ARM_FEATURE_V8) &&
> + !arm_feature(env, ARM_FEATURE_M);
> +}
> +
> +static const VMStateDescription vmstate_pmsav8r = {
> + .name = "cpu/pmsav8/pmsav8r",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = pmsav8r_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_VARRAY_UINT32(env.pmsav8.hprbar[M_REG_NS], ARMCPU,
> + pmsav8r_hdregion, 0, vmstate_info_uint32, uint32_t),
> + VMSTATE_VARRAY_UINT32(env.pmsav8.hprlar[M_REG_NS], ARMCPU,
> + pmsav8r_hdregion, 0, vmstate_info_uint32, uint32_t),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> static const VMStateDescription vmstate_pmsav8 = {
> .name = "cpu/pmsav8",
> .version_id = 1,
> @@ -500,6 +524,10 @@ static const VMStateDescription vmstate_pmsav8 = {
> VMSTATE_UINT32(env.pmsav8.mair0[M_REG_NS], ARMCPU),
> VMSTATE_UINT32(env.pmsav8.mair1[M_REG_NS], ARMCPU),
> VMSTATE_END_OF_LIST()
> + },
> + .subsections = (const VMStateDescription * []) {
> + &vmstate_pmsav8r,
> + NULL
> }
> };
You'll need to adjust the vmstate field array a bit to allow for
hprbar and hprlar not being arrays, but otherwise the vmstate
changes look good.
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index db50715fa7..4bd7389fa9 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1718,6 +1718,13 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> bool hit = false;
> uint32_t addr_page_base = address & TARGET_PAGE_MASK;
> uint32_t addr_page_limit = addr_page_base + (TARGET_PAGE_SIZE - 1);
> + int region_counter;
> +
> + if (regime_el(env, mmu_idx) == 2) {
> + region_counter = cpu->pmsav8r_hdregion;
> + } else {
> + region_counter = cpu->pmsav7_dregion;
> + }
>
> result->page_size = TARGET_PAGE_SIZE;
> result->phys = address;
> @@ -1742,7 +1749,7 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> hit = true;
> }
>
> - for (n = (int)cpu->pmsav7_dregion - 1; n >= 0; n--) {
> + for (n = region_counter - 1; n >= 0; n--) {
> /* region search */
> /*
> * Note that the base address is bits [31:5] from the register
The changes to ptw.c here should be moved to the following patch.
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 6/7] target/arm: Add PMSAv8r functionality
2022-10-23 15:36 [PATCH v4 0/7] Add ARM Cortex-R52 CPU tobias.roehmel
` (4 preceding siblings ...)
2022-10-23 15:36 ` [PATCH v4 5/7] target/arm: Add PMSAv8r registers tobias.roehmel
@ 2022-10-23 15:36 ` tobias.roehmel
2022-11-18 15:03 ` Peter Maydell
2022-10-23 15:36 ` [PATCH v4 7/7] target/arm: Add ARM Cortex-R52 CPU tobias.roehmel
2022-11-18 15:03 ` [PATCH v4 0/7] " Peter Maydell
7 siblings, 1 reply; 18+ messages in thread
From: tobias.roehmel @ 2022-10-23 15:36 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel
From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
Add PMSAv8r translation.
Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
---
target/arm/ptw.c | 130 +++++++++++++++++++++++++++++++++++++++--------
1 file changed, 110 insertions(+), 20 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 4bd7389fa9..a5d890c09a 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1503,6 +1503,23 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx,
if (arm_feature(env, ARM_FEATURE_M)) {
return env->v7m.mpu_ctrl[is_secure] & R_V7M_MPU_CTRL_PRIVDEFENA_MASK;
+ } else if (arm_feature(env, ARM_FEATURE_PMSA)) {
+ if (regime_el(env, mmu_idx) == 2) {
+ if (mmu_idx != ARMMMUIdx_E2) {
+ return false;
+ } else if ((mmu_idx == ARMMMUIdx_E2)
+ &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
+ return false;
+ }
+ } else {
+ if (mmu_idx != ARMMMUIdx_Stage1_E1) {
+ return false;
+ } else if ((mmu_idx == ARMMMUIdx_Stage1_E1)
+ &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
+ return false;
+ }
+ }
+ return true;
} else {
return regime_sctlr(env, mmu_idx) & SCTLR_BR;
}
@@ -1696,6 +1713,26 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
return !(result->prot & (1 << access_type));
}
+static uint32_t *regime_rbar(CPUARMState *env, ARMMMUIdx mmu_idx,
+ uint32_t secure)
+{
+ if (regime_el(env, mmu_idx) == 2) {
+ return env->pmsav8.hprbar[secure];
+ } else {
+ return env->pmsav8.rbar[secure];
+ }
+}
+
+static uint32_t *regime_rlar(CPUARMState *env, ARMMMUIdx mmu_idx,
+ uint32_t secure)
+{
+ if (regime_el(env, mmu_idx) == 2) {
+ return env->pmsav8.hprlar[secure];
+ } else {
+ return env->pmsav8.rlar[secure];
+ }
+}
+
bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
MMUAccessType access_type, ARMMMUIdx mmu_idx,
bool secure, GetPhysAddrResult *result,
@@ -1733,6 +1770,10 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
*mregion = -1;
}
+ if (mmu_idx == ARMMMUIdx_Stage2) {
+ fi->stage2 = true;
+ }
+
/*
* Unlike the ARM ARM pseudocode, we don't need to check whether this
* was an exception vector read from the vector table (which is always
@@ -1749,17 +1790,27 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
hit = true;
}
+ uint32_t bitmask;
+ if (arm_feature(env, ARM_FEATURE_M)) {
+ bitmask = 0x1f;
+ fi->level = 1;
+ } else {
+ bitmask = 0x3f;
+ fi->level = 0;
+ }
+
for (n = region_counter - 1; n >= 0; n--) {
/* region search */
/*
- * Note that the base address is bits [31:5] from the register
- * with bits [4:0] all zeroes, but the limit address is bits
- * [31:5] from the register with bits [4:0] all ones.
+ * Note that the base address is bits [31:x] from the register
+ * with bits [x-1:0] all zeroes, but the limit address is bits
+ * [31:x] from the register with bits [x:0] all ones. Where x is
+ * 5 for Cortex-M and 6 for Cortex-R
*/
- uint32_t base = env->pmsav8.rbar[secure][n] & ~0x1f;
- uint32_t limit = env->pmsav8.rlar[secure][n] | 0x1f;
+ uint32_t base = regime_rbar(env, mmu_idx, secure)[n] & ~bitmask;
+ uint32_t limit = regime_rlar(env, mmu_idx, secure)[n] | bitmask;
- if (!(env->pmsav8.rlar[secure][n] & 0x1)) {
+ if (!(regime_rlar(env, mmu_idx, secure)[n] & 0x1)) {
/* Region disabled */
continue;
}
@@ -1793,7 +1844,6 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
* PMSAv7 where highest-numbered-region wins)
*/
fi->type = ARMFault_Permission;
- fi->level = 1;
return true;
}
@@ -1803,8 +1853,11 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
}
if (!hit) {
- /* background fault */
- fi->type = ARMFault_Background;
+ if (arm_feature(env, ARM_FEATURE_M)) {
+ fi->type = ARMFault_Background;
+ } else {
+ fi->type = ARMFault_Permission;
+ }
return true;
}
@@ -1812,12 +1865,14 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
/* hit using the background region */
get_phys_addr_pmsav7_default(env, mmu_idx, address, &result->prot);
} else {
- uint32_t ap = extract32(env->pmsav8.rbar[secure][matchregion], 1, 2);
- uint32_t xn = extract32(env->pmsav8.rbar[secure][matchregion], 0, 1);
+ uint32_t matched_rbar = regime_rbar(env, mmu_idx, secure)[matchregion];
+ uint32_t matched_rlar = regime_rlar(env, mmu_idx, secure)[matchregion];
+ uint32_t ap = extract32(matched_rbar, 1, 2);
+ uint32_t xn = extract32(matched_rbar, 0, 1);
bool pxn = false;
if (arm_feature(env, ARM_FEATURE_V8_1M)) {
- pxn = extract32(env->pmsav8.rlar[secure][matchregion], 4, 1);
+ pxn = extract32(matched_rlar, 4, 1);
}
if (m_is_system_region(env, address)) {
@@ -1825,21 +1880,49 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
xn = 1;
}
- result->prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
+ if (arm_feature(env, ARM_FEATURE_M)) {
+ /*
+ * We don't need to look the attribute up in the MAIR0/MAIR1
+ * registers because that only tells us about cacheability.
+ */
+ result->prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
+ } else {
+ if (regime_el(env, mmu_idx) == 2) {
+ result->prot = simple_ap_to_rw_prot_is_user(ap,
+ mmu_idx != ARMMMUIdx_E2);
+ } else {
+ result->prot = simple_ap_to_rw_prot_is_user(ap,
+ mmu_idx != ARMMMUIdx_Stage1_E1);
+ }
+
+ if (regime_sctlr(env, mmu_idx) & SCTLR_WXN
+ && (result->prot & PAGE_WRITE)) {
+ xn = 0x1;
+ }
+
+ if ((regime_el(env, mmu_idx) == 1) && regime_sctlr(env, mmu_idx)
+ & SCTLR_UWXN && (ap == 0x1)) {
+ xn = 0x1;
+ }
+
+ uint8_t attrindx = extract32(matched_rlar, 1, 3);
+ uint64_t mair = env->cp15.mair_el[regime_el(env, mmu_idx)];
+ uint8_t sh = extract32(matched_rlar, 3, 2);
+ result->cacheattrs.is_s2_format = false;
+ result->cacheattrs.attrs = extract64(mair, attrindx * 8, 8);
+ result->cacheattrs.shareability = sh;
+ }
+
if (result->prot && !xn && !(pxn && !is_user)) {
result->prot |= PAGE_EXEC;
}
- /*
- * We don't need to look the attribute up in the MAIR0/MAIR1
- * registers because that only tells us about cacheability.
- */
+
if (mregion) {
*mregion = matchregion;
}
}
fi->type = ARMFault_Permission;
- fi->level = 1;
return !(result->prot & (1 << access_type));
}
@@ -2348,8 +2431,15 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
cacheattrs1 = result->cacheattrs;
memset(result, 0, sizeof(*result));
- ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx,
- is_el0, result, fi);
+ /* S1 is done. Now do S2 translation. */
+ if (arm_feature(env, ARM_FEATURE_PMSA)) {
+ ret = get_phys_addr_pmsav8(env, ipa, access_type, s2_mmu_idx,
+ is_secure, result, fi);
+ } else {
+ ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx,
+ is_el0, result, fi);
+ }
+
fi->s2addr = ipa;
/* Combine the S1 and S2 perms. */
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 6/7] target/arm: Add PMSAv8r functionality
2022-10-23 15:36 ` [PATCH v4 6/7] target/arm: Add PMSAv8r functionality tobias.roehmel
@ 2022-11-18 15:03 ` Peter Maydell
0 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2022-11-18 15:03 UTC (permalink / raw)
To: tobias.roehmel; +Cc: qemu-devel
On Sun, 23 Oct 2022 at 16:37, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> Add PMSAv8r translation.
>
> Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
> ---
> target/arm/ptw.c | 130 +++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 110 insertions(+), 20 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 4bd7389fa9..a5d890c09a 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1503,6 +1503,23 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx,
>
> if (arm_feature(env, ARM_FEATURE_M)) {
> return env->v7m.mpu_ctrl[is_secure] & R_V7M_MPU_CTRL_PRIVDEFENA_MASK;
> + } else if (arm_feature(env, ARM_FEATURE_PMSA)) {
> + if (regime_el(env, mmu_idx) == 2) {
> + if (mmu_idx != ARMMMUIdx_E2) {
> + return false;
> + } else if ((mmu_idx == ARMMMUIdx_E2)
> + &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> + return false;
> + }
> + } else {
> + if (mmu_idx != ARMMMUIdx_Stage1_E1) {
> + return false;
> + } else if ((mmu_idx == ARMMMUIdx_Stage1_E1)
> + &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> + return false;
> + }
> + }
> + return true;
> } else {
> return regime_sctlr(env, mmu_idx) & SCTLR_BR;
> }
This logic seems rather confused:
(1) it's not possible to get to this function unless ARM_FEATURE_PMSA,
so the explicit check for that means the final "else" clause is now
unreachable code.
(2) You check for eg "mmu_idx != ARMMMUIdx_E2" and return early,
but that means that in the following
((mmu_idx == ARMMMUIdx_E2)
&&!(regime_sctlr(env, mmu_idx) & SCTLR_BR))
the first clause of the && is always true and is redundant.
(3) the thing we end up actually checking (SCTLR_BR for the
regime SCTLR) is the same now in all three major branches of
this code, so there's a lot of redundancy.
I think what you actually want here is to identify the set
mmu index values (if any!) that we don't want to do the
SCTLR_BR check for. Then return early for those, and have
a single
return regime_sctlr(env, mmu_idx) & SCTLR_BR;
for all the cases where checking BR is the right thing.
I think it may actually be the case that there is no mmuidx
value where we don't want to do the SCLTR_BR check, in
which case we don't need to change this function at all.
> @@ -1696,6 +1713,26 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
> return !(result->prot & (1 << access_type));
> }
>
> +static uint32_t *regime_rbar(CPUARMState *env, ARMMMUIdx mmu_idx,
> + uint32_t secure)
> +{
> + if (regime_el(env, mmu_idx) == 2) {
> + return env->pmsav8.hprbar[secure];
> + } else {
> + return env->pmsav8.rbar[secure];
> + }
> +}
> +
> +static uint32_t *regime_rlar(CPUARMState *env, ARMMMUIdx mmu_idx,
> + uint32_t secure)
> +{
> + if (regime_el(env, mmu_idx) == 2) {
> + return env->pmsav8.hprlar[secure];
> + } else {
> + return env->pmsav8.rlar[secure];
> + }
> +}
> +
> bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> MMUAccessType access_type, ARMMMUIdx mmu_idx,
> bool secure, GetPhysAddrResult *result,
> @@ -1733,6 +1770,10 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> *mregion = -1;
> }
>
> + if (mmu_idx == ARMMMUIdx_Stage2) {
> + fi->stage2 = true;
> + }
> +
> /*
> * Unlike the ARM ARM pseudocode, we don't need to check whether this
> * was an exception vector read from the vector table (which is always
> @@ -1749,17 +1790,27 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> hit = true;
> }
>
> + uint32_t bitmask;
> + if (arm_feature(env, ARM_FEATURE_M)) {
> + bitmask = 0x1f;
> + fi->level = 1;
> + } else {
> + bitmask = 0x3f;
> + fi->level = 0;
> + }
We could in theory clean this up a bit, because on M-profile the
"FSR" value is not guest-facing; we only use it internally to pass
around some details of the fault cause, so as long as we're consistent
between the code here that sets fi->level and the code in m_helper.c
that looks at the FSC values we can set it to any value that's convenient.
But for this patchset we should definitely leave the existing M-profile
behaviour the way it is. I might come back and tweak the code once
this R-profile series has landed (or I might not think it worth bothering
and leave it indefinitely :-))
> +
> for (n = region_counter - 1; n >= 0; n--) {
> /* region search */
> /*
> - * Note that the base address is bits [31:5] from the register
> - * with bits [4:0] all zeroes, but the limit address is bits
> - * [31:5] from the register with bits [4:0] all ones.
> + * Note that the base address is bits [31:x] from the register
> + * with bits [x-1:0] all zeroes, but the limit address is bits
> + * [31:x] from the register with bits [x:0] all ones. Where x is
> + * 5 for Cortex-M and 6 for Cortex-R
> */
> - uint32_t base = env->pmsav8.rbar[secure][n] & ~0x1f;
> - uint32_t limit = env->pmsav8.rlar[secure][n] | 0x1f;
> + uint32_t base = regime_rbar(env, mmu_idx, secure)[n] & ~bitmask;
> + uint32_t limit = regime_rlar(env, mmu_idx, secure)[n] | bitmask;
>
> - if (!(env->pmsav8.rlar[secure][n] & 0x1)) {
> + if (!(regime_rlar(env, mmu_idx, secure)[n] & 0x1)) {
> /* Region disabled */
> continue;
> }
> @@ -1793,7 +1844,6 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> * PMSAv7 where highest-numbered-region wins)
> */
> fi->type = ARMFault_Permission;
> - fi->level = 1;
> return true;
> }
>
> @@ -1803,8 +1853,11 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> }
>
> if (!hit) {
> - /* background fault */
> - fi->type = ARMFault_Background;
> + if (arm_feature(env, ARM_FEATURE_M)) {
> + fi->type = ARMFault_Background;
> + } else {
> + fi->type = ARMFault_Permission;
> + }
> return true;
> }
>
> @@ -1812,12 +1865,14 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> /* hit using the background region */
> get_phys_addr_pmsav7_default(env, mmu_idx, address, &result->prot);
> } else {
> - uint32_t ap = extract32(env->pmsav8.rbar[secure][matchregion], 1, 2);
> - uint32_t xn = extract32(env->pmsav8.rbar[secure][matchregion], 0, 1);
> + uint32_t matched_rbar = regime_rbar(env, mmu_idx, secure)[matchregion];
> + uint32_t matched_rlar = regime_rlar(env, mmu_idx, secure)[matchregion];
> + uint32_t ap = extract32(matched_rbar, 1, 2);
> + uint32_t xn = extract32(matched_rbar, 0, 1);
> bool pxn = false;
>
> if (arm_feature(env, ARM_FEATURE_V8_1M)) {
> - pxn = extract32(env->pmsav8.rlar[secure][matchregion], 4, 1);
> + pxn = extract32(matched_rlar, 4, 1);
> }
>
> if (m_is_system_region(env, address)) {
> @@ -1825,21 +1880,49 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> xn = 1;
> }
>
> - result->prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
> + if (arm_feature(env, ARM_FEATURE_M)) {
> + /*
> + * We don't need to look the attribute up in the MAIR0/MAIR1
> + * registers because that only tells us about cacheability.
> + */
> + result->prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
> + } else {
> + if (regime_el(env, mmu_idx) == 2) {
> + result->prot = simple_ap_to_rw_prot_is_user(ap,
> + mmu_idx != ARMMMUIdx_E2);
> + } else {
> + result->prot = simple_ap_to_rw_prot_is_user(ap,
> + mmu_idx != ARMMMUIdx_Stage1_E1);
This second one should be fine as just
result->prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
I think, because this is the EL1 case.
That in turn means you don't need to have the M-profile case separately,
because M-profile will never have the regime EL be 2.
> + }
> +
> + if (regime_sctlr(env, mmu_idx) & SCTLR_WXN
> + && (result->prot & PAGE_WRITE)) {
> + xn = 0x1;
> + }
I think that this will apply HSCTLR.WXN on the stage 2 check for
EL1/EL0 accesses, which I don't think is correct. In the pseudocode
HSCTLR.WXN is checked only for stage 1 accesses from EL2, not as
part of stage 2 checking (see AArch32.CheckPermission()).
> +
> + if ((regime_el(env, mmu_idx) == 1) && regime_sctlr(env, mmu_idx)
> + & SCTLR_UWXN && (ap == 0x1)) {
Don't break the line like this, it implies that the precedence
of & is less than that of &&, which it isn't.
> + xn = 0x1;
This should be setting pxn = 0x1, because SCTLR.UWXN only means
"unprivileged write permission implies EL1 XN", not "implies XN generally".
> + }
> +
> + uint8_t attrindx = extract32(matched_rlar, 1, 3);
> + uint64_t mair = env->cp15.mair_el[regime_el(env, mmu_idx)];
> + uint8_t sh = extract32(matched_rlar, 3, 2);
> + result->cacheattrs.is_s2_format = false;
> + result->cacheattrs.attrs = extract64(mair, attrindx * 8, 8);
> + result->cacheattrs.shareability = sh;
> + }
> +
> if (result->prot && !xn && !(pxn && !is_user)) {
> result->prot |= PAGE_EXEC;
> }
> - /*
> - * We don't need to look the attribute up in the MAIR0/MAIR1
> - * registers because that only tells us about cacheability.
> - */
> +
> if (mregion) {
> *mregion = matchregion;
> }
> }
>
> fi->type = ARMFault_Permission;
> - fi->level = 1;
> return !(result->prot & (1 << access_type));
> }
>
> @@ -2348,8 +2431,15 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
> cacheattrs1 = result->cacheattrs;
> memset(result, 0, sizeof(*result));
>
> - ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx,
> - is_el0, result, fi);
> + /* S1 is done. Now do S2 translation. */
> + if (arm_feature(env, ARM_FEATURE_PMSA)) {
> + ret = get_phys_addr_pmsav8(env, ipa, access_type, s2_mmu_idx,
> + is_secure, result, fi);
> + } else {
> + ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx,
> + is_el0, result, fi);
> + }
> +
This bit of code has unfortunately changed a lot due to a
recent refactoring landing...
> fi->s2addr = ipa;
>
> /* Combine the S1 and S2 perms. */
> --
> 2.34.1
thanks
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 7/7] target/arm: Add ARM Cortex-R52 CPU
2022-10-23 15:36 [PATCH v4 0/7] Add ARM Cortex-R52 CPU tobias.roehmel
` (5 preceding siblings ...)
2022-10-23 15:36 ` [PATCH v4 6/7] target/arm: Add PMSAv8r functionality tobias.roehmel
@ 2022-10-23 15:36 ` tobias.roehmel
2022-11-18 14:04 ` Peter Maydell
2022-11-18 15:03 ` [PATCH v4 0/7] " Peter Maydell
7 siblings, 1 reply; 18+ messages in thread
From: tobias.roehmel @ 2022-10-23 15:36 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel
From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
All constants are taken from the ARM Cortex-R52 Processor TRM Revision: r1p3
Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
---
target/arm/cpu_tcg.c | 42 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
index 98b5ba2160..52b9d671f7 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -851,6 +851,47 @@ static void cortex_r5_initfn(Object *obj)
define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
}
+static void cortex_r52_initfn(Object *obj)
+{
+ ARMCPU *cpu = ARM_CPU(obj);
+
+ set_feature(&cpu->env, ARM_FEATURE_V8);
+ set_feature(&cpu->env, ARM_FEATURE_EL2);
+ set_feature(&cpu->env, ARM_FEATURE_PMSA);
+ set_feature(&cpu->env, ARM_FEATURE_NEON);
+ set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+ cpu->midr = 0x411fd133; /* r1p3 */
+ cpu->revidr = 0x00000000;
+ cpu->reset_fpsid = 0x41034023;
+ cpu->isar.mvfr0 = 0x10110222;
+ cpu->isar.mvfr1 = 0x12111111;
+ cpu->isar.mvfr2 = 0x00000043;
+ cpu->ctr = 0x8144c004;
+ cpu->reset_sctlr = 0x30c50838;
+ cpu->isar.id_pfr0 = 0x00000131;
+ cpu->isar.id_pfr1 = 0x10111001;
+ cpu->isar.id_dfr0 = 0x03010006;
+ cpu->id_afr0 = 0x00000000;
+ cpu->isar.id_mmfr0 = 0x00211040;
+ cpu->isar.id_mmfr1 = 0x40000000;
+ cpu->isar.id_mmfr2 = 0x01200000;
+ cpu->isar.id_mmfr3 = 0xf0102211;
+ cpu->isar.id_mmfr4 = 0x00000010;
+ cpu->isar.id_isar0 = 0x02101110;
+ cpu->isar.id_isar1 = 0x13112111;
+ cpu->isar.id_isar2 = 0x21232142;
+ cpu->isar.id_isar3 = 0x01112131;
+ cpu->isar.id_isar4 = 0x00010142;
+ cpu->isar.id_isar5 = 0x00010001;
+ cpu->isar.dbgdidr = 0x77168000;
+ cpu->clidr = (1 << 27) | (1 << 24) | 0x3;
+ cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
+ cpu->ccsidr[1] = 0x201fe00a; /* 32KB L1 icache */
+
+ cpu->pmsav7_dregion = 16;
+ cpu->pmsav8r_hdregion = 16;
+}
+
static void cortex_r5f_initfn(Object *obj)
{
ARMCPU *cpu = ARM_CPU(obj);
@@ -1159,6 +1200,7 @@ static const ARMCPUInfo arm_tcg_cpus[] = {
.class_init = arm_v7m_class_init },
{ .name = "cortex-r5", .initfn = cortex_r5_initfn },
{ .name = "cortex-r5f", .initfn = cortex_r5f_initfn },
+ { .name = "cortex-r52", .initfn = cortex_r52_initfn },
{ .name = "ti925t", .initfn = ti925t_initfn },
{ .name = "sa1100", .initfn = sa1100_initfn },
{ .name = "sa1110", .initfn = sa1110_initfn },
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 0/7] Add ARM Cortex-R52 CPU
2022-10-23 15:36 [PATCH v4 0/7] Add ARM Cortex-R52 CPU tobias.roehmel
` (6 preceding siblings ...)
2022-10-23 15:36 ` [PATCH v4 7/7] target/arm: Add ARM Cortex-R52 CPU tobias.roehmel
@ 2022-11-18 15:03 ` Peter Maydell
7 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2022-11-18 15:03 UTC (permalink / raw)
To: tobias.roehmel; +Cc: qemu-devel, Tobias Röhmel
On Sun, 23 Oct 2022 at 16:37, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <quic_trohmel@quicinc.com>
>
> Thanks again for all the help!
>
> Here is v4:
> 2. Made patch cleaner
> 3. Changed commit message
> 4. Replaced V8_R flag with ARM_FEATURE_PMSA|ARM_FEATURE_V8
> 5.
> Reworked the code to use existing pmsav7 variables
> Added migration support
> Added VSCTLR:
> I didn't add any functionality for it because I think
> Qemu doesn't model the behaviour it influences.
> 6.
> Lots of cleanup. I think I overcomplicated this a bit because
> of a misunderstanding. I thought HCR_VM is independent of enabling
> the different MPUs, but I see now that it doesn't make sense to enable
> HCR_VM when the MPUs are not enabled. I also think that there is an Error
> in the armv8-r manual supplement in Figure C1-3. With all that figured out
> the code for pmsav8r doesn't look that different from pmsav7 :)
I've now reviewed this version of the patchset; it's getting
quite close now, I think. Sorry it took me so long to get back to it.
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread