qemu-riscv
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH 1/5] target/riscv: Add Nuclei CSR and Update interrupt ha


From: Wang Junqiang
Subject: Re: [RFC PATCH 1/5] target/riscv: Add Nuclei CSR and Update interrupt handling
Date: Tue, 11 May 2021 11:14:17 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0



On 2021/5/10 上午10:17, Alistair Francis wrote:
  C isOn Fri, May 7, 2021 at 11:25 PM wangjunqiang
<wangjunqiang@iscas.ac.cn> wrote:

This patch adds Nuclei CSR support for ECLIC and update the
related interrupt handling.

https://doc.nucleisys.com/nuclei_spec/isa/core_csr.html

Hello,

Thanks for the patches!

This patch is very long and you will need to split it up before it can
be merged. I understand this is just an RFC, but it's still best to
start with small patches. Generally each patch should add a feature
and it seems like you have added lots of features in this patch. This
patch could probably be broken into at least 4 different patches.

As well as that you will want to ensure that your commit message and
description explains what you are doing in that patch and in some
cases justify the change. For example adding a new CPU doesn't need a
justification (as that's easy for me to understand), but changing some
existing code might need an explanation of why we need/want that
change.

This is still a great start though! I look forward to your future patches.

I have left a few comments below as well.

Thank you for your reply and comments.I will split it into small patches by feature in next version.And add more detailed description. To make a brief explanation, add cpu here to simplify the command line when using -cpu.


---
  target/riscv/cpu.c                      |  25 +-
  target/riscv/cpu.h                      |  42 ++-
  target/riscv/cpu_bits.h                 |  37 +++
  target/riscv/cpu_helper.c               |  80 +++++-
  target/riscv/csr.c                      | 347 +++++++++++++++++++++++-
  target/riscv/insn_trans/trans_rvi.c.inc |  16 +-
  target/riscv/op_helper.c                |  14 +
  7 files changed, 552 insertions(+), 9 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7d6ed80f6b..b2a96effbc 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -173,6 +173,16 @@ static void rv64_sifive_e_cpu_init(Object *obj)
      set_priv_version(env, PRIV_VERSION_1_10_0);
      qdev_prop_set_bit(DEVICE(obj), "mmu", false);
  }
+
+static void rv64imafdcu_nuclei_cpu_init(Object *obj)
+{
+    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
+    set_priv_version(env, PRIV_VERSION_1_10_0);
+    qdev_prop_set_bit(DEVICE(obj), "mmu", false);
+    set_resetvec(env, DEFAULT_RSTVEC);
+    set_feature(env, RISCV_FEATURE_PMP);
+}
  #else
  static void rv32_base_cpu_init(Object *obj)
  {
@@ -212,6 +222,16 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
      set_resetvec(env, DEFAULT_RSTVEC);
      qdev_prop_set_bit(DEVICE(obj), "mmu", false);
  }
+
+static void rv32imafdcu_nuclei_cpu_init(Object *obj)
+{
+    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
+    set_priv_version(env, PRIV_VERSION_1_10_0);
+    qdev_prop_set_bit(DEVICE(obj), "mmu", false);
+    set_resetvec(env, DEFAULT_RSTVEC);
+    set_feature(env, RISCV_FEATURE_PMP);
+}
  #endif

  static ObjectClass *riscv_cpu_class_by_name(const char *cpu_model)
@@ -331,7 +351,7 @@ static bool riscv_cpu_has_work(CPUState *cs)
       * Definition of the WFI instruction requires it to ignore the privilege
       * mode and delegation registers, but respect individual enables
       */
-    return (env->mip & env->mie) != 0;
+    return ((env->mip & env->mie) != 0  || (env->exccode != -1));

This change for example needs to be explained, I'm not sure what exccode is

  #else
      return true;
  #endif
@@ -356,6 +376,7 @@ static void riscv_cpu_reset(DeviceState *dev)
      env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
      env->mcause = 0;
      env->pc = env->resetvec;
+    env->exccode = -1;
      env->two_stage_lookup = false;
  #endif
      cs->exception_index = EXCP_NONE;
@@ -704,10 +725,12 @@ static const TypeInfo riscv_cpu_type_infos[] = {
      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E31,       rv32_sifive_e_cpu_init),
      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E34,       rv32_imafcu_nommu_cpu_init),
      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34,       rv32_sifive_u_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_NUCLEI_N307FD,    rv32imafdcu_nuclei_cpu_init),
  #elif defined(TARGET_RISCV64)
      DEFINE_CPU(TYPE_RISCV_CPU_BASE64,           rv64_base_cpu_init),
      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E51,       rv64_sifive_e_cpu_init),
      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U54,       rv64_sifive_u_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_NUCLEI_NX600FD,    rv64imafdcu_nuclei_cpu_init),
  #endif
  };

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0a33d387ba..1d3a1986a6 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -33,6 +33,7 @@
  #define RISCV_CPU_TYPE_SUFFIX "-" TYPE_RISCV_CPU
  #define RISCV_CPU_TYPE_NAME(name) (name RISCV_CPU_TYPE_SUFFIX)
  #define CPU_RESOLVING_TYPE TYPE_RISCV_CPU
+#define CPU_INTERRUPT_ECLIC CPU_INTERRUPT_TGT_EXT_0

  #define TYPE_RISCV_CPU_ANY              RISCV_CPU_TYPE_NAME("any")
  #define TYPE_RISCV_CPU_BASE32           RISCV_CPU_TYPE_NAME("rv32")
@@ -43,6 +44,8 @@
  #define TYPE_RISCV_CPU_SIFIVE_E51       RISCV_CPU_TYPE_NAME("sifive-e51")
  #define TYPE_RISCV_CPU_SIFIVE_U34       RISCV_CPU_TYPE_NAME("sifive-u34")
  #define TYPE_RISCV_CPU_SIFIVE_U54       RISCV_CPU_TYPE_NAME("sifive-u54")
