All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/9] Add ARM Cortex-R52 cpu
@ 2022-08-20 14:19 tobias.roehmel
  2022-08-20 14:19 ` [PATCH v3 1/9] target/arm: Add ARM_FEATURE_V8_R tobias.roehmel
                   ` (9 more replies)
  0 siblings, 10 replies; 20+ messages in thread
From: tobias.roehmel @ 2022-08-20 14:19 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel

From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>

Thanks for the review!
Here is v3:

v3:
PATCH 2 (Don't add all MIDR aliases for cores that immplement PMSA):
fixed the comment and changed to single element instead of array.
Also the alias is not added for all PMSA CPUs as Peter suggested.
PATCH 3 (Make RVBAR available for all ARMv8 CPUs):
Added the missing RVBAR register as alias to RVBAR_ELx.
Tested that this code actually takes the lower 32 bits
of RVBAR_ELx as RVBAR.

v2:
PATCH 1:
I have left the flag in for now because there there is a lot of use for it in the MPU translation code.
The PMSAv8r differs in quite a view ways from the implementation in the Cortex-M. I think using
!ARM_FEATURE_M in all of those cases might run into problems down the road when new things are added.
But I'll gladly change that if those concerns are not valid.
PATCH 2:
Patch was moved and I removed the ATCM... registers.
PATCH 3:
The issue here is that the R52 has the MPUIR register which shares the encoding with one of the
MIDR alias registers. It's now changed to not add that register for ARM_FEATURE_V8_R.
PATCH 4:
Added RVBAR for all v8 CPUs instead of just ARMv8r
PATCH 7:
Instead of setting TTBCR_EAE to 1, change all functions that rely on that value and are
relevant for Cortex-R52
PATCH 10:
SPSR_hyp changes removed
PATCH 11:
Removed the r52_machine. The issue with adding the Cortex-R52 to the virt board is that target_page.bits
is expected to be 12 but is set to 10 for ARM_FEATURE_PMSA. Simply changing that or using
virt2.7(which doesn't have that restriction) leads to problems with memory accesses. It might be
best to model an existing board.

These patches add the ARM Cortex-R52. The biggest addition is an implementation of the armv8-r MPU.

All information is taken from:
- ARM Cortex-R52 TRM revision r1p3
- ARM Architecture Reference Manual Supplement
    -ARMv8 for the ARMv8-R AArch32 architecture profile Version A.c

Functionality that is not implemented:
- Changing between single and double precision floats
- Some hypervisor related functionality (HCR.T(R)VM,HADFSR,...)


Tobias Röhmel (9):
  target/arm: Add ARM_FEATURE_V8_R
  target/arm: Don't add all MIDR aliases for cores that immplement PMSA
  target/arm: Make RVBAR available for all ARMv8 CPUs
  target/arm: Make stage_2_format for cache attributes optional
  target/arm: Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup
  target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
  target/arm: Add PMSAv8r registers
  target/arm: Add PMSAv8r functionality
  target/arm: Add ARM Cortex-R52 cpu

 target/arm/cpu.c          |   6 +-
 target/arm/cpu.h          |  11 ++
 target/arm/cpu_tcg.c      |  42 +++++++
 target/arm/debug_helper.c |   3 +-
 target/arm/helper.c       | 223 +++++++++++++++++++++++++++++++++++---
 target/arm/internals.h    |  16 +--
 target/arm/m_helper.c     |   3 +-
 target/arm/ptw.c          | 191 +++++++++++++++++++++++++++-----
 target/arm/tlb_helper.c   |   3 +-
 9 files changed, 444 insertions(+), 54 deletions(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 1/9] target/arm: Add ARM_FEATURE_V8_R
  2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
@ 2022-08-20 14:19 ` tobias.roehmel
  2022-08-20 14:19 ` [PATCH v3 2/9] target/arm: Don't add all MIDR aliases for cores that immplement PMSA tobias.roehmel
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: tobias.roehmel @ 2022-08-20 14:19 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel

From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>

This flag is necessary to add features for the Cortex-R52.

Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
---
 target/arm/cpu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index df677b2d5d..86e06116a9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2287,6 +2287,7 @@ enum arm_features {
     ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
     ARM_FEATURE_M_MAIN, /* M profile Main Extension */
     ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
+    ARM_FEATURE_V8_R,
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 2/9] target/arm: Don't add all MIDR aliases for cores that immplement PMSA
  2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
  2022-08-20 14:19 ` [PATCH v3 1/9] target/arm: Add ARM_FEATURE_V8_R tobias.roehmel
@ 2022-08-20 14:19 ` tobias.roehmel
  2022-09-27 16:29   ` Peter Maydell
  2022-08-20 14:19 ` [PATCH v3 3/9] target/arm: Make RVBAR available for all ARMv8 CPUs tobias.roehmel
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: tobias.roehmel @ 2022-08-20 14:19 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 6457e6301c..b9547594ae 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8188,10 +8188,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 },
@@ -8201,6 +8198,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .accessfn = access_aa64_tid1,
               .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
         };
