[PATCH] pci: Add support for multiple DMA aliases

[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 (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.

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.

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>
---
 drivers/iommu/iommu.c |  8 ++++----
 drivers/pci/pci.c     | 19 +++++++++++++++++++
 drivers/pci/probe.c   |  1 +
 drivers/pci/quirks.c  | 15 ++++++---------
 drivers/pci/search.c  | 14 +++++++++-----
 include/linux/pci.h   | 15 ++++++++-------
 6 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index abae363..98ae7ff 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -686,10 +686,10 @@ 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 ((pdev->dma_alias_mask &&
+		     test_bit(tmp->devfn, pdev->dma_alias_mask)) ||
+		    ((tmp->dma_alias_mask &&
+		     test_bit(pdev->devfn, tmp->dma_alias_mask)))) {
 
 			group = get_pci_alias_group(tmp, devfns);
 			if (group) {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 49e3715..5b27d65 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4568,6 +4568,25 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 	return 0;
 }
 
+/**
+ * pci_add_dma_alias - Allows to add multiple devfn aliases for given 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
+ */
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
+{
+	if (!dev->dma_alias_mask) {
+		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
+					      sizeof(long), GFP_KERNEL);
+		dev_warn(&dev->dev, "Unable to allocate DMA alias mask.\n");
+	}
+	if (dev->dma_alias_mask)
+		set_bit(devfn, dev->dma_alias_mask);
+}
+EXPORT_SYMBOL_GPL(pci_add_dma_alias);
+
 bool pci_device_is_present(struct pci_dev *pdev)
 {
 	u32 v;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 32b9f1b..5da4dd3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1501,6 +1501,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/quirks.c b/drivers/pci/quirks.c
index 83e93d7..3ed1f9a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3579,8 +3579,7 @@ 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;
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
 	}
 }
 
@@ -3595,8 +3594,7 @@ 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;
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
 	}
 }
 
@@ -3660,11 +3658,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;
-		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_add_dma_alias(dev, id->driver_data);
+		dev_info(&dev->dev, "Enabling fixed DMA alias to %02lx.%ld\n",
+			 PCI_SLOT(id->driver_data),
+			 PCI_FUNC(id->driver_data));
 	}
 }
 
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 f9f79ad..6200175 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -172,16 +172,14 @@ 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),
+	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 {
@@ -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
@@ -1229,6 +1227,9 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
 
 int pci_set_vga_state(struct pci_dev *pdev, bool decode,
 		      unsigned int command_bits, u32 flags);
+
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
+
 /* kmem_cache style wrapper around pci_alloc_consistent() */
 
 #include <linux/pci-dma.h>
-- 
2.1.4

[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 (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.

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.

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>
---
 drivers/iommu/iommu.c |  8 ++++----
 drivers/pci/pci.c     | 19 +++++++++++++++++++
 drivers/pci/probe.c   |  1 +
 drivers/pci/quirks.c  | 15 ++++++---------
 drivers/pci/search.c  | 14 +++++++++-----
 include/linux/pci.h   | 15 ++++++++-------
 6 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index abae363..98ae7ff 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -686,10 +686,10 @@ 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 ((pdev->dma_alias_mask &&
+		     test_bit(tmp->devfn, pdev->dma_alias_mask)) ||
+		    ((tmp->dma_alias_mask &&
+		     test_bit(pdev->devfn, tmp->dma_alias_mask)))) {
 
 			group = get_pci_alias_group(tmp, devfns);
 			if (group) {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 49e3715..5b27d65 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4568,6 +4568,25 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 	return 0;
 }
 
+/**
+ * pci_add_dma_alias - Allows to add multiple devfn aliases for given 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
+ */
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
+{
+	if (!dev->dma_alias_mask) {
+		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
+					      sizeof(long), GFP_KERNEL);
+		dev_warn(&dev->dev, "Unable to allocate DMA alias mask.\n");
+	}
+	if (dev->dma_alias_mask)
+		set_bit(devfn, dev->dma_alias_mask);
+}
+EXPORT_SYMBOL_GPL(pci_add_dma_alias);
+
 bool pci_device_is_present(struct pci_dev *pdev)
 {
 	u32 v;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 32b9f1b..5da4dd3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1501,6 +1501,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/quirks.c b/drivers/pci/quirks.c
index 83e93d7..3ed1f9a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3579,8 +3579,7 @@ 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;
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
 	}
 }
 
@@ -3595,8 +3594,7 @@ 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;
+		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
 	}
 }
 