+#define TYPE_RISCV_CPU_NUCLEI_N307FD    RISCV_CPU_TYPE_NAME("nuclei-n307fd")
+#define TYPE_RISCV_CPU_NUCLEI_NX600FD    RISCV_CPU_TYPE_NAME("nuclei-nx600fd")

  #if defined(TARGET_RISCV32)
  # define TYPE_RISCV_CPU_BASE            TYPE_RISCV_CPU_BASE32
@@ -80,7 +83,8 @@
  enum {
      RISCV_FEATURE_MMU,
      RISCV_FEATURE_PMP,
-    RISCV_FEATURE_MISA
+    RISCV_FEATURE_MISA,
+    RISCV_FEATURE_ECLIC

The same here, what is ECLIC? The ECLIC should be added in a seperate patch.


ECLIC is Enhanced Core Local Interrupt Controller.And added some customized csr on the basis of clic to speed up Tail-Chaining processing.

https://doc.nucleisys.com/nuclei_spec/isa/eclic.html

  };

  #define PRIV_VERSION_1_10_0 0x00011000
@@ -174,10 +178,34 @@ struct CPURISCVState {
      target_ulong scause;

      target_ulong mtvec;
+    target_ulong mtvt;   /* eclic */
      target_ulong mepc;
      target_ulong mcause;
      target_ulong mtval;  /* since: priv-1.10.0 */

+    target_ulong mnxti; /* eclic */
+    target_ulong mintstatus; /* eclic */
+    target_ulong mscratchcsw;
+    target_ulong mscratchcswl;
+
+    /* NMI  CSR*/
+    target_ulong mnvec;
+    target_ulong msubm;
+    target_ulong mdcause;
+    target_ulong mmisc_ctl;
+    target_ulong msavestatus;
+    target_ulong msaveepc1;
+    target_ulong msavecause1;
+    target_ulong msaveepc2;
+    target_ulong msavecause2;
+    target_ulong msavedcause1;
+    target_ulong msavedcause2;
+    target_ulong pushmsubm;
+    target_ulong mtvt2;
+    target_ulong jalmnxti;
+    target_ulong pushmcause;
+    target_ulong pushmepc;

What are NMI CSRs?


Nuclei's Customized registers are used for NMI related processing

https://doc.nucleisys.com/nuclei_spec/isa/core_csr.html


+
      /* Hypervisor CSRs */
      target_ulong hstatus;
      target_ulong hedeleg;
@@ -228,6 +256,9 @@ struct CPURISCVState {
      uint64_t mtohost;
      uint64_t timecmp;

+    /*nuclei timer comparators */
+    uint64_t mtimecmp;

RISC-V has a mtimecmp, does nuclei add another one?


I will delete it, it was originally used for Shadow copy, I can move it to the device

https://doc.nucleisys.com/nuclei_spec/isa/timer.html

+
      /* physical memory protection */
      pmp_table_t pmp_state;

@@ -243,6 +274,13 @@ struct CPURISCVState {

      /* Fields from here on are preserved across CPU reset. */
      QEMUTimer *timer; /* Internal timer */
+
+    QEMUTimer *mtimer; /* Nuclei Internal timer */

Why do you need a timer here just for the Nuclei CPU?


same as above

+    void *eclic;
+    uint32_t exccode;    /* irq id: 0~11  shv: 12 */
+    uint32_t eclic_flag;
+
+    bool irq_pending;
  };

  OBJECT_DECLARE_TYPE(RISCVCPU, RISCVCPUClass,
@@ -364,6 +402,8 @@ void riscv_cpu_list(void);
  void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env);
  int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint32_t interrupts);
  uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t mask, uint32_t value);
+void riscv_cpu_eclic_clean_pending(void *eclic, int irq);
+void riscv_cpu_eclic_get_next_interrupt(void *eclic);
  #define BOOL_TO_MASK(x) (-!!(x)) /* helper for riscv_cpu_update_mip value */
  void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t (*fn)(uint32_t),
                               uint32_t arg);
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index caf4599207..24ed7a99e1 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -149,6 +149,7 @@
  #define CSR_MIE             0x304
  #define CSR_MTVEC           0x305
  #define CSR_MCOUNTEREN      0x306
+#define CSR_MTVT      0x307 /* customized */

So I'm not sure what to do here. This seems to be a custom CSR just
for the Nuclei that isn't part of the RISC-V spec or a draft spec.

The problem is that accepting custom specs into QEMU makes it hard for
us to maintain the RISC-V port. After it has been merged the
maintainers now have to understand the Nuclei CPU and support it as
part of the core RISC-V code.

On the other hand I have seen a few CPUs that use CSRs and I don't
want to not allow implementations that use custom CSRs. I think there
is a compromise here. We probably don't want to support really custom
features, but we probably can afford to support some extra CSRs.

I think the best course of action here is to split this patch up and
we can then think about each custom feature/CSR and accept some
depending on how intrusive they are into the QEMU code. It will also
have to be added in a way that allows other implementations to have
different custom CSRs. We (the QEMU RISC-V community) can help you
with this.

Alistair


Thanks for your comment. About customized csr, I have a rough idea, whether it is possible to open the interface for manufacturers to allow them to implement their own csr.To implement the registration callback interface, add a branch to the riscv_csrrw function, and define a switch for the cpu. When a custom csr is supported, the vendor registration is preferred.The manufacturer maintains its own csr and does not invade the qemu code much. Of course, there may be some unknown security and stability issues.

Regards
Wang Junqiang




reply via email to

[Prev in Thread] Current Thread [Next in Thread]