[PATCH 0/3] Enable multiple MSI feature in pSeries

Currently, multiple MSI feature hasn't been enabled in pSeries,
These patches try to enbale this feature.

These patches have been tested by using ipr driver, and the driver patch
has been made by Wen Xiong [off-list ref]:

[PATCH 0/7] Add support for new IBM SAS controllers

Test platform: One partition of pSeries with one cpu core(4 SMTs) and 
               RAID bus controller: IBM PCI-E IPR SAS Adapter (ASIC) in POWER7
OS version: SUSE Linux Enterprise Server 11 SP2  (ppc64) with 3.8-rc3 kernel 

IRQ 21 and 22 are assigned to the ipr device which support 2 mutiple MSI.

The test results is shown by 'cat /proc/interrups':
          CPU0       CPU1       CPU2       CPU3       
16:     240458     261601     226310     200425      XICS Level     IPI
17:          0          0          0          0      XICS Level     RAS_EPOW
18:         10          0          3          2      XICS Level     hvc_console
19:     122182      28481      28527      28864      XICS Level     ibmvscsi
20:        506    7388226        108        118      XICS Level     eth0
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1
LOC:     398077     316725     231882     203049   Local timer interrupts
SPU:       1659        919        961        903   Spurious interrupts
CNT:          0          0          0          0   Performance
monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

Mike Qiu (3):
  irq: Set multiple MSI descriptor data for multiple IRQs
  irq: Add hw continuous IRQs map to virtual continuous IRQs support
  powerpc/pci: Enable pSeries multiple MSI feature

 arch/powerpc/kernel/msi.c            |    4 --
 arch/powerpc/platforms/pseries/msi.c |   62 ++++++++++++++++++++++++++++++++-
 include/linux/irq.h                  |    4 ++
 include/linux/irqdomain.h            |    3 ++
 kernel/irq/chip.c                    |   40 ++++++++++++++++-----
 kernel/irq/irqdomain.c               |   61 +++++++++++++++++++++++++++++++++
 6 files changed, 158 insertions(+), 16 deletions(-)

-- 
1.7.7.6

Multiple MSI only requires the IRQ in msi_desc entry to be set as
the value of irq_base.

This patch implements the above mentioned technique.

Signed-off-by: Mike Qiu <redacted>
---
 include/linux/irq.h |    2 ++
 kernel/irq/chip.c   |   40 ++++++++++++++++++++++++++++++----------
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index fdf2c4a..60ef45b 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h

@@ -528,6 +528,8 @@ extern int irq_set_handler_data(unsigned int irq, void *data);
 extern int irq_set_chip_data(unsigned int irq, void *data);
 extern int irq_set_irq_type(unsigned int irq, unsigned int type);
 extern int irq_set_msi_desc(unsigned int irq, struct msi_desc *entry);
+extern int irq_set_multiple_msi_desc(unsigned int irq_base, unsigned int nvec,
+					struct msi_desc *entry);
 extern struct irq_data *irq_get_irq_data(unsigned int irq);
 
 static inline struct irq_chip *irq_get_chip(unsigned int irq)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 3aca9f2..c4c39d3 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c

