| Message ID | 20220224130741.63924-1-marcan@marcan.st |
|---|---|
| Headers | show |
| Series | irqchip/apple-aic: Add support for AICv2 | expand |
On Thu, Feb 24, 2022 at 10:07:34PM +0900, Hector Martin wrote: > Hi folks, Hi Hector, > In the t6000/t6001 (M1 Pro / Max) SoCs, Apple introduced a new version > of their interrupt controller. This is a significant departure from > AICv1 and seems designed to better scale to larger chips. This series > adds support for it to the existing AIC driver. > > Gone are CPU affinities; instead there seems to be some kind of > "automagic" dispatch to willing CPU cores, and cores can also opt-out > via an IMP-DEF sysreg (!). Right now the bootloader just sets up all > cores to accept IRQs, and we ignore all this and let the magic > algorithm pick a CPU to accept the IRQ. Maybe that's ok for the set of peripherals attached, but in general that violates existing expectations regarding affinity, and I fear there'll be some subtle brokenness resulting from this automatic target selection. For example, in the perf events subsystem there are PMU drivers (even those for "uncore" or "system" devices which are shared by many/all CPUs) which rely on a combination of interrupt affinity and local IRQ masking (without any other locking) to provide exclusion between a PMU's IRQ handler and any other management operations for that PMU (which are all handled from the same CPU). > In the future, we might start making use of these finer-grained > capabilities for e.g. better real-time guarantees (CPUs running RT > threads might opt out of IRQs). What mechanism does the HW have for affinity selection? The wording above makes it sound like each CPU has to opt-out rather than having a central affinity selection. Is there a mechanism to select a single target? Thanks, Mark. > Legacy IPI support is also gone, so this implements Fast IPI support. > Fast IPIs are implemented entirely in the CPU core complexes, using > FIQs and IMP-DEF sysregs. This is also supported on t8103/M1, so we > enable it there too, but we keep the legacy AIC IPI codepath in case > it is useful for backporting to older chips. > > This also adds support for multi-die AIC2 controllers. While no > multi-die products exist yet, the AIC2 in t600x is built to support > up to 2 dies, and it's pretty clear how it works, so let's implement > it. If we're lucky, when multi-die products roll around, this will > let us support them with only DT changes. In order to support the > extra die dimension, this introduces a 4-argument IRQ phandle form > (3-argument is always supported and just implies die 0). > > All register offsets are computed based on capability register values, > which should allow forward-compatibility with future AIC2 variants... > except for one. For some inexplicable reason, the number of actually > implemented die register sets is nowhere to be found (t600x has 2, > but claims 1 die in use and 8 dies max, neither of which is what we > need), and this is necessary to compute the event register offset, > which is page-aligned after the die register sets. We have no choice > but to stick this offset in the device tree... which is the same thing > Apple do in their ADT. > > Changes since v1: > - Split off the DT binding > - Changed fast-ipi codepath selection to use a static key for performance > - Added fix for PCI driver to support the new 4-cell IRQ form > - Minor style / review feedback fixes > > Hector Martin (7): > PCI: apple: Change MSI handling to handle 4-cell AIC fwspec form > dt-bindings: interrupt-controller: apple,aic2: New binding for AICv2 > irqchip/apple-aic: Add Fast IPI support > irqchip/apple-aic: Switch to irq_domain_create_tree and sparse hwirqs > irqchip/apple-aic: Dynamically compute register offsets > irqchip/apple-aic: Support multiple dies > irqchip/apple-aic: Add support for AICv2 > > .../interrupt-controller/apple,aic2.yaml | 99 ++++ > MAINTAINERS | 2 +- > drivers/irqchip/irq-apple-aic.c | 432 +++++++++++++++--- > drivers/pci/controller/pcie-apple.c | 2 +- > 4 files changed, 458 insertions(+), 77 deletions(-) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/apple,aic2.yaml > > -- > 2.33.0 >
On Thu, 24 Feb 2022 18:26:41 +0000, Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, Feb 24, 2022 at 10:07:34PM +0900, Hector Martin wrote: > > Hi folks, > > Hi Hector, > > > In the t6000/t6001 (M1 Pro / Max) SoCs, Apple introduced a new version > > of their interrupt controller. This is a significant departure from > > AICv1 and seems designed to better scale to larger chips. This series > > adds support for it to the existing AIC driver. > > > > Gone are CPU affinities; instead there seems to be some kind of > > "automagic" dispatch to willing CPU cores, and cores can also opt-out > > via an IMP-DEF sysreg (!). Right now the bootloader just sets up all > > cores to accept IRQs, and we ignore all this and let the magic > > algorithm pick a CPU to accept the IRQ. > > Maybe that's ok for the set of peripherals attached, but in general that > violates existing expectations regarding affinity, and I fear there'll > be some subtle brokenness resulting from this automatic target > selection. > > For example, in the perf events subsystem there are PMU drivers (even > those for "uncore" or "system" devices which are shared by many/all > CPUs) which rely on a combination of interrupt affinity and local IRQ > masking (without any other locking) to provide exclusion between a PMU's > IRQ handler and any other management operations for that PMU (which are > all handled from the same CPU). It will definitely break anything that relies on managed interrupts, where the kernel expects to allocate interrupts that have a strict affinity. Drivers using this feature can legitimately expect that they can keep their state in per-CPU pointers, and that obviously breaks. This may affect any PCIe device with more than a couple of queues. Maybe users of this HW do not care (yet), but we'll have to find a way to tell drivers of the limitation. M.
On 25/02/2022 04.06, Marc Zyngier wrote: > On Thu, 24 Feb 2022 18:26:41 +0000, > Mark Rutland <mark.rutland@arm.com> wrote: >> >> On Thu, Feb 24, 2022 at 10:07:34PM +0900, Hector Martin wrote: >>> Hi folks, >> >> Hi Hector, >> >>> In the t6000/t6001 (M1 Pro / Max) SoCs, Apple introduced a new version >>> of their interrupt controller. This is a significant departure from >>> AICv1 and seems designed to better scale to larger chips. This series >>> adds support for it to the existing AIC driver. >>> >>> Gone are CPU affinities; instead there seems to be some kind of >>> "automagic" dispatch to willing CPU cores, and cores can also opt-out >>> via an IMP-DEF sysreg (!). Right now the bootloader just sets up all >>> cores to accept IRQs, and we ignore all this and let the magic >>> algorithm pick a CPU to accept the IRQ. >> >> Maybe that's ok for the set of peripherals attached, but in general that >> violates existing expectations regarding affinity, and I fear there'll >> be some subtle brokenness resulting from this automatic target >> selection. >> >> For example, in the perf events subsystem there are PMU drivers (even >> those for "uncore" or "system" devices which are shared by many/all >> CPUs) which rely on a combination of interrupt affinity and local IRQ >> masking (without any other locking) to provide exclusion between a PMU's >> IRQ handler and any other management operations for that PMU (which are >> all handled from the same CPU). > > It will definitely break anything that relies on managed interrupts, > where the kernel expects to allocate interrupts that have a strict > affinity. Drivers using this feature can legitimately expect that they > can keep their state in per-CPU pointers, and that obviously breaks. > > This may affect any PCIe device with more than a couple of queues. > Maybe users of this HW do not care (yet), but we'll have to find a way > to tell drivers of the limitation. Yes, we already had a brief discussion about this in the v1 thread: https://lore.kernel.org/linux-arm-kernel/4a83dfb1-3188-8b09-fc60-d3083230fb54@marcan.st/ TL;DR there is no explicit per-IRQ affinity control, nor does an unknown one seem possible, since there just aren't enough bits for it in per-IRQ registers. AICv1 had that, but AICv2 got rid of it in favor of heuristic magic and global per-CPU controls. This hasn't actually been fully tested yet, but current hypothesis is the mapping goes: 1 IRQ -> group (0-7) -> priority (0-3?) -> 1 CPU (local priority threshold) This is based on the fact that the per-IRQ group field is 3 bits, and the per-CPU mask IMP-DEF sysreg is 2 bits. There may or may not be per-IRQ cluster controls. But that still leaves all IRQs funnelled into, at most, 3-4 classes per CPU cluster, and 8 groups globally, so there's no way to implement proper per-IRQ affinity (since we have 10 CPUs on these platforms). My guess is Apple has bet on heuristic magic to optimize IRQ delivery to avoid waking up (deep?-)sleeping CPUs on low-priority events and optimize for power, and forgone strict per-CPU queues which are how many drivers optimize for performance. This makes some sense, since these are largely consumer/prosumer platforms, many of them battery-powered, not 128-CPU datacenter monsters with multiple 10GbE interfaces. They can probably get away without hard multiqueue stuff. This won't be an issue for PMU interrupts (including the uncore PMU), since those do not go through AIC per se but rather the FIQ path (which is inherently per-CPU), same as the local timers. Marc's PMU support patch set already takes care of adding support for those FIQ sources. But it will indeed break some PCIe drivers for devices that users might have arbitrarily attached through Thunderbolt. Since we do not support Thunderbolt yet, I suggest we kick this can down the road until we have test cases for how this breaks and how to fix it :-) There are also other fun things to be done with the local CPU masking, e.g. directing low-priority IRQs away from CPUs running real-time threads. I definitely want to take a look in more detail at the controls we *do* have, especially since I have a personal interest in RT for audio production (and these platforms have no SMM/TEE, no latency spikes, and fast cpufreq, woo!). But for now this works and brings up the platform, so that yak is probably best shaved in the future. Let me know if you're interested in having more discussions about RT-centric features, though. I suspect we'll need some new kernel mechanisms/interfaces to handle e.g. the CPU IMPDEF mask/prio stuff... Aside, I wonder how they'll handle multi-die devices... for a single die, you can probably well get away with no CPU pinning, but for multi-die, are they going to do NUMA? If so, they'd want at least die controls to avoid bouncing cache lines between dies too much... though for some reason, I'm getting the feeling they're just going to interleave memory and pretend it's UMA. Good chance we find out next month...
On Thu, 24 Feb 2022 13:07:41 +0000, Hector Martin <marcan@marcan.st> wrote: > > Introduce support for the new AICv2 hardware block in t6000/t6001 SoCs. > > It seems these blocks are missing the information required to compute > the event register offset in the capability registers, so we specify > that in the DT. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/irqchip/irq-apple-aic.c | 148 ++++++++++++++++++++++++++++---- > 1 file changed, 129 insertions(+), 19 deletions(-) > > diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c > index 93c622435ba2..602c8b274170 100644 > --- a/drivers/irqchip/irq-apple-aic.c > +++ b/drivers/irqchip/irq-apple-aic.c > @@ -103,6 +103,57 @@ > > #define AIC_MAX_IRQ 0x400 > > +/* > + * AIC v2 registers (MMIO) > + */ > + > +#define AIC2_VERSION 0x0000 > +#define AIC2_VERSION_VER GENMASK(7, 0) > + > +#define AIC2_INFO1 0x0004 > +#define AIC2_INFO1_NR_IRQ GENMASK(15, 0) > +#define AIC2_INFO1_LAST_DIE GENMASK(27, 24) > + > +#define AIC2_INFO2 0x0008 > + > +#define AIC2_INFO3 0x000c > +#define AIC2_INFO3_MAX_IRQ GENMASK(15, 0) > +#define AIC2_INFO3_MAX_DIE GENMASK(27, 24) > + > +#define AIC2_RESET 0x0010 > +#define AIC2_RESET_RESET BIT(0) > + > +#define AIC2_CONFIG 0x0014 > +#define AIC2_CONFIG_ENABLE BIT(0) > +#define AIC2_CONFIG_PREFER_PCPU BIT(28) > + > +#define AIC2_TIMEOUT 0x0028 > +#define AIC2_CLUSTER_PRIO 0x0030 > +#define AIC2_DELAY_GROUPS 0x0100 > + > +#define AIC2_IRQ_CFG 0x2000 > + > +/* > + * AIC2 registers are laid out like this, starting at AIC2_IRQ_CFG: > + * > + * Repeat for each die: > + * IRQ_CFG: u32 * MAX_IRQS > + * SW_SET: u32 * (MAX_IRQS / 32) > + * SW_CLR: u32 * (MAX_IRQS / 32) > + * MASK_SET: u32 * (MAX_IRQS / 32) > + * MASK_CLR: u32 * (MAX_IRQS / 32) > + * HW_STATE: u32 * (MAX_IRQS / 32) > + * > + * This is followed by a set of event registers, each 16K page aligned. > + * The first one is the AP event register we will use. Unfortunately, > + * the actual implemented die count is not specified anywhere in the > + * capability registers, so we have to explicitly specify the event > + * register offset in the device tree to remain forward-compatible. > + */ > + > +#define AIC2_IRQ_CFG_TARGET GENMASK(3, 0) > +#define AIC2_IRQ_CFG_DELAY_IDX GENMASK(7, 5) > + > #define MASK_REG(x) (4 * ((x) >> 5)) > #define MASK_BIT(x) BIT((x) & GENMASK(4, 0)) > > @@ -193,6 +244,7 @@ struct aic_info { > /* Register offsets */ > u32 event; > u32 target_cpu; > + u32 irq_cfg; > u32 sw_set; > u32 sw_clr; > u32 mask_set; > @@ -220,6 +272,14 @@ static const struct aic_info aic1_fipi_info = { > .fast_ipi = true, > }; > > +static const struct aic_info aic2_info = { > + .version = 2, > + > + .irq_cfg = AIC2_IRQ_CFG, > + > + .fast_ipi = true, > +}; > + > static const struct of_device_id aic_info_match[] = { > { > .compatible = "apple,t8103-aic", > @@ -229,6 +289,10 @@ static const struct of_device_id aic_info_match[] = { > .compatible = "apple,aic", > .data = &aic1_info, > }, > + { > + .compatible = "apple,aic2", > + .data = &aic2_info, > + }, > {} > }; > > @@ -373,6 +437,14 @@ static struct irq_chip aic_chip = { > .irq_set_type = aic_irq_set_type, > }; > > +static struct irq_chip aic2_chip = { > + .name = "AIC2", > + .irq_mask = aic_irq_mask, > + .irq_unmask = aic_irq_unmask, > + .irq_eoi = aic_irq_eoi, > + .irq_set_type = aic_irq_set_type, > +}; > + > /* > * FIQ irqchip > */ > @@ -529,10 +601,15 @@ static struct irq_chip fiq_chip = { > static int aic_irq_domain_map(struct irq_domain *id, unsigned int irq, > irq_hw_number_t hw) > { > + struct aic_irq_chip *ic = id->host_data; > u32 type = FIELD_GET(AIC_EVENT_TYPE, hw); > + struct irq_chip *chip = &aic_chip; > + > + if (ic->info.version == 2) > + chip = &aic2_chip; > > if (type == AIC_EVENT_TYPE_IRQ) { > - irq_domain_set_info(id, irq, hw, &aic_chip, id->host_data, > + irq_domain_set_info(id, irq, hw, chip, id->host_data, > handle_fasteoi_irq, NULL, NULL); > irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq))); > } else { > @@ -888,24 +965,26 @@ static int aic_init_cpu(unsigned int cpu) > /* Commit all of the above */ > isb(); > > - /* > - * Make sure the kernel's idea of logical CPU order is the same as AIC's > - * If we ever end up with a mismatch here, we will have to introduce > - * a mapping table similar to what other irqchip drivers do. > - */ > - WARN_ON(aic_ic_read(aic_irqc, AIC_WHOAMI) != smp_processor_id()); > + if (aic_irqc->info.version == 1) { > + /* > + * Make sure the kernel's idea of logical CPU order is the same as AIC's > + * If we ever end up with a mismatch here, we will have to introduce > + * a mapping table similar to what other irqchip drivers do. > + */ > + WARN_ON(aic_ic_read(aic_irqc, AIC_WHOAMI) != smp_processor_id()); Don't you have a similar issue with AICv2? Or is it that AICv2 doesn't have this register? Thanks, M.
On 26/02/2022 00.27, Marc Zyngier wrote: > On Thu, 24 Feb 2022 13:07:41 +0000, > Hector Martin <marcan@marcan.st> wrote: >> - /* >> - * Make sure the kernel's idea of logical CPU order is the same as AIC's >> - * If we ever end up with a mismatch here, we will have to introduce >> - * a mapping table similar to what other irqchip drivers do. >> - */ >> - WARN_ON(aic_ic_read(aic_irqc, AIC_WHOAMI) != smp_processor_id()); >> + if (aic_irqc->info.version == 1) { >> + /* >> + * Make sure the kernel's idea of logical CPU order is the same as AIC's >> + * If we ever end up with a mismatch here, we will have to introduce >> + * a mapping table similar to what other irqchip drivers do. >> + */ >> + WARN_ON(aic_ic_read(aic_irqc, AIC_WHOAMI) != smp_processor_id()); > > Don't you have a similar issue with AICv2? Or is it that AICv2 > doesn't have this register? No concept of individual CPUs in AICv2 at all, so no WHOAMI register either :)
On 25/02/2022 23.39, Marc Zyngier wrote: > On Thu, 24 Feb 2022 13:07:37 +0000, >> if (!(pending & irq_bit) && >> - (atomic_read(per_cpu_ptr(&aic_vipi_enable, cpu)) & irq_bit)) >> - send |= AIC_IPI_SEND_CPU(cpu); >> + (atomic_read(per_cpu_ptr(&aic_vipi_enable, cpu)) & irq_bit)) { >> + if (static_branch_likely(&use_fast_ipi)) >> + aic_ipi_send_fast(cpu); > > OK, this is suffering from the same issue that GICv3 has, which is > that memory barriers don't provide order against sysregs. You need a > DSB for that, which is a pain. Something like this: Doesn't the control flow here guarantee the ordering? atomic_read() must complete before the sysreg is written since there is a control flow dependency, and the prior atomic/barrier dance ensures that read is ordered properly with everything that comes before it.
On Sun, 27 Feb 2022 15:33:54 +0000, Hector Martin <marcan@marcan.st> wrote: > > On 25/02/2022 23.39, Marc Zyngier wrote: > > On Thu, 24 Feb 2022 13:07:37 +0000, > >> if (!(pending & irq_bit) && > >> - (atomic_read(per_cpu_ptr(&aic_vipi_enable, cpu)) & irq_bit)) > >> - send |= AIC_IPI_SEND_CPU(cpu); > >> + (atomic_read(per_cpu_ptr(&aic_vipi_enable, cpu)) & irq_bit)) { > >> + if (static_branch_likely(&use_fast_ipi)) > >> + aic_ipi_send_fast(cpu); > > > > OK, this is suffering from the same issue that GICv3 has, which is > > that memory barriers don't provide order against sysregs. You need a > > DSB for that, which is a pain. Something like this: > > Doesn't the control flow here guarantee the ordering? atomic_read() must > complete before the sysreg is written since there is a control flow > dependency, and the prior atomic/barrier dance ensures that read is > ordered properly with everything that comes before it. Yes, you're right. Mixing memory ordering and control dependency hurts my head badly, but hey, why not. M.