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

[Valentin Schneider]

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);