Re: [Qemu-devel] [PATCH 6/9] target-mips: Add an MMU mode for ERL

[James Hogan]

On Thu, Oct 13, 2016 at 02:18:31PM +0100, Yongbok Kim wrote:
> 
> 
> On 06/09/2016 12:03, James Hogan wrote:
> > The segmentation control feature allows a legacy memory segment to
> > become unmapped uncached at error level (according to CP0_Status.ERL),
> > and in fact the user segment is already treated in this way by QEMU.
> > 
> > Add a new MMU mode for this state so that QEMU's mappings don't persist
> > between ERL=0 and ERL=1.
> > 
> > Signed-off-by: James Hogan <address@hidden>
> > Cc: Leon Alrae <address@hidden>
> > Cc: Aurelien Jarno <address@hidden>
> > ---
> >  target-mips/cpu.h       | 17 +++++++++++++----
> >  target-mips/op_helper.c |  2 ++
> >  2 files changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> > index 8ddc965e4735..2abb330272d9 100644
> > --- a/target-mips/cpu.h
> > +++ b/target-mips/cpu.h
> > @@ -134,7 +134,7 @@ struct CPUMIPSFPUContext {
> >  #define FP_UNIMPLEMENTED  32
> >  };
> >  
> > -#define NB_MMU_MODES 3
> > +#define NB_MMU_MODES 4
> >  #define TARGET_INSN_START_EXTRA_WORDS 2
> >  
> >  typedef struct CPUMIPSMVPContext CPUMIPSMVPContext;
> > @@ -550,7 +550,7 @@ struct CPUMIPSState {
> >  #define EXCP_INST_NOTAVAIL 0x2 /* No valid instruction word for BadInstr */
> >      uint32_t hflags;    /* CPU State */
> >      /* TMASK defines different execution modes */
> > -#define MIPS_HFLAG_TMASK  0xF5807FF
> > +#define MIPS_HFLAG_TMASK  0x1F5807FF
> >  #define MIPS_HFLAG_MODE   0x00007 /* execution modes                    */
> >      /* The KSU flags must be the lowest bits in hflags. The flag order
> >         must be the same as defined for CP0 Status. This allows to use
> > @@ -600,6 +600,7 @@ struct CPUMIPSState {
> >  #define MIPS_HFLAG_FRE   0x2000000 /* FRE enabled */
> >  #define MIPS_HFLAG_ELPA  0x4000000
> >  #define MIPS_HFLAG_ITC_CACHE  0x8000000 /* CACHE instr. operates on ITC 
> > tag */
> > +#define MIPS_HFLAG_ERL   0x10000000 /* error level flag */
> >      target_ulong btarget;        /* Jump / branch target               */
> >      target_ulong bcond;          /* Branch condition (if needed)       */
> >  
> > @@ -694,11 +695,16 @@ extern uint32_t cpu_rddsp(uint32_t mask_num, 
> > CPUMIPSState *env);
> >  #define MMU_MODE0_SUFFIX _kernel
> >  #define MMU_MODE1_SUFFIX _super
> >  #define MMU_MODE2_SUFFIX _user
> > +#define MMU_MODE3_SUFFIX _error
> >  #define MMU_USER_IDX 2
> >  
> >  static inline int hflags_mmu_index(uint32_t hflags)
> >  {
> > -    return hflags & MIPS_HFLAG_KSU;
> > +    if (hflags & MIPS_HFLAG_ERL) {
> > +        return 3; /* ERL */
> 
> Not really useful but consider to macro it.

Yeh, I mainly didn't do this as the other MMU indices don't have
definitions either (see switch of literal numbers below).

Cheers
James

> 
> > +    } else {
> > +        return hflags & MIPS_HFLAG_KSU;
> > +    }
> >  }
> >  
> >  static inline int cpu_mmu_index (CPUMIPSState *env, bool ifetch)
> > @@ -966,7 +972,10 @@ static inline void compute_hflags(CPUMIPSState *env)
> >                       MIPS_HFLAG_F64 | MIPS_HFLAG_FPU | MIPS_HFLAG_KSU |
> >                       MIPS_HFLAG_AWRAP | MIPS_HFLAG_DSP | MIPS_HFLAG_DSPR2 |
> >                       MIPS_HFLAG_SBRI | MIPS_HFLAG_MSA | MIPS_HFLAG_FRE |
> > -                     MIPS_HFLAG_ELPA);
> > +                     MIPS_HFLAG_ELPA | MIPS_HFLAG_ERL);
> > +    if (env->CP0_Status & (1 << CP0St_ERL)) {
> > +        env->hflags |= MIPS_HFLAG_ERL;
> > +    }
> >      if (!(env->CP0_Status & (1 << CP0St_EXL)) &&
> >          !(env->CP0_Status & (1 << CP0St_ERL)) &&
> >          !(env->hflags & MIPS_HFLAG_DM)) {
> > diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> > index 71ad16e41dd4..829ab0bc3cca 100644
> > --- a/target-mips/op_helper.c
> > +++ b/target-mips/op_helper.c
> > @@ -66,6 +66,7 @@ static inline type do_##name(CPUMIPSState *env, 
> > target_ulong addr,      \
> >      case 1: return (type) cpu_##insn##_super_ra(env, addr, retaddr);    \
> >      default:                                                            \
> >      case 2: return (type) cpu_##insn##_user_ra(env, addr, retaddr);     \
> > +    case 3: return (type) cpu_##insn##_error_ra(env, addr, retaddr);    \
> >      }                                                                   \
> >  }
> >  #endif
> > @@ -93,6 +94,7 @@ static inline void do_##name(CPUMIPSState *env, 
> > target_ulong addr,      \
> >      case 1: cpu_##insn##_super_ra(env, addr, val, retaddr); break;      \
> >      default:                                                            \
> >      case 2: cpu_##insn##_user_ra(env, addr, val, retaddr); break;       \
> > +    case 3: cpu_##insn##_error_ra(env, addr, val, retaddr); break;      \
> >      }                                                                   \
> >  }
> >  #endif
> > 
> 
> Otherwise,
> 
> Reviewed-by: Yongbok Kim <address@hidden>
> 
> Regards,
> Yongbok

signature.asc
Description: Digital signature