@@ -3660,11 +3658,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;
-		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_add_dma_alias(dev, id->driver_data);
+		dev_info(&dev->dev, "Enabling fixed DMA alias to %02lx.%ld\n",
+			 PCI_SLOT(id->driver_data),
+			 PCI_FUNC(id->driver_data));
 	}
 }
 
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 f9f79ad..6200175 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -172,16 +172,14 @@ 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),
+	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 {
@@ -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
@@ -1229,6 +1227,9 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
 
 int pci_set_vga_state(struct pci_dev *pdev, bool decode,
 		      unsigned int command_bits, u32 flags);
+
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
+
 /* kmem_cache style wrapper around pci_alloc_consistent() */
 
 #include <linux/pci-dma.h>
-- 
2.1.4

Re: [PATCH] pci: Add support for multiple DMA aliases

[Bjorn Helgaas]

On Mon, Jan 18, 2016 at 05:07:47PM +0100, Jacek Lawrynowicz wrote:
> 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.
> 
> 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.
> 
> 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 applied this to pci/iommu and intend to merge it for v4.5.

I made the following change because the kcalloc() failure warning in
your patch looks wrong:

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b0d6a0a..29cfe1a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4577,13 +4577,15 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
  */
 void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 {
-	if (!dev->dma_alias_mask) {
+	if (!dev->dma_alias_mask)
 		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
 					      sizeof(long), GFP_KERNEL);
-		dev_warn(&dev->dev, "Unable to allocate DMA alias mask.\n");
+	if (!dev->dma_alias_mask) {
+		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
+		return;
 	}
-	if (dev->dma_alias_mask)
-		set_bit(devfn, dev->dma_alias_mask);
+
+	set_bit(devfn, dev->dma_alias_mask);
 }
 EXPORT_SYMBOL_GPL(pci_add_dma_alias);
 

> ---
>  drivers/iommu/iommu.c |  8 ++++----
>  drivers/pci/pci.c     | 19 +++++++++++++++++++
>  drivers/pci/probe.c   |  1 +
>  drivers/pci/quirks.c  | 15 ++++++---------
>  drivers/pci/search.c  | 14 +++++++++-----
>  include/linux/pci.h   | 15 ++++++++-------
>  6 files changed, 47 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index abae363..98ae7ff 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -686,10 +686,10 @@ 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 ((pdev->dma_alias_mask &&
> +		     test_bit(tmp->devfn, pdev->dma_alias_mask)) ||
> +		    ((tmp->dma_alias_mask &&
> +		     test_bit(pdev->devfn, tmp->dma_alias_mask)))) {
>  
>  			group = get_pci_alias_group(tmp, devfns);
>  			if (group) {
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 49e3715..5b27d65 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4568,6 +4568,25 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>  	return 0;
>  }
>  
> +/**
> + * pci_add_dma_alias - Allows to add multiple devfn aliases for given 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
> + */
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> +{
> +	if (!dev->dma_alias_mask) {
> +		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> +					      sizeof(long), GFP_KERNEL);
> +		dev_warn(&dev->dev, "Unable to allocate DMA alias mask.\n");
> +	}
> +	if (dev->dma_alias_mask)
> +		set_bit(devfn, dev->dma_alias_mask);
> +}
> +EXPORT_SYMBOL_GPL(pci_add_dma_alias);
> +
>  bool pci_device_is_present(struct pci_dev *pdev)
>  {
>  	u32 v;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 32b9f1b..5da4dd3 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1501,6 +1501,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/quirks.c b/drivers/pci/quirks.c
> index 83e93d7..3ed1f9a 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3579,8 +3579,7 @@ 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;
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
>  	}
>  }
>  
> @@ -3595,8 +3594,7 @@ 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;
> +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
>  	}
>  }
>  
> @@ -3660,11 +3658,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;
> -		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_add_dma_alias(dev, id->driver_data);
> +		dev_info(&dev->dev, "Enabling fixed DMA alias to %02lx.%ld\n",
> +			 PCI_SLOT(id->driver_data),
> +			 PCI_FUNC(id->driver_data));
>  	}
>  }
>  
> 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 f9f79ad..6200175 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -172,16 +172,14 @@ 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),
> +	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 {
> @@ -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
> @@ -1229,6 +1227,9 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
>  
>  int pci_set_vga_state(struct pci_dev *pdev, bool decode,
>  		      unsigned int command_bits, u32 flags);
> +
> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
> +
>  /* kmem_cache style wrapper around pci_alloc_consistent() */
>  
>  #include <linux/pci-dma.h>
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] pci: Add support for multiple DMA aliases

