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

[Tim Ansell]

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?

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
>