Re: [PATCH v2 5/7] irqchip/gic-v5: Add ACPI IRS probing

Lorenzo Pieralisi
2026-01-08T16:22:34Z

On Mon, Jan 05, 2026 at 01:24:21PM +0000, Jonathan Cameron wrote:
> On Thu, 18 Dec 2025 11:14:31 +0100
> Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
>
> > On ARM64 ACPI systems GICv5 IRSes are described in MADT sub-entries.
> >
> > Add the required plumbing to parse MADT IRS firmware table entries and
> > probe the IRS components in ACPI.
> >
> > Refactor the OF based probe so that common code paths can be reused for
> > ACPI as well in the process.
> >
> > Augment the irqdomain_ops.translate() for PPI and SPI IRQs in order to
> > provide support for their ACPI based firmware translation.
> >
> > Implement an irqchip ACPI based callback to initialize the global GSI
> > domain upon an MADT IRS detection.
> >
> > The IRQCHIP_ACPI_DECLARE() entry in the top level GICv5 driver is only used
> > to trigger the IRS probing (ie the global GSI domain is initialized once on
> > the first call on multi-IRS systems); IRS probing takes place by calling
> > acpi_table_parse_madt() in the IRS sub-driver, that probes all IRSes
> > in sequence.
> >
> > Add a new ACPI interrupt model so that it can be detected at runtime and
> > distinguished from previous GIC architecture models.
> >
> > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> Just a few minor comments inline.
>
> > diff --git a/drivers/irqchip/irq-gic-v5-irs.c b/drivers/irqchip/irq-gic-v5-irs.c
> > index ce2732d649a3..07c3df5692af 100644
> > --- a/drivers/irqchip/irq-gic-v5-irs.c
> > +++ b/drivers/irqchip/irq-gic-v5-irs.c
> > @@ -5,6 +5,7 @@
> >
> > #define pr_fmt(fmt) "GICv5 IRS: " fmt
> >
> > +#include <linux/acpi.h>
> > #include <linux/kmemleak.h>
> > #include <linux/log2.h>
> > #include <linux/of.h>
> > @@ -545,15 +546,15 @@ int gicv5_irs_register_cpu(int cpuid)
> >
> > static void __init gicv5_irs_init_bases(struct gicv5_irs_chip_data *irs_data,
> > void __iomem *irs_base,
> > - struct fwnode_handle *handle)
> > + struct fwnode_handle *handle,
> > + bool noncoherent)
> > {
> > - struct device_node *np = to_of_node(handle);
> > u32 cr0, cr1;
> >
> > irs_data->fwnode = handle;
>
> Looking this again as you are touching it.
>
> This feels a tiny bit out of place, as not obvious to me why the fwnode being
> set is related to initializing bases. Perhaps that belongs at the caller?

Done.

> > irs_data->irs_base = irs_base;
> >
> > - if (of_property_read_bool(np, "dma-noncoherent")) {
> > + if (noncoherent) {
> > /*
> > * A non-coherent IRS implies that some cache levels cannot be
> > * used coherently by the cores and GIC. Our only option is to mark
> > @@ -678,49 +679,13 @@ static void irs_setup_pri_bits(u32 idr1)
> > }
> > }
> >
>
> ...
>
> > +static int __init gicv5_irs_of_init(struct device_node *node)
> > +{
>
> Not sure whether it would be worth it by inclination would have been
> a noop refactor patch, then the ACPI support. I'd have found it a little
> bit easier to review.

Split it into two patches.

> > + struct gicv5_irs_chip_data *irs_data;
> > + void __iomem *irs_base;
> > + u8 iaffid_bits;
> > + u32 idr;
> > + int ret;
> > +
> > + irs_data = kzalloc(sizeof(*irs_data), GFP_KERNEL);
> > + if (!irs_data)
> > + return -ENOMEM;
> > +
> > + raw_spin_lock_init(&irs_data->spi_config_lock);
> > +
> > + ret = of_property_match_string(node, "reg-names", "ns-config");
> > + if (ret < 0) {
> > + pr_err("%pOF: ns-config reg-name not present\n", node);
> > + goto out_err;
>
> Obviously comes form original code, but this label could give some indication
> of where we are going and why. out_free_data maybe?

I could probably change it (or better sneak it in) in this set, though
as you said it could well be a standalone change.

> Perhaps I'm just being fussy as I had to open the code up to check given
> it didn't end up in the context.
>
> > + }
> > +
> ...
>
> > +static int __init gic_acpi_parse_iaffid(union acpi_subtable_headers *header,
> > + const unsigned long end)
> > +{
> > + struct acpi_madt_generic_interrupt *gicc = (struct acpi_madt_generic_interrupt *)header;
> > + int cpu;
> > +
> > + if (!(gicc->flags & (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE)))
> > + return 0;
> > +
> > + if (gicc->irs_id == current_irsid) {
> Unless this is going to get more complex I'd flip the log for an early return on
> 'not this one'
> if (gicc->irs_id != current_irsid)
> return 0;
>
> mostly to save on long lines where they aren't needed.

Done.

> > + cpu = get_logical_index(gicc->arm_mpidr);
> > +
> > + if (gicc->iaffid & ~GENMASK(current_iaffid_bits - 1, 0)) {
> > + pr_warn("CPU %d iaffid 0x%x exceeds IRS iaffid bits\n", cpu, gicc->iaffid);
> > + return 0;
> > + }
> > +
> > + // Bind the IAFFID and the CPU
> > + per_cpu(cpu_iaffid, cpu).iaffid = gicc->iaffid;
> > + per_cpu(cpu_iaffid, cpu).valid = true;
> > + pr_debug("Processed IAFFID %u for CPU%d", per_cpu(cpu_iaffid, cpu).iaffid, cpu);
> > +
> > + // We also know that the CPU is connected to this IRS
> > + per_cpu(per_cpu_irs_data, cpu) = current_irs_data;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int __init gicv5_irs_acpi_init_affinity(u32 irsid, struct gicv5_irs_chip_data *irs_data)
> > +{
> > + u32 idr;
> > +
> > + current_irsid = irsid;
> > + current_irs_data = irs_data;
> > +
> > + idr = irs_readl_relaxed(irs_data, GICV5_IRS_IDR1);
> > + current_iaffid_bits = FIELD_GET(GICV5_IRS_IDR1_IAFFID_BITS, idr) + 1;
> > +
> > + acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, gic_acpi_parse_iaffid, 0);
> > +
> > + return 0;
> > +}
>
> > +
> > +static int __init gic_acpi_parse_madt_irs(union acpi_subtable_headers *header,
> > + const unsigned long end)
> > +{
> > + struct acpi_madt_gicv5_irs *irs = (struct acpi_madt_gicv5_irs *)header;
> > + struct gicv5_irs_chip_data *irs_data;
> > + void __iomem *irs_base;
> > + struct resource *r;
> > + int ret;
> > +
> > + // Per-IRS data structure
>
> It's your code to look after, so I don't care that strongly but this does
> seem to be inconsistent wrt to local style and use of C style
> single line comments. I'd stick to /* */ throughout.

+1

> > + irs_data = kzalloc(sizeof(*irs_data), GFP_KERNEL);
> > + if (!irs_data)
> > + return -ENOMEM;
> > +
> > + // This spinlock is used for SPI config changes
> > + raw_spin_lock_init(&irs_data->spi_config_lock);
> > +
> > + irs_base = ioremap(irs->config_base_address, ACPI_GICV5_IRS_MEM_SIZE);
> > + if (!irs_base) {
> > + pr_err("Unable to map GIC IRS registers\n");
> > + ret = -ENOMEM;
> > + goto out_free;
> > + }
> > +
> > + r = gic_request_region(irs->config_base_address, ACPI_GICV5_IRS_MEM_SIZE, "GICv5 IRS");
>
> Really minor but I wonder if this should follow the same ordering as
> of_io_request_and_map() which does the request_mem_region() before
> trying to ioremap()?

That would make sense, yes.

> Nice to have the same ordering though that would then put you at odds iwth the
> gicv3 redist code that does it in this order. Guess can't win them all and
> an ephemeral mapping that is then torn down on error of the second call
> doesn't really matter.
>
> > + if (!r) {
> > + ret = -EBUSY;
> > + goto out_map;
> > + }
> > +
> > + gicv5_irs_init_bases(irs_data, irs_base, NULL, irs->flags & ACPI_MADT_IRS_NON_COHERENT);
> > +
> > + gicv5_irs_acpi_init_affinity(irs->irs_id, irs_data);
> > +
> > + ret = gicv5_irs_init(irs_data);
> > + if (ret)
> > + goto out_release;
> > +
> > + if (irs_data->spi_range) {
> > + pr_info("%s @%llx detected SPI range [%u-%u]\n", "IRS", irs->config_base_address,
> > + irs_data->spi_min,
> > + irs_data->spi_min +
> > + irs_data->spi_range - 1);
> > + }
> > +
> > + return 0;
> > +
> > +out_release:
> > + release_mem_region(r->start, resource_size(r));
> > +out_map:
> > + iounmap(irs_base);
> > +out_free:
> > + kfree(irs_data);
> > + return ret;
> > +}
>
> > diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c
> > index 41ef286c4d78..23fd551c4347 100644
> > --- a/drivers/irqchip/irq-gic-v5.c
> > +++ b/drivers/irqchip/irq-gic-v5.c
> > @@ -579,16 +579,36 @@ static __always_inline int gicv5_irq_domain_translate(struct irq_domain *d,
> > unsigned int *type,
> > const u8 hwirq_type)
> > {
> > - if (!is_of_node(fwspec->fwnode))
> > - return -EINVAL;
> > + unsigned int hwirq_trigger;
> > + u8 fwspec_irq_type;
> >
> > - if (fwspec->param_count < 3)
> > - return -EINVAL;
> > + if (is_of_node(fwspec->fwnode)) {
> >
> > - if (fwspec->param[0] != hwirq_type)
> > - return -EINVAL;
> > + if (fwspec->param_count < 3)
> > + return -EINVAL;
> >
> > - *hwirq = fwspec->param[1];
> > + fwspec_irq_type = fwspec->param[0];
> > +
> > + if (fwspec->param[0] != hwirq_type)
> > + return -EINVAL;
> > +
> > + *hwirq = fwspec->param[1];
> > + hwirq_trigger = fwspec->param[2];
> > + }
> > +
> > + if (is_fwnode_irqchip(fwspec->fwnode)) {
> > +
> > + if (fwspec->param_count != 2)
> > + return -EINVAL;
> > +
> > + fwspec_irq_type = FIELD_GET(GICV5_HWIRQ_TYPE, fwspec->param[0]);
> > +
> > + if (fwspec_irq_type != hwirq_type)
> > + return -EINVAL;
> > +
> > + *hwirq = FIELD_GET(GICV5_HWIRQ_ID, fwspec->param[0]);
> > + hwirq_trigger = fwspec->param[1];
> > + }
> >
> > switch (hwirq_type) {
> > case GICV5_HWIRQ_TYPE_PPI:
> > @@ -600,7 +620,7 @@ static __always_inline int gicv5_irq_domain_translate(struct irq_domain *d,
> > IRQ_TYPE_EDGE_RISING;
> > break;
> > case GICV5_HWIRQ_TYPE_SPI:
> > - *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> > + *type = hwirq_trigger & IRQ_TYPE_SENSE_MASK;
> > break;
> > default:
> > BUILD_BUG_ON(1);
> > @@ -660,10 +680,18 @@ static void gicv5_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> > static int gicv5_irq_ppi_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec,
> > enum irq_domain_bus_token bus_token)
> > {
> > + u32 hwirq_type;
> > +
> > if (fwspec->fwnode != d->fwnode)
> > return 0;
> >
> > - if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI)
> > + if (is_of_node(fwspec->fwnode))
> > + hwirq_type = fwspec->param[0];
> > +
> > + if (is_fwnode_irqchip(fwspec->fwnode))
> > + hwirq_type = FIELD_GET(GICV5_HWIRQ_TYPE, fwspec->param[0]);
>
> Bit borderline but maybe worth a helper for getting the irq type from
> the fwspec? Or a more generic helper that optionally gets that and the other
> stuff you need in gicv5_irq_domain_translate()

I will think about this one, first let's make sure the series is ready.

Lorenzo

> > +
> > + if (hwirq_type != GICV5_HWIRQ_TYPE_PPI)
> > return 0;
> >
> > return (d == gicv5_global_data.ppi_domain);
> > @@ -718,10 +746,18 @@ static int gicv5_irq_spi_domain_alloc(struct irq_domain *domain, unsigned int vi
> > static int gicv5_irq_spi_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec,
> > enum irq_domain_bus_token bus_token)
> > {
> > + u32 hwirq_type;
> > +
> > if (fwspec->fwnode != d->fwnode)
> > return 0;
> >
> > - if (fwspec->param[0] != GICV5_HWIRQ_TYPE_SPI)
> > + if (is_of_node(fwspec->fwnode))
> > + hwirq_type = fwspec->param[0];
> > +
> > + if (is_fwnode_irqchip(fwspec->fwnode))
> > + hwirq_type = FIELD_GET(GICV5_HWIRQ_TYPE, fwspec->param[0]);
> > +
> > + if (hwirq_type != GICV5_HWIRQ_TYPE_SPI)
> > return 0;
> >
> > return (d == gicv5_global_data.spi_domain);
> > @@ -1082,16 +1118,12 @@ static inline void __init gic_of_setup_kvm_info(struct device_node *node)
> > }
> > #endif // CONFIG_KVM
>
> Thanks,
>
> Jonathan
>