On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi wrote:

Hi Lorenzo,

quoted

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

On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:

quoted

The HiSilicon erratum 161010801 describes the limitation of HiSilicon
platforms Hip06/Hip07 to support the SMMU mappings for MSI

transactions.

quoted

On these platforms GICv3 ITS translator is presented with the deviceID
by extending the MSI payload data to 64 bits to include the deviceID.
Hence, the PCIe controller on this platforms has to differentiate the
MSI payload against other DMA payload and has to modify the MSI

payload.

quoted

This basically makes it difficult for this platforms to have a SMMU
translation for MSI.

This patch implements a ACPI table based quirk to reserve the hw msi
regions in the smmu-v3 driver which means these address regions will
not be translated and will be excluded from iova allocations.

The HW ITS address region associated with the dev is retrieved using a
new helper function added in the IORT code.

Remove or rephrase last paragraph, it reads as if you are adding an IORT
helper function in this patch but you actually aren't.

Thanks for going through this patch series. I will remove this in next version.

quoted

quoted

Signed-off-by: shameer <redacted>
---
 drivers/iommu/arm-smmu-v3.c | 49
++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-

v3.c

quoted

index abe4b88..3767526 100644

--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c

@@ -597,6 +597,7 @@ struct arm_smmu_device {
 	u32				features;

 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
 	u32				options;

 	struct arm_smmu_cmdq		cmdq;

@@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct

arm_smmu_device *smmu, u32 sid)

 static struct iommu_ops arm_smmu_ops;

+#ifdef CONFIG_ACPI
+static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
+device *dev) {
+	struct iommu_resv_region *region;
+	struct	irq_domain *irq_dom;
+	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+	u64	base;

phys_addr_t

Ok.

quoted

quoted

+	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
+	if (irq_dom) {
+		int	ret;
+		u32	rid;
+
+		rid = pci_msi_domain_get_msi_rid(irq_dom,

to_pci_dev(dev));

quoted

+		ret = iort_dev_find_its_base(dev, rid, 0, &base);

Well, here we use ITS id 0 which is fine as long as code in IORT uses the same
policy for getting the irq_domain (ie we want to reserve the ITS address
space that is actually used by the device to send IRQs not a a different one) it
is just a heads-up because I find this confusing.

Ok. Just to make it clear, 0 is the index into the ITS identifier list.
I noted that iort_get_device_domain() uses index 0 while retrieving the ITS identifier.
May be use the same approach here as well? ie, remove the index from function call?

I am not sure, how we can get the index info  though theoretically It is possible for
the ITS group node having multiple ITSs.

Yes, it would be ideal to avoid the look-up through the ITS index and
just reuse the ITS node associated with the MSI domain because I do not
want this quirk to force the ITS domain allocation policy (what I mean
I do not want to be tied to index 0 if for any reason we change
the allocation in IORT for normal ITS<->device mapping).

I will have a further look to see if we can improve the code to
this extent.

quoted

quoted

+		if (!ret) {
+			dev_info(dev, "SMMUv3:HW MSI resv addr

0x%pa\n", &base);

quoted

+			region = iommu_alloc_resv_region(base, SZ_128K,
+							 prot,

IOMMU_RESV_MSI);

quoted

+			return region;
+		}
+	}
+
+	return NULL;
+}
+#else
+static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
+device *dev) {
+	return NULL;
+}
+#endif
+
 static int arm_smmu_add_device(struct device *dev)  {
 	int i, ret;

@@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device

*dev, struct of_phandle_args *args)  static void

arm_smmu_get_resv_regions(struct device *dev,

quoted

 				      struct list_head *head)
 {
-	struct iommu_resv_region *region;
+	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+	struct iommu_resv_region *region = NULL;
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+	struct arm_smmu_device *smmu;
+
+	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);

-	region = iommu_alloc_resv_region(MSI_IOVA_BASE,

MSI_IOVA_LENGTH,

quoted

-					 prot, IOMMU_RESV_SW_MSI);
+	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)

&&

quoted

+		      dev_is_pci(dev))
+		region = arm_smmu_acpi_alloc_hw_msi(dev);

Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?

It is just that PCIe devices won't be functional on this platforms as the endpoint will 
be configured with ITS IOVA address. May be I should add some dev_warn() here.

Well yes and also I am not sure that if arm_smmu_acpi_alloc_hw_msi()
fails you should allocate the SW_MSI region I am not sure I understand
the logic, so you should add a warning and just return on failure right ?

Lorenzo