[08/20] genirq/msi: Make MSI descriptor iterators device domain aware

  To support multiple MSI interrupt domains per device it is necessary to
segment the xarray MSI descriptor storage. Each domain gets up to
MSI_MAX_INDEX entries.

Change the iterators so they operate with domain ids and take the domain
offsets into account.

The publicly available iterators which are mostly used in legacy
implementations and the PCI/MSI core default to MSI_DEFAULT_DOMAIN (0)
which is the id for the existing "global" domains.

No functional change.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/msi.h |   45 +++++++++++++++++++++++++++++++++++++++++----
 kernel/irq/msi.c    |   43 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 76 insertions(+), 12 deletions(-)
  

On Fri, Nov 11, 2022 at 02:56:50PM +0100, Thomas Gleixner wrote:
> To support multiple MSI interrupt domains per device it is necessary to
> segment the xarray MSI descriptor storage. Each domain gets up to
> MSI_MAX_INDEX entries.

This kinds of suggests that the new per-device MSI domains should hold
this storage instead of per-device xarray?

I suppose the reason to avoid this is because alot of the driver
facing API is now built on vector index numbers that index this
xarray?

But on the other hand can we just say drivers using multiple domains
are "new" and they should use some new style pointer based interface
so we don't have to have arrays of things?

At least, I'd like to understand a bit better the motivation for using
a domain ID instead of a pointer.

It feels like we are baking in several hard coded limits with this
choice