[Lawrynowicz, Jacek]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Tuesday, January 19, 2016 4:33 AM
> To: Lawrynowicz, Jacek <jacek.lawrynowicz@intel.com>
> Cc: linux-pci@vger.kernel.org; bhelgaas@google.com;
> dwmw2@infradead.org; jroedel@suse.de
> Subject: Re: [PATCH] pci: Add support for multiple DMA aliases
> 
> On Mon, Jan 18, 2016 at 05:07:47PM +0100, Jacek Lawrynowicz wrote:
> > 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.
> >
> > 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.
> >
> > More details can be found in following article:
> >
> http://www.plxtech.com/files/pdf/technical/expresslane/RTC_Enabling%20
> > MulitHostSystemDesigns.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 applied this to pci/iommu and intend to merge it for v4.5.
> 
> I made the following change because the kcalloc() failure warning in your
> patch looks wrong:
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b0d6a0a..29cfe1a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4577,13 +4577,15 @@ int pci_set_vga_state(struct pci_dev *dev, bool
> decode,
>   */
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)  {
> -	if (!dev->dma_alias_mask) {
> +	if (!dev->dma_alias_mask)
>  		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
>  					      sizeof(long), GFP_KERNEL);
> -		dev_warn(&dev->dev, "Unable to allocate DMA alias
> mask.\n");
> +	if (!dev->dma_alias_mask) {
> +		dev_warn(&dev->dev, "Unable to allocate DMA alias
> mask\n");
> +		return;
>  	}
> -	if (dev->dma_alias_mask)
> -		set_bit(devfn, dev->dma_alias_mask);
> +
> +	set_bit(devfn, dev->dma_alias_mask);
>  }
>  EXPORT_SYMBOL_GPL(pci_add_dma_alias);

Yes, you're right. Thanks.

--
Jacek Lawrynowicz
Intel Technology Poland sp. z o.o.
KRS 101882 - ul. Slowackiego 173, 80-298 Gdansk

Re: [PATCH] pci: Add support for multiple DMA aliases

[Bjorn Helgaas]

[+cc Alex]

On Mon, Jan 18, 2016 at 09:33:15PM -0600, Bjorn Helgaas wrote:
> On Mon, Jan 18, 2016 at 05:07:47PM +0100, Jacek Lawrynowicz wrote:
> > 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.
> > 
> > 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.
> > 
> > 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 applied this to pci/iommu and intend to merge it for v4.5.
> 
> I made the following change because the kcalloc() failure warning in
> your patch looks wrong:
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b0d6a0a..29cfe1a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4577,13 +4577,15 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>   */
>  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>  {
> -	if (!dev->dma_alias_mask) {
> +	if (!dev->dma_alias_mask)
>  		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
>  					      sizeof(long), GFP_KERNEL);
> -		dev_warn(&dev->dev, "Unable to allocate DMA alias mask.\n");
> +	if (!dev->dma_alias_mask) {
> +		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
> +		return;
>  	}
> -	if (dev->dma_alias_mask)
> -		set_bit(devfn, dev->dma_alias_mask);
> +
> +	set_bit(devfn, dev->dma_alias_mask);
>  }
>  EXPORT_SYMBOL_GPL(pci_add_dma_alias);
>  
> 
> > ---
> >  drivers/iommu/iommu.c |  8 ++++----
> >  drivers/pci/pci.c     | 19 +++++++++++++++++++
> >  drivers/pci/probe.c   |  1 +
> >  drivers/pci/quirks.c  | 15 ++++++---------
> >  drivers/pci/search.c  | 14 +++++++++-----
> >  include/linux/pci.h   | 15 ++++++++-------
> >  6 files changed, 47 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index abae363..98ae7ff 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -686,10 +686,10 @@ 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 ((pdev->dma_alias_mask &&
> > +		     test_bit(tmp->devfn, pdev->dma_alias_mask)) ||
> > +		    ((tmp->dma_alias_mask &&
> > +		     test_bit(pdev->devfn, tmp->dma_alias_mask)))) {
> >  
> >  			group = get_pci_alias_group(tmp, devfns);
> >  			if (group) {
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 49e3715..5b27d65 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -4568,6 +4568,25 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
> >  	return 0;
> >  }
> >  
> > +/**
> > + * pci_add_dma_alias - Allows to add multiple devfn aliases for given 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
> > + */
> > +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> > +{
> > +	if (!dev->dma_alias_mask) {
> > +		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> > +					      sizeof(long), GFP_KERNEL);
> > +		dev_warn(&dev->dev, "Unable to allocate DMA alias mask.\n");
> > +	}
> > +	if (dev->dma_alias_mask)
> > +		set_bit(devfn, dev->dma_alias_mask);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_add_dma_alias);
> > +
> >  bool pci_device_is_present(struct pci_dev *pdev)
> >  {
> >  	u32 v;
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 32b9f1b..5da4dd3 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1501,6 +1501,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/quirks.c b/drivers/pci/quirks.c
> > index 83e93d7..3ed1f9a 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3579,8 +3579,7 @@ 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;
> > +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> >  	}
> >  }
> >  
> > @@ -3595,8 +3594,7 @@ 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;
> > +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
> >  	}
> >  }
> >  
> > @@ -3660,11 +3658,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;
> > -		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_add_dma_alias(dev, id->driver_data);
> > +		dev_info(&dev->dev, "Enabling fixed DMA alias to %02lx.%ld\n",
> > +			 PCI_SLOT(id->driver_data),
> > +			 PCI_FUNC(id->driver_data));
> >  	}
> >  }
> >  
> > 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 f9f79ad..6200175 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -172,16 +172,14 @@ 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),
> > +	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 {
> > @@ -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
> > @@ -1229,6 +1227,9 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
> >  
> >  int pci_set_vga_state(struct pci_dev *pdev, bool decode,
> >  		      unsigned int command_bits, u32 flags);
> > +
> > +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
> > +
> >  /* kmem_cache style wrapper around pci_alloc_consistent() */
> >  
> >  #include <linux/pci-dma.h>
> > -- 
> > 2.1.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pci: Add support for multiple DMA aliases

[Alex Williamson]

On Tue, 2016-01-19 at 14:12 -0600, Bjorn Helgaas wrote:
> [+cc Alex]
> 
> On Mon, Jan 18, 2016 at 09:33:15PM -0600, Bjorn Helgaas wrote:
> > On Mon, Jan 18, 2016 at 05:07:47PM +0100, Jacek Lawrynowicz wrote:
> > > 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.
> > > 
> > > 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.

My only concern here is that pci_add_dma_alias() makes aliases seem
more dynamic than they really are.  For instance, when we add a device
to an IOMMU domain, we evaluate the aliases at that point, if an NTB
later adds a new lookup entry and specifies a new alias, it's still not
going to work.  Similarly, IOMMU groups are evaluated as the device is
added, so if an alias is to a physical device and we need the cross
reference to bind them together into a single group, calling
pci_add_dma_alias() from a driver isn't going to work.

The existing code had this problem too, it's just more obvious now that
we have a helper function and that the helper is exported for use
outside of the PCI core.  Thanks,

Alex

> > > 
> > > 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 applied this to pci/iommu and intend to merge it for v4.5.
> > 
> > I made the following change because the kcalloc() failure warning in
> > your patch looks wrong:
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index b0d6a0a..29cfe1a 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -4577,13 +4577,15 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
> >   */
> >  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> >  {
> > -	if (!dev->dma_alias_mask) {
> > +	if (!dev->dma_alias_mask)
> >  		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> >  					      sizeof(long), GFP_KERNEL);
> > -		dev_warn(&dev->dev, "Unable to allocate DMA alias mask.\n");
> > +	if (!dev->dma_alias_mask) {
> > +		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
> > +		return;
> >  	}
> > -	if (dev->dma_alias_mask)
> > -		set_bit(devfn, dev->dma_alias_mask);
> > +
> > +	set_bit(devfn, dev->dma_alias_mask);
> >  }
> >  EXPORT_SYMBOL_GPL(pci_add_dma_alias);
> >  
> > 
> > > ---
> > >  drivers/iommu/iommu.c |  8 ++++----
> > >  drivers/pci/pci.c     | 19 +++++++++++++++++++
> > >  drivers/pci/probe.c   |  1 +
> > >  drivers/pci/quirks.c  | 15 ++++++---------
> > >  drivers/pci/search.c  | 14 +++++++++-----
> > >  include/linux/pci.h   | 15 ++++++++-------
> > >  6 files changed, 47 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index abae363..98ae7ff 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -686,10 +686,10 @@ 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 ((pdev->dma_alias_mask &&
> > > +		     test_bit(tmp->devfn, pdev->dma_alias_mask)) ||
> > > +		    ((tmp->dma_alias_mask &&
> > > +		     test_bit(pdev->devfn, tmp->dma_alias_mask)))) {
> > >  
> > >  			group = get_pci_alias_group(tmp, devfns);
> > >  			if (group) {
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 49e3715..5b27d65 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -4568,6 +4568,25 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
> > >  	return 0;
> > >  }
> > >  
> > > +/**
> > > + * pci_add_dma_alias - Allows to add multiple devfn aliases for given 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
> > > + */
> > > +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> > > +{
> > > +	if (!dev->dma_alias_mask) {
> > > +		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> > > +					      sizeof(long), GFP_KERNEL);
> > > +		dev_warn(&dev->dev, "Unable to allocate DMA alias mask.\n");
> > > +	}
> > > +	if (dev->dma_alias_mask)
> > > +		set_bit(devfn, dev->dma_alias_mask);
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_add_dma_alias);
> > > +
> > >  bool pci_device_is_present(struct pci_dev *pdev)
> > >  {
> > >  	u32 v;
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 32b9f1b..5da4dd3 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -1501,6 +1501,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/quirks.c b/drivers/pci/quirks.c
> > > index 83e93d7..3ed1f9a 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -3579,8 +3579,7 @@ 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;
> > > +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> > >  	}
> > >  }
> > >  
> > > @@ -3595,8 +3594,7 @@ 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;
> > > +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
> > >  	}
> > >  }
> > >  
> > > @@ -3660,11 +3658,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;
> > > -		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_add_dma_alias(dev, id->driver_data);
> > > +		dev_info(&dev->dev, "Enabling fixed DMA alias to %02lx.%ld\n",
> > > +			 PCI_SLOT(id->driver_data),
> > > +			 PCI_FUNC(id->driver_data));
> > >  	}
> > >  }
> > >  
> > > 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 f9f79ad..6200175 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -172,16 +172,14 @@ 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),
> > > +	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 {
> > > @@ -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
> > > @@ -1229,6 +1227,9 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
> > >  
> > >  int pci_set_vga_state(struct pci_dev *pdev, bool decode,
> > >  		      unsigned int command_bits, u32 flags);
> > > +
> > > +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
> > > +
> > >  /* kmem_cache style wrapper around pci_alloc_consistent() */
> > >  
> > >  #include 

Re: [PATCH] pci: Add support for multiple DMA aliases

[Bjorn Helgaas]

On Tue, Jan 19, 2016 at 02:04:31PM -0700, Alex Williamson wrote:
> On Tue, 2016-01-19 at 14:12 -0600, Bjorn Helgaas wrote:
> > [+cc Alex]
> > 
> > On Mon, Jan 18, 2016 at 09:33:15PM -0600, Bjorn Helgaas wrote:
> > > On Mon, Jan 18, 2016 at 05:07:47PM +0100, Jacek Lawrynowicz wrote:
> > > > 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.
> > > > 
> > > > 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.
> 
> My only concern here is that pci_add_dma_alias() makes aliases seem
> more dynamic than they really are.  For instance, when we add a device
> to an IOMMU domain, we evaluate the aliases at that point, if an NTB
> later adds a new lookup entry and specifies a new alias, it's still not
> going to work.  Similarly, IOMMU groups are evaluated as the device is
> added, so if an alias is to a physical device and we need the cross
> reference to bind them together into a single group, calling
> pci_add_dma_alias() from a driver isn't going to work.
> 
> The existing code had this problem too, it's just more obvious now that
> we have a helper function and that the helper is exported for use
> outside of the PCI core.  Thanks,

Oh, that's a really good point.  I hadn't noticed the export.  Is
there any reason pci_add_dma_alias() needs to be declared in
include/linux/pci.h and exported to modules?

I don't think the current patch requires the export, but I suppose you
envision an NTB driver that might be a module?  I guess we can easily
export it when that driver is merged if that seems the best solution.

In the meantime, I made the following change to stop exporting it:

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 29cfe1a..b6434c0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4587,7 +4587,6 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
 
 	set_bit(devfn, dev->dma_alias_mask);
 }
