Re: [PATCH 06/11] irqchip/gic-v3: Configure SGIs as standard interrupts

Marc Zyngier
2020-06-12T10:39:08Z

Hi Valentin,

On Thu, 21 May 2020 15:04:54 +0100
Valentin Schneider <valentin.schneider@arm.com> wrote:

> On 19/05/20 17:17, Marc Zyngier wrote:
> > Change the way we deal with GICv3 SGIs by turning them into proper
> > IRQs, and calling into the arch code to register the interrupt range
> > instead of a callback.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > drivers/irqchip/irq-gic-v3.c | 91 +++++++++++++++++++++---------------
> > 1 file changed, 53 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 23d7c87da407..d57289057b75 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -1163,10 +1142,36 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
> >
> > static void gic_smp_init(void)
> > {
> > - set_smp_cross_call(gic_raise_softirq);
> > + struct irq_fwspec sgi_fwspec = {
> > + .fwnode = gic_data.fwnode,
> > + };
> > + int base_sgi;
> > +
> > cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
> > "irqchip/arm/gicv3:starting",
> > gic_starting_cpu, NULL);
> > +
> > + if (is_of_node(gic_data.fwnode)) {
> > + /* DT */
> > + sgi_fwspec.param_count = 3;
> > + sgi_fwspec.param[0] = GIC_IRQ_TYPE_SGI;
> > + sgi_fwspec.param[1] = 0;
> > + sgi_fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
> > + } else {
> > + /* ACPI */
> > + sgi_fwspec.param_count = 2;
> > + sgi_fwspec.param[0] = 0;
> > + sgi_fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> > + }
> > +
> > + /* Register all 8 non-secure SGIs */
> > + base_sgi = __irq_domain_alloc_irqs(gic_data.domain, -1, 8,
> > + NUMA_NO_NODE, &sgi_fwspec,
> > + false, NULL);
>
> So IIUC using irq_reserve_ipi() would require us to have a separate IPI
> domain, so instead here we can use a fwspec + the 'regular' GIC domain.

Indeed. Using an IPI domain wouldn't bring much. But the major point
against the current state of the IPI domain is that it sucks a bit for
our use case. We want interrupts to be contiguous in the Linux IRQ
space, and the IPI allocator prevents this.

But maybe I should just bite the bullet and hack that as well.

> One thing I see is that by not going through irq_reserve_ipi(), we don't set
> data->common->ipi_offset. I think this is all kzalloc'd, and we want an
> offset of 0 so it all works out, but this feels somewhat fragile.

So far, nothing is using this field on the limited piece of code we
use. But I agree, not the nicest behaviour.

> > + if (WARN_ON(base_sgi <= 0))
> > + return;
> > +
> > + set_smp_ipi_range(base_sgi, 8);
> > }
> >
> > static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> > @@ -1289,6 +1296,13 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
> >
> > switch (__get_intid_range(hw)) {
> > case SGI_RANGE:
> > + irq_set_percpu_devid(irq);
> > + irq_domain_set_info(d, irq, hw, chip, d->host_data,
> > + handle_percpu_devid_fasteoi_ipi,
> > + NULL, NULL);
> > + irq_set_status_flags(irq, IRQ_NOAUTOEN);
>
> FWIW IRQ_NOAUTOEN is already set by irq_set_percpu_devid_flags(), so that's
> not required. I know we do that for (E)PPIs, I think I already have a small
> patch stashed somewhere regarding that.

Already merged! ;-)

Thanks,

M.
--
Jazz is not dead. It just smells funny...