+        ARMCPRegInfo id_v8_midr_alias_cp_reginfo = {
+              .name = "MIDR", .type = ARM_CP_ALIAS | ARM_CP_CONST,
+              .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
+              .access = PL1_R, .resetvalue = cpu->midr
+        };
         ARMCPRegInfo id_cp_reginfo[] = {
             /* These are common to v8 and pre-v8 */
             { .name = "CTR",
@@ -8264,8 +8266,12 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             id_mpuir_reginfo.access = PL1_RW;
             id_tlbtr_reginfo.access = PL1_RW;
         }
+
         if (arm_feature(env, ARM_FEATURE_V8)) {
             define_arm_cp_regs(cpu, id_v8_midr_cp_reginfo);
+            if (!arm_feature(env, ARM_FEATURE_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.25.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 3/9] target/arm: Make RVBAR available for all ARMv8 CPUs
  2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
  2022-08-20 14:19 ` [PATCH v3 1/9] target/arm: Add ARM_FEATURE_V8_R tobias.roehmel
  2022-08-20 14:19 ` [PATCH v3 2/9] target/arm: Don't add all MIDR aliases for cores that immplement PMSA tobias.roehmel
@ 2022-08-20 14:19 ` tobias.roehmel
  2022-09-27 13:01   ` Peter Maydell
  2022-08-20 14:19 ` [PATCH v3 4/9] target/arm: Make stage_2_format for cache attributes optional tobias.roehmel
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: tobias.roehmel @ 2022-08-20 14:19 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 to be changed with
the rvbar property.

Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
---
 target/arm/cpu.c    |  6 +++++-
 target/arm/helper.c | 38 ++++++++++++++++++++++++++------------
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 1b5d535788..9007768418 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -258,6 +258,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)
@@ -1273,7 +1277,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 b9547594ae..23461397e0 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7954,13 +7954,20 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         /* RVBAR_EL1 is only implemented if EL1 is the highest EL */
         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,
-                .access = PL1_R,
-                .fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+            ARMCPRegInfo rvbar[] = {
+                {
+                    .name = "RVBAR_EL1", .state = ARM_CP_STATE_AA64,
+                    .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
+                    .access = PL1_R,
+                    .fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+                },
+                {   .name = "RVBAR", .type = ARM_CP_ALIAS,
+                    .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 1,
+                    .access = PL1_R,
+                    .fieldoffset = offsetof(CPUARMState, cp15.rvbar),
+                },
             };
-            define_one_arm_cp_reg(cpu, &rvbar);
+            define_arm_cp_regs(cpu, rvbar);
         }
         define_arm_cp_regs(cpu, v8_idregs);
         define_arm_cp_regs(cpu, v8_cp_reginfo);
@@ -8022,13 +8029,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, .crn = 12, .crm = 0, .opc1 = 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.25.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 4/9] target/arm: Make stage_2_format for cache attributes optional
  2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
                   ` (2 preceding siblings ...)
  2022-08-20 14:19 ` [PATCH v3 3/9] target/arm: Make RVBAR available for all ARMv8 CPUs tobias.roehmel
@ 2022-08-20 14:19 ` tobias.roehmel
  2022-09-27 12:52   ` Peter Maydell
  2022-08-20 14:19 ` [PATCH v3 5/9] target/arm: Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup tobias.roehmel
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: tobias.roehmel @ 2022-08-20 14:19 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel

From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>

The Cortex-R52 has a 2 stage MPU translation process but doesn't have the
FEAT_S2FWB feature. This makes it neccessary to allow for the old cache
attribut combination. This is facilitated by changing the control path
of combine_cacheattrs instead of failing if the second cache attributes
struct is not in that format.

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 4d97a24808..8b037c1f55 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2108,7 +2108,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);
@@ -2166,6 +2170,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 */
@@ -2215,7 +2221,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.25.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 5/9] target/arm: Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup
  2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
                   ` (3 preceding siblings ...)
  2022-08-20 14:19 ` [PATCH v3 4/9] target/arm: Make stage_2_format for cache attributes optional tobias.roehmel
@ 2022-08-20 14:19 ` tobias.roehmel
  2022-09-27 13:05   ` Peter Maydell
  2022-08-20 14:19 ` [PATCH v3 6/9] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32 tobias.roehmel
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: tobias.roehmel @ 2022-08-20 14:19 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, Tobias Röhmel

From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>

Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup to prepare
for the Cortex-R52 MPU which uses and combines cache attributes
of different translation levels.

Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
---
 target/arm/internals.h | 13 +++++++------
 target/arm/m_helper.c  |  3 ++-
 target/arm/ptw.c       | 11 +++++++----
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 6f94f3019d..b03049d920 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1109,12 +1109,6 @@ void v8m_security_lookup(CPUARMState *env, uint32_t address,
                          MMUAccessType access_type, ARMMMUIdx mmu_idx,
                          V8M_SAttributes *sattrs);
 
-bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
-                       MMUAccessType access_type, ARMMMUIdx mmu_idx,
-                       hwaddr *phys_ptr, MemTxAttrs *txattrs,
-                       int *prot, bool *is_subpage,
-                       ARMMMUFaultInfo *fi, uint32_t *mregion);
-
 /* Cacheability and shareability attributes for a memory access */
 typedef struct ARMCacheAttrs {
     /*
@@ -1126,6 +1120,13 @@ typedef struct ARMCacheAttrs {
     bool is_s2_format:1;
 } ARMCacheAttrs;
 
+bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
+                       MMUAccessType access_type, ARMMMUIdx mmu_idx,
+                       hwaddr *phys_ptr, MemTxAttrs *txattrs,
+                       int *prot, bool *is_subpage,
+                       ARMMMUFaultInfo *fi, uint32_t *mregion,
+                       ARMCacheAttrs *cacheattrs);
+
 bool get_phys_addr(CPUARMState *env, target_ulong address,
                    MMUAccessType access_type, ARMMMUIdx mmu_idx,
                    hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index a740c3e160..44c80d733a 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2829,10 +2829,11 @@ uint32_t HELPER(v7m_tt)(CPUARMState *env, uint32_t addr, uint32_t op)
      * inspecting the other MPU state.
      */
     if (arm_current_el(env) != 0 || alt) {
+        ARMCacheAttrs cacheattrs = {0};
         /* We can ignore the return value as prot is always set */
         pmsav8_mpu_lookup(env, addr, MMU_DATA_LOAD, mmu_idx,
                           &phys_addr, &attrs, &prot, &is_subpage,
-                          &fi, &mregion);
+                          &fi, &mregion, &cacheattrs);
         if (mregion == -1) {
             mrvalid = false;
             mregion = 0;
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 8b037c1f55..c4f5721012 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1702,7 +1702,8 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
                        MMUAccessType access_type, ARMMMUIdx mmu_idx,
                        hwaddr *phys_ptr, MemTxAttrs *txattrs,
                        int *prot, bool *is_subpage,
-                       ARMMMUFaultInfo *fi, uint32_t *mregion)
+                       ARMMMUFaultInfo *fi, uint32_t *mregion,
+                       ARMCacheAttrs *cacheattrs)
 {
     /*
      * Perform a PMSAv8 MPU lookup (without also doing the SAU check
@@ -1968,7 +1969,7 @@ static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
                                  MMUAccessType access_type, ARMMMUIdx mmu_idx,
                                  hwaddr *phys_ptr, MemTxAttrs *txattrs,
                                  int *prot, target_ulong *page_size,
-                                 ARMMMUFaultInfo *fi)
+                                 ARMMMUFaultInfo *fi, ARMCacheAttrs *cacheattrs)
 {
     uint32_t secure = regime_is_secure(env, mmu_idx);
     V8M_SAttributes sattrs = {};
@@ -2036,7 +2037,8 @@ static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
     }
 
     ret = pmsav8_mpu_lookup(env, address, access_type, mmu_idx, phys_ptr,
-                            txattrs, prot, &mpu_is_subpage, fi, NULL);
+                            txattrs, prot, &mpu_is_subpage, fi,
+                            NULL, cacheattrs);
     *page_size = sattrs.subpage || mpu_is_subpage ? 1 : TARGET_PAGE_SIZE;
     return ret;
 }
@@ -2416,7 +2418,8 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
         if (arm_feature(env, ARM_FEATURE_V8)) {
             /* PMSAv8 */
             ret = get_phys_addr_pmsav8(env, address, access_type, mmu_idx,
-                                       phys_ptr, attrs, prot, page_size, fi);
+                                       phys_ptr, attrs, prot, page_size,
+                                       fi, cacheattrs);
         } else if (arm_feature(env, ARM_FEATURE_V7)) {
             /* PMSAv7 */
             ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 6/9] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
  2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
                   ` (4 preceding siblings ...)
  2022-08-20 14:19 ` [PATCH v3 5/9] target/arm: Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup tobias.roehmel
@ 2022-08-20 14:19 ` tobias.roehmel
  2022-09-27 13:20   ` Peter Maydell
  2022-08-20 14:19 ` [PATCH v3 7/9] target/arm: Add PMSAv8r registers tobias.roehmel
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: tobias.roehmel @ 2022-08-20 14:19 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    | 3 ++-
 target/arm/tlb_helper.c   | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index b18a6bd3a2..44b1e32974 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -434,7 +434,8 @@ static uint32_t arm_debug_exception_fsr(CPUARMState *env)
         using_lpae = true;
     } else {
         if (arm_feature(env, ARM_FEATURE_LPAE) &&
-            (env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)) {
+            ((env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)
+            || arm_feature(env, ARM_FEATURE_V8_R))) {
             using_lpae = true;
         }
     }
