Re: [PATCH 2/2] target/riscv: Move Guest irqs out of the core local irqs range.

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

On 5/13/24 08:46, Rajnesh Kanwal wrote:
> Qemu maps IRQs 0:15 for core interrupts and 16 onward for
> guest interrupts which are later translated to hgiep in
> `riscv_cpu_set_irq()` function.
> 
> With virtual IRQ support added, software now can fully
> use the whole local interrupt range without any actual
> hardware attached.
> 
> This change moves the guest interrupt range after the
> core local interrupt range to avoid clash.
> 
> Fixes: 1697837ed9 ("target/riscv: Add M-mode virtual
> interrupt and IRQ filtering support.")
> Fixes: 40336d5b1d ("target/riscv: Add HS-mode virtual
> interrupt and IRQ filtering support.")
> 

As I said in patch 1, please do not split the commit titles in a
"Fixes" tag:

> Fixes: 1697837ed9 ("target/riscv: Add M-mode virtual interrupt and IRQ filtering support.")
> Fixes: 40336d5b1d ("target/riscv: Add HS-mode virtual interrupt and IRQ filtering support.")


> Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
> ---
>   target/riscv/cpu_bits.h | 3 ++-
>   target/riscv/csr.c      | 7 ++++++-
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 13ce2218d1..33f28bb115 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -664,7 +664,8 @@ typedef enum RISCVException {
>   #define IRQ_M_EXT                          11
>   #define IRQ_S_GEXT                         12
>   #define IRQ_PMU_OVF                        13
> -#define IRQ_LOCAL_MAX                      16
> +#define IRQ_LOCAL_MAX                      64
> +/* -1 is due to bit zero of hgeip and hgeie being ROZ. */
>   #define IRQ_LOCAL_GUEST_MAX                (TARGET_LONG_BITS - 1)
>   
>   /* mip masks */
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index c9d685dcc5..78f42fcae5 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1141,7 +1141,12 @@ static RISCVException write_stimecmph(CPURISCVState *env, int csrno,
>   
>   #define VSTOPI_NUM_SRCS 5
>   
> -#define LOCAL_INTERRUPTS (~0x1FFF)
> +/* All core local interrupts except the fixed ones 0:12. This macro is for virtual
> + * interrupts logic so please don't change this to avoid messing up the whole support,
> + * For reference see AIA spec: `5.3 Interrupt filtering and virtual interrupts for
> + * supervisor level` and `6.3.2 Virtual interrupts for VS level`.
> + */

The comment format we use is capped at 80 chars per line and starts with a
leading /*  on a separated line:


> +/*
> +/* All core local interrupts except the fixed ones 0:12. This macro is
> +/* for virtual interrupts logic so please don't change this to avoid
> +/* messing up the whole support. For reference see AIA spec:
> +/* `5.3 Interrupt filtering and virtual interrupts for supervisor level`
> +/* and `6.3.2 Virtual interrupts for VS level`.
> + */

You can run ./scripts/checkpatch.pl in the generated patch file to see if the
patch is compliant with the expected code style.



Thanks,


Daniel



> +#define LOCAL_INTERRUPTS   (~0x1FFFULL)
>   
>   static const uint64_t delegable_ints =
>       S_MODE_INTERRUPTS | VS_MODE_INTERRUPTS | MIP_LCOFIP;