Re: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules

[Marc Zyngier <maz@kernel.org>]

On Tue, 13 Aug 2024 11:33:47 +0100,
Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> On Tue, Aug 13, 2024 at 09:58:34AM +0100, Marc Zyngier wrote:
> > No. This is the wrong approach, and leads to inconsistent behaviour if
> > we ever change this MAX_IPI value. It also breaks 32 bit builds, and
> > makes things completely inconsistent between ACPI and DT.
> >
> > I don't know how the FFA code was tested, because I cannot see how it
> > can work.
> >
> > *IF* we are going to allow random SGIs being requested by random
> > drivers, we need to be able to do it properly. Not as a side hack like
> > this.
> 
> I am open for any ideas as FF-A spec authors/architects decided to allow
> secure world to donate one of its SGI to the normal world for FF-A
> notifications.

Let's first try to answer a simple question: how is that going to work
for interrupt architectures that do not have the concept of SGIs, but
rely on normal interrupts (similar to SPIs or LPIs) for their IPIs?

They don't have a global interrupt number per CPU for their IPIs, and
may not even have the concept of a shared numbering space between
security domains.

This makes the whole concept of "delegating" an interrupt number from
secure to non-secure a dead-end. Should we build a SW ecosystem on that?

The other thing is: if FFA is exposing interrupts to be signalled from
secure to non-secure, and that it insists on using SGIs, why isn't
that described in DT/ACPI, with a reservation mechanism that would
allow the GIC driver to reserve the corresponding SGI and not dish it
out as a normal mechanism?

Because this sort of thing

+       if (acpi_disabled) {
+               struct of_phandle_args oirq = {};
+               struct device_node *gic;
+
+               /* Only GICv3 supported currently with the device tree */
+               gic = of_find_compatible_node(NULL, NULL, "arm,gic-v3");
+               if (!gic)
+                       return -ENXIO;
+
+               oirq.np = gic;
+               oirq.args_count = 1;
+               oirq.args[0] = sr_intid;
+               irq = irq_create_of_mapping(&oirq);
+               of_node_put(gic);
+#ifdef CONFIG_ACPI
+       } else {
+               irq = acpi_register_gsi(NULL, sr_intid, ACPI_EDGE_SENSITIVE,
+                                       ACPI_ACTIVE_HIGH);
+#endif
+       }

is an absolute howler. It is abusing the arch-private interface, is at
the mercy of buggy EL3 returning stupid values, and may tramp over the
kernel's own IPI allocation.

All these problems need to be addressed.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.