[PATCH 0/3] AMD IOMMU Changes for NTB

[PATCH 0/3] AMD IOMMU Changes for NTB

[Logan Gunthorpe]

Hi,

Please find the following patches which help support
Non-Transparent-Bridge (NTB) devices on AMD platforms with the IOMMU
enabled.

The first patch implements dma_map_resource() correctly with the AMD
IOMMU. This is required for correct functioning of ntb_transport which
uses that interface.

The second two patches add support for multiple PCI aliases. NTB
hardware will normally send TLPs from a range of requestor IDs to
facilitate routing the responses back to the correct requestor on the
other side of the bridge. To support this, NTB hardware registers a
number of PCI aliases. Currently the AMD IOMMU only allows for one
PCI alias so TLPs from the other aliases get rejected.

See commit ad281ecf1c7d ("PCI: Add DMA alias quirk for Microsemi
Switchtec NTB") for more information on this.

Similar patches were upstreamed for Intel hardware earlier this year:

commit 21d5d27c042d ("iommu/vt-d: Implement dma_[un]map_resource()")
commit 3f0c625c6ae7 ("iommu/vt-d: Allow interrupts from the entire bus
    for aliased devices")

Thanks,

Logan

--

Kit Chow (1):
  iommu/amd: Implement dma_[un]map_resource()

Logan Gunthorpe (2):
  iommu/amd: Support multiple PCI DMA aliases in device table
  iommu/amd: Support multiple PCI DMA aliases in IRQ Remapping

 drivers/iommu/amd_iommu.c       | 198 +++++++++++++++++++-------------
 drivers/iommu/amd_iommu_types.h |   2 +-
 2 files changed, 120 insertions(+), 80 deletions(-)

--
2.20.1
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/3] iommu/amd: Implement dma_[un]map_resource()

[Logan Gunthorpe]

From: Kit Chow <kchow@gigaio.com>

Currently the Intel IOMMU uses the default dma_[un]map_resource()
implementations does nothing and simply returns the physical address
unmodified.

However, this doesn't create the IOVA entries necessary for addresses
mapped this way to work when the IOMMU is enabled. Thus, when the
IOMMU is enabled, drivers relying on dma_map_resource() will not get the
propper mapping. We see this when running ntb_transport with the IOMMU
enabled, DMA, and switchtec hardware.

The implementation for the amd version of map_resource() is nearly
identical to map_page(), just with a phys_addr passed instead of a page.
dma_unmap_resource() uses unmap_page() directly as the functions are
identical.

Signed-off-by: Kit Chow <kchow@gigaio.com>
[logang@deltatee.com: Cleaned up into a propper commit and wrote the
    commit message]
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/iommu/amd_iommu.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 2369b8af81f3..aa3d9e705a45 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2553,6 +2553,23 @@ static void unmap_page(struct device *dev, dma_addr_t dma_addr, size_t size,
 	__unmap_single(dma_dom, dma_addr, size, dir);
 }
 
+static dma_addr_t map_resource(struct device *dev, phys_addr_t paddr,
+		size_t size, enum dma_data_direction dir, unsigned long attrs)
+{
+	struct protection_domain *domain;
+	struct dma_ops_domain *dma_dom;
+
+	domain = get_domain(dev);
+	if (PTR_ERR(domain) == -EINVAL)
+		return (dma_addr_t)paddr;
+	else if (IS_ERR(domain))
+		return DMA_MAPPING_ERROR;
+
+	dma_dom = to_dma_ops_domain(domain);
+
+	return __map_single(dev, dma_dom, paddr, size, dir, *dev->dma_mask);
+}
+
 static int sg_num_pages(struct device *dev,
 			struct scatterlist *sglist,
 			int nelems)
@@ -2795,6 +2812,8 @@ static const struct dma_map_ops amd_iommu_dma_ops = {
 	.unmap_page	= unmap_page,
 	.map_sg		= map_sg,
 	.unmap_sg	= unmap_sg,
+	.map_resource	= map_resource,
+	.unmap_resource	= unmap_page,
 	.dma_supported	= amd_iommu_dma_supported,
 	.mmap		= dma_common_mmap,
 	.get_sgtable	= dma_common_get_sgtable,
-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/3] iommu/amd: Implement dma_[un]map_resource()

[Christoph Hellwig]

On Tue, Oct 08, 2019 at 04:18:35PM -0600, Logan Gunthorpe wrote:
> From: Kit Chow <kchow@gigaio.com>
> 
> Currently the Intel IOMMU uses the default dma_[un]map_resource()

s/Intel/AMD/ ?

> +static dma_addr_t map_resource(struct device *dev, phys_addr_t paddr,
> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
> +{
> +	struct protection_domain *domain;
> +	struct dma_ops_domain *dma_dom;
> +
> +	domain = get_domain(dev);
> +	if (PTR_ERR(domain) == -EINVAL)
> +		return (dma_addr_t)paddr;

I thought that case can't happen anymore?

Also note that Joerg just applied the patch to convert the AMD iommu
driver to use the dma-iommu ops.  Can you test that series and check
it does the right thing for your use case?  From looking at the code
I think it should.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/3] iommu/amd: Implement dma_[un]map_resource()

[Logan Gunthorpe]

On 2019-10-09 12:57 a.m., Christoph Hellwig wrote:
> On Tue, Oct 08, 2019 at 04:18:35PM -0600, Logan Gunthorpe wrote:
>> From: Kit Chow <kchow@gigaio.com>
>>
>> Currently the Intel IOMMU uses the default dma_[un]map_resource()
> 
> s/Intel/AMD/ ?

Oops, yes, my mistake.

>> +static dma_addr_t map_resource(struct device *dev, phys_addr_t paddr,
>> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +	struct protection_domain *domain;
>> +	struct dma_ops_domain *dma_dom;
>> +
>> +	domain = get_domain(dev);
>> +	if (PTR_ERR(domain) == -EINVAL)
>> +		return (dma_addr_t)paddr;
> 
> I thought that case can't happen anymore?
> 
> Also note that Joerg just applied the patch to convert the AMD iommu
> driver to use the dma-iommu ops.  Can you test that series and check
> it does the right thing for your use case?  From looking at the code
> I think it should.

Yes, looking at the new code, it looks like this patch will not be
needed. So we can drop it. We'll test it to make sure.

I believe the other two patches in this series are still needed though.

Thanks,

Logan


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/3] iommu/amd: Implement dma_[un]map_resource()

[Logan Gunthorpe]

On 2019-10-09 12:57 a.m., Christoph Hellwig wrote:
> On Tue, Oct 08, 2019 at 04:18:35PM -0600, Logan Gunthorpe wrote:
>> From: Kit Chow <kchow@gigaio.com>
>>
>> Currently the Intel IOMMU uses the default dma_[un]map_resource()
> 
> s/Intel/AMD/ ?

Oops, yes, my mistake.

>> +static dma_addr_t map_resource(struct device *dev, phys_addr_t paddr,
>> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +	struct protection_domain *domain;
>> +	struct dma_ops_domain *dma_dom;
>> +
>> +	domain = get_domain(dev);
>> +	if (PTR_ERR(domain) == -EINVAL)
>> +		return (dma_addr_t)paddr;
> 
> I thought that case can't happen anymore?
> 
> Also note that Joerg just applied the patch to convert the AMD iommu
> driver to use the dma-iommu ops.  Can you test that series and check
> it does the right thing for your use case?  From looking at the code
> I think it should.

Yes, looking at the new code, it looks like this patch will not be
needed. So we can drop it. We'll test it to make sure.

I believe the other two patches in this series are still needed though.

Thanks,

Logan



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/3] iommu/amd: Support multiple PCI DMA aliases in device table

[Logan Gunthorpe]

Non-Transparent Bridge (NTB) devices (among others) may have many DMA
aliases seeing the hardware will send requests with different device ids
depending on their origin across the bridged hardware.

See commit ad281ecf1c7d ("PCI: Add DMA alias quirk for Microsemi
Switchtec NTB") for more information on this.

The AMD IOMMU ignores all the PCI aliases except the last one so DMA
transfers from these aliases will be blocked on AMD hardware with the
IOMMU enabled.

To fix this, ensure the DTEs are cloned for every PCI alias. This is
done by copying the DTE data for each alias as well as the IVRS alias
every time it is changed.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/iommu/amd_iommu.c       | 133 +++++++++++++++-----------------
 drivers/iommu/amd_iommu_types.h |   2 +-
 2 files changed, 62 insertions(+), 73 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index aa3d9e705a45..1f15f0bae5b3 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -226,71 +226,61 @@ static struct iommu_dev_data *search_dev_data(u16 devid)
 	return NULL;
 }
 
-static int __last_alias(struct pci_dev *pdev, u16 alias, void *data)
+static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
 {
-	*(u16 *)data = alias;
-	return 0;
-}
-
-static u16 get_alias(struct device *dev)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	u16 devid, ivrs_alias, pci_alias;
+	u16 devid = pci_dev_id(pdev);
 
-	/* The callers make sure that get_device_id() does not fail here */
-	devid = get_device_id(dev);
-
-	/* For ACPI HID devices, we simply return the devid as such */
-	if (!dev_is_pci(dev))
-		return devid;
+	if (devid == alias)
+		return 0;
 
-	ivrs_alias = amd_iommu_alias_table[devid];
+	amd_iommu_rlookup_table[alias] =
+		amd_iommu_rlookup_table[devid];
+	memcpy(amd_iommu_dev_table[alias].data,
+	       amd_iommu_dev_table[devid].data,
+	       sizeof(amd_iommu_dev_table[alias].data));
 
-	pci_for_each_dma_alias(pdev, __last_alias, &pci_alias);
+	return 0;
+}
 
-	if (ivrs_alias == pci_alias)
-		return ivrs_alias;
+static void clone_aliases(struct pci_dev *pdev)
+{
+	if (!pdev)
+		return;
 
 	/*
-	 * DMA alias showdown
-	 *
-	 * The IVRS is fairly reliable in telling us about aliases, but it
-	 * can't know about every screwy device.  If we don't have an IVRS
-	 * reported alias, use the PCI reported alias.  In that case we may
-	 * still need to initialize the rlookup and dev_table entries if the
-	 * alias is to a non-existent device.
+	 * The IVRS alias stored in the alias table may not be
+	 * part of the PCI DMA aliases if it's bus differs
+	 * from the original device.
 	 */
-	if (ivrs_alias == devid) {
-		if (!amd_iommu_rlookup_table[pci_alias]) {
-			amd_iommu_rlookup_table[pci_alias] =
-				amd_iommu_rlookup_table[devid];
-			memcpy(amd_iommu_dev_table[pci_alias].data,
-			       amd_iommu_dev_table[devid].data,
-			       sizeof(amd_iommu_dev_table[pci_alias].data));
-		}
+	clone_alias(pdev, amd_iommu_alias_table[pci_dev_id(pdev)], NULL);
 
-		return pci_alias;
-	}
+	pci_for_each_dma_alias(pdev, clone_alias, NULL);
+}
 
-	pci_info(pdev, "Using IVRS reported alias %02x:%02x.%d "
-		"for device [%04x:%04x], kernel reported alias "
-		"%02x:%02x.%d\n", PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
-		PCI_FUNC(ivrs_alias), pdev->vendor, pdev->device,
-		PCI_BUS_NUM(pci_alias), PCI_SLOT(pci_alias),
-		PCI_FUNC(pci_alias));
+static struct pci_dev *setup_aliases(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	u16 ivrs_alias;
+
+	/* For ACPI HID devices, there are no aliases */
+	if (!dev_is_pci(dev))
+		return NULL;
 
 	/*
-	 * If we don't have a PCI DMA alias and the IVRS alias is on the same
-	 * bus, then the IVRS table may know about a quirk that we don't.
+	 * Add the IVRS alias to the pci aliases if it is on the same
+	 * bus. The IVRS table may know about a quirk that we don't.
 	 */
-	if (pci_alias == devid &&
+	ivrs_alias = amd_iommu_alias_table[pci_dev_id(pdev)];
+	if (ivrs_alias != pci_dev_id(pdev) &&
 	    PCI_BUS_NUM(ivrs_alias) == pdev->bus->number) {
 		pci_add_dma_alias(pdev, ivrs_alias & 0xff);
 		pci_info(pdev, "Added PCI DMA alias %02x.%d\n",
 			PCI_SLOT(ivrs_alias), PCI_FUNC(ivrs_alias));
 	}
 
-	return ivrs_alias;
+	clone_aliases(pdev);
+
+	return pdev;
 }
 
 static struct iommu_dev_data *find_dev_data(u16 devid)
@@ -428,7 +418,7 @@ static int iommu_init_device(struct device *dev)
 	if (!dev_data)
 		return -ENOMEM;
 
-	dev_data->alias = get_alias(dev);
+	dev_data->pdev = setup_aliases(dev);
 
 	/*
 	 * By default we use passthrough mode for IOMMUv2 capable device.
@@ -453,20 +443,16 @@ static int iommu_init_device(struct device *dev)
 
 static void iommu_ignore_device(struct device *dev)
 {
-	u16 alias;
 	int devid;
 
 	devid = get_device_id(dev);
 	if (devid < 0)
 		return;
 
-	alias = get_alias(dev);
-
+	amd_iommu_rlookup_table[devid] = NULL;
 	memset(&amd_iommu_dev_table[devid], 0, sizeof(struct dev_table_entry));
-	memset(&amd_iommu_dev_table[alias], 0, sizeof(struct dev_table_entry));
 
-	amd_iommu_rlookup_table[devid] = NULL;
-	amd_iommu_rlookup_table[alias] = NULL;
+	setup_aliases(dev);
 }
 
 static void iommu_uninit_device(struct device *dev)
@@ -1235,6 +1221,13 @@ static int device_flush_iotlb(struct iommu_dev_data *dev_data,
 	return iommu_queue_command(iommu, &cmd);
 }
 
+static int device_flush_dte_alias(struct pci_dev *pdev, u16 alias, void *data)
+{
+	struct amd_iommu *iommu = data;
+
+	return iommu_flush_dte(iommu, alias);
+}
+
 /*
  * Command send function for invalidating a device table entry
  */
@@ -1245,14 +1238,22 @@ static int device_flush_dte(struct iommu_dev_data *dev_data)
 	int ret;
 
 	iommu = amd_iommu_rlookup_table[dev_data->devid];
-	alias = dev_data->alias;
 
-	ret = iommu_flush_dte(iommu, dev_data->devid);
-	if (!ret && alias != dev_data->devid)
-		ret = iommu_flush_dte(iommu, alias);
+	if (dev_data->pdev)
+		ret = pci_for_each_dma_alias(dev_data->pdev,
+					     device_flush_dte_alias, iommu);
+	else
+		ret = iommu_flush_dte(iommu, dev_data->devid);
 	if (ret)
 		return ret;
 
+	alias = amd_iommu_alias_table[dev_data->devid];
+	if (alias != dev_data->devid) {
+		ret = iommu_flush_dte(iommu, alias);
+		if (ret)
+			return ret;
+	}
+
 	if (dev_data->ats.enabled)
 		ret = device_flush_iotlb(dev_data, 0, ~0UL);
 
@@ -2033,11 +2034,9 @@ static void do_attach(struct iommu_dev_data *dev_data,
 		      struct protection_domain *domain)
 {
 	struct amd_iommu *iommu;
-	u16 alias;
 	bool ats;
 
 	iommu = amd_iommu_rlookup_table[dev_data->devid];
-	alias = dev_data->alias;
 	ats   = dev_data->ats.enabled;
 
 	/* Update data structures */
@@ -2050,8 +2049,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
 
 	/* Update device table */
 	set_dte_entry(dev_data->devid, domain, ats, dev_data->iommu_v2);
-	if (alias != dev_data->devid)
-		set_dte_entry(alias, domain, ats, dev_data->iommu_v2);
+	clone_aliases(dev_data->pdev);
 
 	device_flush_dte(dev_data);
 }
@@ -2060,17 +2058,14 @@ static void do_detach(struct iommu_dev_data *dev_data)
 {
 	struct protection_domain *domain = dev_data->domain;
 	struct amd_iommu *iommu;
-	u16 alias;
 
 	iommu = amd_iommu_rlookup_table[dev_data->devid];
-	alias = dev_data->alias;
 
 	/* Update data structures */
 	dev_data->domain = NULL;
 	list_del(&dev_data->list);
 	clear_dte_entry(dev_data->devid);
-	if (alias != dev_data->devid)
-		clear_dte_entry(alias);
+	clone_aliases(dev_data->pdev);
 
 	/* Flush the DTE entry */
 	device_flush_dte(dev_data);
@@ -2382,13 +2377,7 @@ static void update_device_table(struct protection_domain *domain)
 	list_for_each_entry(dev_data, &domain->dev_list, list) {
 		set_dte_entry(dev_data->devid, domain, dev_data->ats.enabled,
 			      dev_data->iommu_v2);
-
-		if (dev_data->devid == dev_data->alias)
-			continue;
-
-		/* There is an alias, update device table entry for it */
-		set_dte_entry(dev_data->alias, domain, dev_data->ats.enabled,
-			      dev_data->iommu_v2);
+		clone_aliases(dev_data->pdev);
 	}
 }
 
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index c9c1612d52e0..500de434181f 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -639,8 +639,8 @@ struct iommu_dev_data {
 	struct list_head list;		  /* For domain->dev_list */
 	struct llist_node dev_data_list;  /* For global dev_data_list */
 	struct protection_domain *domain; /* Domain the device is bound to */
+	struct pci_dev *pdev;
 	u16 devid;			  /* PCI Device ID */
-	u16 alias;			  /* Alias Device ID */
 	bool iommu_v2;			  /* Device can make use of IOMMUv2 */
 	bool passthrough;		  /* Device is identity mapped */
 	struct {
-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/3] iommu/amd: Support multiple PCI DMA aliases in IRQ Remapping

[Logan Gunthorpe]

Non-Transparent Bridge (NTB) devices (among others) may have many DMA
aliases seeing the hardware will send requests with different device ids
depending on their origin across the bridged hardware.

See commit ad281ecf1c7d ("PCI: Add DMA alias quirk for Microsemi Switchtec
NTB") for more information on this.

The AMD IOMMU IRQ remapping functionality ignores all PCI aliases for
IRQs so if devices send an interrupt from one of their aliases they
will be blocked on AMD hardware with the IOMMU enabled.

To fix this, ensure IRQ remapping is enabled for all aliases with
MSI interrupts.

This is analogous to the functionality added to the Intel IRQ remapping
code in commit 3f0c625c6ae7 ("iommu/vt-d: Allow interrupts from the entire
bus for aliased devices")

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/iommu/amd_iommu.c | 46 +++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 1f15f0bae5b3..f8a6fa8d639b 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3758,7 +3758,27 @@ static void set_remap_table_entry(struct amd_iommu *iommu, u16 devid,
 	iommu_flush_dte(iommu, devid);
 }
 
-static struct irq_remap_table *alloc_irq_table(u16 devid)
+static int set_remap_table_entry_alias(struct pci_dev *pdev, u16 alias,
+				       void *data)
+{
+	struct irq_remap_table *table = data;
+
+	irq_lookup_table[alias] = table;
+	set_dte_irq_entry(alias, table);
+
+	return 0;
+}
+
+static int iommu_flush_dte_alias(struct pci_dev *pdev, u16 alias, void *data)
+{
+	struct amd_iommu *iommu = data;
+
+	iommu_flush_dte(iommu, alias);
+
+	return 0;
+}
+
+static struct irq_remap_table *alloc_irq_table(u16 devid, struct pci_dev *pdev)
 {
 	struct irq_remap_table *table = NULL;
 	struct irq_remap_table *new_table = NULL;
@@ -3804,7 +3824,14 @@ static struct irq_remap_table *alloc_irq_table(u16 devid)
 	table = new_table;
 	new_table = NULL;
 
-	set_remap_table_entry(iommu, devid, table);
+	if (pdev) {
+		pci_for_each_dma_alias(pdev, set_remap_table_entry_alias,
+				       table);
+		pci_for_each_dma_alias(pdev, iommu_flush_dte_alias, iommu);
+	} else {
+		set_remap_table_entry(iommu, devid, table);
+	}
+
 	if (devid != alias)
 		set_remap_table_entry(iommu, alias, table);
 
@@ -3821,7 +3848,8 @@ static struct irq_remap_table *alloc_irq_table(u16 devid)
 	return table;
 }
 
-static int alloc_irq_index(u16 devid, int count, bool align)
+static int alloc_irq_index(u16 devid, int count, bool align,
+			   struct pci_dev *pdev)
 {
 	struct irq_remap_table *table;
 	int index, c, alignment = 1;
@@ -3831,7 +3859,7 @@ static int alloc_irq_index(u16 devid, int count, bool align)
 	if (!iommu)
 		return -ENODEV;
 
-	table = alloc_irq_table(devid);
+	table = alloc_irq_table(devid, pdev);
 	if (!table)
 		return -ENODEV;
 
@@ -4264,7 +4292,7 @@ static int irq_remapping_alloc(struct irq_domain *domain, unsigned int virq,
 		struct irq_remap_table *table;
 		struct amd_iommu *iommu;
 
-		table = alloc_irq_table(devid);
+		table = alloc_irq_table(devid, NULL);
 		if (table) {
 			if (!table->min_index) {
 				/*
@@ -4281,11 +4309,15 @@ static int irq_remapping_alloc(struct irq_domain *domain, unsigned int virq,
 		} else {
 			index = -ENOMEM;
 		}
-	} else {
+	} else if (info->type == X86_IRQ_ALLOC_TYPE_MSI ||
+		   info->type == X86_IRQ_ALLOC_TYPE_MSIX) {
 		bool align = (info->type == X86_IRQ_ALLOC_TYPE_MSI);
 
-		index = alloc_irq_index(devid, nr_irqs, align);
+		index = alloc_irq_index(devid, nr_irqs, align, info->msi_dev);
+	} else {
+		index = alloc_irq_index(devid, nr_irqs, false, NULL);
 	}
+
 	if (index < 0) {
 		pr_warn("Failed to allocate IRTE\n");
 		ret = index;
-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/3] iommu/amd: Support multiple PCI DMA aliases in IRQ Remapping

[Joerg Roedel]

On Tue, Oct 08, 2019 at 04:18:37PM -0600, Logan Gunthorpe wrote:
> -static struct irq_remap_table *alloc_irq_table(u16 devid)
> +static int set_remap_table_entry_alias(struct pci_dev *pdev, u16 alias,
> +				       void *data)
> +{
> +	struct irq_remap_table *table = data;
> +
> +	irq_lookup_table[alias] = table;
> +	set_dte_irq_entry(alias, table);
> +
> +	return 0;
> +}
> +
> +static int iommu_flush_dte_alias(struct pci_dev *pdev, u16 alias, void *data)
> +{
> +	struct amd_iommu *iommu = data;
> +
> +	iommu_flush_dte(iommu, alias);
> +
> +	return 0;
> +}

I think these two functions can be merged into one, saving one
pci_for_each_dma_alias() call below. You can lookup the iommu using the
amd_iommu_rlookup_table[alias] in the first function and issue the flush
there.


Regards,

	Joerg

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/3] iommu/amd: Support multiple PCI DMA aliases in IRQ Remapping

[Logan Gunthorpe]

On 2019-10-15 7:37 a.m., Joerg Roedel wrote:
> On Tue, Oct 08, 2019 at 04:18:37PM -0600, Logan Gunthorpe wrote:
>> -static struct irq_remap_table *alloc_irq_table(u16 devid)
>> +static int set_remap_table_entry_alias(struct pci_dev *pdev, u16 alias,
>> +				       void *data)
>> +{
>> +	struct irq_remap_table *table = data;
>> +
>> +	irq_lookup_table[alias] = table;
>> +	set_dte_irq_entry(alias, table);
>> +
>> +	return 0;
>> +}
>> +
>> +static int iommu_flush_dte_alias(struct pci_dev *pdev, u16 alias, void *data)
>> +{
>> +	struct amd_iommu *iommu = data;
>> +
>> +	iommu_flush_dte(iommu, alias);
>> +
>> +	return 0;
>> +}
> 
> I think these two functions can be merged into one, saving one
> pci_for_each_dma_alias() call below. You can lookup the iommu using the
> amd_iommu_rlookup_table[alias] in the first function and issue the flush
> there.

Makes sense, thanks.

I'll rework this and send a v2 shortly.

Logan
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu