Re: [Qemu-devel] [PATCH 1/2] target/openrisc: Implement EVBAR register

[Stafford Horne]

On Thu, Apr 27, 2017 at 10:55:28AM +1000, Tim Ansell wrote:
> I'm about to add support for disabling the inbuilt or1k timer peripheral
> (as our SoC does not have it enabled). That isn't really a CPU feature so I
> think it still makes sense to have some type of feature field? Maybe CPU
> features should just be a separate category and set directly on that
> register?
> 
> I also thinks it makes sense to maybe define a couple of new CPU
> configurations. I think we might want,
> 
>  * or1k-all - OpenRISC CPU with all emulated features enabled.
>  * or1k-minimal - Only features enabled which can't be disabled.
>  * mor1k-default - Configured to match the defaults in the mor1k verilog
> repo.
> 
> Then I think we also want configs for the "major users" of mor1k like,
> 
>  * mor1k-litex - Configured to match the default config used by litex/misoc
> (maybe mor1k-misoc as an alias?)
>  * mor1k-fusesoc - Configured to match the default of FuseSoC
>  * mor1k-minsoc - Configured to match the default of minsoc (which is
> different to misoc)
> 
> Thoughts?

I think this makes sense, we have the UPR and CPUCFGR read only SPR's to
track many of these features.  Currently specifying a different model will
change the cpucfgr.  We could expand to also define UPR as well to give
something like like below.

static void or1200_initfn(Object *obj)
{
    OpenRISCCPU *cpu = OPENRISC_CPU(obj);

    cpu->env.cpucfgr = CPUCFGR_NSGF | CPUCFGR_OB32S | CPUCFGR_OF32S |
                       CPUCFGR_EVBARP;

    cpu->env.upr = UPR_UP | UPR_DMP | UPR_IMP | UPR_PICP | UPR_TTP |
                   UPR_PMP;

    /* Features not supported in SPR config */
    cpu->env.features = /* Any examples? */
}

static void or1k_minimal_initfn(Object *obj)
{
    OpenRISCCPU *cpu = OPENRISC_CPU(obj);

    /* Nothing Enabled? */
}


static const OpenRISCCPUInfo openrisc_cpus[] = {
    { .name = "or1200",      .initfn = or1200_initfn },
    { .name = "or1k-all",    .initfn = or1200_initfn },
    { .name = "or1k-minimal",.initfn = or1k_miniamal_initfn },
    ...
    { .name = "any",         .initfn = openrisc_any_initfn },
};


These could then be used in different parts of the code to track if we
actually enable those features or not.  i.e:

    if (cpu->env.upr & UPR_PICP) {
        cpu_openrisc_pic_init(cpu);
    }
    if (cpu->env.upr & UPR_TTP) {
        cpu_openrisc_clock_init(cpu);
    }

I think its possible and it will be a bit of work to get done.

Thoughts?

-Stafford