> +static int msi_get_domain_base_index(struct device *dev, unsigned int domid)
> +{
> +	lockdep_assert_held(&dev->msi.data->mutex);
> +
> +	if (WARN_ON_ONCE(domid >= MSI_MAX_DEVICE_IRQDOMAINS))
> +		return -ENODEV;
> +
> +	if (WARN_ON_ONCE(!dev->msi.data->__irqdomains[domid]))
> +		return -ENODEV;
> +
> +	return domid * MSI_XA_DOMAIN_SIZE;
> +}
> +
> +
>  /**

Extra new line

Jason
  

On Wed, Nov 16 2022 at 14:36, Jason Gunthorpe wrote:
> On Fri, Nov 11, 2022 at 02:56:50PM +0100, Thomas Gleixner wrote:
>> To support multiple MSI interrupt domains per device it is necessary to
>> segment the xarray MSI descriptor storage. Each domain gets up to
>> MSI_MAX_INDEX entries.
>
> This kinds of suggests that the new per-device MSI domains should hold
> this storage instead of per-device xarray?

No, really not. This would create random storage in random driver places
instead of having a central storage place which is managed by the core
code. We've had that back in the days when every architecture had it's
own magic place to store and manage interrupt descriptors. Seen that,
mopped it up and never want to go back.

> I suppose the reason to avoid this is because alot of the driver
> facing API is now built on vector index numbers that index this
> xarray?

That's one aspect, but as I demonstrate later even for the IMS domains
which do not have a real requirement for 'index' you still need to have
a place to store the MSI descriptor and allocate storage space for it.

I really don't want to have random places doing that because then I
can't provide implicit MSI descriptor management, e.g. automatic
alloc/free anymore and everything has to happen at the driver side. The
only reason why I still need to do that for PCI/MSI is to be able to
support the museum architectures which still depend on the arch_....()
interfaces from 20 years ago.

So if a IMS domain, which e.g. stores the MSI message in queue memory,
wants a new interrupt then it allocates it with MSI_ANY_INDEX, which
gives it the next free slot in the XARRAY section of the MSI domain.

This avoids having IDA, bitmap allocators or whatever at the driver side
and having a virtual index number to track things does not affect the
flexibility of the driver side in any way.

All the driver needs at the very end is the interrupt number and the
message itself.

> But on the other hand can we just say drivers using multiple domains
> are "new" and they should use some new style pointer based interface
> so we don't have to have arrays of things?

Then driver writers have to provide storage for the domain pointer and
care about teardown etc. Seriously? NO!

> At least, I'd like to understand a bit better the motivation for using
> a domain ID instead of a pointer.

The main motivation was to avoid device specific storage for the irq
domain pointers. It would have started with PCI/MSI[X]: I'd had to add a
irqdomain pointer to struct pci_dev and then have the PCI core care
about it. So we'd add that to everything and the world which utilizes
per device MSI domains which is quite a few places outside of PCI in the
ARM64 world and growing.

The msi_device_data struct which is allocated on demand for MSI usage is
the obvious point to store _and_ manage these things, i.e. managed
teardown etc.

Giving this up makes any change to the core code hard because you have
to chase all usage sites and mop them up. Just look at the ARM part of
this series which is by now 40+ patches just to mop up the irqchip
core. There are still 25 PCI/MSI global irqdomain left.

> It feels like we are baking in several hard coded limits with this
> choice

Which ones?

The chosen array section size per domain is arbitrary and can be changed
at any given time. Though you have to exhaust 64k vectors per domain
first before we start debating that.

The number of irqdomains is not really hard limited either. It's trivial
enough to extend that number and once we hit 32 we just can stash them
away in the xarray. I pondered to do that right away, but that wastes
too much memory for now.

It really does not matter whether the domain creation results in a
number or in a pointer. Pointers are required for the inner workings of
the domain hierarchy but absolutely uninteresting for endpoint domains.

All you need there is a conveniant way to create the domain and then
allocate/free interrupts as you see fit.

We agreed a year ago that we want to abstract most of these things away
for driver writers and that all they need is simple way to create the
domains and the corresponding interrupt chip is mostly about writing the
MSI message to implementation defined storage and eventually providing a
implementation specific mask/unmask operation.

So what are you concerned about?

Thanks,

        tglx
  

On Wed, Nov 16, 2022 at 11:32:15PM +0100, Thomas Gleixner wrote:
> On Wed, Nov 16 2022 at 14:36, Jason Gunthorpe wrote:
> > On Fri, Nov 11, 2022 at 02:56:50PM +0100, Thomas Gleixner wrote:
> >> To support multiple MSI interrupt domains per device it is necessary to
> >> segment the xarray MSI descriptor storage. Each domain gets up to
> >> MSI_MAX_INDEX entries.
> >
> > This kinds of suggests that the new per-device MSI domains should hold
> > this storage instead of per-device xarray?
> 
> No, really not. This would create random storage in random driver places
> instead of having a central storage place which is managed by the core
> code. We've had that back in the days when every architecture had it's
> own magic place to store and manage interrupt descriptors. Seen that,
> mopped it up and never want to go back.

I don't mean shift it into the msi_domain driver logic, I just mean
stick an xarray in the struct msi_domain that the core code, and only
the core code, manages.

But I suppose, on reflection, the strong reason not to do this is that
the msi_descriptor array is per-device, and while it would work OK
with per-device msi_domains we still have the legacy of global msi
domains and thus still need a per-device place to store the global msi
domain's per-device descriptors.

> > At least, I'd like to understand a bit better the motivation for using
> > a domain ID instead of a pointer.
> 
> The main motivation was to avoid device specific storage for the irq
> domain pointers. It would have started with PCI/MSI[X]: I'd had to add a
> irqdomain pointer to struct pci_dev and then have the PCI core care
> about it. So we'd add that to everything and the world which utilizes
> per device MSI domains which is quite a few places outside of PCI in the
> ARM64 world and growing.

I was thinking more that the "default" domain (eg the domain ID 0 as
this series has it) would remain as a domain pointer in the device
data as it is here, but any secondary domains would be handled with a
pointer that the driver owns.

You could have as many secondary domains as is required this way. Few
drivers would ever use a secondary domain, so it not really a big deal
for them to hold the pointer lifetime.

> So what are you concerned about?

Mostly API clarity, I find it very un-kernly to swap a clear pointer
for an ID #. We loose typing, the APIs become less clear and we now
have to worry about ID allocation policy if we ever need more than 2.

Thanks,
Jason
  

On Wed, Nov 16 2022 at 20:37, Jason Gunthorpe wrote:
> On Wed, Nov 16, 2022 at 11:32:15PM +0100, Thomas Gleixner wrote:
>> On Wed, Nov 16 2022 at 14:36, Jason Gunthorpe wrote:
>> > On Fri, Nov 11, 2022 at 02:56:50PM +0100, Thomas Gleixner wrote:
>> >> To support multiple MSI interrupt domains per device it is necessary to
>> >> segment the xarray MSI descriptor storage. Each domain gets up to
>> >> MSI_MAX_INDEX entries.
>> >
>> > This kinds of suggests that the new per-device MSI domains should hold
>> > this storage instead of per-device xarray?
>> 
>> No, really not. This would create random storage in random driver places
>> instead of having a central storage place which is managed by the core
>> code. We've had that back in the days when every architecture had it's
>> own magic place to store and manage interrupt descriptors. Seen that,
>> mopped it up and never want to go back.
>
> I don't mean shift it into the msi_domain driver logic, I just mean
> stick an xarray in the struct msi_domain that the core code, and only
> the core code, manages.
>
> But I suppose, on reflection, the strong reason not to do this is that
> the msi_descriptor array is per-device, and while it would work OK
> with per-device msi_domains we still have the legacy of global msi
> domains and thus still need a per-device place to store the global msi
> domain's per-device descriptors.

I tried several approaches but all of them ended up having slightly
different code pathes and decided to keep everything the same from
legacy arch over global MSI and the modern per device MSI models.

Due to that some of the constructs are slightly awkward, but the
important outcome for me was that I ended up with as many shared code
pathes as possible. Having separate code pathes for all variants is for
one causing code bloat and what's worse it's a guarantee for divergance
and maintenance nightmares. As this is setup/teardown management code
and not the fancy hotpath where we really want to spare cycles, I went
for the unified model.

> You could have as many secondary domains as is required this way. Few
> drivers would ever use a secondary domain, so it not really a big deal
> for them to hold the pointer lifetime.
>
>> So what are you concerned about?
>
> Mostly API clarity, I find it very un-kernly to swap a clear pointer
> for an ID #. We loose typing, the APIs become less clear and we now
> have to worry about ID allocation policy if we ever need more than 2.

I don't see an issue with that.

  id = msi_create_device_domain(dev, &template, ...);
  
is not much different from:

  ptr = msi_create_device_domain(dev, &template, ...);

But it makes a massive difference vs. encapsulation and pointer leakage.

If you have a stale ID then you can't do harm, a stale pointer very much
so.

Aside of that once pointers are available people insist on fiddling in
the guts. As I'm mopping up behind driver writers for the last twenty
years now, my confidence in them is pretty close to zero.

So I rather be defensive and work towards encapsulation where ever its
possible. Interrupts are a source of hard to debug subtle bugs, so
taking the tinkerers the tools away to cause them is a good thing IMO.

Thanks,

        tglx
  

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 11, 2022 9:57 PM
> 
> +/* Invalid XA index which is outside of any searchable range */
> +#define MSI_XA_MAX_INDEX	(ULONG_MAX - 1)
> +#define MSI_XA_DOMAIN_SIZE	(MSI_MAX_INDEX + 1)
> +

Out of curiosity. Other places treat MSI_MAX_INDEX - 1 as the upper
bound of a valid range. This size definition here implies that the last ID
is wasted for every domain. Is it intended?
  

On Fri, Nov 18 2022 at 07:57, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>> Sent: Friday, November 11, 2022 9:57 PM
>> 
>> +/* Invalid XA index which is outside of any searchable range */
>> +#define MSI_XA_MAX_INDEX	(ULONG_MAX - 1)
>> +#define MSI_XA_DOMAIN_SIZE	(MSI_MAX_INDEX + 1)
>> +
>
> Out of curiosity. Other places treat MSI_MAX_INDEX - 1 as the upper
> bound of a valid range. This size definition here implies that the last ID
> is wasted for every domain. Is it intended?

Bah. MSI_MAX_INDEX is inclusive so the size must be + 1. I obviously
missed that in the other places which use it as upper bound.

Not that it matters, but yes. Let me fix that.