Re: [Qemu-devel] [PATCH 1/9] target-mips: Add CP0_Ebase.WG (write gate) support

[James Hogan]

On Fri, Oct 07, 2016 at 02:42:15PM +0100, Yongbok Kim wrote:
> 
> 
> On 06/09/2016 12:03, 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 <address@hidden>
> > Cc: Leon Alrae <address@hidden>
> > Cc: Aurelien Jarno <address@hidden>
> > ---
> >  target-mips/cpu.h            |  4 +++-
> >  target-mips/helper.c         | 13 +++++++------
> >  target-mips/machine.c        |  6 +++---
> >  target-mips/op_helper.c      | 13 +++++++++++--
> >  target-mips/translate.c      |  8 +++++---
> >  target-mips/translate_init.c |  1 +
> >  6 files changed, 30 insertions(+), 15 deletions(-)
> > 
> > diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> > index 5182dc74ffa3..6ea2bf14c817 100644
> > --- a/target-mips/cpu.h
> > +++ b/target-mips/cpu.h
> > @@ -399,7 +399,9 @@ struct CPUMIPSState {
> >  #define CP0Ca_EC    2
> >      target_ulong CP0_EPC;
> >      int32_t CP0_PRid;
> > -    int32_t CP0_EBase;
> > +    target_ulong CP0_EBase;
> > +    target_ulong CP0_EBase_rw_bitmask;
> > +#define CP0EBase_WG 11
> >      target_ulong CP0_CMGCRBase;
> >      int32_t CP0_Config0;
> >  #define CP0C0_M    31
> > diff --git a/target-mips/helper.c b/target-mips/helper.c
> > index c864b15b97a8..29ebf391cb94 100644
> > --- a/target-mips/helper.c
> > +++ b/target-mips/helper.c
> > @@ -821,11 +821,7 @@ void mips_cpu_do_interrupt(CPUState *cs)
> >          goto set_EPC;
> >      case EXCP_CACHE:
> >          cause = 30;
> > -        if (env->CP0_Status & (1 << CP0St_BEV)) {
> > -            offset = 0x100;
> > -        } else {
> > -            offset = 0x20000100;
> > -        }
> > +        offset = 0x100;
> >   set_EPC:
> >          if (!(env->CP0_Status & (1 << CP0St_EXL))) {
> >              env->CP0_EPC = exception_resume_pc(env);
> > @@ -851,9 +847,14 @@ void mips_cpu_do_interrupt(CPUState *cs)
> >          env->hflags &= ~MIPS_HFLAG_BMASK;
> >          if (env->CP0_Status & (1 << CP0St_BEV)) {
> >              env->active_tc.PC = env->exception_base + 0x200;
> > +        } else if (cause == 30 && !(env->CP0_Config5 & (1 << CP0C5_CV))) {
> 
> && !Config3SC? per MIPS32PRA 4.10.1.6.
> It is not able to check Config3SC yet as it has not been defined but it can
> be added later after the patch #7

Okay, though CV is controlled via .CP0_Config5_rw_bitmask already, but
no harm adding the Config3.SC check too.

> 
> > +            /* Force KSeg1 for cache errors */
> > +            env->active_tc.PC = (int32_t)KSEG1_BASE |
> > +                                (env->CP0_EBase & 0x1FFFF000);
> >          } else {
> > -            env->active_tc.PC = (int32_t)(env->CP0_EBase & ~0x3ff);
> > +            env->active_tc.PC = env->CP0_EBase & ~0xfff;
> >          }
> > +
> >          env->active_tc.PC += offset;
> >          set_hflags_for_handler(env);
> >          env->CP0_Cause = (env->CP0_Cause & ~(0x1f << CP0Ca_EC)) | (cause 
> > << CP0Ca_EC);
> > diff --git a/target-mips/machine.c b/target-mips/machine.c
> > index a27f2f156da9..e795fecaa0d6 100644
> > --- a/target-mips/machine.c
> > +++ b/target-mips/machine.c
> > @@ -206,8 +206,8 @@ const VMStateDescription vmstate_tlb = {
> >  
> >  const VMStateDescription vmstate_mips_cpu = {
> >      .name = "cpu",
> > -    .version_id = 8,
> > -    .minimum_version_id = 8,
> > +    .version_id = 9,
> > +    .minimum_version_id = 9,
> >      .post_load = cpu_post_load,
> >      .fields = (VMStateField[]) {
> >          /* Active TC */
> > @@ -267,7 +267,7 @@ const VMStateDescription vmstate_mips_cpu = {
> >          VMSTATE_INT32(env.CP0_Cause, MIPSCPU),
> >          VMSTATE_UINTTL(env.CP0_EPC, MIPSCPU),
> >          VMSTATE_INT32(env.CP0_PRid, MIPSCPU),
> > -        VMSTATE_INT32(env.CP0_EBase, MIPSCPU),
> > +        VMSTATE_UINTTL(env.CP0_EBase, MIPSCPU),
> >          VMSTATE_INT32(env.CP0_Config0, MIPSCPU),
> >          VMSTATE_INT32(env.CP0_Config1, MIPSCPU),
> >          VMSTATE_INT32(env.CP0_Config2, MIPSCPU),
> > diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> > index ea2f2abe198c..71ad16e41dd4 100644
> > --- a/target-mips/op_helper.c
> > +++ b/target-mips/op_helper.c
> > @@ -1526,14 +1526,23 @@ 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);
> > +    if (arg1 & (1 << CP0EBase_WG) & env->CP0_EBase_rw_bitmask) {
> 
> I guess it has to check the WG bit of EBase register rather than the new WG
> bit value. "To ensure backward compatibility with MIPS32, the write-gate
> bit must be set before the upper bits can be changed."

Yeh, that is misleadingly worded. The way real hardware behaves is that
the upper bits are only writable when the WG bit is set as part of the
same write.

The description of WG bit itself backs that up:

"Bits 31..30 are unchanged on writes to EBase when WG=0 in the value
being written. The WG bit must be set true in the written value to
change the values of bits 31..30"

> 
> CP0_EBase_rw_bitmask is now added but it is not really used other than WG
> bit. Perhaps rename it into something like CP0_EBaseWG_rw_bitmask would be
> better to avoid any confusion. Otherwise assign those hard-coded mask into
> the bitmask variable in the cpu_state_reset().

Since the mask depends on WG, i'll rename it.

Thanks for the review.

Cheers
James

> 
> > +        env->CP0_EBase = (env->CP0_EBase & 0x7ff) | (arg1 & ~0x7ff);
> > +    } else {
> > +        env->CP0_EBase = (env->CP0_EBase & ~0x3FFFF800) | (arg1 & 
> > 0x3FFFF800);
> > +    }
> >  }
> >  
> >  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);
> > +    if (arg1 & (1 << CP0EBase_WG) & env->CP0_EBase_rw_bitmask) {
> 
> Same here.
> 
> > +        other->CP0_EBase = (other->CP0_EBase & 0x7ff) | (arg1 & ~0x7ff);
> > +    } else {
> > +        other->CP0_EBase = (other->CP0_EBase & ~0x3FFFF800) |
> > +                           (arg1 & 0x3FFFF800);
> > +    }
> >  }
> >  
> >  target_ulong helper_mftc0_configx(CPUMIPSState *env, target_ulong idx)
> > diff --git a/target-mips/translate.c b/target-mips/translate.c
> > index bab52cb25498..e224c2f09af4 100644
> > --- a/target-mips/translate.c
> > +++ b/target-mips/translate.c
> > @@ -5316,7 +5316,8 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
> > reg, int sel)
> >              break;
> >          case 1:
> >              check_insn(ctx, ISA_MIPS32R2);
> > -            gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_EBase));
> > +            tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_EBase));
> > +            tcg_gen_ext32s_tl(arg, arg);
> >              rn = "EBase";
> >              break;
> >          case 3:
> > @@ -6630,7 +6631,7 @@ static void gen_dmfc0(DisasContext *ctx, TCGv arg, 
> > int reg, int sel)
> >              break;
> >          case 1:
> >              check_insn(ctx, ISA_MIPS32R2);
> > -            gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_EBase));
> > +            tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_EBase));
> >              rn = "EBase";
> >              break;
> >          case 3:
> > @@ -20247,6 +20248,7 @@ void cpu_state_reset(CPUMIPSState *env)
> >      env->CP0_SRSConf4 = env->cpu_model->CP0_SRSConf4;
> >      env->CP0_PageGrain_rw_bitmask = 
> > env->cpu_model->CP0_PageGrain_rw_bitmask;
> >      env->CP0_PageGrain = env->cpu_model->CP0_PageGrain;
> > +    env->CP0_EBase_rw_bitmask = env->cpu_model->CP0_EBase_rw_bitmask;
> >      env->active_fpu.fcr0 = env->cpu_model->CP1_fcr0;
> >      env->active_fpu.fcr31_rw_bitmask = 
> > env->cpu_model->CP1_fcr31_rw_bitmask;
> >      env->active_fpu.fcr31 = env->cpu_model->CP1_fcr31;
> > @@ -20297,7 +20299,7 @@ void cpu_state_reset(CPUMIPSState *env)
> >      if (kvm_enabled()) {
> >          env->CP0_EBase |= 0x40000000;
> >      } else {
> > -        env->CP0_EBase |= 0x80000000;
> > +        env->CP0_EBase |= (int32_t)0x80000000;
> >      }
> >      if (env->CP0_Config3 & (1 << CP0C3_CMGCR)) {
> >          env->CP0_CMGCRBase = 0x1fbf8000 >> 4;
> > diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c
> > index 39ed5c4c1b12..f327e6e7de6c 100644
> > --- a/target-mips/translate_init.c
> > +++ b/target-mips/translate_init.c
> > @@ -101,6 +101,7 @@ struct mips_def_t {
> >      int32_t CP0_SRSConf4;
> >      int32_t CP0_PageGrain_rw_bitmask;
> >      int32_t CP0_PageGrain;
> > +    target_ulong CP0_EBase_rw_bitmask;
> >      int insn_flags;
> >      enum mips_mmu_types mmu_type;
> >  };
> > 
> 
> 
> Regards,
> Yongbok
>

signature.asc
Description: Digital signature