[PATCH 0/2] target/xtensa: tidy xtensa memory management

[PATCH 0/2] target/xtensa: tidy xtensa memory management

[Max Filippov]

Hello,

this series separates xtensa MMU and MPU states and improves variable
TLB way logic.

Max Filippov (2):
  target/xtensa: wrap MMU and MPU state into structures
  target/xtensa: tidy TLB way variability logic

 target/xtensa/cpu.h          | 21 +++++++---
 target/xtensa/mmu_helper.c   | 74 ++++++++++++++----------------------
 target/xtensa/overlay_tool.h | 15 +++++++-
 3 files changed, 56 insertions(+), 54 deletions(-)

-- 
2.39.2

[PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures

[Max Filippov]

Make separation of alternative xtensa memory management options state
explicit.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 target/xtensa/cpu.h        | 18 +++++++++++++----
 target/xtensa/mmu_helper.c | 40 +++++++++++++++++++-------------------
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index 8a423706d8c0..497325466397 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -326,11 +326,21 @@ typedef struct xtensa_tlb {
     unsigned nrefillentries;
 } xtensa_tlb;
 
+typedef struct XtensaMMU {
+    xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE];
+    xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE];
+    unsigned autorefill_idx;
+} XtensaMMU;
+
 typedef struct xtensa_mpu_entry {
     uint32_t vaddr;
     uint32_t attr;
 } xtensa_mpu_entry;
 
+typedef struct XtensaMPU {
+    xtensa_mpu_entry fg[MAX_MPU_FOREGROUND_SEGMENTS];
+} XtensaMPU;
+
 typedef struct XtensaGdbReg {
     int targno;
     unsigned flags;
@@ -526,10 +536,10 @@ struct CPUArchState {
     uint32_t exclusive_val;
 
 #ifndef CONFIG_USER_ONLY
-    xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE];
-    xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE];
-    xtensa_mpu_entry mpu_fg[MAX_MPU_FOREGROUND_SEGMENTS];
-    unsigned autorefill_idx;
+    union {
+        XtensaMMU mmu;
+        XtensaMPU mpu;
+    };
     bool runstall;
     AddressSpace *address_space_er;
     MemoryRegion *system_er;
diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c
index 2fda4e887cce..d9f845e7fb6f 100644
--- a/target/xtensa/mmu_helper.c
+++ b/target/xtensa/mmu_helper.c
@@ -250,8 +250,8 @@ static xtensa_tlb_entry *xtensa_tlb_get_entry(CPUXtensaState *env, bool dtlb,
 
     assert(wi < tlb->nways && ei < tlb->way_size[wi]);
     return dtlb ?
-        env->dtlb[wi] + ei :
-        env->itlb[wi] + ei;
+        env->mmu.dtlb[wi] + ei :
+        env->mmu.itlb[wi] + ei;
 }
 
 static xtensa_tlb_entry *get_tlb_entry(CPUXtensaState *env,
@@ -411,11 +411,11 @@ void reset_mmu(CPUXtensaState *env)
         env->sregs[RASID] = 0x04030201;
         env->sregs[ITLBCFG] = 0;
         env->sregs[DTLBCFG] = 0;
-        env->autorefill_idx = 0;
-        reset_tlb_mmu_all_ways(env, &env->config->itlb, env->itlb);
-        reset_tlb_mmu_all_ways(env, &env->config->dtlb, env->dtlb);
-        reset_tlb_mmu_ways56(env, &env->config->itlb, env->itlb);
-        reset_tlb_mmu_ways56(env, &env->config->dtlb, env->dtlb);
+        env->mmu.autorefill_idx = 0;
+        reset_tlb_mmu_all_ways(env, &env->config->itlb, env->mmu.itlb);
+        reset_tlb_mmu_all_ways(env, &env->config->dtlb, env->mmu.dtlb);
+        reset_tlb_mmu_ways56(env, &env->config->itlb, env->mmu.itlb);
+        reset_tlb_mmu_ways56(env, &env->config->dtlb, env->mmu.dtlb);
     } else if (xtensa_option_enabled(env->config, XTENSA_OPTION_MPU)) {
         unsigned i;
 
@@ -430,8 +430,8 @@ void reset_mmu(CPUXtensaState *env)
         }
     } else {
         env->sregs[CACHEATTR] = 0x22222222;
-        reset_tlb_region_way0(env, env->itlb);
-        reset_tlb_region_way0(env, env->dtlb);
+        reset_tlb_region_way0(env, env->mmu.itlb);
+        reset_tlb_region_way0(env, env->mmu.dtlb);
     }
 }
 
