[PATCH v2 1/2] msi: The MSI framework only supports 2048 platform MSIs

[PATCH v2 1/2] msi: The MSI framework only supports 2048 platform MSIs

[Julian Vetter]

Some devices need more MSIs. To support this the number must be
increased.

Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
---
(no changes since v1)

 drivers/base/platform-msi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index 296ea673d661..4b0b2fe3a7ff 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -13,7 +13,7 @@
 #include <linux/msi.h>
 #include <linux/slab.h>
 
-#define DEV_ID_SHIFT	21
+#define DEV_ID_SHIFT	19
 #define MAX_DEV_MSIS	(1 << (32 - DEV_ID_SHIFT))
 
 /*
-- 
2.17.1

[PATCH v2 2/2] msi: Add sanity check if more than MAX_DEV_MSIS MSIs are requested

[Julian Vetter]

If a device requests more than MAX_DEV_MSIS the MSI index will collide with
the devid and might cause Linux to compute twice the same virtual interrupt
number for two different devices.

Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
---
Changes v1->v2:
  - Took Greg Kroah-Hartman's comments into account
  - Replaced WARN_ON() by proper error handling

 drivers/base/platform-msi.c       |  8 +++++++-
 drivers/bus/fsl-mc/fsl-mc-msi.c   |  3 ++-
 drivers/irqchip/irq-ti-sci-inta.c |  3 ++-
 drivers/pci/controller/vmd.c      |  3 ++-
 drivers/pci/msi/irqdomain.c       |  3 ++-
 include/linux/msi.h               |  2 +-
 kernel/irq/msi.c                  | 12 +++++++++---
 7 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index 4b0b2fe3a7ff..8330153c0067 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -43,10 +43,16 @@ static irq_hw_number_t platform_msi_calc_hwirq(struct msi_desc *desc)
 	return (devid << (32 - DEV_ID_SHIFT)) | desc->msi_index;
 }
 
-static void platform_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
+static int platform_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
 {
 	arg->desc = desc;
 	arg->hwirq = platform_msi_calc_hwirq(desc);
+
+	if (desc->msi_index >= MAX_DEV_MSIS) {
+		dev_err(desc->dev, "Number of msis exceeds %d\n", MAX_DEV_MSIS);
+		return -EINVAL;
+	}
+	return 0;
 }
 
 static int platform_msi_init(struct irq_domain *domain,
diff --git a/drivers/bus/fsl-mc/fsl-mc-msi.c b/drivers/bus/fsl-mc/fsl-mc-msi.c
index 0cfe859a4ac4..63b5ae8de391 100644
--- a/drivers/bus/fsl-mc/fsl-mc-msi.c
+++ b/drivers/bus/fsl-mc/fsl-mc-msi.c
@@ -32,12 +32,13 @@ static irq_hw_number_t fsl_mc_domain_calc_hwirq(struct fsl_mc_device *dev,
 	return (irq_hw_number_t)(desc->msi_index + (dev->icid * 10000));
 }
 
-static void fsl_mc_msi_set_desc(msi_alloc_info_t *arg,
+static int fsl_mc_msi_set_desc(msi_alloc_info_t *arg,
 				struct msi_desc *desc)
 {
 	arg->desc = desc;
 	arg->hwirq = fsl_mc_domain_calc_hwirq(to_fsl_mc_device(desc->dev),
 					      desc);
+	return 0;
 }
 #else
 #define fsl_mc_msi_set_desc NULL
diff --git a/drivers/irqchip/irq-ti-sci-inta.c b/drivers/irqchip/irq-ti-sci-inta.c
index 5fdbb4358dd0..0c26690c074b 100644
--- a/drivers/irqchip/irq-ti-sci-inta.c
+++ b/drivers/irqchip/irq-ti-sci-inta.c
@@ -589,13 +589,14 @@ static struct irq_chip ti_sci_inta_msi_irq_chip = {
 	.flags			= IRQCHIP_SUPPORTS_LEVEL_MSI,
 };
 
-static void ti_sci_inta_msi_set_desc(msi_alloc_info_t *arg,
+static int ti_sci_inta_msi_set_desc(msi_alloc_info_t *arg,
 				     struct msi_desc *desc)
 {
 	struct platform_device *pdev = to_platform_device(desc->dev);
 
 	arg->desc = desc;
 	arg->hwirq = TO_HWIRQ(pdev->id, desc->msi_index);
+	return 0;
 }
 
 static struct msi_domain_ops ti_sci_inta_msi_ops = {
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 94a14a3d7e55..929ba2a991b5 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -296,9 +296,10 @@ static int vmd_msi_prepare(struct irq_domain *domain, struct device *dev,
 	return 0;
 }
 
-static void vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
+static int vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
 {
 	arg->desc = desc;
+	return 0;
 }
 
 static struct msi_domain_ops vmd_msi_domain_ops = {
diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
index e9cf318e6670..3191f15a07b8 100644
--- a/drivers/pci/msi/irqdomain.c
+++ b/drivers/pci/msi/irqdomain.c
@@ -108,11 +108,12 @@ static int pci_msi_domain_check_cap(struct irq_domain *domain,
 	return 0;
 }
 
-static void pci_msi_domain_set_desc(msi_alloc_info_t *arg,
+static int pci_msi_domain_set_desc(msi_alloc_info_t *arg,
 				    struct msi_desc *desc)
 {
 	arg->desc = desc;
 	arg->hwirq = pci_msi_domain_calc_hwirq(desc);
+	return 0;
 }
 
 static struct msi_domain_ops pci_msi_domain_ops_default = {
diff --git a/include/linux/msi.h b/include/linux/msi.h
index fc918a658d48..17228d6e8d85 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -355,7 +355,7 @@ struct msi_domain_ops {
 	int		(*msi_prepare)(struct irq_domain *domain,
 				       struct device *dev, int nvec,
 				       msi_alloc_info_t *arg);
-	void		(*set_desc)(msi_alloc_info_t *arg,
+	int		(*set_desc)(msi_alloc_info_t *arg,
 				    struct msi_desc *desc);
 	int		(*domain_alloc_irqs)(struct irq_domain *domain,
 					     struct device *dev, int nvec);
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index a9ee535293eb..a20c85334b58 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -592,10 +592,11 @@ static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev,
 	return 0;
 }
 
-static void msi_domain_ops_set_desc(msi_alloc_info_t *arg,
+static int msi_domain_ops_set_desc(msi_alloc_info_t *arg,
 				    struct msi_desc *desc)
 {
 	arg->desc = desc;
+	return 0;
 }
 
 static int msi_domain_ops_init(struct irq_domain *domain,
@@ -726,7 +727,10 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
 		desc = xa_load(&dev->msi.data->__store, virq);
 		desc->irq = virq;
 
-		ops->set_desc(arg, desc);
+		ret = ops->set_desc(arg, desc);
+		if (ret)
+			goto fail;
+
 		ret = irq_domain_alloc_irqs_hierarchy(domain, virq, 1, arg);
 		if (ret)
 			goto fail;
@@ -888,7 +892,9 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
 	}
 
 	msi_for_each_desc(desc, dev, MSI_DESC_NOTASSOCIATED) {
-		ops->set_desc(&arg, desc);
+		ret = ops->set_desc(&arg, desc);
+		if (ret)
+			return ret;
 
 		virq = __irq_domain_alloc_irqs(domain, -1, desc->nvec_used,
 					       dev_to_node(dev), &arg, false,
-- 
2.17.1

Re: [PATCH v2 2/2] msi: Add sanity check if more than MAX_DEV_MSIS MSIs are requested

[Greg KH]

On Tue, Jun 14, 2022 at 05:14:11PM +0200, Julian Vetter wrote:
> If a device requests more than MAX_DEV_MSIS the MSI index will collide with
> the devid and might cause Linux to compute twice the same virtual interrupt
> number for two different devices.
> 
> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
> Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
> ---
> Changes v1->v2:
>   - Took Greg Kroah-Hartman's comments into account
>   - Replaced WARN_ON() by proper error handling
> 
>  drivers/base/platform-msi.c       |  8 +++++++-
>  drivers/bus/fsl-mc/fsl-mc-msi.c   |  3 ++-
>  drivers/irqchip/irq-ti-sci-inta.c |  3 ++-
>  drivers/pci/controller/vmd.c      |  3 ++-
>  drivers/pci/msi/irqdomain.c       |  3 ++-
>  include/linux/msi.h               |  2 +-
>  kernel/irq/msi.c                  | 12 +++++++++---
>  7 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
> index 4b0b2fe3a7ff..8330153c0067 100644
> --- a/drivers/base/platform-msi.c
> +++ b/drivers/base/platform-msi.c
> @@ -43,10 +43,16 @@ static irq_hw_number_t platform_msi_calc_hwirq(struct msi_desc *desc)
>  	return (devid << (32 - DEV_ID_SHIFT)) | desc->msi_index;
>  }
>  
> -static void platform_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
> +static int platform_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
>  {
>  	arg->desc = desc;
>  	arg->hwirq = platform_msi_calc_hwirq(desc);
> +
> +	if (desc->msi_index >= MAX_DEV_MSIS) {
> +		dev_err(desc->dev, "Number of msis exceeds %d\n", MAX_DEV_MSIS);
> +		return -EINVAL;
> +	}
> +	return 0;
>  }
>  
>  static int platform_msi_init(struct irq_domain *domain,
> diff --git a/drivers/bus/fsl-mc/fsl-mc-msi.c b/drivers/bus/fsl-mc/fsl-mc-msi.c
> index 0cfe859a4ac4..63b5ae8de391 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-msi.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-msi.c
> @@ -32,12 +32,13 @@ static irq_hw_number_t fsl_mc_domain_calc_hwirq(struct fsl_mc_device *dev,
>  	return (irq_hw_number_t)(desc->msi_index + (dev->icid * 10000));
>  }
>  
> -static void fsl_mc_msi_set_desc(msi_alloc_info_t *arg,
> +static int fsl_mc_msi_set_desc(msi_alloc_info_t *arg,
>  				struct msi_desc *desc)
>  {
>  	arg->desc = desc;
>  	arg->hwirq = fsl_mc_domain_calc_hwirq(to_fsl_mc_device(desc->dev),
>  					      desc);
> +	return 0;
>  }
>  #else
>  #define fsl_mc_msi_set_desc NULL
> diff --git a/drivers/irqchip/irq-ti-sci-inta.c b/drivers/irqchip/irq-ti-sci-inta.c
> index 5fdbb4358dd0..0c26690c074b 100644
> --- a/drivers/irqchip/irq-ti-sci-inta.c
> +++ b/drivers/irqchip/irq-ti-sci-inta.c
> @@ -589,13 +589,14 @@ static struct irq_chip ti_sci_inta_msi_irq_chip = {
>  	.flags			= IRQCHIP_SUPPORTS_LEVEL_MSI,
>  };
>  
> -static void ti_sci_inta_msi_set_desc(msi_alloc_info_t *arg,
> +static int ti_sci_inta_msi_set_desc(msi_alloc_info_t *arg,
>  				     struct msi_desc *desc)
>  {
>  	struct platform_device *pdev = to_platform_device(desc->dev);
>  
>  	arg->desc = desc;
>  	arg->hwirq = TO_HWIRQ(pdev->id, desc->msi_index);
> +	return 0;
>  }
>  
>  static struct msi_domain_ops ti_sci_inta_msi_ops = {
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 94a14a3d7e55..929ba2a991b5 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -296,9 +296,10 @@ static int vmd_msi_prepare(struct irq_domain *domain, struct device *dev,
>  	return 0;
>  }
>  
> -static void vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
> +static int vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
>  {
>  	arg->desc = desc;
> +	return 0;
>  }
>  
>  static struct msi_domain_ops vmd_msi_domain_ops = {
> diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
> index e9cf318e6670..3191f15a07b8 100644
> --- a/drivers/pci/msi/irqdomain.c
> +++ b/drivers/pci/msi/irqdomain.c
> @@ -108,11 +108,12 @@ static int pci_msi_domain_check_cap(struct irq_domain *domain,
>  	return 0;
>  }
>  
> -static void pci_msi_domain_set_desc(msi_alloc_info_t *arg,
> +static int pci_msi_domain_set_desc(msi_alloc_info_t *arg,
>  				    struct msi_desc *desc)
>  {
>  	arg->desc = desc;
>  	arg->hwirq = pci_msi_domain_calc_hwirq(desc);
> +	return 0;
>  }
>  
>  static struct msi_domain_ops pci_msi_domain_ops_default = {
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index fc918a658d48..17228d6e8d85 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -355,7 +355,7 @@ struct msi_domain_ops {
>  	int		(*msi_prepare)(struct irq_domain *domain,
>  				       struct device *dev, int nvec,
>  				       msi_alloc_info_t *arg);
> -	void		(*set_desc)(msi_alloc_info_t *arg,
> +	int		(*set_desc)(msi_alloc_info_t *arg,
>  				    struct msi_desc *desc);
>  	int		(*domain_alloc_irqs)(struct irq_domain *domain,
>  					     struct device *dev, int nvec);
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index a9ee535293eb..a20c85334b58 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -592,10 +592,11 @@ static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev,
>  	return 0;
>  }
>  
> -static void msi_domain_ops_set_desc(msi_alloc_info_t *arg,
> +static int msi_domain_ops_set_desc(msi_alloc_info_t *arg,
>  				    struct msi_desc *desc)
>  {
>  	arg->desc = desc;
> +	return 0;
>  }
>  
>  static int msi_domain_ops_init(struct irq_domain *domain,
> @@ -726,7 +727,10 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
>  		desc = xa_load(&dev->msi.data->__store, virq);
>  		desc->irq = virq;
>  
> -		ops->set_desc(arg, desc);
> +		ret = ops->set_desc(arg, desc);
> +		if (ret)
> +			goto fail;
> +
>  		ret = irq_domain_alloc_irqs_hierarchy(domain, virq, 1, arg);
>  		if (ret)
>  			goto fail;
> @@ -888,7 +892,9 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>  	}
>  
>  	msi_for_each_desc(desc, dev, MSI_DESC_NOTASSOCIATED) {
> -		ops->set_desc(&arg, desc);
> +		ret = ops->set_desc(&arg, desc);
> +		if (ret)
> +			return ret;

Shouldn't you unwind anything that was already allocated before this
call?

thanks,

greg k-h

Re: [PATCH v2 1/2] msi: The MSI framework only supports 2048 platform MSIs

[Greg KH]

On Tue, Jun 14, 2022 at 05:14:10PM +0200, Julian Vetter wrote:
> Some devices need more MSIs. To support this the number must be
> increased.

What devices specifically?

How many more did you just increase it by?

How much more memory are you now using?

Why is there a limit in the first place?  Why not just make it dynamic
so that it never runs out in another month or so?

We need lots more specifics here...

thanks,

greg k-h

Re: [PATCH v2 1/2] msi: The MSI framework only supports 2048 platform MSIs

[Julian Vetter]

On Tue, Jun 14, 2022 at 05:23:27PM +0200, Greg KH wrote:
> On Tue, Jun 14, 2022 at 05:14:10PM +0200, Julian Vetter wrote:
> > Some devices need more MSIs. To support this the number must be
> > increased.

Hello Greg,
thank you for the feedback. See below my comments on it.

> What devices specifically?

You're right I didn't specify this. We have a mailbox device in our
system which supports up to 8192 MSIs. Our architecture is not yet upstreamed,
but the code can be found here https://github.com/kalray/linux_coolidge/blob/coolidge/drivers/irqchip/irq-kvx-apic-mailbox.c
It's a mailbox that can be written to, to trigger an interrupt. It has
128 x 64bit registers and each bit can trigger an interrupt.

> How many more did you just increase it by?
> 
> How much more memory are you now using?
> 
> Why is there a limit in the first place?  Why not just make it dynamic
> so that it never runs out in another month or so?
> 
> We need lots more specifics here...

I increased the number from 2048 -> 8192. But you're right. I just
increased it to the amount that it serves our device. But in my opinion could
it be increased to 2^16. Because the value is OR'ed with the device ID
((devid << (32 - DEV_ID_SHIFT)) | desc->msi_index;) and I assume the device ID
will not exceed 65536. They are stored in a variable of type irq_hw_number_t
which is at least 32bit. So, we could just use the lower 16bit for the
MSI index and the upper 16bit for the device ID.

In terms of memory usage I'm not entirely sure. But the value in the
MAX_DEV_MSIS is only a "global" limit that ensures that we don't overflow the
variable. So, no additional memory is consumed. Because the size of the
IRQ bitmap is handled by other variable(s) (i.e. NR_IRQS).

> thanks,
> 
> greg k-h

Thanks you,
Julian