diff --git a/target/arm/internals.h b/target/arm/internals.h
index b03049d920..e2a2b03d41 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -254,7 +254,8 @@ static inline bool extended_addresses_enabled(CPUARMState *env)
 {
     TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
     return arm_el_is_aa64(env, 1) ||
-           (arm_feature(env, ARM_FEATURE_LPAE) && (tcr->raw_tcr & TTBCR_EAE));
+           (arm_feature(env, ARM_FEATURE_LPAE) && ((tcr->raw_tcr & TTBCR_EAE)
+           || arm_feature(env, ARM_FEATURE_V8_R)));
 }
 
 /* Update a QEMU watchpoint based on the information the guest has set in the
diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
index 7d8a86b3c4..891326edb8 100644
--- a/target/arm/tlb_helper.c
+++ b/target/arm/tlb_helper.c
@@ -20,7 +20,8 @@ bool regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx)
         return true;
     }
     if (arm_feature(env, ARM_FEATURE_LPAE)
-        && (regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)) {
+        && ((regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)
+        || arm_feature(env, ARM_FEATURE_V8_R))) {
         return true;
     }
     return false;
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 7/9] target/arm: Add PMSAv8r registers
  2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
                   ` (5 preceding siblings ...)
  2022-08-20 14:19 ` [PATCH v3 6/9] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32 tobias.roehmel
@ 2022-08-20 14:19 ` tobias.roehmel
  2022-09-27 13:50   ` Peter Maydell
  2022-08-20 14:19 ` [PATCH v3 8/9] target/arm: Add PMSAv8r functionality tobias.roehmel
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: tobias.roehmel @ 2022-08-20 14:19 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.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



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 8/9] target/arm: Add PMSAv8r functionality
  2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
                   ` (6 preceding siblings ...)
  2022-08-20 14:19 ` [PATCH v3 7/9] target/arm: Add PMSAv8r registers tobias.roehmel
@ 2022-08-20 14:19 ` tobias.roehmel
  2022-09-27 16:23   ` Peter Maydell
  2022-08-20 14:19 ` [PATCH v3 9/9] target/arm: Add ARM Cortex-R52 cpu tobias.roehmel
  2022-09-27 16:31 ` [PATCH v3 0/9] " Peter Maydell
  9 siblings, 1 reply; 20+ messages in thread
From: tobias.roehmel @ 2022-08-20 14:19 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 | 171 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 150 insertions(+), 21 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index c4f5721012..c7e37c66d0 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -140,6 +140,9 @@ static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx)
              */
             return true;
         }
+    } else if (arm_feature(env, ARM_FEATURE_V8_R)) {
+        return !(regime_sctlr(env, mmu_idx) & SCTLR_M) ||
+        (!(regime_el(env, mmu_idx) == 2) && arm_hcr_el2_eff(env) & HCR_TGE);
     }
 
     hcr_el2 = arm_hcr_el2_eff(env);
