[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: |
Alistair Francis |
Subject: |
Re: [RFC PATCH 1/5] target/riscv: Add Nuclei CSR and Update interrupt handling |
Date: |
Tue, 11 May 2021 14:03:31 +1000 |
On Tue, May 11, 2021 at 2:01 PM Wang Junqiang <wangjunqiang@iscas.ac.cn> wrote:
>
>
>
> On 2021/5/11 上午11:43, Alistair Francis wrote:
> > On Tue, May 11, 2021 at 1:14 PM Wang Junqiang <wangjunqiang@iscas.ac.cn>
> > wrote:
> >>
> >>
> >>
> >> 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.
> >
> > 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
> >
>
> OK, I will try to do it and separate the custom CSR of nuclei into the
> new file.
Great! Feel free to reach out if you would like help.
Alistair
>
> >>
> >> Regards
> >> Wang Junqiang
> >>
> >
>
>
[RFC PATCH 3/5] hw/intc: Add Nuclei Systimer, wangjunqiang, 2021/05/07
[RFC PATCH 2/5] hw/intc: Add Nuclei ECLIC device, wangjunqiang, 2021/05/07
[RFC PATCH 4/5] hw/char: Add Nuclei Uart, wangjunqiang, 2021/05/07
[RFC PATCH 5/5] Nuclei FPGA Evaluation Kit MCU Machine, wangjunqiang, 2021/05/07
Re: [RFC PATCH 0/5] RISC-V:support Nuclei FPGA Evaluation Kit, no-reply, 2021/05/07