@@ -462,7 +462,7 @@ static int xtensa_tlb_lookup(const CPUXtensaState *env,
     const xtensa_tlb *tlb = dtlb ?
         &env->config->dtlb : &env->config->itlb;
     const xtensa_tlb_entry (*entry)[MAX_TLB_WAY_SIZE] = dtlb ?
-        env->dtlb : env->itlb;
+        env->mmu.dtlb : env->mmu.itlb;
 
     int nhits = 0;
     unsigned wi;
@@ -821,7 +821,7 @@ static int get_physical_addr_mmu(CPUXtensaState *env, bool update_tlb,
         split_tlb_entry_spec_way(env, vaddr, dtlb, &vpn, wi, &ei);
 
         if (update_tlb) {
-            wi = ++env->autorefill_idx & 0x3;
+            wi = ++env->mmu.autorefill_idx & 0x3;
             xtensa_tlb_set_entry(env, dtlb, wi, ei, vpn, pte);
             env->sregs[EXCVADDR] = vaddr;
             qemu_log_mask(CPU_LOG_MMU, "%s: autorefill(%08x): %08x -> %08x\n",
@@ -957,8 +957,8 @@ void HELPER(wptlb)(CPUXtensaState *env, uint32_t p, uint32_t v)
     unsigned segment = p & XTENSA_MPU_SEGMENT_MASK;
 
     if (segment < env->config->n_mpu_fg_segments) {
-        env->mpu_fg[segment].vaddr = v & -env->config->mpu_align;
-        env->mpu_fg[segment].attr = p & XTENSA_MPU_ATTR_MASK;
+        env->mpu.fg[segment].vaddr = v & -env->config->mpu_align;
+        env->mpu.fg[segment].attr = p & XTENSA_MPU_ATTR_MASK;
         env->sregs[MPUENB] = deposit32(env->sregs[MPUENB], segment, 1, v);
         tlb_flush(env_cpu(env));
     }
@@ -969,7 +969,7 @@ uint32_t HELPER(rptlb0)(CPUXtensaState *env, uint32_t s)
     unsigned segment = s & XTENSA_MPU_SEGMENT_MASK;
 
     if (segment < env->config->n_mpu_fg_segments) {
-        return env->mpu_fg[segment].vaddr |
+        return env->mpu.fg[segment].vaddr |
             extract32(env->sregs[MPUENB], segment, 1);
     } else {
         return 0;
@@ -981,7 +981,7 @@ uint32_t HELPER(rptlb1)(CPUXtensaState *env, uint32_t s)
     unsigned segment = s & XTENSA_MPU_SEGMENT_MASK;
 
     if (segment < env->config->n_mpu_fg_segments) {
-        return env->mpu_fg[segment].attr;
+        return env->mpu.fg[segment].attr;
     } else {
         return 0;
     }
@@ -993,13 +993,13 @@ uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v)
     unsigned segment = XTENSA_MPU_PROBE_B;
     unsigned bg_segment;
 
-    nhits = xtensa_mpu_lookup(env->mpu_fg, env->config->n_mpu_fg_segments,
+    nhits = xtensa_mpu_lookup(env->mpu.fg, env->config->n_mpu_fg_segments,
                               v, &segment);
     if (nhits > 1) {
         HELPER(exception_cause_vaddr)(env, env->pc,
                                       LOAD_STORE_TLB_MULTI_HIT_CAUSE, v);
     } else if (nhits == 1 && (env->sregs[MPUENB] & (1u << segment))) {
-        return env->mpu_fg[segment].attr | segment | XTENSA_MPU_PROBE_V;
+        return env->mpu.fg[segment].attr | segment | XTENSA_MPU_PROBE_V;
     } else {
         xtensa_mpu_lookup(env->config->mpu_bg,
                           env->config->n_mpu_bg_segments,
@@ -1017,14 +1017,14 @@ static int get_physical_addr_mpu(CPUXtensaState *env,
     unsigned segment;
     uint32_t attr;
 
-    nhits = xtensa_mpu_lookup(env->mpu_fg, env->config->n_mpu_fg_segments,
+    nhits = xtensa_mpu_lookup(env->mpu.fg, env->config->n_mpu_fg_segments,
                               vaddr, &segment);
     if (nhits > 1) {
         return is_write < 2 ?
             LOAD_STORE_TLB_MULTI_HIT_CAUSE :
             INST_TLB_MULTI_HIT_CAUSE;
     } else if (nhits == 1 && (env->sregs[MPUENB] & (1u << segment))) {
-        attr = env->mpu_fg[segment].attr;
+        attr = env->mpu.fg[segment].attr;
     } else {
         xtensa_mpu_lookup(env->config->mpu_bg,
                           env->config->n_mpu_bg_segments,
@@ -1205,7 +1205,7 @@ void dump_mmu(CPUXtensaState *env)
         dump_tlb(env, true);
     } else if (xtensa_option_enabled(env->config, XTENSA_OPTION_MPU)) {
         qemu_printf("Foreground map:\n");
-        dump_mpu(env, env->mpu_fg, env->config->n_mpu_fg_segments);
+        dump_mpu(env, env->mpu.fg, env->config->n_mpu_fg_segments);
         qemu_printf("\nBackground map:\n");
         dump_mpu(NULL, env->config->mpu_bg, env->config->n_mpu_bg_segments);
     } else {
-- 
2.39.2

[PATCH 2/2] target/xtensa: tidy TLB way variability logic

[Max Filippov]

Whether TLB ways 5 and 6 are variable is not a property of the TLB
instance or a TLB entry instance, it's a property of the xtensa core
configuration.
Remove 'varway56' field from the xtensa_tlb structure and remove
'variable' field from the xtensa_tlb_entry structure. Add
'tlb_variable_way' array to the XtensaConfig and use it instead of
removed fields.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 target/xtensa/cpu.h          |  3 +--
 target/xtensa/mmu_helper.c   | 38 ++++++++++--------------------------
 target/xtensa/overlay_tool.h | 15 ++++++++++++--
 3 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index 497325466397..24d3f15ea1bf 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -316,13 +316,11 @@ typedef struct xtensa_tlb_entry {
     uint32_t paddr;
     uint8_t asid;
     uint8_t attr;
-    bool variable;
 } xtensa_tlb_entry;
 
 typedef struct xtensa_tlb {
     unsigned nways;
     const unsigned way_size[10];
-    bool varway56;
     unsigned nrefillentries;
 } xtensa_tlb;
 
@@ -493,6 +491,7 @@ typedef struct XtensaConfig {
 
     xtensa_tlb itlb;
     xtensa_tlb dtlb;
+    bool tlb_variable_way[16];
 
     uint32_t mpu_align;
     unsigned n_mpu_fg_segments;
diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c
index d9f845e7fb6f..414c2f5ef669 100644
--- a/target/xtensa/mmu_helper.c
+++ b/target/xtensa/mmu_helper.c
@@ -105,23 +105,19 @@ static uint32_t xtensa_tlb_get_addr_mask(const CPUXtensaState *env,
                                          bool dtlb, uint32_t way)
 {
     if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) {
-        bool varway56 = dtlb ?
-            env->config->dtlb.varway56 :
-            env->config->itlb.varway56;
-
         switch (way) {
         case 4:
             return 0xfff00000 << get_page_size(env, dtlb, way) * 2;
 
         case 5:
-            if (varway56) {
+            if (env->config->tlb_variable_way[5]) {
                 return 0xf8000000 << get_page_size(env, dtlb, way);
             } else {
                 return 0xf8000000;
             }
 
         case 6:
-            if (varway56) {
+            if (env->config->tlb_variable_way[6]) {
                 return 0xf0000000 << (1 - get_page_size(env, dtlb, way));
             } else {
                 return 0xf0000000;
@@ -150,11 +146,8 @@ static uint32_t get_vpn_mask(const CPUXtensaState *env, bool dtlb, uint32_t way)
         return xtensa_tlb_get_addr_mask(env, dtlb, way) << 2;
     } else if (way <= 6) {
         uint32_t mask = xtensa_tlb_get_addr_mask(env, dtlb, way);
-        bool varway56 = dtlb ?
-            env->config->dtlb.varway56 :
-            env->config->itlb.varway56;
 
-        if (varway56) {
+        if (env->config->tlb_variable_way[5]) {
             return mask << (way == 5 ? 2 : 3);
         } else {
             return mask << 1;
@@ -172,10 +165,6 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v,
                                      bool dtlb, uint32_t *vpn,
                                      uint32_t wi, uint32_t *ei)
 {
-    bool varway56 = dtlb ?
-        env->config->dtlb.varway56 :
-        env->config->itlb.varway56;
-
     if (!dtlb) {
         wi &= 7;
     }
@@ -195,7 +184,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v,
             break;
 
         case 5:
-            if (varway56) {
+            if (env->config->tlb_variable_way[5]) {
                 uint32_t eibase = 27 + get_page_size(env, dtlb, wi);
                 *ei = (v >> eibase) & 0x3;
             } else {
@@ -204,7 +193,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v,
             break;
 
         case 6:
-            if (varway56) {
+            if (env->config->tlb_variable_way[6]) {
                 uint32_t eibase = 29 - get_page_size(env, dtlb, wi);
                 *ei = (v >> eibase) & 0x7;
             } else {
@@ -290,7 +279,7 @@ static void xtensa_tlb_set_entry(CPUXtensaState *env, bool dtlb,
     xtensa_tlb_entry *entry = xtensa_tlb_get_entry(env, dtlb, wi, ei);
 
     if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) {
-        if (entry->variable) {
+        if (env->config->tlb_variable_way[wi]) {
             if (entry->asid) {
                 tlb_flush_page(cs, entry->vaddr);
             }
@@ -338,29 +327,25 @@ static void reset_tlb_mmu_all_ways(CPUXtensaState *env,
     for (wi = 0; wi < tlb->nways; ++wi) {
         for (ei = 0; ei < tlb->way_size[wi]; ++ei) {
             entry[wi][ei].asid = 0;
-            entry[wi][ei].variable = true;
         }
     }
 }
 
 static void reset_tlb_mmu_ways56(CPUXtensaState *env,
-                                 const xtensa_tlb *tlb,
                                  xtensa_tlb_entry entry[][MAX_TLB_WAY_SIZE])
 {
-    if (!tlb->varway56) {
+    if (!env->config->tlb_variable_way[5]) {
         static const xtensa_tlb_entry way5[] = {
             {
                 .vaddr = 0xd0000000,
                 .paddr = 0,
                 .asid = 1,
                 .attr = 7,
-                .variable = false,
             }, {
                 .vaddr = 0xd8000000,
                 .paddr = 0,
                 .asid = 1,
                 .attr = 3,
-                .variable = false,
             }
         };
         static const xtensa_tlb_entry way6[] = {
@@ -369,13 +354,11 @@ static void reset_tlb_mmu_ways56(CPUXtensaState *env,
                 .paddr = 0xf0000000,
                 .asid = 1,
                 .attr = 7,
-                .variable = false,
             }, {
                 .vaddr = 0xf0000000,
                 .paddr = 0xf0000000,
                 .asid = 1,
                 .attr = 3,
-                .variable = false,
             }
         };
         memcpy(entry[5], way5, sizeof(way5));
@@ -401,7 +384,6 @@ static void reset_tlb_region_way0(CPUXtensaState *env,
         entry[0][ei].paddr = ei << 29;
         entry[0][ei].asid = 1;
         entry[0][ei].attr = 2;
-        entry[0][ei].variable = true;
     }
 }
 
@@ -414,8 +396,8 @@ void reset_mmu(CPUXtensaState *env)
         env->mmu.autorefill_idx = 0;
         reset_tlb_mmu_all_ways(env, &env->config->itlb, env->mmu.itlb);
         reset_tlb_mmu_all_ways(env, &env->config->dtlb, env->mmu.dtlb);
-        reset_tlb_mmu_ways56(env, &env->config->itlb, env->mmu.itlb);
-        reset_tlb_mmu_ways56(env, &env->config->dtlb, env->mmu.dtlb);
+        reset_tlb_mmu_ways56(env, env->mmu.itlb);
+        reset_tlb_mmu_ways56(env, env->mmu.dtlb);
     } else if (xtensa_option_enabled(env->config, XTENSA_OPTION_MPU)) {
         unsigned i;
 
@@ -521,7 +503,7 @@ void HELPER(itlb)(CPUXtensaState *env, uint32_t v, uint32_t dtlb)
     if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) {
         uint32_t wi;
         xtensa_tlb_entry *entry = get_tlb_entry(env, v, dtlb, &wi);
-        if (entry && entry->variable && entry->asid) {
+        if (entry && env->config->tlb_variable_way[wi] && entry->asid) {
             tlb_flush_page(env_cpu(env), entry->vaddr);
             entry->asid = 0;
         }
diff --git a/target/xtensa/overlay_tool.h b/target/xtensa/overlay_tool.h
index 701c00eed20a..268a7fe1823f 100644
--- a/target/xtensa/overlay_tool.h
+++ b/target/xtensa/overlay_tool.h
@@ -351,7 +351,6 @@
             (refill_way_size), (refill_way_size), \
             4, (way56) ? 4 : 2, (way56) ? 8 : 2, 1, 1, 1, \
         }, \
-        .varway56 = (way56), \
         .nrefillentries = (refill_way_size) * 4, \
     }
 
@@ -363,7 +362,19 @@
 
 #define TLB_SECTION \
     .itlb = ITLB(XCHAL_HAVE_SPANNING_WAY), \
-    .dtlb = DTLB(XCHAL_HAVE_SPANNING_WAY)
+    .dtlb = DTLB(XCHAL_HAVE_SPANNING_WAY), \
+    .tlb_variable_way = { \
+        true, \
+        XCHAL_HAVE_PTP_MMU, \
+        XCHAL_HAVE_PTP_MMU, \
+        XCHAL_HAVE_PTP_MMU, \
+        XCHAL_HAVE_PTP_MMU, \
+        XCHAL_HAVE_PTP_MMU && XCHAL_HAVE_SPANNING_WAY, \
+        XCHAL_HAVE_PTP_MMU && XCHAL_HAVE_SPANNING_WAY, \
+        XCHAL_HAVE_PTP_MMU, \
+        XCHAL_HAVE_PTP_MMU, \
+        XCHAL_HAVE_PTP_MMU, \
+    }
 
 #ifndef XCHAL_SYSROM0_PADDR
 #define XCHAL_SYSROM0_PADDR 0xfe000000
-- 
2.39.2

Re: [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures

[Peter Maydell]

On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvbkbc@gmail.com> wrote:
>
> Make separation of alternative xtensa memory management options state
> explicit.
>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  target/xtensa/cpu.h        | 18 +++++++++++++----
>  target/xtensa/mmu_helper.c | 40 +++++++++++++++++++-------------------
>  2 files changed, 34 insertions(+), 24 deletions(-)
>
> diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
> index 8a423706d8c0..497325466397 100644
> --- a/target/xtensa/cpu.h
> +++ b/target/xtensa/cpu.h
> @@ -326,11 +326,21 @@ typedef struct xtensa_tlb {
>      unsigned nrefillentries;
>  } xtensa_tlb;
>
> +typedef struct XtensaMMU {
> +    xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE];
> +    xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE];
> +    unsigned autorefill_idx;
> +} XtensaMMU;
> +
>  typedef struct xtensa_mpu_entry {
>      uint32_t vaddr;
>      uint32_t attr;
>  } xtensa_mpu_entry;
>
> +typedef struct XtensaMPU {
> +    xtensa_mpu_entry fg[MAX_MPU_FOREGROUND_SEGMENTS];
> +} XtensaMPU;
> +
>  typedef struct XtensaGdbReg {
>      int targno;
>      unsigned flags;
> @@ -526,10 +536,10 @@ struct CPUArchState {
>      uint32_t exclusive_val;
>
>  #ifndef CONFIG_USER_ONLY
> -    xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE];
> -    xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE];
> -    xtensa_mpu_entry mpu_fg[MAX_MPU_FOREGROUND_SEGMENTS];
> -    unsigned autorefill_idx;
> +    union {
> +        XtensaMMU mmu;
> +        XtensaMPU mpu;
> +    };

Is it really worth having this be a union ? I suspect it will
make adding migration/savevm support later more awkward.

>      bool runstall;
>      AddressSpace *address_space_er;
>      MemoryRegion *system_er;

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

thanks
-- PMM

Re: [PATCH 2/2] target/xtensa: tidy TLB way variability logic

[Peter Maydell]

On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvbkbc@gmail.com> wrote:
>
> Whether TLB ways 5 and 6 are variable is not a property of the TLB
> instance or a TLB entry instance, it's a property of the xtensa core
> configuration.
> Remove 'varway56' field from the xtensa_tlb structure and remove
> 'variable' field from the xtensa_tlb_entry structure. Add
> 'tlb_variable_way' array to the XtensaConfig and use it instead of
> removed fields.
>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  target/xtensa/cpu.h          |  3 +--
>  target/xtensa/mmu_helper.c   | 38 ++++++++++--------------------------
>  target/xtensa/overlay_tool.h | 15 ++++++++++++--
>  3 files changed, 24 insertions(+), 32 deletions(-)
>
> diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
> index 497325466397..24d3f15ea1bf 100644
> --- a/target/xtensa/cpu.h
> +++ b/target/xtensa/cpu.h
> @@ -316,13 +316,11 @@ typedef struct xtensa_tlb_entry {
>      uint32_t paddr;
>      uint8_t asid;
>      uint8_t attr;
> -    bool variable;
>  } xtensa_tlb_entry;
>
>  typedef struct xtensa_tlb {
>      unsigned nways;
>      const unsigned way_size[10];
> -    bool varway56;
>      unsigned nrefillentries;
>  } xtensa_tlb;
>
> @@ -493,6 +491,7 @@ typedef struct XtensaConfig {
>
>      xtensa_tlb itlb;
>      xtensa_tlb dtlb;
> +    bool tlb_variable_way[16];
>
>      uint32_t mpu_align;
>      unsigned n_mpu_fg_segments;
> diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c
> index d9f845e7fb6f..414c2f5ef669 100644
> --- a/target/xtensa/mmu_helper.c
> +++ b/target/xtensa/mmu_helper.c
> @@ -105,23 +105,19 @@ static uint32_t xtensa_tlb_get_addr_mask(const CPUXtensaState *env,
>                                           bool dtlb, uint32_t way)
>  {
>      if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) {
> -        bool varway56 = dtlb ?
> -            env->config->dtlb.varway56 :
> -            env->config->itlb.varway56;
> -
>          switch (way) {
>          case 4:
>              return 0xfff00000 << get_page_size(env, dtlb, way) * 2;
>
>          case 5:
> -            if (varway56) {
> +            if (env->config->tlb_variable_way[5]) {
>                  return 0xf8000000 << get_page_size(env, dtlb, way);
>              } else {
>                  return 0xf8000000;
>              }
>
>          case 6:
> -            if (varway56) {
> +            if (env->config->tlb_variable_way[6]) {
>                  return 0xf0000000 << (1 - get_page_size(env, dtlb, way));
>              } else {
>                  return 0xf0000000;

So we now have a tlb_variable_way bool for all 16 possible
ways, but the code actually only checks it for ways 5 and 6.
Should we have an assertion somewhere that the config
doesn't try to set it on ways where it has no effect ?
Or is there actually a generic behaviour that would make
sense for eg "way 3 is variable-way" that we just don't
currently implement?

> @@ -150,11 +146,8 @@ static uint32_t get_vpn_mask(const CPUXtensaState *env, bool dtlb, uint32_t way)
>          return xtensa_tlb_get_addr_mask(env, dtlb, way) << 2;
>      } else if (way <= 6) {
>          uint32_t mask = xtensa_tlb_get_addr_mask(env, dtlb, way);
> -        bool varway56 = dtlb ?
> -            env->config->dtlb.varway56 :
> -            env->config->itlb.varway56;
>
> -        if (varway56) {
> +        if (env->config->tlb_variable_way[5]) {
>              return mask << (way == 5 ? 2 : 3);
>          } else {
>              return mask << 1;

This doesn't look right -- this branch of the if-else deals
with way == 5 and way == 6, but we're only looking at
tlb_variable_way[5].

> @@ -172,10 +165,6 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v,
>                                       bool dtlb, uint32_t *vpn,
>                                       uint32_t wi, uint32_t *ei)
>  {
> -    bool varway56 = dtlb ?
> -        env->config->dtlb.varway56 :
> -        env->config->itlb.varway56;
> -
>      if (!dtlb) {
>          wi &= 7;
>      }
> @@ -195,7 +184,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v,
>              break;
>
>          case 5:
> -            if (varway56) {
> +            if (env->config->tlb_variable_way[5]) {
>                  uint32_t eibase = 27 + get_page_size(env, dtlb, wi);
>                  *ei = (v >> eibase) & 0x3;
>              } else {
> @@ -204,7 +193,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v,
>              break;
>
>          case 6:
> -            if (varway56) {
> +            if (env->config->tlb_variable_way[6]) {
>                  uint32_t eibase = 29 - get_page_size(env, dtlb, wi);
>                  *ei = (v >> eibase) & 0x7;
>              } else {

There's no direct code duplication, but it definitely feels like
the logic for "figure out how many bits we're dealing with" is
duplicated across these three functions.

I think it ought to be possible to have a function (or maybe two)
which take account of both the way number and tlb_get_variable_way[]
such that all of these three functions then don't need to have
a switch on the way or look at tlb_variable_way[] themselves...

thanks
-- PMM

Re: [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures

[Max Filippov]

On Mon, Jan 22, 2024 at 10:29 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvbkbc@gmail.com> wrote:
> >
> > Make separation of alternative xtensa memory management options state
> > explicit.
> >
> > Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> > ---
> >  target/xtensa/cpu.h        | 18 +++++++++++++----
> >  target/xtensa/mmu_helper.c | 40 +++++++++++++++++++-------------------
> >  2 files changed, 34 insertions(+), 24 deletions(-)
> >
> > diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
> > index 8a423706d8c0..497325466397 100644
> > --- a/target/xtensa/cpu.h
> > +++ b/target/xtensa/cpu.h
> > @@ -326,11 +326,21 @@ typedef struct xtensa_tlb {
> >      unsigned nrefillentries;
> >  } xtensa_tlb;
> >
> > +typedef struct XtensaMMU {
> > +    xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE];
> > +    xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE];
> > +    unsigned autorefill_idx;
> > +} XtensaMMU;
> > +
> >  typedef struct xtensa_mpu_entry {
> >      uint32_t vaddr;
> >      uint32_t attr;
> >  } xtensa_mpu_entry;
> >
> > +typedef struct XtensaMPU {
> > +    xtensa_mpu_entry fg[MAX_MPU_FOREGROUND_SEGMENTS];
> > +} XtensaMPU;
> > +
> >  typedef struct XtensaGdbReg {
> >      int targno;
> >      unsigned flags;
> > @@ -526,10 +536,10 @@ struct CPUArchState {
> >      uint32_t exclusive_val;
> >
> >  #ifndef CONFIG_USER_ONLY
> > -    xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE];
> > -    xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE];
> > -    xtensa_mpu_entry mpu_fg[MAX_MPU_FOREGROUND_SEGMENTS];
> > -    unsigned autorefill_idx;
> > +    union {
> > +        XtensaMMU mmu;
> > +        XtensaMPU mpu;
> > +    };
>
> Is it really worth having this be a union ? I suspect it will
> make adding migration/savevm support later more awkward.

I have a draft implementation of savevm for xtensa and I did this part
using subsections with the .needed callback checking whether the
MMU or MPU option is enabled in the config. I wonder where the
awkwardness is expected.

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

Thanks!

-- Max

Re: [PATCH 2/2] target/xtensa: tidy TLB way variability logic

[Max Filippov]

On Mon, Jan 22, 2024 at 10:42 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvbkbc@gmail.com> wrote:
> >
> > Whether TLB ways 5 and 6 are variable is not a property of the TLB
> > instance or a TLB entry instance, it's a property of the xtensa core
> > configuration.
> > Remove 'varway56' field from the xtensa_tlb structure and remove
> > 'variable' field from the xtensa_tlb_entry structure. Add
> > 'tlb_variable_way' array to the XtensaConfig and use it instead of
> > removed fields.
> >
> > Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> > ---
> >  target/xtensa/cpu.h          |  3 +--
> >  target/xtensa/mmu_helper.c   | 38 ++++++++++--------------------------
> >  target/xtensa/overlay_tool.h | 15 ++++++++++++--
> >  3 files changed, 24 insertions(+), 32 deletions(-)
> >
> > diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
> > index 497325466397..24d3f15ea1bf 100644
> > --- a/target/xtensa/cpu.h
> > +++ b/target/xtensa/cpu.h
> > @@ -316,13 +316,11 @@ typedef struct xtensa_tlb_entry {
> >      uint32_t paddr;
> >      uint8_t asid;
> >      uint8_t attr;
> > -    bool variable;
> >  } xtensa_tlb_entry;
> >
> >  typedef struct xtensa_tlb {
> >      unsigned nways;
> >      const unsigned way_size[10];
> > -    bool varway56;
> >      unsigned nrefillentries;
> >  } xtensa_tlb;
> >
> > @@ -493,6 +491,7 @@ typedef struct XtensaConfig {
> >
> >      xtensa_tlb itlb;
> >      xtensa_tlb dtlb;
> > +    bool tlb_variable_way[16];
> >
> >      uint32_t mpu_align;
> >      unsigned n_mpu_fg_segments;
> > diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c
> > index d9f845e7fb6f..414c2f5ef669 100644
> > --- a/target/xtensa/mmu_helper.c
> > +++ b/target/xtensa/mmu_helper.c
> > @@ -105,23 +105,19 @@ static uint32_t xtensa_tlb_get_addr_mask(const CPUXtensaState *env,
> >                                           bool dtlb, uint32_t way)
> >  {
> >      if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) {
> > -        bool varway56 = dtlb ?
> > -            env->config->dtlb.varway56 :
> > -            env->config->itlb.varway56;
> > -
> >          switch (way) {
> >          case 4:
> >              return 0xfff00000 << get_page_size(env, dtlb, way) * 2;
> >
> >          case 5:
> > -            if (varway56) {
> > +            if (env->config->tlb_variable_way[5]) {
> >                  return 0xf8000000 << get_page_size(env, dtlb, way);
> >              } else {
> >                  return 0xf8000000;
> >              }
> >
> >          case 6:
> > -            if (varway56) {
> > +            if (env->config->tlb_variable_way[6]) {
> >                  return 0xf0000000 << (1 - get_page_size(env, dtlb, way));
> >              } else {
> >                  return 0xf0000000;
>
> So we now have a tlb_variable_way bool for all 16 possible
> ways, but the code actually only checks it for ways 5 and 6.

xtensa_tlb_set_entry checks this for all possible ways.

I would say that this is an unfortunate definition of MMU in the
xtensa ISA book that uses the variability of the ways 5/6 as a
discriminator between MMUv2 and MMUv3.

> Should we have an assertion somewhere that the config
> doesn't try to set it on ways where it has no effect ?
> Or is there actually a generic behaviour that would make
> sense for eg "way 3 is variable-way" that we just don't
> currently implement?

We currently use the TLB structure to implement the following
xtensa memory management options: cacheattr, region protection,
region translation, MMUv2 and MMUv3. First three only have
one variable way, in MMUv2 all ways except 5 and 6 are variable
and in MMUv3 all ways are variable. QEMU supports all of it
and tlb_variable_way is set properly in all of these cases.

> > @@ -150,11 +146,8 @@ static uint32_t get_vpn_mask(const CPUXtensaState *env, bool dtlb, uint32_t way)
> >          return xtensa_tlb_get_addr_mask(env, dtlb, way) << 2;
> >      } else if (way <= 6) {
> >          uint32_t mask = xtensa_tlb_get_addr_mask(env, dtlb, way);
> > -        bool varway56 = dtlb ?
> > -            env->config->dtlb.varway56 :
> > -            env->config->itlb.varway56;
> >
> > -        if (varway56) {
> > +        if (env->config->tlb_variable_way[5]) {
> >              return mask << (way == 5 ? 2 : 3);
> >          } else {
> >              return mask << 1;
>
> This doesn't look right -- this branch of the if-else deals
> with way == 5 and way == 6, but we're only looking at
> tlb_variable_way[5].

Yeah, that's MMUv2 vs MMUv3 check, again.

> > @@ -172,10 +165,6 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v,
> >                                       bool dtlb, uint32_t *vpn,
> >                                       uint32_t wi, uint32_t *ei)
> >  {
> > -    bool varway56 = dtlb ?
> > -        env->config->dtlb.varway56 :
> > -        env->config->itlb.varway56;
> > -
> >      if (!dtlb) {
> >          wi &= 7;
> >      }
> > @@ -195,7 +184,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v,
> >              break;
> >
> >          case 5:
> > -            if (varway56) {
> > +            if (env->config->tlb_variable_way[5]) {
> >                  uint32_t eibase = 27 + get_page_size(env, dtlb, wi);
> >                  *ei = (v >> eibase) & 0x3;
> >              } else {
> > @@ -204,7 +193,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState *env, uint32_t v,
> >              break;
> >
> >          case 6:
> > -            if (varway56) {
> > +            if (env->config->tlb_variable_way[6]) {
> >                  uint32_t eibase = 29 - get_page_size(env, dtlb, wi);
> >                  *ei = (v >> eibase) & 0x7;
> >              } else {
>
> There's no direct code duplication, but it definitely feels like
> the logic for "figure out how many bits we're dealing with" is
> duplicated across these three functions.
>
> I think it ought to be possible to have a function (or maybe two)
> which take account of both the way number and tlb_get_variable_way[]
> such that all of these three functions then don't need to have
> a switch on the way or look at tlb_variable_way[] themselves...

Ok, let me have another look at cleaning this up.

-- 
Thanks.
-- Max

Re: [PATCH 1/2] target/xtensa: wrap MMU and MPU state into structures

[Peter Maydell]

On Mon, 22 Jan 2024 at 18:45, Max Filippov <jcmvbkbc@gmail.com> wrote:
>
> On Mon, Jan 22, 2024 at 10:29 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvbkbc@gmail.com> wrote:
> > > +    union {
> > > +        XtensaMMU mmu;
> > > +        XtensaMPU mpu;
> > > +    };
> >
> > Is it really worth having this be a union ? I suspect it will
> > make adding migration/savevm support later more awkward.
>
> I have a draft implementation of savevm for xtensa and I did this part
> using subsections with the .needed callback checking whether the
> MMU or MPU option is enabled in the config. I wonder where the
> awkwardness is expected.

Oh, well if it works that's fine I guess. I was kind of thinking
that if you didn't have the union you could avoid having
subsections entirely.

On Arm we don't try to save space in the CPU struct by
using unions, even though some fields are A-profile
specific and some are R or M-profile specific.

-- PMM