[PATCH v4 0/6] PCI: Support multiple DMA aliases

[PATCH v4 0/6] PCI: Support multiple DMA aliases

[Bjorn Helgaas]

This is a revision of Jacek's v3 posting:
http://lkml.kernel.org/r/1454152012-46337-1-git-send-email-jacek.lawrynowicz@intel.com

The changes from v3 are:

  - Split into smaller patches for reviewability
  - Move printk when adding DMA alias
  - Change dma_alias_is_enabled() interface to take two pci_devs
  - Rename dma_alias_is_enabled() to indicate PCI context

The only remaining thing I want to sort out is the dma_alias_is_enabled()
vs pci_for_each_dma_alias() question Alex raised.  I'll respond to the
relevant part of the patch in this series with my specific questions.

---

Bjorn Helgaas (1):
      PCI: Rename dma_alias_is_enabled() to pci_devs_are_dma_aliases()

Jacek Lawrynowicz (5):
      PCI: Add pci_add_dma_alias() to abstract implementation
      PCI: Move informational printk to pci_add_dma_alias()
      PCI: Add support for multiple DMA aliases
      pci: Add DMA alias quirk for mic_x200_dma
      PCI: Squash pci_dev_flags to remove holes


 drivers/iommu/iommu.c |   18 +++++++++++-------
 drivers/pci/pci.c     |   23 +++++++++++++++++++++++
 drivers/pci/pci.h     |    2 ++
 drivers/pci/probe.c   |    1 +
 drivers/pci/quirks.c  |   34 +++++++++++++++++++---------------
 drivers/pci/search.c  |   14 +++++++++-----
 include/linux/pci.h   |   12 +++++-------
 7 files changed, 70 insertions(+), 34 deletions(-)

[PATCH v4 1/6] PCI: Add pci_add_dma_alias() to abstract implementation

[Bjorn Helgaas]

From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>

Add a pci_add_dma_alias() interface to encapsulate the details of adding an
alias.  No functional change intended.
---
 drivers/pci/pci.c    |   14 ++++++++++++++
 drivers/pci/pci.h    |    2 ++
 drivers/pci/quirks.c |   19 +++++++------------
 3 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 602eb42..7fccc8a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4568,6 +4568,20 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 	return 0;
 }
 
+/**
+ * pci_add_dma_alias - Add a DMA devfn alias for a device
+ * @dev: the PCI device for which alias is added
+ * @devfn: alias slot and function
+ *
+ * This helper encodes 8-bit devfn as bit number in dma_alias_mask.
+ * It should be called early, preferably as PCI fixup header quirk.
+ */
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
+{
+	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
+	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+}
+
 bool pci_device_is_present(struct pci_dev *pdev)
 {
 	u32 v;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9a1660f..c5dc8dc 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -335,4 +335,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 }
 #endif
 
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
+
 #endif /* DRIVERS_PCI_H */
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0575a1e..df28dce 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3581,10 +3581,8 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 
 static void quirk_dma_func0_alias(struct pci_dev *dev)
 {
-	if (PCI_FUNC(dev->devfn) != 0) {
-		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
-		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
-	}
+	if (PCI_FUNC(dev->devfn) != 0)
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
 }
 
 /*
@@ -3597,10 +3595,8 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476, quirk_dma_func0_alias);
 
 static void quirk_dma_func1_alias(struct pci_dev *dev)
 {
-	if (PCI_FUNC(dev->devfn) != 1) {
-		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 1);
-		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
-	}
+	if (PCI_FUNC(dev->devfn) != 1)
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
 }
 
 /*
@@ -3667,11 +3663,10 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
 
 	id = pci_match_id(fixed_dma_alias_tbl, dev);
 	if (id) {
-		dev->dma_alias_devfn = id->driver_data;
-		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+		pci_add_dma_alias(dev, id->driver_data);
 		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
-			 PCI_SLOT(dev->dma_alias_devfn),
-			 PCI_FUNC(dev->dma_alias_devfn));
+			 PCI_SLOT(id->driver_data),
+			 PCI_FUNC(id->driver_data));
 	}
 }
 

[PATCH v4 2/6] PCI: Move informational printk to pci_add_dma_alias()

[Bjorn Helgaas]

From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>

One of the quirks that adds DMA aliases logs an informational message in
dmesg.  Move that to pci_add_dma_alias() so all users log the message
consistently.  No functional change intended (except extra message).
---
 drivers/pci/pci.c    |    2 ++
 drivers/pci/quirks.c |    6 +-----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7fccc8a..8b0a637 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4580,6 +4580,8 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
 	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
 	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
+		 PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
 
 bool pci_device_is_present(struct pci_dev *pdev)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index df28dce..cf023ea 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3662,12 +3662,8 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
 	const struct pci_device_id *id;
 
 	id = pci_match_id(fixed_dma_alias_tbl, dev);
-	if (id) {
+	if (id)
 		pci_add_dma_alias(dev, id->driver_data);
-		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
-			 PCI_SLOT(id->driver_data),
-			 PCI_FUNC(id->driver_data));
-	}
 }
 
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285, quirk_fixed_dma_alias);

[PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Bjorn Helgaas]

From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>

<Insert changelog here>
---
 drivers/iommu/iommu.c |   17 ++++++++++-------
 drivers/pci/pci.c     |   11 +++++++++--
 drivers/pci/probe.c   |    1 +
 drivers/pci/search.c  |   14 +++++++++-----
 include/linux/pci.h   |    4 +---
 5 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0e3b009..a214e19 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -659,9 +659,15 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
 	return NULL;
 }
 
+static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
+{
+	return dev->dma_alias_mask &&
+	       test_bit(devfn, dev->dma_alias_mask);
+}
+
 /*
- * Look for aliases to or from the given device for exisiting groups.  The
- * dma_alias_devfn only supports aliases on the same bus, therefore the search
+ * Look for aliases to or from the given device for existing groups. DMA
+ * aliases are only supported on the same bus, therefore the search
  * space is quite small (especially since we're really only looking at pcie
  * device, and therefore only expect multiple slots on the root complex or
  * downstream switch ports).  It's conceivable though that a pair of
@@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
 			continue;
 
 		/* We alias them or they alias us */
-		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
-		     pdev->dma_alias_devfn == tmp->devfn) ||
-		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
-		     tmp->dma_alias_devfn == pdev->devfn)) {
-
+		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
+		    dma_alias_is_enabled(tmp, pdev->devfn)) {
 			group = get_pci_alias_group(tmp, devfns);
 			if (group) {
 				pci_dev_put(tmp);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8b0a637..eb4dd49 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4578,8 +4578,15 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  */
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
-	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
-	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+	if (!dev->dma_alias_mask)
+		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
+					      sizeof(long), GFP_KERNEL);
+	if (!dev->dma_alias_mask) {
+		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
+		return;
+	}
+
+	set_bit(devfn, dev->dma_alias_mask);
 	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
 		 PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d7ab9b..23fc397 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1503,6 +1503,7 @@ static void pci_release_dev(struct device *dev)
 	pcibios_release_device(pci_dev);
 	pci_bus_put(pci_dev->bus);
 	kfree(pci_dev->driver_override);
+	kfree(pci_dev->dma_alias_mask);
 	kfree(pci_dev);
 }
 
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index a20ce7d..33e0f03 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -40,11 +40,15 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
 	 * If the device is broken and uses an alias requester ID for
 	 * DMA, iterate over that too.
 	 */
-	if (unlikely(pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)) {
-		ret = fn(pdev, PCI_DEVID(pdev->bus->number,
-					 pdev->dma_alias_devfn), data);
-		if (ret)
-			return ret;
+	if (unlikely(pdev->dma_alias_mask)) {
+		u8 devfn;
+
+		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
+			ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
+				 data);
+			if (ret)
+				return ret;
+		}
 	}
 
 	for (bus = pdev->bus; !pci_is_root_bus(bus); bus = bus->parent) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 27df4a6..d9e0c84 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -172,8 +172,6 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) (1 << 2),
 	/* Flag for quirk use to store if quirk-specific ACS is enabled */
 	PCI_DEV_FLAGS_ACS_ENABLED_QUIRK = (__force pci_dev_flags_t) (1 << 3),
-	/* Flag to indicate the device uses dma_alias_devfn */
-	PCI_DEV_FLAGS_DMA_ALIAS_DEVFN = (__force pci_dev_flags_t) (1 << 4),
 	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
 	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 5),
 	/* Do not use bus resets for device */
@@ -279,7 +277,7 @@ struct pci_dev {
 	u8		rom_base_reg;	/* which config register controls the ROM */
 	u8		pin;		/* which interrupt pin this device uses */
 	u16		pcie_flags_reg;	/* cached PCIe Capabilities Register */
-	u8		dma_alias_devfn;/* devfn of DMA alias, if any */
+	unsigned long	*dma_alias_mask;/* mask of enabled devfn aliases */
 
 	struct pci_driver *driver;	/* which driver has allocated this device */
 	u64		dma_mask;	/* Mask of the bits of bus address this

[PATCH v4 4/6] PCI: Rename dma_alias_is_enabled() to pci_devs_are_dma_aliases()

[Bjorn Helgaas]

This should be folded into the previous patch.  I left it separate to show
the interface difference more clearly.

Also, pci_devs_are_dma_aliases() uses PCI internals (dma_alias_mask), so I
think it should be in PCI code instead of in IOMMU code.  That would mean
both it and pci_add_dma_alias() should probably be declared in
include/linux/pci.h (but not exported to modules with EXPORT_SYMBOL()).
---
 drivers/iommu/iommu.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a214e19..031d06b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -659,10 +659,12 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
 	return NULL;
 }
 
-static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
+static bool pci_devs_are_dma_aliases(struct pci_dev *pdev1, struct pci_dev *pdev2)
 {
-	return dev->dma_alias_mask &&
-	       test_bit(devfn, dev->dma_alias_mask);
+	return (pdev1->dma_alias_mask &&
+		test_bit(pdev2->devfn, pdev1->dma_alias_mask)) ||
+	       (pdev2->dma_alias_mask &&
+		test_bit(pdev1->devfn, pdev2->dma_alias_mask));
 }
 
 /*
@@ -692,8 +694,7 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
 			continue;
 
 		/* We alias them or they alias us */
-		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
-		    dma_alias_is_enabled(tmp, pdev->devfn)) {
+		if (pci_devs_are_dma_aliases(pdev, tmp)) {
 			group = get_pci_alias_group(tmp, devfns);
 			if (group) {
 				pci_dev_put(tmp);

[PATCH v4 5/6] pci: Add DMA alias quirk for mic_x200_dma

[Bjorn Helgaas]

From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>

MIC x200 NTB forwards PCIe traffic using multiple alien RID. They have to
be added as aliases to the DMA device in order to allow buffer access
when IOMMU is enabled.

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
Acked-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/pci/quirks.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index cf023ea..ac23a30 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3696,6 +3696,19 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
 
 /*
+ * MIC x200 NTB forwards PCIe traffic using multiple alien RID. They have to
+ * be added as aliases to the DMA device in order to allow buffer access
+ * when IOMMU is enabled.
+ */
+static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
+{
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */

[PATCH v4 5/6] pci: Add DMA alias quirk for mic_x200_dma

[Bjorn Helgaas via iommu]

From: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

MIC x200 NTB forwards PCIe traffic using multiple alien RID. They have to
be added as aliases to the DMA device in order to allow buffer access
when IOMMU is enabled.

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Acked-by: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/pci/quirks.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index cf023ea..ac23a30 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3696,6 +3696,19 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
 
 /*
+ * MIC x200 NTB forwards PCIe traffic using multiple alien RID. They have to
+ * be added as aliases to the DMA device in order to allow buffer access
+ * when IOMMU is enabled.
+ */
+static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
+{
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */

[PATCH v4 6/6] PCI: Squash pci_dev_flags to remove holes

[Bjorn Helgaas]

From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>

After removing PCI_DEV_FLAGS_DMA_ALIAS_DEVFN, the (1 << 4) value was
unused.  Squash the other values so all the bits are adjacent.  No
functional change intended.

(I'm not sure this is worth doing.  We have 16 flag bits and we're not
even close to exhausting them.  But if we do squash them, it should be
in a separate patch so we don't clutter up the main patches.)
---
 include/linux/pci.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index d9e0c84..4e36024 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -173,13 +173,13 @@ enum pci_dev_flags {
 	/* Flag for quirk use to store if quirk-specific ACS is enabled */
 	PCI_DEV_FLAGS_ACS_ENABLED_QUIRK = (__force pci_dev_flags_t) (1 << 3),
 	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
-	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 5),
+	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 4),
 	/* Do not use bus resets for device */
-	PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
+	PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 5),
 	/* Do not use PM reset even if device advertises NoSoftRst- */
-	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
+	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 6),
 	/* Get VPD from function 0 VPD */
-	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
+	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 7),
 };
 
 enum pci_irq_reroute_variant {

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Bjorn Helgaas]

On Wed, Feb 24, 2016 at 01:44:06PM -0600, Bjorn Helgaas wrote:
> From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> 
> <Insert changelog here>

(Sorry, I should have copied this changelog in the patch; I copied
this manually from your v3 posting):

> This patch solves IOMMU support issues with PCIe non-transparent bridges
> that use Requester ID look-up tables (LUT), e.g. PEX8733. Before exiting
> the bridge, packet's RID is rewritten according to LUT programmed by
> a driver. Modified packets are then passed to a destination bus and
> processed upstream. The problem is that such packets seem to come from
> non-existent nodes that are hidden behind NTB and are not discoverable
> by a destination node, so IOMMU discards them. Adding DMA alias for a
> given LUT entry allows IOMMU to create a proper mapping that enables
> inter-node communication.

A specific example here would help me understand.  Here's how I
understand this (correct me if I'm wrong): We're talking about a DMA
packet being forwarded upstream from an NTB.  The NTB uses the LUT to
rewrite the RID in the DMA packet.  The new RID from the LUT is
unknown to the IOMMU, so it discards the DMA packet.

> The current DMA alias implementation supports only single alias, so it's
> not possible to connect more than two nodes when IOMMU is enabled. This
> implementation enables all possible aliases on a given bus (256) that
> are stored in a bitset. Alias devfn is directly translated to a bit
> number. The bitset is not allocated for devices that have no need for
> DMA aliases.

I think "two nodes" is referring to two PCIe devices on the other side
of the NTB.  You want DMA packets from those devices to have different
RIDs so the IOMMU can distinguish them.

The LUT entries basically create aliases of the NTB (one alias for
each device beyond the NTB).  Your quirk uses pci_add_dma_alias(), and
the aliases are all on the same bus as the NTB itself.

The quirk adds PCI_DEVFN(0x10, 0x0), PCI_DEVFN(0x11, 0x0), and
PCI_DEVFN(0x12, 0x0).  Shouldn't there be some connection between this
and the LUT programming?  I assume the LUT is programmed to correspond
to those aliases.  Does this mean you're limited to three devices
beyond the NTB?

> ---
>  drivers/iommu/iommu.c |   17 ++++++++++-------
>  drivers/pci/pci.c     |   11 +++++++++--
>  drivers/pci/probe.c   |    1 +
>  drivers/pci/search.c  |   14 +++++++++-----
>  include/linux/pci.h   |    4 +---
>  5 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0e3b009..a214e19 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -659,9 +659,15 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
>  	return NULL;
>  }
>  
> +static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
> +{
> +	return dev->dma_alias_mask &&
> +	       test_bit(devfn, dev->dma_alias_mask);
> +}
> +
>  /*
> - * Look for aliases to or from the given device for exisiting groups.  The
> - * dma_alias_devfn only supports aliases on the same bus, therefore the search
> + * Look for aliases to or from the given device for existing groups. DMA
> + * aliases are only supported on the same bus, therefore the search

I'm trying to reconcile this statement that "DMA aliases are only
supported on the same bus" (which was there even before this patch)
with the fact that pci_for_each_dma_alias() does not have that
limitation.

>   * space is quite small (especially since we're really only looking at pcie
>   * device, and therefore only expect multiple slots on the root complex or
>   * downstream switch ports).  It's conceivable though that a pair of
> @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
>  			continue;
>  
>  		/* We alias them or they alias us */
> -		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> -		     pdev->dma_alias_devfn == tmp->devfn) ||
> -		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> -		     tmp->dma_alias_devfn == pdev->devfn)) {
> -
> +		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> +		    dma_alias_is_enabled(tmp, pdev->devfn)) {
>  			group = get_pci_alias_group(tmp, devfns);

We basically have this:

  for_each_pci_dev(tmp) {
    if (<pdev and tmp are DMA aliases>)
      group = get_pci_alias_group();
      ...
  }

The DMA alias stuff relies on PCI internals, so it doesn't doesn't
seem quite right to use things like PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and
dma_alias_devfn here in the IOMMU code.  

I'm trying to figure out why we don't do something like the following
instead:

  callback(struct pci_dev *pdev, u16 alias, void *opaque)
  {
    struct iommu_group *group;

    group = get_pci_alias_group();
    if (group)
      return group;

    return 0;
  }

  pci_for_each_dma_alias(pdev, callback, ...);

Is the existing code some sort of optimization, e.g., checking
PCI_DEV_FLAGS_DMA_ALIAS_DEVFN is cheaper than using
pci_for_each_dma_alias()?

It seems like this won't work for some very unlikely but theoretically
possible topologies, e.g.,

  PCIe Root Complex/IOMMU
    PCIe switch A
      PCIe to conventional PCI bridge
        PCI to PCIe Root Complex
	  PCIe NTB

Here, I think the IOMMU will only see RIDs from PCIe switch A, but the
current code only looks at DMA aliases that are on the same bus as the
PCIe NTB.  Wouldn't using pci_for_each_dma_alias() handle this
correctly?

>  			if (group) {
>  				pci_dev_put(tmp);

RE: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Lawrynowicz, Jacek]

[-- Attachment #1: Type: text/plain, Size: 7152 bytes --]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Thursday, February 25, 2016 3:39 PM
> To: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Lawrynowicz, Jacek <jacek.lawrynowicz@intel.com>; linux-
> pci@vger.kernel.org; Alex Williamson <alex.williamson@redhat.com>; Joerg
> Roedel <jroedel@suse.de>; David Woodhouse <dwmw2@infradead.org>;
> iommu@lists.linux-foundation.org
> Subject: Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases
> 
> On Wed, Feb 24, 2016 at 01:44:06PM -0600, Bjorn Helgaas wrote:
> > From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> >
> > <Insert changelog here>
> 
> (Sorry, I should have copied this changelog in the patch; I copied
> this manually from your v3 posting):
> 
> > This patch solves IOMMU support issues with PCIe non-transparent bridges
> > that use Requester ID look-up tables (LUT), e.g. PEX8733. Before exiting
> > the bridge, packet's RID is rewritten according to LUT programmed by
> > a driver. Modified packets are then passed to a destination bus and
> > processed upstream. The problem is that such packets seem to come from
> > non-existent nodes that are hidden behind NTB and are not discoverable
> > by a destination node, so IOMMU discards them. Adding DMA alias for a
> > given LUT entry allows IOMMU to create a proper mapping that enables
> > inter-node communication.
> 
> A specific example here would help me understand.  Here's how I
> understand this (correct me if I'm wrong): We're talking about a DMA
> packet being forwarded upstream from an NTB.  The NTB uses the LUT to
> rewrite the RID in the DMA packet.  The new RID from the LUT is
> unknown to the IOMMU, so it discards the DMA packet.

Yes, this is exactly the problem.

> > The current DMA alias implementation supports only single alias, so it's
> > not possible to connect more than two nodes when IOMMU is enabled. This
> > implementation enables all possible aliases on a given bus (256) that
> > are stored in a bitset. Alias devfn is directly translated to a bit
> > number. The bitset is not allocated for devices that have no need for
> > DMA aliases.
> 
> I think "two nodes" is referring to two PCIe devices on the other side
> of the NTB.  You want DMA packets from those devices to have different
> RIDs so the IOMMU can distinguish them.

Right.

> The LUT entries basically create aliases of the NTB (one alias for
> each device beyond the NTB).  Your quirk uses pci_add_dma_alias(), and
> the aliases are all on the same bus as the NTB itself.
> 
> The quirk adds PCI_DEVFN(0x10, 0x0), PCI_DEVFN(0x11, 0x0), and
> PCI_DEVFN(0x12, 0x0).  Shouldn't there be some connection between this
> and the LUT programming?  I assume the LUT is programmed to correspond
> to those aliases.  Does this mean you're limited to three devices
> beyond the NTB?

Yes, there is an indirect connection between LUT table and devfns used in the
quirk.
Dev part is an offset in the LUT table and function is taken from the device
behind the NTB.
So the driver can only change the dev part by using different LUT offsets.
We don't plan to modify this quirk. The number of PCIe devices beyond single
x200 card NTB will not change.
Two are used by x200 CPU (host bridge & root port) and one is used by x200 DMA
engine.
I'm not sure introducing some dependencies to make sure the offsets are set
correctly is really worth it.

So regarding the improvements in the patch description, you want me to update
and repost it?

BTW I posted x200 DMA driver (the client for this change) on DMA list:
https://lkml.org/lkml/2016/2/9/287
I'm working on integrating review comments and hope to get it included in 4.6.

Regards,
Jacek 

> > ---
> >  drivers/iommu/iommu.c |   17 ++++++++++-------
> >  drivers/pci/pci.c     |   11 +++++++++--
> >  drivers/pci/probe.c   |    1 +
> >  drivers/pci/search.c  |   14 +++++++++-----
> >  include/linux/pci.h   |    4 +---
> >  5 files changed, 30 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 0e3b009..a214e19 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -659,9 +659,15 @@ static struct iommu_group
> *get_pci_function_alias_group(struct pci_dev *pdev,
> >  	return NULL;
> >  }
> >
> > +static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
> > +{
> > +	return dev->dma_alias_mask &&
> > +	       test_bit(devfn, dev->dma_alias_mask);
> > +}
> > +
> >  /*
> > - * Look for aliases to or from the given device for exisiting groups.  The
> > - * dma_alias_devfn only supports aliases on the same bus, therefore the
> search
> > + * Look for aliases to or from the given device for existing groups. DMA
> > + * aliases are only supported on the same bus, therefore the search
> 
> I'm trying to reconcile this statement that "DMA aliases are only
> supported on the same bus" (which was there even before this patch)
> with the fact that pci_for_each_dma_alias() does not have that
> limitation.
> 
> >   * space is quite small (especially since we're really only looking at pcie
> >   * device, and therefore only expect multiple slots on the root complex or
> >   * downstream switch ports).  It's conceivable though that a pair of
> > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct
> pci_dev *pdev,
> >  			continue;
> >
> >  		/* We alias them or they alias us */
> > -		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)
> &&
> > -		     pdev->dma_alias_devfn == tmp->devfn) ||
> > -		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > -		     tmp->dma_alias_devfn == pdev->devfn)) {
> > -
> > +		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > +		    dma_alias_is_enabled(tmp, pdev->devfn)) {
> >  			group = get_pci_alias_group(tmp, devfns);
> 
> We basically have this:
> 
>   for_each_pci_dev(tmp) {
>     if (<pdev and tmp are DMA aliases>)
>       group = get_pci_alias_group();
>       ...
>   }
> 
> The DMA alias stuff relies on PCI internals, so it doesn't doesn't
> seem quite right to use things like PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and
> dma_alias_devfn here in the IOMMU code.
> 
> I'm trying to figure out why we don't do something like the following
> instead:
> 
>   callback(struct pci_dev *pdev, u16 alias, void *opaque)
>   {
>     struct iommu_group *group;
> 
>     group = get_pci_alias_group();
>     if (group)
>       return group;
> 
>     return 0;
>   }
> 
>   pci_for_each_dma_alias(pdev, callback, ...);
> 
> Is the existing code some sort of optimization, e.g., checking
> PCI_DEV_FLAGS_DMA_ALIAS_DEVFN is cheaper than using
> pci_for_each_dma_alias()?
> 
> It seems like this won't work for some very unlikely but theoretically
> possible topologies, e.g.,
> 
>   PCIe Root Complex/IOMMU
>     PCIe switch A
>       PCIe to conventional PCI bridge
>         PCI to PCIe Root Complex
> 	  PCIe NTB
> 
> Here, I think the IOMMU will only see RIDs from PCIe switch A, but the
> current code only looks at DMA aliases that are on the same bus as the
> PCIe NTB.  Wouldn't using pci_for_each_dma_alias() handle this
> correctly?
> 
> >  			if (group) {
> >  				pci_dev_put(tmp);

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7756 bytes --]

RE: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Lawrynowicz, Jacek]

