[PATCH v9 2/4] iommu/dma: Add a helper function to reserve HW MSI address regions for IOMMU drivers

-----Original Message-----
From: Robin Murphy [mailto:robin.murphy at arm.com]
Sent: Wednesday, October 18, 2017 1:34 PM
To: Shameerali Kolothum Thodi <redacted>;
Will Deacon [off-list ref]
Cc: lorenzo.pieralisi at arm.com; Gabriele Paoloni
[off-list ref]; marc.zyngier at arm.com; linux-
pci at vger.kernel.org; joro at 8bytes.org; John Garry [off-list ref];
Guohanjun (Hanjun Guo) [off-list ref]; Linuxarm
[off-list ref]; linux-acpi at vger.kernel.org; iommu at lists.linux-
foundation.org; Wangzhou (B) [off-list ref];
sudeep.holla at arm.com; bhelgaas at google.com; linux-arm-
kernel at lists.infradead.org; devel at acpica.org
Subject: Re: [PATCH v9 2/4] iommu/dma: Add a helper function to reserve HW
MSI address regions for IOMMU drivers

On 16/10/17 17:09, Shameerali Kolothum Thodi wrote:

quoted

Hi Robin,

quoted

-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Friday, October 13, 2017 8:24 PM
To: Shameerali Kolothum Thodi <redacted>
Cc: lorenzo.pieralisi at arm.com; marc.zyngier at arm.com;
sudeep.holla at arm.com; robin.murphy at arm.com; joro at 8bytes.org;
bhelgaas at google.com; Gabriele Paoloni [off-list ref];
John Garry [off-list ref]; iommu at lists.linux-foundation.org;
linux-arm-kernel at lists.infradead.org; linux-acpi at vger.kernel.org; linux-
pci at vger.kernel.org; devel at acpica.org; Linuxarm

[off-list ref];

quoted

quoted

Wangzhou (B) [off-list ref]; Guohanjun (Hanjun Guo)
[off-list ref]
Subject: Re: [PATCH v9 2/4] iommu/dma: Add a helper function to reserve

HW

quoted

quoted

MSI address regions for IOMMU drivers

On Fri, Oct 06, 2017 at 03:04:48PM +0100, Shameer Kolothum wrote:

quoted

IOMMU drivers can use this to implement their .get_resv_regions callback
for HW MSI specific reservations(e.g. ARM GICv3 ITS MSI region).

Signed-off-by: Shameer Kolothum

[off-list ref]

quoted

quoted

quoted

---
 drivers/iommu/dma-iommu.c | 20 ++++++++++++++++++++
 include/linux/dma-iommu.h |  7 +++++++
 2 files changed, 27 insertions(+)

I'd like to see Robin's Ack on this, because this is his code and he had
ideas on ways to solve this problem properly.

Please let us know if it is ok to go ahead with ACPI support for now.
It will help our customers to start using pass-through for PCIe.

Thanks,
Shameer

quoted

Will

quoted

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 9d1cebe..bae677e 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c

@@ -19,6 +19,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */

+#include <linux/acpi_iort.h>
 #include <linux/device.h>
 #include <linux/dma-iommu.h>
 #include <linux/gfp.h>

@@ -27,6 +28,7 @@
 #include <linux/iova.h>
 #include <linux/irq.h>
 #include <linux/mm.h>
+#include <linux/of_iommu.h>
 #include <linux/pci.h>
 #include <linux/scatterlist.h>
 #include <linux/vmalloc.h>

@@ -198,6 +200,24 @@ void iommu_dma_get_resv_regions(struct device

*dev, struct list_head *list)

quoted

 }
 EXPORT_SYMBOL(iommu_dma_get_resv_regions);

+/**
+ * iommu_dma_get_msi_resv_regions - Reserved region driver helper
+ * @dev: Device from iommu_get_resv_regions()
+ * @list: Reserved region list from iommu_get_resv_regions()
+ *
+ * IOMMU drivers can use this to implement their .get_resv_regions
+ * callback for HW MSI specific reservations. For now, this only

This doesn't make an awful lot of sense - there's only one reserved
region callback, so iommu-dma shouldn't be offering two separate and
non-overlapping implementations.

quoted

quoted

quoted

+ * covers ITS MSI region reservation using ACPI IORT helper function.
+ */
+int iommu_dma_get_msi_resv_regions(struct device *dev, struct

list_head

quoted

quoted

*list)

quoted

+{
+	if (!is_of_node(dev->iommu_fwspec->iommu_fwnode))
+		return iort_iommu_msi_get_resv_regions(dev, list);

Either this call knows how to do the right thing for any platform and
should be made from iommu_dma_get_reserved_regions() directly, or it's
tightly coupled to the HiSilicon quirk in the SMMUv3 driver and
iommu-dma doesn't need to know - the middle ground presented here is
surely the worst of both worlds.

Right. I think we have discussed this earlier[1] and had a v4 based on invoking
the iort_iommu_its_get_resv_regions() within the iommu_dma_get_resv_regions().
But later as you rightly pointed out, we were not checking for platforms which
requires this quirk inside the iort code and that will break the platforms which are 
happy with MSI translations. Hence moved to the current implementation in v6
after this discussion here[2]
 
And earlier I think in the v3 version we had the function called from smmu driver
directly and the feedback was that it should be abstracted from the driver.

May be it is still possible to move the function call inside the 
iommu_dma_get_resv_regions() and do the smmu model check inside
 the iort helper function and selectively apply the HW MSI reservations.

But I think it is much neater if we can invoke the iort_get_msi_regions() directly
from SMMUv3 based on the model.

Thoughts?

Thanks,
Shameer
1. https://patches.linaro.org/patch/106268/
2. https://www.spinics.net/lists/arm-kernel/msg599162.html