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