[-- Attachment #1.1: Type: text/plain, Size: 7404 bytes --]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Thursday, February 25, 2016 3:39 PM
> To: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Lawrynowicz, Jacek <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; linux-
> pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; Joerg
> Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>; David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Subject: Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases
> 
> On Wed, Feb 24, 2016 at 01:44:06PM -0600, Bjorn Helgaas wrote:
> > From: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >
> > <Insert changelog here>
> 
> (Sorry, I should have copied this changelog in the patch; I copied
> this manually from your v3 posting):
> 
> > This patch solves IOMMU support issues with PCIe non-transparent bridges
> > that use Requester ID look-up tables (LUT), e.g. PEX8733. Before exiting
> > the bridge, packet's RID is rewritten according to LUT programmed by
> > a driver. Modified packets are then passed to a destination bus and
> > processed upstream. The problem is that such packets seem to come from
> > non-existent nodes that are hidden behind NTB and are not discoverable
> > by a destination node, so IOMMU discards them. Adding DMA alias for a
> > given LUT entry allows IOMMU to create a proper mapping that enables
> > inter-node communication.
> 
> A specific example here would help me understand.  Here's how I
> understand this (correct me if I'm wrong): We're talking about a DMA
> packet being forwarded upstream from an NTB.  The NTB uses the LUT to
> rewrite the RID in the DMA packet.  The new RID from the LUT is
> unknown to the IOMMU, so it discards the DMA packet.

Yes, this is exactly the problem.

> > The current DMA alias implementation supports only single alias, so it's
> > not possible to connect more than two nodes when IOMMU is enabled. This
> > implementation enables all possible aliases on a given bus (256) that
> > are stored in a bitset. Alias devfn is directly translated to a bit
> > number. The bitset is not allocated for devices that have no need for
> > DMA aliases.
> 
> I think "two nodes" is referring to two PCIe devices on the other side
> of the NTB.  You want DMA packets from those devices to have different
> RIDs so the IOMMU can distinguish them.

Right.

> The LUT entries basically create aliases of the NTB (one alias for
> each device beyond the NTB).  Your quirk uses pci_add_dma_alias(), and
> the aliases are all on the same bus as the NTB itself.
> 
> The quirk adds PCI_DEVFN(0x10, 0x0), PCI_DEVFN(0x11, 0x0), and
> PCI_DEVFN(0x12, 0x0).  Shouldn't there be some connection between this
> and the LUT programming?  I assume the LUT is programmed to correspond
> to those aliases.  Does this mean you're limited to three devices
> beyond the NTB?

Yes, there is an indirect connection between LUT table and devfns used in the
quirk.
Dev part is an offset in the LUT table and function is taken from the device
behind the NTB.
So the driver can only change the dev part by using different LUT offsets.
We don't plan to modify this quirk. The number of PCIe devices beyond single
x200 card NTB will not change.
Two are used by x200 CPU (host bridge & root port) and one is used by x200 DMA
engine.
I'm not sure introducing some dependencies to make sure the offsets are set
correctly is really worth it.

So regarding the improvements in the patch description, you want me to update
and repost it?

BTW I posted x200 DMA driver (the client for this change) on DMA list:
https://lkml.org/lkml/2016/2/9/287
I'm working on integrating review comments and hope to get it included in 4.6.

Regards,
Jacek 

> > ---
> >  drivers/iommu/iommu.c |   17 ++++++++++-------
> >  drivers/pci/pci.c     |   11 +++++++++--
> >  drivers/pci/probe.c   |    1 +
> >  drivers/pci/search.c  |   14 +++++++++-----
> >  include/linux/pci.h   |    4 +---
> >  5 files changed, 30 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 0e3b009..a214e19 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -659,9 +659,15 @@ static struct iommu_group
> *get_pci_function_alias_group(struct pci_dev *pdev,
> >  	return NULL;
> >  }
> >
> > +static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
> > +{
> > +	return dev->dma_alias_mask &&
> > +	       test_bit(devfn, dev->dma_alias_mask);
> > +}
> > +
> >  /*
> > - * Look for aliases to or from the given device for exisiting groups.  The
> > - * dma_alias_devfn only supports aliases on the same bus, therefore the
> search
> > + * Look for aliases to or from the given device for existing groups. DMA
> > + * aliases are only supported on the same bus, therefore the search
> 
> I'm trying to reconcile this statement that "DMA aliases are only
> supported on the same bus" (which was there even before this patch)
> with the fact that pci_for_each_dma_alias() does not have that
> limitation.
> 
> >   * space is quite small (especially since we're really only looking at pcie
> >   * device, and therefore only expect multiple slots on the root complex or
> >   * downstream switch ports).  It's conceivable though that a pair of
> > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct
> pci_dev *pdev,
> >  			continue;
> >
> >  		/* We alias them or they alias us */
> > -		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)
> &&
> > -		     pdev->dma_alias_devfn == tmp->devfn) ||
> > -		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > -		     tmp->dma_alias_devfn == pdev->devfn)) {
> > -
> > +		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > +		    dma_alias_is_enabled(tmp, pdev->devfn)) {
> >  			group = get_pci_alias_group(tmp, devfns);
> 
> We basically have this:
> 
>   for_each_pci_dev(tmp) {
>     if (<pdev and tmp are DMA aliases>)
>       group = get_pci_alias_group();
>       ...
>   }
> 
> The DMA alias stuff relies on PCI internals, so it doesn't doesn't
> seem quite right to use things like PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and
> dma_alias_devfn here in the IOMMU code.
> 
> I'm trying to figure out why we don't do something like the following
> instead:
> 
>   callback(struct pci_dev *pdev, u16 alias, void *opaque)
>   {
>     struct iommu_group *group;
> 
>     group = get_pci_alias_group();
>     if (group)
>       return group;
> 
>     return 0;
>   }
> 
>   pci_for_each_dma_alias(pdev, callback, ...);
> 
> Is the existing code some sort of optimization, e.g., checking
> PCI_DEV_FLAGS_DMA_ALIAS_DEVFN is cheaper than using
> pci_for_each_dma_alias()?
> 
> It seems like this won't work for some very unlikely but theoretically
> possible topologies, e.g.,
> 
>   PCIe Root Complex/IOMMU
>     PCIe switch A
>       PCIe to conventional PCI bridge
>         PCI to PCIe Root Complex
> 	  PCIe NTB
> 
> Here, I think the IOMMU will only see RIDs from PCIe switch A, but the
> current code only looks at DMA aliases that are on the same bus as the
> PCIe NTB.  Wouldn't using pci_for_each_dma_alias() handle this
> correctly?
> 
> >  			if (group) {
> >  				pci_dev_put(tmp);

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7756 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Bjorn Helgaas]

On Thu, Feb 25, 2016 at 03:41:51PM +0000, Lawrynowicz, Jacek wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> > Sent: Thursday, February 25, 2016 3:39 PM
> > To: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Lawrynowicz, Jacek <jacek.lawrynowicz@intel.com>; linux-
> > pci@vger.kernel.org; Alex Williamson <alex.williamson@redhat.com>; Joerg
> > Roedel <jroedel@suse.de>; David Woodhouse <dwmw2@infradead.org>;
> > iommu@lists.linux-foundation.org
> > Subject: Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases
> > 
> > On Wed, Feb 24, 2016 at 01:44:06PM -0600, Bjorn Helgaas wrote:
> > > From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> > >
> > > <Insert changelog here>
> > 
> > (Sorry, I should have copied this changelog in the patch; I copied
> > this manually from your v3 posting):
> > 
> > > This patch solves IOMMU support issues with PCIe non-transparent bridges
> > > that use Requester ID look-up tables (LUT), e.g. PEX8733. Before exiting
> > > the bridge, packet's RID is rewritten according to LUT programmed by
> > > a driver. Modified packets are then passed to a destination bus and
> > > processed upstream. The problem is that such packets seem to come from
> > > non-existent nodes that are hidden behind NTB and are not discoverable
> > > by a destination node, so IOMMU discards them. Adding DMA alias for a
> > > given LUT entry allows IOMMU to create a proper mapping that enables
> > > inter-node communication.
> > 
> > A specific example here would help me understand.  Here's how I
> > understand this (correct me if I'm wrong): We're talking about a DMA
> > packet being forwarded upstream from an NTB.  The NTB uses the LUT to
> > rewrite the RID in the DMA packet.  The new RID from the LUT is
> > unknown to the IOMMU, so it discards the DMA packet.
> 
> Yes, this is exactly the problem.
> 
> > > The current DMA alias implementation supports only single alias, so it's
> > > not possible to connect more than two nodes when IOMMU is enabled. This
> > > implementation enables all possible aliases on a given bus (256) that
> > > are stored in a bitset. Alias devfn is directly translated to a bit
> > > number. The bitset is not allocated for devices that have no need for
> > > DMA aliases.
> > 
> > I think "two nodes" is referring to two PCIe devices on the other side
> > of the NTB.  You want DMA packets from those devices to have different
> > RIDs so the IOMMU can distinguish them.
> 
> Right.
> 
> > The LUT entries basically create aliases of the NTB (one alias for
> > each device beyond the NTB).  Your quirk uses pci_add_dma_alias(), and
> > the aliases are all on the same bus as the NTB itself.
> > 
> > The quirk adds PCI_DEVFN(0x10, 0x0), PCI_DEVFN(0x11, 0x0), and
> > PCI_DEVFN(0x12, 0x0).  Shouldn't there be some connection between this
> > and the LUT programming?  I assume the LUT is programmed to correspond
> > to those aliases.  Does this mean you're limited to three devices
> > beyond the NTB?
> 
> Yes, there is an indirect connection between LUT table and devfns used in the
> quirk.
> Dev part is an offset in the LUT table and function is taken from the device
> behind the NTB.
> So the driver can only change the dev part by using different LUT offsets.
> We don't plan to modify this quirk. The number of PCIe devices beyond single
> x200 card NTB will not change.
> Two are used by x200 CPU (host bridge & root port) and one is used by x200 DMA
> engine.
> I'm not sure introducing some dependencies to make sure the offsets are set
> correctly is really worth it.

I'd like at least a comment that points to the specific x200 code that
must coordinate with this.

> So regarding the improvements in the patch description, you want me to update
> and repost it?

Yes, please.

> BTW I posted x200 DMA driver (the client for this change) on DMA list:
> https://lkml.org/lkml/2016/2/9/287
> I'm working on integrating review comments and hope to get it included in 4.6.

What about my questions on the code itself, below?

> > > ---
> > >  drivers/iommu/iommu.c |   17 ++++++++++-------
> > >  drivers/pci/pci.c     |   11 +++++++++--
> > >  drivers/pci/probe.c   |    1 +
> > >  drivers/pci/search.c  |   14 +++++++++-----
> > >  include/linux/pci.h   |    4 +---
> > >  5 files changed, 30 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index 0e3b009..a214e19 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -659,9 +659,15 @@ static struct iommu_group
> > *get_pci_function_alias_group(struct pci_dev *pdev,
> > >  	return NULL;
> > >  }
> > >
> > > +static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
> > > +{
> > > +	return dev->dma_alias_mask &&
> > > +	       test_bit(devfn, dev->dma_alias_mask);
> > > +}
> > > +
> > >  /*
> > > - * Look for aliases to or from the given device for exisiting groups.  The
> > > - * dma_alias_devfn only supports aliases on the same bus, therefore the
> > search
> > > + * Look for aliases to or from the given device for existing groups. DMA
> > > + * aliases are only supported on the same bus, therefore the search
> > 
> > I'm trying to reconcile this statement that "DMA aliases are only
> > supported on the same bus" (which was there even before this patch)
> > with the fact that pci_for_each_dma_alias() does not have that
> > limitation.
> > 
> > >   * space is quite small (especially since we're really only looking at pcie
> > >   * device, and therefore only expect multiple slots on the root complex or
> > >   * downstream switch ports).  It's conceivable though that a pair of
> > > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct
> > pci_dev *pdev,
> > >  			continue;
> > >
> > >  		/* We alias them or they alias us */
> > > -		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)
> > &&
> > > -		     pdev->dma_alias_devfn == tmp->devfn) ||
> > > -		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > -		     tmp->dma_alias_devfn == pdev->devfn)) {
> > > -
> > > +		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > > +		    dma_alias_is_enabled(tmp, pdev->devfn)) {
> > >  			group = get_pci_alias_group(tmp, devfns);
> > 
> > We basically have this:
> > 
> >   for_each_pci_dev(tmp) {
> >     if (<pdev and tmp are DMA aliases>)
> >       group = get_pci_alias_group();
> >       ...
> >   }
> > 
> > The DMA alias stuff relies on PCI internals, so it doesn't doesn't
> > seem quite right to use things like PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and
> > dma_alias_devfn here in the IOMMU code.
> > 
> > I'm trying to figure out why we don't do something like the following
> > instead:
> > 
> >   callback(struct pci_dev *pdev, u16 alias, void *opaque)
> >   {
> >     struct iommu_group *group;
> > 
> >     group = get_pci_alias_group();
> >     if (group)
> >       return group;
> > 
> >     return 0;
> >   }
> > 
> >   pci_for_each_dma_alias(pdev, callback, ...);
> > 
> > Is the existing code some sort of optimization, e.g., checking
> > PCI_DEV_FLAGS_DMA_ALIAS_DEVFN is cheaper than using
> > pci_for_each_dma_alias()?
> > 
> > It seems like this won't work for some very unlikely but theoretically
> > possible topologies, e.g.,
> > 
> >   PCIe Root Complex/IOMMU
> >     PCIe switch A
> >       PCIe to conventional PCI bridge
> >         PCI to PCIe Root Complex
> > 	  PCIe NTB
> > 
> > Here, I think the IOMMU will only see RIDs from PCIe switch A, but the
> > current code only looks at DMA aliases that are on the same bus as the
> > PCIe NTB.  Wouldn't using pci_for_each_dma_alias() handle this
> > correctly?
> > 
> > >  			if (group) {
> > >  				pci_dev_put(tmp);

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Jacek Lawrynowicz]

On Mon, Feb 29, 2016 at 04:44:17PM -0600, Bjorn Helgaas wrote:
> On Thu, Feb 25, 2016 at 03:41:51PM +0000, Lawrynowicz, Jacek wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> > > Sent: Thursday, February 25, 2016 3:39 PM
> > > To: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Lawrynowicz, Jacek <jacek.lawrynowicz@intel.com>; linux-
> > > pci@vger.kernel.org; Alex Williamson <alex.williamson@redhat.com>; Joerg
> > > Roedel <jroedel@suse.de>; David Woodhouse <dwmw2@infradead.org>;
> > > iommu@lists.linux-foundation.org
> > > Subject: Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases
> > > 
> > > On Wed, Feb 24, 2016 at 01:44:06PM -0600, Bjorn Helgaas wrote:
> > > > From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> 
> What about my questions on the code itself, below?
> 

Sorry but I'm not able to answer it.
Maybe Joerg or David could help here?

> > > > ---
> > > >  drivers/iommu/iommu.c |   17 ++++++++++-------
> > > >  drivers/pci/pci.c     |   11 +++++++++--
> > > >  drivers/pci/probe.c   |    1 +
> > > >  drivers/pci/search.c  |   14 +++++++++-----
> > > >  include/linux/pci.h   |    4 +---
> > > >  5 files changed, 30 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > index 0e3b009..a214e19 100644
> > > > --- a/drivers/iommu/iommu.c
> > > > +++ b/drivers/iommu/iommu.c
> > > > @@ -659,9 +659,15 @@ static struct iommu_group
> > > *get_pci_function_alias_group(struct pci_dev *pdev,
> > > >  	return NULL;
> > > >  }
> > > >
> > > > +static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
> > > > +{
> > > > +	return dev->dma_alias_mask &&
> > > > +	       test_bit(devfn, dev->dma_alias_mask);
> > > > +}
> > > > +
> > > >  /*
> > > > - * Look for aliases to or from the given device for exisiting groups.  The
> > > > - * dma_alias_devfn only supports aliases on the same bus, therefore the
> > > search
> > > > + * Look for aliases to or from the given device for existing groups. DMA
> > > > + * aliases are only supported on the same bus, therefore the search
> > > 
> > > I'm trying to reconcile this statement that "DMA aliases are only
> > > supported on the same bus" (which was there even before this patch)
> > > with the fact that pci_for_each_dma_alias() does not have that
> > > limitation.
> > > 
> > > >   * space is quite small (especially since we're really only looking at pcie
> > > >   * device, and therefore only expect multiple slots on the root complex or
> > > >   * downstream switch ports).  It's conceivable though that a pair of
> > > > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct
> > > pci_dev *pdev,
> > > >  			continue;
> > > >
> > > >  		/* We alias them or they alias us */
> > > > -		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)
> > > &&
> > > > -		     pdev->dma_alias_devfn == tmp->devfn) ||
> > > > -		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > > -		     tmp->dma_alias_devfn == pdev->devfn)) {
> > > > -
> > > > +		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > > > +		    dma_alias_is_enabled(tmp, pdev->devfn)) {
> > > >  			group = get_pci_alias_group(tmp, devfns);
> > > 
> > > We basically have this:
> > > 
> > >   for_each_pci_dev(tmp) {
> > >     if (<pdev and tmp are DMA aliases>)
> > >       group = get_pci_alias_group();
> > >       ...
> > >   }
> > > 
> > > The DMA alias stuff relies on PCI internals, so it doesn't doesn't
> > > seem quite right to use things like PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and
> > > dma_alias_devfn here in the IOMMU code.
> > > 
> > > I'm trying to figure out why we don't do something like the following
> > > instead:
> > > 
> > >   callback(struct pci_dev *pdev, u16 alias, void *opaque)
> > >   {
> > >     struct iommu_group *group;
> > > 
> > >     group = get_pci_alias_group();
> > >     if (group)
> > >       return group;
> > > 
> > >     return 0;
> > >   }
> > > 
> > >   pci_for_each_dma_alias(pdev, callback, ...);
> > > 
> > > Is the existing code some sort of optimization, e.g., checking
> > > PCI_DEV_FLAGS_DMA_ALIAS_DEVFN is cheaper than using
> > > pci_for_each_dma_alias()?
> > > 
> > > It seems like this won't work for some very unlikely but theoretically
> > > possible topologies, e.g.,
> > > 
> > >   PCIe Root Complex/IOMMU
> > >     PCIe switch A
> > >       PCIe to conventional PCI bridge
> > >         PCI to PCIe Root Complex
> > > 	  PCIe NTB
> > > 
> > > Here, I think the IOMMU will only see RIDs from PCIe switch A, but the
> > > current code only looks at DMA aliases that are on the same bus as the
> > > PCIe NTB.  Wouldn't using pci_for_each_dma_alias() handle this
> > > correctly?
> > > 
> > > >  			if (group) {
> > > >  				pci_dev_put(tmp);
> 
> 

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Jacek Lawrynowicz]

On Mon, Feb 29, 2016 at 04:44:17PM -0600, Bjorn Helgaas wrote:
> On Thu, Feb 25, 2016 at 03:41:51PM +0000, Lawrynowicz, Jacek wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas [mailto:helgaas-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> > > Sent: Thursday, February 25, 2016 3:39 PM
> > > To: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > Cc: Lawrynowicz, Jacek <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; linux-
> > > pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; Joerg
> > > Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>; David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>;
> > > iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> > > Subject: Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases
> > > 
> > > On Wed, Feb 24, 2016 at 01:44:06PM -0600, Bjorn Helgaas wrote:
> > > > From: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> What about my questions on the code itself, below?
> 

Sorry but I'm not able to answer it.
Maybe Joerg or David could help here?

> > > > ---
> > > >  drivers/iommu/iommu.c |   17 ++++++++++-------
> > > >  drivers/pci/pci.c     |   11 +++++++++--
> > > >  drivers/pci/probe.c   |    1 +
> > > >  drivers/pci/search.c  |   14 +++++++++-----
> > > >  include/linux/pci.h   |    4 +---
> > > >  5 files changed, 30 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > index 0e3b009..a214e19 100644
> > > > --- a/drivers/iommu/iommu.c
> > > > +++ b/drivers/iommu/iommu.c
> > > > @@ -659,9 +659,15 @@ static struct iommu_group
> > > *get_pci_function_alias_group(struct pci_dev *pdev,
> > > >  	return NULL;
> > > >  }
> > > >
> > > > +static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
> > > > +{
> > > > +	return dev->dma_alias_mask &&
> > > > +	       test_bit(devfn, dev->dma_alias_mask);
> > > > +}
> > > > +
> > > >  /*
> > > > - * Look for aliases to or from the given device for exisiting groups.  The
> > > > - * dma_alias_devfn only supports aliases on the same bus, therefore the
> > > search
> > > > + * Look for aliases to or from the given device for existing groups. DMA
> > > > + * aliases are only supported on the same bus, therefore the search
> > > 
> > > I'm trying to reconcile this statement that "DMA aliases are only
> > > supported on the same bus" (which was there even before this patch)
> > > with the fact that pci_for_each_dma_alias() does not have that
> > > limitation.
> > > 
> > > >   * space is quite small (especially since we're really only looking at pcie
> > > >   * device, and therefore only expect multiple slots on the root complex or
> > > >   * downstream switch ports).  It's conceivable though that a pair of
> > > > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct
> > > pci_dev *pdev,
> > > >  			continue;
> > > >
> > > >  		/* We alias them or they alias us */
> > > > -		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)
> > > &&
> > > > -		     pdev->dma_alias_devfn == tmp->devfn) ||
> > > > -		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > > -		     tmp->dma_alias_devfn == pdev->devfn)) {
> > > > -
> > > > +		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > > > +		    dma_alias_is_enabled(tmp, pdev->devfn)) {
> > > >  			group = get_pci_alias_group(tmp, devfns);
> > > 
> > > We basically have this:
> > > 
> > >   for_each_pci_dev(tmp) {
> > >     if (<pdev and tmp are DMA aliases>)
> > >       group = get_pci_alias_group();
> > >       ...
> > >   }
> > > 
> > > The DMA alias stuff relies on PCI internals, so it doesn't doesn't
> > > seem quite right to use things like PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and
> > > dma_alias_devfn here in the IOMMU code.
> > > 
> > > I'm trying to figure out why we don't do something like the following
> > > instead:
> > > 
> > >   callback(struct pci_dev *pdev, u16 alias, void *opaque)
> > >   {
> > >     struct iommu_group *group;
> > > 
> > >     group = get_pci_alias_group();
> > >     if (group)
> > >       return group;
> > > 
> > >     return 0;
> > >   }
> > > 
> > >   pci_for_each_dma_alias(pdev, callback, ...);
> > > 
> > > Is the existing code some sort of optimization, e.g., checking
> > > PCI_DEV_FLAGS_DMA_ALIAS_DEVFN is cheaper than using
> > > pci_for_each_dma_alias()?
> > > 
> > > It seems like this won't work for some very unlikely but theoretically
> > > possible topologies, e.g.,
> > > 
> > >   PCIe Root Complex/IOMMU
> > >     PCIe switch A
> > >       PCIe to conventional PCI bridge
> > >         PCI to PCIe Root Complex
> > > 	  PCIe NTB
> > > 
> > > Here, I think the IOMMU will only see RIDs from PCIe switch A, but the
> > > current code only looks at DMA aliases that are on the same bus as the
> > > PCIe NTB.  Wouldn't using pci_for_each_dma_alias() handle this
> > > correctly?
> > > 
> > > >  			if (group) {
> > > >  				pci_dev_put(tmp);
> 
> 

[PATCH] PCI: Add support for multiple DMA aliases

[Jacek Lawrynowicz]

This patch solves IOMMU support issues with PCIe non-transparent bridges
that use Requester ID look-up tables (RID-LUT), e.g. PEX8733.

The NTB connects devices in two independent PCI domains. Devices
separated by the NTB are not able to discover each other. A PCI packet
being forwared from one domain to another has to have its RID modified
so it appears on correct bus and completions are forwarded back to the
original domain through the NTB. RID is translated using preprogrammed
table (LUT) and the PCI packet propagates upstream away from the NTB.
If the destination system has IOMMU enabled, the packet will be
discarded because the new RID is unknown to the IOMMU. Adding a DMA
alias for the new RID allows IOMMU to properly recognize the packet.

Each device behind the NTB has a unique RID assigned in the RID-LUT.
Current DMA alias implementation supports only single alias, so it's not
possible to support mutiple devices behind the NTB when IOMMU is enabled.

This implementation enables all possible aliases on a given bus (256)
that are stored in a bitset. Alias devfn is directly translated to a bit
number. The bitset is not allocated for devices that have no need for
DMA aliases.

More details can be found in following article:
http://www.plxtech.com/files/pdf/technical/expresslane/RTC_Enabling%20MulitHostSystemDesigns.pdf

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
Acked-by: David Woodhouse <David.Woodhouse@intel.com>
Acked-by: Joerg Roedel <jroedel@suse.de>
---
 I updated the commit message based on discussion with Bjorn. It should be
 now a little easier to understand. I'm not resubmitting the whole patch set
 because it could make the thread harder to follow.

 drivers/iommu/iommu.c | 17 ++++++++++-------
 drivers/pci/pci.c     | 11 +++++++++--
 drivers/pci/probe.c   |  1 +
 drivers/pci/search.c  | 14 +++++++++-----
 include/linux/pci.h   |  4 +---
 5 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index bfd4f7c..4c10da1 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -659,9 +659,15 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
 	return NULL;
 }
 
+static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
+{
+	return dev->dma_alias_mask &&
+	       test_bit(devfn, dev->dma_alias_mask);
+}
+
 /*
- * Look for aliases to or from the given device for exisiting groups.  The
- * dma_alias_devfn only supports aliases on the same bus, therefore the search
+ * Look for aliases to or from the given device for existing groups. DMA
+ * aliases are only supported on the same bus, therefore the search
  * space is quite small (especially since we're really only looking at pcie
  * device, and therefore only expect multiple slots on the root complex or
  * downstream switch ports).  It's conceivable though that a pair of
@@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
 			continue;
 
 		/* We alias them or they alias us */
-		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
-		     pdev->dma_alias_devfn == tmp->devfn) ||
-		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
-		     tmp->dma_alias_devfn == pdev->devfn)) {
-
+		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
+		    dma_alias_is_enabled(tmp, pdev->devfn)) {
 			group = get_pci_alias_group(tmp, devfns);
 			if (group) {
 				pci_dev_put(tmp);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e5e2c9d..33f3d24 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4577,8 +4577,15 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  */
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
-	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
-	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+	if (!dev->dma_alias_mask)
+		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
+					      sizeof(long), GFP_KERNEL);
+	if (!dev->dma_alias_mask) {
+		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
+		return;
+	}
+
+	set_bit(devfn, dev->dma_alias_mask);
 	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
 		 PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5eb378f..cf09307 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1502,6 +1502,7 @@ static void pci_release_dev(struct device *dev)
 	pcibios_release_device(pci_dev);
 	pci_bus_put(pci_dev->bus);
 	kfree(pci_dev->driver_override);
+	kfree(pci_dev->dma_alias_mask);
 	kfree(pci_dev);
 }
 
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index a20ce7d..33e0f03 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -40,11 +40,15 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
 	 * If the device is broken and uses an alias requester ID for
 	 * DMA, iterate over that too.
 	 */
