Support using MSI interrupts in ntb_transport

drivers/iommu/intel_irq_remapping.c static int set_msi_sid(struct irte *irte, struct pci_dev *dev) · line 408

Quoted 8 lines · Logan Gunthorpe

drivers/iommu/intel_irq_remapping.c +4 -0

@@ -394,6 +402,10 @@ static int set_msi_sid(struct irte *irte, struct pci_dev *dev)
 		set_irte_sid(irte, SVT_VERIFY_BUS, SQ_ALL_16,
 			     PCI_DEVID(PCI_BUS_NUM(data.alias),
 				       dev->bus->number));
+	else if (data.count >= 2 && data.busmatch_count == data.count)
+		set_irte_sid(irte, SVT_VERIFY_BUS, SQ_ALL_16,
+			     PCI_DEVID(dev->bus->number,
+				       dev->bus->number));

The dev->bus->number argument for the devfn parameter of PCI_DEVID is not needed, right?

Also, this requires at least a comment to document that special case.

Quoted 5 lines · Joerg Roedel

drivers/iommu/intel_irq_remapping.c static int set_msi_sid(struct irte *irte, · line 404

Quoted 4 lines · Logan Gunthorpe

drivers/iommu/intel_irq_remapping.c +0 -0

@@ -394,6 +402,10 @@ static int set_msi_sid(struct irte *irte,
 struct pci_dev *dev) set_irte_sid(irte, SVT_VERIFY_BUS, SQ_ALL_16,
 			     PCI_DEVID(PCI_BUS_NUM(data.alias),
 				       dev->bus->number));

Quoted 8 lines

drivers/iommu/intel_irq_remapping.c static int set_msi_sid(struct irte *irte, struct pci_dev *dev) · line 408

Quoted 5 lines

drivers/iommu/intel_irq_remapping.c +4 -0

+	else if (data.count >= 2 && data.busmatch_count ==
 data.count)
+		set_irte_sid(irte, SVT_VERIFY_BUS, SQ_ALL_16,
+			     PCI_DEVID(dev->bus->number,
+				       dev->bus->number));  

The dev->bus->number argument for the devfn parameter of PCI_DEVID is not needed, right?

Oh, yes. I think you are right.

Quoted 2 lines

Also, this requires at least a comment to document that special case.

I'll add a comment for v2.

Thanks for the review!

Logan
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Quoted 6 lines · On Fri, 1 Feb 2019 10:27:29 -0700 Logan Gunthorpe <[email protected]> wrote:

Quoted 5 lines · Joerg Roedel

drivers/iommu/intel_irq_remapping.c static int set_msi_sid(struct irte *irte, · line 404

Quoted 4 lines · Logan Gunthorpe

drivers/iommu/intel_irq_remapping.c +0 -0

@@ -394,6 +402,10 @@ static int set_msi_sid(struct irte *irte,
 struct pci_dev *dev) set_irte_sid(irte, SVT_VERIFY_BUS, SQ_ALL_16,
 			     PCI_DEVID(PCI_BUS_NUM(data.alias),
 				       dev->bus->number));

I guess devfn can be removed also. but that is separate cleanup.

Quoted 11 lines · Jacob Pan

Quoted 7 lines · On Fri, 1 Feb 2019 10:27:29 -0700 Logan Gunthorpe <[email protected]> wrote:

Quoted 6 lines · Joerg Roedel

Review static int set_msi_sid(struct irte *irte, · line 404

Quoted 4 lines · Logan Gunthorpe

(quoted diff) +0 -0

@@ -394,6 +402,10 @@ static int set_msi_sid(struct irte *irte,
 struct pci_dev *dev) set_irte_sid(irte, SVT_VERIFY_BUS,
 SQ_ALL_16, PCI_DEVID(PCI_BUS_NUM(data.alias),
 				       dev->bus->number));  

I guess devfn can be removed also. but that is separate cleanup.

Actually, no, I've dug into this and we *do* need the devfn here but it's needlessly confusing. We should not be using PCI_DEVID() as we aren't actually representing a DEVID in this case...

According to the Intel VT-D spec, when using SVT_VERIFY_BUS, the MSB of the SID field represents the starting bus number and the LSB represents the end bus number. The requester id's bus number must then be within that range. The PCI_DEVID macro matches these semantics if you assume the devfn is the end bus, but doesn't really make sense here and just confuses the issue.

So the code was correct, I'll just try to clean it up to make it less confusing.

Thanks,

Logan

For NTB devices, we want to be able to trigger MSI interrupts through a memory window. In these cases we may want to use more interrupts than the NTB PCI device has available in its MSI-X table.

We allow for this by creating a new 'virtual' interrupt. These interrupts are allocated as usual but are not programmed into the MSI-X table (as there may not be space for them).

The MSI address and data will then handled through an NTB MSI library introduced later in this series.

Signed-off-by Logan Gunthorpe <[email protected]>

Cc Bjorn Helgaas <[email protected]>

---
 drivers/pci/msi.c   | 51 +++++++++++++++++++++++++++++++++++++--------
 include/linux/msi.h |  1 +
 include/linux/pci.h |  9 ++++++++
 3 files changed, 52 insertions(+), 9 deletions(-)

drivers/pci/msi.c +42 -9

index 4c0b47867258..145587da686c 100644
@@ -192,6 +192,9 @@ static void msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
 
 static void __iomem *pci_msix_desc_addr(struct msi_desc *desc)
 {
+	if (desc->msi_attrib.is_virtual)
+		return NULL;
+
 	return desc->mask_base +
 		desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE;
 }

@@ -206,14 +209,19 @@ static void __iomem *pci_msix_desc_addr(struct msi_desc *desc)
 u32 __pci_msix_desc_mask_irq(struct msi_desc *desc, u32 flag)
 {
 	u32 mask_bits = desc->masked;
+	void __iomem *desc_addr;
 
 	if (pci_msi_ignore_mask)
 		return 0;
+	desc_addr = pci_msix_desc_addr(desc);
+	if (!desc_addr)
+		return 0;
 
 	mask_bits &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
 	if (flag)
 		mask_bits |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
-	writel(mask_bits, pci_msix_desc_addr(desc) + PCI_MSIX_ENTRY_VECTOR_CTRL);
+
+	writel(mask_bits, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
 
 	return mask_bits;
 }

@@ -273,6 +281,11 @@ void __pci_read_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 	if (entry->msi_attrib.is_msix) {
 		void __iomem *base = pci_msix_desc_addr(entry);
 
+		if (!base) {
+			WARN_ON(1);
+			return;
+		}
+
 		msg->address_lo = readl(base + PCI_MSIX_ENTRY_LOWER_ADDR);
 		msg->address_hi = readl(base + PCI_MSIX_ENTRY_UPPER_ADDR);
 		msg->data = readl(base + PCI_MSIX_ENTRY_DATA);

@@ -303,6 +316,9 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 	} else if (entry->msi_attrib.is_msix) {
 		void __iomem *base = pci_msix_desc_addr(entry);
 
+		if (!base)
+			goto skip;
+
 		writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
 		writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
 		writel(msg->data, base + PCI_MSIX_ENTRY_DATA);

@@ -327,6 +343,8 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 					      msg->data);
 		}
 	}
+
+skip:
 	entry->msg = *msg;
 }
 

@@ -550,6 +568,7 @@ msi_setup_entry(struct pci_dev *dev, int nvec, const struct irq_affinity *affd)
 
 	entry->msi_attrib.is_msix	= 0;
 	entry->msi_attrib.is_64		= !!(control & PCI_MSI_FLAGS_64BIT);
+	entry->msi_attrib.is_virtual    = 0;
 	entry->msi_attrib.entry_nr	= 0;
 	entry->msi_attrib.maskbit	= !!(control & PCI_MSI_FLAGS_MASKBIT);
 	entry->msi_attrib.default_irq	= dev->irq;	/* Save IOAPIC IRQ */

@@ -674,6 +693,7 @@ static int msix_setup_entries(struct pci_dev *dev, void __iomem *base,
 	struct irq_affinity_desc *curmsk, *masks = NULL;
 	struct msi_desc *entry;
 	int ret, i;
+	int vec_count = pci_msix_vec_count(dev);
 
 	if (affd)
 		masks = irq_create_affinity_masks(nvec, affd);

@@ -696,6 +716,10 @@ static int msix_setup_entries(struct pci_dev *dev, void __iomem *base,
 			entry->msi_attrib.entry_nr = entries[i].entry;
 		else
 			entry->msi_attrib.entry_nr = i;
+
+		entry->msi_attrib.is_virtual =
+			entry->msi_attrib.entry_nr >= vec_count;
+
 		entry->msi_attrib.default_irq	= dev->irq;
 		entry->mask_base		= base;
 

@@ -714,12 +738,19 @@ static void msix_program_entries(struct pci_dev *dev,
 {
 	struct msi_desc *entry;
 	int i = 0;
+	void __iomem *desc_addr;
 
 	for_each_pci_msi_entry(entry, dev) {
 		if (entries)
 			entries[i++].vector = entry->irq;
-		entry->masked = readl(pci_msix_desc_addr(entry) +
-				PCI_MSIX_ENTRY_VECTOR_CTRL);
+
+		desc_addr = pci_msix_desc_addr(entry);
+		if (desc_addr)
+			entry->masked = readl(desc_addr +
+					      PCI_MSIX_ENTRY_VECTOR_CTRL);
+		else
+			entry->masked = 0;
+
 		msix_mask_irq(entry, 1);
 	}
 }

@@ -932,7 +963,8 @@ int pci_msix_vec_count(struct pci_dev *dev)
 EXPORT_SYMBOL(pci_msix_vec_count);
 
 static int __pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
-			     int nvec, const struct irq_affinity *affd)
+			     int nvec, const struct irq_affinity *affd,
+			     int flags)
 {
 	int nr_entries;
 	int i, j;

@@ -943,7 +975,7 @@ static int __pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
 	nr_entries = pci_msix_vec_count(dev);
 	if (nr_entries < 0)
 		return nr_entries;
-	if (nvec > nr_entries)
+	if (nvec > nr_entries && !(flags & PCI_IRQ_VIRTUAL))
 		return nr_entries;
 
 	if (entries) {

@@ -1086,7 +1118,8 @@ EXPORT_SYMBOL(pci_enable_msi);
 
 static int __pci_enable_msix_range(struct pci_dev *dev,
 				   struct msix_entry *entries, int minvec,
-				   int maxvec, const struct irq_affinity *affd)
+				   int maxvec, const struct irq_affinity *affd,
+				   int flags)
 {
 	int rc, nvec = maxvec;
 

@@ -1110,7 +1143,7 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
 				return -ENOSPC;
 		}
 
-		rc = __pci_enable_msix(dev, entries, nvec, affd);
+		rc = __pci_enable_msix(dev, entries, nvec, affd, flags);
 		if (rc == 0)
 			return nvec;
 

@@ -1141,7 +1174,7 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
 int pci_enable_msix_range(struct pci_dev *dev, struct msix_entry *entries,
 		int minvec, int maxvec)
 {
-	return __pci_enable_msix_range(dev, entries, minvec, maxvec, NULL);
+	return __pci_enable_msix_range(dev, entries, minvec, maxvec, NULL, 0);
 }
 EXPORT_SYMBOL(pci_enable_msix_range);
 

@@ -1181,7 +1214,7 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
 
 	if (flags & PCI_IRQ_MSIX) {
 		msix_vecs = __pci_enable_msix_range(dev, NULL, min_vecs,
-						    max_vecs, affd);
+						    max_vecs, affd, flags);
 		if (msix_vecs > 0)
 			return msix_vecs;
 	}

include/linux/msi.h +0 -0

index 784fb52b9900..6458ab049852 100644

include/linux/msi.h +0 -0

 				__u16	entry_nr;
 				unsigned default_irq;
 			} msi_attrib;

include/linux/pci.h +0 -0

index 65f1d8c2f082..ce0815c2c498 100644

include/linux/pci.h +4 -0

+ * other driver means.
+ */
+#define PCI_IRQ_VIRTUAL		(1 << 4)
+
 #define PCI_IRQ_ALL_TYPES \
 	(PCI_IRQ_LEGACY | PCI_IRQ_MSI | PCI_IRQ_MSIX)
 

Signature · 1 line

2.19.0

I assume you'll merge this along with the rest of the series, so:

Acked-by Bjorn Helgaas <[email protected]>

include/linux/msi.h struct msi_desc { · line 91

Quoted 9 lines

include/linux/msi.h +1 -0

index 784fb52b9900..6458ab049852 100644
@@ -88,6 +88,7 @@ struct msi_desc {
 				__u8	multi_cap	: 3;
 				__u8	maskbit		: 1;
 				__u8	is_64		: 1;
+				__u8    is_virtual      : 1;

You did the right thing by using the same style as what's already here, but does anybody know why are we using __u8 and __u16 here?

Those typedefs are in include/uapi/asm-generic/int-l64.h, which suggests they're for things exported to user space, but I don't think that's the case here, so I'm wondering if we could someday replace these with u8 and u16. Obviously that wouldn't be part of *this* series.

include/linux/msi.h struct msi_desc {

Quoted 4 lines

include/linux/msi.h +4 -0

+/*
+ * Virtual interrupts allow for more interrupts to be allocated
+ * than the device has interrupts for. These are not programmed
+ * into the devices MSI-X table and must be handled by some

s/devices/device's/

Quoted 13 lines · Bjorn Helgaas

include/linux/msi.h struct msi_desc { · line 91

Quoted 9 lines

include/linux/msi.h +1 -0

index 784fb52b9900..6458ab049852 100644
@@ -88,6 +88,7 @@ struct msi_desc {
 				__u8	multi_cap	: 3;
 				__u8	maskbit		: 1;
 				__u8	is_64		: 1;
+				__u8    is_virtual      : 1;

You did the right thing by using the same style as what's already here, but does anybody know why are we using __u8 and __u16 here?

Quoted 5 lines

Those typedefs are in include/uapi/asm-generic/int-l64.h, which suggests they're for things exported to user space, but I don't think that's the case here, so I'm wondering if we could someday replace these with u8 and u16. Obviously that wouldn't be part of *this* series.

Yes, I was also confused by this. But I always follow the "when-in-rome" rule.

My understanding is the same as yours is that __u8 should be used for userspace compatibility which doesn't apply here. If there is consensus on this being wrong, I'd be happy to write a cleanup patch that fixes it separate from this series.

Hi,

This patch series adds optional support for using MSI interrupts instead of NTB doorbells in ntb_transport. This is desirable seeing doorbells on current hardware are quite slow and therefore switching to MSI interrupts provides a significant performance gain. On switchtec hardware, a simple apples-to-apples comparison shows ntb_netdev/iperf numbers going from 3.88Gb/s to 14.1Gb/s when switching to MSI interrupts.

To do this, a couple changes are required outside of the NTB tree:

1) The IOMMU must know to accept MSI requests from aliased bused numbers seeing NTB hardware typically sends proxied request IDs through additional requester IDs. The first patch in this series adds support for the Intel IOMMU. A quirk to add these aliases for switchtec hardware was already accepted. See commit ad281ecf1c7d ("PCI: Add DMA alias quirk for Microsemi Switchtec NTB") for a description of NTB proxy IDs and why this is necessary.

2) NTB transport (and other clients) may often need more MSI interrupts than the NTB hardware actually advertises support for. However, seeing these interrupts will not be triggered by the hardware but through an NTB memory window, the hardware does not actually need support or need to know about them. Therefore we add the concept of Virtual MSI interrupts which are allocated just like any other MSI interrupt but are not programmed into the hardware's MSI table. This is done in Patch 2 and then made use of in Patch 3.

The remaining patches in this series add a library for dealing with MSI interrupts, a test client and finally support in ntb_transport.

The series is based off of v5.0-rc4 and I've tested it on top of a of the patches I've already sent to the NTB tree (though they are independent changes). A git repo is available here:

https://github.com/sbates130272/linux-p2pmem/ ntb_transport_msi_v1

Thanks,

Logan

--

Logan Gunthorpe (9):
iommu/vt-d: Allow interrupts from the entire bus for aliased devices PCI/MSI: Support allocating virtual MSI interrupts PCI/switchtec: Add module parameter to request more interrupts
NTB: Introduce functions to calculate multi-port resource index
NTB: Rename ntb.c to support multiple source files in the module
NTB: Introduce MSI library
NTB: Introduce NTB MSI Test Client
NTB: Add ntb_msi_test support to ntb_test
NTB: Add MSI interrupt support to ntb_transport

  drivers/iommu/intel_irq_remapping.c     |  12 +
  drivers/ntb/Kconfig                     |  10 +
  drivers/ntb/Makefile                    |   3 +
  drivers/ntb/{ntb.c => core.c}           |   0
  drivers/ntb/msi.c                       | 313 ++++++++++++++++++
  drivers/ntb/ntb_transport.c             | 134 +++++++-
  drivers/ntb/test/Kconfig                |   9 +
  drivers/ntb/test/Makefile               |   1 +
  drivers/ntb/test/ntb_msi_test.c         | 416 ++++++++++++++++++++++++
  drivers/pci/msi.c                       |  51 ++-
  drivers/pci/switch/switchtec.c          |  12 +-
  include/linux/msi.h                     |   1 +
  include/linux/ntb.h                     | 139 ++++++++
  include/linux/pci.h                     |   9 +
  tools/testing/selftests/ntb/ntb_test.sh |  54 ++-
  15 files changed, 1150 insertions(+), 14 deletions(-)

rename drivers/ntb/{ntb.c => core.c} (100%) create mode 100644 drivers/ntb/msi.c create mode 100644 drivers/ntb/test/ntb_msi_test.c

--
2.19.0

Does this work when the system moves the MSI vector either via software (irqbalance) or BIOS APIC programming (some modes cause round robin behavior)?

Quoted 3 lines · Dave Jiang

Does this work when the system moves the MSI vector either via software (irqbalance) or BIOS APIC programming (some modes cause round robin behavior)?

I don't know how irqbalance works, and I'm not sure what you are referring to by BIOS APIC programming, however I would expect these things would not be a problem.

The MSI code I'm presenting here doesn't do anything crazy with the interrupts, it allocates and uses them just as any PCI driver would. The only real difference here is that instead of a piece of hardware sending the IRQ TLP, it will be sent through the memory window (which, from the OS's perspective, is just coming from an NTB hardware proxy alias).

Logan

Quoted 16 lines · Logan Gunthorpe

Quoted 3 lines · Dave Jiang

Does this work when the system moves the MSI vector either via software (irqbalance) or BIOS APIC programming (some modes cause round robin behavior)?

I don't know how irqbalance works, and I'm not sure what you are referring to by BIOS APIC programming, however I would expect these things would not be a problem.

The MSI code I'm presenting here doesn't do anything crazy with the interrupts, it allocates and uses them just as any PCI driver would. The only real difference here is that instead of a piece of hardware sending the IRQ TLP, it will be sent through the memory window (which, from the OS's perspective, is just coming from an NTB hardware proxy alias).

Quoted 1 line

Logan

Right. I did that as a hack a while back for some silicon errata workaround. When the vector moves, the address for the LAPIC changes. So unless it gets updated, you end up writing to the old location and lose all the new interrupts. irqbalance is a user daemon that rotates the system interrupts around to ensure that not all interrupts are pinned on a single core.

I think it's enabled by default on several distros.

Although MSIX has nothing to do with the IOAPIC, the mode that the APIC is programmed can have an influence on how the interrupts are delivered. There are certain Intel platforms (I don't know if AMD does anything like that) puts the IOAPIC in a certain configuration that causes the interrupts to be moved in a round robin fashion. I think it's physical flat mode? I don't quite recall. Normally on the low end Xeons. It's probably worth doing a test run with the irqbalance daemon running and make sure you traffic stream doesn't all of sudden stop.

Quoted 15 lines · Dave Jiang

Quoted 14 lines · Logan Gunthorpe

Quoted 3 lines · Dave Jiang

Does this work when the system moves the MSI vector either via software (irqbalance) or BIOS APIC programming (some modes cause round robin behavior)?

I don't know how irqbalance works, and I'm not sure what you are referring to by BIOS APIC programming, however I would expect these things would not be a problem.

The MSI code I'm presenting here doesn't do anything crazy with the interrupts, it allocates and uses them just as any PCI driver would. The only real difference here is that instead of a piece of hardware sending the IRQ TLP, it will be sent through the memory window (which, from the OS's perspective, is just coming from an NTB hardware proxy alias).

Quoted 7 lines

Quoted 1 line

Logan

Right. I did that as a hack a while back for some silicon errata workaround. When the vector moves, the address for the LAPIC changes. So unless it gets updated, you end up writing to the old location and lose all the new interrupts. irqbalance is a user daemon that rotates the system interrupts around to ensure that not all interrupts are pinned on a single core.

Yes, that would be a problem if something changes the MSI vectors out from under us. Seems like that would be a bit difficult to do even with regular hardware. So far I haven't seen anything that would do that. If you know of where in the kernel this happens I'd be interested in getting a pointer to the flow in the code. If that is the case this MSI stuff will need to get much more complicated...

Quoted 10 lines

I think it's enabled by default on several distros.
Although MSIX has nothing to do with the IOAPIC, the mode that the APIC is programmed can have an influence on how the interrupts are delivered. There are certain Intel platforms (I don't know if AMD does anything like that) puts the IOAPIC in a certain configuration that causes the interrupts to be moved in a round robin fashion. I think it's physical flat mode? I don't quite recall. Normally on the low end Xeons. It's probably worth doing a test run with the irqbalance daemon running and make sure you traffic stream doesn't all of sudden stop.

I've tested with irqbalance running and haven't found any noticeable difference.

Logan

I believe irqbalance writes to the file /proc/irq/N/smp_affinity. So maybe take a look at the code that starts from there and see if it would have any impact on your stuff.

Quoted 3 lines · Dave Jiang

I believe irqbalance writes to the file /proc/irq/N/smp_affinity. So maybe take a look at the code that starts from there and see if it would have any impact on your stuff.

Ok, well on my system I can write to the smp_affinity all day and the MSI interrupts still work fine.

The MSI code is a bit difficult to trace and audit with all the different chips and the parent chips which I don't have a good understanding of. But I can definitely see that it could be possible for some chips to change the address as smp_affinitiy will eventually sometimes call msi_domain_set_affinity() which does seem to recompose the message and write it back to the chip.

So, I could relatively easily add a callback to msi_desc to catch this and resend the MSI address/data. However, I'm not sure how this is ever done atomically. It seems like there would be a race while the device updates its address where old interrupts could be triggered. This race would be much longer for us when sending this information over the NTB link. Though, I guess if the only change is that it encodes CPU information in the address then that would not be an issue. However, I'm not sure I can say that for certain without a comprehensive understanding of all the IRQ chips.

Any thoughts on this?

Logan

Quoted 2 lines · Logan Gunthorpe

On 2019-01-31 3:46 p.m., Dave Jiang wrote:

Quoted 5 lines

Quoted 3 lines

I believe irqbalance writes to the file /proc/irq/N/smp_affinity. So maybe take a look at the code that starts from there and see if it would have any impact on your stuff.

Ok, well on my system I can write to the smp_affinity all day and the MSI interrupts still work fine.

Maybe your code is ok then. If the stats show up in /proc/interrupts then you can see it moving to different cores.

Yeah I'm not sure what to do about it either as I'm not super familiar with that area either. Just making note of what I encountered. And you are right, the updated info has to go over NTB for the other side to write to the updated place. So there's a lot of latency involved.