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

From: Valentin Schneider
Date: Thu May 21 2020 - 10:05:00 EST



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@xxxxxxxxxx>
> ---
> 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.

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.

> + 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.

> + break;
> +
> case PPI_RANGE:
> case EPPI_RANGE:
> irq_set_percpu_devid(irq);