Re: [PATCH 3/4] target/riscv: Implement AIA local interrupt CSRs

[Alistair Francis]

On Sat, Jun 12, 2021 at 12:04 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Fri, Jun 11, 2021 at 2:16 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Fri, Jun 11, 2021 at 3:04 PM Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Fri, Jun 11, 2021 at 4:49 AM Alistair Francis <alistair23@gmail.com> 
> > > wrote:
> > > >
> > > > On Sat, May 15, 2021 at 12:34 AM Anup Patel <anup.patel@wdc.com> wrote:
> > > > >
> > > > > We implement various AIA local interrupt CSRs for M-mode, HS-mode,
> > > > > and VS-mode.
> > > > >
> > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > > > > ---
> > > > >  target/riscv/cpu.c        |   27 +-
> > > > >  target/riscv/cpu.h        |   52 +-
> > > > >  target/riscv/cpu_helper.c |  245 ++++++++-
> > > > >  target/riscv/csr.c        | 1059 
> > > > > +++++++++++++++++++++++++++++++++++--
> > > > >  target/riscv/machine.c    |   26 +-
> > > > >  5 files changed, 1309 insertions(+), 100 deletions(-)
> > > >
> > > > I feel this patch could be split up more :)
> > >
> > > This is patch is large because I did not want to break functionality.
> > >
> > > I try again to break this patch. At the moment, the best I can do is
> > > to break in to two parts.
> > > 1) AIA local interrupt CSRs without IMSIC
> > > 2) Extend AIA local interrupt CSRs to support IMSIC register access
> >
> > As the patch is being added while AIA isn't enabled you are able to
> > add the AIA in breaking stages. That is the AIA isn't fully
> > functional, you still have to make sure not to break existing users.
> >
> > >
> > > >
> > > > >
> > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > index f3702111ae..795162834b 100644
> > > > > --- a/target/riscv/cpu.c
> > > > > +++ b/target/riscv/cpu.c
> > > > > @@ -256,11 +256,11 @@ static void riscv_cpu_dump_state(CPUState *cs, 
> > > > > FILE *f, int flags)
> > > > >          qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsstatus ",
> > > > >                       (target_ulong)env->vsstatus);
> > > > >      }
> > > > > -    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mip     ", env->mip);
> > > > > -    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mie     ", env->mie);
> > > > > -    qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mideleg ", 
> > > > > env->mideleg);
> > > > > +    qemu_fprintf(f, " %s %016" PRIx64 "\n", "mip     ", env->mip);
> > > > > +    qemu_fprintf(f, " %s %016" PRIx64 "\n", "mie     ", env->mie);
> > > > > +    qemu_fprintf(f, " %s %016" PRIx64 "\n", "mideleg ", 
> > > > > env->mideleg);
> > > > >      if (riscv_has_ext(env, RVH)) {
> > > > > -        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "hideleg ", 
> > > > > env->hideleg);
> > > > > +        qemu_fprintf(f, " %s %016" PRIx64 "\n", "hideleg ", 
> > > > > env->hideleg);
> > > > >      }
> > > > >      qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "medeleg ", 
> > > > > env->medeleg);
> > > > >      if (riscv_has_ext(env, RVH)) {
> > > > > @@ -345,6 +345,8 @@ void restore_state_to_opc(CPURISCVState *env, 
> > > > > TranslationBlock *tb,
> > > > >
> > > > >  static void riscv_cpu_reset(DeviceState *dev)
> > > > >  {
> > > > > +    uint8_t iprio;
> > > > > +    int i, irq, rdzero;
> > > > >      CPUState *cs = CPU(dev);
> > > > >      RISCVCPU *cpu = RISCV_CPU(cs);
> > > > >      RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > > > > @@ -357,6 +359,23 @@ static void riscv_cpu_reset(DeviceState *dev)
> > > > >      env->mcause = 0;
> > > > >      env->pc = env->resetvec;
> > > > >      env->two_stage_lookup = false;
> > > > > +
> > > > > +    /* Initialized default priorities of local interrupts. */
> > > > > +    for (i = 0; i < ARRAY_SIZE(env->miprio); i++) {
> > > > > +        iprio = riscv_cpu_default_priority(i);
> > > > > +        env->miprio[i] = iprio;
> > > > > +        env->siprio[i] = iprio;
> > > > > +        env->hviprio[i] = IPRIO_DEFAULT_MMAXIPRIO;
> > > > > +    }
> > > > > +    i = 0;
> > > > > +    while (!riscv_cpu_hviprio_index2irq(i, &irq, &rdzero)) {
> > > > > +        if (rdzero) {
> > > > > +            env->hviprio[irq] = 0;
> > > > > +        } else {
> > > > > +            env->hviprio[irq] = env->miprio[irq];
> > > > > +        }
> > > > > +        i++;
> > > > > +    }
> > > > >  #endif
> > > > >      cs->exception_index = EXCP_NONE;
> > > > >      env->load_res = -1;
> > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > > index f00c60c840..780d3f9058 100644
> > > > > --- a/target/riscv/cpu.h
> > > > > +++ b/target/riscv/cpu.h
> > > > > @@ -157,12 +157,12 @@ struct CPURISCVState {
> > > > >       */
> > > > >      uint64_t mstatus;
> > > > >
> > > > > -    target_ulong mip;
> > > > > +    uint64_t mip;
> > > >
> > > > This isn't right. MIP is a MXLEN-1 CSR. I didn't check but I assume
> > > > all the other existing target_ulong CSRs are the same.
> > >
> > > When AIA is available the number of local interrupts are 64 for
> > > both RV32 and RV64.
> >
> > Is that going to be reflected in the priv spec?
>
> The AIA spec is going to be separate from priv spec since
> it is totally optional.
>
> This AIA local interrupt CSRs will be part of AIA spec and should
> only be implemented if a RISC-V implementations wants to use
> AIA.
>
> We have four types of changes as far as CSRs go:
> 1) RV32 CSRs to support 64 local interrupts on RV32

Hmmm... This isn't ideal. So implementations without AIA will have a
Xlen MIP while implementations with AIA will have a 64-bit MIP?

I guess we just don't expose all of MIP, so it isn't that bad. Still
that change should be it's own patch with good justification as why we
aren't following the priv spec.

> 2) Indirect CSRs to access local interrupt priorities
> 3) Interrupt filtering CSRs
> 4) IMSIC support CSRs

New AIA CSRs are fine, it's just core changes that are more of a worry.

Alistair

>
> From above #3 is totally optional and not implemented by this
> patch whereas #4 is only required when platform has RISC-V IMSIC.
>
> A platform can skip all four changes mentioned above, if the
> platform only wants AIA APLIC to manage wired interrupts.
>
> Regards,
> Anup