Re: [patch 21/32] NTB/msi: Convert to msi_on_each_desc()

Jason Gunthorpe
2021-12-06T14:44:10Z

On Sun, Dec 05, 2021 at 03:16:40PM +0100, Thomas Gleixner wrote:
> On Sat, Dec 04 2021 at 15:20, Thomas Gleixner wrote:
> > On Fri, Dec 03 2021 at 12:41, Jason Gunthorpe wrote:
> > So I need to break that up in a way which caters for both cases, but
> > does neither create a special case for PCI nor for the rest of the
> > universe, i.e. the 1:1 case has to be a subset of the 1:2 case which
> > means all of it is common case. With that solved the storage question
> > becomes a nobrainer.
> >
> > When I looked at it again yesterday while writing mail, I went back to
> > my notes and found the loose ends where I left off. Let me go back and
> > start over from there.
>
> I found out why I stopped looking at it. I came from a similar point of
> view what you were suggesting:
>
> >> If IMS == MSI, then couldn't we conceptually have the dev->irqdomain
> >> completely unable to handle IMS/MSI/MSI-X at all, and instead, when
> >> the driver asks for PCI MSI access we create a new hierarchical
> >> irqdomain and link it to a MSI chip_ops or a MSI-X chip_ops - just as
> >> you outlined for IMS? (again, not saying to do this, but let's ask if
> >> that makes more sense than the current configuration)
>
> Which I shot down with:
>
> > That's not really a good idea because dev->irqdomain is a generic
> > mechanism and not restricted to the PCI use case. Special casing it for
> > PCI is just wrong. Special casing it for all use cases just to please
> > PCI is equally wrong. There is a world outside of PCI and x86.
>
> That argument is actually only partially correct.

I'm not sure I understood your reply? I think we are both agreeing
that dev->irqdomain wants to be a generic mechanism?

I'd say that today we've special cased it to handle PCI. IMHO that is
exactly what pci_msi_create_irq_domain() is doing - it replaces the
chip ops with ops that can *ONLY* do PCI MSI and so dev->irqdomain
becomes PCI only and non-generic.

> After studying my notes and some more code (sigh), it looks feasible
> under certain assumptions, constraints and consequences.
>
> Assumptions:
>
> 1) The irqdomain pointer of PCI devices which is set up during device
> discovery is not used by anything else than infrastructure which
> knows how to handle it.
>
> Of course there is no guarantee, but I'm not that horrified about
> it anymore after chasing the exposure with yet more coccinelle
> scripts.

OK

> Constraints:
>
> 1) This is strictly opt-in and depends on hierarchical irqdomains.

OK

> That means that devices which depend on IMS won't work on anything
> which is not up to date.

OK - I think any pressure to get platforms to update their code is
only positive.

> 2) Guest support is strictly opt-in
>
> The underlying architecture/subarchitecture specific irqdomain has
> to detect at setup time (eventually early boot), whether the
> underlying hypervisor supports it.
>
> The only reasonable way to support that is the availability of
> interrupt remapping via vIOMMU, as we discussed before.

This is talking about IMS specifically because of the legacy issue
where the MSI addr/data pair inside a guest is often completely fake?

> 4) The resulting irqdomain hierarchy would ideally look like this:
>
> VECTOR -> [IOMMU, ROUTING, ...] -> PCI/[MSI/MSI-X/IMS] domains

OK, this matches where I have come from as well

> That does not work in all cases due to architecture and host
> controller constraints, so we might end up with:
>
> VECTOR -> IOMMU -> SHIM -> PCI/[MSI/MSI-X/IMS] domains

OK - I dont' know enough about the architecture/controller details to
imagine what SHIM is, but if it allows keeping the PCI code as purely
PCI code, then great

> 5) The design rules for the device specific IMS irqdomains have to be
> documented and enforced to the extent possible.
>
> Rules which I have in my notes as of today:
>
> - The device specific IMS irq chip / irqdomain has to be strictly
> separated from the rest of the driver code and can only
> interact via the irq chip data which is either per interrupt or
> per device.

It seems OK with the observaion that IDXD and mlx5 will both need to
set 'per-interrupt' data before provisioning the IRQ

> I have some ideas how to enforce these things to go into
> drivers/irqchip/ so they are exposed to scrutiny and not
> burried in some "my device is special" driver code and applied
> by subsystem maintainers before anyone can even look at it.

Means more modules, but OK

> - The irqchip callbacks which can be implemented by these top
> level domains are going to be restricted.

OK - I think it is great that the driver will see a special ops struct
that is 'ops for device's MSI addr/data pair storage'. It makes it
really clear what it is

> - For the irqchip callbacks which are allowed/required the rules
> vs. following down the hierarchy need to be defined and
> enforced.

The driver should be the ultimate origin of the interrupt so it is
always end-point in the hierarchy, opposite the CPU?

I would hope the driver doesn't have an exposure to hierarchy?

> - To achieve that the registration interface will not be based on
> struct irq_chip. This will be a new representation and the core
> will convert that into a proper irq chip which fits into the
> hierarchy. This provides one central place where the hierarchy
> requirements can be handled as they depend on the underlying
> MSI domain (IOMMU, SHIM, etc.). Otherwise any change on that
> would require to chase the IMS irqchips all over the place.

OK, I like this too.

So we have a new concept: 'device MSI storage ops'

Put them along with the xarray holding the msi_descs and you've got my
msi_table :)

> 2) The device centric storage concept will stay as it does not make
> any sense to push it towards drivers and what's worse it would be a
> major pain vs. the not yet up to the task irqdomains and the legacy
> architecture backends to change that. I really have no interrest to
> make the legacy code
>
> It also makes sense because the interrupts are strictly tied to the
> device. They cannot originate from some disconnected layer of thin
> air.
>
> Sorry Jason, no tables for you. :)

How does the driver select with 'device MSI storage ops' it is
requesting a MSI for ?

> 1) I'm going to post part 1-3 of the series once more with the fallout
> and review comments addressed.

OK, I didn't see anything in there that was making anything harder in
this direction

> 5) Implement an IMS user.
>
> The obvious candidate which should be halfways accessible is the
> ath11 PCI driver which falls into that category.

Aiiee:

drivers/net/wireless/ath/ath11k/pci.c: ab_pci->msi_ep_base_data = msi_desc->msg.data;

So, we already have two in-tree PCI IMS devices!!

Agree this makes a lot of sense to focus on some first steps

Along with NTB which is in the same general camp

Thanksm
Jason