[patch 01/20] genirq/msi: Move IRQ_DOMAIN_MSI_NOMASK_QUIRK to MSI flags

[patch 01/20] genirq/msi: Move IRQ_DOMAIN_MSI_NOMASK_QUIRK to MSI flags

[Thomas Gleixner]

It's truly a MSI only flag and for the upcoming per device MSI domains this
must be in the MSI flags so it can be set during domain setup without
exposing this quirk outside of x86.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/apic/msi.c |    5 ++---
 include/linux/irqdomain.h  |    9 +--------
 include/linux/msi.h        |    6 ++++++
 kernel/irq/msi.c           |    2 +-
 4 files changed, 10 insertions(+), 12 deletions(-)

--- a/arch/x86/kernel/apic/msi.c
+++ b/arch/x86/kernel/apic/msi.c
@@ -176,7 +176,8 @@ static struct msi_domain_ops pci_msi_dom
 
 static struct msi_domain_info pci_msi_domain_info = {
 	.flags		= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-			  MSI_FLAG_PCI_MSIX,
+			  MSI_FLAG_PCI_MSIX | MSI_FLAG_NOMASK_QUIRK,
+
 	.ops		= &pci_msi_domain_ops,
 	.chip		= &pci_msi_controller,
 	.handler	= handle_edge_irq,
@@ -200,8 +201,6 @@ struct irq_domain * __init native_create
 	if (!d) {
 		irq_domain_free_fwnode(fn);
 		pr_warn("Failed to initialize PCI-MSI irqdomain.\n");
-	} else {
-		d->flags |= IRQ_DOMAIN_MSI_NOMASK_QUIRK;
 	}
 	return d;
 }
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -186,15 +186,8 @@ enum {
 	/* Irq domain implements MSI remapping */
 	IRQ_DOMAIN_FLAG_MSI_REMAP	= (1 << 5),
 
-	/*
-	 * Quirk to handle MSI implementations which do not provide
-	 * masking. Currently known to affect x86, but partially
-	 * handled in core code.
-	 */
-	IRQ_DOMAIN_MSI_NOMASK_QUIRK	= (1 << 6),
-
 	/* Irq domain doesn't translate anything */
-	IRQ_DOMAIN_FLAG_NO_MAP		= (1 << 7),
+	IRQ_DOMAIN_FLAG_NO_MAP		= (1 << 6),
 
 	/*
 	 * Flags starting from IRQ_DOMAIN_FLAG_NONCORE are reserved
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -395,6 +395,12 @@ enum {
 	MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS	= (1 << 9),
 	/* Free MSI descriptors */
 	MSI_FLAG_FREE_MSI_DESCS		= (1 << 10),
+	/*
+	 * Quirk to handle MSI implementations which do not provide
+	 * masking. Currently known to affect x86, but has to be partially
+	 * handled in the core MSI code.
+	 */
+	MSI_FLAG_NOMASK_QUIRK		= (1 << 11),
 };
 
 int msi_domain_set_affinity(struct irq_data *data, const struct cpumask *mask,
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -875,7 +875,7 @@ static int __msi_domain_alloc_irqs(struc
 		 * MSI affinity setting requires a special quirk (X86) when
 		 * reservation mode is active.
 		 */
-		if (domain->flags & IRQ_DOMAIN_MSI_NOMASK_QUIRK)
+		if (info->flags & MSI_FLAG_NOMASK_QUIRK)
 			vflags |= VIRQ_NOMASK_QUIRK;
 	}
 

Re: [patch 01/20] genirq/msi: Move IRQ_DOMAIN_MSI_NOMASK_QUIRK to MSI flags

[Jason Gunthorpe]

On Fri, Nov 11, 2022 at 02:56:39PM +0100, Thomas Gleixner wrote:
> It's truly a MSI only flag and for the upcoming per device MSI domains this
> must be in the MSI flags so it can be set during domain setup without
> exposing this quirk outside of x86.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/apic/msi.c |    5 ++---
>  include/linux/irqdomain.h  |    9 +--------
>  include/linux/msi.h        |    6 ++++++
>  kernel/irq/msi.c           |    2 +-
>  4 files changed, 10 insertions(+), 12 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[patch 02/20] genirq/irqdomain: Rename irq_domain::dev to irq_domain::pm_dev

[Thomas Gleixner]

irq_domain::dev is a misnomer as it's usually the rule that a device
pointer points to something which is directly related to the instance.

irq_domain::dev can point to some other device for power management to
ensure that this underlying device is not powered down when an interrupt is
allocated.

The upcoming per device MSI domains really require a pointer to the device
which instantiated the irq domain and not to some random other device which
is required for power management down the chain.

Rename irq_domain::dev to irq_domain::pm_dev and fixup the few sites which
use that pointer.

Conversion was done with the help of coccinelle.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/irq-gic.c |    4 +-
 include/linux/irqdomain.h |   76 +++++++++++++++++++++++-----------------------
 kernel/irq/chip.c         |    8 ++--
 3 files changed, 44 insertions(+), 44 deletions(-)

--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -401,8 +401,8 @@ static void gic_irq_print_chip(struct ir
 {
 	struct gic_chip_data *gic = irq_data_get_irq_chip_data(d);
 
-	if (gic->domain->dev)
-		seq_printf(p, gic->domain->dev->of_node->name);
+	if (gic->domain->pm_dev)
+		seq_printf(p, gic->domain->pm_dev->of_node->name);
 	else
 		seq_printf(p, "GIC-%d", (int)(gic - &gic_data[0]));
 }
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -117,53 +117,53 @@ struct irq_domain_chip_generic;
 
 /**
  * struct irq_domain - Hardware interrupt number translation object
- * @link: Element in global irq_domain list.
- * @name: Name of interrupt domain
- * @ops: pointer to irq_domain methods
- * @host_data: private data pointer for use by owner.  Not touched by irq_domain
- *             core code.
- * @flags: host per irq_domain flags
- * @mapcount: The number of mapped interrupts
+ * @link:	Element in global irq_domain list.
+ * @name:	Name of interrupt domain
+ * @ops:	Pointer to irq_domain methods
+ * @host_data:	Private data pointer for use by owner.  Not touched by irq_domain
+ *		core code.
+ * @flags:	Per irq_domain flags
+ * @mapcount:	The number of mapped interrupts
  *
- * Optional elements
- * @fwnode: Pointer to firmware node associated with the irq_domain. Pretty easy
- *          to swap it for the of_node via the irq_domain_get_of_node accessor
- * @gc: Pointer to a list of generic chips. There is a helper function for
- *      setting up one or more generic chips for interrupt controllers
- *      drivers using the generic chip library which uses this pointer.
- * @dev: Pointer to a device that the domain represent, and that will be
- *       used for power management purposes.
- * @parent: Pointer to parent irq_domain to support hierarchy irq_domains
+ * Optional elements:
+ * @fwnode:	Pointer to firmware node associated with the irq_domain. Pretty easy
+ *		to swap it for the of_node via the irq_domain_get_of_node accessor
+ * @gc:		Pointer to a list of generic chips. There is a helper function for
+ *		setting up one or more generic chips for interrupt controllers
+ *		drivers using the generic chip library which uses this pointer.
+ * @pm_dev:	Pointer to a device that can be utilized for power management
+ *		purposes related to the irq domain.
+ * @parent:	Pointer to parent irq_domain to support hierarchy irq_domains
  *
- * Revmap data, used internally by irq_domain
- * @revmap_size: Size of the linear map table @revmap[]
- * @revmap_tree: Radix map tree for hwirqs that don't fit in the linear map
- * @revmap_mutex: Lock for the revmap
- * @revmap: Linear table of irq_data pointers
+ * Revmap data, used internally by the irq domain code:
+ * @revmap_size:	Size of the linear map table @revmap[]
+ * @revmap_tree:	Radix map tree for hwirqs that don't fit in the linear map
+ * @revmap_mutex:	Lock for the revmap
+ * @revmap:		Linear table of irq_data pointers
  */
 struct irq_domain {
-	struct list_head link;
-	const char *name;
-	const struct irq_domain_ops *ops;
-	void *host_data;
-	unsigned int flags;
-	unsigned int mapcount;
+	struct list_head		link;
+	const char			*name;
+	const struct irq_domain_ops	*ops;
+	void				*host_data;
+	unsigned int			flags;
+	unsigned int			mapcount;
 
 	/* Optional data */
-	struct fwnode_handle *fwnode;
-	enum irq_domain_bus_token bus_token;
-	struct irq_domain_chip_generic *gc;
-	struct device *dev;
+	struct fwnode_handle		*fwnode;
+	enum irq_domain_bus_token	bus_token;
+	struct irq_domain_chip_generic	*gc;
+	struct device			*pm_dev;
 #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
-	struct irq_domain *parent;
+	struct irq_domain		*parent;
 #endif
 
 	/* reverse map data. The linear map gets appended to the irq_domain */
-	irq_hw_number_t hwirq_max;
-	unsigned int revmap_size;
-	struct radix_tree_root revmap_tree;
-	struct mutex revmap_mutex;
-	struct irq_data __rcu *revmap[];
+	irq_hw_number_t			hwirq_max;
+	unsigned int			revmap_size;
+	struct radix_tree_root		revmap_tree;
+	struct mutex			revmap_mutex;
+	struct irq_data __rcu		*revmap[];
 };
 
 /* Irq domain flags */
@@ -206,7 +206,7 @@ static inline void irq_domain_set_pm_dev
 					    struct device *dev)
 {
 	if (d)
-		d->dev = dev;
+		d->pm_dev = dev;
 }
 
 #ifdef CONFIG_IRQ_DOMAIN
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1561,10 +1561,10 @@ int irq_chip_compose_msi_msg(struct irq_
 	return 0;
 }
 
-static struct device *irq_get_parent_device(struct irq_data *data)
+static struct device *irq_get_pm_device(struct irq_data *data)
 {
 	if (data->domain)
-		return data->domain->dev;
+		return data->domain->pm_dev;
 
 	return NULL;
 }
@@ -1578,7 +1578,7 @@ static struct device *irq_get_parent_dev
  */
 int irq_chip_pm_get(struct irq_data *data)
 {
-	struct device *dev = irq_get_parent_device(data);
+	struct device *dev = irq_get_pm_device(data);
 	int retval = 0;
 
 	if (IS_ENABLED(CONFIG_PM) && dev)
@@ -1597,7 +1597,7 @@ int irq_chip_pm_get(struct irq_data *dat
  */
 int irq_chip_pm_put(struct irq_data *data)
 {
-	struct device *dev = irq_get_parent_device(data);
+	struct device *dev = irq_get_pm_device(data);
 	int retval = 0;
 
 	if (IS_ENABLED(CONFIG_PM) && dev)

Re: [patch 02/20] genirq/irqdomain: Rename irq_domain::dev to irq_domain::pm_dev

[Jason Gunthorpe]

On Fri, Nov 11, 2022 at 02:56:40PM +0100, Thomas Gleixner wrote:
> irq_domain::dev is a misnomer as it's usually the rule that a device
> pointer points to something which is directly related to the instance.
> 
> irq_domain::dev can point to some other device for power management to
> ensure that this underlying device is not powered down when an interrupt is
> allocated.
> 
> The upcoming per device MSI domains really require a pointer to the device
> which instantiated the irq domain and not to some random other device which
> is required for power management down the chain.
> 
> Rename irq_domain::dev to irq_domain::pm_dev and fixup the few sites which
> use that pointer.
> 
> Conversion was done with the help of coccinelle.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/irqchip/irq-gic.c |    4 +-
>  include/linux/irqdomain.h |   76 +++++++++++++++++++++++-----------------------
>  kernel/irq/chip.c         |    8 ++--
>  3 files changed, 44 insertions(+), 44 deletions(-)
> 
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -401,8 +401,8 @@ static void gic_irq_print_chip(struct ir
>  {
>  	struct gic_chip_data *gic = irq_data_get_irq_chip_data(d);
>  
> -	if (gic->domain->dev)
> -		seq_printf(p, gic->domain->dev->of_node->name);
> +	if (gic->domain->pm_dev)
> +		seq_printf(p, gic->domain->pm_dev->of_node->name);

This looks a bit odd now? I guess it still prints something meaningful
to debugfs but I suppose this would someday make more sense to be the
::dev version?

> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -117,53 +117,53 @@ struct irq_domain_chip_generic;
>  
>  /**
>   * struct irq_domain - Hardware interrupt number translation object
> - * @link: Element in global irq_domain list.
> - * @name: Name of interrupt domain
> - * @ops: pointer to irq_domain methods
> - * @host_data: private data pointer for use by owner.  Not touched by irq_domain
> - *             core code.
> - * @flags: host per irq_domain flags
> - * @mapcount: The number of mapped interrupts
> + * @link:	Element in global irq_domain list.
> + * @name:	Name of interrupt domain
> + * @ops:	Pointer to irq_domain methods

Maybe all this whitespace change would like to be in its own patch?

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [patch 02/20] genirq/irqdomain: Rename irq_domain::dev to irq_domain::pm_dev

[Thomas Gleixner]

On Wed, Nov 16 2022 at 14:22, Jason Gunthorpe wrote:
>> +	if (gic->domain->pm_dev)
>> +		seq_printf(p, gic->domain->pm_dev->of_node->name);
>
> This looks a bit odd now? I guess it still prints something meaningful
> to debugfs but I suppose this would someday make more sense to be the
> ::dev version?

Maybe, but for now I just want to keep it compatible.

>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -117,53 +117,53 @@ struct irq_domain_chip_generic;
>>  
>>  /**
>>   * struct irq_domain - Hardware interrupt number translation object
>> - * @link: Element in global irq_domain list.
>> - * @name: Name of interrupt domain
>> - * @ops: pointer to irq_domain methods
>> - * @host_data: private data pointer for use by owner.  Not touched by irq_domain
>> - *             core code.
>> - * @flags: host per irq_domain flags
>> - * @mapcount: The number of mapped interrupts
>> + * @link:	Element in global irq_domain list.
>> + * @name:	Name of interrupt domain
>> + * @ops:	Pointer to irq_domain methods
>
> Maybe all this whitespace change would like to be in its own patch?

Yes.

[patch 03/20] genirq/msi: Create msi_api.h

[Thomas Gleixner]

Create a API header for MSI specific functions which are relevant to device
drivers.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/msi.h     |    6 ++++--
 include/linux/msi_api.h |   15 +++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -13,11 +13,14 @@
  *
  * Regular device drivers have no business with any of these functions and
  * especially storing MSI descriptor pointers in random code is considered
- * abuse. The only function which is relevant for drivers is msi_get_virq().
+ * abuse.
+ *
+ * Device driver relevant functions are available in <linux/msi_api.h>
  */
 
 #include <linux/irqdomain_defs.h>
 #include <linux/cpumask.h>
+#include <linux/msi_api.h>
 #include <linux/xarray.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
@@ -184,7 +187,6 @@ struct msi_device_data {
 
 int msi_setup_device_data(struct device *dev);
 
-unsigned int msi_get_virq(struct device *dev, unsigned int index);
 void msi_lock_descs(struct device *dev);
 void msi_unlock_descs(struct device *dev);
 
--- /dev/null
+++ b/include/linux/msi_api.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef LINUX_MSI_API_H
+#define LINUX_MSI_API_H
+
+/*
+ * APIs which are relevant for device driver code for allocating and
+ * freeing MSI interrupts and querying the associations between
+ * hardware/software MSI indices and the Linux interrupt number.
+ */
+
+struct device;
+
+unsigned int msi_get_virq(struct device *dev, unsigned int index);
+
+#endif

Re: [patch 03/20] genirq/msi: Create msi_api.h

[Jason Gunthorpe]

On Fri, Nov 11, 2022 at 02:56:42PM +0100, Thomas Gleixner wrote:
> Create a API header for MSI specific functions which are relevant to device
> drivers.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/linux/msi.h     |    6 ++++--
>  include/linux/msi_api.h |   15 +++++++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[patch 04/20] genirq/irqdomain: Provide IRQ_DOMAIN_FLAG_MSI_PARENT

[Thomas Gleixner]

The new PCI/IMS (Interrupt Message Store) functionality is allowing
hardware vendors to provide implementation specific storage for the MSI
messages. This can be device memory and also host/guest memory, e.g. in
queue memory which is shared with the hardware.

This requires device specific MSI interrupt domains, which cannot be
achived by expanding the existing PCI/MSI interrupt domain concept which is
a global interrupt domain shared by all PCI devices on a particular (IOMMU)
segment:

                                         |--- device 1
     [Vector]---[Remapping]---[PCI/MSI]--|...
                                         |--- device N

This works because the PCI/MSI[-X] space is uniform, but falls apart with
PCI/IMS which is implementation defined and must be available along with
PCI/MSI[-X] on the same device.

To support PCI/MSI[-X] plus PCI/IMS on the same device it is required to
rework the PCI/MSI interrupt domain hierarchy concept in the following way:

                              |--- [PCI/MSI] device 1
     [Vector]---[Remapping]---|...
                              |--- [PCI/MSI] device N

That allows in the next step to create multiple interrupt domains per device:


                              |--- [PCI/MSI] device 1
                              |--- [PCI/IMS] device 1
     [Vector]---[Remapping]---|...
                              |--- [PCI/MSI] device N
                              |--- [PCI/IMS] device N

So the domain which previously created the global PCI/MSI domain must now
act as parent domain for the per device domains.

The hierarchy depth is the same as before, but the PCI/MSI domains are then
device specific and not longer global.

Provide IRQ_DOMAIN_FLAG_MSI_PARENT, which allows to identify these parent
domains, along with helpers to query it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/irqdomain.h |   14 ++++++++++++++
 1 file changed, 14 insertions(+)
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -189,6 +189,9 @@ enum {
 	/* Irq domain doesn't translate anything */
 	IRQ_DOMAIN_FLAG_NO_MAP		= (1 << 6),
 
+	/* Irq domain is a MSI parent domain */
+	IRQ_DOMAIN_FLAG_MSI_PARENT	= (1 << 8),
+
 	/*
 	 * Flags starting from IRQ_DOMAIN_FLAG_NONCORE are reserved
 	 * for implementation specific purposes and ignored by the
@@ -551,6 +554,11 @@ static inline bool irq_domain_is_msi_rem
 
 extern bool irq_domain_hierarchical_is_msi_remap(struct irq_domain *domain);
 
+static inline bool irq_domain_is_msi_parent(struct irq_domain *domain)
+{
+	return domain->flags & IRQ_DOMAIN_FLAG_MSI_PARENT;
+}
+
 #else	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
 static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
 			unsigned int nr_irqs, int node, void *arg)
@@ -596,6 +604,12 @@ irq_domain_hierarchical_is_msi_remap(str
 {
 	return false;
 }
+
+static inline bool irq_domain_is_msi_parent(struct irq_domain *domain)
+{
+	return false;
+}
+
 #endif	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
 
 #else /* CONFIG_IRQ_DOMAIN */

RE: [patch 04/20] genirq/irqdomain: Provide IRQ_DOMAIN_FLAG_MSI_PARENT

[Tian, Kevin]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 11, 2022 9:57 PM
> 
> The new PCI/IMS (Interrupt Message Store) functionality is allowing
> hardware vendors to provide implementation specific storage for the MSI
> messages. This can be device memory and also host/guest memory, e.g. in
> queue memory which is shared with the hardware.
> 
> This requires device specific MSI interrupt domains, which cannot be
> achived by expanding the existing PCI/MSI interrupt domain concept which is

s/achived/achieved/

[patch 05/20] genirq/irqdomain: Provide IRQ_DOMAIN_FLAG_MSI_DEVICE

[Thomas Gleixner]

Similar to marking parent MSI domains it's required to identify per device
domains. Add flag and helpers.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/irqdomain.h |   13 +++++++++++++
 1 file changed, 13 insertions(+)

--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -192,6 +192,9 @@ enum {
 	/* Irq domain is a MSI parent domain */
 	IRQ_DOMAIN_FLAG_MSI_PARENT	= (1 << 8),
 
+	/* Irq domain is a MSI device domain */
+	IRQ_DOMAIN_FLAG_MSI_DEVICE	= (1 << 9),
+
 	/*
 	 * Flags starting from IRQ_DOMAIN_FLAG_NONCORE are reserved
 	 * for implementation specific purposes and ignored by the
@@ -559,6 +562,11 @@ static inline bool irq_domain_is_msi_par
 	return domain->flags & IRQ_DOMAIN_FLAG_MSI_PARENT;
 }
 
+static inline bool irq_domain_is_msi_device(struct irq_domain *domain)
+{
+	return domain->flags & IRQ_DOMAIN_FLAG_MSI_DEVICE;
+}
+
 #else	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
 static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
 			unsigned int nr_irqs, int node, void *arg)
@@ -609,6 +617,11 @@ static inline bool irq_domain_is_msi_par
 {
 	return false;
 }
+
+static inline bool irq_domain_is_msi_device(struct irq_domain *domain)
+{
+	return false;
+}
 
 #endif	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
 

[patch 06/20] genirq/msi: Check for invalid MSI parent domain usage

[Thomas Gleixner]

In the upcoming per device MSI domain concept the MSI parent domains are
not allowed to be used as regular MSI domains where the MSI allocation/free
operations are applicable.

Add appropriate checks.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/msi.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -937,13 +937,21 @@ int msi_domain_alloc_irqs_descs_locked(s
 
 	lockdep_assert_held(&dev->msi.data->mutex);
 
+	if (WARN_ON_ONCE(irq_domain_is_msi_parent(domain))) {
+		ret = -EINVAL;
+		goto free;
+	}
+
+	/* Frees allocated descriptors in case of failure. */
 	ret = msi_domain_add_simple_msi_descs(info, dev, nvec);
 	if (ret)
 		return ret;
 
 	ret = ops->domain_alloc_irqs(domain, dev, nvec);
-	if (ret)
-		msi_domain_free_irqs_descs_locked(domain, dev);
+	if (!ret)
+		return 0;
+free:
+	msi_domain_free_irqs_descs_locked(domain, dev);
 	return ret;
 }
 
@@ -1013,6 +1021,9 @@ void msi_domain_free_irqs_descs_locked(s
 
 	lockdep_assert_held(&dev->msi.data->mutex);
 
+	if (WARN_ON_ONCE(irq_domain_is_msi_parent(domain)))
+		return;
+
 	ops->domain_free_irqs(domain, dev);
 	if (ops->msi_post_free)
 		ops->msi_post_free(domain, dev);

RE: [patch 06/20] genirq/msi: Check for invalid MSI parent domain usage

[Tian, Kevin]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 11, 2022 9:57 PM
> 
> @@ -937,13 +937,21 @@ int msi_domain_alloc_irqs_descs_locked(s
> 
>  	lockdep_assert_held(&dev->msi.data->mutex);
> 
> +	if (WARN_ON_ONCE(irq_domain_is_msi_parent(domain))) {
> +		ret = -EINVAL;
> +		goto free;
> +	}
> +
> +	/* Frees allocated descriptors in case of failure. */
>  	ret = msi_domain_add_simple_msi_descs(info, dev, nvec);
>  	if (ret)
>  		return ret;

it's unusual to see a direct return when error unwind is already required
at an early failure point. is it something which can be improved here?

RE: [patch 06/20] genirq/msi: Check for invalid MSI parent domain usage

[Thomas Gleixner]

On Fri, Nov 18 2022 at 07:50, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>> Sent: Friday, November 11, 2022 9:57 PM
>> 
>> @@ -937,13 +937,21 @@ int msi_domain_alloc_irqs_descs_locked(s
>> 
>>  	lockdep_assert_held(&dev->msi.data->mutex);
>> 
>> +	if (WARN_ON_ONCE(irq_domain_is_msi_parent(domain))) {
>> +		ret = -EINVAL;
>> +		goto free;
>> +	}
>> +
>> +	/* Frees allocated descriptors in case of failure. */
>>  	ret = msi_domain_add_simple_msi_descs(info, dev, nvec);
>>  	if (ret)
>>  		return ret;
>
> it's unusual to see a direct return when error unwind is already required
> at an early failure point. is it something which can be improved here?

It's redundant in this case, but you are right it looks weird.

[patch 07/20] genirq/msi: Add pointers for per device irq domains

[Thomas Gleixner]

With the upcoming per device MSI interrupt domain support it is necessary
to store the domain pointers per device.

Instead of delegating that storage to device drivers or subsystems create a
storage array in struct msi_device_data which will also take care of
tearing down the irq domains when msi_device_data is cleaned up via devres.

The interfaces into the MSI core will be changed from irqdomain pointer
based interfaces to domain id based interfaces to support multiple MSI
domains on a single device (e.g. PCI/MSI[-X] and PCI/IMS.

Once the per device domain support is complete the irq domain pointer in
struct device::msi.domain will not longer contain a pointer to the "global"
MSI domain. It will contain a pointer to the MSI parent domain instead.

It would be a horrible maze of conditionals to evaluate all over the place
which domain pointer should be used, i.e. the "global" one in
device::msi::domain or one from the internal pointer array.

To avoid this evaluate in msi_setup_device_data() whether the irq domain
which is associated to a device is a "global" or a parent MSI domain. If it
is global then copy the pointer into the first entry in the irqdomain
pointer array.

This allows to convert interfaces and implementation to domain ids while
keeping everything existing working.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/msi.h     |    3 +++
 include/linux/msi_api.h |    8 ++++++++
 kernel/irq/msi.c        |   14 ++++++++++++++
 3 files changed, 25 insertions(+)

--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -77,6 +77,7 @@ struct msi_desc;
 struct pci_dev;
 struct platform_msi_priv_data;
 struct device_attribute;
+struct irq_domain;
 
 void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
 void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
@@ -176,6 +177,7 @@ enum msi_desc_filter {
  * @mutex:		Mutex protecting the MSI descriptor store
  * @__store:		Xarray for storing MSI descriptor pointers
  * @__iter_idx:		Index to search the next entry for iterators
+ * @__irqdomains:	Per device interrupt domains
  */
 struct msi_device_data {
 	unsigned long			properties;
@@ -183,6 +185,7 @@ struct msi_device_data {
 	struct mutex			mutex;
 	struct xarray			__store;
 	unsigned long			__iter_idx;
+	struct irq_domain		*__irqdomains[MSI_MAX_DEVICE_IRQDOMAINS];
 };
 
 int msi_setup_device_data(struct device *dev);
--- a/include/linux/msi_api.h
+++ b/include/linux/msi_api.h
@@ -10,6 +10,14 @@
 
 struct device;
 
+/*
+ * Per device interrupt domain related constants.
+ */
+enum msi_domain_ids {
+	MSI_DEFAULT_DOMAIN,
+	MSI_MAX_DEVICE_IRQDOMAINS,
+};
+
 unsigned int msi_get_virq(struct device *dev, unsigned int index);
 
 #endif
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -21,6 +21,18 @@
 
 static inline int msi_sysfs_create_group(struct device *dev);
 
+static inline void msi_setup_default_irqdomain(struct device *dev, struct msi_device_data *md)
+{
+	if (!dev->msi.domain)
+		return;
+	/*
+	 * If @dev::msi::domain is a global MSI domain, copy the pointer
+	 * into the domain array to avoid conditionals all over the place.
+	 */
+	if (!irq_domain_is_msi_parent(dev->msi.domain))
+		md->__irqdomains[MSI_DEFAULT_DOMAIN] = dev->msi.domain;
+}
+
 /**
  * msi_alloc_desc - Allocate an initialized msi_desc
  * @dev:	Pointer to the device for which this is allocated
@@ -213,6 +225,8 @@ int msi_setup_device_data(struct device
 		return ret;
 	}
 
+	msi_setup_default_irqdomain(dev, md);
+
 	xa_init(&md->__store);
 	mutex_init(&md->mutex);
 	dev->msi.data = md;

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

[Thomas Gleixner]

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(-)

--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -177,6 +177,7 @@ enum msi_desc_filter {
  * @mutex:		Mutex protecting the MSI descriptor store
  * @__store:		Xarray for storing MSI descriptor pointers
  * @__iter_idx:		Index to search the next entry for iterators
+ * @__iter_max:		Index to limit the search
  * @__irqdomains:	Per device interrupt domains
  */
 struct msi_device_data {
@@ -185,6 +186,7 @@ struct msi_device_data {
 	struct mutex			mutex;
 	struct xarray			__store;
 	unsigned long			__iter_idx;
+	unsigned long			__iter_max;
 	struct irq_domain		*__irqdomains[MSI_MAX_DEVICE_IRQDOMAINS];
 };
 
@@ -193,14 +195,34 @@ int msi_setup_device_data(struct device
 void msi_lock_descs(struct device *dev);
 void msi_unlock_descs(struct device *dev);
 
-struct msi_desc *msi_first_desc(struct device *dev, enum msi_desc_filter filter);
+struct msi_desc *msi_domain_first_desc(struct device *dev, unsigned int domid,
+				       enum msi_desc_filter filter);
+
+/**
+ * msi_first_desc - Get the first MSI descriptor of the default irqdomain
+ * @dev:	Device to operate on
+ * @filter:	Descriptor state filter
+ *
+ * Must be called with the MSI descriptor mutex held, i.e. msi_lock_descs()
+ * must be invoked before the call.
+ *
+ * Return: Pointer to the first MSI descriptor matching the search
+ *	   criteria, NULL if none found.
+ */
+static inline struct msi_desc *msi_first_desc(struct device *dev,
+					      enum msi_desc_filter filter)
+{
+	return msi_domain_first_desc(dev, MSI_DEFAULT_DOMAIN, filter);
+}
+
 struct msi_desc *msi_next_desc(struct device *dev, enum msi_desc_filter filter);
 
 /**
- * msi_for_each_desc - Iterate the MSI descriptors
+ * msi_domain_for_each_desc - Iterate the MSI descriptors in a specific domain
  *
  * @desc:	struct msi_desc pointer used as iterator
  * @dev:	struct device pointer - device to iterate
+ * @domid:	The id of the interrupt domain which should be walked.
  * @filter:	Filter for descriptor selection
  *
  * Notes:
@@ -208,10 +230,25 @@ struct msi_desc *msi_next_desc(struct de
  *    pair.
  *  - It is safe to remove a retrieved MSI descriptor in the loop.
  */
-#define msi_for_each_desc(desc, dev, filter)			\
-	for ((desc) = msi_first_desc((dev), (filter)); (desc);	\
+#define msi_domain_for_each_desc(desc, dev, domid, filter)			\
+	for ((desc) = msi_domain_first_desc((dev), (domid), (filter)); (desc);	\
 	     (desc) = msi_next_desc((dev), (filter)))
 
+/**
+ * msi_for_each_desc - Iterate the MSI descriptors in the default irqdomain
+ *
+ * @desc:	struct msi_desc pointer used as iterator
+ * @dev:	struct device pointer - device to iterate
+ * @filter:	Filter for descriptor selection
+ *
+ * Notes:
+ *  - The loop must be protected with a msi_lock_descs()/msi_unlock_descs()
+ *    pair.
+ *  - It is safe to remove a retrieved MSI descriptor in the loop.
+ */
+#define msi_for_each_desc(desc, dev, filter)					\
+	msi_domain_for_each_desc((desc), (dev), MSI_DEFAULT_DOMAIN, (filter))
+
 #define msi_desc_to_dev(desc)		((desc)->dev)
 
 #ifdef CONFIG_IRQ_MSI_IOMMU
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -21,6 +21,10 @@
 
 static inline int msi_sysfs_create_group(struct device *dev);
 
+/* 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)
+
 static inline void msi_setup_default_irqdomain(struct device *dev, struct msi_device_data *md)
 {
 	if (!dev->msi.domain)
@@ -33,6 +37,20 @@ static inline void msi_setup_default_irq
 		md->__irqdomains[MSI_DEFAULT_DOMAIN] = dev->msi.domain;
 }
 
+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;
+}
+
+
 /**
  * msi_alloc_desc - Allocate an initialized msi_desc
  * @dev:	Pointer to the device for which this is allocated
@@ -229,6 +247,7 @@ int msi_setup_device_data(struct device
 
 	xa_init(&md->__store);
 	mutex_init(&md->mutex);
+	md->__iter_idx = MSI_XA_MAX_INDEX;
 	dev->msi.data = md;
 	devres_add(dev, md);
 	return 0;
@@ -251,7 +270,7 @@ EXPORT_SYMBOL_GPL(msi_lock_descs);
 void msi_unlock_descs(struct device *dev)
 {
 	/* Invalidate the index wich was cached by the iterator */
-	dev->msi.data->__iter_idx = MSI_MAX_INDEX;
+	dev->msi.data->__iter_idx = MSI_XA_MAX_INDEX;
 	mutex_unlock(&dev->msi.data->mutex);
 }
 EXPORT_SYMBOL_GPL(msi_unlock_descs);
@@ -260,17 +279,18 @@ static struct msi_desc *msi_find_desc(st
 {
 	struct msi_desc *desc;
 
-	xa_for_each_start(&md->__store, md->__iter_idx, desc, md->__iter_idx) {
+	xa_for_each_range(&md->__store, md->__iter_idx, desc, md->__iter_idx, md->__iter_max) {
 		if (msi_desc_match(desc, filter))
 			return desc;
 	}
-	md->__iter_idx = MSI_MAX_INDEX;
+	md->__iter_idx = MSI_XA_MAX_INDEX;
 	return NULL;
 }
 
 /**
- * msi_first_desc - Get the first MSI descriptor of a device
+ * msi_domain_first_desc - Get the first MSI descriptor of an irqdomain associated to a device
  * @dev:	Device to operate on
+ * @domid:	The id of the interrupt domain which should be walked.
  * @filter:	Descriptor state filter
  *
  * Must be called with the MSI descriptor mutex held, i.e. msi_lock_descs()
@@ -279,19 +299,26 @@ static struct msi_desc *msi_find_desc(st
  * Return: Pointer to the first MSI descriptor matching the search
  *	   criteria, NULL if none found.
  */
-struct msi_desc *msi_first_desc(struct device *dev, enum msi_desc_filter filter)
+struct msi_desc *msi_domain_first_desc(struct device *dev, unsigned int domid,
+				       enum msi_desc_filter filter)
 {
 	struct msi_device_data *md = dev->msi.data;
+	int baseidx;
 
 	if (WARN_ON_ONCE(!md))
 		return NULL;
 
 	lockdep_assert_held(&md->mutex);
 
-	md->__iter_idx = 0;
+	baseidx = msi_get_domain_base_index(dev, domid);
+	if (baseidx < 0)
+		return NULL;
+
+	md->__iter_idx = baseidx;
+	md->__iter_max = baseidx + MSI_MAX_INDEX - 1;
 	return msi_find_desc(md, filter);
 }
-EXPORT_SYMBOL_GPL(msi_first_desc);
+EXPORT_SYMBOL_GPL(msi_domain_first_desc);
 
 /**
  * msi_next_desc - Get the next MSI descriptor of a device
@@ -315,7 +342,7 @@ struct msi_desc *msi_next_desc(struct de
 
 	lockdep_assert_held(&md->mutex);
 
-	if (md->__iter_idx >= (unsigned long)MSI_MAX_INDEX)
+	if (md->__iter_idx >= md->__iter_max)
 		return NULL;
 
 	md->__iter_idx++;

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

[Jason Gunthorpe]

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

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

[Thomas Gleixner]

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

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

[Jason Gunthorpe]

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

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

[Thomas Gleixner]

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

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

[Tian, Kevin]

> 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?

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

[Thomas Gleixner]

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.

[patch 09/20] genirq/msi: Make msi_get_virq() device domain aware

[Thomas Gleixner]

From: Ahmed S. Darwish <darwi@linutronix.de>

In preparation of the upcoming per device multi MSI domain support, change
the interface to support lookups based on domain id and zero based index
within the domain.

Signed-off-by: Ahmed S. Darwish <darwi@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/msi_api.h |   14 +++++++++++++-
 kernel/irq/msi.c        |   24 ++++++++++++++++++------
 2 files changed, 31 insertions(+), 7 deletions(-)

--- a/include/linux/msi_api.h
+++ b/include/linux/msi_api.h
@@ -18,6 +18,18 @@ enum msi_domain_ids {
 	MSI_MAX_DEVICE_IRQDOMAINS,
 };
 
-unsigned int msi_get_virq(struct device *dev, unsigned int index);
+unsigned int msi_domain_get_virq(struct device *dev, unsigned int domid, unsigned int index);
+
+/**
+ * msi_get_virq - Lookup the Linux interrupt number for a MSI index on the default interrupt domain
+ * @dev:	Device for which the lookup happens
+ * @index:	The MSI index to lookup
+ *
+ * Return: The Linux interrupt number on success (> 0), 0 if not found
+ */
+static inline unsigned int msi_get_virq(struct device *dev, unsigned int index)
+{
+	return msi_domain_get_virq(dev, MSI_DEFAULT_DOMAIN, index);
+}
 
 #endif
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -351,25 +351,37 @@ struct msi_desc *msi_next_desc(struct de
 EXPORT_SYMBOL_GPL(msi_next_desc);
 
 /**
- * msi_get_virq - Return Linux interrupt number of a MSI interrupt
+ * msi_domain_get_virq - Lookup the Linux interrupt number for a MSI index on a interrupt domain
  * @dev:	Device to operate on
+ * @domid:	Domain ID of the interrupt domain associated to the device
  * @index:	MSI interrupt index to look for (0-based)
  *
  * Return: The Linux interrupt number on success (> 0), 0 if not found
  */
-unsigned int msi_get_virq(struct device *dev, unsigned int index)
+unsigned int msi_domain_get_virq(struct device *dev, unsigned int domid, unsigned int index)
 {
 	struct msi_desc *desc;
 	unsigned int ret = 0;
-	bool pcimsi;
+	bool pcimsi = false;
+	int base;
 
 	if (!dev->msi.data)
 		return 0;
 
-	pcimsi = dev_is_pci(dev) ? to_pci_dev(dev)->msi_enabled : false;
+	if (WARN_ON_ONCE(index >= MSI_MAX_INDEX))
+		return 0;
+
+	/* This check is only valid for the PCI default MSI domain */
+	if (dev_is_pci(dev) && domid == MSI_DEFAULT_DOMAIN)
+		pcimsi = to_pci_dev(dev)->msi_enabled;
 
 	msi_lock_descs(dev);
-	desc = xa_load(&dev->msi.data->__store, pcimsi ? 0 : index);
+
+	base = msi_get_domain_base_index(dev, domid);
+	if (base < 0)
+		return 0;
+
+	desc = xa_load(&dev->msi.data->__store, base + pcimsi ? 0 : index);
 	if (desc && desc->irq) {
 		/*
 		 * PCI-MSI has only one descriptor for multiple interrupts.
@@ -386,7 +398,7 @@ unsigned int msi_get_virq(struct device
 	msi_unlock_descs(dev);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(msi_get_virq);
+EXPORT_SYMBOL_GPL(msi_domain_get_virq);
 
 #ifdef CONFIG_SYSFS
 static struct attribute *msi_dev_attrs[] = {

[patch V1A 09/20] genirq/msi: Make msi_get_virq() device domain aware

[Thomas Gleixner]

From: Ahmed S. Darwish <darwi@linutronix.de>

In preparation of the upcoming per device multi MSI domain support, change
the interface to support lookups based on domain id and zero based index
within the domain.

Signed-off-by: Ahmed S. Darwish <darwi@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
V1A: Fix the locking leak and a operator precedence issue (Dan, kernel robot)

     This was reported earlier and I missed to fold the fix before
     posting. :(
---
 include/linux/msi_api.h |   14 +++++++++++++-
 kernel/irq/msi.c        |   27 +++++++++++++++++++++------
 2 files changed, 34 insertions(+), 7 deletions(-)

--- a/include/linux/msi_api.h
+++ b/include/linux/msi_api.h
@@ -18,6 +18,18 @@ enum msi_domain_ids {
 	MSI_MAX_DEVICE_IRQDOMAINS,
 };
 
-unsigned int msi_get_virq(struct device *dev, unsigned int index);
+unsigned int msi_domain_get_virq(struct device *dev, unsigned int domid, unsigned int index);
+
+/**
+ * msi_get_virq - Lookup the Linux interrupt number for a MSI index on the default interrupt domain
+ * @dev:	Device for which the lookup happens
+ * @index:	The MSI index to lookup
+ *
+ * Return: The Linux interrupt number on success (> 0), 0 if not found
+ */
+static inline unsigned int msi_get_virq(struct device *dev, unsigned int index)
+{
+	return msi_domain_get_virq(dev, MSI_DEFAULT_DOMAIN, index);
+}
 
 #endif
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -351,25 +351,38 @@ struct msi_desc *msi_next_desc(struct de
 EXPORT_SYMBOL_GPL(msi_next_desc);
 
 /**
- * msi_get_virq - Return Linux interrupt number of a MSI interrupt
+ * msi_domain_get_virq - Lookup the Linux interrupt number for a MSI index on a interrupt domain
  * @dev:	Device to operate on
+ * @domid:	Domain ID of the interrupt domain associated to the device
  * @index:	MSI interrupt index to look for (0-based)
  *
  * Return: The Linux interrupt number on success (> 0), 0 if not found
  */
-unsigned int msi_get_virq(struct device *dev, unsigned int index)
+unsigned int msi_domain_get_virq(struct device *dev, unsigned int domid, unsigned int index)
 {
 	struct msi_desc *desc;
 	unsigned int ret = 0;
-	bool pcimsi;
+	bool pcimsi = false;
+	int base;
 
 	if (!dev->msi.data)
 		return 0;
 
-	pcimsi = dev_is_pci(dev) ? to_pci_dev(dev)->msi_enabled : false;
+	if (WARN_ON_ONCE(index >= MSI_MAX_INDEX))
+		return 0;
+
+	/* This check is only valid for the PCI default MSI domain */
+	if (dev_is_pci(dev) && domid == MSI_DEFAULT_DOMAIN)
+		pcimsi = to_pci_dev(dev)->msi_enabled;
 
 	msi_lock_descs(dev);
-	desc = xa_load(&dev->msi.data->__store, pcimsi ? 0 : index);
+
+	base = msi_get_domain_base_index(dev, domid);
+	if (base < 0)
+		goto unlock;
+
+	base += pcimsi ? 0 : index;
+	desc = xa_load(&dev->msi.data->__store, base);
 	if (desc && desc->irq) {
 		/*
 		 * PCI-MSI has only one descriptor for multiple interrupts.
@@ -383,10 +396,12 @@ unsigned int msi_get_virq(struct device
 			ret = desc->irq;
 		}
 	}
+
+unlock:
 	msi_unlock_descs(dev);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(msi_get_virq);
+EXPORT_SYMBOL_GPL(msi_domain_get_virq);
 
 #ifdef CONFIG_SYSFS
 static struct attribute *msi_dev_attrs[] = {

[patch 10/20] genirq/msi: Rename msi_add_msi_desc() to msi_insert_msi_desc()

[Thomas Gleixner]

This reflects the functionality better. No functional change.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/pci/msi/msi.c            |    4 ++--
 drivers/soc/ti/ti_sci_inta_msi.c |    4 ++--
 include/linux/msi.h              |    2 +-
 kernel/irq/msi.c                 |    6 ++++--
 4 files changed, 9 insertions(+), 7 deletions(-)

--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -308,7 +308,7 @@ static int msi_setup_msi_desc(struct pci
 	if (desc.pci.msi_attrib.can_mask)
 		pci_read_config_dword(dev, desc.pci.mask_pos, &desc.pci.msi_mask);
 
-	return msi_add_msi_desc(&dev->dev, &desc);
+	return msi_insert_msi_desc(&dev->dev, &desc);
 }
 
 static int msi_verify_entries(struct pci_dev *dev)
@@ -591,7 +591,7 @@ static int msix_setup_msi_descs(struct p
 			desc.pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
 		}
 
-		ret = msi_add_msi_desc(&dev->dev, &desc);
+		ret = msi_insert_msi_desc(&dev->dev, &desc);
 		if (ret)
 			break;
 	}
--- a/drivers/soc/ti/ti_sci_inta_msi.c
+++ b/drivers/soc/ti/ti_sci_inta_msi.c
@@ -73,13 +73,13 @@ static int ti_sci_inta_msi_alloc_descs(s
 	for (set = 0; set < res->sets; set++) {
 		for (i = 0; i < res->desc[set].num; i++, count++) {
 			msi_desc.msi_index = res->desc[set].start + i;
-			if (msi_add_msi_desc(dev, &msi_desc))
+			if (msi_insert_msi_desc(dev, &msi_desc))
 				goto fail;
 		}
 
 		for (i = 0; i < res->desc[set].num_sec; i++, count++) {
 			msi_desc.msi_index = res->desc[set].start_sec + i;
-			if (msi_add_msi_desc(dev, &msi_desc))
+			if (msi_insert_msi_desc(dev, &msi_desc))
 				goto fail;
 		}
 	}
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -274,7 +274,7 @@ static inline void msi_desc_set_iommu_co
 }
 #endif
 
-int msi_add_msi_desc(struct device *dev, struct msi_desc *init_desc);
+int msi_insert_msi_desc(struct device *dev, struct msi_desc *init_desc);
 void msi_free_msi_descs_range(struct device *dev, unsigned int first_index, unsigned int last_index);
 
 /**
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -100,13 +100,15 @@ static int msi_insert_desc(struct msi_de
 }
 
 /**
- * msi_add_msi_desc - Allocate and initialize a MSI descriptor
+ * msi_insert_msi_desc - Allocate and initialize a MSI descriptor and
+ *			 insert it at @init_desc->msi_index
+ *
  * @dev:	Pointer to the device for which the descriptor is allocated
  * @init_desc:	Pointer to an MSI descriptor to initialize the new descriptor
  *
  * Return: 0 on success or an appropriate failure code.
  */
-int msi_add_msi_desc(struct device *dev, struct msi_desc *init_desc)
+int msi_insert_msi_desc(struct device *dev, struct msi_desc *init_desc)
 {
 	struct msi_desc *desc;
 

Re: [patch 10/20] genirq/msi: Rename msi_add_msi_desc() to msi_insert_msi_desc()

[Jason Gunthorpe]

On Fri, Nov 11, 2022 at 02:56:53PM +0100, Thomas Gleixner wrote:
> This reflects the functionality better. No functional change.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/pci/msi/msi.c            |    4 ++--
>  drivers/soc/ti/ti_sci_inta_msi.c |    4 ++--
>  include/linux/msi.h              |    2 +-
>  kernel/irq/msi.c                 |    6 ++++--
>  4 files changed, 9 insertions(+), 7 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[patch 11/20] genirq/msi: Make descriptor allocation device domain aware

[Thomas Gleixner]

Change the descriptor allocation and insertion functions to take a domain
id to prepare for the upcoming multi MSI domain per device support.

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

--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -274,7 +274,20 @@ static inline void msi_desc_set_iommu_co
 }
 #endif
 
-int msi_insert_msi_desc(struct device *dev, struct msi_desc *init_desc);
+int msi_domain_insert_msi_desc(struct device *dev, unsigned int domid,
+			       struct msi_desc *init_desc);
+/**
+ * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the default domain
+ * @dev:	Pointer to the device for which the descriptor is allocated
+ * @init_desc:	Pointer to an MSI descriptor to initialize the new descriptor
+ *
+ * Return: 0 on success or an appropriate failure code.
+ */
+static inline int msi_insert_msi_desc(struct device *dev, struct msi_desc *init_desc)
+{
+	return msi_domain_insert_msi_desc(dev, MSI_DEFAULT_DOMAIN, init_desc);
+}
+
 void msi_free_msi_descs_range(struct device *dev, unsigned int first_index, unsigned int last_index);
 
 /**
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -63,7 +63,7 @@ static int msi_get_domain_base_index(str
  * Return: pointer to allocated &msi_desc on success or %NULL on failure
  */
 static struct msi_desc *msi_alloc_desc(struct device *dev, int nvec,
-					const struct irq_affinity_desc *affinity)
+				       const struct irq_affinity_desc *affinity)
 {
 	struct msi_desc *desc = kzalloc(sizeof(*desc), GFP_KERNEL);
 
@@ -88,39 +88,58 @@ static void msi_free_desc(struct msi_des
 	kfree(desc);
 }
 
-static int msi_insert_desc(struct msi_device_data *md, struct msi_desc *desc, unsigned int index)
+static int msi_insert_desc(struct device *dev, struct msi_desc *desc,
+			   unsigned int domid, unsigned int index)
 {
-	int ret;
+	struct msi_device_data *md = dev->msi.data;
+	int baseidx, ret;
+
+	baseidx = msi_get_domain_base_index(dev, domid);
+	if (baseidx < 0) {
+		ret = baseidx;
+		goto fail;
+	}
 
 	desc->msi_index = index;
+	index += baseidx;
 	ret = xa_insert(&md->__store, index, desc, GFP_KERNEL);
 	if (ret)
-		msi_free_desc(desc);
+		goto fail;
+	return 0;
+
+fail:
+	msi_free_desc(desc);
 	return ret;
 }
 
 /**
- * msi_insert_msi_desc - Allocate and initialize a MSI descriptor and
- *			 insert it at @init_desc->msi_index
+ * msi_domain_insert_msi_desc - Allocate and initialize a MSI descriptor and
+ *				insert it at @init_desc->msi_index
  *
  * @dev:	Pointer to the device for which the descriptor is allocated
+ * @domid:	The id of the interrupt domain to which the desriptor is added
  * @init_desc:	Pointer to an MSI descriptor to initialize the new descriptor
  *
  * Return: 0 on success or an appropriate failure code.
  */
-int msi_insert_msi_desc(struct device *dev, struct msi_desc *init_desc)
+int msi_domain_insert_msi_desc(struct device *dev, unsigned int domid,
+			       struct msi_desc *init_desc)
 {
 	struct msi_desc *desc;
 
 	lockdep_assert_held(&dev->msi.data->mutex);
 
+	if (WARN_ON_ONCE(init_desc->msi_index >= MSI_MAX_INDEX))
+		return -EINVAL;
+
 	desc = msi_alloc_desc(dev, init_desc->nvec_used, init_desc->affinity);
 	if (!desc)
 		return -ENOMEM;
 
 	/* Copy type specific data to the new descriptor. */
 	desc->pci = init_desc->pci;
-	return msi_insert_desc(dev->msi.data, desc, init_desc->msi_index);
+
+	return msi_insert_desc(dev, desc, domid, init_desc->msi_index);
 }
 
 /**
@@ -143,7 +162,7 @@ static int msi_add_simple_msi_descs(stru
 		desc = msi_alloc_desc(dev, 1, NULL);
 		if (!desc)
 			goto fail_mem;
-		ret = msi_insert_desc(dev->msi.data, desc, idx);
+		ret = msi_insert_desc(dev, desc, MSI_DEFAULT_DOMAIN, idx);
 		if (ret)
 			goto fail;
 	}
@@ -183,6 +202,9 @@ void msi_free_msi_descs_range(struct dev
 	struct msi_desc *desc;
 	unsigned long idx;
 
+	if (WARN_ON_ONCE(first_index >= MSI_MAX_INDEX || last_index >= MSI_MAX_INDEX))
+		return;
+
 	lockdep_assert_held(&dev->msi.data->mutex);
 
 	xa_for_each_range(xa, idx, desc, first_index, last_index) {
@@ -779,6 +801,10 @@ int msi_domain_populate_irqs(struct irq_
 	struct msi_desc *desc;
 	int ret, virq;
 
+	if (WARN_ON_ONCE(virq_base >= MSI_MAX_INDEX ||
+			 (virq_base + nvec) >= MSI_MAX_INDEX))
+		return 0;
+
 	msi_lock_descs(dev);
 	ret = msi_add_simple_msi_descs(dev, virq_base, nvec);
 	if (ret)

RE: [patch 11/20] genirq/msi: Make descriptor allocation device domain aware

[Tian, Kevin]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 11, 2022 9:57 PM
> 
> +/**
> + * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the
> default domain

and "insert it at @init_desc->msi_index", same as the text for
msi_domain_insert_msi_desc().

> @@ -183,6 +202,9 @@ void msi_free_msi_descs_range(struct dev
>  	struct msi_desc *desc;
>  	unsigned long idx;
> 
> +	if (WARN_ON_ONCE(first_index >= MSI_MAX_INDEX || last_index >=
> MSI_MAX_INDEX))
> +		return;
> +
>  	lockdep_assert_held(&dev->msi.data->mutex);
> 
>  	xa_for_each_range(xa, idx, desc, first_index, last_index) {

this doesn't belong to allocation. and next patch will convert it to
domid aware plus a formal check. Probably can just remove it
from this patch.

RE: [patch 11/20] genirq/msi: Make descriptor allocation device domain aware

[Thomas Gleixner]

On Fri, Nov 18 2022 at 08:10, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>> +	if (WARN_ON_ONCE(first_index >= MSI_MAX_INDEX || last_index >=
>> MSI_MAX_INDEX))
>> +		return;
>> +
>>  	lockdep_assert_held(&dev->msi.data->mutex);
>> 
>>  	xa_for_each_range(xa, idx, desc, first_index, last_index) {
>
> this doesn't belong to allocation. and next patch will convert it to
> domid aware plus a formal check. Probably can just remove it
> from this patch.

I added this just for paranoia sake so I could catch my conversion
error. So yes and no.

[patch 12/20] genirq/msi: Make descriptor freeing domain aware

[Thomas Gleixner]

Change the descriptor free functions to take a domain id to prepare for the
upcoming multi MSI domain per device support.

To avoid changing and extending the interfaces over and over use an core
internal control struct and hand the pointer through the various functions.

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

--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -277,7 +277,8 @@ static inline void msi_desc_set_iommu_co
 int msi_domain_insert_msi_desc(struct device *dev, unsigned int domid,
 			       struct msi_desc *init_desc);
 /**
- * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the default domain
+ * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the default irqdomain
+ *
  * @dev:	Pointer to the device for which the descriptor is allocated
  * @init_desc:	Pointer to an MSI descriptor to initialize the new descriptor
  *
@@ -288,10 +289,25 @@ static inline int msi_insert_msi_desc(st
 	return msi_domain_insert_msi_desc(dev, MSI_DEFAULT_DOMAIN, init_desc);
 }
 
-void msi_free_msi_descs_range(struct device *dev, unsigned int first_index, unsigned int last_index);
+void msi_domain_free_msi_descs_range(struct device *dev, unsigned int domid,
+				     unsigned int first, unsigned int last);
+
+/**
+ * msi_free_msi_descs_range - Free a range of MSI descriptors of a device
+ *			      in the default irqdomain
+ *
+ * @dev:	Device for which to free the descriptors
+ * @first:	Index to start freeing from (inclusive)
+ * @last:	Last index to be freed (inclusive)
+ */
+static inline void msi_free_msi_descs_range(struct device *dev, unsigned int first,
+					    unsigned int last)
+{
+	msi_domain_free_msi_descs_range(dev, MSI_DEFAULT_DOMAIN, first, last);
+}
 
 /**
- * msi_free_msi_descs - Free MSI descriptors of a device
+ * msi_free_msi_descs - Free all MSI descriptors of a device in the default irqdomain
  * @dev:	Device to free the descriptors
  */
 static inline void msi_free_msi_descs(struct device *dev)
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -19,6 +19,18 @@
 
 #include "internals.h"
 
+/**
+ * struct msi_ctrl - MSI internal management control structure
+ * @domid:	ID of the domain on which management operations should be done
+ * @first:	First (hardware) slot index to operate on
+ * @last:	Last (hardware) slot index to operate on
+ */
+struct msi_ctrl {
+	unsigned int			domid;
+	unsigned int			first;
+	unsigned int			last;
+};
+
 static inline int msi_sysfs_create_group(struct device *dev);
 
 /* Invalid XA index which is outside of any searchable range */
@@ -189,25 +201,32 @@ static bool msi_desc_match(struct msi_de
 	return false;
 }
 
-/**
- * msi_free_msi_descs_range - Free MSI descriptors of a device
- * @dev:		Device to free the descriptors
- * @first_index:	Index to start freeing from
- * @last_index:		Last index to be freed
- */
-void msi_free_msi_descs_range(struct device *dev, unsigned int first_index,
-			      unsigned int last_index)
+static bool msi_ctrl_valid(struct device *dev, struct msi_ctrl *ctrl)
+{
+	if (WARN_ON_ONCE(ctrl->first > ctrl->last ||
+			 ctrl->first >= MSI_MAX_INDEX ||
+			 ctrl->last >= MSI_MAX_INDEX))
+		return false;
+	return true;
+}
+
+static void msi_domain_free_descs(struct device *dev, struct msi_ctrl *ctrl)
 {
 	struct xarray *xa = &dev->msi.data->__store;
 	struct msi_desc *desc;
 	unsigned long idx;
+	int base;
+
+	lockdep_assert_held(&dev->msi.data->mutex);
 
-	if (WARN_ON_ONCE(first_index >= MSI_MAX_INDEX || last_index >= MSI_MAX_INDEX))
+	if (!msi_ctrl_valid(dev, ctrl))
 		return;
 
-	lockdep_assert_held(&dev->msi.data->mutex);
+	base = msi_get_domain_base_index(dev, ctrl->domid);
+	if (base < 0)
+		return;
 
-	xa_for_each_range(xa, idx, desc, first_index, last_index) {
+	xa_for_each_range(xa, idx, desc, ctrl->first + base, ctrl->last + base) {
 		xa_erase(xa, idx);
 
 		/* Leak the descriptor when it is still referenced */
@@ -217,6 +236,25 @@ void msi_free_msi_descs_range(struct dev
 	}
 }
 
+/**
+ * msi_domain_free_msi_descs_range - Free a range of MSI descriptors of a device in an irqdomain
+ * @dev:	Device for which to free the descriptors
+ * @domid:	Id of the domain to operate on
+ * @first:	Index to start freeing from (inclusive)
+ * @last:	Last index to be freed (inclusive)
+ */
+void msi_domain_free_msi_descs_range(struct device *dev, unsigned int domid,
+				     unsigned int first, unsigned int last)
+{
+	struct msi_ctrl ctrl = {
+		.domid	= domid,
+		.first	= first,
+		.last	= last,
+	};
+
+	msi_domain_free_descs(dev, &ctrl);
+}
+
 void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	*msg = entry->msg;

RE: [patch 12/20] genirq/msi: Make descriptor freeing domain aware

[Tian, Kevin]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 11, 2022 9:57 PM
> 
>  int msi_domain_insert_msi_desc(struct device *dev, unsigned int domid,
>  			       struct msi_desc *init_desc);
>  /**
> - * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the
> default domain
> + * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the
> default irqdomain
> + *

belong to last patch

> +/**
> + * struct msi_ctrl - MSI internal management control structure
> + * @domid:	ID of the domain on which management operations should
> be done
> + * @first:	First (hardware) slot index to operate on
> + * @last:	Last (hardware) slot index to operate on
> + */
> +struct msi_ctrl {
> +	unsigned int			domid;
> +	unsigned int			first;
> +	unsigned int			last;
> +};
> +

this really contains the range information. what about msi_range and
then msi_range_valid()?

> +static void msi_domain_free_descs(struct device *dev, struct msi_ctrl *ctrl)
>  {
>  	struct xarray *xa = &dev->msi.data->__store;
>  	struct msi_desc *desc;
>  	unsigned long idx;
> +	int base;
> +
> +	lockdep_assert_held(&dev->msi.data->mutex);
> 
> -	if (WARN_ON_ONCE(first_index >= MSI_MAX_INDEX || last_index >=
> MSI_MAX_INDEX))
> +	if (!msi_ctrl_valid(dev, ctrl))
>  		return;
> 
> -	lockdep_assert_held(&dev->msi.data->mutex);
> +	base = msi_get_domain_base_index(dev, ctrl->domid);
> +	if (base < 0)
> +		return;

What about putting domid checks in msi_ctrl_valid() then here could
be a simple calculation on domid * MSI_XA_DOMAIN_SIZE.

domid is part of msi_ctrl. then it sound reasonable to validate it
together with first/last.

RE: [patch 12/20] genirq/msi: Make descriptor freeing domain aware

[Thomas Gleixner]

On Fri, Nov 18 2022 at 08:17, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>>  /**
>> - * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the
>> default domain
>> + * msi_insert_msi_desc - Allocate and initialize a MSI descriptor in the
>> default irqdomain
>> + *
>
> belong to last patch

Yes.

>> +/**
>> + * struct msi_ctrl - MSI internal management control structure
>> + * @domid:	ID of the domain on which management operations should
>> be done
>> + * @first:	First (hardware) slot index to operate on
>> + * @last:	Last (hardware) slot index to operate on
>> + */
>> +struct msi_ctrl {
>> +	unsigned int			domid;
>> +	unsigned int			first;
>> +	unsigned int			last;
>> +};
>> +
>
> this really contains the range information. what about msi_range and
> then msi_range_valid()?

It's range plus domain id and later it gains nirqs. So its awkward in
any case.

>> +static void msi_domain_free_descs(struct device *dev, struct msi_ctrl *ctrl)
>>  {
>>  	struct xarray *xa = &dev->msi.data->__store;
>>  	struct msi_desc *desc;
>>  	unsigned long idx;
>> +	int base;
>> +
>> +	lockdep_assert_held(&dev->msi.data->mutex);
>> 
>> -	if (WARN_ON_ONCE(first_index >= MSI_MAX_INDEX || last_index >=
>> MSI_MAX_INDEX))
>> +	if (!msi_ctrl_valid(dev, ctrl))
>>  		return;
>> 
>> -	lockdep_assert_held(&dev->msi.data->mutex);
>> +	base = msi_get_domain_base_index(dev, ctrl->domid);
>> +	if (base < 0)
>> +		return;
>
> What about putting domid checks in msi_ctrl_valid() then here could
> be a simple calculation on domid * MSI_XA_DOMAIN_SIZE.
>
> domid is part of msi_ctrl. then it sound reasonable to validate it
> together with first/last.

Let me look at that.

RE: [patch 12/20] genirq/msi: Make descriptor freeing domain aware

[Thomas Gleixner]

On Fri, Nov 18 2022 at 13:22, Thomas Gleixner wrote:
> On Fri, Nov 18 2022 at 08:17, Kevin Tian wrote:
>>> -	lockdep_assert_held(&dev->msi.data->mutex);
>>> +	base = msi_get_domain_base_index(dev, ctrl->domid);
>>> +	if (base < 0)
>>> +		return;
>>
>> What about putting domid checks in msi_ctrl_valid() then here could
>> be a simple calculation on domid * MSI_XA_DOMAIN_SIZE.

I need the base domain index w/o the ctrl thing too.

>> domid is part of msi_ctrl. then it sound reasonable to validate it
>> together with first/last.
>
> Let me look at that.

So I kept it as is, but renamed msi_ctrl_valid() to
msi_ctrl_range_valid().

Thanks,

        tglx

[patch 13/20] genirq/msi: Make msi_add_simple_msi_descs() device domain aware

[Thomas Gleixner]

Allocating simple interrupt descriptors in the core code has to be multi
device aware for the upcoming PCI/IMS support.

Change the interfaces to take a domain id into account. Use the internal
control struct for transport of arguments.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/msi.c |   99 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 57 insertions(+), 42 deletions(-)

--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -154,39 +154,6 @@ int msi_domain_insert_msi_desc(struct de
 	return msi_insert_desc(dev, desc, domid, init_desc->msi_index);
 }
 
-/**
- * msi_add_simple_msi_descs - Allocate and initialize MSI descriptors
- * @dev:	Pointer to the device for which the descriptors are allocated
- * @index:	Index for the first MSI descriptor
- * @ndesc:	Number of descriptors to allocate
- *
- * Return: 0 on success or an appropriate failure code.
- */
-static int msi_add_simple_msi_descs(struct device *dev, unsigned int index, unsigned int ndesc)
-{
-	unsigned int idx, last = index + ndesc - 1;
-	struct msi_desc *desc;
-	int ret;
-
-	lockdep_assert_held(&dev->msi.data->mutex);
-
-	for (idx = index; idx <= last; idx++) {
-		desc = msi_alloc_desc(dev, 1, NULL);
-		if (!desc)
-			goto fail_mem;
-		ret = msi_insert_desc(dev, desc, MSI_DEFAULT_DOMAIN, idx);
-		if (ret)
-			goto fail;
-	}
-	return 0;
-
-fail_mem:
-	ret = -ENOMEM;
-fail:
-	msi_free_msi_descs_range(dev, index, last);
-	return ret;
-}
-
 static bool msi_desc_match(struct msi_desc *desc, enum msi_desc_filter filter)
 {
 	switch (filter) {
@@ -255,6 +222,45 @@ void msi_domain_free_msi_descs_range(str
 	msi_domain_free_descs(dev, &ctrl);
 }
 
+/**
+ * msi_domain_add_simple_msi_descs - Allocate and initialize MSI descriptors
+ * @dev:	Pointer to the device for which the descriptors are allocated
+ * @ctrl:	Allocation control struct
+ *
+ * Return: 0 on success or an appropriate failure code.
+ */
+static int msi_domain_add_simple_msi_descs(struct device *dev, struct msi_ctrl *ctrl)
+{
+	struct msi_desc *desc;
+	unsigned int idx;
+	int ret, baseidx;
+
+	lockdep_assert_held(&dev->msi.data->mutex);
+
+	if (!msi_ctrl_valid(dev, ctrl))
+		return -EINVAL;
+
+	baseidx = msi_get_domain_base_index(dev, ctrl->domid);
+	if (baseidx < 0)
+		return baseidx;
+
+	for (idx = ctrl->first; idx <= ctrl->last; idx++) {
+		desc = msi_alloc_desc(dev, 1, NULL);
+		if (!desc)
+			goto fail_mem;
+		ret = msi_insert_desc(dev, desc, ctrl->domid, idx);
+		if (ret)
+			goto fail;
+	}
+	return 0;
+
+fail_mem:
+	ret = -ENOMEM;
+fail:
+	msi_domain_free_descs(dev, ctrl);
+	return ret;
+}
+
 void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	*msg = entry->msg;
@@ -821,15 +827,19 @@ int msi_domain_populate_irqs(struct irq_
 {
 	struct msi_domain_info *info = domain->host_data;
 	struct msi_domain_ops *ops = info->ops;
+	struct msi_ctrl ctrl = {
+		.domid	= MSI_DEFAULT_DOMAIN,
+		.first  = virq_base,
+		.last	= virq_base + nvec - 1,
+	};
 	struct msi_desc *desc;
 	int ret, virq;
 
-	if (WARN_ON_ONCE(virq_base >= MSI_MAX_INDEX ||
-			 (virq_base + nvec) >= MSI_MAX_INDEX))
+	if (!msi_ctrl_valid(dev, &ctrl))
 		return 0;
 
 	msi_lock_descs(dev);
-	ret = msi_add_simple_msi_descs(dev, virq_base, nvec);
+	ret = msi_domain_add_simple_msi_descs(dev, &ctrl);
 	if (ret)
 		goto unlock;
 
@@ -850,7 +860,7 @@ int msi_domain_populate_irqs(struct irq_
 fail:
 	for (--virq; virq >= virq_base; virq--)
 		irq_domain_free_irqs_common(domain, virq, 1);
-	msi_free_msi_descs_range(dev, virq_base, virq_base + nvec - 1);
+	msi_domain_free_descs(dev, &ctrl);
 unlock:
 	msi_unlock_descs(dev);
 	return ret;
@@ -1024,14 +1034,19 @@ static int __msi_domain_alloc_irqs(struc
 	return 0;
 }
 
-static int msi_domain_add_simple_msi_descs(struct msi_domain_info *info,
-					   struct device *dev,
-					   unsigned int num_descs)
+static int msi_domain_alloc_simple_msi_descs(struct device *dev,
+					     struct msi_domain_info *info,
+					     unsigned int num_descs)
 {
+	struct msi_ctrl ctrl = {
+		.domid	= MSI_DEFAULT_DOMAIN,
+		.last	= num_descs - 1,
+	};
+
 	if (!(info->flags & MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS))
 		return 0;
 
-	return msi_add_simple_msi_descs(dev, 0, num_descs);
+	return msi_domain_add_simple_msi_descs(dev, &ctrl);
 }
 
 /**
@@ -1062,7 +1077,7 @@ int msi_domain_alloc_irqs_descs_locked(s
 	}
 
 	/* Frees allocated descriptors in case of failure. */
-	ret = msi_domain_add_simple_msi_descs(info, dev, nvec);
+	ret = msi_domain_alloc_simple_msi_descs(dev, info, nvec);
 	if (ret)
 		return ret;
 

RE: [patch 13/20] genirq/msi: Make msi_add_simple_msi_descs() device domain aware

[Tian, Kevin]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 11, 2022 9:57 PM
> 
> Allocating simple interrupt descriptors in the core code has to be multi
> device aware for the upcoming PCI/IMS support.
> 

"multi device irqdomains"

[patch 14/20] genirq/msi: Provide new domain id based interfaces for freeing interrupts

[Thomas Gleixner]

Provide two sorts of interfaces to handle the different use cases:

  - msi_domain_free_irqs_range():

	Handles a caller defined precise range

  - msi_domain_free_irqs_all():

	Frees all interrupts associated to a domain

The latter is useful for device teardown and to handle the legacy MSI support
which does not have any range information available.

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

--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -486,6 +486,15 @@ int msi_domain_alloc_irqs(struct irq_dom
 			  int nvec);
 void msi_domain_free_irqs_descs_locked(struct irq_domain *domain, struct device *dev);
 void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev);
+
+void msi_domain_free_irqs_range_locked(struct device *dev, unsigned int domid,
+				       unsigned int first, unsigned int last);
+void msi_domain_free_irqs_range(struct device *dev, unsigned int domid,
+				unsigned int first, unsigned int last);
+
+void msi_domain_free_irqs_all_locked(struct device *dev, unsigned int domid);
+void msi_domain_free_irqs_all(struct device *dev, unsigned int domid);
+
 struct msi_domain_info *msi_get_domain_info(struct irq_domain *domain);
 
 struct irq_domain *platform_msi_create_irq_domain(struct fwnode_handle *fwnode,
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -583,7 +583,25 @@ static inline void msi_sysfs_remove_desc
 #endif /* !CONFIG_SYSFS */
 
 static int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, int nvec);
-static void __msi_domain_free_irqs(struct irq_domain *domain, struct device *dev);
+
+static struct irq_domain *msi_get_device_domain(struct device *dev, unsigned int domid)
+{
+	struct irq_domain *domain;
+
+	lockdep_assert_held(&dev->msi.data->mutex);
+
+	if (WARN_ON_ONCE(domid >= MSI_MAX_DEVICE_IRQDOMAINS))
+		return NULL;
+
+	domain = dev->msi.data->__irqdomains[domid];
+	if (!domain)
+		return NULL;
+
+	if (WARN_ON_ONCE(irq_domain_is_msi_parent(domain)))
+		return NULL;
+
+	return domain;
+}
 
 static inline void irq_chip_write_msi_msg(struct irq_data *data,
 					  struct msi_msg *msg)
@@ -744,7 +762,6 @@ static struct msi_domain_ops msi_domain_
 	.msi_prepare		= msi_domain_ops_prepare,
 	.set_desc		= msi_domain_ops_set_desc,
 	.domain_alloc_irqs	= __msi_domain_alloc_irqs,
-	.domain_free_irqs	= __msi_domain_free_irqs,
 };
 
 static void msi_domain_update_dom_ops(struct msi_domain_info *info)
@@ -758,8 +775,6 @@ static void msi_domain_update_dom_ops(st
 
 	if (ops->domain_alloc_irqs == NULL)
 		ops->domain_alloc_irqs = msi_domain_ops_default.domain_alloc_irqs;
-	if (ops->domain_free_irqs == NULL)
-		ops->domain_free_irqs = msi_domain_ops_default.domain_free_irqs;
 
 	if (!(info->flags & MSI_FLAG_USE_DEF_DOM_OPS))
 		return;
@@ -1108,15 +1123,23 @@ int msi_domain_alloc_irqs(struct irq_dom
 	return ret;
 }
 
-static void __msi_domain_free_irqs(struct irq_domain *domain, struct device *dev)
+static void __msi_domain_free_irqs(struct device *dev, struct irq_domain *domain,
+				   struct msi_ctrl *ctrl)
 {
 	struct msi_domain_info *info = domain->host_data;
+	struct xarray *xa = &dev->msi.data->__store;
 	struct irq_data *irqd;
 	struct msi_desc *desc;
-	int i;
+	unsigned long idx;
+	int i, base;
+
+	base = ctrl->domid * MSI_XA_DOMAIN_SIZE;
+
+	xa_for_each_range(xa, idx, desc, ctrl->first + base, ctrl->last + base) {
+		/* Only handle MSI entries which have an interrupt associated */
+		if (!msi_desc_match(desc, MSI_DESC_ASSOCIATED))
+			continue;
 
-	/* Only handle MSI entries which have an interrupt associated */
-	msi_for_each_desc(desc, dev, MSI_DESC_ASSOCIATED) {
 		/* Make sure all interrupts are deactivated */
 		for (i = 0; i < desc->nvec_used; i++) {
 			irqd = irq_domain_get_irq_data(domain, desc->irq + i);
@@ -1131,11 +1154,99 @@ static void __msi_domain_free_irqs(struc
 	}
 }
 
-static void msi_domain_free_msi_descs(struct msi_domain_info *info,
-				      struct device *dev)
+static void msi_domain_free_locked(struct device *dev, struct msi_ctrl *ctrl)
 {
+	struct msi_domain_info *info;
+	struct msi_domain_ops *ops;
+	struct irq_domain *domain;
+
+	if (!msi_ctrl_valid(dev, ctrl))
+		return;
+
+	domain = msi_get_device_domain(dev, ctrl->domid);
+	if (!domain)
+		return;
+
+	info = domain->host_data;
+	ops = info->ops;
+
+	if (ops->domain_free_irqs)
+		ops->domain_free_irqs(domain, dev);
+	else
+		__msi_domain_free_irqs(dev, domain, ctrl);
+
+	if (ops->msi_post_free)
+		ops->msi_post_free(domain, dev);
+
 	if (info->flags & MSI_FLAG_FREE_MSI_DESCS)
-		msi_free_msi_descs(dev);
+		msi_domain_free_descs(dev, ctrl);
+}
+
+/**
+ * msi_domain_free_irqs_range_locked - Free a range of interrupts from a MSI interrupt domain
+ *				       associated to @dev with msi_lock held
+ * @dev:	Pointer to device struct of the device for which the interrupts
+ *		are freed
+ * @domid:	Id of the interrupt domain to operate on
+ * @first:	First index to free (inclusive)
+ * @last:	Last index to free (inclusive)
+ */
+void msi_domain_free_irqs_range_locked(struct device *dev, unsigned int domid,
+				       unsigned int first, unsigned int last)
+{
+	struct msi_ctrl ctrl = {
+		.domid	= domid,
+		.first	= first,
+		.last	= last,
+	};
+	msi_domain_free_locked(dev, &ctrl);
+}
+
+/**
+ * msi_domain_free_irqs_range - Free a range of interrupts from a MSI interrupt domain
+ *				associated to @dev
+ * @dev:	Pointer to device struct of the device for which the interrupts
+ *		are freed
+ * @domid:	Id of the interrupt domain to operate on
+ * @first:	First index to free (inclusive)
+ * @last:	Last index to free (inclusive)
+ */
+void msi_domain_free_irqs_range(struct device *dev, unsigned int domid,
+				unsigned int first, unsigned int last)
+{
+	msi_lock_descs(dev);
+	msi_domain_free_irqs_range_locked(dev, domid, first, last);
+	msi_unlock_descs(dev);
+}
+
+/**
+ * msi_domain_free_irqs_all_locked - Free all interrupts from a MSI interrupt
+ *				     associated to a device
+ * @dev:	Pointer to device struct of the device for which the interrupts
+ *		are freed
+ * @domid:	The id of the domain to operate on
+ *
+ * Must be invoked from within a msi_lock_descs() / msi_unlock_descs()
+ * pair. Use this for MSI irqdomains which implement their own vector
+ * allocation.
+ */
+void msi_domain_free_irqs_all_locked(struct device *dev, unsigned int domid)
+{
+	msi_domain_free_irqs_range_locked(dev, domid, 0, MSI_MAX_INDEX);
+}
+
+/**
+ * msi_domain_free_irqs_all - Free all interrupts from a MSI interrupt
+ *			      associated to a device
+ * @dev:	Pointer to device struct of the device for which the interrupts
+ *		are freed
+ * @domid:	The id of the domain to operate on
+ */
+void msi_domain_free_irqs_all(struct device *dev, unsigned int domid)
+{
+	msi_lock_descs(dev);
+	msi_domain_free_irqs_range_locked(dev, domid, 0, MSI_MAX_INDEX);
+	msi_unlock_descs(dev);
 }
 
 /**
@@ -1150,18 +1261,7 @@ static void msi_domain_free_msi_descs(st
  */
 void msi_domain_free_irqs_descs_locked(struct irq_domain *domain, struct device *dev)
 {
-	struct msi_domain_info *info = domain->host_data;
-	struct msi_domain_ops *ops = info->ops;
-
-	lockdep_assert_held(&dev->msi.data->mutex);
-
-	if (WARN_ON_ONCE(irq_domain_is_msi_parent(domain)))
-		return;
-
-	ops->domain_free_irqs(domain, dev);
-	if (ops->msi_post_free)
-		ops->msi_post_free(domain, dev);
-	msi_domain_free_msi_descs(info, dev);
+	msi_domain_free_irqs_range_locked(dev, MSI_DEFAULT_DOMAIN, 0, MSI_MAX_INDEX);
 }
 
 /**

RE: [patch 14/20] genirq/msi: Provide new domain id based interfaces for freeing interrupts

[Tian, Kevin]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 11, 2022 9:57 PM
> +/**
> + * msi_domain_free_irqs_all_locked - Free all interrupts from a MSI
> interrupt

from a MSI interrupt "domain"

> +
> +/**
> + * msi_domain_free_irqs_all - Free all interrupts from a MSI interrupt
> + *			      associated to a device

ditto

> -	ops->domain_free_irqs(domain, dev);
> -	if (ops->msi_post_free)
> -		ops->msi_post_free(domain, dev);
> -	msi_domain_free_msi_descs(info, dev);
> +	msi_domain_free_irqs_range_locked(dev, MSI_DEFAULT_DOMAIN, 0,
> MSI_MAX_INDEX);

msi_domain_free_irqs_all_locked(dev, MSI_DEFAULT_DOMAIN);

[patch 15/20] genirq/msi: Provide new domain id allocation functions

[Thomas Gleixner]

Provide two sorts of interfaces to handle the different use cases:

  - msi_domain_alloc_irqs_range():

	Handles a caller defined precise range

  - msi_domain_alloc_irqs_all():

	Allocates all interrupts associated to a domain by scanning the
    	allocated MSI descriptors

The latter is useful for the existing PCI/MSI support which does not have
range information available.

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

--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -371,8 +371,8 @@ struct msi_domain_info;
  * @get_hwirq, @msi_init and @msi_free are callbacks used by the underlying
  * irqdomain.
  *
- * @msi_check, @msi_prepare and @set_desc are callbacks used by
- * msi_domain_alloc/free_irqs().
+ * @msi_check, @msi_prepare and @set_desc are callbacks used by the
+ * msi_domain_alloc/free_irqs*() variants.
  *
  * @domain_alloc_irqs, @domain_free_irqs can be used to override the
  * default allocation/free functions (__msi_domain_alloc/free_irqs). This
@@ -380,11 +380,6 @@ struct msi_domain_info;
  * be wrapped into the regular irq domains concepts by mere mortals.  This
  * allows to universally use msi_domain_alloc/free_irqs without having to
  * special case XEN all over the place.
- *
- * Contrary to other operations @domain_alloc_irqs and @domain_free_irqs
- * are set to the default implementation if NULL and even when
- * MSI_FLAG_USE_DEF_DOM_OPS is not set to avoid breaking existing users and
- * because these callbacks are obviously mandatory.
  */
 struct msi_domain_ops {
 	irq_hw_number_t	(*get_hwirq)(struct msi_domain_info *info,
@@ -484,14 +479,21 @@ int msi_domain_alloc_irqs_descs_locked(s
 				       int nvec);
 int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
 			  int nvec);
+
 void msi_domain_free_irqs_descs_locked(struct irq_domain *domain, struct device *dev);
 void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev);
 
+int msi_domain_alloc_irqs_range_locked(struct device *dev, unsigned int domid,
+				       unsigned int first, unsigned int last);
+int msi_domain_alloc_irqs_range(struct device *dev, unsigned int domid,
+				unsigned int first, unsigned int last);
+int msi_domain_alloc_irqs_all_locked(struct device *dev, unsigned int domid, int nirqs);
+
+
 void msi_domain_free_irqs_range_locked(struct device *dev, unsigned int domid,
 				       unsigned int first, unsigned int last);
 void msi_domain_free_irqs_range(struct device *dev, unsigned int domid,
 				unsigned int first, unsigned int last);
-
 void msi_domain_free_irqs_all_locked(struct device *dev, unsigned int domid);
 void msi_domain_free_irqs_all(struct device *dev, unsigned int domid);
 
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -24,11 +24,14 @@
  * @domid:	ID of the domain on which management operations should be done
  * @first:	First (hardware) slot index to operate on
  * @last:	Last (hardware) slot index to operate on
+ * @nirqs:	The number of Linux interrupts to allocate. Can be larger
+ *		than the range due to PCI/multi-MSI.
  */
 struct msi_ctrl {
 	unsigned int			domid;
 	unsigned int			first;
 	unsigned int			last;
+	unsigned int			nirqs;
 };
 
 static inline int msi_sysfs_create_group(struct device *dev);
@@ -582,8 +585,6 @@ static inline int msi_sysfs_populate_des
 static inline void msi_sysfs_remove_desc(struct device *dev, struct msi_desc *desc) { }
 #endif /* !CONFIG_SYSFS */
 
-static int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, int nvec);
-
 static struct irq_domain *msi_get_device_domain(struct device *dev, unsigned int domid)
 {
 	struct irq_domain *domain;
@@ -597,7 +598,7 @@ static struct irq_domain *msi_get_device
 	if (!domain)
 		return NULL;
 
-	if (WARN_ON_ONCE(irq_domain_is_msi_parent(domain)))
+	if (irq_domain_is_msi_parent(domain))
 		return NULL;
 
 	return domain;
@@ -761,7 +762,6 @@ static struct msi_domain_ops msi_domain_
 	.msi_init		= msi_domain_ops_init,
 	.msi_prepare		= msi_domain_ops_prepare,
 	.set_desc		= msi_domain_ops_set_desc,
-	.domain_alloc_irqs	= __msi_domain_alloc_irqs,
 };
 
 static void msi_domain_update_dom_ops(struct msi_domain_info *info)
@@ -773,9 +773,6 @@ static void msi_domain_update_dom_ops(st
 		return;
 	}
 
-	if (ops->domain_alloc_irqs == NULL)
-		ops->domain_alloc_irqs = msi_domain_ops_default.domain_alloc_irqs;
-
 	if (!(info->flags & MSI_FLAG_USE_DEF_DOM_OPS))
 		return;
 
@@ -986,18 +983,19 @@ static int msi_init_virq(struct irq_doma
 	return 0;
 }
 
-static int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
-				   int nvec)
+static int __msi_domain_alloc_irqs(struct device *dev, struct irq_domain *domain,
+				   struct msi_ctrl *ctrl)
 {
 	struct msi_domain_info *info = domain->host_data;
+	struct xarray *xa = &dev->msi.data->__store;
 	struct msi_domain_ops *ops = info->ops;
+	unsigned int vflags = 0, allocated = 0;
 	msi_alloc_info_t arg = { };
-	unsigned int vflags = 0;
+	int i, ret, virq, base;
 	struct msi_desc *desc;
-	int allocated = 0;
-	int i, ret, virq;
+	unsigned long idx;
 
-	ret = msi_domain_prepare_irqs(domain, dev, nvec, &arg);
+	ret = msi_domain_prepare_irqs(domain, dev, ctrl->nirqs, &arg);
 	if (ret)
 		return ret;
 
@@ -1023,7 +1021,16 @@ static int __msi_domain_alloc_irqs(struc
 			vflags |= VIRQ_NOMASK_QUIRK;
 	}
 
-	msi_for_each_desc(desc, dev, MSI_DESC_NOTASSOCIATED) {
+	base = ctrl->domid * MSI_XA_DOMAIN_SIZE;
+
+	xa_for_each_range(xa, idx, desc, ctrl->first + base, ctrl->last + base) {
+		if (!msi_desc_match(desc, MSI_DESC_NOTASSOCIATED))
+			continue;
+
+		/* This should return -ECONFUSED... */
+		if (WARN_ON_ONCE(allocated >= ctrl->nirqs))
+			return -EINVAL;
+
 		ops->set_desc(&arg, desc);
 
 		virq = __irq_domain_alloc_irqs(domain, -1, desc->nvec_used,
@@ -1051,17 +1058,122 @@ static int __msi_domain_alloc_irqs(struc
 
 static int msi_domain_alloc_simple_msi_descs(struct device *dev,
 					     struct msi_domain_info *info,
-					     unsigned int num_descs)
+					     struct msi_ctrl *ctrl)
+{
+	if (!(info->flags & MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS))
+		return 0;
+
+	return msi_domain_add_simple_msi_descs(dev, ctrl);
+}
+
+static int __msi_domain_alloc_locked(struct device *dev, struct msi_ctrl *ctrl)
+{
+	struct msi_domain_info *info;
+	struct msi_domain_ops *ops;
+	struct irq_domain *domain;
+	int ret;
+
+	if (!msi_ctrl_valid(dev, ctrl))
+		return -EINVAL;
+
+	domain = msi_get_device_domain(dev, ctrl->domid);
+	if (!domain)
+		return -ENODEV;
+
+	info = domain->host_data;
+
+	ret = msi_domain_alloc_simple_msi_descs(dev, info, ctrl);
+	if (ret)
+		return ret;
+
+	ops = info->ops;
+	if (ops->domain_alloc_irqs)
+		return ops->domain_alloc_irqs(domain, dev, ctrl->nirqs);
+
+	return __msi_domain_alloc_irqs(dev, domain, ctrl);
+}
+
+static int msi_domain_alloc_locked(struct device *dev, struct msi_ctrl *ctrl)
+{
+	int ret = __msi_domain_alloc_locked(dev, ctrl);
+
+	if (ret)
+		msi_domain_free_descs(dev, ctrl);
+	return ret;
+}
+
+/**
+ * msi_domain_alloc_irqs_range_locked - Allocate interrupts from a MSI interrupt domain
+ * @dev:	Pointer to device struct of the device for which the interrupts
+ *		are allocated
+ * @domid:	Id of the interrupt domain to operate on
+ * @first:	First index to allocate (inclusive)
+ * @last:	Last index to allocate (inclusive)
+ *
+ * Must be invoked from within a msi_lock_descs() / msi_unlock_descs()
+ * pair. Use this for MSI irqdomains which implement their own descriptor
+ * allocation/free.
+ *
+ * Return: %0 on success or an error code.
+ */
+int msi_domain_alloc_irqs_range_locked(struct device *dev, unsigned int domid,
+				       unsigned int first, unsigned int last)
 {
 	struct msi_ctrl ctrl = {
-		.domid	= MSI_DEFAULT_DOMAIN,
-		.last	= num_descs - 1,
+		.domid	= domid,
+		.first	= first,
+		.last	= last,
+		.nirqs	= last + 1 - first,
 	};
 
-	if (!(info->flags & MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS))
-		return 0;
+	return msi_domain_alloc_locked(dev, &ctrl);
+}
+
+/**
+ * msi_domain_alloc_irqs_range - Allocate interrupts from a MSI interrupt domain
+ * @dev:	Pointer to device struct of the device for which the interrupts
+ *		are allocated
+ * @domid:	Id of the interrupt domain to operate on
+ * @first:	First index to allocate (inclusive)
+ * @last:	Last index to allocate (inclusive)
+ *
+ * Return: %0 on success or an error code.
+ */
+int msi_domain_alloc_irqs_range(struct device *dev, unsigned int domid,
+				unsigned int first, unsigned int last)
+{
+	int ret;
+
+	msi_lock_descs(dev);
+	ret = msi_domain_alloc_irqs_range_locked(dev, domid, first, last);
+	msi_unlock_descs(dev);
+	return ret;
+}
+
+/**
+ * msi_domain_alloc_irqs_all_locked - Allocate all interrupts from a MSI interrupt domain
+ *
+ * @dev:	Pointer to device struct of the device for which the interrupts
+ *		are allocated
+ * @domid:	Id of the interrupt domain to operate on
+ * @nirqs:	The number of interrupts to allocate
+ *
+ * This function scans all MSI descriptors of the MSI domain and allocates interrupts
+ * for all unassigned ones. That function is to be used for MSI domain usage where
+ * the descriptor allocation is handled at the call site, e.g. PCI/MSI[X].
+ *
+ * Return: %0 on success or an error code.
+ */
+int msi_domain_alloc_irqs_all_locked(struct device *dev, unsigned int domid, int nirqs)
+{
+	struct msi_ctrl ctrl = {
+		.domid	= domid,
+		.first	= 0,
+		.last	= MSI_MAX_INDEX,
+		.nirqs	= nirqs,
+	};
 
-	return msi_domain_add_simple_msi_descs(dev, &ctrl);
+	return msi_domain_alloc_locked(dev, &ctrl);
 }
 
 /**
@@ -1080,28 +1192,14 @@ static int msi_domain_alloc_simple_msi_d
 int msi_domain_alloc_irqs_descs_locked(struct irq_domain *domain, struct device *dev,
 				       int nvec)
 {
-	struct msi_domain_info *info = domain->host_data;
-	struct msi_domain_ops *ops = info->ops;
-	int ret;
-
-	lockdep_assert_held(&dev->msi.data->mutex);
-
-	if (WARN_ON_ONCE(irq_domain_is_msi_parent(domain))) {
-		ret = -EINVAL;
-		goto free;
-	}
-
-	/* Frees allocated descriptors in case of failure. */
-	ret = msi_domain_alloc_simple_msi_descs(dev, info, nvec);
-	if (ret)
-		return ret;
+	struct msi_ctrl ctrl = {
+		.domid	= MSI_DEFAULT_DOMAIN,
+		.first	= 0,
+		.last	= MSI_MAX_INDEX,
+		.nirqs	= nvec,
+	};
 
-	ret = ops->domain_alloc_irqs(domain, dev, nvec);
-	if (!ret)
-		return 0;
-free:
-	msi_domain_free_irqs_descs_locked(domain, dev);
-	return ret;
+	return msi_domain_alloc_locked(dev, &ctrl);
 }
 
 /**

RE: [patch 15/20] genirq/msi: Provide new domain id allocation functions

[Tian, Kevin]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 11, 2022 9:57 PM
> 
> @@ -597,7 +598,7 @@ static struct irq_domain *msi_get_device
>  	if (!domain)
>  		return NULL;
> 
> -	if (WARN_ON_ONCE(irq_domain_is_msi_parent(domain)))
> +	if (irq_domain_is_msi_parent(domain))
>  		return NULL;

Is it intended to remove the warning here? If yes, what specific change
in this patch leads to that removal?

> +	xa_for_each_range(xa, idx, desc, ctrl->first + base, ctrl->last + base) {
> +		if (!msi_desc_match(desc, MSI_DESC_NOTASSOCIATED))
> +			continue;
> +
> +		/* This should return -ECONFUSED... */
> +		if (WARN_ON_ONCE(allocated >= ctrl->nirqs))
> +			return -EINVAL;
> +

why is "==" an error?

RE: [patch 15/20] genirq/msi: Provide new domain id allocation functions

[Thomas Gleixner]

On Fri, Nov 18 2022 at 08:43, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>> Sent: Friday, November 11, 2022 9:57 PM
>> 
>> @@ -597,7 +598,7 @@ static struct irq_domain *msi_get_device
>>  	if (!domain)
>>  		return NULL;
>> 
>> -	if (WARN_ON_ONCE(irq_domain_is_msi_parent(domain)))
>> +	if (irq_domain_is_msi_parent(domain))
>>  		return NULL;
>
> Is it intended to remove the warning here? If yes, what specific change
> in this patch leads to that removal?

Valid question

>> +	xa_for_each_range(xa, idx, desc, ctrl->first + base, ctrl->last + base) {
>> +		if (!msi_desc_match(desc, MSI_DESC_NOTASSOCIATED))
>> +			continue;
>> +
>> +		/* This should return -ECONFUSED... */
>> +		if (WARN_ON_ONCE(allocated >= ctrl->nirqs))
>> +			return -EINVAL;
>> +
>
> why is "==" an error?

because if you get here _after_ having allocated all interrupts already
then you have more descriptors than what you want to allocate, which
should never happen right?

Thanks,

        tglx

RE: [patch 15/20] genirq/msi: Provide new domain id allocation functions

[Tian, Kevin]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 18, 2022 8:26 PM
> 
> >> +	xa_for_each_range(xa, idx, desc, ctrl->first + base, ctrl->last + base) {
> >> +		if (!msi_desc_match(desc, MSI_DESC_NOTASSOCIATED))
> >> +			continue;
> >> +
> >> +		/* This should return -ECONFUSED... */
> >> +		if (WARN_ON_ONCE(allocated >= ctrl->nirqs))
> >> +			return -EINVAL;
> >> +
> >
> > why is "==" an error?
> 
> because if you get here _after_ having allocated all interrupts already
> then you have more descriptors than what you want to allocate, which
> should never happen right?

yes

[patch 16/20] PCI/MSI: Use msi_domain_alloc/free_irqs_all_locked()

[Thomas Gleixner]

Switch to the new domain id aware interfaces to phase out the previous
ones. No functional change.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/pci/msi/irqdomain.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/pci/msi/irqdomain.c
+++ b/drivers/pci/msi/irqdomain.c
@@ -14,7 +14,7 @@ int pci_msi_setup_msi_irqs(struct pci_de
 
 	domain = dev_get_msi_domain(&dev->dev);
 	if (domain && irq_domain_is_hierarchy(domain))
-		return msi_domain_alloc_irqs_descs_locked(domain, &dev->dev, nvec);
+		return msi_domain_alloc_irqs_all_locked(&dev->dev, MSI_DEFAULT_DOMAIN, nvec);
 
 	return pci_msi_legacy_setup_msi_irqs(dev, nvec, type);
 }
@@ -25,7 +25,7 @@ void pci_msi_teardown_msi_irqs(struct pc
 
 	domain = dev_get_msi_domain(&dev->dev);
 	if (domain && irq_domain_is_hierarchy(domain)) {
-		msi_domain_free_irqs_descs_locked(domain, &dev->dev);
+		msi_domain_free_irqs_all_locked(&dev->dev, MSI_DEFAULT_DOMAIN);
 	} else {
 		pci_msi_legacy_teardown_msi_irqs(dev);
 		msi_free_msi_descs(&dev->dev);

Re: [patch 16/20] PCI/MSI: Use msi_domain_alloc/free_irqs_all_locked()

[Bjorn Helgaas]

On Fri, Nov 11, 2022 at 02:57:03PM +0100, Thomas Gleixner wrote:
> Switch to the new domain id aware interfaces to phase out the previous
> ones. No functional change.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/msi/irqdomain.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/drivers/pci/msi/irqdomain.c
> +++ b/drivers/pci/msi/irqdomain.c
> @@ -14,7 +14,7 @@ int pci_msi_setup_msi_irqs(struct pci_de
>  
>  	domain = dev_get_msi_domain(&dev->dev);
>  	if (domain && irq_domain_is_hierarchy(domain))
> -		return msi_domain_alloc_irqs_descs_locked(domain, &dev->dev, nvec);
> +		return msi_domain_alloc_irqs_all_locked(&dev->dev, MSI_DEFAULT_DOMAIN, nvec);
>  
>  	return pci_msi_legacy_setup_msi_irqs(dev, nvec, type);
>  }
> @@ -25,7 +25,7 @@ void pci_msi_teardown_msi_irqs(struct pc
>  
>  	domain = dev_get_msi_domain(&dev->dev);
>  	if (domain && irq_domain_is_hierarchy(domain)) {
> -		msi_domain_free_irqs_descs_locked(domain, &dev->dev);
> +		msi_domain_free_irqs_all_locked(&dev->dev, MSI_DEFAULT_DOMAIN);
>  	} else {
>  		pci_msi_legacy_teardown_msi_irqs(dev);
>  		msi_free_msi_descs(&dev->dev);
> 

[patch 17/20] platform-msi: Switch to the domain id aware MSI interfaces

[Thomas Gleixner]

From: Ahmed S. Darwish <darwi@linutronix.de>

Switch to the new domain id aware interfaces to phase out the previous
ones. No functional change.

Signed-off-by: Ahmed S. Darwish <darwi@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/base/platform-msi.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -213,7 +213,7 @@ int platform_msi_domain_alloc_irqs(struc
 	if (err)
 		return err;
 
-	err = msi_domain_alloc_irqs(dev->msi.domain, dev, nvec);
+	err = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN, 0, nvec - 1);
 	if (err)
 		platform_msi_free_priv_data(dev);
 
@@ -227,7 +227,7 @@ EXPORT_SYMBOL_GPL(platform_msi_domain_al
  */
 void platform_msi_domain_free_irqs(struct device *dev)
 {
-	msi_domain_free_irqs(dev->msi.domain, dev);
+	msi_domain_free_irqs_all(dev, MSI_DEFAULT_DOMAIN);
 	platform_msi_free_priv_data(dev);
 }
 EXPORT_SYMBOL_GPL(platform_msi_domain_free_irqs);

RE: [patch 17/20] platform-msi: Switch to the domain id aware MSI interfaces

[Tian, Kevin]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 11, 2022 9:57 PM
> 
> From: Ahmed S. Darwish <darwi@linutronix.de>
> 
> Switch to the new domain id aware interfaces to phase out the previous
> ones. No functional change.
> 
> Signed-off-by: Ahmed S. Darwish <darwi@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/base/platform-msi.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/drivers/base/platform-msi.c
> +++ b/drivers/base/platform-msi.c
> @@ -213,7 +213,7 @@ int platform_msi_domain_alloc_irqs(struc
>  	if (err)
>  		return err;
> 
> -	err = msi_domain_alloc_irqs(dev->msi.domain, dev, nvec);
> +	err = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN, 0,
> nvec - 1);
>  	if (err)
>  		platform_msi_free_priv_data(dev);
> 

Out of curiosity. Why don't we provide an unlocked version of
msi_domain_alloc_irqs_all()?

RE: [patch 17/20] platform-msi: Switch to the domain id aware MSI interfaces

[Thomas Gleixner]

On Fri, Nov 18 2022 at 08:53, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>> 
>> -	err = msi_domain_alloc_irqs(dev->msi.domain, dev, nvec);
>> +	err = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN, 0,
>> nvec - 1);
>>  	if (err)
>>  		platform_msi_free_priv_data(dev);
>> 
>
> Out of curiosity. Why don't we provide an unlocked version of
> msi_domain_alloc_irqs_all()?

-ENOUSER

RE: [patch 17/20] platform-msi: Switch to the domain id aware MSI interfaces

[Tian, Kevin]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, November 18, 2022 8:27 PM
> 
> On Fri, Nov 18 2022 at 08:53, Kevin Tian wrote:
> >> From: Thomas Gleixner <tglx@linutronix.de>
> >>
> >> -	err = msi_domain_alloc_irqs(dev->msi.domain, dev, nvec);
> >> +	err = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN, 0,
> >> nvec - 1);
> >>  	if (err)
> >>  		platform_msi_free_priv_data(dev);
> >>
> >
> > Out of curiosity. Why don't we provide an unlocked version of
> > msi_domain_alloc_irqs_all()?
> 
> -ENOUSER

msi_domain_alloc_irqs() and msi_domain_alloc_irqs_descs_locked()
are a pair.

What I didn't get was why the unlocked invocation in this patch
is replaced by a range-based helper while the locked invocation
in previous patch16 was replaced by an all-based helper:

	if (domain && irq_domain_is_hierarchy(domain))
-		return msi_domain_alloc_irqs_descs_locked(domain, &dev->dev, nvec);
+		return msi_domain_alloc_irqs_all_locked(&dev->dev, MSI_DEFAULT_DOMAIN, nvec);

The reason could probably be marked out in the commit msg.

RE: [patch 17/20] platform-msi: Switch to the domain id aware MSI interfaces

[Thomas Gleixner]

On Mon, Nov 21 2022 at 03:42, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>> On Fri, Nov 18 2022 at 08:53, Kevin Tian wrote:
>> > Out of curiosity. Why don't we provide an unlocked version of
>> > msi_domain_alloc_irqs_all()?
>> 
>> -ENOUSER
>
> msi_domain_alloc_irqs() and msi_domain_alloc_irqs_descs_locked()
> are a pair.

Sure, but if there is no use case why should we provide the interface?

> What I didn't get was why the unlocked invocation in this patch
> is replaced by a range-based helper while the locked invocation
> in previous patch16 was replaced by an all-based helper:
>
> 	if (domain && irq_domain_is_hierarchy(domain))
> -		return msi_domain_alloc_irqs_descs_locked(domain, &dev->dev, nvec);
> +		return msi_domain_alloc_irqs_all_locked(&dev->dev, MSI_DEFAULT_DOMAIN, nvec);
>
> The reason could probably be marked out in the commit msg.

The point is that range based is obviously the better choice because
it's precise. Especially for domains which let the core code allocate
the MSI descriptors a precise range is required. The old interface was
kinda blury there.

In case of PCI/MSI[-X] the MSI descriptors are allocated by the PCI core
upfront and there are allocations which have gaps in the indices, so the
range is not well defined and we just keep using the existing scan all
mechanism.

Thanks,

        tglx

[patch 18/20] bus: fsl-mc-msi: Switch to domain id aware interfaces

[Thomas Gleixner]

Switch to the new domain id aware interfaces to phase out the previous
ones.

Get rid of the MSI descriptor and domain checks as the core code detects
these issues anyway.

No functional change.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/bus/fsl-mc/fsl-mc-msi.c |   25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

--- a/drivers/bus/fsl-mc/fsl-mc-msi.c
+++ b/drivers/bus/fsl-mc/fsl-mc-msi.c
@@ -213,21 +213,8 @@ struct irq_domain *fsl_mc_find_msi_domai
 
 int fsl_mc_msi_domain_alloc_irqs(struct device *dev,  unsigned int irq_count)
 {
-	struct irq_domain *msi_domain;
-	int error;
+	int error = msi_setup_device_data(dev);
 
-	msi_domain = dev_get_msi_domain(dev);
-	if (!msi_domain)
-		return -EINVAL;
-
-	error = msi_setup_device_data(dev);
-	if (error)
-		return error;
-
-	msi_lock_descs(dev);
-	if (msi_first_desc(dev, MSI_DESC_ALL))
-		error = -EINVAL;
-	msi_unlock_descs(dev);
 	if (error)
 		return error;
 
@@ -235,7 +222,7 @@ int fsl_mc_msi_domain_alloc_irqs(struct
 	 * NOTE: Calling this function will trigger the invocation of the
 	 * its_fsl_mc_msi_prepare() callback
 	 */
-	error = msi_domain_alloc_irqs(msi_domain, dev, irq_count);
+	error = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN, 0, irq_count - 1);
 
 	if (error)
 		dev_err(dev, "Failed to allocate IRQs\n");
@@ -244,11 +231,5 @@ int fsl_mc_msi_domain_alloc_irqs(struct
 
 void fsl_mc_msi_domain_free_irqs(struct device *dev)
 {
-	struct irq_domain *msi_domain;
-
-	msi_domain = dev_get_msi_domain(dev);
-	if (!msi_domain)
-		return;
-
-	msi_domain_free_irqs(msi_domain, dev);
+	msi_domain_free_irqs_all(dev, MSI_DEFAULT_DOMAIN);
 }

[patch 19/20] oc: ti: ti_sci_inta_msi: Switch to domain id aware MSI functions

[Thomas Gleixner]

From: Ahmed S. Darwish <darwi@linutronix.de>

Switch to the new domain id aware interfaces to phase out the previous
ones. Remove the domain check as it happens in the core code now.

No functional change.

Signed-off-by: Ahmed S. Darwish <darwi@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/soc/ti/ti_sci_inta_msi.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

--- a/drivers/soc/ti/ti_sci_inta_msi.c
+++ b/drivers/soc/ti/ti_sci_inta_msi.c
@@ -93,13 +93,8 @@ int ti_sci_inta_msi_domain_alloc_irqs(st
 				      struct ti_sci_resource *res)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-	struct irq_domain *msi_domain;
 	int ret, nvec;
 
-	msi_domain = dev_get_msi_domain(dev);
-	if (!msi_domain)
-		return -EINVAL;
-
 	if (pdev->id < 0)
 		return -ENODEV;
 
@@ -114,7 +109,8 @@ int ti_sci_inta_msi_domain_alloc_irqs(st
 		goto unlock;
 	}
 
-	ret = msi_domain_alloc_irqs_descs_locked(msi_domain, dev, nvec);
+	/* Use alloc ALL as it's unclear whether there are gaps in the indices */
+	ret = msi_domain_alloc_irqs_all_locked(dev, MSI_DEFAULT_DOMAIN, nvec);
 	if (ret)
 		dev_err(dev, "Failed to allocate IRQs %d\n", ret);
 unlock:

[patch 20/20] genirq/msi: Remove unused alloc/free interfaces

[Thomas Gleixner]

Now that all users are converted remove the old interfaces.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/msi.h |    7 ----
 kernel/irq/msi.c    |   73 ----------------------------------------------------
 2 files changed, 80 deletions(-)

--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -475,13 +475,6 @@ int msi_domain_set_affinity(struct irq_d
 struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode,
 					 struct msi_domain_info *info,
 					 struct irq_domain *parent);
-int msi_domain_alloc_irqs_descs_locked(struct irq_domain *domain, struct device *dev,
-				       int nvec);
-int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
-			  int nvec);
-
-void msi_domain_free_irqs_descs_locked(struct irq_domain *domain, struct device *dev);
-void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev);
 
 int msi_domain_alloc_irqs_range_locked(struct device *dev, unsigned int domid,
 				       unsigned int first, unsigned int last);
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -1176,51 +1176,6 @@ int msi_domain_alloc_irqs_all_locked(str
 	return msi_domain_alloc_locked(dev, &ctrl);
 }
 
-/**
- * msi_domain_alloc_irqs_descs_locked - Allocate interrupts from a MSI interrupt domain
- * @domain:	The domain to allocate from
- * @dev:	Pointer to device struct of the device for which the interrupts
- *		are allocated
- * @nvec:	The number of interrupts to allocate
- *
- * Must be invoked from within a msi_lock_descs() / msi_unlock_descs()
- * pair. Use this for MSI irqdomains which implement their own vector
- * allocation/free.
- *
- * Return: %0 on success or an error code.
- */
-int msi_domain_alloc_irqs_descs_locked(struct irq_domain *domain, struct device *dev,
-				       int nvec)
-{
-	struct msi_ctrl ctrl = {
-		.domid	= MSI_DEFAULT_DOMAIN,
-		.first	= 0,
-		.last	= MSI_MAX_INDEX,
-		.nirqs	= nvec,
-	};
-
-	return msi_domain_alloc_locked(dev, &ctrl);
-}
-
-/**
- * msi_domain_alloc_irqs - Allocate interrupts from a MSI interrupt domain
- * @domain:	The domain to allocate from
- * @dev:	Pointer to device struct of the device for which the interrupts
- *		are allocated
- * @nvec:	The number of interrupts to allocate
- *
- * Return: %0 on success or an error code.
- */
-int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, int nvec)
-{
-	int ret;
-
-	msi_lock_descs(dev);
-	ret = msi_domain_alloc_irqs_descs_locked(domain, dev, nvec);
-	msi_unlock_descs(dev);
-	return ret;
-}
-
 static void __msi_domain_free_irqs(struct device *dev, struct irq_domain *domain,
 				   struct msi_ctrl *ctrl)
 {
@@ -1347,34 +1302,6 @@ void msi_domain_free_irqs_all(struct dev
 	msi_unlock_descs(dev);
 }
 
-/**
- * msi_domain_free_irqs_descs_locked - Free interrupts from a MSI interrupt @domain associated to @dev
- * @domain:	The domain to managing the interrupts
- * @dev:	Pointer to device struct of the device for which the interrupts
- *		are free
- *
- * Must be invoked from within a msi_lock_descs() / msi_unlock_descs()
- * pair. Use this for MSI irqdomains which implement their own vector
- * allocation.
- */
-void msi_domain_free_irqs_descs_locked(struct irq_domain *domain, struct device *dev)
-{
-	msi_domain_free_irqs_range_locked(dev, MSI_DEFAULT_DOMAIN, 0, MSI_MAX_INDEX);
-}
-
-/**
- * msi_domain_free_irqs - Free interrupts from a MSI interrupt @domain associated to @dev
- * @domain:	The domain to managing the interrupts
- * @dev:	Pointer to device struct of the device for which the interrupts
- *		are free
- */
-void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev)
-{
-	msi_lock_descs(dev);
-	msi_domain_free_irqs_descs_locked(domain, dev);
-	msi_unlock_descs(dev);
-}
-
 /**
  * msi_get_domain_info - Get the MSI interrupt domain info for @domain
  * @domain:	The interrupt domain to retrieve data from