-	if (unlikely(pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)) {
-		ret = fn(pdev, PCI_DEVID(pdev->bus->number,
-					 pdev->dma_alias_devfn), data);
-		if (ret)
-			return ret;
+	if (unlikely(pdev->dma_alias_mask)) {
+		u8 devfn;
+
+		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
+			ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
+				 data);
+			if (ret)
+				return ret;
+		}
 	}
 
 	for (bus = pdev->bus; !pci_is_root_bus(bus); bus = bus->parent) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 614d70d..0c176e5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -172,8 +172,6 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) (1 << 2),
 	/* Flag for quirk use to store if quirk-specific ACS is enabled */
 	PCI_DEV_FLAGS_ACS_ENABLED_QUIRK = (__force pci_dev_flags_t) (1 << 3),
-	/* Flag to indicate the device uses dma_alias_devfn */
-	PCI_DEV_FLAGS_DMA_ALIAS_DEVFN = (__force pci_dev_flags_t) (1 << 4),
 	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
 	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 5),
 	/* Do not use bus resets for device */
@@ -279,7 +277,7 @@ struct pci_dev {
 	u8		rom_base_reg;	/* which config register controls the ROM */
 	u8		pin;		/* which interrupt pin this device uses */
 	u16		pcie_flags_reg;	/* cached PCIe Capabilities Register */
-	u8		dma_alias_devfn;/* devfn of DMA alias, if any */
+	unsigned long	*dma_alias_mask;/* mask of enabled devfn aliases */
 
 	struct pci_driver *driver;	/* which driver has allocated this device */
 	u64		dma_mask;	/* Mask of the bits of bus address this
-- 
1.8.3.1

[PATCH] PCI: Add support for multiple DMA aliases

[Jacek Lawrynowicz]

This patch solves IOMMU support issues with PCIe non-transparent bridges
that use Requester ID look-up tables (RID-LUT), e.g. PEX8733.

The NTB connects devices in two independent PCI domains. Devices
separated by the NTB are not able to discover each other. A PCI packet
being forwared from one domain to another has to have its RID modified
so it appears on correct bus and completions are forwarded back to the
original domain through the NTB. RID is translated using preprogrammed
table (LUT) and the PCI packet propagates upstream away from the NTB.
If the destination system has IOMMU enabled, the packet will be
discarded because the new RID is unknown to the IOMMU. Adding a DMA
alias for the new RID allows IOMMU to properly recognize the packet.

Each device behind the NTB has a unique RID assigned in the RID-LUT.
Current DMA alias implementation supports only single alias, so it's not
possible to support mutiple devices behind the NTB when IOMMU is enabled.

This implementation enables all possible aliases on a given bus (256)
that are stored in a bitset. Alias devfn is directly translated to a bit
number. The bitset is not allocated for devices that have no need for
DMA aliases.

More details can be found in following article:
http://www.plxtech.com/files/pdf/technical/expresslane/RTC_Enabling%20MulitHostSystemDesigns.pdf

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Acked-by: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Acked-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 I updated the commit message based on discussion with Bjorn. It should be
 now a little easier to understand. I'm not resubmitting the whole patch set
 because it could make the thread harder to follow.

 drivers/iommu/iommu.c | 17 ++++++++++-------
 drivers/pci/pci.c     | 11 +++++++++--
 drivers/pci/probe.c   |  1 +
 drivers/pci/search.c  | 14 +++++++++-----
 include/linux/pci.h   |  4 +---
 5 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index bfd4f7c..4c10da1 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -659,9 +659,15 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
 	return NULL;
 }
 
+static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
+{
+	return dev->dma_alias_mask &&
+	       test_bit(devfn, dev->dma_alias_mask);
+}
+
 /*
- * Look for aliases to or from the given device for exisiting groups.  The
- * dma_alias_devfn only supports aliases on the same bus, therefore the search
+ * Look for aliases to or from the given device for existing groups. DMA
+ * aliases are only supported on the same bus, therefore the search
  * space is quite small (especially since we're really only looking at pcie
  * device, and therefore only expect multiple slots on the root complex or
  * downstream switch ports).  It's conceivable though that a pair of
@@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
 			continue;
 
 		/* We alias them or they alias us */
-		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
-		     pdev->dma_alias_devfn == tmp->devfn) ||
-		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
-		     tmp->dma_alias_devfn == pdev->devfn)) {
-
+		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
+		    dma_alias_is_enabled(tmp, pdev->devfn)) {
 			group = get_pci_alias_group(tmp, devfns);
 			if (group) {
 				pci_dev_put(tmp);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e5e2c9d..33f3d24 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4577,8 +4577,15 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  */
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
-	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
-	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+	if (!dev->dma_alias_mask)
+		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
+					      sizeof(long), GFP_KERNEL);
+	if (!dev->dma_alias_mask) {
+		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
+		return;
+	}
+
+	set_bit(devfn, dev->dma_alias_mask);
 	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
 		 PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5eb378f..cf09307 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1502,6 +1502,7 @@ static void pci_release_dev(struct device *dev)
 	pcibios_release_device(pci_dev);
 	pci_bus_put(pci_dev->bus);
 	kfree(pci_dev->driver_override);
+	kfree(pci_dev->dma_alias_mask);
 	kfree(pci_dev);
 }
 
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index a20ce7d..33e0f03 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -40,11 +40,15 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
 	 * If the device is broken and uses an alias requester ID for
 	 * DMA, iterate over that too.
 	 */
-	if (unlikely(pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)) {
-		ret = fn(pdev, PCI_DEVID(pdev->bus->number,
-					 pdev->dma_alias_devfn), data);
-		if (ret)
-			return ret;
+	if (unlikely(pdev->dma_alias_mask)) {
+		u8 devfn;
+
+		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
+			ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
+				 data);
+			if (ret)
+				return ret;
+		}
 	}
 
 	for (bus = pdev->bus; !pci_is_root_bus(bus); bus = bus->parent) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 614d70d..0c176e5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -172,8 +172,6 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) (1 << 2),
 	/* Flag for quirk use to store if quirk-specific ACS is enabled */
 	PCI_DEV_FLAGS_ACS_ENABLED_QUIRK = (__force pci_dev_flags_t) (1 << 3),
-	/* Flag to indicate the device uses dma_alias_devfn */
-	PCI_DEV_FLAGS_DMA_ALIAS_DEVFN = (__force pci_dev_flags_t) (1 << 4),
 	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
 	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 5),
 	/* Do not use bus resets for device */
@@ -279,7 +277,7 @@ struct pci_dev {
 	u8		rom_base_reg;	/* which config register controls the ROM */
 	u8		pin;		/* which interrupt pin this device uses */
 	u16		pcie_flags_reg;	/* cached PCIe Capabilities Register */
-	u8		dma_alias_devfn;/* devfn of DMA alias, if any */
+	unsigned long	*dma_alias_mask;/* mask of enabled devfn aliases */
 
 	struct pci_driver *driver;	/* which driver has allocated this device */
 	u64		dma_mask;	/* Mask of the bits of bus address this
-- 
1.8.3.1

[PATCH v5 3/6] PCI: Add support for multiple DMA aliases

[Jacek Lawrynowicz]

This patch solves IOMMU support issues with PCIe non-transparent bridges
that use Requester ID look-up tables (RID-LUT), e.g. PEX8733.

The NTB connects devices in two independent PCI domains. Devices
separated by the NTB are not able to discover each other. A PCI packet
being forwared from one domain to another has to have its RID modified
so it appears on correct bus and completions are forwarded back to the
original domain through the NTB. RID is translated using preprogrammed
table (LUT) and the PCI packet propagates upstream away from the NTB.
If the destination system has IOMMU enabled, the packet will be
discarded because the new RID is unknown to the IOMMU. Adding a DMA
alias for the new RID allows IOMMU to properly recognize the packet.

Each device behind the NTB has a unique RID assigned in the RID-LUT.
Current DMA alias implementation supports only single alias, so it's not
possible to support mutiple devices behind the NTB when IOMMU is enabled.

This implementation enables all possible aliases on a given bus (256)
that are stored in a bitset. Alias devfn is directly translated to a bit
number. The bitset is not allocated for devices that have no need for
DMA aliases.

More details can be found in following article:
http://www.plxtech.com/files/pdf/technical/expresslane/RTC_Enabling%20MulitHostSystemDesigns.pdf

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
Acked-by: David Woodhouse <David.Woodhouse@intel.com>
Acked-by: Joerg Roedel <jroedel@suse.de>
---
 I updated the commit message based on discussion with Bjorn. It should be
 now a little easier to understand. I'm not resubmitting the whole patch set
 because it could make the thread harder to follow.
 
 This time resubmitting with correct subject.

 drivers/iommu/iommu.c | 17 ++++++++++-------
 drivers/pci/pci.c     | 11 +++++++++--
 drivers/pci/probe.c   |  1 +
 drivers/pci/search.c  | 14 +++++++++-----
 include/linux/pci.h   |  4 +---
 5 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index bfd4f7c..4c10da1 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -659,9 +659,15 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
 	return NULL;
 }
 
+static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
+{
+	return dev->dma_alias_mask &&
+	       test_bit(devfn, dev->dma_alias_mask);
+}
+
 /*
- * Look for aliases to or from the given device for exisiting groups.  The
- * dma_alias_devfn only supports aliases on the same bus, therefore the search
+ * Look for aliases to or from the given device for existing groups. DMA
+ * aliases are only supported on the same bus, therefore the search
  * space is quite small (especially since we're really only looking at pcie
  * device, and therefore only expect multiple slots on the root complex or
  * downstream switch ports).  It's conceivable though that a pair of
@@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
 			continue;
 
 		/* We alias them or they alias us */
-		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
-		     pdev->dma_alias_devfn == tmp->devfn) ||
-		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
-		     tmp->dma_alias_devfn == pdev->devfn)) {
-
+		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
+		    dma_alias_is_enabled(tmp, pdev->devfn)) {
 			group = get_pci_alias_group(tmp, devfns);
 			if (group) {
 				pci_dev_put(tmp);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e5e2c9d..33f3d24 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4577,8 +4577,15 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  */
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
-	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
-	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+	if (!dev->dma_alias_mask)
+		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
+					      sizeof(long), GFP_KERNEL);
+	if (!dev->dma_alias_mask) {
+		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
+		return;
+	}
+
+	set_bit(devfn, dev->dma_alias_mask);
 	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
 		 PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5eb378f..cf09307 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1502,6 +1502,7 @@ static void pci_release_dev(struct device *dev)
 	pcibios_release_device(pci_dev);
 	pci_bus_put(pci_dev->bus);
 	kfree(pci_dev->driver_override);
+	kfree(pci_dev->dma_alias_mask);
 	kfree(pci_dev);
 }
 
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index a20ce7d..33e0f03 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -40,11 +40,15 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
 	 * If the device is broken and uses an alias requester ID for
 	 * DMA, iterate over that too.
 	 */