> Tim 'mithro' Ansell
> 
> On Apr 18, 2017 10:47 PM, "Stafford Horne" <address@hidden> wrote:
> 
> On Tue, Apr 18, 2017 at 04:15:50PM +1000, Tim 'mithro' Ansell wrote:
> > Exception Vector Base Address Register (EVBAR) - This optional register
> > can be used to apply an offset to the exception vector addresses.
> >
> > The significant bits (31-12) of the vector offset address for each
> > exception depend on the setting of the Supervision Register (SR)'s EPH
> > bit and the Exception Vector Base Address Register (EVBAR).
> >
> > Its presence is indicated by the EVBARP bit in the CPU Configuration
> > Register (CPUCFGR).
> >
> > Signed-off-by: Tim 'mithro' Ansell <address@hidden>
> > ---
> >  target/openrisc/cpu.c        | 2 ++
> >  target/openrisc/cpu.h        | 7 +++++++
> >  target/openrisc/interrupt.c  | 6 +++++-
> >  target/openrisc/sys_helper.c | 7 +++++++
> >  4 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> > index 7fd2b9a216..1524ed981a 100644
> > --- a/target/openrisc/cpu.c
> > +++ b/target/openrisc/cpu.c
> > @@ -134,6 +134,7 @@ static void or1200_initfn(Object *obj)
> >
> >      set_feature(cpu, OPENRISC_FEATURE_OB32S);
> >      set_feature(cpu, OPENRISC_FEATURE_OF32S);
> > +    set_feature(cpu, OPENRISC_FEATURE_EVBAR);
> >  }
> >
> >  static void openrisc_any_initfn(Object *obj)
> > @@ -141,6 +142,7 @@ static void openrisc_any_initfn(Object *obj)
> >      OpenRISCCPU *cpu = OPENRISC_CPU(obj);
> >
> >      set_feature(cpu, OPENRISC_FEATURE_OB32S);
> > +    set_feature(cpu, OPENRISC_FEATURE_EVBAR);
> >  }
> >
> >  typedef struct OpenRISCCPUInfo {
> > diff --git a/target/openrisc/cpu.h b/target/openrisc/cpu.h
> > index 418a0e6960..1958b72718 100644
> > --- a/target/openrisc/cpu.h
> > +++ b/target/openrisc/cpu.h
> > @@ -111,6 +111,11 @@ enum {
> >      CPUCFGR_OF32S = (1 << 7),
> >      CPUCFGR_OF64S = (1 << 8),
> >      CPUCFGR_OV64S = (1 << 9),
> > +    /* CPUCFGR_ND = (1 << 10), */
> > +    /* CPUCFGR_AVRP = (1 << 11), */
> > +    CPUCFGR_EVBARP = (1 << 12),
> > +    /* CPUCFGR_ISRP = (1 << 13), */
> > +    /* CPUCFGR_AECSRP = (1 << 14), */
> >  };
> >
> >  /* DMMU configure register */
> > @@ -200,6 +205,7 @@ enum {
> >      OPENRISC_FEATURE_OF32S = (1 << 7),
> >      OPENRISC_FEATURE_OF64S = (1 << 8),
> >      OPENRISC_FEATURE_OV64S = (1 << 9),
> > +    OPENRISC_FEATURE_EVBAR = (1 << 12),
> 
> Does anyone know why we have to add both Features and CPUCFG bits?
> 
> It seems like duplication to me.
> 
> >  };
> >
> >  /* Tick Timer Mode Register */
> > @@ -289,6 +295,7 @@ typedef struct CPUOpenRISCState {
> >      uint32_t dmmucfgr;        /* DMMU configure register */
> >      uint32_t immucfgr;        /* IMMU configure register */
> >      uint32_t esr;             /* Exception supervisor register */
> > +    uint32_t evbar;           /* Exception vector base address register
> */
> 
> If we add something to CPUOpenRISCState, we ideally need to also add it to
> machine.c vmstate definition.  However, I currently have a patch out to
> rework the vmstate serialization.  I can add this to that patch.
> 
> >      uint32_t fpcsr;           /* Float register */
> >      float_status fp_status;
> >
> > diff --git a/target/openrisc/interrupt.c b/target/openrisc/interrupt.c
> > index a2eec6fb32..78f0ba9421 100644
> > --- a/target/openrisc/interrupt.c
> > +++ b/target/openrisc/interrupt.c
> > @@ -65,7 +65,11 @@ void openrisc_cpu_do_interrupt(CPUState *cs)
> >      env->lock_addr = -1;
> >
> >      if (cs->exception_index > 0 && cs->exception_index < EXCP_NR) {
> > -        env->pc = (cs->exception_index << 8);
> > +        hwaddr vect_pc = cs->exception_index << 8;
> > +        if (env->cpucfgr & CPUCFGR_EVBARP) {
> > +            vect_pc |= env->evbar;
> > +        }
> > +        env->pc = vect_pc;
> >      } else {
> >          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
> >      }
> > diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
> > index 60c3193656..6ba816249b 100644
> > --- a/target/openrisc/sys_helper.c
> > +++ b/target/openrisc/sys_helper.c
> > @@ -39,6 +39,10 @@ void HELPER(mtspr)(CPUOpenRISCState *env,
> >          env->vr = rb;
> >          break;
> >
> > +    case TO_SPR(0, 11): /* EVBAR */
> > +        env->evbar = rb;
> > +        break;
> > +
> >      case TO_SPR(0, 16): /* NPC */
> >          cpu_restore_state(cs, GETPC());
> >          /* ??? Mirror or1ksim in not trashing delayed branch state
> > @@ -206,6 +210,9 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env,
> >      case TO_SPR(0, 4): /* IMMUCFGR */
> >          return env->immucfgr;
> >
> > +    case TO_SPR(0, 11): /* EVBAR */
> > +        return env->evbar;
> > +
> >      case TO_SPR(0, 16): /* NPC (equals PC) */
> >          cpu_restore_state(cs, GETPC());
> >          return env->pc;
> 
> Reviewed-by: Stafford Horne <address@hidden>
> 
> I will pull into my branch and send PR as is unless someone has more to
> say.
> 
> -Stafford
> 
> > --
> > 2.12.1
> >