Re: [RFC PATCH 0/9] pci: portdrv: Add auxiliary bus and register CXL PMUs (and aer)

Jonathan Cameron
2024-09-05T11:23:51Z

Hi Thomas,

One quick follow up question below.

>
> So looking at the ASCII art of the cover letter:
>
> _____________ __ ________ __________
> | Port | | creates | | |
> | PCI Dev | |--------->| CPMU A | |
> |_____________| | |________| |
> |portdrv binds | | perf/cxlpmu binds |
> |________________| |___________________|
> \
> \
> \ ________ __________
> \ | | |
> ------>| CPMU B | |
> |________| |
> | perf/cxlpmu binds |
> |___________________|
>
> AND
>
> _____________ __
> | Type 3 | | creates ________ _________
> | PCI Dev | |--------------------------------------->| | |
> |_____________| | | CPMU C | |
> | cxl_pci binds | |________| |
> |________________| | perf/cxpmu binds |
> |__________________|
>
> If I understand it correctly then both the portdrv and the cxl_pci
> drivers create a "bus". The CPMU devices are attached to these buses.
>
> So we are talking about distinctly different devices with the twist that
> these devices somehow need to utilize the MSI/X (forget MSI) facility of
> the device which creates the bus.
>
> From the devres perspective we look at separate devices and that's what
> the interrupt code expects too. This reminds me of the lengthy
> discussion we had about IMS a couple of years ago.
>
> https://lore.kernel.org/all/87bljg7u4f.fsf@nanos.tec.linutronix.de/
>
> My view on that issue was wrong because the Intel people described the
> problem wrong. But the above pretty much begs for a proper separation
> and hierarchical design because you provide an actual bus and distinct
> devices. Reusing the ASCII art from that old thread for the second case,
> but it's probably the same for the first one:
>
> ]-------------------------------------------|
> | PCI device |
> ]-------------------| |
> | Physical function | |
> ]-------------------| |
> ]-------------------|----------| |
> | Control block for subdevices | |
> ]------------------------------| |
> | | <- "Subdevice BUS" |
> | | |
> | |-- Subddevice 0 |
> | |-- Subddevice 1 |
> | |-- ... |
> | |-- Subddevice N |
> ]-------------------------------------------|
>
> 1) cxl_pci driver binds to the PCI device.
>
> 2) cxl_pci driver creates AUXbus
>
> 3) Bus enumerates devices on AUXbus
>
> 4) Drivers bind to the AUXbus devices
>
> So you have a clear provider consumer relationship. Whether the
> subdevice utilizes resources of the PCI device or not is a hardware
> implementation detail.
>
> The important aspect is that you want to associate the subdevice
> resources to the subdevice instances and not to the PCI device which
> provides them.
>
> Let me focus on interrupts, but it's probably the same for everything
> else which is shared.
>
> Look at how the PCI device manages interrupts with the per device MSI
> mechanism. Forget doing this with either one of the legacy mechanisms.
>
> 1) It creates a hierarchical interrupt domain and gets the required
> resources from the provided parent domain. The PCI side does not
> care whether this is x86 or arm64 and it neither cares whether the
> parent domain does remapping or not. The only way it cares is about
> the features supported by the different parent domains (think
> multi-MSI).
>
> 2) Driver side allocations go through the per device domain
>
> That AUXbus is not any different. When the CPMU driver binds it wants to
> allocate interrupts. So instead of having a convoluted construct
> reaching into the parent PCI device, you simply can do:
>
> 1) Let the cxl_pci driver create a MSI parent domain and set that in
> the subdevice::msi::irqdomain pointer.
>
> 2) Provide cxl_aux_bus_create_irqdomain() which allows the CPMU device
> driver to create a per device interrupt domain.
>
> 3) Let the CPMU driver create its per device interrupt domain with
> the provided parent domain
>
> 4) Let the CPMU driver allocate its MSI-X interrupts through the per
> device domain
>
> Now on the cxl_pci side the AUXbus parent interrupt domain allocation
> function does:
>
> if (!pci_msix_enabled(pdev))
> return pci_msix_enable_and_alloc_irq_at(pdev, ....);
> else
> return pci_msix_alloc_irq_at(pdev, ....);

Hi Thomas,

I'm struggling to follow this suggestion
Would you expect the cxl_pci MSI parent domain to set it's parent as
msi_domain = irq_domain_create_hierarchy(dev_get_msi_domain(&pdev->dev),
IRQ_DOMAIN_FLAG_MSI_PARENT,
...
which seems to cause a problem with deadlocks in __irq_domain_alloc_irqs()
or create a separate domain structure and provide callbacks that reach into
the parent domain as necessary?

Or do I have this entirely wrong? I'm struggling to relate what existing
code like PCI does to what I need to do here.

Jonathan