On Tue, May 11, 2021 at 1:14 PM Wang Junqiang <wangjunqi...@iscas.ac.cn> wrote: > > > > On 2021/5/10 上午10:17, Alistair Francis wrote: > > C isOn Fri, May 7, 2021 at 11:25 PM wangjunqiang > > <wangjunqi...@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.
That sounds like a great idea! If we could contain all vendor changes to a single file (for example a nuclei.c file in target/riscv/) I think it would be much easier to maintain. Do you want to start by adding something like that in the next patch series? Alistair > > Regards > Wang Junqiang >
