Re: [patch V2 06/40] PCI/MSI: Provide static key for parent mask/unmask

[Marc Zyngier]

On Tue, 23 May 2023 14:05:56 +0100,
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 
> On Tue, May 23 2023 at 11:25, Marc Zyngier wrote:
> > On Mon, 22 May 2023 15:19:39 +0100,
> > Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >> On the other hand for PCI/MSI[x] the mask/unmask operations are not in
> >> the hot path as PCI/MSI[x] are strictly edge. Mask/unmask is only
> >> happening on startup, shutdown and when an interrupt arrives after
> >> disable_irq() incremented the lazy disable counter.
> >> 
> >> For regular interrupt handling mask/unmask is not involved.
> >> 
> >> So to avoid that global key we can let the parent domain set a new flag,
> >> e.g. MSI_FLAG_PCI_MSI_MASK_PARENT, in msi_parent_ops::supported_flags
> >> and let the PCI/MSI core code query that information when the per device
> >> domain is created and select the appropriate template or fixup the
> >> callbacks after the domain is created.
> >> 
> >> Does that address your concerns?
> >
> > It does to a certain extent.
> >
> > But what I'd really like is that in the most common case where the
> > interrupt controller is capable of masking MSIs, the PCI/MSI
> > *enabling* becomes the responsibility of the PCI core code and not the
> > IRQ code.
> >
> > The IRQ code should ideally only be concerned with the masking of the
> > interrupt at the irqchip level, and not beyond that. And that'd solve
> > the Xen problem by merely ignoring it.
> >
> > If we have HW out there that cannot mask MSIs at the interrupt
> > controller level, then we'd have to fallback to device-side masking,
> > which doesn't really work in general (MultiMSI being my favourite
> > example). My gut feeling is that this is rare, but I'm pretty sure it
> > exists.
> 
> Sure. There are 3 parts involved:
> 
>       [Device]--->[PCI/MSI]---->[GIC]
>                    irqchip      irqchip
> 
> Controlling the interrupt machinery in the device happens at the device
> driver level and is conceptually independent of the interrupt
> manangement code. The device driver has no access to the PCI/MSI irqchip
> and all it can do is to enable/disable the source of the interrupt in
> the device.
> 
> For the interrupt management code the job is to ensure that an interrupt
> can be prevented from disrupting the OS operation independent of the
> device driver correctness.
> 
> As a matter of fact we know that PCI/MSI masking ranges from not
> possible over flaky to properly working. So we can't reliably prevent
> that a rougue device spams the PCIe bus with messages.
> 
> Which means that we should utilize the fact that the next interrupt chip
> in the hierarchy can mask reliably. I wish I could disable individual
> vectors at the local APIC level on x86...
> 
> Now the question is whether we want to make this conditional depending
> on what the PCI/MSI[X] hardware advertises or just keep it simple and do
> it unconditionally.

I think this should be unconditional if the root irqchip (the GIC in
this instance) is capable of it.

So a suggestion where the root irqchip exposes its masking capability,
which upon detection by the upper layer (whateverbusyouwant/MSI) makes
it stop playing with its own device-level mask has my full support
(and now breathe normally).

Thanks,

	M.

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