Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines

[Marc Zyngier]

On Thu, 02 Aug 2018 03:03:20 +0100,
Ricardo Neri <ricardo.neri-calderon@xxxxxxxxxxxxxxx> wrote:
> 
> On Wed, Aug 01, 2018 at 04:09:59PM +0100, Julien Thierry wrote:
> > >>+static bool irq_supports_nmi(struct irq_desc *desc)
> > >>+{
> > >>+	struct irq_data *d = irq_desc_get_irq_data(desc);
> > >>+
> > >>+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> > >>+	/* Only IRQs directly managed by the root irqchip can be set as NMI */
> > >>+	if (d->parent_data)
> > >>+		return false;
> > >>+#endif
> > >
> > >I don't think this works in x86 because the local APIC irq_chip is the
> > >root of the hierarchy; the rest of the irq_chips are children of it.
> > >Furthermore, the delivery mode (i.e., regular interrupt vs NMI) is set in
> > >these children controllers, either MSI or interrupt remapping.
> > >
> > >Instead, could it be possible to determine if NMI mode is supported by
> > >checking if any of the parents of the irq_data supports NMI mode? If it
> > >has to be the root, perhaps a flag can indicate irq_supports_nmi() about
> > >this restriction.
> > >
> > 
> > I see, I'm not very sure how to deal with the semantics here. Does it make
> > sense for an irqchip to be able to deliver NMIs if its parent doesn't?
> 
> I think it does not make sense. 
> 
> > 
> > If we want to handle NMI in and irqchip hierachy, shouldn't all irqchips
> > between the one delivering the NMI and the root irqchip also be able to
> > deliver NMIs? (Or is it the other way around and an irqchip can deliver an
> > NMI if all its children irqchip are also able to deliver an NMI?)
> 
> At least in APIC, all the irq_chips support NMI delivery. However only one
> of them will generate the NMI interrupt message, the rest (e.g., the local
> APIC) will only relay it to the CPU.
> 
> So yes, I agree that the whole hierarchy should support NMI delivery, but
> the chip generating the NMI is not necessarily the root chip.
> 
> One question that I have is that whether the new flag IRQCHIP_SUPPORTS_NMI
> should be added to irq_chips that only relay NMIs or only to those from
> which an NMI can be requested.

If we need to distinguish between the two, then we need two flags. One
that indicates the generation capability, and one that indicates the
forwarding capability.

> > >>+
> > >>+	desc = irq_to_desc(irq);
> > >>+
> > >>+	if (!desc || irq_settings_can_autoenable(desc)
> > >>+	    || !irq_settings_can_request(desc)
> > >
> > >Is this a hard requirement for ARM? I guess it is, because you added
> > >enable_nmi() for this very purpose. I could not find any irq in the x86
> > >core code that uses the IRQ_NOAUTOEN flag.
> > >
> > 
> > On Arm64, we do use IRQ_NOAUTOEN and it is used for one of the interrupts
> > we'd like to handle as NMI. So we do need IRQ_NOAUTOEN to be supported for
> > NMIs.
> 
> I am not sure what would be the changes to add IRQ_NOAUTOEN to a specific
> interrupt in my use case and whether these changes would be regarded as a
> hack for a special case. I'll investigate and get back to you.
> 
> > 
> > However, the rationale for forcing it for NMIs was:
> > - I wanted to avoid too many things being done automatically for NMIs, this
> > way most of the responsability of managing the NMI is on the NMI requester
> > rather than the core code.
> 
> But doesn't the NMI requester already have some control? It can control at
> least the source of the NMIs (e.g., it can control when to start the timer
> that would generate the NMI).
> 
> > 
> > - There would be a need to pass down the information that we are requesting
> > an NMI to __setup_irq and do the proper setup when enabling depending on
> > whether we have an NMI or not.
> > 
> > I'm not completely against supporting autoenable for NMIs, but I'd like to
> > have the opinion of others on the matter
> 
> +1

I guess it is a matter of defining precisely where you want the
enabling to take place, and using NOAUTOEN does give you
flexibility. It also gives a coherent API with the percpu NMIs which
do require a separate, percpu enable.

Now, it is pretty easy to support both behaviours: we could bring back
the auto-enable on request for non-percpu NMIs, and rely on a
irq_set_status_flags(irq, IRQ_NOAUTOEN) before the request...

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.