@@ -1504,6 +1507,8 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx,
     if (arm_feature(env, ARM_FEATURE_M)) {
         return env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)]
             & R_V7M_MPU_CTRL_PRIVDEFENA_MASK;
+    } else if (arm_feature(env, ARM_FEATURE_V8_R)) {
+        return false;
     } else {
         return regime_sctlr(env, mmu_idx) & SCTLR_BR;
     }
@@ -1698,6 +1703,77 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
     return !(*prot & (1 << access_type));
 }
 
+static uint32_t *regime_rbar(CPUARMState *env, ARMMMUIdx mmu_idx,
+                             uint32_t secure)
+{
+    if (arm_feature(env, ARM_FEATURE_V8_R)) {
+        if (regime_el(env, mmu_idx) == 2) {
+            return env->pmsav8.hprbarn;
+        } else {
+            return env->pmsav8.prbarn;
+        }
+    } else {
+         return env->pmsav8.rbar[secure];
+    }
+}
+
+static uint32_t *regime_rlar(CPUARMState *env, ARMMMUIdx mmu_idx,
+                             uint32_t secure)
+{
+    if (arm_feature(env, ARM_FEATURE_V8_R)) {
+        if (regime_el(env, mmu_idx) == 2) {
+            return env->pmsav8.hprlarn;
+        } else {
+            return env->pmsav8.prlarn;
+        }
+    } else {
+        return env->pmsav8.rlar[secure];
+    }
+}
+
+static inline void get_phys_addr_pmsav8_default(CPUARMState *env,
+                                                ARMMMUIdx mmu_idx,
+                                                uint32_t address, int *prot)
+{
+    if (arm_feature(env, ARM_FEATURE_V8_R)) {
+        *prot = PAGE_READ | PAGE_WRITE;
+        if (address <= 0x7FFFFFFF) {
+            *prot |= PAGE_EXEC;
+        }
+        if ((regime_el(env, mmu_idx) == 2)
+            && (regime_sctlr(env, mmu_idx) & SCTLR_WXN)
+            && (regime_sctlr(env, mmu_idx) & SCTLR_M)) {
+            *prot &= ~PAGE_EXEC;
+        }
+    } else {
+        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
+    }
+}
+
+static bool pmsav8_fault(bool hit, CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    if (arm_feature(env, ARM_FEATURE_V8_R)) {
+        if (regime_el(env, mmu_idx) == 2) {
+            if (!hit && (mmu_idx != ARMMMUIdx_E2)) {
+                return true;
+            } else if (!hit && (mmu_idx == ARMMMUIdx_E2)
+                       &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
+                return true;
+            }
+        } else {
+            if (!hit && (mmu_idx != ARMMMUIdx_Stage1_E1)) {
+                return true;
+            } else if (!hit && (mmu_idx == ARMMMUIdx_Stage1_E1)
+                       &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
+                return true;
+            }
+        }
+        return false;
+    } else {
+        return !hit;
+    }
+}
+
 bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
                        MMUAccessType access_type, ARMMMUIdx mmu_idx,
                        hwaddr *phys_ptr, MemTxAttrs *txattrs,
@@ -1730,6 +1806,12 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
         *mregion = -1;
     }
 
+    if (arm_feature(env, ARM_FEATURE_V8_R)) {
+        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
@@ -1746,17 +1828,26 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
             hit = true;
         }
 
+        uint32_t bitmask;
+        if (arm_feature(env, ARM_FEATURE_V8_R)) {
+            bitmask = 0x3f;
+        } else {
+            bitmask = 0x1f;
+        }
+
+
         for (n = (int)cpu->pmsav7_dregion - 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;
             }
@@ -1799,22 +1890,25 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
         }
     }
 
-    if (!hit) {
-        /* background fault */
-        fi->type = ARMFault_Background;
+    if (pmsav8_fault(hit, env, mmu_idx)) {
+        fi->type = ARMFault_Permission;
+        fi->level = 0;
         return true;
     }
 
     if (matchregion == -1) {
         /* hit using the background region */
-        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
+        get_phys_addr_pmsav8_default(env, mmu_idx, address, 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 ap = extract32(regime_rbar(env,
+                                mmu_idx, secure)[matchregion], 1, 2);
+        uint32_t xn = extract32(regime_rbar(env,
+                                mmu_idx, secure)[matchregion], 0, 1);
         bool pxn = false;
 
         if (arm_feature(env, ARM_FEATURE_V8_1M)) {
-            pxn = extract32(env->pmsav8.rlar[secure][matchregion], 4, 1);
+            pxn = extract32(regime_rlar(env,
+                            mmu_idx, secure)[matchregion], 4, 1);
         }
 
         if (m_is_system_region(env, address)) {
@@ -1822,14 +1916,42 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
             xn = 1;
         }
 
-        *prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
+        if (arm_feature(env, ARM_FEATURE_V8_R)) {
+            if (regime_el(env, mmu_idx) == 2) {
+                *prot = simple_ap_to_rw_prot_is_user(ap,
+                                                mmu_idx != ARMMMUIdx_E2);
+            } else {
+                *prot = simple_ap_to_rw_prot_is_user(ap,
+                                                mmu_idx != ARMMMUIdx_Stage1_E1);
+            }
+
+            if (regime_sctlr(env, mmu_idx) & SCTLR_WXN
+                && (*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(regime_rlar(env,
+                                         mmu_idx, secure)[matchregion], 1, 3);
+            uint64_t mair = env->cp15.mair_el[regime_el(env, mmu_idx)];
+            uint8_t sh = extract32(regime_rlar(env,
+                                   mmu_idx, secure)[matchregion], 3, 2);
+            assert(attrindx <= 4);
+            cacheattrs->is_s2_format = false;
+            cacheattrs->attrs = extract64(mair, attrindx * 8, 8);
+            cacheattrs->shareability = sh;
+        } else {
+            *prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
+        }
+
         if (*prot && !xn && !(pxn && !is_user)) {
             *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;
         }
@@ -2342,9 +2464,16 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
             is_el0 = mmu_idx == ARMMMUIdx_E10_0 || mmu_idx == ARMMMUIdx_SE10_0;
 
             /* S1 is done. Now do S2 translation.  */
-            ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx, is_el0,
-                                     phys_ptr, attrs, &s2_prot,
-                                     page_size, fi, &cacheattrs2);
+            if (arm_feature(env, ARM_FEATURE_V8_R)) {
+                ret = get_phys_addr_pmsav8(env, ipa, access_type, s2_mmu_idx,
+                                       phys_ptr, attrs, &s2_prot, page_size,
+                                       fi, &cacheattrs2);
+            } else {
+                ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx,
+                                      is_el0, phys_ptr, attrs, &s2_prot,
+                                      page_size, fi, &cacheattrs2);
+            }
+
             fi->s2addr = ipa;
             /* Combine the S1 and S2 perms.  */
             *prot &= s2_prot;
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 9/9] target/arm: Add ARM Cortex-R52 cpu
  2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
                   ` (7 preceding siblings ...)
  2022-08-20 14:19 ` [PATCH v3 8/9] target/arm: Add PMSAv8r functionality tobias.roehmel
@ 2022-08-20 14:19 ` tobias.roehmel
  2022-09-27 16:31 ` [PATCH v3 0/9] " Peter Maydell
  9 siblings, 0 replies; 20+ messages in thread
From: tobias.roehmel @ 2022-08-20 14:19 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 b751a19c8a..e0f445dc91 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -843,6 +843,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_V8_R);
+    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;
+}
+
 static void cortex_r5f_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