-	if (unlikely(pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)) {
-		ret = fn(pdev, PCI_DEVID(pdev->bus->number,
-					 pdev->dma_alias_devfn), data);
-		if (ret)
-			return ret;
+	if (unlikely(pdev->dma_alias_mask)) {
+		u8 devfn;
+
+		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
+			ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
+				 data);
+			if (ret)
+				return ret;
+		}
 	}
 
 	for (bus = pdev->bus; !pci_is_root_bus(bus); bus = bus->parent) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 614d70d..0c176e5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -172,8 +172,6 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) (1 << 2),
 	/* Flag for quirk use to store if quirk-specific ACS is enabled */
 	PCI_DEV_FLAGS_ACS_ENABLED_QUIRK = (__force pci_dev_flags_t) (1 << 3),
-	/* Flag to indicate the device uses dma_alias_devfn */
-	PCI_DEV_FLAGS_DMA_ALIAS_DEVFN = (__force pci_dev_flags_t) (1 << 4),
 	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
 	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 5),
 	/* Do not use bus resets for device */
@@ -279,7 +277,7 @@ struct pci_dev {
 	u8		rom_base_reg;	/* which config register controls the ROM */
 	u8		pin;		/* which interrupt pin this device uses */
 	u16		pcie_flags_reg;	/* cached PCIe Capabilities Register */
-	u8		dma_alias_devfn;/* devfn of DMA alias, if any */
+	unsigned long	*dma_alias_mask;/* mask of enabled devfn aliases */
 
 	struct pci_driver *driver;	/* which driver has allocated this device */
 	u64		dma_mask;	/* Mask of the bits of bus address this
-- 
1.8.3.1

[PATCH v5 5/6] PCI: Add DMA alias quirk for mic_x200_dma

[Jacek Lawrynowicz]

MIC x200 NTB forwards PCIe traffic using multiple alien RID. They have to
be added as aliases to the DMA device in order to allow buffer access
when IOMMU is enabled.

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
Acked-by: David Woodhouse <David.Woodhouse@intel.com>
---
 Updated quirk comment with requirement that aliases have to be matching
 values programmed in the EEPROM. The LUT table on x200 is static so there
 is no need to program it from the driver.

 drivers/pci/quirks.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 51927a5..e66f09d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3696,6 +3696,20 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
 
 /*
+ * MIC x200 NTB forwards PCIe traffic using multiple alien RIDs. They have to
+ * be added as aliases to the DMA device in order to allow buffer access
+ * when IOMMU is enabled. Following devfns have to match RIT-LUT table
+ * programmed in the EEPROM.
+ */
+static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
+{
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */
-- 
1.8.3.1

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3109 bytes --]

On Thu, 2016-02-25 at 08:38 -0600, Bjorn Helgaas wrote:
> 
> >  /*
> > - * Look for aliases to or from the given device for exisiting groups.  The
> > - * dma_alias_devfn only supports aliases on the same bus, therefore the search
> > + * Look for aliases to or from the given device for existing groups. DMA
> > + * aliases are only supported on the same bus, therefore the search
> 
> I'm trying to reconcile this statement that "DMA aliases are only
> supported on the same bus" (which was there even before this patch)
> with the fact that pci_for_each_dma_alias() does not have that
> limitation.

Doesn't it? You can still only set a DMA alias on the same bus with
pci_add_dma_alias(), can't you?

> >   * space is quite small (especially since we're really only looking at pcie
> >   * device, and therefore only expect multiple slots on the root complex or
> >   * downstream switch ports).  It's conceivable though that a pair of
> > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
> >                       continue;
> >  
> >               /* We alias them or they alias us */
> > -             if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > -                  pdev->dma_alias_devfn == tmp->devfn) ||
> > -                 ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > -                  tmp->dma_alias_devfn == pdev->devfn)) {
> > -
> > +             if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > +                 dma_alias_is_enabled(tmp, pdev->devfn)) {
> >                       group = get_pci_alias_group(tmp, devfns);
> 
> We basically have this:
> 
>   for_each_pci_dev(tmp) {
>     if ()
>       group = get_pci_alias_group();
>       ...
>   }

Strictly, that's:

 for_each_pci_dev(tmp) {
   if (pdev is an alias of tmp || tmp is an alias of pdev)
     group = get_pci_alias_group();
   ...
 }

> The DMA alias stuff relies on PCI internals, so it doesn't doesn't
> seem quite right to use things like PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and
> dma_alias_devfn here in the IOMMU code.  
>
> I'm trying to figure out why we don't do something like the following
> instead:
> 
>   callback(struct pci_dev *pdev, u16 alias, void *opaque)
>   {
>     struct iommu_group *group;
> 
>     group = get_pci_alias_group();
>     if (group)
>       return group;
> 
>     return 0;
>   }
> 
>   pci_for_each_dma_alias(pdev, callback, ...);

And this would be equivalent to

 for_each_pci_dev(tmp) {
   if (tmp is an alias of pdev)
     group = get_pci_alias_group();
   ...
 }

The "is an alias of" property is not commutative. Perhaps it should be.
But that's hard because in some cases the alias doesn't even *exist* as
a real PCI device. It's just that you appear to get DMA transactions
from a given source-id.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Bjorn Helgaas]

On Mon, Mar 14, 2016 at 10:43:40PM +0000, David Woodhouse wrote:
> On Thu, 2016-02-25 at 08:38 -0600, Bjorn Helgaas wrote:
> > 
> > >  /*
> > > - * Look for aliases to or from the given device for exisiting groups.  The
> > > - * dma_alias_devfn only supports aliases on the same bus, therefore the search
> > > + * Look for aliases to or from the given device for existing groups. DMA
> > > + * aliases are only supported on the same bus, therefore the search
> > 
> > I'm trying to reconcile this statement that "DMA aliases are only
> > supported on the same bus" (which was there even before this patch)
> > with the fact that pci_for_each_dma_alias() does not have that
> > limitation.
> 
> Doesn't it? You can still only set a DMA alias on the same bus with
> pci_add_dma_alias(), can't you?

I guess it's true that PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and the proposed
pci_add_dma_alias() only add aliases on the same bus.  I was thinking
about a scenario like this:

  00:00.0 PCIe-to-PCI bridge to [bus 01]
  01:01.0 conventional PCI device

where I think 01:00.0 is a DMA alias for 01:01.0 because the bridge
takes ownership of DMA transactions from 01:01.0 and assigns a
Requester ID of 01:00.0 (secondary bus number, device 0, function 0).

> > >   * space is quite small (especially since we're really only looking at pcie
> > >   * device, and therefore only expect multiple slots on the root complex or
> > >   * downstream switch ports).  It's conceivable though that a pair of
> > > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
> > >                       continue;
> > >  
> > >               /* We alias them or they alias us */
> > > -             if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > -                  pdev->dma_alias_devfn == tmp->devfn) ||
> > > -                 ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > -                  tmp->dma_alias_devfn == pdev->devfn)) {
> > > -
> > > +             if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > > +                 dma_alias_is_enabled(tmp, pdev->devfn)) {
> > >                       group = get_pci_alias_group(tmp, devfns);
> > 
> > We basically have this:
> > 
> >   for_each_pci_dev(tmp) {
> >     if ()
> >       group = get_pci_alias_group();
> >       ...
> >   }
> 
> Strictly, that's:
> 
>  for_each_pci_dev(tmp) {
>    if (pdev is an alias of tmp || tmp is an alias of pdev)
>      group = get_pci_alias_group();
>    ...
>  }

OK.  

> > I'm trying to figure out why we don't do something like the following
> > instead:
> > 
> >   callback(struct pci_dev *pdev, u16 alias, void *opaque)
> >   {
> >     struct iommu_group *group;
> > 
> >     group = get_pci_alias_group();
> >     if (group)
> >       return group;
> > 
> >     return 0;
> >   }
> > 
> >   pci_for_each_dma_alias(pdev, callback, ...);
> 
> And this would be equivalent to
> 
>  for_each_pci_dev(tmp) {
>    if (tmp is an alias of pdev)
>      group = get_pci_alias_group();
>    ...
>  }
> 
> The "is an alias of" property is not commutative. Perhaps it should be.
> But that's hard because in some cases the alias doesn't even *exist* as
> a real PCI device. It's just that you appear to get DMA transactions
> from a given source-id.

Right.  In my example above, 01:00.0 is not a PCI device; it's only a
Requester ID that is fabricated by the bridge when it forwards DMA
transactions upstream.

I think I'm confused because I don't really understand IOMMU groups.

Let me explain what I think they are and you can correct me when I go
wrong.  The iommu_group_alloc() comment says "The IOMMU group
represents the minimum granularity of the IOMMU."  So I suppose the
IOMMU cannot distinguish between devices in a group.  All the devices
in the group use the same set of DMA mappings.  Granting device A DMA
access to a buffer grants the same access to all other members of A's
IOMMU group.

That would mean my question was fundamentally backwards.  In
get_pci_alias_group(A), we're not trying to figure out what all the
aliases of A are, which is what pci_for_each_dma_alias() does.

Instead, we're trying to figure out which IOMMU group A belongs to.
But I still don't quite understand how aliases fit into this.  Let's
go back to my example and assume we've already put 00:00.0 and 01:01.0
in IOMMU groups:

  00:00.0 PCIe-to-PCI bridge to [bus 01]     # in IOMMU group G0
  01:01.0 conventional PCI device            # in IOMMU group G1

I assume these devices are in different IOMMU groups because if the
bridge generated its own DMA, it would use Requester ID 00:00.0, which
is distinct from the 01:00.0 it would use when forwarding DMAs from
its secondary side.

What happens when we add 01:02.0?  I think 01:01.0 and 01:02.0 should
both end up in IOMMU group G1 because the IOMMU will see only
Requester ID 01:00.0, so it can't distinguish them.

When we add 01:02.0, the ops->add_device() ... ops->device_group()
path calls pci_device_group(01:02.0):

  pci_device_group(01:02.0)
    pci_for_each_dma_alias(01:02.0, get_pci_alias_or_group)
      get_pci_alias_or_group(01:02.0, 01:02.0)   # callback
        return 0           # 01:02.0 group not set yet
      get_pci_alias_or_group(00:00.0, 01:00.0)   # callback
        return 1           # 00:00.0 is in G0

It seems like we'll assign 01:02.0 to group G0, when I think it should
be in G1.  Where did I go wrong?

Bjorn

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Bjorn Helgaas]

On Mon, Mar 14, 2016 at 10:43:40PM +0000, David Woodhouse wrote:
> On Thu, 2016-02-25 at 08:38 -0600, Bjorn Helgaas wrote:
> > 
> > >  /*
> > > - * Look for aliases to or from the given device for exisiting groups.  The
> > > - * dma_alias_devfn only supports aliases on the same bus, therefore the search
> > > + * Look for aliases to or from the given device for existing groups. DMA
> > > + * aliases are only supported on the same bus, therefore the search
> > 
> > I'm trying to reconcile this statement that "DMA aliases are only
> > supported on the same bus" (which was there even before this patch)
> > with the fact that pci_for_each_dma_alias() does not have that
> > limitation.
> 
> Doesn't it? You can still only set a DMA alias on the same bus with
> pci_add_dma_alias(), can't you?

I guess it's true that PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and the proposed
pci_add_dma_alias() only add aliases on the same bus.  I was thinking
about a scenario like this:

  00:00.0 PCIe-to-PCI bridge to [bus 01]
  01:01.0 conventional PCI device

where I think 01:00.0 is a DMA alias for 01:01.0 because the bridge
takes ownership of DMA transactions from 01:01.0 and assigns a
Requester ID of 01:00.0 (secondary bus number, device 0, function 0).

> > >   * space is quite small (especially since we're really only looking at pcie
> > >   * device, and therefore only expect multiple slots on the root complex or
> > >   * downstream switch ports).  It's conceivable though that a pair of
> > > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
> > >                       continue;
> > >  
> > >               /* We alias them or they alias us */
> > > -             if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > -                  pdev->dma_alias_devfn == tmp->devfn) ||
> > > -                 ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > -                  tmp->dma_alias_devfn == pdev->devfn)) {
> > > -
> > > +             if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > > +                 dma_alias_is_enabled(tmp, pdev->devfn)) {
> > >                       group = get_pci_alias_group(tmp, devfns);
> > 
> > We basically have this:
> > 
> >   for_each_pci_dev(tmp) {
> >     if ()
> >       group = get_pci_alias_group();
> >       ...
> >   }
> 
> Strictly, that's:
> 
>  for_each_pci_dev(tmp) {
>    if (pdev is an alias of tmp || tmp is an alias of pdev)
>      group = get_pci_alias_group();
>    ...
>  }

OK.  

> > I'm trying to figure out why we don't do something like the following
> > instead:
> > 
> >   callback(struct pci_dev *pdev, u16 alias, void *opaque)
> >   {
> >     struct iommu_group *group;
> > 
> >     group = get_pci_alias_group();
> >     if (group)
> >       return group;
> > 
> >     return 0;
> >   }
> > 
> >   pci_for_each_dma_alias(pdev, callback, ...);
> 
> And this would be equivalent to
> 
>  for_each_pci_dev(tmp) {
>    if (tmp is an alias of pdev)
>      group = get_pci_alias_group();
>    ...
>  }
> 
> The "is an alias of" property is not commutative. Perhaps it should be.
> But that's hard because in some cases the alias doesn't even *exist* as
> a real PCI device. It's just that you appear to get DMA transactions
> from a given source-id.

Right.  In my example above, 01:00.0 is not a PCI device; it's only a
Requester ID that is fabricated by the bridge when it forwards DMA
transactions upstream.

I think I'm confused because I don't really understand IOMMU groups.

Let me explain what I think they are and you can correct me when I go
wrong.  The iommu_group_alloc() comment says "The IOMMU group
represents the minimum granularity of the IOMMU."  So I suppose the
IOMMU cannot distinguish between devices in a group.  All the devices
in the group use the same set of DMA mappings.  Granting device A DMA
access to a buffer grants the same access to all other members of A's
IOMMU group.

That would mean my question was fundamentally backwards.  In
get_pci_alias_group(A), we're not trying to figure out what all the
aliases of A are, which is what pci_for_each_dma_alias() does.

Instead, we're trying to figure out which IOMMU group A belongs to.
But I still don't quite understand how aliases fit into this.  Let's
go back to my example and assume we've already put 00:00.0 and 01:01.0
in IOMMU groups:

  00:00.0 PCIe-to-PCI bridge to [bus 01]     # in IOMMU group G0
  01:01.0 conventional PCI device            # in IOMMU group G1

I assume these devices are in different IOMMU groups because if the
bridge generated its own DMA, it would use Requester ID 00:00.0, which
is distinct from the 01:00.0 it would use when forwarding DMAs from
its secondary side.

What happens when we add 01:02.0?  I think 01:01.0 and 01:02.0 should
both end up in IOMMU group G1 because the IOMMU will see only
Requester ID 01:00.0, so it can't distinguish them.

When we add 01:02.0, the ops->add_device() ... ops->device_group()
path calls pci_device_group(01:02.0):

  pci_device_group(01:02.0)
    pci_for_each_dma_alias(01:02.0, get_pci_alias_or_group)
      get_pci_alias_or_group(01:02.0, 01:02.0)   # callback
        return 0           # 01:02.0 group not set yet
      get_pci_alias_or_group(00:00.0, 01:00.0)   # callback
        return 1           # 00:00.0 is in G0

It seems like we'll assign 01:02.0 to group G0, when I think it should
be in G1.  Where did I go wrong?

Bjorn

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Bjorn Helgaas]

On Tue, Mar 15, 2016 at 07:48:17PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 14, 2016 at 10:43:40PM +0000, David Woodhouse wrote:
> > On Thu, 2016-02-25 at 08:38 -0600, Bjorn Helgaas wrote:
> > > 
> > > >  /*
> > > > - * Look for aliases to or from the given device for exisiting groups.  The
> > > > - * dma_alias_devfn only supports aliases on the same bus, therefore the search
> > > > + * Look for aliases to or from the given device for existing groups. DMA
> > > > + * aliases are only supported on the same bus, therefore the search
> > > 
> > > I'm trying to reconcile this statement that "DMA aliases are only
> > > supported on the same bus" (which was there even before this patch)
> > > with the fact that pci_for_each_dma_alias() does not have that
> > > limitation.
> > 
> > Doesn't it? You can still only set a DMA alias on the same bus with
> > pci_add_dma_alias(), can't you?
> 
> I guess it's true that PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and the proposed
> pci_add_dma_alias() only add aliases on the same bus.  I was thinking
> about a scenario like this:
> 
>   00:00.0 PCIe-to-PCI bridge to [bus 01]
>   01:01.0 conventional PCI device
> 
> where I think 01:00.0 is a DMA alias for 01:01.0 because the bridge
> takes ownership of DMA transactions from 01:01.0 and assigns a
> Requester ID of 01:00.0 (secondary bus number, device 0, function 0).
> 
> > > >   * space is quite small (especially since we're really only looking at pcie
> > > >   * device, and therefore only expect multiple slots on the root complex or
> > > >   * downstream switch ports).  It's conceivable though that a pair of
> > > > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
> > > >                       continue;
> > > >  
> > > >               /* We alias them or they alias us */
> > > > -             if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > > -                  pdev->dma_alias_devfn == tmp->devfn) ||
> > > > -                 ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > > -                  tmp->dma_alias_devfn == pdev->devfn)) {
> > > > -
> > > > +             if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > > > +                 dma_alias_is_enabled(tmp, pdev->devfn)) {
> > > >                       group = get_pci_alias_group(tmp, devfns);
> > > 
> > > We basically have this:
> > > 
> > >   for_each_pci_dev(tmp) {
> > >     if ()
> > >       group = get_pci_alias_group();
> > >       ...
> > >   }
> > 
> > Strictly, that's:
> > 
> >  for_each_pci_dev(tmp) {
> >    if (pdev is an alias of tmp || tmp is an alias of pdev)
> >      group = get_pci_alias_group();
> >    ...
> >  }
> 
> OK.  
> 
> > > I'm trying to figure out why we don't do something like the following
> > > instead:
> > > 
> > >   callback(struct pci_dev *pdev, u16 alias, void *opaque)
> > >   {
> > >     struct iommu_group *group;
> > > 
> > >     group = get_pci_alias_group();
> > >     if (group)
> > >       return group;
> > > 
> > >     return 0;
> > >   }
> > > 
> > >   pci_for_each_dma_alias(pdev, callback, ...);
> > 
> > And this would be equivalent to
> > 
> >  for_each_pci_dev(tmp) {
> >    if (tmp is an alias of pdev)
> >      group = get_pci_alias_group();
> >    ...
> >  }
> > 
> > The "is an alias of" property is not commutative. Perhaps it should be.
> > But that's hard because in some cases the alias doesn't even *exist* as
> > a real PCI device. It's just that you appear to get DMA transactions
> > from a given source-id.
> 
> Right.  In my example above, 01:00.0 is not a PCI device; it's only a
> Requester ID that is fabricated by the bridge when it forwards DMA
> transactions upstream.
> 
> I think I'm confused because I don't really understand IOMMU groups.
> 
> Let me explain what I think they are and you can correct me when I go
> wrong.  The iommu_group_alloc() comment says "The IOMMU group
> represents the minimum granularity of the IOMMU."  So I suppose the
> IOMMU cannot distinguish between devices in a group.  All the devices
> in the group use the same set of DMA mappings.  Granting device A DMA
> access to a buffer grants the same access to all other members of A's
> IOMMU group.
> 
> That would mean my question was fundamentally backwards.  In
> get_pci_alias_group(A), we're not trying to figure out what all the
> aliases of A are, which is what pci_for_each_dma_alias() does.
> 
> Instead, we're trying to figure out which IOMMU group A belongs to.
> But I still don't quite understand how aliases fit into this.  Let's
> go back to my example and assume we've already put 00:00.0 and 01:01.0
> in IOMMU groups:
> 
>   00:00.0 PCIe-to-PCI bridge to [bus 01]     # in IOMMU group G0
>   01:01.0 conventional PCI device            # in IOMMU group G1
> 
> I assume these devices are in different IOMMU groups because if the
> bridge generated its own DMA, it would use Requester ID 00:00.0, which
> is distinct from the 01:00.0 it would use when forwarding DMAs from
> its secondary side.
> 
> What happens when we add 01:02.0?  I think 01:01.0 and 01:02.0 should
> both end up in IOMMU group G1 because the IOMMU will see only
> Requester ID 01:00.0, so it can't distinguish them.
> 
> When we add 01:02.0, the ops->add_device() ... ops->device_group()
> path calls pci_device_group(01:02.0):
> 
>   pci_device_group(01:02.0)
>     pci_for_each_dma_alias(01:02.0, get_pci_alias_or_group)
>       get_pci_alias_or_group(01:02.0, 01:02.0)   # callback
>         return 0           # 01:02.0 group not set yet
>       get_pci_alias_or_group(00:00.0, 01:00.0)   # callback
>         return 1           # 00:00.0 is in G0
> 
> It seems like we'll assign 01:02.0 to group G0, when I think it should
> be in G1.  Where did I go wrong?

Ping?

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 226 bytes --]

