diff mbox series

[V2,07/13] irqdomain/msi: Provide msi_alloc/free_store() callbacks

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

Commit Message

Dey, Megha Feb. 26, 2021, 8:11 p.m. UTC
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(+)

Comments

Marc Zyngier March 25, 2021, 5:08 p.m. UTC | #1
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.
Thomas Gleixner March 25, 2021, 6:44 p.m. UTC | #2
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
Marc Zyngier March 26, 2021, 10:14 a.m. UTC | #3
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 mbox series

Patch

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);
 }
 
 /**