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.
---
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.
+ 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