-EXPORT_SYMBOL_GPL(pci_add_dma_alias);
 
 bool pci_device_is_present(struct pci_dev *pdev)
 {
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fd2f03f..1aad757 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -339,4 +339,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 
 struct pci_host_bridge *pci_find_host_bridge(struct pci_bus *bus);
 
+void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
+
 #endif /* DRIVERS_PCI_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 66c07d0..00d0862 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1228,8 +1228,6 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
 int pci_set_vga_state(struct pci_dev *pdev, bool decode,
 		      unsigned int command_bits, u32 flags);
 
-void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
-
 /* kmem_cache style wrapper around pci_alloc_consistent() */
 
 #include <linux/pci-dma.h>



> > > > 
> > > > 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 applied this to pci/iommu and intend to merge it for v4.5.
> > > 
> > > I made the following change because the kcalloc() failure warning in
> > > your patch looks wrong:
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index b0d6a0a..29cfe1a 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -4577,13 +4577,15 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
> > >   */
> > >  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> > >  {
> > > -	if (!dev->dma_alias_mask) {
> > > +	if (!dev->dma_alias_mask)
> > >  		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> > >  					      sizeof(long), GFP_KERNEL);
> > > -		dev_warn(&dev->dev, "Unable to allocate DMA alias mask.\n");
> > > +	if (!dev->dma_alias_mask) {
> > > +		dev_warn(&dev->dev, "Unable to allocate DMA alias mask\n");
> > > +		return;
> > >  	}
> > > -	if (dev->dma_alias_mask)
> > > -		set_bit(devfn, dev->dma_alias_mask);
> > > +
> > > +	set_bit(devfn, dev->dma_alias_mask);
> > >  }
> > >  EXPORT_SYMBOL_GPL(pci_add_dma_alias);
> > >  
> > > 
> > > > ---
> > > >  drivers/iommu/iommu.c |  8 ++++----
> > > >  drivers/pci/pci.c     | 19 +++++++++++++++++++
> > > >  drivers/pci/probe.c   |  1 +
> > > >  drivers/pci/quirks.c  | 15 ++++++---------
> > > >  drivers/pci/search.c  | 14 +++++++++-----
> > > >  include/linux/pci.h   | 15 ++++++++-------
> > > >  6 files changed, 47 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > index abae363..98ae7ff 100644
> > > > --- a/drivers/iommu/iommu.c
> > > > +++ b/drivers/iommu/iommu.c
> > > > @@ -686,10 +686,10 @@ 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 ((pdev->dma_alias_mask &&
> > > > +		     test_bit(tmp->devfn, pdev->dma_alias_mask)) ||
> > > > +		    ((tmp->dma_alias_mask &&
> > > > +		     test_bit(pdev->devfn, tmp->dma_alias_mask)))) {
> > > >  
> > > >  			group = get_pci_alias_group(tmp, devfns);
> > > >  			if (group) {
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 49e3715..5b27d65 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -4568,6 +4568,25 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +/**
> > > > + * pci_add_dma_alias - Allows to add multiple devfn aliases for given 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
> > > > + */
> > > > +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
> > > > +{
> > > > +	if (!dev->dma_alias_mask) {
> > > > +		dev->dma_alias_mask = kcalloc(BITS_TO_LONGS(U8_MAX),
> > > > +					      sizeof(long), GFP_KERNEL);
> > > > +		dev_warn(&dev->dev, "Unable to allocate DMA alias mask.\n");
> > > > +	}
> > > > +	if (dev->dma_alias_mask)
> > > > +		set_bit(devfn, dev->dma_alias_mask);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_add_dma_alias);
> > > > +
> > > >  bool pci_device_is_present(struct pci_dev *pdev)
> > > >  {
> > > >  	u32 v;
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 32b9f1b..5da4dd3 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -1501,6 +1501,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/quirks.c b/drivers/pci/quirks.c
> > > > index 83e93d7..3ed1f9a 100644
> > > > --- a/drivers/pci/quirks.c
> > > > +++ b/drivers/pci/quirks.c
> > > > @@ -3579,8 +3579,7 @@ 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;
> > > > +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> > > >  	}
> > > >  }
> > > >  
> > > > @@ -3595,8 +3594,7 @@ 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;
> > > > +		pci_add_dma_alias(dev, PCI_DEVFN(PCI_SLOT(dev->devfn), 1));
> > > >  	}
> > > >  }
> > > >  
> > > > @@ -3660,11 +3658,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;
> > > > -		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_add_dma_alias(dev, id->driver_data);
> > > > +		dev_info(&dev->dev, "Enabling fixed DMA alias to %02lx.%ld\n",
> > > > +			 PCI_SLOT(id->driver_data),
> > > > +			 PCI_FUNC(id->driver_data));
> > > >  	}
> > > >  }
> > > >  
> > > > 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 f9f79ad..6200175 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -172,16 +172,14 @@ 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),
> > > > +	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 {
> > > > @@ -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
> > > > @@ -1229,6 +1227,9 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *dev, int resno);
> > > >  
> > > >  int pci_set_vga_state(struct pci_dev *pdev, bool decode,
> > > >  		      unsigned int command_bits, u32 flags);
> > > > +
> > > > +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn);
> > > > +
> > > >  /* kmem_cache style wrapper around pci_alloc_consistent() */
> > > >  
> > > >  #include 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] pci: Add support for multiple DMA aliases

