On Thu, 04 Mar 2021 21:38:51 +0000,
Hector Martin <marcan@xxxxxxxxx> 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 <marcan@xxxxxxxxx>
> +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.
