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

2021-05-10 Thread Wang Junqiang




On 2021/5/11 上午11:43, Alistair Francis wrote:

On Tue, May 11, 2021 at 1:14 PM Wang Junqiang  wrote:




On 2021/5/10 上午10:17, Alistair Francis wrote:

   C isOn Fri, May 7, 2021 at 11:25 PM wangjunqiang
 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

Re: [RFC PATCH 2/5] hw/intc: Add Nuclei ECLIC device

2021-05-10 Thread Wang Junqiang




On 2021/5/10 下午1:26, Bin Meng wrote:

On Mon, May 10, 2021 at 10:27 AM Bin Meng  wrote:


On Mon, May 10, 2021 at 10:21 AM Alistair Francis  wrote:


On Fri, May 7, 2021 at 11:24 PM wangjunqiang  wrote:


This patch provides an implementation of Nuclei ECLIC Device.
Nuclei processor core have been equipped with an Enhanced Core Local
Interrupt Controller (ECLIC), which is optimized based on the RISC-V
standard CLIC, to manage all interrupt sources.

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


Hello,

There are patches on the QEMU list adding support for the CLIC. How
different is the ECLIC from the CLIC? Could you use the CLIC as a
starting point instead of implementing a new interrupt controller?



That's my thought too when I saw this patch at first.

A better way is to scandalize the CLIC support in QEMU first, then we


Sorry for the typo. I meant to say: standardize the CLIC support


will see how Nuclei's eCLIC could be built on top of that. Thanks!


Regards,
Bin



I agree with both of you.the CLIC support in QEMU first. I read the 
patch of clic, and there is no problem with compatibility in the 
target/riscv directory.I will split eclic in next version. Thanks


Regards
Wang Junqiang




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

2021-05-10 Thread Wang Junqiang
he 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