@@ -90,6 +90,35 @@ int irq_set_handler_data(unsigned int irq, void *data)
 EXPORT_SYMBOL(irq_set_handler_data);
 
 /**
+ *	irq_set_multiple_msi_desc - set Multiple MSI descriptor data
+ *	for multiple IRQs
+ *	@irq_base:	Interrupt number base
+ *	@nvec:	The number of interrupts
+ *	@entry:	Pointer to MSI descriptor data
+ *
+ *	Set IRQ descriptors for multiple MSIs
+ */
+int irq_set_multiple_msi_desc(unsigned int irq_base, unsigned int nvec,
+				struct msi_desc *entry)
+{
+	unsigned long flags, i;
+	struct irq_desc *desc;
+
+	for (i = 0; i < nvec; i++) {
+		desc = irq_get_desc_lock(irq_base + i, &flags,
+					IRQ_GET_DESC_CHECK_GLOBAL);
+		if (!desc)
+			return -EINVAL;
+		desc->irq_data.msi_desc = entry;
+		if (i == 0 && entry)
+			entry->irq = irq_base;
+		irq_put_desc_unlock(desc, flags);
+	}
+
+	return 0;
+}
+
+/**
  *	irq_set_msi_desc - set MSI descriptor data for an irq
  *	@irq:	Interrupt number
  *	@entry:	Pointer to MSI descriptor data

@@ -98,16 +127,7 @@ EXPORT_SYMBOL(irq_set_handler_data);
  */
 int irq_set_msi_desc(unsigned int irq, struct msi_desc *entry)
 {
-	unsigned long flags;
-	struct irq_desc *desc = irq_get_desc_lock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
-
-	if (!desc)
-		return -EINVAL;
-	desc->irq_data.msi_desc = entry;
-	if (entry)
-		entry->irq = irq;
-	irq_put_desc_unlock(desc, flags);
-	return 0;
+	return irq_set_multiple_msi_desc(irq, 1, entry);
 }
 
 /**

-- 
1.7.7.6

PCI devices support MSI, MSIX as well as multiple MSI.
But pSeries does not support multiple MSI yet.

This patch enable multiple MSI feature in pSeries.

Signed-off-by: Mike Qiu <redacted>
---
 arch/powerpc/kernel/msi.c            |    4 --
 arch/powerpc/platforms/pseries/msi.c |   62 ++++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/msi.c b/arch/powerpc/kernel/msi.c
index 8bbc12d..46b1470 100644
--- a/arch/powerpc/kernel/msi.c
+++ b/arch/powerpc/kernel/msi.c

@@ -20,10 +20,6 @@ int arch_msi_check_device(struct pci_dev* dev, int nvec, int type)
 		return -ENOSYS;
 	}
 
-	/* PowerPC doesn't support multiple MSI yet */
-	if (type == PCI_CAP_ID_MSI && nvec > 1)
-		return 1;
-
 	if (ppc_md.msi_check_device) {
 		pr_debug("msi: Using platform check routine.\n");
 		return ppc_md.msi_check_device(dev, nvec, type);

diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index e5b0847..6633b18 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c

@@ -132,13 +132,17 @@ static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
 static void rtas_teardown_msi_irqs(struct pci_dev *pdev)
 {
 	struct msi_desc *entry;
+	int nvec, i;
 
 	list_for_each_entry(entry, &pdev->msi_list, list) {
 		if (entry->irq == NO_IRQ)
 			continue;
 
 		irq_set_msi_desc(entry->irq, NULL);
-		irq_dispose_mapping(entry->irq);
+		nvec = entry->msi_attrib.is_msix ? 1 : 1 <<
+					entry->msi_attrib.multiple;
+		for (i = 0; i < nvec; i++)
+			irq_dispose_mapping(entry->irq + i);
 	}
 
 	rtas_disable_msi(pdev);

@@ -392,6 +396,55 @@ static int check_msix_entries(struct pci_dev *pdev)
 	return 0;
 }
 
+static int setup_multiple_msi_irqs(struct pci_dev *pdev, int nvec)
+{
+	struct pci_dn *pdn;
+	int hwirq, virq_base, i, hwirq_base = 0;
+	struct msi_desc *entry;
+	struct msi_msg msg;
+
+	pdn = get_pdn(pdev);
+	entry = list_entry(pdev->msi_list.next, typeof(*entry), list);
+
+	/*
+	 * Get the hardware IRQ base and ensure the retrieved
+	 * hardware IRQs are continuous
+	 */
+	for (i = 0; i < nvec; i++) {
+		hwirq = rtas_query_irq_number(pdn, i);
+		if (i == 0)
+			hwirq_base = hwirq;
+
+		if (hwirq < 0 || hwirq != (hwirq_base + i)) {
+			pr_debug("rtas_msi: Failure to get %d IRQs on"
+				"PCI device %04x:%02x:%02x.%01x\n", nvec,
+				pci_domain_nr(pdev->bus), pdev->bus->number,
+				PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+			return hwirq;
+		}
+	}
+
+	virq_base = irq_create_mapping_many(NULL, hwirq_base, nvec);
+	if (virq_base <= 0) {
+		pr_debug("rtas_msi: Failure to map IRQs (%d, %d) "
+			"for PCI device %04x:%02x:%02x.%01x\n",
+			hwirq_base, nvec, pci_domain_nr(pdev->bus),
+			pdev->bus->number, PCI_SLOT(pdev->devfn),
+			PCI_FUNC(pdev->devfn));
+		return -ENOSPC;
+	}
+
+	entry->msi_attrib.multiple = ilog2(nvec & 0x3f);
+	irq_set_multiple_msi_desc(virq_base, nvec, entry);
+	for (i = 0; i < nvec; i++) {
+		/* Read config space back so we can restore after reset */
+		read_msi_msg(virq_base + i, &msg);
+		entry->msg = msg;
+	}
+
+	return 0;
+}
+
 static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
 {
 	struct pci_dn *pdn;

@@ -444,11 +497,16 @@ again:
 		return rc;
 	}
 
+	if (type == PCI_CAP_ID_MSI && nvec > 1) {
+		rc = setup_multiple_msi_irqs(pdev, nvec);
+		return rc;
+	}
+
 	i = 0;
 	list_for_each_entry(entry, &pdev->msi_list, list) {
 		hwirq = rtas_query_irq_number(pdn, i++);
 		if (hwirq < 0) {
-			pr_debug("rtas_msi: error (%d) getting hwirq\n", rc);
+			pr_debug("rtas_msi: error (%d) getting hwirq\n", nvec);
 			return hwirq;
 		}
 

-- 
1.7.7.6

Adding a function irq_create_mapping_many() which can associate
multiple MSIs to a continous irq mapping.

This is needed to enable multiple MSI support for pSeries.

Signed-off-by: Mike Qiu <redacted>
---
 include/linux/irq.h       |    2 +
 include/linux/irqdomain.h |    3 ++
 kernel/irq/irqdomain.c    |   61 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 60ef45b..e00a7ec 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h

@@ -592,6 +592,8 @@ int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 #define irq_alloc_desc_from(from, node)		\
 	irq_alloc_descs(-1, from, 1, node)
 
+#define irq_alloc_desc_n(nevc, node)		\
+	irq_alloc_descs(-1, 0, nevc, node)
 void irq_free_descs(unsigned int irq, unsigned int cnt);
 int irq_reserve_irqs(unsigned int from, unsigned int cnt);
 

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 0d5b17b..831dded 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h

@@ -168,6 +168,9 @@ extern int irq_create_strict_mappings(struct irq_domain *domain,
 				      unsigned int irq_base,
 				      irq_hw_number_t hwirq_base, int count);
 
+extern int irq_create_mapping_many(struct irq_domain *domain,
+					irq_hw_number_t hwirq_base, int count);
+
 static inline int irq_create_identity_mapping(struct irq_domain *host,
 					      irq_hw_number_t hwirq)
 {

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 96f3a1d..38648e6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c

@@ -636,6 +636,67 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
 }
 EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
 
+/**
+ * irq_create_mapping_many - Map a range of hw IRQs to a range of virtual IRQs
+ * @domain: domain owning the interrupt range
+ * @hwirq_base: beginning of continuous hardware IRQ range
+ * @count: Number of interrupts to map
+ *
+ * This routine is used for allocating and mapping a range of hardware
+ * irqs to virtual IRQs where the virtual irq numbers are not at pre-defined
+ * locations.
+ *
+ * Greater than 0 is returned upon success, while any failure to establish a
+ * static mapping is treated as an error.
+ */
+int irq_create_mapping_many(struct irq_domain *domain,
+		irq_hw_number_t hwirq_base, int count)
+{
+	int ret, irq_base;
+	int virq, i;
+
+	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq_base);
+
+	/* Look for default domain if nececssary */
+	if (!domain)
+		domain = irq_default_domain;
+	if (!domain) {
+		pr_warn("irq_create_mapping called for NULL domain, hwirq=%lx\n"
+			, hwirq_base);
+		WARN_ON(1);
+		return 0;
+	}
+	pr_debug("-> using domain @%p\n", domain);
+
+	/* For IRQ_DOMAIN_MAP_LEGACY, get the first virtual interrupt number */
+	if (domain->revmap_type == IRQ_DOMAIN_MAP_LEGACY)
+		return irq_domain_legacy_revmap(domain, hwirq_base);
+
+	/* Check if mapping already exists */
+	for (i = 0; i < count; i++) {
+		virq = irq_find_mapping(domain, hwirq_base+i);
+		if (virq) {
+			pr_debug("existing mapping on virq %d,"
+					" now dispose it first\n", virq);
+			irq_dispose_mapping(virq);
+		}
+	}
+
+	/* Allocate the continuous virtual interrupt numbers */
+	irq_base = irq_alloc_desc_n(count, of_node_to_nid(domain->of_node));
+	if (unlikely(irq_base < 0))
+		return  irq_base;
+
+	ret = irq_domain_associate_many(domain, irq_base, hwirq_base, count);
+	if (unlikely(ret < 0)) {
+		irq_free_descs(irq_base, count);
+		return ret;
+	}
+
+	return irq_base;
+}
+EXPORT_SYMBOL_GPL(irq_create_mapping_many);
+
 unsigned int irq_create_of_mapping(struct device_node *controller,
 				   const u32 *intspec, unsigned int intsize)
 {

-- 
1.7.7.6

Hi all

Any comments about my patchset?

Thanks 

Mike
在 2013-01-15二的 15:38 +0800，Mike Qiu写道：

Currently, multiple MSI feature hasn't been enabled in pSeries,
These patches try to enbale this feature.

These patches have been tested by using ipr driver, and the driver patch
has been made by Wen Xiong [off-list ref]:

[PATCH 0/7] Add support for new IBM SAS controllers

Test platform: One partition of pSeries with one cpu core(4 SMTs) and 
               RAID bus controller: IBM PCI-E IPR SAS Adapter (ASIC) in POWER7
OS version: SUSE Linux Enterprise Server 11 SP2  (ppc64) with 3.8-rc3 kernel 

IRQ 21 and 22 are assigned to the ipr device which support 2 mutiple MSI.

The test results is shown by 'cat /proc/interrups':
          CPU0       CPU1       CPU2       CPU3       
16:     240458     261601     226310     200425      XICS Level     IPI
17:          0          0          0          0      XICS Level     RAS_EPOW
18:         10          0          3          2      XICS Level     hvc_console
19:     122182      28481      28527      28864      XICS Level     ibmvscsi
20:        506    7388226        108        118      XICS Level     eth0
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1
LOC:     398077     316725     231882     203049   Local timer interrupts
SPU:       1659        919        961        903   Spurious interrupts
CNT:          0          0          0          0   Performance
monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

Mike Qiu (3):
  irq: Set multiple MSI descriptor data for multiple IRQs
  irq: Add hw continuous IRQs map to virtual continuous IRQs support
  powerpc/pci: Enable pSeries multiple MSI feature

 arch/powerpc/kernel/msi.c            |    4 --
 arch/powerpc/platforms/pseries/msi.c |   62 ++++++++++++++++++++++++++++++++-
 include/linux/irq.h                  |    4 ++
 include/linux/irqdomain.h            |    3 ++
 kernel/irq/chip.c                    |   40 ++++++++++++++++-----
 kernel/irq/irqdomain.c               |   61 +++++++++++++++++++++++++++++++++
 6 files changed, 158 insertions(+), 16 deletions(-)

On Tue, 2013-01-15 at 15:38 +0800, Mike Qiu wrote:

Currently, multiple MSI feature hasn't been enabled in pSeries,
These patches try to enbale this feature.

Hi Mike,

These patches have been tested by using ipr driver, and the driver patch
has been made by Wen Xiong [off-list ref]:

So who wrote these patches? Normally we would expect the original author
to post the patches if at all possible.

[PATCH 0/7] Add support for new IBM SAS controllers

I would like to see the full series, including the driver enablement.

Test platform: One partition of pSeries with one cpu core(4 SMTs) and 
               RAID bus controller: IBM PCI-E IPR SAS Adapter (ASIC) in POWER7
OS version: SUSE Linux Enterprise Server 11 SP2  (ppc64) with 3.8-rc3 kernel 

IRQ 21 and 22 are assigned to the ipr device which support 2 mutiple MSI.

The test results is shown by 'cat /proc/interrups':
          CPU0       CPU1       CPU2       CPU3       
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1

This shows that you are correctly configuring two MSIs.

But the key advantage of using multiple interrupts is to distribute load
across CPUs and improve performance. So I would like to see some
performance numbers that show that there is a real benefit for all the
extra complexity in the code.

cheers

On Tue, 2013-01-15 at 15:38 +0800, Mike Qiu wrote:

quoted

Currently, multiple MSI feature hasn't been enabled in pSeries,
These patches try to enbale this feature.

Hi Mike,

quoted

These patches have been tested by using ipr driver, and the driver patch
has been made by Wen Xiong [off-list ref]:

So who wrote these patches? Normally we would expect the original author
to post the patches if at all possible.

Hi Michael

These Multiple MSI patches were wrote by myself, you know this feature 
has not enabled
and it need device driver to test whether it works suitable. So I test 
my patches use
Wen Xiong's ipr patches, which has been send out to the maillinglist.

I'm the**original author :)

quoted

[PATCH 0/7] Add support for new IBM SAS controllers

I would like to see the full series, including the driver enablement.

Yep, but the driver patches were wrote by Wen Xiong and has been send out.
I just use her patches to test my patches. all device support Multiple MSI
can use my feature not only IBM SAS controllers, I also test my patches use
the broadcom wireless card tg3, and also works OK.

quoted

Test platform: One partition of pSeries with one cpu core(4 SMTs) and
                RAID bus controller: IBM PCI-E IPR SAS Adapter (ASIC) in POWER7
OS version: SUSE Linux Enterprise Server 11 SP2  (ppc64) with 3.8-rc3 kernel

IRQ 21 and 22 are assigned to the ipr device which support 2 mutiple MSI.

The test results is shown by 'cat /proc/interrups':
           CPU0       CPU1       CPU2       CPU3
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1

This shows that you are correctly configuring two MSIs.

But the key advantage of using multiple interrupts is to distribute load
across CPUs and improve performance. So I would like to see some
performance numbers that show that there is a real benefit for all the
extra complexity in the code.

Yes, the system just has suport two MSIs. Anyway, I will try to do some 
proformance
test, to show the real benefit.
But actually it needs the driver to do so. As the data show above, it 
seems there is
some problems in use the interrupt, the irq 21 use few, most use 22, I 
will discuss
with the driver author to see why and if she fixed, I will give out the 
proformance
result.

Thanks

Mike

cheers

On Mon, 2013-02-04 at 11:49 +0800, Mike Qiu wrote:

quoted

On Tue, 2013-01-15 at 15:38 +0800, Mike Qiu wrote:

quoted

Currently, multiple MSI feature hasn't been enabled in pSeries,
These patches try to enbale this feature.

Hi Mike,

quoted

These patches have been tested by using ipr driver, and the driver patch
has been made by Wen Xiong [off-list ref]:

So who wrote these patches? Normally we would expect the original author
to post the patches if at all possible.

Hi Michael

These Multiple MSI patches were wrote by myself, you know this feature
has not enabled
and it need device driver to test whether it works suitable. So I test
my patches use 
Wen Xiong's ipr patches, which has been send out to the maillinglist.

I'm the original author :)

Ah OK, sorry, that was more or less clear from your mail but I just
misunderstood.

quoted

quoted

[PATCH 0/7] Add support for new IBM SAS controllers

I would like to see the full series, including the driver enablement.

Yep, but the driver patches were wrote by Wen Xiong and has been send
out.

OK, you mean this series?

http://thread.gmane.org/gmane.linux.scsi/79639

I just use her patches to test my patches. all device support Multiple
MSI can use my feature not only IBM SAS controllers, I also test my
patches use the broadcom wireless card tg3, and also works OK.

You mean drivers/net/ethernet/broadcom/tg3.c ? I don't see where it
calls pci_enable_msi_block() ?

All devices /can/ use it, but the driver needs to be updated. Currently
we have two drivers that do so (in Linus' tree), plus the updated IPR.

quoted

quoted

Test platform: One partition of pSeries with one cpu core(4 SMTs) and 
               RAID bus controller: IBM PCI-E IPR SAS Adapter (ASIC) in POWER7
OS version: SUSE Linux Enterprise Server 11 SP2  (ppc64) with 3.8-rc3 kernel 

IRQ 21 and 22 are assigned to the ipr device which support 2 mutiple MSI.

The test results is shown by 'cat /proc/interrups':
          CPU0       CPU1       CPU2       CPU3       
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1

This shows that you are correctly configuring two MSIs.

But the key advantage of using multiple interrupts is to distribute load
across CPUs and improve performance. So I would like to see some
performance numbers that show that there is a real benefit for all the
extra complexity in the code.

Yes, the system just has suport two MSIs. Anyway, I will try to do
some proformance test, to show the real benefit.
But actually it needs the driver to do so. As the data show above, it
seems there is some problems in use the interrupt, the irq 21 use few,
most use 22, I will discuss with the driver author to see why and if
she fixed, I will give out the proformance result.

Yeah that would be good.

I really dislike that we have a separate API for multi-MSI vs MSI-X, and
pci_enable_msi_block() also pushes the contiguous power-of-2 allocation
into the irq domain layer, which is unpleasant. So if we really must do
multi-MSI I would like to do it differently.

cheers

2013/2/4 13:56, Michael Ellerman:

On Mon, 2013-02-04 at 11:49 +0800, Mike Qiu wrote:

quoted

quoted

On Tue, 2013-01-15 at 15:38 +0800, Mike Qiu wrote:

quoted

Currently, multiple MSI feature hasn't been enabled in pSeries,
These patches try to enbale this feature.

Hi Mike,

quoted

These patches have been tested by using ipr driver, and the driver patch
has been made by Wen Xiong [off-list ref]:

So who wrote these patches? Normally we would expect the original author
to post the patches if at all possible.

Hi Michael

These Multiple MSI patches were wrote by myself, you know this feature
has not enabled
and it need device driver to test whether it works suitable. So I test
my patches use
Wen Xiong's ipr patches, which has been send out to the maillinglist.

I'm the original author :)

Ah OK, sorry, that was more or less clear from your mail but I just
misunderstood.

quoted

quoted

quoted

[PATCH 0/7] Add support for new IBM SAS controllers

I would like to see the full series, including the driver enablement.

Yep, but the driver patches were wrote by Wen Xiong and has been send
out.

OK, you mean this series?

http://thread.gmane.org/gmane.linux.scsi/79639

Yes, exactly.

quoted

I just use her patches to test my patches. all device support Multiple
MSI can use my feature not only IBM SAS controllers, I also test my
patches use the broadcom wireless card tg3, and also works OK.

You mean drivers/net/ethernet/broadcom/tg3.c ? I don't see where it
calls pci_enable_msi_block() ?

Yes, I just modify the driver to support mutiple MSI.

All devices /can/ use it, but the driver needs to be updated. Currently
we have two drivers that do so (in Linus' tree), plus the updated IPR.

Not all devices, just the device which support the multiple MSI by hardware,
can use it

quoted

quoted

quoted

Test platform: One partition of pSeries with one cpu core(4 SMTs) and
                RAID bus controller: IBM PCI-E IPR SAS Adapter (ASIC) in POWER7
OS version: SUSE Linux Enterprise Server 11 SP2  (ppc64) with 3.8-rc3 kernel

IRQ 21 and 22 are assigned to the ipr device which support 2 mutiple MSI.

The test results is shown by 'cat /proc/interrups':
           CPU0       CPU1       CPU2       CPU3
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1

This shows that you are correctly configuring two MSIs.

But the key advantage of using multiple interrupts is to distribute load
across CPUs and improve performance. So I would like to see some
performance numbers that show that there is a real benefit for all the
extra complexity in the code.

Yes, the system just has suport two MSIs. Anyway, I will try to do
some proformance test, to show the real benefit.
But actually it needs the driver to do so. As the data show above, it
seems there is some problems in use the interrupt, the irq 21 use few,
most use 22, I will discuss with the driver author to see why and if
she fixed, I will give out the proformance result.

Yeah that would be good.

I really dislike that we have a separate API for multi-MSI vs MSI-X, and
pci_enable_msi_block() also pushes the contiguous power-of-2 allocation
into the irq domain layer, which is unpleasant. So if we really must do
multi-MSI I would like to do it differently.

Yes, but the multi-MSI must need the hardware support, it is one extend 
for MSI,
The device may sopport MSI and multiple MSI, but not support MSI-X.
for these devices, we'd better use multiple MSI to makes it more efficiency,
compare with MSI.

multi-MSI just can use no more than 32 interrupts

Thanks

cheers

Hi all

Any comments? or any questions about my patchset?

Thanks
Mike
在 2013-01-15二的 15:38 +0800，Mike Qiu写道：

Currently, multiple MSI feature hasn't been enabled in pSeries,
These patches try to enbale this feature.

These patches have been tested by using ipr driver, and the driver patch
has been made by Wen Xiong [off-list ref]:

[PATCH 0/7] Add support for new IBM SAS controllers

Test platform: One partition of pSeries with one cpu core(4 SMTs) and 
               RAID bus controller: IBM PCI-E IPR SAS Adapter (ASIC) in POWER7
OS version: SUSE Linux Enterprise Server 11 SP2  (ppc64) with 3.8-rc3 kernel 

IRQ 21 and 22 are assigned to the ipr device which support 2 mutiple MSI.

The test results is shown by 'cat /proc/interrups':
          CPU0       CPU1       CPU2       CPU3       
16:     240458     261601     226310     200425      XICS Level     IPI
17:          0          0          0          0      XICS Level     RAS_EPOW
18:         10          0          3          2      XICS Level     hvc_console
19:     122182      28481      28527      28864      XICS Level     ibmvscsi
20:        506    7388226        108        118      XICS Level     eth0
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1
LOC:     398077     316725     231882     203049   Local timer interrupts
SPU:       1659        919        961        903   Spurious interrupts
CNT:          0          0          0          0   Performance
monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

Mike Qiu (3):
  irq: Set multiple MSI descriptor data for multiple IRQs
  irq: Add hw continuous IRQs map to virtual continuous IRQs support
  powerpc/pci: Enable pSeries multiple MSI feature

 arch/powerpc/kernel/msi.c            |    4 --
 arch/powerpc/platforms/pseries/msi.c |   62 ++++++++++++++++++++++++++++++++-
 include/linux/irq.h                  |    4 ++
 include/linux/irqdomain.h            |    3 ++
 kernel/irq/chip.c                    |   40 ++++++++++++++++-----
 kernel/irq/irqdomain.c               |   61 +++++++++++++++++++++++++++++++++
 6 files changed, 158 insertions(+), 16 deletions(-)

On Fri, Mar 01, 2013 at 11:08:45AM +0800, Mike wrote:

Hi all

Any comments? or any questions about my patchset?

You were going to get some performance numbers that show a definite
benefit for using more than one MSI.

cheers

于 2013/3/1 11:54, Michael Ellerman 写道:

On Fri, Mar 01, 2013 at 11:08:45AM +0800, Mike wrote:

quoted

Hi all

Any comments? or any questions about my patchset?

You were going to get some performance numbers that show a definite
benefit for using more than one MSI.

Yes, but my patch just enable the kernel to support this feature, whether
to use it depens on the device driver.

And this feature has been merged to the kernel for X86 for a long time.
See commit: 5ca72c4f7c412c2002363218901eba5516c476b1
51906e779f2b13b38f8153774c4c7163d412ffd9

Actually, I'm trying to do the test. but it is difficult to do that test,
because it mostly depends on how the device driver to use this feature,
while the ipr driver patch was wrote by another person. also no any reply
from her.

cheers

On Mon, Mar 04, 2013 at 11:14:53AM +0800, Mike Qiu wrote:

于 2013/3/1 11:54, Michael Ellerman 写道:

quoted

On Fri, Mar 01, 2013 at 11:08:45AM +0800, Mike wrote:

quoted

Hi all

Any comments? or any questions about my patchset?

You were going to get some performance numbers that show a definite
benefit for using more than one MSI.

Yes, but my patch just enable the kernel to support this feature, whether
to use it depens on the device driver.

Sure, but we don't add code just for fun, so unless there's a good
reason to add the feature - like better performance - we won't bother.

And this feature has been merged to the kernel for X86 for a long time.
See commit: 5ca72c4f7c412c2002363218901eba5516c476b1
51906e779f2b13b38f8153774c4c7163d412ffd9

That commit was merged in 3.9-rc1, ie. a few days ago, so no it has not been
in x86 for a long time.

That code removes the need for your first patch, which is a good start.
Please send a new version using irq_set_msi_desc_off().

cheers

On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:

This has been superseeded by irq_alloc_descs_from(), which is the right
way to do it.

For multiple-MSI the allocated interrupt numbers must be a power-of-2,
and must be naturally aligned. I don't /think/ that's a requirement for
the virtual numbers, but it's probably best that we do it anyway.

So this API needs to specify that it will give you back a power-of-2
block that is naturally aligned - otherwise you can't use it for MSI.

+ * This routine is used for allocating and mapping a range of hardware
+ * irqs to virtual IRQs where the virtual irq numbers are not at pre-defined
+ * locations.

This comment doesn't make sense to me.

+ *
+ * Greater than 0 is returned upon success, while any failure to establish a
+ * static mapping is treated as an error.
+ */
+int irq_create_mapping_many(struct irq_domain *domain,
+		irq_hw_number_t hwirq_base, int count)
+{
+	int ret, irq_base;
+	int virq, i;
+
+	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq_base);

I'd like to see this whole function rewritten to reduce the duplication
vs irq_create_mapping(). I don't see any reason why this can't be the
core routine, and irq_create_mapping() becomes a caller of it, passing a
count of 1 ?

+	/* Look for default domain if nececssary */
+	if (!domain)
+		domain = irq_default_domain;
+	if (!domain) {
+		pr_warn("irq_create_mapping called for NULL domain, hwirq=%lx\n"
+			, hwirq_base);
+		WARN_ON(1);
+		return 0;
+	}
+	pr_debug("-> using domain @%p\n", domain);
+
+	/* For IRQ_DOMAIN_MAP_LEGACY, get the first virtual interrupt number */
+	if (domain->revmap_type == IRQ_DOMAIN_MAP_LEGACY)
+		return irq_domain_legacy_revmap(domain, hwirq_base);

The above doesn't work.

+	/* Check if mapping already exists */
+	for (i = 0; i < count; i++) {
+		virq = irq_find_mapping(domain, hwirq_base+i);
+		if (virq) {
+			pr_debug("existing mapping on virq %d,"
+					" now dispose it first\n", virq);
+			irq_dispose_mapping(virq);

You might have just disposed of someone elses mapping, we shouldn't do
that. It should be an error to the caller.

cheers

On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:

Adding a function irq_create_mapping_many() which can associate
multiple MSIs to a continous irq mapping.

This is needed to enable multiple MSI support for pSeries.

+int irq_create_mapping_many(struct irq_domain *domain,
+		irq_hw_number_t hwirq_base, int count)
+{

Other than the other review comments already made, I think you can
simplify this considerably by simply doing what irq_create_strict_mappings() does,
and relaxing the irq_base requirements.

In any event, as you are creating a new interface, I don't think you want
to carry around half of the legacy crap that irq_create_mapping() has to
deal with. We made the decision to avoid this with irq_create_strict_mappings()
intentionally, too.

于 2013/3/5 10:23, Michael Ellerman 写道:

On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:

quoted

Adding a function irq_create_mapping_many() which can associate
multiple MSIs to a continous irq mapping.

This is needed to enable multiple MSI support for pSeries.

Signed-off-by: Mike Qiu <redacted>
---
  include/linux/irq.h       |    2 +
  include/linux/irqdomain.h |    3 ++
  kernel/irq/irqdomain.c    |   61 +++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 60ef45b..e00a7ec 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h

@@ -592,6 +592,8 @@ int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
  #define irq_alloc_desc_from(from, node)		\
  	irq_alloc_descs(-1, from, 1, node)
  
+#define irq_alloc_desc_n(nevc, node)		\
+	irq_alloc_descs(-1, 0, nevc, node)

This has been superseeded by irq_alloc_descs_from(), which is the right
way to do it.

Yes, but irq_alloc_descs_from() just for 1 irq, and if I change the api, 
maybe a lot places which call this
function will be affact.

quoted

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 0d5b17b..831dded 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h

@@ -168,6 +168,9 @@ extern int irq_create_strict_mappings(struct irq_domain *domain,
  				      unsigned int irq_base,
  				      irq_hw_number_t hwirq_base, int count);
  
+extern int irq_create_mapping_many(struct irq_domain *domain,
+					irq_hw_number_t hwirq_base, int count);
+
  static inline int irq_create_identity_mapping(struct irq_domain *host,
  					      irq_hw_number_t hwirq)
  {

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 96f3a1d..38648e6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c

@@ -636,6 +636,67 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
  }
  EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
  
+/**
+ * irq_create_mapping_many - Map a range of hw IRQs to a range of virtual IRQs
+ * @domain: domain owning the interrupt range
+ * @hwirq_base: beginning of continuous hardware IRQ range
+ * @count: Number of interrupts to map

For multiple-MSI the allocated interrupt numbers must be a power-of-2,
and must be naturally aligned. I don't /think/ that's a requirement for
the virtual numbers, but it's probably best that we do it anyway.

So this API needs to specify that it will give you back a power-of-2
block that is naturally aligned - otherwise you can't use it for MSI.

rtas_call will return the numbers of hardware interrupt, and it should 
be power-of-2,
as this I think do not need to specify

quoted

+ * This routine is used for allocating and mapping a range of hardware
+ * irqs to virtual IRQs where the virtual irq numbers are not at pre-defined
+ * locations.

This comment doesn't make sense to me.

quoted

+ *
+ * Greater than 0 is returned upon success, while any failure to establish a
+ * static mapping is treated as an error.
+ */
+int irq_create_mapping_many(struct irq_domain *domain,
+		irq_hw_number_t hwirq_base, int count)
+{
+	int ret, irq_base;
+	int virq, i;
+
+	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq_base);

I'd like to see this whole function rewritten to reduce the duplication
vs irq_create_mapping(). I don't see any reason why this can't be the
core routine, and irq_create_mapping() becomes a caller of it, passing a
count of 1 ?

It's good suggestion.

quoted

+	/* Look for default domain if nececssary */
+	if (!domain)
+		domain = irq_default_domain;
+	if (!domain) {
+		pr_warn("irq_create_mapping called for NULL domain, hwirq=%lx\n"
+			, hwirq_base);
+		WARN_ON(1);
+		return 0;
+	}
+	pr_debug("-> using domain @%p\n", domain);
+
+	/* For IRQ_DOMAIN_MAP_LEGACY, get the first virtual interrupt number */
+	if (domain->revmap_type == IRQ_DOMAIN_MAP_LEGACY)
+		return irq_domain_legacy_revmap(domain, hwirq_base);

The above doesn't work.

Why it doesn't work ?

quoted

+	/* Check if mapping already exists */
+	for (i = 0; i < count; i++) {
+		virq = irq_find_mapping(domain, hwirq_base+i);
+		if (virq) {
+			pr_debug("existing mapping on virq %d,"
+					" now dispose it first\n", virq);
+			irq_dispose_mapping(virq);

You might have just disposed of someone elses mapping, we shouldn't do
that. It should be an error to the caller.

It's a good question. If the interrupt used for someone elses, why I can 
apply it from the system?
So it may someone else forget to dispose mapping, and it never be used 
for others as I have got
the interrupt I think.

cheers

于 2013/3/5 10:41, Paul Mundt 写道:

On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:

quoted

Adding a function irq_create_mapping_many() which can associate
multiple MSIs to a continous irq mapping.

This is needed to enable multiple MSI support for pSeries.

+int irq_create_mapping_many(struct irq_domain *domain,
+		irq_hw_number_t hwirq_base, int count)
+{

Other than the other review comments already made, I think you can
simplify this considerably by simply doing what irq_create_strict_mappings() does,
and relaxing the irq_base requirements.

In any event, as you are creating a new interface, I don't think you want
to carry around half of the legacy crap that irq_create_mapping() has to
deal with. We made the decision to avoid this with irq_create_strict_mappings()
intentionally, too.

Oh, yes, you are right, I will send out V2 of my patch to make it more 
comfortable , and hope you can review my patch again

Thanks

Mike

On Tue, Mar 05, 2013 at 03:19:57PM +0800, Mike Qiu wrote:

于 2013/3/5 10:23, Michael Ellerman 写道:

quoted

On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:

quoted

Adding a function irq_create_mapping_many() which can associate
multiple MSIs to a continous irq mapping.

This is needed to enable multiple MSI support for pSeries.

Signed-off-by: Mike Qiu <redacted>
---
 include/linux/irq.h       |    2 +
 include/linux/irqdomain.h |    3 ++
 kernel/irq/irqdomain.c    |   61 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 60ef45b..e00a7ec 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h

@@ -592,6 +592,8 @@ int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 #define irq_alloc_desc_from(from, node)		\
 	irq_alloc_descs(-1, from, 1, node)
+#define irq_alloc_desc_n(nevc, node)		\
+	irq_alloc_descs(-1, 0, nevc, node)

This has been superseeded by irq_alloc_descs_from(), which is the right
way to do it.

Yes, but irq_alloc_descs_from() just for 1 irq

No it's not, look again.

#define irq_alloc_descs_from(from, cnt, node)   \
	irq_alloc_descs(-1, from, cnt, node)

quoted

quoted

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 96f3a1d..38648e6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c

@@ -636,6 +636,67 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
 }
 EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
+/**
+ * irq_create_mapping_many - Map a range of hw IRQs to a range of virtual IRQs
+ * @domain: domain owning the interrupt range
+ * @hwirq_base: beginning of continuous hardware IRQ range
+ * @count: Number of interrupts to map

quoted

For multiple-MSI the allocated interrupt numbers must be a power-of-2,
and must be naturally aligned. I don't /think/ that's a requirement for
the virtual numbers, but it's probably best that we do it anyway.

So this API needs to specify that it will give you back a power-of-2
block that is naturally aligned - otherwise you can't use it for MSI.

rtas_call will return the numbers of hardware interrupt, and it
should be power-of-2, as this I think do not need to specify

You're confusing hardware interrupt numbers and virtual interrupt
numbers. My comment is about irq_create_mapping_many(), which returns
virtual interrupt numbers.

As I said I don't think there is a requirement that the virtual
interrupt numbers are also a power-of-2 naturally aligned block, but we
should allocate them as one anyway, to avoid any issues in future.

And so this API, which returns virtual interrupt numbers, must satisfy
that specification.

quoted

quoted

+	/* Look for default domain if nececssary */
+	if (!domain)
+		domain = irq_default_domain;
+	if (!domain) {
+		pr_warn("irq_create_mapping called for NULL domain, hwirq=%lx\n"
+			, hwirq_base);
+		WARN_ON(1);
+		return 0;
+	}
+	pr_debug("-> using domain @%p\n", domain);
+
+	/* For IRQ_DOMAIN_MAP_LEGACY, get the first virtual interrupt number */
+	if (domain->revmap_type == IRQ_DOMAIN_MAP_LEGACY)
+		return irq_domain_legacy_revmap(domain, hwirq_base);

The above doesn't work.

Why it doesn't work ?

Because irq_domain_legacy_revmap() only allocates a single interrupt
number.

quoted

quoted

+	/* Check if mapping already exists */
+	for (i = 0; i < count; i++) {
+		virq = irq_find_mapping(domain, hwirq_base+i);
+		if (virq) {
+			pr_debug("existing mapping on virq %d,"
+					" now dispose it first\n", virq);
+			irq_dispose_mapping(virq);

quoted

You might have just disposed of someone elses mapping, we shouldn't do
that. It should be an error to the caller.

It's a good question. If the interrupt used for someone elses, why I
can apply it from the system?

I agree, that would be a bug. But disposing of someone elses mapping is
not OK.

So it may someone else forget to dispose mapping, and it never be
used for others as I have got the interrupt I think.

Perhaps, but that is a bug that needs to be fixed in the code that
forgets to dispose of the mapping.

cheers

于 2013/3/6 11:54, Michael Ellerman 写道:

On Tue, Mar 05, 2013 at 03:19:57PM +0800, Mike Qiu wrote:

quoted

于 2013/3/5 10:23, Michael Ellerman 写道:

quoted

On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:

quoted

Adding a function irq_create_mapping_many() which can associate
multiple MSIs to a continous irq mapping.

This is needed to enable multiple MSI support for pSeries.

Signed-off-by: Mike Qiu <redacted>
---
  include/linux/irq.h       |    2 +
  include/linux/irqdomain.h |    3 ++
  kernel/irq/irqdomain.c    |   61 +++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 60ef45b..e00a7ec 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h

@@ -592,6 +592,8 @@ int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
  #define irq_alloc_desc_from(from, node)		\
  	irq_alloc_descs(-1, from, 1, node)
+#define irq_alloc_desc_n(nevc, node)		\
+	irq_alloc_descs(-1, 0, nevc, node)

This has been superseeded by irq_alloc_descs_from(), which is the right
way to do it.

Yes, but irq_alloc_descs_from() just for 1 irq

No it's not, look again.

#define irq_alloc_descs_from(from, cnt, node)   \
	irq_alloc_descs(-1, from, cnt, node)

Sorry, I see as irq_alloc_desc_from(from, node)
you are right

quoted

quoted

quoted

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 96f3a1d..38648e6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c

@@ -636,6 +636,67 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
  }
  EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
+/**
+ * irq_create_mapping_many - Map a range of hw IRQs to a range of virtual IRQs
+ * @domain: domain owning the interrupt range
+ * @hwirq_base: beginning of continuous hardware IRQ range
+ * @count: Number of interrupts to map

For multiple-MSI the allocated interrupt numbers must be a power-of-2,
and must be naturally aligned. I don't /think/ that's a requirement for
the virtual numbers, but it's probably best that we do it anyway.

So this API needs to specify that it will give you back a power-of-2
block that is naturally aligned - otherwise you can't use it for MSI.

rtas_call will return the numbers of hardware interrupt, and it
should be power-of-2, as this I think do not need to specify

You're confusing hardware interrupt numbers and virtual interrupt
numbers. My comment is about irq_create_mapping_many(), which returns
virtual interrupt numbers.

As I said I don't think there is a requirement that the virtual
interrupt numbers are also a power-of-2 naturally aligned block, but we
should allocate them as one anyway, to avoid any issues in future.

But for virtual interrupt numbersit should be a power-of-2 naturally
aligned block, because it must be continuous, as the MSI-HOWTO.txt says:

     4.2.2 pci_enable_msi_block
     int pci_enable_msi_block(struct pci_dev *dev, int count)
     This variation on the above call allows a device driver to request
     multiple MSIs.  The MSI specification only allows interrupts to be
     allocated in powers of two, up to a maximum of 2^5 (32).
     If this function returns 0, it has succeeded in allocating at least
     as many interrupts as the driver requested
     (it may have allocated more in order to satisfy the power-of-two
     requirement). In this case, the function enables MSI on this device
     and updates dev->irq to be the lowest of the new interrupts
     assigned to it. The other interrupts assigned to the device are in
     the range dev->irq to dev->irq + count - 1.

See the last line, that means for the virtual interrupts must be a
continuous block.

And so this API, which returns virtual interrupt numbers, must satisfy
that specification.

quoted

quoted

quoted

+	/* Look for default domain if nececssary */
+	if (!domain)
+		domain = irq_default_domain;
+	if (!domain) {
+		pr_warn("irq_create_mapping called for NULL domain, hwirq=%lx\n"
+			, hwirq_base);
+		WARN_ON(1);
+		return 0;
+	}
+	pr_debug("-> using domain @%p\n", domain);
+
+	/* For IRQ_DOMAIN_MAP_LEGACY, get the first virtual interrupt number */
+	if (domain->revmap_type == IRQ_DOMAIN_MAP_LEGACY)
+		return irq_domain_legacy_revmap(domain, hwirq_base);

The above doesn't work.

Why it doesn't work ?

Because irq_domain_legacy_revmap() only allocates a single interrupt
number.

OK, your right.

quoted

quoted

quoted

+	/* Check if mapping already exists */
+	for (i = 0; i < count; i++) {
+		virq = irq_find_mapping(domain, hwirq_base+i);
+		if (virq) {
+			pr_debug("existing mapping on virq %d,"
+					" now dispose it first\n", virq);
+			irq_dispose_mapping(virq);

You might have just disposed of someone elses mapping, we shouldn't do
that. It should be an error to the caller.

It's a good question. If the interrupt used for someone elses, why I
can apply it from the system?

I agree, that would be a bug. But disposing of someone elses mapping is
not OK.

quoted

So it may someone else forget to dispose mapping, and it never be
used for others as I have got the interrupt I think.

Perhaps, but that is a bug that needs to be fixed in the code that
forgets to dispose of the mapping.

cheers

On Wed, Mar 06, 2013 at 01:34:58PM +0800, Mike Qiu wrote:

于 2013/3/6 11:54, Michael Ellerman 写道:

quoted

On Tue, Mar 05, 2013 at 03:19:57PM +0800, Mike Qiu wrote:

quoted

于 2013/3/5 10:23, Michael Ellerman 写道:

quoted

On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:

quoted

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 96f3a1d..38648e6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c

@@ -636,6 +636,67 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
 }
 EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
+/**
+ * irq_create_mapping_many - Map a range of hw IRQs to a range of virtual IRQs
+ * @domain: domain owning the interrupt range
+ * @hwirq_base: beginning of continuous hardware IRQ range
+ * @count: Number of interrupts to map

For multiple-MSI the allocated interrupt numbers must be a power-of-2,
and must be naturally aligned. I don't /think/ that's a requirement for
the virtual numbers, but it's probably best that we do it anyway.

So this API needs to specify that it will give you back a power-of-2
block that is naturally aligned - otherwise you can't use it for MSI.

rtas_call will return the numbers of hardware interrupt, and it
should be power-of-2, as this I think do not need to specify

You're confusing hardware interrupt numbers and virtual interrupt
numbers. My comment is about irq_create_mapping_many(), which returns
virtual interrupt numbers.

As I said I don't think there is a requirement that the virtual
interrupt numbers are also a power-of-2 naturally aligned block, but we
should allocate them as one anyway, to avoid any issues in future.

But for virtual interrupt numbersit should be a power-of-2 naturally
aligned block, because it must be continuous, as the MSI-HOWTO.txt says:

    4.2.2 pci_enable_msi_block
    int pci_enable_msi_block(struct pci_dev *dev, int count)
    This variation on the above call allows a device driver to request
    multiple MSIs.  The MSI specification only allows interrupts to be
    allocated in powers of two, up to a maximum of 2^5 (32).
    If this function returns 0, it has succeeded in allocating at least
    as many interrupts as the driver requested
    (it may have allocated more in order to satisfy the power-of-two
    requirement). In this case, the function enables MSI on this device
    and updates dev->irq to be the lowest of the new interrupts
    assigned to it. The other interrupts assigned to the device are in
    the range dev->irq to dev->irq + count - 1.

See the last line, that means for the virtual interrupts must be a
continuous block.

In practice I think things could work if we didn't, because we are not
using the mask routines that assume that layout.

But you're right, we must implement the API as it's specified, so the
virtual interrupt numbers must be a naturally aligned power-of-2.

cheers

于 2013/3/6 13:42, Michael Ellerman 写道:

On Wed, Mar 06, 2013 at 01:34:58PM +0800, Mike Qiu wrote:

quoted

于 2013/3/6 11:54, Michael Ellerman 写道:

quoted

On Tue, Mar 05, 2013 at 03:19:57PM +0800, Mike Qiu wrote:

quoted

于 2013/3/5 10:23, Michael Ellerman 写道:

quoted

On Tue, Jan 15, 2013 at 03:38:55PM +0800, Mike Qiu wrote:

quoted

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 96f3a1d..38648e6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c

@@ -636,6 +636,67 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
  }
  EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
+/**
+ * irq_create_mapping_many - Map a range of hw IRQs to a range of virtual IRQs
+ * @domain: domain owning the interrupt range
+ * @hwirq_base: beginning of continuous hardware IRQ range
+ * @count: Number of interrupts to map

For multiple-MSI the allocated interrupt numbers must be a power-of-2,
and must be naturally aligned. I don't /think/ that's a requirement for
the virtual numbers, but it's probably best that we do it anyway.

So this API needs to specify that it will give you back a power-of-2
block that is naturally aligned - otherwise you can't use it for MSI.

rtas_call will return the numbers of hardware interrupt, and it
should be power-of-2, as this I think do not need to specify

You're confusing hardware interrupt numbers and virtual interrupt
numbers. My comment is about irq_create_mapping_many(), which returns
virtual interrupt numbers.

As I said I don't think there is a requirement that the virtual
interrupt numbers are also a power-of-2 naturally aligned block, but we
should allocate them as one anyway, to avoid any issues in future.

But for virtual interrupt numbersit should be a power-of-2 naturally
aligned block, because it must be continuous, as the MSI-HOWTO.txt says:

     4.2.2 pci_enable_msi_block
     int pci_enable_msi_block(struct pci_dev *dev, int count)
     This variation on the above call allows a device driver to request
     multiple MSIs.  The MSI specification only allows interrupts to be
     allocated in powers of two, up to a maximum of 2^5 (32).
     If this function returns 0, it has succeeded in allocating at least
     as many interrupts as the driver requested
     (it may have allocated more in order to satisfy the power-of-two
     requirement). In this case, the function enables MSI on this device
     and updates dev->irq to be the lowest of the new interrupts
     assigned to it. The other interrupts assigned to the device are in
     the range dev->irq to dev->irq + count - 1.

See the last line, that means for the virtual interrupts must be a
continuous block.

In practice I think things could work if we didn't, because we are not
using the mask routines that assume that layout.

But you're right, we must implement the API as it's specified, so the
virtual interrupt numbers must be a naturally aligned power-of-2.

Yes, also your opinion is also right, just becasue the API requires
a naturally aligned power-of-2 interrupt numbers, so we need to
implement it like this.

cheers

cheers

Hi all

Any comments? or any questions about my patchset?

Thanks
Mike
在 2013-01-15二的 15:38 +0800，Mike Qiu写道：

Currently, multiple MSI feature hasn't been enabled in pSeries,
These patches try to enbale this feature.

These patches have been tested by using ipr driver, and the driver patch
has been made by Wen Xiong [off-list ref]:

[PATCH 0/7] Add support for new IBM SAS controllers

Test platform: One partition of pSeries with one cpu core(4 SMTs) and 
               RAID bus controller: IBM PCI-E IPR SAS Adapter (ASIC) in POWER7
OS version: SUSE Linux Enterprise Server 11 SP2  (ppc64) with 3.8-rc3 kernel 

IRQ 21 and 22 are assigned to the ipr device which support 2 mutiple MSI.

The test results is shown by 'cat /proc/interrups':
          CPU0       CPU1       CPU2       CPU3       
16:     240458     261601     226310     200425      XICS Level     IPI
17:          0          0          0          0      XICS Level     RAS_EPOW
18:         10          0          3          2      XICS Level     hvc_console
19:     122182      28481      28527      28864      XICS Level     ibmvscsi
20:        506    7388226        108        118      XICS Level     eth0
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1
LOC:     398077     316725     231882     203049   Local timer interrupts
SPU:       1659        919        961        903   Spurious interrupts
CNT:          0          0          0          0   Performance
monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

Mike Qiu (3):
  irq: Set multiple MSI descriptor data for multiple IRQs
  irq: Add hw continuous IRQs map to virtual continuous IRQs support
  powerpc/pci: Enable pSeries multiple MSI feature

 arch/powerpc/kernel/msi.c            |    4 --
 arch/powerpc/platforms/pseries/msi.c |   62 ++++++++++++++++++++++++++++++++-
 include/linux/irq.h                  |    4 ++
 include/linux/irqdomain.h            |    3 ++
 kernel/irq/chip.c                    |   40 ++++++++++++++++-----
 kernel/irq/irqdomain.c               |   61 +++++++++++++++++++++++++++++++++
 6 files changed, 158 insertions(+), 16 deletions(-)

On Tue, Jan 15, 2013 at 03:38:53PM +0800, Mike Qiu wrote:

The test results is shown by 'cat /proc/interrups':
          CPU0       CPU1       CPU2       CPU3       
16:     240458     261601     226310     200425      XICS Level     IPI
17:          0          0          0          0      XICS Level     RAS_EPOW
18:         10          0          3          2      XICS Level     hvc_console
19:     122182      28481      28527      28864      XICS Level     ibmvscsi
20:        506    7388226        108        118      XICS Level     eth0
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1

Hi Mike,

I am curious if pSeries firmware allows changing affinity masks independently
for multiple MSIs? I.e. in your example, would it be possible to assign IRQ21
and IRQ22 to different CPUs?

Thanks!

LOC:     398077     316725     231882     203049   Local timer interrupts
SPU:       1659        919        961        903   Spurious interrupts
CNT:          0          0          0          0   Performance
monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

On Tue, 2013-05-21 at 16:45 +0200, Alexander Gordeev wrote:

On Tue, Jan 15, 2013 at 03:38:53PM +0800, Mike Qiu wrote:

quoted

The test results is shown by 'cat /proc/interrups':
          CPU0       CPU1       CPU2       CPU3       
16:     240458     261601     226310     200425      XICS Level     IPI
17:          0          0          0          0      XICS Level     RAS_EPOW
18:         10          0          3          2      XICS Level     hvc_console
19:     122182      28481      28527      28864      XICS Level     ibmvscsi
20:        506    7388226        108        118      XICS Level     eth0
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1

Hi Mike,

I am curious if pSeries firmware allows changing affinity masks independently
for multiple MSIs? I.e. in your example, would it be possible to assign IRQ21
and IRQ22 to different CPUs?

Yes. Each interrupt has its own affinity, whether it's an MSI or not,
the affinity is not driven by the address.

Cheers,
Ben.

Thanks!

quoted

LOC:     398077     316725     231882     203049   Local timer interrupts
SPU:       1659        919        961        903   Spurious interrupts
CNT:          0          0          0          0   Performance
monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

于 2013/5/21 22:45, Alexander Gordeev 写道:

On Tue, Jan 15, 2013 at 03:38:53PM +0800, Mike Qiu wrote:

quoted

The test results is shown by 'cat /proc/interrups':
           CPU0       CPU1       CPU2       CPU3
16:     240458     261601     226310     200425      XICS Level     IPI
17:          0          0          0          0      XICS Level     RAS_EPOW
18:         10          0          3          2      XICS Level     hvc_console
19:     122182      28481      28527      28864      XICS Level     ibmvscsi
20:        506    7388226        108        118      XICS Level     eth0
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1

Hi Mike,

I am curious if pSeries firmware allows changing affinity masks independently
for multiple MSIs? I.e. in your example, would it be possible to assign IRQ21
and IRQ22 to different CPUs?

Yes, as Ben says, this is very different from other firmware :)

Thanks
Mike

Thanks!

quoted

LOC:     398077     316725     231882     203049   Local timer interrupts
SPU:       1659        919        961        903   Spurious interrupts
CNT:          0          0          0          0   Performance
monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

于 2013/5/22 8:15, Benjamin Herrenschmidt 写道:

On Tue, 2013-05-21 at 16:45 +0200, Alexander Gordeev wrote:

quoted

On Tue, Jan 15, 2013 at 03:38:53PM +0800, Mike Qiu wrote:

quoted

The test results is shown by 'cat /proc/interrups':
           CPU0       CPU1       CPU2       CPU3
16:     240458     261601     226310     200425      XICS Level     IPI
17:          0          0          0          0      XICS Level     RAS_EPOW
18:         10          0          3          2      XICS Level     hvc_console
19:     122182      28481      28527      28864      XICS Level     ibmvscsi
20:        506    7388226        108        118      XICS Level     eth0
21:          6          5          5          5      XICS Level     host1-0
22:        817        814        816        813      XICS Level     host1-1

Hi Mike,

I am curious if pSeries firmware allows changing affinity masks independently
for multiple MSIs? I.e. in your example, would it be possible to assign IRQ21
and IRQ22 to different CPUs?

Yes. Each interrupt has its own affinity, whether it's an MSI or not,
the affinity is not driven by the address.

Cheers,
Ben.

Hi Ben,

May this patch be accepted? if so I will send out the 3.9 version.

As Michael Ellerman says, he want to see the performance data,

but this depends on the driver.

It is something like MSI, and the driver can use more than 1 MSI.

That is to say, the driver has more interrupt resource to use,
but whether the driver is full use of the resource, is out of
  this patch's control.

I test this patch use ipr driver, which add multiple MSI
  support by others. and it can work.

Thanks
Mike

quoted

Thanks!

quoted

LOC:     398077     316725     231882     203049   Local timer interrupts
SPU:       1659        919        961        903   Spurious interrupts
CNT:          0          0          0          0   Performance
monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

On Tue, 15 Jan 2013 15:38:54 +0800, Mike Qiu [off-list ref] wrote:

Multiple MSI only requires the IRQ in msi_desc entry to be set as
the value of irq_base.

This patch implements the above mentioned technique.

Signed-off-by: Mike Qiu <redacted>

Hi Mike,

question below...

---
+int irq_set_multiple_msi_desc(unsigned int irq_base, unsigned int nvec,
+				struct msi_desc *entry)
+{
+	unsigned long flags, i;
+	struct irq_desc *desc;
+
+	for (i = 0; i < nvec; i++) {
+		desc = irq_get_desc_lock(irq_base + i, &flags,
+					IRQ_GET_DESC_CHECK_GLOBAL);
+		if (!desc)
+			return -EINVAL;
+		desc->irq_data.msi_desc = entry;
+		if (i == 0 && entry)
+			entry->irq = irq_base;

It's not clear to me why this code only sets the irq value for the first
desc. Why don't the other descs in the array want (irq_base + i) here? A
comment describing what is going on would be appropriate and helpful.

g.