On Thu, 04 Mar 2021 21:38:51 +0000,
Hector Martin [off-list ref] wrote:

This is the root interrupt controller used on Apple ARM SoCs such as the
M1. This irqchip driver performs multiple functions:

* Handles both IRQs and FIQs

* Drives the AIC peripheral itself (which handles IRQs)

* Dispatches FIQs to downstream hard-wired clients (currently the ARM
  timer).

* Implements a virtual IPI multiplexer to funnel multiple Linux IPIs
  into a single hardware IPI

[...]

Signed-off-by: Hector Martin <redacted>
+static void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
+{
+	struct aic_irq_chip *ic = aic_irqc;
+	u32 event, type, irq;
+
+	do {
+		/*
+		 * We cannot use a relaxed read here, as DMA needs to be
+		 * ordered with respect to the IRQ firing.
+		 */
+		event = readl(ic->base + AIC_EVENT);
+		type = FIELD_GET(AIC_EVENT_TYPE, event);
+		irq = FIELD_GET(AIC_EVENT_NUM, event);
+
+		if (type == AIC_EVENT_TYPE_HW)
+			handle_domain_irq(aic_irqc->hw_domain, irq, regs);
+		else if (type == AIC_EVENT_TYPE_IPI && irq == 1)
+			aic_handle_ipi(regs);
+		else if (event != 0)
+			pr_err("Unknown IRQ event %d, %d\n", type, irq);
+	} while (event);
+
+	/*
+	 * vGIC maintenance interrupts end up here too, so we need to check
+	 * for them separately. Just report and disable vGIC for now, until
+	 * we implement this properly.
+	 */
+	if ((read_sysreg_s(SYS_ICH_HCR_EL2) & ICH_HCR_EN) &&
+		read_sysreg_s(SYS_ICH_MISR_EL2) != 0) {
+		pr_err("vGIC IRQ fired, disabling.\n");

Please add a _ratelimited here. Whilst debugging KVM on this machine,
I ended up with this firing at such a rate that it was impossible to
do anything useful. Ratelimiting it allowed me to pinpoint the
problem.

[...]

+/*
+ * FIQ irqchip
+ */
+
+static void aic_fiq_mask(struct irq_data *d)
+{
+	/* Only the guest timers have real mask bits, unfortunately. */
+	switch (d->hwirq) {
+	case AIC_TMR_GUEST_PHYS:
+		sysreg_clear_set_s(SYS_APL_VM_TMR_FIQ_ENA_EL1, VM_TMR_FIQ_ENABLE_P, 0);
+		break;
+	case AIC_TMR_GUEST_VIRT:
+		sysreg_clear_set_s(SYS_APL_VM_TMR_FIQ_ENA_EL1, VM_TMR_FIQ_ENABLE_V, 0);
+		break;
+	}
+}
+
+static void aic_fiq_unmask(struct irq_data *d)
+{
+	switch (d->hwirq) {
+	case AIC_TMR_GUEST_PHYS:
+		sysreg_clear_set_s(SYS_APL_VM_TMR_FIQ_ENA_EL1, 0, VM_TMR_FIQ_ENABLE_P);
+		break;
+	case AIC_TMR_GUEST_VIRT:
+		sysreg_clear_set_s(SYS_APL_VM_TMR_FIQ_ENA_EL1, 0, VM_TMR_FIQ_ENABLE_V);
+		break;
+	}
+}
+
+static void aic_fiq_eoi(struct irq_data *d)
+{
+	/* We mask to ack (where we can), so we need to unmask at EOI. */
+	if (!irqd_irq_disabled(d) && !irqd_irq_masked(d))

Ah, be careful here: irqd_irq_masked() doesn't do what you think it
does for per-CPU interrupts. It's been on my list to fix for the rVIC
implementation, but I never got around to doing it, and all decent ICs
hide this from SW by having a HW-managed mask, similar to what is on
the IRQ side.

I can see two possibilities:

- you can track the masked state directly and use that instead of
  these predicates

- you can just drop the masking altogether as this is only useful to a
  hosted hypervisor (KVM), which will have to do its own masking
  behind the scenes anyway

+		aic_fiq_unmask(d);
+}
+

The rest looks good to me.

Thanks,

	M.

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