Re: [PATCH v3 1/4] target/riscv: Add Smrnmi cpu extension.

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 5/22/23 10:11, Tommy Wu wrote:
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> ---
>   hw/riscv/riscv_hart.c         | 21 +++++++++++++++++++++
>   include/hw/riscv/riscv_hart.h |  4 ++++
>   target/riscv/cpu.c            | 14 ++++++++++++++
>   target/riscv/cpu.h            |  7 +++++++
>   target/riscv/cpu_bits.h       | 12 ++++++++++++
>   target/riscv/cpu_helper.c     | 24 ++++++++++++++++++++++++
>   6 files changed, 82 insertions(+)
> 
> diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
> index 613ea2aaa0..eac18f8c29 100644
> --- a/hw/riscv/riscv_hart.c
> +++ b/hw/riscv/riscv_hart.c
> @@ -33,6 +33,12 @@ static Property riscv_harts_props[] = {
>       DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
>       DEFINE_PROP_UINT64("resetvec", RISCVHartArrayState, resetvec,
>                          DEFAULT_RSTVEC),
> +    DEFINE_PROP_ARRAY("rnmi-interrupt-vector", RISCVHartArrayState,
> +                      num_rnmi_irqvec, rnmi_irqvec, qdev_prop_uint64,
> +                      uint64_t),
> +    DEFINE_PROP_ARRAY("rnmi-exception-vector", RISCVHartArrayState,
> +                      num_rnmi_excpvec, rnmi_excpvec, qdev_prop_uint64,
> +                      uint64_t),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> @@ -47,6 +53,21 @@ static bool riscv_hart_realize(RISCVHartArrayState *s, int idx,
>   {
>       object_initialize_child(OBJECT(s), "harts[*]", &s->harts[idx], cpu_type);
>       qdev_prop_set_uint64(DEVICE(&s->harts[idx]), "resetvec", s->resetvec);
> +
> +    if (s->harts[idx].cfg.ext_smrnmi) {
> +        if (s->rnmi_irqvec) {
> +            qdev_prop_set_uint64(DEVICE(&s->harts[idx]),
> +                                 "rnmi-interrupt-vector",
> +                                 s->rnmi_irqvec[idx]);
> +        }
> +
> +        if (s->rnmi_excpvec) {
> +            qdev_prop_set_uint64(DEVICE(&s->harts[idx]),
> +                                 "rnmi-exception-vector",
> +                                 s->rnmi_excpvec[idx]);
> +        }
> +    }
> +
>       s->harts[idx].env.mhartid = s->hartid_base + idx;
>       qemu_register_reset(riscv_harts_cpu_reset, &s->harts[idx]);
>       return qdev_realize(DEVICE(&s->harts[idx]), NULL, errp);
> diff --git a/include/hw/riscv/riscv_hart.h b/include/hw/riscv/riscv_hart.h
> index bbc21cdc9a..99c0ac5009 100644
> --- a/include/hw/riscv/riscv_hart.h
> +++ b/include/hw/riscv/riscv_hart.h
> @@ -38,6 +38,10 @@ struct RISCVHartArrayState {
>       uint32_t hartid_base;
>       char *cpu_type;
>       uint64_t resetvec;
> +    uint32_t num_rnmi_irqvec;
> +    uint64_t *rnmi_irqvec;
> +    uint32_t num_rnmi_excpvec;
> +    uint64_t *rnmi_excpvec;
>       RISCVCPU *harts;
>   };
>   
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index db0875fb43..39b74569b1 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -119,6 +119,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
>       ISA_EXT_DATA_ENTRY(zhinx, PRIV_VERSION_1_12_0, ext_zhinx),
>       ISA_EXT_DATA_ENTRY(zhinxmin, PRIV_VERSION_1_12_0, ext_zhinxmin),
>       ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
> +    ISA_EXT_DATA_ENTRY(smrnmi, PRIV_VERSION_1_12_0, ext_smrnmi),
>       ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
>       ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
>       ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> @@ -1404,6 +1405,12 @@ static void riscv_cpu_set_irq(void *opaque, int irq, int level)
>           g_assert_not_reached();
>       }
>   }
> +
> +static void riscv_cpu_set_nmi(void *opaque, int irq, int level)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(opaque);
> +    riscv_cpu_set_rnmi(cpu, irq, level);

Minor commennt/nit: you can do:

> +    riscv_cpu_set_rnmi(RISCV_CPU(opaque), irq, level);

And avoid the extra 'cpu' pointer.


> +}
>   #endif /* CONFIG_USER_ONLY */
>   
>   static void riscv_cpu_init(Object *obj)
> @@ -1420,6 +1427,8 @@ static void riscv_cpu_init(Object *obj)
>   #ifndef CONFIG_USER_ONLY
>       qdev_init_gpio_in(DEVICE(cpu), riscv_cpu_set_irq,
>                         IRQ_LOCAL_MAX + IRQ_LOCAL_GUEST_MAX);
> +    qdev_init_gpio_in_named(DEVICE(cpu), riscv_cpu_set_nmi,
> +                            "riscv.cpu.rnmi", RNMI_MAX);
>   #endif /* CONFIG_USER_ONLY */
>   }
>   
> @@ -1600,6 +1609,7 @@ static Property riscv_cpu_extensions[] = {
>       DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
>       DEFINE_PROP_BOOL("x-smaia", RISCVCPU, cfg.ext_smaia, false),
>       DEFINE_PROP_BOOL("x-ssaia", RISCVCPU, cfg.ext_ssaia, false),
> +    DEFINE_PROP_BOOL("x-smrnmi", RISCVCPU, cfg.ext_smrnmi, false),
>   
>       DEFINE_PROP_BOOL("x-zvfh", RISCVCPU, cfg.ext_zvfh, false),
>       DEFINE_PROP_BOOL("x-zvfhmin", RISCVCPU, cfg.ext_zvfhmin, false),
> @@ -1644,6 +1654,10 @@ static Property riscv_cpu_properties[] = {
>   
>       DEFINE_PROP_BOOL("rvv_ta_all_1s", RISCVCPU, cfg.rvv_ta_all_1s, false),
>       DEFINE_PROP_BOOL("rvv_ma_all_1s", RISCVCPU, cfg.rvv_ma_all_1s, false),
> +    DEFINE_PROP_UINT64("rnmi-interrupt-vector", RISCVCPU, env.rnmi_irqvec,
> +                       DEFAULT_RNMI_IRQVEC),
> +    DEFINE_PROP_UINT64("rnmi-exception-vector", RISCVCPU, env.rnmi_excpvec,
> +                       DEFAULT_RNMI_EXCPVEC),
>   
>       /*
>        * write_misa() is marked as experimental for now so mark
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index de7e43126a..6c14b93cb5 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -366,6 +366,11 @@ struct CPUArchState {
>       uint64_t kvm_timer_compare;
>       uint64_t kvm_timer_state;
>       uint64_t kvm_timer_frequency;
> +
> +    /* RNMI */
> +    target_ulong rnmip;
> +    uint64_t rnmi_irqvec;
> +    uint64_t rnmi_excpvec;
>   };
>   
>   /*
> @@ -436,6 +441,7 @@ struct RISCVCPUConfig {
>       bool ext_smaia;
>       bool ext_ssaia;
>       bool ext_sscofpmf;
> +    bool ext_smrnmi;
>       bool rvv_ta_all_1s;
>       bool rvv_ma_all_1s;
>   
> @@ -562,6 +568,7 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env);
>   int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint64_t interrupts);
>   uint64_t riscv_cpu_update_mip(CPURISCVState *env, uint64_t mask,
>                                 uint64_t value);
> +void riscv_cpu_set_rnmi(RISCVCPU *cpu, uint32_t irq, bool level);
>   #define BOOL_TO_MASK(x) (-!!(x)) /* helper for riscv_cpu_update_mip value */
>   void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t (*fn)(void *),
>                                void *arg);
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 59f0ffd9e1..7cb43b88f3 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -659,6 +659,12 @@ typedef enum {
>   /* Default Reset Vector adress */
>   #define DEFAULT_RSTVEC      0x1000
>   
> +/* Default RNMI Interrupt Vector address */
> +#define DEFAULT_RNMI_IRQVEC     0x0
> +
> +/* Default RNMI Exception Vector address */
> +#define DEFAULT_RNMI_EXCPVEC    0x0
> +
>   /* Exception causes */
>   typedef enum RISCVException {
>       RISCV_EXCP_NONE = -1, /* sentinel value */
> @@ -705,6 +711,9 @@ typedef enum RISCVException {
>   #define IRQ_LOCAL_MAX                      16
>   #define IRQ_LOCAL_GUEST_MAX                (TARGET_LONG_BITS - 1)
>   
> +/* RNMI causes */
> +#define RNMI_MAX                           16
> +
>   /* mip masks */
>   #define MIP_USIP                           (1 << IRQ_U_SOFT)
>   #define MIP_SSIP                           (1 << IRQ_S_SOFT)
> @@ -896,6 +905,9 @@ typedef enum RISCVException {
>   #define MHPMEVENT_IDX_MASK                 0xFFFFF
>   #define MHPMEVENT_SSCOF_RESVD              16
>   
> +/* RISC-V-specific interrupt pending bits. */
> +#define CPU_INTERRUPT_RNMI                 CPU_INTERRUPT_TGT_EXT_0
> +
>   /* JVT CSR bits */
>   #define JVT_MODE                           0x3F
>   #define JVT_BASE                           (~0x3F)
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 57d04385f1..cc7898f103 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -635,6 +635,30 @@ uint64_t riscv_cpu_update_mip(CPURISCVState *env, uint64_t mask,
>       return old;
>   }
>   
> +void riscv_cpu_set_rnmi(RISCVCPU *cpu, uint32_t irq, bool level)
> +{
> +    CPURISCVState *env = &cpu->env;
> +    CPUState *cs = CPU(cpu);
> +    bool locked = false;
> +
> +    if (!qemu_mutex_iothread_locked()) {
> +        locked = true;
> +        qemu_mutex_lock_iothread();
> +    }
> +
> +    if (level) {
> +        env->rnmip |= 1 << irq;
> +        cpu_interrupt(cs, CPU_INTERRUPT_RNMI);
> +    } else {
> +        env->rnmip &= ~(1 << irq);
> +        cpu_reset_interrupt(cs, CPU_INTERRUPT_RNMI);
> +    }
> +
> +    if (locked) {
> +        qemu_mutex_unlock_iothread();
> +    }


'locked' is not a good named for this flag because you guaranteed that the iothread
will always be locked at this point. Questions is whether you locked it yourself,
and then you need to unlock it, or if it was locked beforehand.

I suggest renaming 'locked' to 'release_lock' for clarity. Asumming you agree:


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>





> +}
> +
>   void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t (*fn)(void *),
>                                void *arg)
>   {