[Lawrynowicz, Jacek]

> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> Sent: Tuesday, January 19, 2016 10:39 PM
> To: Alex Williamson <alex.williamson@redhat.com>
> Cc: Lawrynowicz, Jacek <jacek.lawrynowicz@intel.com>; linux-
> pci@vger.kernel.org; bhelgaas@google.com; dwmw2@infradead.org;
> jroedel@suse.de
> Subject: Re: [PATCH] pci: Add support for multiple DMA aliases
> 
> On Tue, Jan 19, 2016 at 02:04:31PM -0700, Alex Williamson wrote:
> > On Tue, 2016-01-19 at 14:12 -0600, Bjorn Helgaas wrote:
> > > [+cc Alex]
> > >
> > > On Mon, Jan 18, 2016 at 09:33:15PM -0600, Bjorn Helgaas wrote:
> > > > On Mon, Jan 18, 2016 at 05:07:47PM +0100, Jacek Lawrynowicz wrote:
> > > > > 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.
> > > > >
> > > > > 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.
> >
> > My only concern here is that pci_add_dma_alias() makes aliases seem
> > more dynamic than they really are.  For instance, when we add a device
> > to an IOMMU domain, we evaluate the aliases at that point, if an NTB
> > later adds a new lookup entry and specifies a new alias, it's still
> > not going to work.  Similarly, IOMMU groups are evaluated as the
> > device is added, so if an alias is to a physical device and we need
> > the cross reference to bind them together into a single group, calling
> > pci_add_dma_alias() from a driver isn't going to work.
> >
> > The existing code had this problem too, it's just more obvious now
> > that we have a helper function and that the helper is exported for use
> > outside of the PCI core.  Thanks,
> 
> Oh, that's a really good point.  I hadn't noticed the export.  Is there any
> reason pci_add_dma_alias() needs to be declared in include/linux/pci.h and
> exported to modules?
> 
> I don't think the current patch requires the export, but I suppose you
> envision an NTB driver that might be a module?  I guess we can easily export
> it when that driver is merged if that seems the best solution.