@@ -1149,6 +1190,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.25.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 4/9] target/arm: Make stage_2_format for cache attributes optional
  2022-08-20 14:19 ` [PATCH v3 4/9] target/arm: Make stage_2_format for cache attributes optional tobias.roehmel
@ 2022-09-27 12:52   ` Peter Maydell
  0 siblings, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2022-09-27 12:52 UTC (permalink / raw)
  To: tobias.roehmel; +Cc: qemu-devel

On Sat, 20 Aug 2022 at 15:19, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> The Cortex-R52 has a 2 stage MPU translation process but doesn't have the
> FEAT_S2FWB feature. This makes it neccessary to allow for the old cache
> attribut combination. This is facilitated by changing the control path
> of combine_cacheattrs instead of failing if the second cache attributes
> struct is not in that format.

I see what you mean but I think I would phrase it differently:

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 4d97a24808..8b037c1f55 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2108,7 +2108,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);
> @@ -2166,6 +2170,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 */
> @@ -2215,7 +2221,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) {

With the commit message tweaks,

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 3/9] target/arm: Make RVBAR available for all ARMv8 CPUs
  2022-08-20 14:19 ` [PATCH v3 3/9] target/arm: Make RVBAR available for all ARMv8 CPUs tobias.roehmel
@ 2022-09-27 13:01   ` Peter Maydell
  0 siblings, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2022-09-27 13:01 UTC (permalink / raw)
  To: tobias.roehmel; +Cc: qemu-devel

On Sat, 20 Aug 2022 at 15:19, <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 to be changed 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 b9547594ae..23461397e0 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7954,13 +7954,20 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          /* RVBAR_EL1 is only implemented if EL1 is the highest EL */
>          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,
> -                .access = PL1_R,
> -                .fieldoffset = offsetof(CPUARMState, cp15.rvbar),
> +            ARMCPRegInfo rvbar[] = {
> +                {
> +                    .name = "RVBAR_EL1", .state = ARM_CP_STATE_AA64,
> +                    .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
> +                    .access = PL1_R,
> +                    .fieldoffset = offsetof(CPUARMState, cp15.rvbar),
> +                },
> +                {   .name = "RVBAR", .type = ARM_CP_ALIAS,
> +                    .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 1,
> +                    .access = PL1_R,
> +                    .fieldoffset = offsetof(CPUARMState, cp15.rvbar),
> +                },

Because the encodings here match, you don't need a separate
struct for the AArch32 RVBAR -- you can just change
the ARM_CP_STATE_AA64 to ARM_CP_STATE_BOTH.

>              };
> -            define_one_arm_cp_reg(cpu, &rvbar);
> +            define_arm_cp_regs(cpu, rvbar);
>          }
>          define_arm_cp_regs(cpu, v8_idregs);
>          define_arm_cp_regs(cpu, v8_cp_reginfo);
> @@ -8022,13 +8029,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, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 1,
> +                    .access = PL2_R,
> +                    .fieldoffset = offsetof(CPUARMState, cp15.rvbar),
> +                },

(Here we do need the second struct, beacuse the opc1 fields don't line up.)

Preferred ordering for the encoding fields in a cp struct is
 .cp .opc1 .crn .crm .opc2
(ie in order as used in the asm instruction).

Otherwise this patch looks good.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 5/9] target/arm: Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup
  2022-08-20 14:19 ` [PATCH v3 5/9] target/arm: Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup tobias.roehmel
@ 2022-09-27 13:05   ` Peter Maydell
  0 siblings, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2022-09-27 13:05 UTC (permalink / raw)
  To: tobias.roehmel; +Cc: qemu-devel

On Sat, 20 Aug 2022 at 15:19, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup to prepare
> for the Cortex-R52 MPU which uses and combines cache attributes
> of different translation levels.
>
> Signed-off-by: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>

> +bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> +                       MMUAccessType access_type, ARMMMUIdx mmu_idx,
> +                       hwaddr *phys_ptr, MemTxAttrs *txattrs,
> +                       int *prot, bool *is_subpage,
> +                       ARMMMUFaultInfo *fi, uint32_t *mregion,
> +                       ARMCacheAttrs *cacheattrs);