On Fri, 2016-04-08 at 11:06 -0500, Bjorn Helgaas wrote:
> > I think I'm confused because I don't really understand IOMMU
> > groups.
,,,

> Ping?

I think this ended up being more of a Alex question...

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 226 bytes --]

On Fri, 2016-04-08 at 11:06 -0500, Bjorn Helgaas wrote:
> > I think I'm confused because I don't really understand IOMMU
> > groups.
,,,

> Ping?

I think this ended up being more of a Alex question...

-- 
dwmw2


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Alex Williamson]

On Fri, 8 Apr 2016 11:06:32 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Tue, Mar 15, 2016 at 07:48:17PM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 14, 2016 at 10:43:40PM +0000, David Woodhouse wrote:  
> > > On Thu, 2016-02-25 at 08:38 -0600, Bjorn Helgaas wrote:  
> > > >   
> > > > >  /*
> > > > > - * Look for aliases to or from the given device for exisiting groups.  The
> > > > > - * dma_alias_devfn only supports aliases on the same bus, therefore the search
> > > > > + * Look for aliases to or from the given device for existing groups. DMA
> > > > > + * aliases are only supported on the same bus, therefore the search  
> > > > 
> > > > I'm trying to reconcile this statement that "DMA aliases are only
> > > > supported on the same bus" (which was there even before this patch)
> > > > with the fact that pci_for_each_dma_alias() does not have that
> > > > limitation.  
> > > 
> > > Doesn't it? You can still only set a DMA alias on the same bus with
> > > pci_add_dma_alias(), can't you?  
> > 
> > I guess it's true that PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and the proposed
> > pci_add_dma_alias() only add aliases on the same bus.  I was thinking
> > about a scenario like this:
> > 
> >   00:00.0 PCIe-to-PCI bridge to [bus 01]
> >   01:01.0 conventional PCI device
> > 
> > where I think 01:00.0 is a DMA alias for 01:01.0 because the bridge
> > takes ownership of DMA transactions from 01:01.0 and assigns a
> > Requester ID of 01:00.0 (secondary bus number, device 0, function 0).

This is true, but this is a topology alias, not a quirk.
pci_for_each_dma_alias() already handles this case.  It would trigger
the callback function for any direct alias of the conventional device
as well as the bridge itself.  Obviously we don't end up with any
quirks for conventional devices because the aliases are masked behind
the bridge anyway.
 
> > > > >   * space is quite small (especially since we're really only looking at pcie
> > > > >   * device, and therefore only expect multiple slots on the root complex or
> > > > >   * downstream switch ports).  It's conceivable though that a pair of
> > > > > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
> > > > >                       continue;
> > > > >  
> > > > >               /* We alias them or they alias us */
> > > > > -             if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > > > -                  pdev->dma_alias_devfn == tmp->devfn) ||
> > > > > -                 ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > > > -                  tmp->dma_alias_devfn == pdev->devfn)) {
> > > > > -
> > > > > +             if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > > > > +                 dma_alias_is_enabled(tmp, pdev->devfn)) {
> > > > >                       group = get_pci_alias_group(tmp, devfns);  
> > > > 
> > > > We basically have this:
> > > > 
> > > >   for_each_pci_dev(tmp) {
> > > >     if ()
> > > >       group = get_pci_alias_group();
> > > >       ...
> > > >   }  
> > > 
> > > Strictly, that's:
> > > 
> > >  for_each_pci_dev(tmp) {
> > >    if (pdev is an alias of tmp || tmp is an alias of pdev)
> > >      group = get_pci_alias_group();
> > >    ...
> > >  }  
> > 
> > OK.  
> >   
> > > > I'm trying to figure out why we don't do something like the following
> > > > instead:
> > > > 
> > > >   callback(struct pci_dev *pdev, u16 alias, void *opaque)
> > > >   {
> > > >     struct iommu_group *group;
> > > > 
> > > >     group = get_pci_alias_group();
> > > >     if (group)
> > > >       return group;
> > > > 
> > > >     return 0;
> > > >   }
> > > > 
> > > >   pci_for_each_dma_alias(pdev, callback, ...);  
> > > 
> > > And this would be equivalent to
> > > 
> > >  for_each_pci_dev(tmp) {
> > >    if (tmp is an alias of pdev)
> > >      group = get_pci_alias_group();
> > >    ...
> > >  }
> > > 
> > > The "is an alias of" property is not commutative. Perhaps it should be.
> > > But that's hard because in some cases the alias doesn't even *exist* as
> > > a real PCI device. It's just that you appear to get DMA transactions
> > > from a given source-id.  
> > 
> > Right.  In my example above, 01:00.0 is not a PCI device; it's only a
> > Requester ID that is fabricated by the bridge when it forwards DMA
> > transactions upstream.
> > 
> > I think I'm confused because I don't really understand IOMMU groups.
> > 
> > Let me explain what I think they are and you can correct me when I go
> > wrong.  The iommu_group_alloc() comment says "The IOMMU group
> > represents the minimum granularity of the IOMMU."  So I suppose the
> > IOMMU cannot distinguish between devices in a group.  All the devices
> > in the group use the same set of DMA mappings.  Granting device A DMA
> > access to a buffer grants the same access to all other members of A's
> > IOMMU group.
> > 
> > That would mean my question was fundamentally backwards.  In
> > get_pci_alias_group(A), we're not trying to figure out what all the
> > aliases of A are, which is what pci_for_each_dma_alias() does.
> > 
> > Instead, we're trying to figure out which IOMMU group A belongs to.
> > But I still don't quite understand how aliases fit into this.  Let's
> > go back to my example and assume we've already put 00:00.0 and 01:01.0
> > in IOMMU groups:
> > 
> >   00:00.0 PCIe-to-PCI bridge to [bus 01]     # in IOMMU group G0
> >   01:01.0 conventional PCI device            # in IOMMU group G1
> > 
> > I assume these devices are in different IOMMU groups because if the
> > bridge generated its own DMA, it would use Requester ID 00:00.0, which
> > is distinct from the 01:00.0 it would use when forwarding DMAs from
> > its secondary side.

The actual requester ID of the bridge depends on quirks as well, see
PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS, so it might either be the bridge itself
or the subordinate bus address.  However, trace how these groups would
be created, the bridge device is discovered first, so we call
pci_device_group() for it.  since it's on the root bus and it's the 0th
function, there will be no existing groups for it to alias, so it gets
a new group.  Then we discover 01:01.0, pci_device_group() calls
pci_for_each_dma_alias(), which hits the bridge, regardless of which
alias is used.  So 01:01.0 ends up in the same iommu group as the
bridge.

>From the purist standpoint of iommu groups, your expectations are
probably correct, but we also include within IOMMU groups DMA isolation
between devices.  So if two devices can DMA between each other without
it necessarily passing through the iommu, the devices are grouped
together.  The more important cases of this are ACS isolation on PCI-e
devices, but it still sort of applies to this conventional PCI example
here.
 
> > What happens when we add 01:02.0?  I think 01:01.0 and 01:02.0
> > should both end up in IOMMU group G1 because the IOMMU will see only
> > Requester ID 01:00.0, so it can't distinguish them.
> > 
> > When we add 01:02.0, the ops->add_device() ... ops->device_group()
> > path calls pci_device_group(01:02.0):
> > 
> >   pci_device_group(01:02.0)
> >     pci_for_each_dma_alias(01:02.0, get_pci_alias_or_group)
> >       get_pci_alias_or_group(01:02.0, 01:02.0)   # callback
> >         return 0           # 01:02.0 group not set yet
> >       get_pci_alias_or_group(00:00.0, 01:00.0)   # callback
> >         return 1           # 00:00.0 is in G0
> > 
> > It seems like we'll assign 01:02.0 to group G0, when I think it
> > should be in G1.  Where did I go wrong?  

In fact they're all part of G0, so you went wrong assuming the initial
grouping rather than applying the same rules and tracing how 01:01.0 is
added.  It's not an ideal representation, it tends to be more
conservative than necessary in some cases, but keeping the group
attached to devices has advantages in being able to search and find
points like this where we don't necessarily have access to the group
behind the bridge without tying it to the bridge itself.  Thanks,

Alex

Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Alex Williamson]

On Fri, 8 Apr 2016 11:06:32 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Tue, Mar 15, 2016 at 07:48:17PM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 14, 2016 at 10:43:40PM +0000, David Woodhouse wrote:  
> > > On Thu, 2016-02-25 at 08:38 -0600, Bjorn Helgaas wrote:  
> > > >   
> > > > >  /*
> > > > > - * Look for aliases to or from the given device for exisiting groups.  The
> > > > > - * dma_alias_devfn only supports aliases on the same bus, therefore the search
> > > > > + * Look for aliases to or from the given device for existing groups. DMA
> > > > > + * aliases are only supported on the same bus, therefore the search  
> > > > 
> > > > I'm trying to reconcile this statement that "DMA aliases are only
> > > > supported on the same bus" (which was there even before this patch)
> > > > with the fact that pci_for_each_dma_alias() does not have that
> > > > limitation.  
> > > 
> > > Doesn't it? You can still only set a DMA alias on the same bus with
> > > pci_add_dma_alias(), can't you?  
> > 
> > I guess it's true that PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and the proposed
> > pci_add_dma_alias() only add aliases on the same bus.  I was thinking
> > about a scenario like this:
> > 
> >   00:00.0 PCIe-to-PCI bridge to [bus 01]
> >   01:01.0 conventional PCI device
> > 
> > where I think 01:00.0 is a DMA alias for 01:01.0 because the bridge
> > takes ownership of DMA transactions from 01:01.0 and assigns a
> > Requester ID of 01:00.0 (secondary bus number, device 0, function 0).

This is true, but this is a topology alias, not a quirk.
pci_for_each_dma_alias() already handles this case.  It would trigger
the callback function for any direct alias of the conventional device
as well as the bridge itself.  Obviously we don't end up with any
quirks for conventional devices because the aliases are masked behind
the bridge anyway.
 
> > > > >   * space is quite small (especially since we're really only looking at pcie
> > > > >   * device, and therefore only expect multiple slots on the root complex or
> > > > >   * downstream switch ports).  It's conceivable though that a pair of
> > > > > @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
> > > > >                       continue;
> > > > >  
> > > > >               /* We alias them or they alias us */
> > > > > -             if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > > > -                  pdev->dma_alias_devfn == tmp->devfn) ||
> > > > > -                 ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > > > -                  tmp->dma_alias_devfn == pdev->devfn)) {
> > > > > -
> > > > > +             if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > > > > +                 dma_alias_is_enabled(tmp, pdev->devfn)) {
> > > > >                       group = get_pci_alias_group(tmp, devfns);  
> > > > 
> > > > We basically have this:
> > > > 
> > > >   for_each_pci_dev(tmp) {
> > > >     if ()
> > > >       group = get_pci_alias_group();
> > > >       ...
> > > >   }  
> > > 
> > > Strictly, that's:
> > > 
> > >  for_each_pci_dev(tmp) {
> > >    if (pdev is an alias of tmp || tmp is an alias of pdev)
> > >      group = get_pci_alias_group();
> > >    ...
> > >  }  
> > 
> > OK.  
> >   
> > > > I'm trying to figure out why we don't do something like the following
> > > > instead:
> > > > 
> > > >   callback(struct pci_dev *pdev, u16 alias, void *opaque)
> > > >   {
> > > >     struct iommu_group *group;
> > > > 
> > > >     group = get_pci_alias_group();
> > > >     if (group)
> > > >       return group;
> > > > 
> > > >     return 0;
> > > >   }
> > > > 
> > > >   pci_for_each_dma_alias(pdev, callback, ...);  
> > > 
> > > And this would be equivalent to
> > > 
> > >  for_each_pci_dev(tmp) {
> > >    if (tmp is an alias of pdev)
> > >      group = get_pci_alias_group();
> > >    ...
> > >  }
> > > 
> > > The "is an alias of" property is not commutative. Perhaps it should be.
> > > But that's hard because in some cases the alias doesn't even *exist* as
> > > a real PCI device. It's just that you appear to get DMA transactions
> > > from a given source-id.  
> > 
> > Right.  In my example above, 01:00.0 is not a PCI device; it's only a
> > Requester ID that is fabricated by the bridge when it forwards DMA
> > transactions upstream.
> > 
> > I think I'm confused because I don't really understand IOMMU groups.
> > 
> > Let me explain what I think they are and you can correct me when I go
> > wrong.  The iommu_group_alloc() comment says "The IOMMU group
> > represents the minimum granularity of the IOMMU."  So I suppose the
> > IOMMU cannot distinguish between devices in a group.  All the devices
> > in the group use the same set of DMA mappings.  Granting device A DMA
> > access to a buffer grants the same access to all other members of A's
> > IOMMU group.
> > 
> > That would mean my question was fundamentally backwards.  In
> > get_pci_alias_group(A), we're not trying to figure out what all the
> > aliases of A are, which is what pci_for_each_dma_alias() does.
> > 
> > Instead, we're trying to figure out which IOMMU group A belongs to.
> > But I still don't quite understand how aliases fit into this.  Let's
> > go back to my example and assume we've already put 00:00.0 and 01:01.0
> > in IOMMU groups:
> > 
> >   00:00.0 PCIe-to-PCI bridge to [bus 01]     # in IOMMU group G0
> >   01:01.0 conventional PCI device            # in IOMMU group G1
> > 
> > I assume these devices are in different IOMMU groups because if the
> > bridge generated its own DMA, it would use Requester ID 00:00.0, which
> > is distinct from the 01:00.0 it would use when forwarding DMAs from
> > its secondary side.

The actual requester ID of the bridge depends on quirks as well, see
PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS, so it might either be the bridge itself
or the subordinate bus address.  However, trace how these groups would
be created, the bridge device is discovered first, so we call
pci_device_group() for it.  since it's on the root bus and it's the 0th
function, there will be no existing groups for it to alias, so it gets
a new group.  Then we discover 01:01.0, pci_device_group() calls
pci_for_each_dma_alias(), which hits the bridge, regardless of which
alias is used.  So 01:01.0 ends up in the same iommu group as the
bridge.

From the purist standpoint of iommu groups, your expectations are
probably correct, but we also include within IOMMU groups DMA isolation
between devices.  So if two devices can DMA between each other without
it necessarily passing through the iommu, the devices are grouped
together.  The more important cases of this are ACS isolation on PCI-e
devices, but it still sort of applies to this conventional PCI example
here.
 
> > What happens when we add 01:02.0?  I think 01:01.0 and 01:02.0
> > should both end up in IOMMU group G1 because the IOMMU will see only
> > Requester ID 01:00.0, so it can't distinguish them.
> > 
> > When we add 01:02.0, the ops->add_device() ... ops->device_group()
> > path calls pci_device_group(01:02.0):
> > 
> >   pci_device_group(01:02.0)
> >     pci_for_each_dma_alias(01:02.0, get_pci_alias_or_group)
> >       get_pci_alias_or_group(01:02.0, 01:02.0)   # callback
> >         return 0           # 01:02.0 group not set yet
> >       get_pci_alias_or_group(00:00.0, 01:00.0)   # callback
> >         return 1           # 00:00.0 is in G0
> > 
> > It seems like we'll assign 01:02.0 to group G0, when I think it
> > should be in G1.  Where did I go wrong?  

In fact they're all part of G0, so you went wrong assuming the initial
grouping rather than applying the same rules and tracing how 01:01.0 is
added.  It's not an ideal representation, it tends to be more
conservative than necessary in some cases, but keeping the group
attached to devices has advantages in being able to search and find
points like this where we don't necessarily have access to the group
behind the bridge without tying it to the bridge itself.  Thanks,

Alex

Re: [PATCH v4 1/6] PCI: Add pci_add_dma_alias() to abstract implementation

[Alex Williamson]

On Wed, 24 Feb 2016 13:43:45 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> 
> Add a pci_add_dma_alias() interface to encapsulate the details of adding an
> alias.  No functional change intended.
> ---
>  drivers/pci/pci.c    |   14 ++++++++++++++
>  drivers/pci/pci.h    |    2 ++
>  drivers/pci/quirks.c |   19 +++++++------------
>  3 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 602eb42..7fccc8a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4568,6 +4568,20 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>  	return 0;
>  }
>  
> +/**
> + * pci_add_dma_alias - Add a DMA devfn alias for a device
> + * @dev: the PCI device for which alias is added
> + * @devfn: alias slot and function
> + *
> + * This helper encodes 8-bit devfn as bit number in dma_alias_mask.
> + * It should be called early, preferably as PCI fixup header quirk.
> + */
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> +{
> +	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);

This should be:

dev->dma_alias_devfn = devfn;

It was silently fixed in 3/6.  Also needs a Sign-off.  With those
changes:

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>