This export would be useful for Xeon Phi x200 which uses on a NTB generating
multiple RIDs. x200 is not yet ready for upstreming (x100 is already upstreamed) and
having this export would make driver development less painful.

--
Jacek Lawrynowicz
Intel Technology Poland sp. z o.o.
KRS 101882 - ul. Slowackiego 173, 80-298 Gdansk

Re: [PATCH] pci: Add support for multiple DMA aliases

[Bjorn Helgaas]

Hi Jacek,

On Wed, Jan 20, 2016 at 03:02:26PM +0000, Lawrynowicz, Jacek wrote:
> > -----Original Message-----
> > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> > Sent: Tuesday, January 19, 2016 10:39 PM
> > To: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Lawrynowicz, Jacek <jacek.lawrynowicz@intel.com>; linux-
> > pci@vger.kernel.org; bhelgaas@google.com; dwmw2@infradead.org;
> > jroedel@suse.de
> > Subject: Re: [PATCH] pci: Add support for multiple DMA aliases
> > 
> > On Tue, Jan 19, 2016 at 02:04:31PM -0700, Alex Williamson wrote:
> > > On Tue, 2016-01-19 at 14:12 -0600, Bjorn Helgaas wrote:
> > > > [+cc Alex]
> > > >
> > > > On Mon, Jan 18, 2016 at 09:33:15PM -0600, Bjorn Helgaas wrote:
> > > > > On Mon, Jan 18, 2016 at 05:07:47PM +0100, Jacek Lawrynowicz wrote:
> > > > > > 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.
> > > > > >
> > > > > > 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.
> > >
> > > My only concern here is that pci_add_dma_alias() makes aliases seem
> > > more dynamic than they really are.  For instance, when we add a device
> > > to an IOMMU domain, we evaluate the aliases at that point, if an NTB
> > > later adds a new lookup entry and specifies a new alias, it's still
> > > not going to work.  Similarly, IOMMU groups are evaluated as the
> > > device is added, so if an alias is to a physical device and we need
> > > the cross reference to bind them together into a single group, calling
> > > pci_add_dma_alias() from a driver isn't going to work.
> > >
> > > The existing code had this problem too, it's just more obvious now
> > > that we have a helper function and that the helper is exported for use
> > > outside of the PCI core.  Thanks,
> > 
> > Oh, that's a really good point.  I hadn't noticed the export.  Is there any
> > reason pci_add_dma_alias() needs to be declared in include/linux/pci.h and
> > exported to modules?
> > 
> > I don't think the current patch requires the export, but I suppose you
> > envision an NTB driver that might be a module?  I guess we can easily export
> > it when that driver is merged if that seems the best solution.
> 
> This export would be useful for Xeon Phi x200 which uses on a NTB generating
> multiple RIDs. x200 is not yet ready for upstreming (x100 is already upstreamed) and
> having this export would make driver development less painful.

I don't really want to merge things that only exist to enable
out-of-tree development, because (1) they're an extra maintenance
burden for which we get risk without benefit, and (2) we can't see
the out-of-tree code, so it's easy for people to make changes that
accidentally break that code.

Looking at the patch again, I see that even without the export,
there's no current benefit, and there are a couple things that should
be fixed up:

  - Fix the comment that references dma_alias_devfn (since you removed
    that field).

  - Add an interface that get_pci_alias_group() can use instead of
    accessing the dma_alias_mask directly.

  - Figure out the scope and exportability of pci_add_dma_alias() and
    the new boolean interface I'm suggesting.

So I'm going to drop this for now, and you can carry it along with
your driver patches.  Then when we merge the driver, we should think
about whether it makes sense to export pci_add_dma_alias(), or whether
we can come up with an interface that is safer with regard to the
issues Alex mentioned.

I think this patch makes a lot of sense, so I'm definitely not
rejecting it.  But I think it will make even more sense in the context
of the driver, when we can think about the lifetime of the aliases.
(*You* know that already, but I don't, so I'm operating with a lot of
missing information :))

Bjorn

