From: Wang Junqiang <wangjunqiang@iscas.ac.cn>
To: Alistair Francis <alistair23@gmail.com>
Cc: liweiwei@iscas.ac.cn, "open list:RISC-V" <qemu-riscv@nongnu.org>,
Bin Meng <bin.meng@windriver.com>,
"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
Alistair Francis <Alistair.Francis@wdc.com>,
alapha23@gmail.com, Palmer Dabbelt <palmer@dabbelt.com>
Subject: Re: [RFC PATCH 1/5] target/riscv: Add Nuclei CSR and Update interrupt handling
Date: Tue, 11 May 2021 11:14:17 +0800 [thread overview]
Message-ID: <e97fecab-78d2-1a55-c7ad-c542ab5d1fbf@iscas.ac.cn> (raw)
In-Reply-To: <CAKmqyKPE8O6LbZQc2H+kkWvVqf9qW705S85XGGWpUtQTWjzE8Q@mail.gmail.com>
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
WARNING: multiple messages have this Message-ID (diff)
From: Wang Junqiang <wangjunqiang@iscas.ac.cn>
To: Alistair Francis <alistair23@gmail.com>
Cc: "open list:RISC-V" <qemu-riscv@nongnu.org>,
"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
liweiwei@iscas.ac.cn, Bin Meng <bin.meng@windriver.com>,
Alistair Francis <Alistair.Francis@wdc.com>,
alapha23@gmail.com, Palmer Dabbelt <palmer@dabbelt.com>
Subject: Re: [RFC PATCH 1/5] target/riscv: Add Nuclei CSR and Update interrupt handling
Date: Tue, 11 May 2021 11:14:17 +0800 [thread overview]
Message-ID: <e97fecab-78d2-1a55-c7ad-c542ab5d1fbf@iscas.ac.cn> (raw)
In-Reply-To: <CAKmqyKPE8O6LbZQc2H+kkWvVqf9qW705S85XGGWpUtQTWjzE8Q@mail.gmail.com>
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
next prev parent reply other threads:[~2021-05-11 3:15 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-07 8:16 [RFC PATCH 0/5] RISC-V:support Nuclei FPGA Evaluation Kit wangjunqiang
2021-05-07 8:16 ` wangjunqiang
2021-05-07 8:16 ` [RFC PATCH 1/5] target/riscv: Add Nuclei CSR and Update interrupt handling wangjunqiang
2021-05-07 8:16 ` wangjunqiang
2021-05-10 2:17 ` Alistair Francis
2021-05-10 2:17 ` Alistair Francis
2021-05-11 3:14 ` Wang Junqiang [this message]
2021-05-11 3:14 ` Wang Junqiang
2021-05-11 3:43 ` Alistair Francis
2021-05-11 3:43 ` Alistair Francis
2021-05-11 4:00 ` Wang Junqiang
2021-05-11 4:00 ` Wang Junqiang
2021-05-11 4:03 ` Alistair Francis
2021-05-11 4:03 ` Alistair Francis
2021-05-07 8:16 ` [RFC PATCH 2/5] hw/intc: Add Nuclei ECLIC device wangjunqiang
2021-05-07 8:16 ` wangjunqiang
2021-05-10 2:20 ` Alistair Francis
2021-05-10 2:20 ` Alistair Francis
2021-05-10 2:27 ` Bin Meng
2021-05-10 2:27 ` Bin Meng
2021-05-10 5:26 ` Bin Meng
2021-05-10 5:26 ` Bin Meng
2021-05-11 3:18 ` Wang Junqiang
2021-05-07 8:16 ` [RFC PATCH 3/5] hw/intc: Add Nuclei Systimer wangjunqiang
2021-05-07 8:16 ` wangjunqiang
2021-05-07 8:16 ` [RFC PATCH 4/5] hw/char: Add Nuclei Uart wangjunqiang
2021-05-07 8:16 ` wangjunqiang
2021-05-07 8:16 ` [RFC PATCH 5/5] Nuclei FPGA Evaluation Kit MCU Machine wangjunqiang
2021-05-07 8:16 ` wangjunqiang
2021-05-07 13:33 ` [RFC PATCH 0/5] RISC-V:support Nuclei FPGA Evaluation Kit no-reply
2021-05-07 13:33 ` no-reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e97fecab-78d2-1a55-c7ad-c542ab5d1fbf@iscas.ac.cn \
--to=wangjunqiang@iscas.ac.cn \
--cc=Alistair.Francis@wdc.com \
--cc=alapha23@gmail.com \
--cc=alistair23@gmail.com \
--cc=bin.meng@windriver.com \
--cc=liweiwei@iscas.ac.cn \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.