> +	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +}
> +
>  bool pci_device_is_present(struct pci_dev *pdev)
>  {
>  	u32 v;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9a1660f..c5dc8dc 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -335,4 +335,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  }
>  #endif
>  
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
> +
>  #endif /* DRIVERS_PCI_H */
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 0575a1e..df28dce 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3581,10 +3581,8 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  
>  static void quirk_dma_func0_alias(struct pci_dev *dev)
>  {
> -	if (PCI_FUNC(dev->devfn) != 0) {
> -		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> -	}
> +	if (PCI_FUNC(dev->devfn) != 0)
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
>  }
>  
>  /*
> @@ -3597,10 +3595,8 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476, quirk_dma_func0_alias);
>  
>  static void quirk_dma_func1_alias(struct pci_dev *dev)
>  {
> -	if (PCI_FUNC(dev->devfn) != 1) {
> -		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 1);
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> -	}
> +	if (PCI_FUNC(dev->devfn) != 1)
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
>  }
>  
>  /*
> @@ -3667,11 +3663,10 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
>  
>  	id = pci_match_id(fixed_dma_alias_tbl, dev);
>  	if (id) {
> -		dev->dma_alias_devfn = id->driver_data;
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +		pci_add_dma_alias(dev, id->driver_data);
>  		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> -			 PCI_SLOT(dev->dma_alias_devfn),
> -			 PCI_FUNC(dev->dma_alias_devfn));
> +			 PCI_SLOT(id->driver_data),
> +			 PCI_FUNC(id->driver_data));
>  	}
>  }
>  
> 

Re: [PATCH v4 1/6] PCI: Add pci_add_dma_alias() to abstract implementation

[Alex Williamson]

On Wed, 24 Feb 2016 13:43:45 -0600
Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:

> From: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Add a pci_add_dma_alias() interface to encapsulate the details of adding an
> alias.  No functional change intended.
> ---
>  drivers/pci/pci.c    |   14 ++++++++++++++
>  drivers/pci/pci.h    |    2 ++
>  drivers/pci/quirks.c |   19 +++++++------------
>  3 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 602eb42..7fccc8a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4568,6 +4568,20 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>  	return 0;
>  }
>  
> +/**
> + * pci_add_dma_alias - Add a DMA devfn alias for a device
> + * @dev: the PCI device for which alias is added
> + * @devfn: alias slot and function
> + *
> + * This helper encodes 8-bit devfn as bit number in dma_alias_mask.
> + * It should be called early, preferably as PCI fixup header quirk.
> + */
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> +{
> +	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);

This should be:

dev->dma_alias_devfn = devfn;

It was silently fixed in 3/6.  Also needs a Sign-off.  With those
changes:

Reviewed-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>


> +	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +}
> +
>  bool pci_device_is_present(struct pci_dev *pdev)
>  {
>  	u32 v;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9a1660f..c5dc8dc 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -335,4 +335,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  }
>  #endif
>  
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
> +
>  #endif /* DRIVERS_PCI_H */
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 0575a1e..df28dce 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3581,10 +3581,8 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  
>  static void quirk_dma_func0_alias(struct pci_dev *dev)
>  {
> -	if (PCI_FUNC(dev->devfn) != 0) {
> -		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> -	}
> +	if (PCI_FUNC(dev->devfn) != 0)
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
>  }
>  
>  /*
> @@ -3597,10 +3595,8 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476, quirk_dma_func0_alias);
>  
>  static void quirk_dma_func1_alias(struct pci_dev *dev)
>  {
> -	if (PCI_FUNC(dev->devfn) != 1) {
> -		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 1);
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> -	}
> +	if (PCI_FUNC(dev->devfn) != 1)
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
>  }
>  
>  /*
> @@ -3667,11 +3663,10 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
>  
>  	id = pci_match_id(fixed_dma_alias_tbl, dev);
>  	if (id) {
> -		dev->dma_alias_devfn = id->driver_data;
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +		pci_add_dma_alias(dev, id->driver_data);
>  		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> -			 PCI_SLOT(dev->dma_alias_devfn),
> -			 PCI_FUNC(dev->dma_alias_devfn));
> +			 PCI_SLOT(id->driver_data),
> +			 PCI_FUNC(id->driver_data));
>  	}
>  }
>  
> 

Re: [PATCH v4 2/6] PCI: Move informational printk to pci_add_dma_alias()

[Alex Williamson]

On Wed, 24 Feb 2016 13:43:54 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> 
> One of the quirks that adds DMA aliases logs an informational message in
> dmesg.  Move that to pci_add_dma_alias() so all users log the message
> consistently.  No functional change intended (except extra message).
> ---
>  drivers/pci/pci.c    |    2 ++
>  drivers/pci/quirks.c |    6 +-----
>  2 files changed, 3 insertions(+), 5 deletions(-)


Needs a Sign-off, otherwise:

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7fccc8a..8b0a637 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4580,6 +4580,8 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
>  	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
>  	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> +		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
>  
>  bool pci_device_is_present(struct pci_dev *pdev)
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index df28dce..cf023ea 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3662,12 +3662,8 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
>  	const struct pci_device_id *id;
>  
>  	id = pci_match_id(fixed_dma_alias_tbl, dev);
> -	if (id) {
> +	if (id)
>  		pci_add_dma_alias(dev, id->driver_data);
> -		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> -			 PCI_SLOT(id->driver_data),
> -			 PCI_FUNC(id->driver_data));
> -	}
>  }
>  
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285, quirk_fixed_dma_alias);
> 

Re: [PATCH v4 2/6] PCI: Move informational printk to pci_add_dma_alias()

[Alex Williamson]

On Wed, 24 Feb 2016 13:43:54 -0600
Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:

> From: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> One of the quirks that adds DMA aliases logs an informational message in
> dmesg.  Move that to pci_add_dma_alias() so all users log the message
> consistently.  No functional change intended (except extra message).
> ---
>  drivers/pci/pci.c    |    2 ++
>  drivers/pci/quirks.c |    6 +-----
>  2 files changed, 3 insertions(+), 5 deletions(-)


Needs a Sign-off, otherwise:

Reviewed-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7fccc8a..8b0a637 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4580,6 +4580,8 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
>  	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
>  	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> +		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
>  
>  bool pci_device_is_present(struct pci_dev *pdev)
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index df28dce..cf023ea 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3662,12 +3662,8 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
>  	const struct pci_device_id *id;
>  
>  	id = pci_match_id(fixed_dma_alias_tbl, dev);
> -	if (id) {
> +	if (id)
>  		pci_add_dma_alias(dev, id->driver_data);
> -		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> -			 PCI_SLOT(id->driver_data),
> -			 PCI_FUNC(id->driver_data));
> -	}
>  }
>  
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285, quirk_fixed_dma_alias);
> 

Re: [PATCH v5 3/6] PCI: Add support for multiple DMA aliases

[Alex Williamson]

On Thu,  3 Mar 2016 15:38:02 +0100
Jacek Lawrynowicz <jacek.lawrynowicz@intel.com> wrote:

> This patch solves IOMMU support issues with PCIe non-transparent bridges
> that use Requester ID look-up tables (RID-LUT), e.g. PEX8733.
> 
> The NTB connects devices in two independent PCI domains. Devices
> separated by the NTB are not able to discover each other. A PCI packet
> being forwared from one domain to another has to have its RID modified
> so it appears on correct bus and completions are forwarded back to the
> original domain through the NTB. RID is translated using preprogrammed
> table (LUT) and the PCI packet propagates upstream away from the NTB.
> If the destination system has IOMMU enabled, the packet will be
> discarded because the new RID is unknown to the IOMMU. Adding a DMA
> alias for the new RID allows IOMMU to properly recognize the packet.
> 
> Each device behind the NTB has a unique RID assigned in the RID-LUT.
> Current DMA alias implementation supports only single alias, so it's not
> possible to support mutiple devices behind the NTB when IOMMU is enabled.
> 
> This implementation enables all possible aliases on a given bus (256)
> that are stored in a bitset. Alias devfn is directly translated to a bit
> number. The bitset is not allocated for devices that have no need for
> DMA aliases.
> 
> More details can be found in following article:
> http://www.plxtech.com/files/pdf/technical/expresslane/RTC_Enabling%20MulitHostSystemDesigns.pdf
> 
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> Acked-by: David Woodhouse <David.Woodhouse@intel.com>
> Acked-by: Joerg Roedel <jroedel@suse.de>
> ---
>  I updated the commit message based on discussion with Bjorn. It should be
>  now a little easier to understand. I'm not resubmitting the whole patch set
>  because it could make the thread harder to follow.
>  
>  This time resubmitting with correct subject.
> 
>  drivers/iommu/iommu.c | 17 ++++++++++-------
>  drivers/pci/pci.c     | 11 +++++++++--
>  drivers/pci/probe.c   |  1 +
>  drivers/pci/search.c  | 14 +++++++++-----
>  include/linux/pci.h   |  4 +---
>  5 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index bfd4f7c..4c10da1 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -659,9 +659,15 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
>  	return NULL;
>  }
>  
> +static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
> +{
> +	return dev->dma_alias_mask &&
> +	       test_bit(devfn, dev->dma_alias_mask);
> +}
> +
>  /*
> - * Look for aliases to or from the given device for exisiting groups.  The
> - * dma_alias_devfn only supports aliases on the same bus, therefore the search
> + * Look for aliases to or from the given device for existing groups. DMA
> + * aliases are only supported on the same bus, therefore the search
>   * space is quite small (especially since we're really only looking at pcie
>   * device, and therefore only expect multiple slots on the root complex or
>   * downstream switch ports).  It's conceivable though that a pair of
> @@ -686,11 +692,8 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
>  			continue;
>  
>  		/* We alias them or they alias us */
> -		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> -		     pdev->dma_alias_devfn == tmp->devfn) ||
> -		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> -		     tmp->dma_alias_devfn == pdev->devfn)) {
> -
> +		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> +		    dma_alias_is_enabled(tmp, pdev->devfn)) {
>  			group = get_pci_alias_group(tmp, devfns);
>  			if (group) {
>  				pci_dev_put(tmp);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e5e2c9d..33f3d24 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4577,8 +4577,15 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>   */
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
> -	dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
> -	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +	if (!dev->dma_alias_mask)
> +		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> +					      sizeof(long), GFP_KERNEL);
> +	if (!dev->dma_alias_mask) {
> +		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
> +		return;
> +	}
> +
> +	set_bit(devfn, dev->dma_alias_mask);

This silently fixes the bug in 1/, it should be updated after 1/ is
fixed.  Otherwise,

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>


>  	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
>  		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 5eb378f..cf09307 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1502,6 +1502,7 @@ static void pci_release_dev(struct device *dev)
>  	pcibios_release_device(pci_dev);
>  	pci_bus_put(pci_dev->bus);
>  	kfree(pci_dev->driver_override);
> +	kfree(pci_dev->dma_alias_mask);
>  	kfree(pci_dev);
>  }
>  
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index a20ce7d..33e0f03 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -40,11 +40,15 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
>  	 * If the device is broken and uses an alias requester ID for
>  	 * DMA, iterate over that too.
>  	 */
> -	if (unlikely(pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)) {
> -		ret = fn(pdev, PCI_DEVID(pdev->bus->number,
> -					 pdev->dma_alias_devfn), data);
> -		if (ret)
> -			return ret;
> +	if (unlikely(pdev->dma_alias_mask)) {
> +		u8 devfn;
> +
> +		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
> +			ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
> +				 data);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	for (bus = pdev->bus; !pci_is_root_bus(bus); bus = bus->parent) {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 614d70d..0c176e5 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -172,8 +172,6 @@ enum pci_dev_flags {
>  	PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) (1 << 2),
>  	/* Flag for quirk use to store if quirk-specific ACS is enabled */
>  	PCI_DEV_FLAGS_ACS_ENABLED_QUIRK = (__force pci_dev_flags_t) (1 << 3),
> -	/* Flag to indicate the device uses dma_alias_devfn */
> -	PCI_DEV_FLAGS_DMA_ALIAS_DEVFN = (__force pci_dev_flags_t) (1 << 4),
>  	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
>  	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 5),
>  	/* Do not use bus resets for device */
> @@ -279,7 +277,7 @@ struct pci_dev {
>  	u8		rom_base_reg;	/* which config register controls the ROM */
>  	u8		pin;		/* which interrupt pin this device uses */
>  	u16		pcie_flags_reg;	/* cached PCIe Capabilities Register */
> -	u8		dma_alias_devfn;/* devfn of DMA alias, if any */
> +	unsigned long	*dma_alias_mask;/* mask of enabled devfn aliases */
>  
>  	struct pci_driver *driver;	/* which driver has allocated this device */
>  	u64		dma_mask;	/* Mask of the bits of bus address this

Re: [PATCH v4 4/6] PCI: Rename dma_alias_is_enabled() to pci_devs_are_dma_aliases()

[Alex Williamson]

On Wed, 24 Feb 2016 13:44:15 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> This should be folded into the previous patch.  I left it separate to show
> the interface difference more clearly.
> 
> Also, pci_devs_are_dma_aliases() uses PCI internals (dma_alias_mask), so I
> think it should be in PCI code instead of in IOMMU code.  That would mean
> both it and pci_add_dma_alias() should probably be declared in
> include/linux/pci.h (but not exported to modules with EXPORT_SYMBOL()).
> ---
>  drivers/iommu/iommu.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Needs a Sign-off.

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index a214e19..031d06b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -659,10 +659,12 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
>  	return NULL;
>  }
>  
> -static bool dma_alias_is_enabled(struct pci_dev *dev, u8 devfn)
> +static bool pci_devs_are_dma_aliases(struct pci_dev *pdev1, struct pci_dev *pdev2)
>  {
> -	return dev->dma_alias_mask &&
> -	       test_bit(devfn, dev->dma_alias_mask);
> +	return (pdev1->dma_alias_mask &&
> +		test_bit(pdev2->devfn, pdev1->dma_alias_mask)) ||
> +	       (pdev2->dma_alias_mask &&
> +		test_bit(pdev1->devfn, pdev2->dma_alias_mask));
>  }
>  
>  /*
> @@ -692,8 +694,7 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
>  			continue;
>  
>  		/* We alias them or they alias us */
> -		if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> -		    dma_alias_is_enabled(tmp, pdev->devfn)) {
> +		if (pci_devs_are_dma_aliases(pdev, tmp)) {
>  			group = get_pci_alias_group(tmp, devfns);
>  			if (group) {
>  				pci_dev_put(tmp);
> 

Re: [PATCH v5 5/6] PCI: Add DMA alias quirk for mic_x200_dma

[Alex Williamson]

On Thu,  3 Mar 2016 15:53:20 +0100
Jacek Lawrynowicz <jacek.lawrynowicz@intel.com> wrote:

> MIC x200 NTB forwards PCIe traffic using multiple alien RID. They have to
> be added as aliases to the DMA device in order to allow buffer access
> when IOMMU is enabled.
> 
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> Acked-by: David Woodhouse <David.Woodhouse@intel.com>
> ---

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>


>  Updated quirk comment with requirement that aliases have to be matching
>  values programmed in the EEPROM. The LUT table on x200 is static so there
>  is no need to program it from the driver.
> 
>  drivers/pci/quirks.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 51927a5..e66f09d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3696,6 +3696,20 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
>  
>  /*
> + * MIC x200 NTB forwards PCIe traffic using multiple alien RIDs. They have to
> + * be added as aliases to the DMA device in order to allow buffer access
> + * when IOMMU is enabled. Following devfns have to match RIT-LUT table
> + * programmed in the EEPROM.
> + */
> +static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
> +{
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
> +
> +/*
>   * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
>   * class code.  Fix it.
>   */

Re: [PATCH v5 5/6] PCI: Add DMA alias quirk for mic_x200_dma

[Alex Williamson]

On Thu,  3 Mar 2016 15:53:20 +0100
Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> MIC x200 NTB forwards PCIe traffic using multiple alien RID. They have to
> be added as aliases to the DMA device in order to allow buffer access
> when IOMMU is enabled.
> 
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Acked-by: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---

Reviewed-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>


>  Updated quirk comment with requirement that aliases have to be matching
>  values programmed in the EEPROM. The LUT table on x200 is static so there
>  is no need to program it from the driver.
> 
>  drivers/pci/quirks.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 51927a5..e66f09d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3696,6 +3696,20 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
>  
>  /*
> + * MIC x200 NTB forwards PCIe traffic using multiple alien RIDs. They have to
> + * be added as aliases to the DMA device in order to allow buffer access
> + * when IOMMU is enabled. Following devfns have to match RIT-LUT table
> + * programmed in the EEPROM.
> + */
> +static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
> +{
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
> +
> +/*
>   * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
>   * class code.  Fix it.
>   */

Re: [PATCH v4 6/6] PCI: Squash pci_dev_flags to remove holes

[Alex Williamson]

On Wed, 24 Feb 2016 13:44:31 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> 
> After removing PCI_DEV_FLAGS_DMA_ALIAS_DEVFN, the (1 << 4) value was
> unused.  Squash the other values so all the bits are adjacent.  No
> functional change intended.
> 
> (I'm not sure this is worth doing.  We have 16 flag bits and we're not
> even close to exhausting them.  But if we do squash them, it should be
> in a separate patch so we don't clutter up the main patches.)
> ---

Needs a Sign-off

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>


>  include/linux/pci.h |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d9e0c84..4e36024 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -173,13 +173,13 @@ enum pci_dev_flags {
>  	/* Flag for quirk use to store if quirk-specific ACS is enabled */
>  	PCI_DEV_FLAGS_ACS_ENABLED_QUIRK = (__force pci_dev_flags_t) (1 << 3),
>  	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
> -	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 5),
> +	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 4),
>  	/* Do not use bus resets for device */
> -	PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
> +	PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 5),
>  	/* Do not use PM reset even if device advertises NoSoftRst- */
> -	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> +	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 6),
>  	/* Get VPD from function 0 VPD */
> -	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
> +	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 7),
>  };
>  
>  enum pci_irq_reroute_variant {
> 

Re: [PATCH v4 0/6] PCI: Support multiple DMA aliases

[Bjorn Helgaas]

On Wed, Feb 24, 2016 at 01:43:32PM -0600, Bjorn Helgaas wrote:
> This is a revision of Jacek's v3 posting:
> http://lkml.kernel.org/r/1454152012-46337-1-git-send-email-jacek.lawrynowicz@intel.com
> 
> The changes from v3 are:
> 
>   - Split into smaller patches for reviewability
>   - Move printk when adding DMA alias
>   - Change dma_alias_is_enabled() interface to take two pci_devs
>   - Rename dma_alias_is_enabled() to indicate PCI context
> 
> The only remaining thing I want to sort out is the dma_alias_is_enabled()
> vs pci_for_each_dma_alias() question Alex raised.  I'll respond to the
> relevant part of the patch in this series with my specific questions.
> 
> ---
> 
> Bjorn Helgaas (1):
>       PCI: Rename dma_alias_is_enabled() to pci_devs_are_dma_aliases()
> 
> Jacek Lawrynowicz (5):
>       PCI: Add pci_add_dma_alias() to abstract implementation
>       PCI: Move informational printk to pci_add_dma_alias()
>       PCI: Add support for multiple DMA aliases
>       pci: Add DMA alias quirk for mic_x200_dma
>       PCI: Squash pci_dev_flags to remove holes

I applied this series to pci/ntb for v4.7.  It got a little confusing
with v5 versions of a couple patches posted, and a couple issues Alex
found, so here's the series as applied:

commit f0af9593372abfde34460aa1250e670cc535a7d8
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Wed Feb 24 13:43:45 2016 -0600

    PCI: Add pci_add_dma_alias() to abstract implementation
    
    Add a pci_add_dma_alias() interface to encapsulate the details of adding an
    alias.  No functional change intended.
    
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 25e0327..1162118 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4578,6 +4578,20 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 	return 0;
 }
 
+/**
+ * pci_add_dma_alias - Add a DMA devfn alias for a device
+ * @dev: the PCI device for which alias is added
+ * @devfn: alias slot and function
+ *
+ * This helper encodes 8-bit devfn as bit number in dma_alias_mask.
+ * It should be called early, preferably as PCI fixup header quirk.
+ */
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
+{
+	dev->dma_alias_devfn = devfn;
+	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+}
+
 bool pci_device_is_present(struct pci_dev *pdev)
 {
 	u32 v;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 8e67802..e45a7a8 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3610,10 +3610,8 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 
 static void quirk_dma_func0_alias(struct pci_dev *dev)
 {
-	if (PCI_FUNC(dev->devfn) != 0) {
-		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
-		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
-	}
+	if (PCI_FUNC(dev->devfn) != 0)
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
 }
 
 /*
@@ -3626,10 +3624,8 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476, quirk_dma_func0_alias);
 
 static void quirk_dma_func1_alias(struct pci_dev *dev)
 {
-	if (PCI_FUNC(dev->devfn) != 1) {
-		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 1);
-		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
-	}
+	if (PCI_FUNC(dev->devfn) != 1)
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
 }
 
 /*
@@ -3696,11 +3692,10 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
 
 	id = pci_match_id(fixed_dma_alias_tbl, dev);
 	if (id) {
-		dev->dma_alias_devfn = id->driver_data;
-		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+		pci_add_dma_alias(dev, id->driver_data);
 		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
-			 PCI_SLOT(dev->dma_alias_devfn),
-			 PCI_FUNC(dev->dma_alias_devfn));
+			 PCI_SLOT(id->driver_data),
+			 PCI_FUNC(id->driver_data));
 	}
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 004b813..7e70190 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1988,6 +1988,7 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
 }
 #endif
 
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
 int pci_for_each_dma_alias(struct pci_dev *pdev,
 			   int (*fn)(struct pci_dev *pdev,
 				     u16 alias, void *data), void *data);

commit 48c830809ce6e143781172c03a9794cb66802b31
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Wed Feb 24 13:43:54 2016 -0600

    PCI: Move informational printk to pci_add_dma_alias()
    
    One of the quirks that adds DMA aliases logs an informational message in
    dmesg.  Move that to pci_add_dma_alias() so all users log the message
    consistently.  No functional change intended (except extra message).
    
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1162118..c82ebd0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4590,6 +4590,8 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
 	dev->dma_alias_devfn = devfn;
 	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
+		 PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
 
 bool pci_device_is_present(struct pci_dev *pdev)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index e45a7a8..7559e40 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3691,12 +3691,8 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
 	const struct pci_device_id *id;
 
 	id = pci_match_id(fixed_dma_alias_tbl, dev);
-	if (id) {
+	if (id)
 		pci_add_dma_alias(dev, id->driver_data);
-		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
-			 PCI_SLOT(id->driver_data),
-			 PCI_FUNC(id->driver_data));
-	}
 }
 
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285, quirk_fixed_dma_alias);

commit 338c3149a221527e202ee26b1e35f76c965bb6c0
Author: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
Date:   Thu Mar 3 15:38:02 2016 +0100

    PCI: Add support for multiple DMA aliases
    
    Solve IOMMU support issues with PCIe non-transparent bridges that use
    Requester ID look-up tables (RID-LUT), e.g., the PEX8733.
    
    The NTB connects devices in two independent PCI domains.  Devices separated
    by the NTB are not able to discover each other.  A PCI packet being
    forwared from one domain to another has to have its RID modified so it
    appears on correct bus and completions are forwarded back to the original
    domain through the NTB.  The RID is translated using a preprogrammed table
    (LUT) and the PCI packet propagates upstream away from the NTB.  If the
    destination system has IOMMU enabled, the packet will be discarded because
    the new RID is unknown to the IOMMU.  Adding a DMA alias for the new RID
    allows IOMMU to properly recognize the packet.
    
    Each device behind the NTB has a unique RID assigned in the RID-LUT.  The
    current DMA alias implementation supports only a single alias, so it's not
    possible to support mutiple devices behind the NTB when IOMMU is enabled.
    
    Enable all possible aliases on a given bus (256) that are stored in a
    bitset.  Alias devfn is directly translated to a bit number.  The bitset is
    not allocated for devices that have no need for DMA aliases.
    
    More details can be found in the following article:
    http://www.plxtech.com/files/pdf/technical/expresslane/RTC_Enabling%20MulitHostSystemDesigns.pdf
    
    Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
    Acked-by: David Woodhouse <David.Woodhouse@intel.com>
    Acked-by: Joerg Roedel <jroedel@suse.de>

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index bfd4f7c..1b49e94 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -660,8 +660,8 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
 }
 
 /*
- * Look for aliases to or from the given device for exisiting groups.  The
- * dma_alias_devfn only supports aliases on the same bus, therefore the search
+ * Look for aliases to or from the given device for existing groups. DMA
+ * aliases are only supported on the same bus, therefore the search
  * space is quite small (especially since we're really only looking at pcie
  * device, and therefore only expect multiple slots on the root complex or
  * downstream switch ports).  It's conceivable though that a pair of
@@ -686,11 +686,7 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
 			continue;
 
 		/* We alias them or they alias us */
-		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
-		     pdev->dma_alias_devfn == tmp->devfn) ||
-		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
-		     tmp->dma_alias_devfn == pdev->devfn)) {
-
+		if (pci_devs_are_dma_aliases(pdev, tmp)) {
 			group = get_pci_alias_group(tmp, devfns);
 			if (group) {
 				pci_dev_put(tmp);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c82ebd0..0b90c21 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4588,12 +4588,27 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  */
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
-	dev->dma_alias_devfn = devfn;
-	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
+	if (!dev->dma_alias_mask)
+		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
+					      sizeof(long), GFP_KERNEL);
+	if (!dev->dma_alias_mask) {
+		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
+		return;
+	}
+
+	set_bit(devfn, dev->dma_alias_mask);
 	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
 		 PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
 
+bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
+{
+	return (dev1->dma_alias_mask &&
+		test_bit(dev2->devfn, dev1->dma_alias_mask)) ||
+	       (dev2->dma_alias_mask &&
+		test_bit(dev1->devfn, dev2->dma_alias_mask));
+}
+
 bool pci_device_is_present(struct pci_dev *pdev)
 {
 	u32 v;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8004f67..ae7daeb 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1537,6 +1537,7 @@ static void pci_release_dev(struct device *dev)
 	pcibios_release_device(pci_dev);
 	pci_bus_put(pci_dev->bus);
 	kfree(pci_dev->driver_override);
+	kfree(pci_dev->dma_alias_mask);
 	kfree(pci_dev);
 }
 
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index a20ce7d..33e0f03 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -40,11 +40,15 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
 	 * If the device is broken and uses an alias requester ID for
 	 * DMA, iterate over that too.
 	 */
-	if (unlikely(pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)) {
-		ret = fn(pdev, PCI_DEVID(pdev->bus->number,
-					 pdev->dma_alias_devfn), data);
-		if (ret)
-			return ret;
+	if (unlikely(pdev->dma_alias_mask)) {
+		u8 devfn;
+
+		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
+			ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
+				 data);
+			if (ret)
+				return ret;
+		}
 	}
 
 	for (bus = pdev->bus; !pci_is_root_bus(bus); bus = bus->parent) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7e70190..5581d05 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -166,8 +166,6 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) (1 << 2),
 	/* Flag for quirk use to store if quirk-specific ACS is enabled */
 	PCI_DEV_FLAGS_ACS_ENABLED_QUIRK = (__force pci_dev_flags_t) (1 << 3),