Re: [PATCH] pci: Add support for multiple DMA aliases

[David Woodhouse]

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

On Wed, 2016-01-20 at 11:46 -0600, Bjorn Helgaas wrote:
> 
> I don't really want to merge things that only exist to enable
> out-of-tree development, because (1) they're an extra maintenance
> burden for which we get risk without benefit, and (2) we can't see
> the out-of-tree code, so it's easy for people to make changes that
> accidentally break that code.
> 
> Looking at the patch again, I see that even without the export,
> there's no current benefit,

This is just a PCI quirk; I'm not sure it should be considered part of
the driver code at all. With this patch, even without a Linux driver,
we could correctly handle assignment to VM guests (which *might* have a
driver), and also theoretically we should be able to handle fault
storms and shoot the right device in the head if it was left in an odd
state and misbehaves (not that I've hooked that up yet).

So I'm not sure it makes sense to tie this patch to the existence of a
driver.
 
-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


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

RE: [PATCH] pci: Add support for multiple DMA aliases

[Lawrynowicz, Jacek]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Wednesday, January 20, 2016 6:46 PM
> To: Lawrynowicz, Jacek <jacek.lawrynowicz@intel.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>; linux-
> pci@vger.kernel.org; bhelgaas@google.com; dwmw2@infradead.org;
> jroedel@suse.de
> Subject: Re: [PATCH] pci: Add support for multiple DMA aliases
> 
> Hi Jacek,
> 
> On Wed, Jan 20, 2016 at 03:02:26PM +0000, Lawrynowicz, Jacek wrote:
> > > -----Original Message-----
> > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> > > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> > > Sent: Tuesday, January 19, 2016 10:39 PM
> > > To: Alex Williamson <alex.williamson@redhat.com>
> > > Cc: Lawrynowicz, Jacek <jacek.lawrynowicz@intel.com>; linux-
> > > pci@vger.kernel.org; bhelgaas@google.com; dwmw2@infradead.org;
> > > jroedel@suse.de
> > > Subject: Re: [PATCH] pci: Add support for multiple DMA aliases
> > >
> > > On Tue, Jan 19, 2016 at 02:04:31PM -0700, Alex Williamson wrote:
> > > > On Tue, 2016-01-19 at 14:12 -0600, Bjorn Helgaas wrote:
> > > > > [+cc Alex]
> > > > >
> > > > > On Mon, Jan 18, 2016 at 09:33:15PM -0600, Bjorn Helgaas wrote:
> > > > > > On Mon, Jan 18, 2016 at 05:07:47PM +0100, Jacek Lawrynowicz
> wrote:
> > > > > > > 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.
> > > > > > >
> > > > > > > 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.
> > > >
> > > > My only concern here is that pci_add_dma_alias() makes aliases
> > > > seem more dynamic than they really are.  For instance, when we add
> > > > a device to an IOMMU domain, we evaluate the aliases at that
> > > > point, if an NTB later adds a new lookup entry and specifies a new
> > > > alias, it's still not going to work.  Similarly, IOMMU groups are
> > > > evaluated as the device is added, so if an alias is to a physical
> > > > device and we need the cross reference to bind them together into
> > > > a single group, calling
> > > > pci_add_dma_alias() from a driver isn't going to work.
> > > >
> > > > The existing code had this problem too, it's just more obvious now
> > > > that we have a helper function and that the helper is exported for
> > > > use outside of the PCI core.  Thanks,
> > >
> > > Oh, that's a really good point.  I hadn't noticed the export.  Is
> > > there any reason pci_add_dma_alias() needs to be declared in
> > > include/linux/pci.h and exported to modules?
> > >
> > > I don't think the current patch requires the export, but I suppose
> > > you envision an NTB driver that might be a module?  I guess we can
> > > easily export it when that driver is merged if that seems the best
> solution.
> >
> > This export would be useful for Xeon Phi x200 which uses on a NTB
> > generating multiple RIDs. x200 is not yet ready for upstreming (x100
> > is already upstreamed) and having this export would make driver
> development less painful.
> 
> I don't really want to merge things that only exist to enable out-of-tree
> development, because (1) they're an extra maintenance burden for which
> we get risk without benefit, and (2) we can't see the out-of-tree code, so it's
> easy for people to make changes that accidentally break that code.
> 
> Looking at the patch again, I see that even without the export, there's no
> current benefit, and there are a couple things that should be fixed up:
> 
>   - Fix the comment that references dma_alias_devfn (since you removed
>     that field).
> 
>   - Add an interface that get_pci_alias_group() can use instead of
>     accessing the dma_alias_mask directly.
> 
>   - Figure out the scope and exportability of pci_add_dma_alias() and
>     the new boolean interface I'm suggesting.
> 
> So I'm going to drop this for now, and you can carry it along with your driver
> patches.  Then when we merge the driver, we should think about whether it
> makes sense to export pci_add_dma_alias(), or whether we can come up
> with an interface that is safer with regard to the issues Alex mentioned.
> 
> I think this patch makes a lot of sense, so I'm definitely not rejecting it.  But I
> think it will make even more sense in the context of the driver, when we can
> think about the lifetime of the aliases.
> (*You* know that already, but I don't, so I'm operating with a lot of missing
> information :))

I would understand rejecting the patch if it would be specific to out of the tree
driver. It make perfect sense from kernel development perspective but this
patch is not device specific and everyone previously agreed it improves current
dma alias handling. It's also small and without the export it has very little
maintenance impact.

Please reconsider merging the patch.

--
Jacek Lawrynowicz
Intel Technology Poland sp. z o.o.
KRS 101882 - ul. Slowackiego 173, 80-298 Gdansk