When you next rebase this series on current upstream, you'll find
there's been a refactoring of the ptw.c code, so that instead of
passing half a dozen pointers to each function, we pass a
single GetPhysAddrResult* for the function to fill in. That
struct includes an "ARMCacheAttrs cacheattrs" field, so
pmsav8_mpu_lookup() has access to fill it in, and this
patch won't be necessary.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 6/9] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32
  2022-08-20 14:19 ` [PATCH v3 6/9] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32 tobias.roehmel
@ 2022-09-27 13:20   ` Peter Maydell
  0 siblings, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2022-09-27 13:20 UTC (permalink / raw)
  To: tobias.roehmel; +Cc: qemu-devel

On Sat, 20 Aug 2022 at 15:19, <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    | 3 ++-
>  target/arm/tlb_helper.c   | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
> index b18a6bd3a2..44b1e32974 100644
> --- a/target/arm/debug_helper.c
> +++ b/target/arm/debug_helper.c
> @@ -434,7 +434,8 @@ static uint32_t arm_debug_exception_fsr(CPUARMState *env)
>          using_lpae = true;
>      } else {
>          if (arm_feature(env, ARM_FEATURE_LPAE) &&
> -            (env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)) {
> +            ((env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)
> +            || arm_feature(env, ARM_FEATURE_V8_R))) {
>              using_lpae = true;
>          }
>      }
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index b03049d920..e2a2b03d41 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -254,7 +254,8 @@ static inline bool extended_addresses_enabled(CPUARMState *env)
>  {
>      TCR *tcr = &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1];
>      return arm_el_is_aa64(env, 1) ||
> -           (arm_feature(env, ARM_FEATURE_LPAE) && (tcr->raw_tcr & TTBCR_EAE));
> +           (arm_feature(env, ARM_FEATURE_LPAE) && ((tcr->raw_tcr & TTBCR_EAE)
> +           || arm_feature(env, ARM_FEATURE_V8_R)));
>  }
>
>  /* Update a QEMU watchpoint based on the information the guest has set in the
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index 7d8a86b3c4..891326edb8 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -20,7 +20,8 @@ bool regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx)
>          return true;
>      }
>      if (arm_feature(env, ARM_FEATURE_LPAE)
> -        && (regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)) {
> +        && ((regime_tcr(env, mmu_idx)->raw_tcr & TTBCR_EAE)
> +        || arm_feature(env, ARM_FEATURE_V8_R))) {
>          return true;
>      }
>      return false;

In all of these I think you've put the "is this v8R?" condition
in a weird place in the existing conditional structure. v8R
always uses the extended-address format, so we should have
the test at the same kind of level we have the "is this AArch64?"
test, not buried inside the "if LPAE" test.

Also, you can write the check
  (arm_feature(env, ARM_FEATURE_V8) && arm_feature(ARM_FEATURE_PMSA))
-- I still don't think there is any need for a separate V8_R
feature bit.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 7/9] target/arm: Add PMSAv8r registers
  2022-08-20 14:19 ` [PATCH v3 7/9] target/arm: Add PMSAv8r registers tobias.roehmel
@ 2022-09-27 13:50   ` Peter Maydell
  2022-10-15 13:07     ` Tobias Roehmel
  0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2022-09-27 13:50 UTC (permalink / raw)
  To: tobias.roehmel; +Cc: qemu-devel

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 8/9] target/arm: Add PMSAv8r functionality
  2022-08-20 14:19 ` [PATCH v3 8/9] target/arm: Add PMSAv8r functionality tobias.roehmel
@ 2022-09-27 16:23   ` Peter Maydell
  0 siblings, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2022-09-27 16:23 UTC (permalink / raw)
  To: tobias.roehmel; +Cc: qemu-devel

On Sat, 20 Aug 2022 at 15:19, <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 | 171 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 150 insertions(+), 21 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index c4f5721012..c7e37c66d0 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -140,6 +140,9 @@ static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx)
>               */
>              return true;
>          }
> +    } else if (arm_feature(env, ARM_FEATURE_V8_R)) {

In general I think you can replace all these checks on
ARM_FEATURE_V8_R with suitable other tests. I've suggested
various ones in the code below.

> +        return !(regime_sctlr(env, mmu_idx) & SCTLR_M) ||
> +        (!(regime_el(env, mmu_idx) == 2) && arm_hcr_el2_eff(env) & HCR_TGE);
>      }

This doesn't look right. What goes wrong if you just use the v8A code?
In particular v8R still needs to consider HCR.TGE and HCR.DC, same
as v8A, as far as I can see.

>      hcr_el2 = arm_hcr_el2_eff(env);
> @@ -1504,6 +1507,8 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx,
>      if (arm_feature(env, ARM_FEATURE_M)) {
>          return env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)]
>              & R_V7M_MPU_CTRL_PRIVDEFENA_MASK;
> +    } else if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        return false;
>      } else {
>          return regime_sctlr(env, mmu_idx) & SCTLR_BR;
>      }

What's going on here? v8R still has SCTLR.BR to enable the background region.

> @@ -1698,6 +1703,77 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>      return !(*prot & (1 << access_type));
>  }
>
> +static uint32_t *regime_rbar(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                             uint32_t secure)
> +{
> +    if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        if (regime_el(env, mmu_idx) == 2) {
> +            return env->pmsav8.hprbarn;
> +        } else {
> +            return env->pmsav8.prbarn;
> +        }
> +    } else {
> +         return env->pmsav8.rbar[secure];
> +    }

If you set up the CPU state fields correctly (as noted in the
previous patch) then you shouldn't need to test a feature bit
at all here -- M profile and R profile should be able to share
the same pmsav8.rbar[] array for stage 1, and for M-profile
regime_el() will never be 2 so it won't go into those parts
of the function.