-	/* Flag to indicate the device uses dma_alias_devfn */
-	PCI_DEV_FLAGS_DMA_ALIAS_DEVFN = (__force pci_dev_flags_t) (1 << 4),
 	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
 	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 5),
 	/* Do not use bus resets for device */
@@ -273,7 +271,7 @@ struct pci_dev {
 	u8		rom_base_reg;	/* which config register controls the ROM */
 	u8		pin;		/* which interrupt pin this device uses */
 	u16		pcie_flags_reg;	/* cached PCIe Capabilities Register */
-	u8		dma_alias_devfn;/* devfn of DMA alias, if any */
+	unsigned long	*dma_alias_mask;/* mask of enabled devfn aliases */
 
 	struct pci_driver *driver;	/* which driver has allocated this device */
 	u64		dma_mask;	/* Mask of the bits of bus address this
@@ -1989,6 +1987,7 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
 #endif
 
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
+bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
 int pci_for_each_dma_alias(struct pci_dev *pdev,
 			   int (*fn)(struct pci_dev *pdev,
 				     u16 alias, void *data), void *data);

commit 8e6b62b5dc00bee0bd1f6fada928969159b8083a
Author: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
Date:   Thu Mar 3 15:53:20 2016 +0100

    PCI: Add DMA alias quirk for mic_x200_dma
    
    The MIC x200 NTB forwards DMA transactions upstream using multiple alien
    RIDs.  These RIDs have to be added as aliases to the DMA device to allow
    buffer access when the IOMMU is enabled.
    
    Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
    Acked-by: David Woodhouse <David.Woodhouse@intel.com>

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 7559e40..5cccc78 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3725,6 +3725,20 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
 
 /*
+ * MIC x200 NTB forwards PCIe traffic using multiple alien RIDs. They have to
+ * be added as aliases to the DMA device in order to allow buffer access
+ * when IOMMU is enabled. Following devfns have to match RIT-LUT table
+ * programmed in the EEPROM.
+ */
+static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
+{
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */

RE: [PATCH v4 0/6] PCI: Support multiple DMA aliases

[Lawrynowicz, Jacek]

[-- Attachment #1.1: Type: text/plain, Size: 15872 bytes --]

Great, thanks! I tested pci/ntb tree and it works but I had to add another PCI
ID.
Would it be a problem to add second PCI ID to the x200 quirk in this patch set?
I attached a patch with additional line.

Regards,
Jacek

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Tuesday, April 12, 2016 6:38 AM
> To: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Lawrynowicz, Jacek <jacek.lawrynowicz@intel.com>; linux-
> pci@vger.kernel.org; Alex Williamson <alex.williamson@redhat.com>; Joerg
> Roedel <jroedel@suse.de>; David Woodhouse <dwmw2@infradead.org>;
> iommu@lists.linux-foundation.org
> Subject: Re: [PATCH v4 0/6] PCI: Support multiple DMA aliases
> 
> On Wed, Feb 24, 2016 at 01:43:32PM -0600, Bjorn Helgaas wrote:
> > This is a revision of Jacek's v3 posting:
> > http://lkml.kernel.org/r/1454152012-46337-1-git-send-email-
> jacek.lawrynowicz@intel.com
> >
> > The changes from v3 are:
> >
> >   - Split into smaller patches for reviewability
> >   - Move printk when adding DMA alias
> >   - Change dma_alias_is_enabled() interface to take two pci_devs
> >   - Rename dma_alias_is_enabled() to indicate PCI context
> >
> > The only remaining thing I want to sort out is the dma_alias_is_enabled()
> > vs pci_for_each_dma_alias() question Alex raised.  I'll respond to the
> > relevant part of the patch in this series with my specific questions.
> >
> > ---
> >
> > Bjorn Helgaas (1):
> >       PCI: Rename dma_alias_is_enabled() to pci_devs_are_dma_aliases()
> >
> > Jacek Lawrynowicz (5):
> >       PCI: Add pci_add_dma_alias() to abstract implementation
> >       PCI: Move informational printk to pci_add_dma_alias()
> >       PCI: Add support for multiple DMA aliases
> >       pci: Add DMA alias quirk for mic_x200_dma
> >       PCI: Squash pci_dev_flags to remove holes
> 
> I applied this series to pci/ntb for v4.7.  It got a little confusing
> with v5 versions of a couple patches posted, and a couple issues Alex
> found, so here's the series as applied:
> 
> commit f0af9593372abfde34460aa1250e670cc535a7d8
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Wed Feb 24 13:43:45 2016 -0600
> 
>     PCI: Add pci_add_dma_alias() to abstract implementation
> 
>     Add a pci_add_dma_alias() interface to encapsulate the details of adding
an
>     alias.  No functional change intended.
> 
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 25e0327..1162118 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4578,6 +4578,20 @@ int pci_set_vga_state(struct pci_dev *dev, bool
> decode,
>  	return 0;
>  }
> 
> +/**
> + * pci_add_dma_alias - Add a DMA devfn alias for a device
> + * @dev: the PCI device for which alias is added
> + * @devfn: alias slot and function
> + *
> + * This helper encodes 8-bit devfn as bit number in dma_alias_mask.
> + * It should be called early, preferably as PCI fixup header quirk.
> + */
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> +{
> +	dev->dma_alias_devfn = devfn;
> +	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +}
> +
>  bool pci_device_is_present(struct pci_dev *pdev)
>  {
>  	u32 v;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 8e67802..e45a7a8 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3610,10 +3610,8 @@ int pci_dev_specific_reset(struct pci_dev *dev, int
> probe)
> 
>  static void quirk_dma_func0_alias(struct pci_dev *dev)
>  {
> -	if (PCI_FUNC(dev->devfn) != 0) {
> -		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> -	}
> +	if (PCI_FUNC(dev->devfn) != 0)
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
>  }
> 
>  /*
> @@ -3626,10 +3624,8 @@
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476,
> quirk_dma_func0_alias);
> 
>  static void quirk_dma_func1_alias(struct pci_dev *dev)
>  {
> -	if (PCI_FUNC(dev->devfn) != 1) {
> -		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 1);
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> -	}
> +	if (PCI_FUNC(dev->devfn) != 1)
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
>  }
> 
>  /*
> @@ -3696,11 +3692,10 @@ static void quirk_fixed_dma_alias(struct pci_dev
> *dev)
> 
>  	id = pci_match_id(fixed_dma_alias_tbl, dev);
>  	if (id) {
> -		dev->dma_alias_devfn = id->driver_data;
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +		pci_add_dma_alias(dev, id->driver_data);
>  		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> -			 PCI_SLOT(dev->dma_alias_devfn),
> -			 PCI_FUNC(dev->dma_alias_devfn));
> +			 PCI_SLOT(id->driver_data),
> +			 PCI_FUNC(id->driver_data));
>  	}
>  }
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 004b813..7e70190 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1988,6 +1988,7 @@ static inline struct eeh_dev
> *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>  }
>  #endif
> 
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
>  int pci_for_each_dma_alias(struct pci_dev *pdev,
>  			   int (*fn)(struct pci_dev *pdev,
>  				     u16 alias, void *data), void *data);
> 
> commit 48c830809ce6e143781172c03a9794cb66802b31
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Wed Feb 24 13:43:54 2016 -0600
> 
>     PCI: Move informational printk to pci_add_dma_alias()
> 
>     One of the quirks that adds DMA aliases logs an informational message in
>     dmesg.  Move that to pci_add_dma_alias() so all users log the message
>     consistently.  No functional change intended (except extra message).
> 
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1162118..c82ebd0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4590,6 +4590,8 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
>  	dev->dma_alias_devfn = devfn;
>  	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> +		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
> 
>  bool pci_device_is_present(struct pci_dev *pdev)
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index e45a7a8..7559e40 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3691,12 +3691,8 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
>  	const struct pci_device_id *id;
> 
>  	id = pci_match_id(fixed_dma_alias_tbl, dev);
> -	if (id) {
> +	if (id)
>  		pci_add_dma_alias(dev, id->driver_data);
> -		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> -			 PCI_SLOT(id->driver_data),
> -			 PCI_FUNC(id->driver_data));
> -	}
>  }
> 
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285,
> quirk_fixed_dma_alias);
> 
> commit 338c3149a221527e202ee26b1e35f76c965bb6c0
> Author: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> Date:   Thu Mar 3 15:38:02 2016 +0100
> 
>     PCI: Add support for multiple DMA aliases
> 
>     Solve IOMMU support issues with PCIe non-transparent bridges that use
>     Requester ID look-up tables (RID-LUT), e.g., the PEX8733.
> 
>     The NTB connects devices in two independent PCI domains.  Devices
separated
>     by the NTB are not able to discover each other.  A PCI packet being
>     forwared from one domain to another has to have its RID modified so it
>     appears on correct bus and completions are forwarded back to the original
>     domain through the NTB.  The RID is translated using a preprogrammed table
>     (LUT) and the PCI packet propagates upstream away from the NTB.  If the
>     destination system has IOMMU enabled, the packet will be discarded because
>     the new RID is unknown to the IOMMU.  Adding a DMA alias for the new RID
>     allows IOMMU to properly recognize the packet.
> 
>     Each device behind the NTB has a unique RID assigned in the RID-LUT.  The
>     current DMA alias implementation supports only a single alias, so it's not
>     possible to support mutiple devices behind the NTB when IOMMU is enabled.
> 
>     Enable all possible aliases on a given bus (256) that are stored in a
>     bitset.  Alias devfn is directly translated to a bit number.  The bitset
is
>     not allocated for devices that have no need for DMA aliases.
> 
>     More details can be found in the following article:
> 
> http://www.plxtech.com/files/pdf/technical/expresslane/RTC_Enabling%20Muli
> tHostSystemDesigns.pdf
> 
>     Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
>     Acked-by: David Woodhouse <David.Woodhouse@intel.com>
>     Acked-by: Joerg Roedel <jroedel@suse.de>
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index bfd4f7c..1b49e94 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -660,8 +660,8 @@ static struct iommu_group
> *get_pci_function_alias_group(struct pci_dev *pdev,
>  }
> 
>  /*
> - * Look for aliases to or from the given device for exisiting groups.  The
> - * dma_alias_devfn only supports aliases on the same bus, therefore the
search
> + * Look for aliases to or from the given device for existing groups. DMA
> + * aliases are only supported on the same bus, therefore the search
>   * space is quite small (especially since we're really only looking at pcie
>   * device, and therefore only expect multiple slots on the root complex or
>   * downstream switch ports).  It's conceivable though that a pair of
> @@ -686,11 +686,7 @@ static struct iommu_group *get_pci_alias_group(struct
> pci_dev *pdev,
>  			continue;
> 
>  		/* We alias them or they alias us */
> -		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)
> &&
> -		     pdev->dma_alias_devfn == tmp->devfn) ||
> -		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> -		     tmp->dma_alias_devfn == pdev->devfn)) {
> -
> +		if (pci_devs_are_dma_aliases(pdev, tmp)) {
>  			group = get_pci_alias_group(tmp, devfns);
>  			if (group) {
>  				pci_dev_put(tmp);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c82ebd0..0b90c21 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4588,12 +4588,27 @@ int pci_set_vga_state(struct pci_dev *dev, bool
> decode,
>   */
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
> -	dev->dma_alias_devfn = devfn;
> -	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +	if (!dev->dma_alias_mask)
> +		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> +					      sizeof(long), GFP_KERNEL);
> +	if (!dev->dma_alias_mask) {
> +		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
> +		return;
> +	}
> +
> +	set_bit(devfn, dev->dma_alias_mask);
>  	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
>  		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
> 
> +bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
> +{
> +	return (dev1->dma_alias_mask &&
> +		test_bit(dev2->devfn, dev1->dma_alias_mask)) ||
> +	       (dev2->dma_alias_mask &&
> +		test_bit(dev1->devfn, dev2->dma_alias_mask));
> +}
> +
>  bool pci_device_is_present(struct pci_dev *pdev)
>  {
>  	u32 v;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8004f67..ae7daeb 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1537,6 +1537,7 @@ static void pci_release_dev(struct device *dev)
>  	pcibios_release_device(pci_dev);
>  	pci_bus_put(pci_dev->bus);
>  	kfree(pci_dev->driver_override);
> +	kfree(pci_dev->dma_alias_mask);
>  	kfree(pci_dev);
>  }
> 
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index a20ce7d..33e0f03 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -40,11 +40,15 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
>  	 * If the device is broken and uses an alias requester ID for
>  	 * DMA, iterate over that too.
>  	 */
> -	if (unlikely(pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)) {
> -		ret = fn(pdev, PCI_DEVID(pdev->bus->number,
> -					 pdev->dma_alias_devfn), data);
> -		if (ret)
> -			return ret;
> +	if (unlikely(pdev->dma_alias_mask)) {
> +		u8 devfn;
> +
> +		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
> +			ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
> +				 data);
> +			if (ret)
> +				return ret;
> +		}
>  	}
> 
>  	for (bus = pdev->bus; !pci_is_root_bus(bus); bus = bus->parent) {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7e70190..5581d05 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -166,8 +166,6 @@ enum pci_dev_flags {
>  	PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) (1 << 2),
>  	/* Flag for quirk use to store if quirk-specific ACS is enabled */
>  	PCI_DEV_FLAGS_ACS_ENABLED_QUIRK = (__force pci_dev_flags_t) (1
> << 3),
> -	/* Flag to indicate the device uses dma_alias_devfn */
> -	PCI_DEV_FLAGS_DMA_ALIAS_DEVFN = (__force pci_dev_flags_t) (1 <<
> 4),
>  	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
>  	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 <<
> 5),
>  	/* Do not use bus resets for device */
> @@ -273,7 +271,7 @@ struct pci_dev {
>  	u8		rom_base_reg;	/* which config register controls
> the ROM */
>  	u8		pin;		/* which interrupt pin this device uses
*/
>  	u16		pcie_flags_reg;	/* cached PCIe Capabilities
> Register */
> -	u8		dma_alias_devfn;/* devfn of DMA alias, if any */
> +	unsigned long	*dma_alias_mask;/* mask of enabled devfn aliases */
> 
>  	struct pci_driver *driver;	/* which driver has allocated this
device */
>  	u64		dma_mask;	/* Mask of the bits of bus address this
> @@ -1989,6 +1987,7 @@ static inline struct eeh_dev
> *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>  #endif
> 
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
> +bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
>  int pci_for_each_dma_alias(struct pci_dev *pdev,
>  			   int (*fn)(struct pci_dev *pdev,
>  				     u16 alias, void *data), void *data);
> 
> commit 8e6b62b5dc00bee0bd1f6fada928969159b8083a
> Author: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
> Date:   Thu Mar 3 15:53:20 2016 +0100
> 
>     PCI: Add DMA alias quirk for mic_x200_dma
> 
>     The MIC x200 NTB forwards DMA transactions upstream using multiple alien
>     RIDs.  These RIDs have to be added as aliases to the DMA device to allow
>     buffer access when the IOMMU is enabled.
> 
>     Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
>     Acked-by: David Woodhouse <David.Woodhouse@intel.com>
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 7559e40..5cccc78 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3725,6 +3725,20 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892,
> quirk_use_pcie_bridge_dma_alias);
>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e,
> quirk_use_pcie_bridge_dma_alias);
> 
>  /*
> + * MIC x200 NTB forwards PCIe traffic using multiple alien RIDs. They have to
> + * be added as aliases to the DMA device in order to allow buffer access
> + * when IOMMU is enabled. Following devfns have to match RIT-LUT table
> + * programmed in the EEPROM.
> + */
> +static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
> +{
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264,
> quirk_mic_x200_dma_alias);
> +
> +/*
>   * Intersil/Techwell TW686[4589]-based video capture cards have an empty
> (zero)
>   * class code.  Fix it.
>   */

[-- Attachment #1.2: 0001-PCI-Add-DMA-alias-quirk-for-mic_x200_dma.patch --]
[-- Type: application/octet-stream, Size: 1853 bytes --]

>From 2c4c6ae7e915290f8b432f0fca9c77b03bc63609 Mon Sep 17 00:00:00 2001
From: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
Date: Thu, 3 Mar 2016 15:53:20 +0100
Subject: [PATCH] PCI: Add DMA alias quirk for mic_x200_dma

The MIC x200 NTB forwards DMA transactions upstream using multiple alien
RIDs.  These RIDs have to be added as aliases to the DMA device to allow
buffer access when the IOMMU is enabled.

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Acked-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 7559e40..8889ac4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3725,6 +3725,21 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
 
 /*
+ * MIC x200 NTB forwards PCIe traffic using multiple alien RIDs. They have to
+ * be added as aliases to the DMA device in order to allow buffer access
+ * when IOMMU is enabled. Following devfns have to match RIT-LUT table
+ * programmed in the EEPROM.
+ */
+static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
+{
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2260, quirk_mic_x200_dma_alias);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */
-- 
1.8.3.1


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7756 bytes --]

RE: [PATCH v4 0/6] PCI: Support multiple DMA aliases

[Lawrynowicz, Jacek]

[-- Attachment #1.1.1: Type: text/plain, Size: 16615 bytes --]

Great, thanks! I tested pci/ntb tree and it works but I had to add another PCI
ID.
Would it be a problem to add second PCI ID to the x200 quirk in this patch set?
I attached a patch with additional line.

Regards,
Jacek

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Tuesday, April 12, 2016 6:38 AM
> To: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Lawrynowicz, Jacek <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; linux-
> pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; Joerg
> Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>; David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Subject: Re: [PATCH v4 0/6] PCI: Support multiple DMA aliases
> 
> On Wed, Feb 24, 2016 at 01:43:32PM -0600, Bjorn Helgaas wrote:
> > This is a revision of Jacek's v3 posting:
> > http://lkml.kernel.org/r/1454152012-46337-1-git-send-email-
> jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> >
> > The changes from v3 are:
> >
> >   - Split into smaller patches for reviewability
> >   - Move printk when adding DMA alias
> >   - Change dma_alias_is_enabled() interface to take two pci_devs
> >   - Rename dma_alias_is_enabled() to indicate PCI context
> >
> > The only remaining thing I want to sort out is the dma_alias_is_enabled()
> > vs pci_for_each_dma_alias() question Alex raised.  I'll respond to the
> > relevant part of the patch in this series with my specific questions.
> >
> > ---
> >
> > Bjorn Helgaas (1):
> >       PCI: Rename dma_alias_is_enabled() to pci_devs_are_dma_aliases()
> >
> > Jacek Lawrynowicz (5):
> >       PCI: Add pci_add_dma_alias() to abstract implementation
> >       PCI: Move informational printk to pci_add_dma_alias()
> >       PCI: Add support for multiple DMA aliases
> >       pci: Add DMA alias quirk for mic_x200_dma
> >       PCI: Squash pci_dev_flags to remove holes
> 
> I applied this series to pci/ntb for v4.7.  It got a little confusing
> with v5 versions of a couple patches posted, and a couple issues Alex
> found, so here's the series as applied:
> 
> commit f0af9593372abfde34460aa1250e670cc535a7d8
> Author: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Date:   Wed Feb 24 13:43:45 2016 -0600
> 
>     PCI: Add pci_add_dma_alias() to abstract implementation
> 
>     Add a pci_add_dma_alias() interface to encapsulate the details of adding
an
>     alias.  No functional change intended.
> 
>     Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>     Reviewed-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 25e0327..1162118 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4578,6 +4578,20 @@ int pci_set_vga_state(struct pci_dev *dev, bool
> decode,
>  	return 0;
>  }
> 
> +/**
> + * pci_add_dma_alias - Add a DMA devfn alias for a device
> + * @dev: the PCI device for which alias is added
> + * @devfn: alias slot and function
> + *
> + * This helper encodes 8-bit devfn as bit number in dma_alias_mask.
> + * It should be called early, preferably as PCI fixup header quirk.
> + */
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> +{
> +	dev->dma_alias_devfn = devfn;
> +	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +}
> +
>  bool pci_device_is_present(struct pci_dev *pdev)
>  {
>  	u32 v;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 8e67802..e45a7a8 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3610,10 +3610,8 @@ int pci_dev_specific_reset(struct pci_dev *dev, int
> probe)
> 
>  static void quirk_dma_func0_alias(struct pci_dev *dev)
>  {
> -	if (PCI_FUNC(dev->devfn) != 0) {
> -		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> -	}
> +	if (PCI_FUNC(dev->devfn) != 0)
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
>  }
> 
>  /*
> @@ -3626,10 +3624,8 @@
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe476,
> quirk_dma_func0_alias);
> 
>  static void quirk_dma_func1_alias(struct pci_dev *dev)
>  {
> -	if (PCI_FUNC(dev->devfn) != 1) {
> -		dev->dma_alias_devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 1);
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> -	}
> +	if (PCI_FUNC(dev->devfn) != 1)
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
>  }
> 
>  /*
> @@ -3696,11 +3692,10 @@ static void quirk_fixed_dma_alias(struct pci_dev
> *dev)
> 
>  	id = pci_match_id(fixed_dma_alias_tbl, dev);
>  	if (id) {
> -		dev->dma_alias_devfn = id->driver_data;
> -		dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +		pci_add_dma_alias(dev, id->driver_data);
>  		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> -			 PCI_SLOT(dev->dma_alias_devfn),
> -			 PCI_FUNC(dev->dma_alias_devfn));
> +			 PCI_SLOT(id->driver_data),
> +			 PCI_FUNC(id->driver_data));
>  	}
>  }
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 004b813..7e70190 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1988,6 +1988,7 @@ static inline struct eeh_dev
> *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>  }
>  #endif
> 
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
>  int pci_for_each_dma_alias(struct pci_dev *pdev,
>  			   int (*fn)(struct pci_dev *pdev,
>  				     u16 alias, void *data), void *data);
> 
> commit 48c830809ce6e143781172c03a9794cb66802b31
> Author: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Date:   Wed Feb 24 13:43:54 2016 -0600
> 
>     PCI: Move informational printk to pci_add_dma_alias()
> 
>     One of the quirks that adds DMA aliases logs an informational message in
>     dmesg.  Move that to pci_add_dma_alias() so all users log the message
>     consistently.  No functional change intended (except extra message).
> 
>     Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>     Reviewed-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1162118..c82ebd0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4590,6 +4590,8 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
>  	dev->dma_alias_devfn = devfn;
>  	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> +		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
> 
>  bool pci_device_is_present(struct pci_dev *pdev)
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index e45a7a8..7559e40 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3691,12 +3691,8 @@ static void quirk_fixed_dma_alias(struct pci_dev *dev)
>  	const struct pci_device_id *id;
> 
>  	id = pci_match_id(fixed_dma_alias_tbl, dev);
> -	if (id) {
> +	if (id)
>  		pci_add_dma_alias(dev, id->driver_data);
> -		dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
> -			 PCI_SLOT(id->driver_data),
> -			 PCI_FUNC(id->driver_data));
> -	}
>  }
> 
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ADAPTEC2, 0x0285,
> quirk_fixed_dma_alias);
> 
> commit 338c3149a221527e202ee26b1e35f76c965bb6c0
> Author: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date:   Thu Mar 3 15:38:02 2016 +0100
> 
>     PCI: Add support for multiple DMA aliases
> 
>     Solve IOMMU support issues with PCIe non-transparent bridges that use
>     Requester ID look-up tables (RID-LUT), e.g., the PEX8733.
> 
>     The NTB connects devices in two independent PCI domains.  Devices
separated
>     by the NTB are not able to discover each other.  A PCI packet being
>     forwared from one domain to another has to have its RID modified so it
>     appears on correct bus and completions are forwarded back to the original
>     domain through the NTB.  The RID is translated using a preprogrammed table
>     (LUT) and the PCI packet propagates upstream away from the NTB.  If the
>     destination system has IOMMU enabled, the packet will be discarded because
>     the new RID is unknown to the IOMMU.  Adding a DMA alias for the new RID
>     allows IOMMU to properly recognize the packet.
> 
>     Each device behind the NTB has a unique RID assigned in the RID-LUT.  The
>     current DMA alias implementation supports only a single alias, so it's not
>     possible to support mutiple devices behind the NTB when IOMMU is enabled.
> 
>     Enable all possible aliases on a given bus (256) that are stored in a
>     bitset.  Alias devfn is directly translated to a bit number.  The bitset
is
>     not allocated for devices that have no need for DMA aliases.
> 
>     More details can be found in the following article:
> 
> http://www.plxtech.com/files/pdf/technical/expresslane/RTC_Enabling%20Muli
> tHostSystemDesigns.pdf
> 
>     Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>     Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>     Reviewed-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     Acked-by: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>     Acked-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index bfd4f7c..1b49e94 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -660,8 +660,8 @@ static struct iommu_group
> *get_pci_function_alias_group(struct pci_dev *pdev,
>  }
> 
>  /*
> - * Look for aliases to or from the given device for exisiting groups.  The
> - * dma_alias_devfn only supports aliases on the same bus, therefore the
search
> + * Look for aliases to or from the given device for existing groups. DMA
> + * aliases are only supported on the same bus, therefore the search
>   * space is quite small (especially since we're really only looking at pcie
>   * device, and therefore only expect multiple slots on the root complex or
>   * downstream switch ports).  It's conceivable though that a pair of
> @@ -686,11 +686,7 @@ static struct iommu_group *get_pci_alias_group(struct
> pci_dev *pdev,
>  			continue;
> 
>  		/* We alias them or they alias us */
> -		if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)
> &&
> -		     pdev->dma_alias_devfn == tmp->devfn) ||
> -		    ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> -		     tmp->dma_alias_devfn == pdev->devfn)) {
> -
> +		if (pci_devs_are_dma_aliases(pdev, tmp)) {
>  			group = get_pci_alias_group(tmp, devfns);
>  			if (group) {
>  				pci_dev_put(tmp);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c82ebd0..0b90c21 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4588,12 +4588,27 @@ int pci_set_vga_state(struct pci_dev *dev, bool
> decode,
>   */
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
> -	dev->dma_alias_devfn = devfn;
> -	dev->dev_flags |= PCI_DEV_FLAGS_DMA_ALIAS_DEVFN;
> +	if (!dev->dma_alias_mask)
> +		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> +					      sizeof(long), GFP_KERNEL);
> +	if (!dev->dma_alias_mask) {
> +		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
> +		return;
> +	}
> +
> +	set_bit(devfn, dev->dma_alias_mask);
>  	dev_info(&dev->dev, "Enabling fixed DMA alias to %02x.%d\n",
>  		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
> 
> +bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
> +{
> +	return (dev1->dma_alias_mask &&
> +		test_bit(dev2->devfn, dev1->dma_alias_mask)) ||
> +	       (dev2->dma_alias_mask &&
> +		test_bit(dev1->devfn, dev2->dma_alias_mask));
> +}
> +
>  bool pci_device_is_present(struct pci_dev *pdev)
>  {
>  	u32 v;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8004f67..ae7daeb 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1537,6 +1537,7 @@ static void pci_release_dev(struct device *dev)
>  	pcibios_release_device(pci_dev);
>  	pci_bus_put(pci_dev->bus);
>  	kfree(pci_dev->driver_override);
> +	kfree(pci_dev->dma_alias_mask);
>  	kfree(pci_dev);
>  }
> 
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index a20ce7d..33e0f03 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -40,11 +40,15 @@ int pci_for_each_dma_alias(struct pci_dev *pdev,
>  	 * If the device is broken and uses an alias requester ID for
>  	 * DMA, iterate over that too.
>  	 */
> -	if (unlikely(pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN)) {
> -		ret = fn(pdev, PCI_DEVID(pdev->bus->number,
> -					 pdev->dma_alias_devfn), data);
> -		if (ret)
> -			return ret;
> +	if (unlikely(pdev->dma_alias_mask)) {
> +		u8 devfn;
> +
> +		for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) {
> +			ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
> +				 data);
> +			if (ret)
> +				return ret;
> +		}
>  	}
> 
>  	for (bus = pdev->bus; !pci_is_root_bus(bus); bus = bus->parent) {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7e70190..5581d05 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -166,8 +166,6 @@ enum pci_dev_flags {
>  	PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) (1 << 2),
>  	/* Flag for quirk use to store if quirk-specific ACS is enabled */
>  	PCI_DEV_FLAGS_ACS_ENABLED_QUIRK = (__force pci_dev_flags_t) (1
> << 3),
> -	/* Flag to indicate the device uses dma_alias_devfn */
> -	PCI_DEV_FLAGS_DMA_ALIAS_DEVFN = (__force pci_dev_flags_t) (1 <<
> 4),
>  	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
>  	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 <<
> 5),
>  	/* Do not use bus resets for device */
> @@ -273,7 +271,7 @@ struct pci_dev {
>  	u8		rom_base_reg;	/* which config register controls
> the ROM */
>  	u8		pin;		/* which interrupt pin this device uses
*/
>  	u16		pcie_flags_reg;	/* cached PCIe Capabilities
> Register */
> -	u8		dma_alias_devfn;/* devfn of DMA alias, if any */
> +	unsigned long	*dma_alias_mask;/* mask of enabled devfn aliases */
> 
>  	struct pci_driver *driver;	/* which driver has allocated this
device */
>  	u64		dma_mask;	/* Mask of the bits of bus address this
> @@ -1989,6 +1987,7 @@ static inline struct eeh_dev
> *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>  #endif
> 
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
> +bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
>  int pci_for_each_dma_alias(struct pci_dev *pdev,
>  			   int (*fn)(struct pci_dev *pdev,
>  				     u16 alias, void *data), void *data);
> 
> commit 8e6b62b5dc00bee0bd1f6fada928969159b8083a
> Author: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date:   Thu Mar 3 15:53:20 2016 +0100
> 
>     PCI: Add DMA alias quirk for mic_x200_dma
> 
>     The MIC x200 NTB forwards DMA transactions upstream using multiple alien
>     RIDs.  These RIDs have to be added as aliases to the DMA device to allow
>     buffer access when the IOMMU is enabled.
> 
>     Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>     Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>     Reviewed-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     Acked-by: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 7559e40..5cccc78 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3725,6 +3725,20 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892,
> quirk_use_pcie_bridge_dma_alias);
>  DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e,
> quirk_use_pcie_bridge_dma_alias);
> 
>  /*
> + * MIC x200 NTB forwards PCIe traffic using multiple alien RIDs. They have to
> + * be added as aliases to the DMA device in order to allow buffer access
> + * when IOMMU is enabled. Following devfns have to match RIT-LUT table
> + * programmed in the EEPROM.
> + */
> +static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
> +{
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
> +	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264,
> quirk_mic_x200_dma_alias);
> +
> +/*
>   * Intersil/Techwell TW686[4589]-based video capture cards have an empty
> (zero)
>   * class code.  Fix it.
>   */

[-- Attachment #1.1.2: 0001-PCI-Add-DMA-alias-quirk-for-mic_x200_dma.patch --]
[-- Type: application/octet-stream, Size: 2001 bytes --]

>From 2c4c6ae7e915290f8b432f0fca9c77b03bc63609 Mon Sep 17 00:00:00 2001
From: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Thu, 3 Mar 2016 15:53:20 +0100
Subject: [PATCH] PCI: Add DMA alias quirk for mic_x200_dma

The MIC x200 NTB forwards DMA transactions upstream using multiple alien
RIDs.  These RIDs have to be added as aliases to the DMA device to allow
buffer access when the IOMMU is enabled.

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 7559e40..8889ac4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3725,6 +3725,21 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
 
 /*
+ * MIC x200 NTB forwards PCIe traffic using multiple alien RIDs. They have to
+ * be added as aliases to the DMA device in order to allow buffer access
+ * when IOMMU is enabled. Following devfns have to match RIT-LUT table
+ * programmed in the EEPROM.
+ */
+static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
+{
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2260, quirk_mic_x200_dma_alias);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */
-- 
1.8.3.1


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7756 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v4 0/6] PCI: Support multiple DMA aliases

[Bjorn Helgaas]

On Mon, Apr 11, 2016 at 11:38:28PM -0500, Bjorn Helgaas wrote:
> On Wed, Feb 24, 2016 at 01:43:32PM -0600, Bjorn Helgaas wrote:
> > This is a revision of Jacek's v3 posting:
> > http://lkml.kernel.org/r/1454152012-46337-1-git-send-email-jacek.lawrynowicz@intel.com
> > 
> > The changes from v3 are:
> > 
> >   - Split into smaller patches for reviewability
> >   - Move printk when adding DMA alias
> >   - Change dma_alias_is_enabled() interface to take two pci_devs
> >   - Rename dma_alias_is_enabled() to indicate PCI context
> > 
> > The only remaining thing I want to sort out is the dma_alias_is_enabled()
> > vs pci_for_each_dma_alias() question Alex raised.  I'll respond to the
> > relevant part of the patch in this series with my specific questions.
> > 
> > ---
> > 
> > Bjorn Helgaas (1):
> >       PCI: Rename dma_alias_is_enabled() to pci_devs_are_dma_aliases()
> > 
> > Jacek Lawrynowicz (5):
> >       PCI: Add pci_add_dma_alias() to abstract implementation
> >       PCI: Move informational printk to pci_add_dma_alias()
> >       PCI: Add support for multiple DMA aliases
> >       pci: Add DMA alias quirk for mic_x200_dma
> >       PCI: Squash pci_dev_flags to remove holes
> 
> I applied this series to pci/ntb for v4.7.

Jacek sent another patch to add a second PCI ID.  I don't see it on the
list, probably because it's got too much fancy encoding.  I applied it, so
the last patch now looks like this:

commit b1a928cdb477037fb7c10fbf94c47f65f2bcce77
Author: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
Date:   Thu Mar 3 15:53:20 2016 +0100

    PCI: Add DMA alias quirk for mic_x200_dma
    
    The MIC x200 NTB forwards DMA transactions upstream using multiple alien
    RIDs.  These RIDs have to be added as aliases to the DMA device to allow
    buffer access when the IOMMU is enabled.
    
    Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
    Acked-by: David Woodhouse <David.Woodhouse@intel.com>

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 7559e40..8889ac4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3725,6 +3725,21 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892, quirk_use_pcie_bridge_dma_alias);
 DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
 
 /*
+ * MIC x200 NTB forwards PCIe traffic using multiple alien RIDs. They have to
+ * be added as aliases to the DMA device in order to allow buffer access
+ * when IOMMU is enabled. Following devfns have to match RIT-LUT table
+ * programmed in the EEPROM.
+ */
+static void quirk_mic_x200_dma_alias(struct pci_dev *pdev)
+{
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x10, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x11, 0x0));
+	pci_add_dma_alias(pdev, PCI_DEVFN(0x12, 0x3));
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2260, quirk_mic_x200_dma_alias);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2264, quirk_mic_x200_dma_alias);
+
+/*
  * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
  * class code.  Fix it.
  */