Re: [PATCH] pci: Add support for multiple DMA aliases

[Bjorn Helgaas]

On Thu, Jan 21, 2016 at 09:39:01AM +0000, David Woodhouse wrote:
> On Wed, 2016-01-20 at 11:46 -0600, Bjorn Helgaas wrote:
> > 
> > I don't really want to merge things that only exist to enable
> > out-of-tree development, because (1) they're an extra maintenance
> > burden for which we get risk without benefit, and (2) we can't see
> > the out-of-tree code, so it's easy for people to make changes that
> > accidentally break that code.
> > 
> > Looking at the patch again, I see that even without the export,
> > there's no current benefit,
> 
> This is just a PCI quirk; I'm not sure it should be considered part of
> the driver code at all. With this patch, even without a Linux driver,
> we could correctly handle assignment to VM guests (which *might* have a
> driver), and also theoretically we should be able to handle fault
> storms and shoot the right device in the head if it was left in an odd
> state and misbehaves (not that I've hooked that up yet).
> 
> So I'm not sure it makes sense to tie this patch to the existence of a
> driver.

This definitely isn't part of the driver code; I didn't mean to
suggest that.  I'd like to see this as a separate patch, but as part
of a series that adds a user of the multiple-alias functionality.

All I'm saying is that as-is, this patch makes the quirks easier to
read but doesn't actually change any behavior: we set up at most one
alias, and we do it as a header quirk at enumeration-time, so there
are no new lifetime issues.  Normally we merge things when they're
needed, and multiple alias support is a bit of infrastructure that
isn't used yet.

I already said I'm not rejecting the patch.  Alex and I raised a few
questions.  Usually that leads to a little discussion and possibly a
v2 of the patch, but so far, I haven't seen any answers.

Here are a couple more questions/concerns:

  - If we export an "add" function, do we need a corresponding
    "remove"?  This depends on how the alias lifetimes are managed,
    and I haven't seen that yet.

  - Changing pci_dev_flags and struct pci_dev changes the ABI and makes
    work for distros, with no current benefit.

I'm not sure why we're even having this discussion.  The merge window
opened Jan 10, and IIRC, I first saw the patch Jan 11 and it first
appeared on linux-pci Jan 13, so this is just late for v4.5 to begin
with.  

Bjorn

Re: [PATCH] pci: Add support for multiple DMA aliases

[David Woodhouse]

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

On Thu, 2016-01-21 at 09:22 -0600, Bjorn Helgaas wrote:
> 
> This definitely isn't part of the driver code; I didn't mean to
> suggest that.  I'd like to see this as a separate patch, but as part
> of a series that adds a user of the multiple-alias functionality.
> 
> All I'm saying is that as-is, this patch makes the quirks easier to
> read but doesn't actually change any behavior: we set up at most one
> alias, and we do it as a header quirk at enumeration-time, so there
> are no new lifetime issues.  Normally we merge things when they're
> needed, and multiple alias support is a bit of infrastructure that
> isn't used yet.

Ah, right. I see your point.

I don't actually see why pci_add_dma_alias() should be exported at all.

I suspect the best approach is for Jacek to add a second patch in this
series, adding the required quirk to drivers/pci/quirks.c for the
device in question, and then resubmit them to you when Linus releases
4.5-rc1.

This is completely independent of any native driver for the device, of
course.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


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

RE: [PATCH] pci: Add support for multiple DMA aliases

[Lawrynowicz, Jacek]

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

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@infradead.org]
> Sent: Thursday, January 21, 2016 4:33 PM
> To: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Lawrynowicz, Jacek <jacek.lawrynowicz@intel.com>; Alex Williamson
> <alex.williamson@redhat.com>; linux-pci@vger.kernel.org;
> bhelgaas@google.com; jroedel@suse.de
> Subject: Re: [PATCH] pci: Add support for multiple DMA aliases
>
> On Thu, 2016-01-21 at 09:22 -0600, Bjorn Helgaas wrote:
> >
> > This definitely isn't part of the driver code; I didn't mean to
> > suggest that.  I'd like to see this as a separate patch, but as part
> > of a series that adds a user of the multiple-alias functionality.
> >
> > All I'm saying is that as-is, this patch makes the quirks easier to
> > read but doesn't actually change any behavior: we set up at most one
> > alias, and we do it as a header quirk at enumeration-time, so there
> > are no new lifetime issues.  Normally we merge things when they're
> > needed, and multiple alias support is a bit of infrastructure that
> > isn't used yet.
>
> Ah, right. I see your point.
>
> I don't actually see why pci_add_dma_alias() should be exported at all.
>
> I suspect the best approach is for Jacek to add a second patch in this
> series, adding the required quirk to drivers/pci/quirks.c for the
> device in question, and then resubmit them to you when Linus releases
> 4.5-rc1.
>
> This is completely independent of any native driver for the device, of
> course.

OK guys, I've prepared a second version of the patch that includes fixes for 
all
your review comments. I will post it today to linux-pci. It also includes a 
quirk
for x200 dma driver that is undergoing internal review and will be posted to 
dma
tree probably early next week. It would be great if v2 could be included in 
4.5 but
I understand that it's a bit late and you may prefer to wait for x200 dma 
driver to
be posted.

Thanks for all your feedback. The patch now make a lot more sense.

Regards,
Jacek

--
Jacek Lawrynowicz
Intel Technology Poland sp. z o.o.
KRS 101882 - ul. Slowackiego 173, 80-298 Gdansk



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

[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