> +}
> +
> +static uint32_t *regime_rlar(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                             uint32_t secure)
> +{
> +    if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        if (regime_el(env, mmu_idx) == 2) {
> +            return env->pmsav8.hprlarn;
> +        } else {
> +            return env->pmsav8.prlarn;
> +        }
> +    } else {
> +        return env->pmsav8.rlar[secure];
> +    }
> +}
> +
> +static inline void get_phys_addr_pmsav8_default(CPUARMState *env,
> +                                                ARMMMUIdx mmu_idx,
> +                                                uint32_t address, int *prot)
> +{
> +    if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        *prot = PAGE_READ | PAGE_WRITE;
> +        if (address <= 0x7FFFFFFF) {
> +            *prot |= PAGE_EXEC;
> +        }
> +        if ((regime_el(env, mmu_idx) == 2)
> +            && (regime_sctlr(env, mmu_idx) & SCTLR_WXN)
> +            && (regime_sctlr(env, mmu_idx) & SCTLR_M)) {
> +            *prot &= ~PAGE_EXEC;
> +        }

Put the default handling in get_phys_addr_pmsav7_default(), not
as a wrapper function around it. In particular the stage 1
default is the same as the existing R-profile default map,
so you can just use that code. And PMSAv7 will never have
stage 2, so you can add any code to handle stage 2's default
map without conditionalizing it on "is this v8R?".

Trying to handle SCTLR.WXN and SCTLR.M here looks wrong in
two ways:
(1) the default memory maps are not documented as depending on
either of those two bits
(2) those bits don't affect only stage 2.

> +    } else {
> +        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> +    }
> +}
> +
> +static bool pmsav8_fault(bool hit, CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        if (regime_el(env, mmu_idx) == 2) {
> +            if (!hit && (mmu_idx != ARMMMUIdx_E2)) {
> +                return true;
> +            } else if (!hit && (mmu_idx == ARMMMUIdx_E2)
> +                       &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> +                return true;
> +            }
> +        } else {
> +            if (!hit && (mmu_idx != ARMMMUIdx_Stage1_E1)) {
> +                return true;
> +            } else if (!hit && (mmu_idx == ARMMMUIdx_Stage1_E1)
> +                       &&!(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> +                return true;
> +            }
> +        }
> +        return false;
> +    } else {
> +        return !hit;
> +    }
> +}
> +
>  bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
>                         MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                         hwaddr *phys_ptr, MemTxAttrs *txattrs,
> @@ -1730,6 +1806,12 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
>          *mregion = -1;
>      }
>
> +    if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +        if (mmu_idx == ARMMMUIdx_Stage2) {
> +            fi->stage2 = true;
> +        }

No need to guard this with a V8_R check, because there's no other
way you'll get here with a stage 2 mmu index.

> +    }
> +
>      /*
>       * 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
> @@ -1746,17 +1828,26 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
>              hit = true;
>          }
>
> +        uint32_t bitmask;
> +        if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +            bitmask = 0x3f;
> +        } else {
> +            bitmask = 0x1f;
> +        }

You can avoid a v8R check here by doing an "is this M profile" feature
test instead.

> +
> +
>          for (n = (int)cpu->pmsav7_dregion - 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;
>              }
> @@ -1799,22 +1890,25 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
>          }
>      }
>
> -    if (!hit) {
> -        /* background fault */
> -        fi->type = ARMFault_Background;
> +    if (pmsav8_fault(hit, env, mmu_idx)) {
> +        fi->type = ARMFault_Permission;
> +        fi->level = 0;
>          return true;
>      }

This code change looks like it's going to break the v8M reporting
of background faults.

>
>      if (matchregion == -1) {
>          /* hit using the background region */
> -        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> +        get_phys_addr_pmsav8_default(env, mmu_idx, address, 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 ap = extract32(regime_rbar(env,
> +                                mmu_idx, secure)[matchregion], 1, 2);
> +        uint32_t xn = extract32(regime_rbar(env,
> +                                mmu_idx, secure)[matchregion], 0, 1);
>          bool pxn = false;
>
>          if (arm_feature(env, ARM_FEATURE_V8_1M)) {
> -            pxn = extract32(env->pmsav8.rlar[secure][matchregion], 4, 1);
> +            pxn = extract32(regime_rlar(env,
> +                            mmu_idx, secure)[matchregion], 4, 1);
>          }
>
>          if (m_is_system_region(env, address)) {
> @@ -1822,14 +1916,42 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
>              xn = 1;
>          }
>
> -        *prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
> +        if (arm_feature(env, ARM_FEATURE_V8_R)) {
> +            if (regime_el(env, mmu_idx) == 2) {
> +                *prot = simple_ap_to_rw_prot_is_user(ap,
> +                                                mmu_idx != ARMMMUIdx_E2);
> +            } else {
> +                *prot = simple_ap_to_rw_prot_is_user(ap,
> +                                                mmu_idx != ARMMMUIdx_Stage1_E1);
> +            }
> +
> +            if (regime_sctlr(env, mmu_idx) & SCTLR_WXN
> +                && (*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(regime_rlar(env,
> +                                         mmu_idx, secure)[matchregion], 1, 3);
> +            uint64_t mair = env->cp15.mair_el[regime_el(env, mmu_idx)];
> +            uint8_t sh = extract32(regime_rlar(env,
> +                                   mmu_idx, secure)[matchregion], 3, 2);

The accesses to the rlar/rbar for the matched region have now got cumbersome
enough that we should put them in a local variable at the top of the if().

> +            assert(attrindx <= 4);

What is this assert for? The spec says bits [3:1] can take all of the
8 possible values.

> +            cacheattrs->is_s2_format = false;
> +            cacheattrs->attrs = extract64(mair, attrindx * 8, 8);
> +            cacheattrs->shareability = sh;
> +        } else {
> +            *prot = simple_ap_to_rw_prot(env, mmu_idx, ap);
> +        }
> +
>          if (*prot && !xn && !(pxn && !is_user)) {
>              *prot |= PAGE_EXEC;
>          }
> -        /*
> -         * We don't need to look the attribute up in the MAIR0/MAIR1
> -         * registers because that only tells us about cacheability.
> -         */

Why drop the comment ? It's still relevant for M profile, so should move
into the M-profile arm of the above check.

> +
>          if (mregion) {
>              *mregion = matchregion;
>          }
> @@ -2342,9 +2464,16 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
>              is_el0 = mmu_idx == ARMMMUIdx_E10_0 || mmu_idx == ARMMMUIdx_SE10_0;
>
>              /* S1 is done. Now do S2 translation.  */
> -            ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx, is_el0,
> -                                     phys_ptr, attrs, &s2_prot,
> -                                     page_size, fi, &cacheattrs2);
> +            if (arm_feature(env, ARM_FEATURE_V8_R)) {

"if PMSA" would be a more obvious check to test here.

> +                ret = get_phys_addr_pmsav8(env, ipa, access_type, s2_mmu_idx,
> +                                       phys_ptr, attrs, &s2_prot, page_size,
> +                                       fi, &cacheattrs2);
> +            } else {
> +                ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx,
> +                                      is_el0, phys_ptr, attrs, &s2_prot,
> +                                      page_size, fi, &cacheattrs2);
> +            }
> +
>              fi->s2addr = ipa;
>              /* Combine the S1 and S2 perms.  */
>              *prot &= s2_prot;
> --
> 2.25.1

thanks
-- PMM


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/9] target/arm: Don't add all MIDR aliases for cores that immplement PMSA
  2022-08-20 14:19 ` [PATCH v3 2/9] target/arm: Don't add all MIDR aliases for cores that immplement PMSA tobias.roehmel
@ 2022-09-27 16:29   ` Peter Maydell
  0 siblings, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2022-09-27 16:29 UTC (permalink / raw)
  To: tobias.roehmel; +Cc: qemu-devel

On Sat, 20 Aug 2022 at 15:19, <tobias.roehmel@rwth-aachen.de> wrote:
>
> 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 ++++++++++----

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 0/9] Add ARM Cortex-R52 cpu
  2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
                   ` (8 preceding siblings ...)
  2022-08-20 14:19 ` [PATCH v3 9/9] target/arm: Add ARM Cortex-R52 cpu tobias.roehmel
