On 19/07/17 11:48, Shameerali Kolothum Thodi wrote:

quoted

-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Friday, July 14, 2017 8:33 PM
To: Shameerali Kolothum Thodi
Cc: lorenzo.pieralisi at arm.com; marc.zyngier at arm.com;
sudeep.holla at arm.com; robin.murphy at arm.com; hanjun.guo at linaro.org;
Gabriele Paoloni; John Garry; Linuxarm; linux-acpi at vger.kernel.org;
iommu at lists.linux-foundation.org; Wangzhou (B); Guohanjun (Hanjun Guo);
linux-arm-kernel at lists.infradead.org; devel at acpica.org
Subject: Re: [PATCH v3 2/2] iommu/arm-smmu-v3:Enable ACPI based
HiSilicon erratum 161010801

[...]

quoted

quoted

quoted

quoted

-	list_add_tail(&region->list, head);
+	if ((smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)) {
+
+		if (!is_of_node(smmu->dev->fwnode))
+			resv = iort_iommu_its_get_resv_regions(dev, head);

How does this work when we're not using ACPI? Shouldn't of vs ACPI
be abstracted from the driver?

At present ARM_SMMU_OPT_RESV_HW_MSI is only set for ACPI and  DT
support for this is a low priority for us at the moment. Is the
suggestion is to have a common function outside the smmu driver for
_iommu_its_get_resv_regions() ? I am not sure what is the best way here.

Right, something like that. The driver shouldn't need to care whether or not
it's using ACPI or DT when setting these options.

Below is what I have in mind for the common function for msi reserve.
But just wondering invoking iort_ functions from iommu code 
is acceptable or not . Could you please take a look and let me know.

At that point, it seems like we might as well just roll it into
iommu_dma_get_resv_regions() directly[1]. It probably makes sense for
any DT equivalent to be described generically, rather than
SMMU-specific, so parsing that would fit into common code as well.

Then in the SMMU drivers we can skip creating the SW_MSI region if
iommu-dma gave us back any real ones (and remove the apparently
unnecessary resv_msi check in VFIO). Or be lazy and just leave it, as it
doesn't seem to do much harm to have both.

Robin.

[1] This is what I hacked up locally on top of patch #1:
----->8-----

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 9d1cebe7f6cb..50292827da49 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>

@@ -174,6 +175,10 @@ void iommu_dma_get_resv_regions(struct device *dev,

struct list_head *list)
 	struct pci_host_bridge *bridge;
 	struct resource_entry *window;

+	if (!is_of_node(dev->iommu_fwspec->iommu_fwnode) &&
+		iort_iommu_its_get_resv_regions(dev, list) < 0)
+		return;
+
 	if (!dev_is_pci(dev))
 		return;