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

[Max Filippov <jcmvbkbc@gmail.com>]

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