Re: [PATCH v5 02/16] irqchip/riscv-intc: Allow large non-standard interrupt number

[Anup Patel]

On Wed, Dec 13, 2023 at 7:58 PM Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Dec 13, 2023 at 12:34 PM Yu Chien Peter Lin
> <peterlin@xxxxxxxxxxxxx> wrote:
> >
> > Currently, the implementation of the RISC-V INTC driver uses the
> > interrupt cause as hardware interrupt number and has a limitation of
> > supporting a maximum of 64 interrupts. However, according to the
> > privileged spec, interrupt causes >= 16 are defined for platform use.
>
> I disagree with this patch.
>
> Even though RISC-V priv sepc allows interrupt causes >= 16, we
> still need CSRs to manage arbitrary local interrupts
>
> Currently, we have following standard CSRs:
> 1) [m|s]ie and [m|s]ip which are XLEN wide
> 2) With AIA, we have [m|s]ieh and [m|s]iph for RV32
>
> Clearly, we can only have a XLEN number of standard local
> interrupts without AIA and 64 local interrupts with AIA.
>
> Now for implementations with custom CSRs (such as Andes),
> we still can't assume infinite local interrupts because HW will
> have a finite number of custom CSRs.
>
> >
> > This limitation prevents to fully utilize the available local interrupt
> > sources. Additionally, the interrupt number used on RISC-V are sparse,
> > with only interrupt numbers 1, 5 and 9 (plus Sscofpmf or T-Head's PMU
> > interrupt) being currently used for supervisor mode.
> >
> > Switch to using irq_domain_create_tree() to create the radix tree
> > map, so a larger number of hardware interrupts can be handled.
> >
> > Signed-off-by: Yu Chien Peter Lin <peterlin@xxxxxxxxxxxxx>
> > Reviewed-by: Charles Ci-Jyun Wu <dminus@xxxxxxxxxxxxx>
> > Reviewed-by: Leo Yu-Chi Liang <ycliang@xxxxxxxxxxxxx>
> > ---
> > Changes v1 -> v2:
> >   - Fixed irq mapping failure checking (suggested by Clément and Anup)
> > Changes v2 -> v3:
> >   - No change
> > Changes v3 -> v4: (Suggested by Thomas [1])
> >   - Use pr_warn_ratelimited instead
> >   - Fix coding style and commit message
> > Changes v4 -> v5: (Suggested by Thomas)
> >   - Fix commit message
> >
> > [1] https://patchwork.kernel.org/project/linux-riscv/patch/20231023004100.2663486-3-peterlin@xxxxxxxxxxxxx/#25573085
> > ---
> >  drivers/irqchip/irq-riscv-intc.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> > index e8d01b14ccdd..2fdd40f2a791 100644
> > --- a/drivers/irqchip/irq-riscv-intc.c
> > +++ b/drivers/irqchip/irq-riscv-intc.c
> > @@ -24,10 +24,9 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
> >  {
> >         unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG;
> >
> > -       if (unlikely(cause >= BITS_PER_LONG))
> > -               panic("unexpected interrupt cause");
> > -
> > -       generic_handle_domain_irq(intc_domain, cause);
> > +       if (generic_handle_domain_irq(intc_domain, cause))
> > +               pr_warn_ratelimited("Failed to handle interrupt (cause: %ld)\n",
> > +                                   cause);
> >  }
> >
> >  /*
> > @@ -117,8 +116,7 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
> >  {
> >         int rc;
> >
> > -       intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
> > -                                              &riscv_intc_domain_ops, NULL);
> > +       intc_domain = irq_domain_create_tree(fn, &riscv_intc_domain_ops, NULL);
>
> I disagree with this change based on the reasoning above.
>
> Instead of this, we should determine the number of local interrupts
> based on the type of RISC-V intc:
> 1) For standard INTC without AIA, we have XLEN (or BITS_PER_LONG)
>     local interrupts
> 2) For standart INTC with AIA, we have 64 local interrupts
> 3) For custom INTC (such as Andes), the number of local interrupt
>     should be custom (Andes specific) which can be determined based
>     on compatible string.
>
> Also, creating a linear domain with a fixed number of local interrupts
> ensures that drivers can't map a local interrupt beyond the availability
> of CSRs to manage it.

Thinking about this more. We do have a problem because Andes local
interrupts are really sparse which is not the case for standard local
interrupts

I have an alternate suggestion which goes as follows ...

We use irq_domain_create_tree() in-place of irq_domain_create_linear()
and enforce checks on hwirq in riscv_intc_domain_alloc() to ensure that
we only allow hwirq for which we have corresponding standard or custom
CSR.

To achieve this, riscv_intc_init_common() will have to save the following
as static global variables:
1) riscv_intc_nr_irqs: Number of standard local interrupts
2) riscv_intc_custom_base and riscv_intc_custom_nr_irqs: Base and
    number of custom local interrupts.

Using the above static global variables, the riscv_intc_domain_alloc()
can return error if one of the following conditions are met:
1) riscv_intc_nr_irqs<= hwirq && hwirq < riscv_intc_custom_base
2) (riscv_intc_custom_base + riscv_intc_custom_nr_irqs) <= hwirq

For standard INTC, we can set the static global variable as follows:
riscv_intc_nr_irqs = XLEN or BITS_PER_LONG
riscv_intc_custom_base = riscv_intc_nr_irqs
riscv_intc_custom_nr_irqs = 0

For Andes INTC, we can set the static global variables as follows:
riscv_intc_nr_irqs = XLEN or BITS_PER_LONG
riscv_intc_custom_base = 256
riscv_intc_custom_nr_irqs = XLEN or BITS_PER_LONG

Regards,
Anup

>
> >         if (!intc_domain) {
> >                 pr_err("unable to add IRQ domain\n");
> >                 return -ENXIO;
> > @@ -132,8 +130,6 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
> >
> >         riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
> >
> > -       pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
> > -
>
> Same as above, we should definitely advertise the type of INTC and
> number of local interrupts mapped.
>
> Regards,
> Anup
>
> >         return 0;
> >  }
> >
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-riscv