@ 2022-09-27 16:31 ` Peter Maydell
  9 siblings, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2022-09-27 16:31 UTC (permalink / raw)
  To: tobias.roehmel; +Cc: qemu-devel

On Sat, 20 Aug 2022 at 15:19, <tobias.roehmel@rwth-aachen.de> wrote:
>
> From: Tobias Röhmel <tobias.roehmel@rwth-aachen.de>
>
> Thanks for the review!
> Here is v3:
>
> v3:
> PATCH 2 (Don't add all MIDR aliases for cores that immplement PMSA):
> fixed the comment and changed to single element instead of array.
> Also the alias is not added for all PMSA CPUs as Peter suggested.
> PATCH 3 (Make RVBAR available for all ARMv8 CPUs):
> Added the missing RVBAR register as alias to RVBAR_ELx.
> Tested that this code actually takes the lower 32 bits
> of RVBAR_ELx as RVBAR.

Hi -- sorry it's taken me so long to get to this, but I had
a combination of holiday and KVM Forum :-(  Anyway, I've now
completed my review pass for v3.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 7/9] target/arm: Add PMSAv8r registers
  2022-09-27 13:50   ` Peter Maydell
@ 2022-10-15 13:07     ` Tobias Roehmel
  2022-10-20 15:02       ` Peter Maydell
  0 siblings, 1 reply; 20+ messages in thread
From: Tobias Roehmel @ 2022-10-15 13:07 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 7/9] target/arm: Add PMSAv8r registers
  2022-10-15 13:07     ` Tobias Roehmel
@ 2022-10-20 15:02       ` Peter Maydell
  0 siblings, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2022-10-20 15:02 UTC (permalink / raw)
  To: Tobias Roehmel; +Cc: qemu-devel

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2022-10-20 15:48 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-20 14:19 [PATCH v3 0/9] Add ARM Cortex-R52 cpu tobias.roehmel
2022-08-20 14:19 ` [PATCH v3 1/9] target/arm: Add ARM_FEATURE_V8_R tobias.roehmel
2022-08-20 14:19 ` [PATCH v3 2/9] target/arm: Don't add all MIDR aliases for cores that immplement PMSA tobias.roehmel
2022-09-27 16:29   ` Peter Maydell
2022-08-20 14:19 ` [PATCH v3 3/9] target/arm: Make RVBAR available for all ARMv8 CPUs tobias.roehmel
2022-09-27 13:01   ` Peter Maydell
2022-08-20 14:19 ` [PATCH v3 4/9] target/arm: Make stage_2_format for cache attributes optional tobias.roehmel
2022-09-27 12:52   ` Peter Maydell
2022-08-20 14:19 ` [PATCH v3 5/9] target/arm: Add ARMCacheAttrs to the signature of pmsav8_mpu_lookup tobias.roehmel
2022-09-27 13:05   ` Peter Maydell
2022-08-20 14:19 ` [PATCH v3 6/9] target/arm: Enable TTBCR_EAE for ARMv8-R AArch32 tobias.roehmel
2022-09-27 13:20   ` Peter Maydell
2022-08-20 14:19 ` [PATCH v3 7/9] target/arm: Add PMSAv8r registers tobias.roehmel
2022-09-27 13:50   ` Peter Maydell
2022-10-15 13:07     ` Tobias Roehmel
2022-10-20 15:02       ` Peter Maydell
2022-08-20 14:19 ` [PATCH v3 8/9] target/arm: Add PMSAv8r functionality tobias.roehmel
2022-09-27 16:23   ` Peter Maydell
2022-08-20 14:19 ` [PATCH v3 9/9] target/arm: Add ARM Cortex-R52 cpu tobias.roehmel
2022-09-27 16:31 ` [PATCH v3 0/9] " Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.