| Message ID | 1614370277-23235-8-git-send-email-megha.dey@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Introduce dev-msi and interrupt message store | expand |
On Fri, 26 Feb 2021 20:11:11 +0000, Megha Dey <megha.dey@intel.com> wrote: > > From: Thomas Gleixner <tglx@linutronix.de> > > For devices which don't have a standard storage for MSI messages like the > upcoming IMS (Interrupt Message Store) it's required to allocate storage > space before allocating interrupts and after freeing them. > > This could be achieved with the existing callbacks, but that would be > awkward because they operate on msi_alloc_info_t which is not uniform > across architectures. Also these callbacks are invoked per interrupt but > the allocation might have bulk requirements depending on the device. > > As such devices can operate on different architectures it is simpler to > have separate callbacks which operate on struct device. The resulting > storage information has to be stored in struct msi_desc so the underlying > irq chip implementation can retrieve it for the relevant operations. > > Reviewed-by: Tony Luck <tony.luck@intel.com> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Megha Dey <megha.dey@intel.com> > --- > include/linux/msi.h | 8 ++++++++ > kernel/irq/msi.c | 11 +++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/include/linux/msi.h b/include/linux/msi.h > index 46e879c..e915932 100644 > --- a/include/linux/msi.h > +++ b/include/linux/msi.h > @@ -323,6 +323,10 @@ struct msi_domain_info; > * function. > * @domain_free_irqs: Optional function to override the default free > * function. > + * @msi_alloc_store: Optional callback to allocate storage in a device > + * specific non-standard MSI store > + * @msi_alloc_free: Optional callback to free storage in a device > + * specific non-standard MSI store > * > * @get_hwirq, @msi_init and @msi_free are callbacks used by > * msi_create_irq_domain() and related interfaces > @@ -372,6 +376,10 @@ struct msi_domain_ops { > struct device *dev, int nvec); > void (*domain_free_irqs)(struct irq_domain *domain, > struct device *dev); > + int (*msi_alloc_store)(struct irq_domain *domain, > + struct device *dev, int nvec); > + void (*msi_free_store)(struct irq_domain *domain, > + struct device *dev); > }; > > /** > diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c > index c54316d..047b59d 100644 > --- a/kernel/irq/msi.c > +++ b/kernel/irq/msi.c > @@ -434,6 +434,12 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, > if (ret) > return ret; > > + if (ops->msi_alloc_store) { > + ret = ops->msi_alloc_store(domain, dev, nvec); What is supposed to happen if we get aliasing devices (similar to what we have with devices behind a PCI bridge)? The ITS code goes through all kind of hoops to try and detect this case when sizing the translation tables (in the .prepare callback), and I have the feeling that sizing the message store is analogous. Or do we all have the warm fuzzy feeling that aliasing is a thing of the past and that we can ignore this potential problem? Thanks, M.
On Thu, Mar 25 2021 at 17:08, Marc Zyngier wrote: > Megha Dey <megha.dey@intel.com> wrote: >> @@ -434,6 +434,12 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, >> if (ret) >> return ret; >> >> + if (ops->msi_alloc_store) { >> + ret = ops->msi_alloc_store(domain, dev, nvec); > > What is supposed to happen if we get aliasing devices (similar to what > we have with devices behind a PCI bridge)? > > The ITS code goes through all kind of hoops to try and detect this > case when sizing the translation tables (in the .prepare callback), > and I have the feeling that sizing the message store is analogous. No. The message store itself is sized upfront by the underlying 'master' device. Each 'master' device has it's own irqdomain. This is the allocation for the subdevice and this is not part of PCI and therefore not subject to PCI aliasing. |-----------| | PCI dev | <- driver creates irqdomain and manages MSI |-----------| Sizing is either fixed (hardware property) or just managed by that irqdomain/driver with some hardware constraints |subdev| <- subdev gets ^^irqdomain assigned and allocates from it. ..... |subdev| So this is fundamentally different from ITS because ITS has to size the translation memory, i.e. where the MSI message is written to by the device. IMS just handles the storage of the message in the (sub)device. So if that needs to be supported on ARM then the issue is not with the subdevices, the issue is with the 'master' device, but that does not use that alloc_store() callback as it provides it with the irqdomain it manages. Thanks, tglx
On Thu, 25 Mar 2021 18:44:48 +0000, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Thu, Mar 25 2021 at 17:08, Marc Zyngier wrote: > > Megha Dey <megha.dey@intel.com> wrote: > >> @@ -434,6 +434,12 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, > >> if (ret) > >> return ret; > >> > >> + if (ops->msi_alloc_store) { > >> + ret = ops->msi_alloc_store(domain, dev, nvec); > > > > What is supposed to happen if we get aliasing devices (similar to what > > we have with devices behind a PCI bridge)? > > > > The ITS code goes through all kind of hoops to try and detect this > > case when sizing the translation tables (in the .prepare callback), > > and I have the feeling that sizing the message store is analogous. > > No. The message store itself is sized upfront by the underlying 'master' > device. Each 'master' device has it's own irqdomain. > > This is the allocation for the subdevice and this is not part of PCI and > therefore not subject to PCI aliasing. Fair enough. If we are guaranteed that there is no aliasing, then this point is moot. M.
diff --git a/include/linux/msi.h b/include/linux/msi.h index 46e879c..e915932 100644 --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -323,6 +323,10 @@ struct msi_domain_info; * function. * @domain_free_irqs: Optional function to override the default free * function. + * @msi_alloc_store: Optional callback to allocate storage in a device + * specific non-standard MSI store + * @msi_alloc_free: Optional callback to free storage in a device + * specific non-standard MSI store * * @get_hwirq, @msi_init and @msi_free are callbacks used by * msi_create_irq_domain() and related interfaces @@ -372,6 +376,10 @@ struct msi_domain_ops { struct device *dev, int nvec); void (*domain_free_irqs)(struct irq_domain *domain, struct device *dev); + int (*msi_alloc_store)(struct irq_domain *domain, + struct device *dev, int nvec); + void (*msi_free_store)(struct irq_domain *domain, + struct device *dev); }; /** diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c index c54316d..047b59d 100644 --- a/kernel/irq/msi.c +++ b/kernel/irq/msi.c @@ -434,6 +434,12 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, if (ret) return ret; + if (ops->msi_alloc_store) { + ret = ops->msi_alloc_store(domain, dev, nvec); + if (ret) + return ret; + } + for_each_msi_entry(desc, dev) { ops->set_desc(&arg, desc); @@ -529,6 +535,8 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, void __msi_domain_free_irqs(struct irq_domain *domain, struct device *dev) { + struct msi_domain_info *info = domain->host_data; + struct msi_domain_ops *ops = info->ops; struct msi_desc *desc; for_each_msi_entry(desc, dev) { @@ -542,6 +550,9 @@ void __msi_domain_free_irqs(struct irq_domain *domain, struct device *dev) desc->irq = 0; } } + + if (ops->msi_free_store) + ops->msi_free_store(domain, dev); } /**