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.2On 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
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
© 2016 - 2024 Red Hat, Inc.