On Wed, Jul 19, 2017 at 03:54:47PM +0100, Yongbok Kim wrote:
> 
> 
> On 18/07/2017 12:55, James Hogan wrote:
> > Add support for the CP0_EBase.WG bit, which allows upper bits to be
> > written (bits 31:30 on MIPS32, or bits 63:30 on MIPS64), along with the
> > CP0_Config5.CV bit to control whether the exception vector for Cache
> > Error exceptions is forced into KSeg1.
> > 
> > This is necessary on MIPS32 to support Segmentation Control and Enhanced
> > Virtual Addressing (EVA) extensions (where KSeg1 addresses may not
> > represent an unmapped uncached segment).
> > 
> > It is also useful on MIPS64 to allow the exception base to reside in
> > XKPhys, and possibly out of range of KSEG0 and KSEG1.
> > 
> > Signed-off-by: James Hogan <james.hogan@imgtec.com>
> > Cc: Yongbok Kim <yongbok.kim@imgtec.com>
> > Cc: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> > Changes in v2:
> > - Fix CP0_EBase.WG to be read only when WG is not set in
> >   CP0_EBase_rw_bitmask, otherwise it will be wrongly probed as present.
> > - Make cache error exception vector conditional on Config3.SC as well as
> >   Config5.CV, as per the PRA, and take the CP0C3_SC definition from
> >   patch 7 (Yongbok).
> > - Rename CP0_EBase_rw_bitmask to CP0_EBaseWG_rw_bitmask (Yongbok).
> > ---
> >  target/mips/cpu.h            |  5 ++++-
> >  target/mips/helper.c         | 14 ++++++++------
> >  target/mips/machine.c        |  6 +++---
> >  target/mips/op_helper.c      | 12 ++++++++++--
> >  target/mips/translate.c      |  8 +++++---
> >  target/mips/translate_init.c |  1 +
> >  6 files changed, 31 insertions(+), 15 deletions(-)
> > 
> 
> 
> > --- a/target/mips/op_helper.c
> > +++ b/target/mips/op_helper.c
> > @@ -1515,14 +1515,22 @@ target_ulong helper_mftc0_ebase(CPUMIPSState *env)
> >  
> >  void helper_mtc0_ebase(CPUMIPSState *env, target_ulong arg1)
> >  {
> > -    env->CP0_EBase = (env->CP0_EBase & ~0x3FFFF000) | (arg1 & 0x3FFFF000);
> > +    target_ulong mask = 0x3FFFF000 | env->CP0_EBaseWG_rw_bitmask;
> > +    if (arg1 & (1 << CP0EBase_WG) & mask) {
> 
> isn't it just ...?
> if (arg1 & env->CP0_EBaseWG_rw_bitmask) {

I suppose, now that the field is specific to the WG bit.

Thanks
James

> 
> > +        mask |= ~0x3FFFFFFF;
> > +    }
> > +    env->CP0_EBase = (env->CP0_EBase & ~mask) | (arg1 & mask);
> >  }
> >  
> >  void helper_mttc0_ebase(CPUMIPSState *env, target_ulong arg1)
> >  {
> >      int other_tc = env->CP0_VPEControl & (0xff << CP0VPECo_TargTC);
> >      CPUMIPSState *other = mips_cpu_map_tc(env, &other_tc);
> > -    other->CP0_EBase = (other->CP0_EBase & ~0x3FFFF000) | (arg1 & 0x3FFFF000);
> > +    target_ulong mask = 0x3FFFF000 | env->CP0_EBaseWG_rw_bitmask;
> > +    if (arg1 & (1 << CP0EBase_WG) & mask) {
> 
> here as well.
> 
> > +        mask |= ~0x3FFFFFFF;
> > +    }
> > +    other->CP0_EBase = (other->CP0_EBase & ~mask) | (arg1 & mask);
> >  }
> >  
> 
> 
> Otherwise,
> Reviewed-by: Yongbok Kim <yongbok.kim@imgtec.com>
> 
> Regards,
> Yongbok