[PATCH v6 0/3] iommu/smmu-v3: Workaround for hisilicon 161010801 erratum(reserve HW MSI)

[PATCH v6 0/3] iommu/smmu-v3: Workaround for hisilicon 161010801 erratum(reserve HW MSI)

[Shameer Kolothum]

On certain HiSilicon platforms (Hip06/Hip07) the GIC ITS and
PCIe RC deviates from the standard implementation and this breaks
PCIe MSI functionality when SMMU is enabled.

The HiSilicon erratum 161010801 describes this limitation of certain
HiSilicon platforms to support the SMMU mappings for MSI transactions.
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.
This basically makes it difficult for this platforms to have a SMMU
translation for MSI.

To implement this quirk, the following changes are incorporated:

1. Added a generic helper function to IORT code to retrieve and
   reserve the HW ITS address regions.
2. Added quirk to SMMUv3 to reserve HW ITS address regions based
    on IORT SMMUv3 model with the help of a generic iommu helper
    function.

Thanks,
Shameer

Changelog:

v5 --> v6
Addressed comments from Robin and Lorenzo:
-No change to patch#1 .
-Reverted v5 patch#2 as this might break the platforms where this quirk
is not applicable. Provided a generic function in iommu code and
added back the quirk implementation in SMMU v3 driver(patch#3)
 
v4 --> v5
Addressed comments from Robin and Lorenzo:
-Added a comment to make it clear that, for now, only straightforward 
 HW topologies are handled while reserving ITS regions(patch #1).

v3 --> v4
Rebased on 4.13-rc1.
Addressed comments from Robin, Will and Lorenzo:
-As suggested by Robin, moved the ITS msi reservation into
 iommu_dma_get_resv_regions().
-Added its_count != resv region failure case(patch #1).

v2 --> v3
Addressed comments from Lorenzo and Robin:
-Removed dev_is_pci() check in smmuV3 driver.
-Don't treat device not having an ITS mapping as an error in
 iort helper function.

v1 --> v2
-patch 2/2: Invoke iort helper fn based on fwnode type(acpi).

RFCv2 -->PATCH
-Incorporated Lorenzo's review comments.

RFC v1 --> RFC v2
Based on Robin's review comments,
-Removed  the generic erratum framework.
-Using IORT/MADT tables to retrieve the ITS base addr instead
 of vendor specific CSRT table.

Shameer Kolothum (3):
  ACPI/IORT: Add ITS address regions reservation helper
  iommu/dma: Add a helper function to reserve HW MSI address regions for
    IOMMU drivers
  iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

 drivers/acpi/arm64/iort.c        | 95 ++++++++++++++++++++++++++++++++++++++--
 drivers/iommu/arm-smmu-v3.c      | 27 +++++++++---
 drivers/iommu/dma-iommu.c        | 19 ++++++++
 drivers/irqchip/irq-gic-v3-its.c |  3 +-
 include/linux/acpi_iort.h        |  7 ++-
 include/linux/dma-iommu.h        |  7 +++
 6 files changed, 148 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH v6 0/3] iommu/smmu-v3: Workaround for hisilicon 161010801 erratum(reserve HW MSI)

[Shameer Kolothum]

On certain HiSilicon platforms (Hip06/Hip07) the GIC ITS and
PCIe RC deviates from the standard implementation and this breaks
PCIe MSI functionality when SMMU is enabled.

The HiSilicon erratum 161010801 describes this limitation of certain
HiSilicon platforms to support the SMMU mappings for MSI transactions.
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.
This basically makes it difficult for this platforms to have a SMMU
translation for MSI.

To implement this quirk, the following changes are incorporated:

1. Added a generic helper function to IORT code to retrieve and
   reserve the HW ITS address regions.
2. Added quirk to SMMUv3 to reserve HW ITS address regions based
    on IORT SMMUv3 model with the help of a generic iommu helper
    function.

Thanks,
Shameer

Changelog:

v5 --> v6
Addressed comments from Robin and Lorenzo:
-No change to patch#1 .
-Reverted v5 patch#2 as this might break the platforms where this quirk
is not applicable. Provided a generic function in iommu code and
added back the quirk implementation in SMMU v3 driver(patch#3)
 
v4 --> v5
Addressed comments from Robin and Lorenzo:
-Added a comment to make it clear that, for now, only straightforward 
 HW topologies are handled while reserving ITS regions(patch #1).

v3 --> v4
Rebased on 4.13-rc1.
Addressed comments from Robin, Will and Lorenzo:
-As suggested by Robin, moved the ITS msi reservation into
 iommu_dma_get_resv_regions().
-Added its_count != resv region failure case(patch #1).

v2 --> v3
Addressed comments from Lorenzo and Robin:
-Removed dev_is_pci() check in smmuV3 driver.
-Don't treat device not having an ITS mapping as an error in
 iort helper function.

v1 --> v2
-patch 2/2: Invoke iort helper fn based on fwnode type(acpi).

RFCv2 -->PATCH
-Incorporated Lorenzo's review comments.

RFC v1 --> RFC v2
Based on Robin's review comments,
-Removed  the generic erratum framework.
-Using IORT/MADT tables to retrieve the ITS base addr instead
 of vendor specific CSRT table.

Shameer Kolothum (3):
  ACPI/IORT: Add ITS address regions reservation helper
  iommu/dma: Add a helper function to reserve HW MSI address regions for
    IOMMU drivers
  iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

 drivers/acpi/arm64/iort.c        | 95 ++++++++++++++++++++++++++++++++++++++--
 drivers/iommu/arm-smmu-v3.c      | 27 +++++++++---
 drivers/iommu/dma-iommu.c        | 19 ++++++++
 drivers/irqchip/irq-gic-v3-its.c |  3 +-
 include/linux/acpi_iort.h        |  7 ++-
 include/linux/dma-iommu.h        |  7 +++
 6 files changed, 148 insertions(+), 10 deletions(-)

-- 
1.9.1

[Devel] [PATCH v6 0/3] iommu/smmu-v3: Workaround for hisilicon 161010801 erratum(reserve HW MSI)

[Shameer Kolothum]

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

On certain HiSilicon platforms (Hip06/Hip07) the GIC ITS and
PCIe RC deviates from the standard implementation and this breaks
PCIe MSI functionality when SMMU is enabled.

The HiSilicon erratum 161010801 describes this limitation of certain
HiSilicon platforms to support the SMMU mappings for MSI transactions.
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.
This basically makes it difficult for this platforms to have a SMMU
translation for MSI.

To implement this quirk, the following changes are incorporated:

1. Added a generic helper function to IORT code to retrieve and
   reserve the HW ITS address regions.
2. Added quirk to SMMUv3 to reserve HW ITS address regions based
    on IORT SMMUv3 model with the help of a generic iommu helper
    function.

Thanks,
Shameer

Changelog:

v5 --> v6
Addressed comments from Robin and Lorenzo:
-No change to patch#1 .
-Reverted v5 patch#2 as this might break the platforms where this quirk
is not applicable. Provided a generic function in iommu code and
added back the quirk implementation in SMMU v3 driver(patch#3)
 
v4 --> v5
Addressed comments from Robin and Lorenzo:
-Added a comment to make it clear that, for now, only straightforward 
 HW topologies are handled while reserving ITS regions(patch #1).

v3 --> v4
Rebased on 4.13-rc1.
Addressed comments from Robin, Will and Lorenzo:
-As suggested by Robin, moved the ITS msi reservation into
 iommu_dma_get_resv_regions().
-Added its_count != resv region failure case(patch #1).

v2 --> v3
Addressed comments from Lorenzo and Robin:
-Removed dev_is_pci() check in smmuV3 driver.
-Don't treat device not having an ITS mapping as an error in
 iort helper function.

v1 --> v2
-patch 2/2: Invoke iort helper fn based on fwnode type(acpi).

RFCv2 -->PATCH
-Incorporated Lorenzo's review comments.

RFC v1 --> RFC v2
Based on Robin's review comments,
-Removed  the generic erratum framework.
-Using IORT/MADT tables to retrieve the ITS base addr instead
 of vendor specific CSRT table.

Shameer Kolothum (3):
  ACPI/IORT: Add ITS address regions reservation helper
  iommu/dma: Add a helper function to reserve HW MSI address regions for
    IOMMU drivers
  iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

 drivers/acpi/arm64/iort.c        | 95 ++++++++++++++++++++++++++++++++++++++--
 drivers/iommu/arm-smmu-v3.c      | 27 +++++++++---
 drivers/iommu/dma-iommu.c        | 19 ++++++++
 drivers/irqchip/irq-gic-v3-its.c |  3 +-
 include/linux/acpi_iort.h        |  7 ++-
 include/linux/dma-iommu.h        |  7 +++
 6 files changed, 148 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH v6 1/3] ACPI/IORT: Add ITS address regions reservation helper

[Shameer Kolothum]

On some platforms ITS address regions have to be excluded from normal
IOVA allocation in that they are detected and decoded in a HW specific
way by system components and so they cannot be considered normal IOVA
address space.

Add an helper function that retrieves ITS address regions through IORT
device <-> ITS mappings and reserves it so that these regions will not
be translated by IOMMU and will be excluded from IOVA allocations.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
[lorenzo.pieralisi@arm.com: updated commit log/added comments]
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/acpi/arm64/iort.c        | 95 ++++++++++++++++++++++++++++++++++++++--
 drivers/irqchip/irq-gic-v3-its.c |  3 +-
 include/linux/acpi_iort.h        |  7 ++-
 3 files changed, 100 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index a3215ee..86b5a51 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -39,6 +39,7 @@
 struct iort_its_msi_chip {
 	struct list_head	list;
 	struct fwnode_handle	*fw_node;
+	phys_addr_t		base_addr;
 	u32			translation_id;
 };
 
@@ -136,14 +137,16 @@ typedef acpi_status (*iort_find_node_callback)
 static DEFINE_SPINLOCK(iort_msi_chip_lock);
 
 /**
- * iort_register_domain_token() - register domain token and related ITS ID
- * to the list from where we can get it back later on.
+ * iort_register_domain_token() - register domain token along with related
+ * ITS ID and base address to the list from where we can get it back later on.
  * @trans_id: ITS ID.
+ * @base: ITS base address.
  * @fw_node: Domain token.
  *
  * Returns: 0 on success, -ENOMEM if no memory when allocating list element
  */
-int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)
+int iort_register_domain_token(int trans_id, phys_addr_t base,
+			       struct fwnode_handle *fw_node)
 {
 	struct iort_its_msi_chip *its_msi_chip;
 
@@ -153,6 +156,7 @@ int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)
 
 	its_msi_chip->fw_node = fw_node;
 	its_msi_chip->translation_id = trans_id;
+	its_msi_chip->base_addr = base;
 
 	spin_lock(&iort_msi_chip_lock);
 	list_add(&its_msi_chip->list, &iort_msi_chip_list);
@@ -481,6 +485,24 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
 	return -ENODEV;
 }
 
+static int __maybe_unused iort_find_its_base(u32 its_id, phys_addr_t *base)
+{
+	struct iort_its_msi_chip *its_msi_chip;
+	bool match = false;
+
+	spin_lock(&iort_msi_chip_lock);
+	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {
+		if (its_msi_chip->translation_id == its_id) {
+			*base = its_msi_chip->base_addr;
+			match = true;
+			break;
+		}
+	}
+	spin_unlock(&iort_msi_chip_lock);
+
+	return match ? 0 : -ENODEV;
+}
+
 /**
  * iort_dev_find_its_id() - Find the ITS identifier for a device
  * @dev: The device.
@@ -639,6 +661,71 @@ int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)
 
 	return err;
 }
+
+/**
+ * iort_iommu_its_get_resv_regions - Reserved region driver helper
+ * @dev: Device from iommu_get_resv_regions()
+ * @list: Reserved region list from iommu_get_resv_regions()
+ *
+ * Returns: Number of reserved regions on success(0 if no associated ITS),
+ *          appropriate error value otherwise.
+ */
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)
+{
+	struct acpi_iort_its_group *its;
+	struct acpi_iort_node *node, *its_node = NULL;
+	int i, resv = 0;
+
+	node = iort_find_dev_node(dev);
+	if (!node)
+		return -ENODEV;
+
+	/*
+	 * Current logic to reserve ITS regions relies on HW topologies
+	 * where a given PCI or named component maps its IDs to only one
+	 * ITS group; if a PCI or named component can map its IDs to
+	 * different ITS groups through IORT mappings this function has
+	 * to be reworked to ensure we reserve regions for all ITS groups
+	 * a given PCI or named component may map IDs to.
+	 */
+	if (dev_is_pci(dev)) {
+		u32 rid;
+
+		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, &rid);
+		its_node = iort_node_map_id(node, rid, NULL, IORT_MSI_TYPE);
+	} else {
+		for (i = 0; i < node->mapping_count; i++) {
+			its_node = iort_node_map_platform_id(node, NULL,
+							 IORT_MSI_TYPE, i);
+			if (its_node)
+				break;
+		}
+	}
+
+	if (!its_node)
+		return 0;
+
+	/* Move to ITS specific data */
+	its = (struct acpi_iort_its_group *)its_node->node_data;
+
+	for (i = 0; i < its->its_count; i++) {
+		phys_addr_t base;
+
+		if (!iort_find_its_base(its->identifiers[i], &base)) {
+			int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+			struct iommu_resv_region *region;
+
+			region = iommu_alloc_resv_region(base, SZ_128K, prot,
+							 IOMMU_RESV_MSI);
+			if (region) {
+				list_add_tail(&region->list, head);
+				resv++;
+			}
+		}
+	}
+
+	return (resv == its->its_count) ? resv : -ENODEV;
+}
 #else
 static inline
 const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)
@@ -646,6 +733,8 @@ const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)
 static inline
 int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)
 { return 0; }
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)
+{ return -ENODEV; }
 #endif
 
 static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 6893287..77322b3 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1928,7 +1928,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
 		return -ENOMEM;
 	}
 
-	err = iort_register_domain_token(its_entry->translation_id, dom_handle);
+	err = iort_register_domain_token(its_entry->translation_id, res.start,
+					 dom_handle);
 	if (err) {
 		pr_err("ITS@%pa: Unable to register GICv3 ITS domain token (ITS ID %d) to IORT\n",
 		       &res.start, its_entry->translation_id);
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 8379d40..d7ed49c 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -26,7 +26,8 @@
 #define IORT_IRQ_MASK(irq)		(irq & 0xffffffffULL)
 #define IORT_IRQ_TRIGGER_MASK(irq)	((irq >> 32) & 0xffffffffULL)
 
-int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);
+int iort_register_domain_token(int trans_id, phys_addr_t base,
+			       struct fwnode_handle *fw_node);
 void iort_deregister_domain_token(int trans_id);
 struct fwnode_handle *iort_find_domain_token(int trans_id);
 #ifdef CONFIG_ACPI_IORT
@@ -38,6 +39,7 @@
 /* IOMMU interface */
 void iort_set_dma_mask(struct device *dev);
 const struct iommu_ops *iort_iommu_configure(struct device *dev);
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head);
 #else
 static inline void acpi_iort_init(void) { }
 static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
@@ -51,6 +53,9 @@ static inline void iort_set_dma_mask(struct device *dev) { }
 static inline
 const struct iommu_ops *iort_iommu_configure(struct device *dev)
 { return NULL; }
+static inline
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)
+{ return -ENODEV; }
 #endif
 
 #endif /* __ACPI_IORT_H__ */
-- 
1.9.1

[PATCH v6 1/3] ACPI/IORT: Add ITS address regions reservation helper

[Shameer Kolothum]

On some platforms ITS address regions have to be excluded from normal
IOVA allocation in that they are detected and decoded in a HW specific
way by system components and so they cannot be considered normal IOVA
address space.

Add an helper function that retrieves ITS address regions through IORT
device <-> ITS mappings and reserves it so that these regions will not
be translated by IOMMU and will be excluded from IOVA allocations.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
[lorenzo.pieralisi at arm.com: updated commit log/added comments]
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/acpi/arm64/iort.c        | 95 ++++++++++++++++++++++++++++++++++++++--
 drivers/irqchip/irq-gic-v3-its.c |  3 +-
 include/linux/acpi_iort.h        |  7 ++-
 3 files changed, 100 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index a3215ee..86b5a51 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -39,6 +39,7 @@
 struct iort_its_msi_chip {
 	struct list_head	list;
 	struct fwnode_handle	*fw_node;
+	phys_addr_t		base_addr;
 	u32			translation_id;
 };
 
@@ -136,14 +137,16 @@ typedef acpi_status (*iort_find_node_callback)
 static DEFINE_SPINLOCK(iort_msi_chip_lock);
 
 /**
- * iort_register_domain_token() - register domain token and related ITS ID
- * to the list from where we can get it back later on.
+ * iort_register_domain_token() - register domain token along with related
+ * ITS ID and base address to the list from where we can get it back later on.
  * @trans_id: ITS ID.
+ * @base: ITS base address.
  * @fw_node: Domain token.
  *
  * Returns: 0 on success, -ENOMEM if no memory when allocating list element
  */
-int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)
+int iort_register_domain_token(int trans_id, phys_addr_t base,
+			       struct fwnode_handle *fw_node)
 {
 	struct iort_its_msi_chip *its_msi_chip;
 
@@ -153,6 +156,7 @@ int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)
 
 	its_msi_chip->fw_node = fw_node;
 	its_msi_chip->translation_id = trans_id;
+	its_msi_chip->base_addr = base;
 
 	spin_lock(&iort_msi_chip_lock);
 	list_add(&its_msi_chip->list, &iort_msi_chip_list);
@@ -481,6 +485,24 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
 	return -ENODEV;
 }
 
+static int __maybe_unused iort_find_its_base(u32 its_id, phys_addr_t *base)
+{
+	struct iort_its_msi_chip *its_msi_chip;
+	bool match = false;
+
+	spin_lock(&iort_msi_chip_lock);
+	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {
+		if (its_msi_chip->translation_id == its_id) {
+			*base = its_msi_chip->base_addr;
+			match = true;
+			break;
+		}
+	}
+	spin_unlock(&iort_msi_chip_lock);
+
+	return match ? 0 : -ENODEV;
+}
+
 /**
  * iort_dev_find_its_id() - Find the ITS identifier for a device
  * @dev: The device.
@@ -639,6 +661,71 @@ int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)
 
 	return err;
 }
+
+/**
+ * iort_iommu_its_get_resv_regions - Reserved region driver helper
+ * @dev: Device from iommu_get_resv_regions()
+ * @list: Reserved region list from iommu_get_resv_regions()
+ *
+ * Returns: Number of reserved regions on success(0 if no associated ITS),
+ *          appropriate error value otherwise.
+ */
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)
+{
+	struct acpi_iort_its_group *its;
+	struct acpi_iort_node *node, *its_node = NULL;
+	int i, resv = 0;
+
+	node = iort_find_dev_node(dev);
+	if (!node)
+		return -ENODEV;
+
+	/*
+	 * Current logic to reserve ITS regions relies on HW topologies
+	 * where a given PCI or named component maps its IDs to only one
+	 * ITS group; if a PCI or named component can map its IDs to
+	 * different ITS groups through IORT mappings this function has
+	 * to be reworked to ensure we reserve regions for all ITS groups
+	 * a given PCI or named component may map IDs to.
+	 */
+	if (dev_is_pci(dev)) {
+		u32 rid;
+
+		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, &rid);
+		its_node = iort_node_map_id(node, rid, NULL, IORT_MSI_TYPE);
+	} else {
+		for (i = 0; i < node->mapping_count; i++) {
+			its_node = iort_node_map_platform_id(node, NULL,
+							 IORT_MSI_TYPE, i);
+			if (its_node)
+				break;
+		}
+	}
+
+	if (!its_node)
+		return 0;
+
+	/* Move to ITS specific data */
+	its = (struct acpi_iort_its_group *)its_node->node_data;
+
+	for (i = 0; i < its->its_count; i++) {
+		phys_addr_t base;
+
+		if (!iort_find_its_base(its->identifiers[i], &base)) {
+			int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+			struct iommu_resv_region *region;
+
+			region = iommu_alloc_resv_region(base, SZ_128K, prot,
+							 IOMMU_RESV_MSI);
+			if (region) {
+				list_add_tail(&region->list, head);
+				resv++;
+			}
+		}
+	}
+
+	return (resv == its->its_count) ? resv : -ENODEV;
+}
 #else
 static inline
 const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)
@@ -646,6 +733,8 @@ const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)
 static inline
 int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)
 { return 0; }
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)
+{ return -ENODEV; }
 #endif
 
 static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 6893287..77322b3 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1928,7 +1928,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
 		return -ENOMEM;
 	}
 
-	err = iort_register_domain_token(its_entry->translation_id, dom_handle);
+	err = iort_register_domain_token(its_entry->translation_id, res.start,
+					 dom_handle);
 	if (err) {
 		pr_err("ITS@%pa: Unable to register GICv3 ITS domain token (ITS ID %d) to IORT\n",
 		       &res.start, its_entry->translation_id);
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 8379d40..d7ed49c 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -26,7 +26,8 @@
 #define IORT_IRQ_MASK(irq)		(irq & 0xffffffffULL)
 #define IORT_IRQ_TRIGGER_MASK(irq)	((irq >> 32) & 0xffffffffULL)
 
-int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);
+int iort_register_domain_token(int trans_id, phys_addr_t base,
+			       struct fwnode_handle *fw_node);
 void iort_deregister_domain_token(int trans_id);
 struct fwnode_handle *iort_find_domain_token(int trans_id);
 #ifdef CONFIG_ACPI_IORT
@@ -38,6 +39,7 @@
 /* IOMMU interface */
 void iort_set_dma_mask(struct device *dev);
 const struct iommu_ops *iort_iommu_configure(struct device *dev);
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head);
 #else
 static inline void acpi_iort_init(void) { }
 static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
@@ -51,6 +53,9 @@ static inline void iort_set_dma_mask(struct device *dev) { }
 static inline
 const struct iommu_ops *iort_iommu_configure(struct device *dev)
 { return NULL; }
+static inline
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)
+{ return -ENODEV; }
 #endif
 
 #endif /* __ACPI_IORT_H__ */
-- 
1.9.1

[Devel] [PATCH v6 1/3] ACPI/IORT: Add ITS address regions reservation helper

[Shameer Kolothum]

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

On some platforms ITS address regions have to be excluded from normal
IOVA allocation in that they are detected and decoded in a HW specific
way by system components and so they cannot be considered normal IOVA
address space.

Add an helper function that retrieves ITS address regions through IORT
device <-> ITS mappings and reserves it so that these regions will not
be translated by IOMMU and will be excluded from IOVA allocations.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com>
[lorenzo.pieralisi(a)arm.com: updated commit log/added comments]
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi(a)arm.com>
---
 drivers/acpi/arm64/iort.c        | 95 ++++++++++++++++++++++++++++++++++++++--
 drivers/irqchip/irq-gic-v3-its.c |  3 +-
 include/linux/acpi_iort.h        |  7 ++-
 3 files changed, 100 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index a3215ee..86b5a51 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -39,6 +39,7 @@
 struct iort_its_msi_chip {
 	struct list_head	list;
 	struct fwnode_handle	*fw_node;
+	phys_addr_t		base_addr;
 	u32			translation_id;
 };
 
@@ -136,14 +137,16 @@ typedef acpi_status (*iort_find_node_callback)
 static DEFINE_SPINLOCK(iort_msi_chip_lock);
 
 /**
- * iort_register_domain_token() - register domain token and related ITS ID
- * to the list from where we can get it back later on.
+ * iort_register_domain_token() - register domain token along with related
+ * ITS ID and base address to the list from where we can get it back later on.
  * @trans_id: ITS ID.
+ * @base: ITS base address.
  * @fw_node: Domain token.
  *
  * Returns: 0 on success, -ENOMEM if no memory when allocating list element
  */
-int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)
+int iort_register_domain_token(int trans_id, phys_addr_t base,
+			       struct fwnode_handle *fw_node)
 {
 	struct iort_its_msi_chip *its_msi_chip;
 
@@ -153,6 +156,7 @@ int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)
 
 	its_msi_chip->fw_node = fw_node;
 	its_msi_chip->translation_id = trans_id;
+	its_msi_chip->base_addr = base;
 
 	spin_lock(&iort_msi_chip_lock);
 	list_add(&its_msi_chip->list, &iort_msi_chip_list);
@@ -481,6 +485,24 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
 	return -ENODEV;
 }
 
+static int __maybe_unused iort_find_its_base(u32 its_id, phys_addr_t *base)
+{
+	struct iort_its_msi_chip *its_msi_chip;
+	bool match = false;
+
+	spin_lock(&iort_msi_chip_lock);
+	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {
+		if (its_msi_chip->translation_id == its_id) {
+			*base = its_msi_chip->base_addr;
+			match = true;
+			break;
+		}
+	}
+	spin_unlock(&iort_msi_chip_lock);
+
+	return match ? 0 : -ENODEV;
+}
+
 /**
  * iort_dev_find_its_id() - Find the ITS identifier for a device
  * @dev: The device.
@@ -639,6 +661,71 @@ int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)
 
 	return err;
 }
+
+/**
+ * iort_iommu_its_get_resv_regions - Reserved region driver helper
+ * @dev: Device from iommu_get_resv_regions()
+ * @list: Reserved region list from iommu_get_resv_regions()
+ *
+ * Returns: Number of reserved regions on success(0 if no associated ITS),
+ *          appropriate error value otherwise.
+ */
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)
+{
+	struct acpi_iort_its_group *its;
+	struct acpi_iort_node *node, *its_node = NULL;
+	int i, resv = 0;
+
+	node = iort_find_dev_node(dev);
+	if (!node)
+		return -ENODEV;
+
+	/*
+	 * Current logic to reserve ITS regions relies on HW topologies
+	 * where a given PCI or named component maps its IDs to only one
+	 * ITS group; if a PCI or named component can map its IDs to
+	 * different ITS groups through IORT mappings this function has
+	 * to be reworked to ensure we reserve regions for all ITS groups
+	 * a given PCI or named component may map IDs to.
+	 */
+	if (dev_is_pci(dev)) {
+		u32 rid;
+
+		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, &rid);
+		its_node = iort_node_map_id(node, rid, NULL, IORT_MSI_TYPE);
+	} else {
+		for (i = 0; i < node->mapping_count; i++) {
+			its_node = iort_node_map_platform_id(node, NULL,
+							 IORT_MSI_TYPE, i);
+			if (its_node)
+				break;
+		}
+	}
+
+	if (!its_node)
+		return 0;
+
+	/* Move to ITS specific data */
+	its = (struct acpi_iort_its_group *)its_node->node_data;
+
+	for (i = 0; i < its->its_count; i++) {
+		phys_addr_t base;
+
+		if (!iort_find_its_base(its->identifiers[i], &base)) {
+			int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+			struct iommu_resv_region *region;
+
+			region = iommu_alloc_resv_region(base, SZ_128K, prot,
+							 IOMMU_RESV_MSI);
+			if (region) {
+				list_add_tail(&region->list, head);
+				resv++;
+			}
+		}
+	}
+
+	return (resv == its->its_count) ? resv : -ENODEV;
+}
 #else
 static inline
 const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)
@@ -646,6 +733,8 @@ const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)
 static inline
 int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)
 { return 0; }
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)
+{ return -ENODEV; }
 #endif
 
 static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 6893287..77322b3 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1928,7 +1928,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
 		return -ENOMEM;
 	}
 
-	err = iort_register_domain_token(its_entry->translation_id, dom_handle);
+	err = iort_register_domain_token(its_entry->translation_id, res.start,
+					 dom_handle);
 	if (err) {
 		pr_err("ITS@%pa: Unable to register GICv3 ITS domain token (ITS ID %d) to IORT\n",
 		       &res.start, its_entry->translation_id);
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 8379d40..d7ed49c 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -26,7 +26,8 @@
 #define IORT_IRQ_MASK(irq)		(irq & 0xffffffffULL)
 #define IORT_IRQ_TRIGGER_MASK(irq)	((irq >> 32) & 0xffffffffULL)
 
-int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);
+int iort_register_domain_token(int trans_id, phys_addr_t base,
+			       struct fwnode_handle *fw_node);
 void iort_deregister_domain_token(int trans_id);
 struct fwnode_handle *iort_find_domain_token(int trans_id);
 #ifdef CONFIG_ACPI_IORT
@@ -38,6 +39,7 @@
 /* IOMMU interface */
 void iort_set_dma_mask(struct device *dev);
 const struct iommu_ops *iort_iommu_configure(struct device *dev);
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head);
 #else
 static inline void acpi_iort_init(void) { }
 static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
@@ -51,6 +53,9 @@ static inline void iort_set_dma_mask(struct device *dev) { }
 static inline
 const struct iommu_ops *iort_iommu_configure(struct device *dev)
 { return NULL; }
+static inline
+int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)
+{ return -ENODEV; }
 #endif
 
 #endif /* __ACPI_IORT_H__ */
-- 
1.9.1

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

[Shameer Kolothum]

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 <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/dma-iommu.c | 19 +++++++++++++++++++
 include/linux/dma-iommu.h |  7 +++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 9d1cebe..952ecdd 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>
@@ -198,6 +199,24 @@ void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)
 }
 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
+ * covers ITS MSI region reservation using ACPI IORT helper function.
+ */
+int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head *list)
+{
+	if (!is_of_node(dev->iommu_fwspec->iommu_fwnode))
+		return iort_iommu_its_get_resv_regions(dev, list);
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL(iommu_dma_get_msi_resv_regions);
+
 static int cookie_init_hw_msi_region(struct iommu_dma_cookie *cookie,
 		phys_addr_t start, phys_addr_t end)
 {
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 92f2083..6062ef0 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -74,6 +74,8 @@ void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
 void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
 void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
 
+int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head *list);
+
 #else
 
 struct iommu_domain;
@@ -107,6 +109,11 @@ static inline void iommu_dma_get_resv_regions(struct device *dev, struct list_he
 {
 }
 
+static inline int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head *list)
+{
+	return -ENODEV;
+}
+
 #endif	/* CONFIG_IOMMU_DMA */
 #endif	/* __KERNEL__ */
 #endif	/* __DMA_IOMMU_H */
-- 
1.9.1

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

[Shameer Kolothum]

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 <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/dma-iommu.c | 19 +++++++++++++++++++
 include/linux/dma-iommu.h |  7 +++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 9d1cebe..952ecdd 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>
@@ -198,6 +199,24 @@ void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)
 }
 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
+ * covers ITS MSI region reservation using ACPI IORT helper function.
+ */
+int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head *list)
+{
+	if (!is_of_node(dev->iommu_fwspec->iommu_fwnode))
+		return iort_iommu_its_get_resv_regions(dev, list);
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL(iommu_dma_get_msi_resv_regions);
+
 static int cookie_init_hw_msi_region(struct iommu_dma_cookie *cookie,
 		phys_addr_t start, phys_addr_t end)
 {
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 92f2083..6062ef0 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -74,6 +74,8 @@ void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
 void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
 void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
 
+int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head *list);
+
 #else
 
 struct iommu_domain;
@@ -107,6 +109,11 @@ static inline void iommu_dma_get_resv_regions(struct device *dev, struct list_he
 {
 }
 
+static inline int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head *list)
+{
+	return -ENODEV;
+}
+
 #endif	/* CONFIG_IOMMU_DMA */
 #endif	/* __KERNEL__ */
 #endif	/* __DMA_IOMMU_H */
-- 
1.9.1

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

[Shameer Kolothum]

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

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 <shameerali.kolothum.thodi(a)huawei.com>
---
 drivers/iommu/dma-iommu.c | 19 +++++++++++++++++++
 include/linux/dma-iommu.h |  7 +++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 9d1cebe..952ecdd 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>
@@ -198,6 +199,24 @@ void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)
 }
 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
+ * covers ITS MSI region reservation using ACPI IORT helper function.
+ */
+int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head *list)
+{
+	if (!is_of_node(dev->iommu_fwspec->iommu_fwnode))
+		return iort_iommu_its_get_resv_regions(dev, list);
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL(iommu_dma_get_msi_resv_regions);
+
 static int cookie_init_hw_msi_region(struct iommu_dma_cookie *cookie,
 		phys_addr_t start, phys_addr_t end)
 {
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 92f2083..6062ef0 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -74,6 +74,8 @@ void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
 void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
 void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
 
+int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head *list);
+
 #else
 
 struct iommu_domain;
@@ -107,6 +109,11 @@ static inline void iommu_dma_get_resv_regions(struct device *dev, struct list_he
 {
 }
 
+static inline int iommu_dma_get_msi_resv_regions(struct device *dev, struct list_head *list)
+{
+	return -ENODEV;
+}
+
 #endif	/* CONFIG_IOMMU_DMA */
 #endif	/* __KERNEL__ */
 #endif	/* __DMA_IOMMU_H */
-- 
1.9.1

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameer Kolothum]

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

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

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 568c400..6f21dd7 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -608,6 +608,7 @@ struct arm_smmu_device {
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
 #define ARM_SMMU_OPT_PAGE0_REGS_ONLY	(1 << 1)
+#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 2)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;
@@ -1934,14 +1935,29 @@ static void arm_smmu_get_resv_regions(struct device *dev,
 				      struct list_head *head)
 {
 	struct iommu_resv_region *region;
+	struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv;
+	struct arm_smmu_device *smmu = master->smmu;
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+	int resv = 0;
 
-	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
-					 prot, IOMMU_RESV_SW_MSI);
-	if (!region)
-		return;
+	if ((smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)) {
 
-	list_add_tail(&region->list, head);
+		resv = iommu_dma_get_msi_resv_regions(dev, head);
+
+		if (resv < 0) {
+			dev_warn(dev, "HW MSI region resv failed: %d\n", resv);
+			return;
+		}
+	}
+
+	if (!resv) {
+		region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
+						 prot, IOMMU_RESV_SW_MSI);
+		if (!region)
+			return;
+
+		list_add_tail(&region->list, head);
+	}
 
 	iommu_dma_get_resv_regions(dev, head);
 }
@@ -2667,6 +2683,7 @@ static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu)
 		break;
 	case ACPI_IORT_SMMU_HISILICON_HI161X:
 		smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH;
+		smmu->options |= ARM_SMMU_OPT_RESV_HW_MSI;
 		break;
 	}
 
-- 
1.9.1

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameer Kolothum]

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

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

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 568c400..6f21dd7 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -608,6 +608,7 @@ struct arm_smmu_device {
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
 #define ARM_SMMU_OPT_PAGE0_REGS_ONLY	(1 << 1)
+#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 2)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;
@@ -1934,14 +1935,29 @@ static void arm_smmu_get_resv_regions(struct device *dev,
 				      struct list_head *head)
 {
 	struct iommu_resv_region *region;
+	struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv;
+	struct arm_smmu_device *smmu = master->smmu;
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+	int resv = 0;
 
-	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
-					 prot, IOMMU_RESV_SW_MSI);
-	if (!region)
-		return;
+	if ((smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)) {
 
-	list_add_tail(&region->list, head);
+		resv = iommu_dma_get_msi_resv_regions(dev, head);
+
+		if (resv < 0) {
+			dev_warn(dev, "HW MSI region resv failed: %d\n", resv);
+			return;
+		}
+	}
+
+	if (!resv) {
+		region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
+						 prot, IOMMU_RESV_SW_MSI);
+		if (!region)
+			return;
+
+		list_add_tail(&region->list, head);
+	}
 
 	iommu_dma_get_resv_regions(dev, head);
 }
@@ -2667,6 +2683,7 @@ static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu)
 		break;
 	case ACPI_IORT_SMMU_HISILICON_HI161X:
 		smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH;
+		smmu->options |= ARM_SMMU_OPT_RESV_HW_MSI;
 		break;
 	}
 
-- 
1.9.1

[Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameer Kolothum]

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

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

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

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 568c400..6f21dd7 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -608,6 +608,7 @@ struct arm_smmu_device {
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
 #define ARM_SMMU_OPT_PAGE0_REGS_ONLY	(1 << 1)
+#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 2)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;
@@ -1934,14 +1935,29 @@ static void arm_smmu_get_resv_regions(struct device *dev,
 				      struct list_head *head)
 {
 	struct iommu_resv_region *region;
+	struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv;
+	struct arm_smmu_device *smmu = master->smmu;
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+	int resv = 0;
 
-	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
-					 prot, IOMMU_RESV_SW_MSI);
-	if (!region)
-		return;
+	if ((smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)) {
 
-	list_add_tail(&region->list, head);
+		resv = iommu_dma_get_msi_resv_regions(dev, head);
+
+		if (resv < 0) {
+			dev_warn(dev, "HW MSI region resv failed: %d\n", resv);
+			return;
+		}
+	}
+
+	if (!resv) {
+		region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
+						 prot, IOMMU_RESV_SW_MSI);
+		if (!region)
+			return;
+
+		list_add_tail(&region->list, head);
+	}
 
 	iommu_dma_get_resv_regions(dev, head);
 }
@@ -2667,6 +2683,7 @@ static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu)
 		break;
 	case ACPI_IORT_SMMU_HISILICON_HI161X:
 		smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH;
+		smmu->options |= ARM_SMMU_OPT_RESV_HW_MSI;
 		break;
 	}
 
-- 
1.9.1

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
> The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.
> 
> 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.
> 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.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)

Please can you also add a devicetree binding with corresponding
documentation to enable this workaround on non-ACPI based systems too? It
should be straightforward if you update the arm_smmu_options table.

Thanks,

Will

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
> The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.
> 
> 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.
> 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.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)

Please can you also add a devicetree binding with corresponding
documentation to enable this workaround on non-ACPI based systems too? It
should be straightforward if you update the arm_smmu_options table.

Thanks,

Will

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

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

On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
> The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.
> 
> 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.
> 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.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com>
> ---
>  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)

Please can you also add a devicetree binding with corresponding
documentation to enable this workaround on non-ACPI based systems too? It
should be straightforward if you update the arm_smmu_options table.

Thanks,

Will

RE: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

Hi Will,

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon-5wv7dgnIgG8@public.gmane.org]
> Sent: Thursday, August 10, 2017 6:27 PM
> To: Shameerali Kolothum Thodi
> Cc: lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org; marc.zyngier-5wv7dgnIgG8@public.gmane.org;
> sudeep.holla-5wv7dgnIgG8@public.gmane.org; robin.murphy-5wv7dgnIgG8@public.gmane.org; hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org;
> Gabriele Paoloni; John Garry; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; linux-arm-
> kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devel-E0kO6a4B6psdnm+yROfE0A@public.gmane.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon erratum 161010801
> 
> On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
> > The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> > platforms Hip06/Hip07 to support the SMMU mappings for MSI
> transactions.
> >
> > 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.
> > 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.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> Please can you also add a devicetree binding with corresponding
> documentation to enable this workaround on non-ACPI based systems too?
> It should be straightforward if you update the arm_smmu_options table.

As I mentioned before, devicetree was a lower priority and we would definitely
submit patch to support that. Even if we update the arm_smmu_options table
with DT binding, the generic function to retrieve the MSI address regions only
works on ACPI/IORT case now.

Moreover I am on holidays starting tomorrow, and really appreciate if this series
can go through now.

Please let me know.

Thanks,
Shameer

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

Hi Will,

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon at arm.com]
> Sent: Thursday, August 10, 2017 6:27 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; 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: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon erratum 161010801
> 
> On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
> > The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> > platforms Hip06/Hip07 to support the SMMU mappings for MSI
> transactions.
> >
> > 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.
> > 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.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> Please can you also add a devicetree binding with corresponding
> documentation to enable this workaround on non-ACPI based systems too?
> It should be straightforward if you update the arm_smmu_options table.

As I mentioned before, devicetree was a lower priority and we would definitely
submit patch to support that. Even if we update the arm_smmu_options table
with DT binding, the generic function to retrieve the MSI address regions only
works on ACPI/IORT case now.

Moreover I am on holidays starting tomorrow, and really appreciate if this series
can go through now.

Please let me know.

Thanks,
Shameer

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

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

Hi Will,

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon(a)arm.com]
> Sent: Thursday, August 10, 2017 6:27 PM
> To: Shameerali Kolothum Thodi
> Cc: lorenzo.pieralisi(a)arm.com; marc.zyngier(a)arm.com;
> sudeep.holla(a)arm.com; robin.murphy(a)arm.com; hanjun.guo(a)linaro.org;
> Gabriele Paoloni; John Garry; iommu(a)lists.linux-foundation.org; linux-arm-
> kernel(a)lists.infradead.org; linux-acpi(a)vger.kernel.org; devel(a)acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon erratum 161010801
> 
> On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
> > The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> > platforms Hip06/Hip07 to support the SMMU mappings for MSI
> transactions.
> >
> > 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.
> > 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.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi(a)huawei.com>
> > ---
> >  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> Please can you also add a devicetree binding with corresponding
> documentation to enable this workaround on non-ACPI based systems too?
> It should be straightforward if you update the arm_smmu_options table.

As I mentioned before, devicetree was a lower priority and we would definitely
submit patch to support that. Even if we update the arm_smmu_options table
with DT binding, the generic function to retrieve the MSI address regions only
works on ACPI/IORT case now.

Moreover I am on holidays starting tomorrow, and really appreciate if this series
can go through now.

Please let me know.

Thanks,
Shameer

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>>  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> Please can you also add a devicetree binding with corresponding
>> documentation to enable this workaround on non-ACPI based systems too?
>> It should be straightforward if you update the arm_smmu_options table.
>
> As I mentioned before, devicetree was a lower priority and we would definitely
> submit patch to support that. Even if we update the arm_smmu_options table
> with DT binding, the generic function to retrieve the MSI address regions only
> works on ACPI/IORT case now.
>

Hi Will,

Can you confirm your stance on supporting this workaround for DT as well 
as ACPI?

For us, we now only "officially" support ACPI FW, and DT support at this 
point is patchy/limited. To me, adding DT support is just more errata 
workaround code to maintain with little useful gain.

Thanks very much,
John

> Moreover I am on holidays starting tomorrow, and really appreciate if this series
> can go through now.
>
> Please let me know.
>
> Thanks,
> Shameer
>
> .
>

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>>  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> Please can you also add a devicetree binding with corresponding
>> documentation to enable this workaround on non-ACPI based systems too?
>> It should be straightforward if you update the arm_smmu_options table.
>
> As I mentioned before, devicetree was a lower priority and we would definitely
> submit patch to support that. Even if we update the arm_smmu_options table
> with DT binding, the generic function to retrieve the MSI address regions only
> works on ACPI/IORT case now.
>

Hi Will,

Can you confirm your stance on supporting this workaround for DT as well 
as ACPI?

For us, we now only "officially" support ACPI FW, and DT support at this 
point is patchy/limited. To me, adding DT support is just more errata 
workaround code to maintain with little useful gain.

Thanks very much,
John

> Moreover I am on holidays starting tomorrow, and really appreciate if this series
> can go through now.
>
> Please let me know.
>
> Thanks,
> Shameer
>
> .
>

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

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

>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.thodi(a)huawei.com>
>>> ---
>>>  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> Please can you also add a devicetree binding with corresponding
>> documentation to enable this workaround on non-ACPI based systems too?
>> It should be straightforward if you update the arm_smmu_options table.
>
> As I mentioned before, devicetree was a lower priority and we would definitely
> submit patch to support that. Even if we update the arm_smmu_options table
> with DT binding, the generic function to retrieve the MSI address regions only
> works on ACPI/IORT case now.
>

Hi Will,

Can you confirm your stance on supporting this workaround for DT as well 
as ACPI?

For us, we now only "officially" support ACPI FW, and DT support at this 
point is patchy/limited. To me, adding DT support is just more errata 
workaround code to maintain with little useful gain.

Thanks very much,
John

> Moreover I am on holidays starting tomorrow, and really appreciate if this series
> can go through now.
>
> Please let me know.
>
> Thanks,
> Shameer
>
> .
>

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>Signed-off-by: Shameer Kolothum
> >><shameerali.kolothum.thodi@huawei.com>
> >>>---
> >>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >>> 1 file changed, 22 insertions(+), 5 deletions(-)
> >>
> >>Please can you also add a devicetree binding with corresponding
> >>documentation to enable this workaround on non-ACPI based systems too?
> >>It should be straightforward if you update the arm_smmu_options table.
> >
> >As I mentioned before, devicetree was a lower priority and we would definitely
> >submit patch to support that. Even if we update the arm_smmu_options table
> >with DT binding, the generic function to retrieve the MSI address regions only
> >works on ACPI/IORT case now.
> >
> 
> Hi Will,
> 
> Can you confirm your stance on supporting this workaround for DT as well as
> ACPI?
> 
> For us, we now only "officially" support ACPI FW, and DT support at this
> point is patchy/limited. To me, adding DT support is just more errata
> workaround code to maintain with little useful gain.

I basically don't like the idea of a driver that only works for one of
ACPI or DT yet claims to support both. I'm less fussed about functionality
differences (feature X is only available with firmware Y), but not working
around a hardware erratum that we know about is just lazy.

So I'd prefer that we handle this in both cases, or blacklist affected
devices when booting with DT. Continuing as though there isn't an erratum
is the worst thing we can do.

Will

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>Signed-off-by: Shameer Kolothum
> >><shameerali.kolothum.thodi@huawei.com>
> >>>---
> >>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >>> 1 file changed, 22 insertions(+), 5 deletions(-)
> >>
> >>Please can you also add a devicetree binding with corresponding
> >>documentation to enable this workaround on non-ACPI based systems too?
> >>It should be straightforward if you update the arm_smmu_options table.
> >
> >As I mentioned before, devicetree was a lower priority and we would definitely
> >submit patch to support that. Even if we update the arm_smmu_options table
> >with DT binding, the generic function to retrieve the MSI address regions only
> >works on ACPI/IORT case now.
> >
> 
> Hi Will,
> 
> Can you confirm your stance on supporting this workaround for DT as well as
> ACPI?
> 
> For us, we now only "officially" support ACPI FW, and DT support at this
> point is patchy/limited. To me, adding DT support is just more errata
> workaround code to maintain with little useful gain.

I basically don't like the idea of a driver that only works for one of
ACPI or DT yet claims to support both. I'm less fussed about functionality
differences (feature X is only available with firmware Y), but not working
around a hardware erratum that we know about is just lazy.

So I'd prefer that we handle this in both cases, or blacklist affected
devices when booting with DT. Continuing as though there isn't an erratum
is the worst thing we can do.

Will

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

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

On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>Signed-off-by: Shameer Kolothum
> >><shameerali.kolothum.thodi(a)huawei.com>
> >>>---
> >>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >>> 1 file changed, 22 insertions(+), 5 deletions(-)
> >>
> >>Please can you also add a devicetree binding with corresponding
> >>documentation to enable this workaround on non-ACPI based systems too?
> >>It should be straightforward if you update the arm_smmu_options table.
> >
> >As I mentioned before, devicetree was a lower priority and we would definitely
> >submit patch to support that. Even if we update the arm_smmu_options table
> >with DT binding, the generic function to retrieve the MSI address regions only
> >works on ACPI/IORT case now.
> >
> 
> Hi Will,
> 
> Can you confirm your stance on supporting this workaround for DT as well as
> ACPI?
> 
> For us, we now only "officially" support ACPI FW, and DT support at this
> point is patchy/limited. To me, adding DT support is just more errata
> workaround code to maintain with little useful gain.

I basically don't like the idea of a driver that only works for one of
ACPI or DT yet claims to support both. I'm less fussed about functionality
differences (feature X is only available with firmware Y), but not working
around a hardware erratum that we know about is just lazy.

So I'd prefer that we handle this in both cases, or blacklist affected
devices when booting with DT. Continuing as though there isn't an erratum
is the worst thing we can do.

Will

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

On 23/08/2017 14:24, Will Deacon wrote:
> On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
>>>>> Signed-off-by: Shameer Kolothum
>>>> <shameerali.kolothum.thodi@huawei.com>
>>>>> ---
>>>>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>>>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>>>
>>>> Please can you also add a devicetree binding with corresponding
>>>> documentation to enable this workaround on non-ACPI based systems too?
>>>> It should be straightforward if you update the arm_smmu_options table.
>>>
>>> As I mentioned before, devicetree was a lower priority and we would definitely
>>> submit patch to support that. Even if we update the arm_smmu_options table
>>> with DT binding, the generic function to retrieve the MSI address regions only
>>> works on ACPI/IORT case now.
>>>
>>
>> Hi Will,
>>
>> Can you confirm your stance on supporting this workaround for DT as well as
>> ACPI?
>>
>> For us, we now only "officially" support ACPI FW, and DT support at this
>> point is patchy/limited. To me, adding DT support is just more errata
>> workaround code to maintain with little useful gain.
>
> I basically don't like the idea of a driver that only works for one of
> ACPI or DT yet claims to support both. I'm less fussed about functionality
> differences (feature X is only available with firmware Y), but not working
> around a hardware erratum that we know about is just lazy.
>
> So I'd prefer that we handle this in both cases, or blacklist affected
> devices when booting with DT. Continuing as though there isn't an erratum
> is the worst thing we can do.

OK, seems reasonable.

We would consider blacklisting the device, where/how to do is the question.

So the errata is in the GICv3 ITS/PCI host controller, and we just use 
the in-between SMMU (driver) to provide the workaround. So my initial 
impression is that the PCI host controller would have to be blacklisted 
IFF behind an IOMMU for DT firmware in pcie-hisi.c or pci quirks 
framework. How does it sound?

Please also note that in our SoC we have other devices behind the same 
SMMU, like storage controller, which are not affected or related to the 
Errata.

BTW, ignoring DT support, are you happy with this patchset?

Regards,
John

>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> .
>

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

On 23/08/2017 14:24, Will Deacon wrote:
> On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
>>>>> Signed-off-by: Shameer Kolothum
>>>> <shameerali.kolothum.thodi@huawei.com>
>>>>> ---
>>>>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>>>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>>>
>>>> Please can you also add a devicetree binding with corresponding
>>>> documentation to enable this workaround on non-ACPI based systems too?
>>>> It should be straightforward if you update the arm_smmu_options table.
>>>
>>> As I mentioned before, devicetree was a lower priority and we would definitely
>>> submit patch to support that. Even if we update the arm_smmu_options table
>>> with DT binding, the generic function to retrieve the MSI address regions only
>>> works on ACPI/IORT case now.
>>>
>>
>> Hi Will,
>>
>> Can you confirm your stance on supporting this workaround for DT as well as
>> ACPI?
>>
>> For us, we now only "officially" support ACPI FW, and DT support at this
>> point is patchy/limited. To me, adding DT support is just more errata
>> workaround code to maintain with little useful gain.
>
> I basically don't like the idea of a driver that only works for one of
> ACPI or DT yet claims to support both. I'm less fussed about functionality
> differences (feature X is only available with firmware Y), but not working
> around a hardware erratum that we know about is just lazy.
>
> So I'd prefer that we handle this in both cases, or blacklist affected
> devices when booting with DT. Continuing as though there isn't an erratum
> is the worst thing we can do.

OK, seems reasonable.

We would consider blacklisting the device, where/how to do is the question.

So the errata is in the GICv3 ITS/PCI host controller, and we just use 
the in-between SMMU (driver) to provide the workaround. So my initial 
impression is that the PCI host controller would have to be blacklisted 
IFF behind an IOMMU for DT firmware in pcie-hisi.c or pci quirks 
framework. How does it sound?

Please also note that in our SoC we have other devices behind the same 
SMMU, like storage controller, which are not affected or related to the 
Errata.

BTW, ignoring DT support, are you happy with this patchset?

Regards,
John

>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> .
>

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

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

On 23/08/2017 14:24, Will Deacon wrote:
> On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
>>>>> Signed-off-by: Shameer Kolothum
>>>> <shameerali.kolothum.thodi(a)huawei.com>
>>>>> ---
>>>>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>>>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>>>
>>>> Please can you also add a devicetree binding with corresponding
>>>> documentation to enable this workaround on non-ACPI based systems too?
>>>> It should be straightforward if you update the arm_smmu_options table.
>>>
>>> As I mentioned before, devicetree was a lower priority and we would definitely
>>> submit patch to support that. Even if we update the arm_smmu_options table
>>> with DT binding, the generic function to retrieve the MSI address regions only
>>> works on ACPI/IORT case now.
>>>
>>
>> Hi Will,
>>
>> Can you confirm your stance on supporting this workaround for DT as well as
>> ACPI?
>>
>> For us, we now only "officially" support ACPI FW, and DT support at this
>> point is patchy/limited. To me, adding DT support is just more errata
>> workaround code to maintain with little useful gain.
>
> I basically don't like the idea of a driver that only works for one of
> ACPI or DT yet claims to support both. I'm less fussed about functionality
> differences (feature X is only available with firmware Y), but not working
> around a hardware erratum that we know about is just lazy.
>
> So I'd prefer that we handle this in both cases, or blacklist affected
> devices when booting with DT. Continuing as though there isn't an erratum
> is the worst thing we can do.

OK, seems reasonable.

We would consider blacklisting the device, where/how to do is the question.

So the errata is in the GICv3 ITS/PCI host controller, and we just use 
the in-between SMMU (driver) to provide the workaround. So my initial 
impression is that the PCI host controller would have to be blacklisted 
IFF behind an IOMMU for DT firmware in pcie-hisi.c or pci quirks 
framework. How does it sound?

Please also note that in our SoC we have other devices behind the same 
SMMU, like storage controller, which are not affected or related to the 
Errata.

BTW, ignoring DT support, are you happy with this patchset?

Regards,
John

>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> .
>

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
> On 23/08/2017 14:24, Will Deacon wrote:
> >On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>>>Signed-off-by: Shameer Kolothum
> >>>><shameerali.kolothum.thodi@huawei.com>
> >>>>>---
> >>>>>drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >>>>>1 file changed, 22 insertions(+), 5 deletions(-)
> >>>>
> >>>>Please can you also add a devicetree binding with corresponding
> >>>>documentation to enable this workaround on non-ACPI based systems too?
> >>>>It should be straightforward if you update the arm_smmu_options table.
> >>>
> >>>As I mentioned before, devicetree was a lower priority and we would definitely
> >>>submit patch to support that. Even if we update the arm_smmu_options table
> >>>with DT binding, the generic function to retrieve the MSI address regions only
> >>>works on ACPI/IORT case now.
> >>>
> >>
> >>Hi Will,
> >>
> >>Can you confirm your stance on supporting this workaround for DT as well as
> >>ACPI?
> >>
> >>For us, we now only "officially" support ACPI FW, and DT support at this
> >>point is patchy/limited. To me, adding DT support is just more errata
> >>workaround code to maintain with little useful gain.
> >
> >I basically don't like the idea of a driver that only works for one of
> >ACPI or DT yet claims to support both. I'm less fussed about functionality
> >differences (feature X is only available with firmware Y), but not working
> >around a hardware erratum that we know about is just lazy.
> >
> >So I'd prefer that we handle this in both cases, or blacklist affected
> >devices when booting with DT. Continuing as though there isn't an erratum
> >is the worst thing we can do.
> 
> OK, seems reasonable.
> 
> We would consider blacklisting the device, where/how to do is the question.
> 
> So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> in-between SMMU (driver) to provide the workaround. So my initial impression
> is that the PCI host controller would have to be blacklisted IFF behind an
> IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> sound?

If that avoids us running into the erratum, then fine by me. I'd obviously
prefer we work-around it since we know how to, but that's up to you.

> Please also note that in our SoC we have other devices behind the same SMMU,
> like storage controller, which are not affected or related to the Errata.
> 
> BTW, ignoring DT support, are you happy with this patchset?

Yes, but I won't ack it without the above taken care of.

Will

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
> On 23/08/2017 14:24, Will Deacon wrote:
> >On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>>>Signed-off-by: Shameer Kolothum
> >>>><shameerali.kolothum.thodi@huawei.com>
> >>>>>---
> >>>>>drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >>>>>1 file changed, 22 insertions(+), 5 deletions(-)
> >>>>
> >>>>Please can you also add a devicetree binding with corresponding
> >>>>documentation to enable this workaround on non-ACPI based systems too?
> >>>>It should be straightforward if you update the arm_smmu_options table.
> >>>
> >>>As I mentioned before, devicetree was a lower priority and we would definitely
> >>>submit patch to support that. Even if we update the arm_smmu_options table
> >>>with DT binding, the generic function to retrieve the MSI address regions only
> >>>works on ACPI/IORT case now.
> >>>
> >>
> >>Hi Will,
> >>
> >>Can you confirm your stance on supporting this workaround for DT as well as
> >>ACPI?
> >>
> >>For us, we now only "officially" support ACPI FW, and DT support at this
> >>point is patchy/limited. To me, adding DT support is just more errata
> >>workaround code to maintain with little useful gain.
> >
> >I basically don't like the idea of a driver that only works for one of
> >ACPI or DT yet claims to support both. I'm less fussed about functionality
> >differences (feature X is only available with firmware Y), but not working
> >around a hardware erratum that we know about is just lazy.
> >
> >So I'd prefer that we handle this in both cases, or blacklist affected
> >devices when booting with DT. Continuing as though there isn't an erratum
> >is the worst thing we can do.
> 
> OK, seems reasonable.
> 
> We would consider blacklisting the device, where/how to do is the question.
> 
> So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> in-between SMMU (driver) to provide the workaround. So my initial impression
> is that the PCI host controller would have to be blacklisted IFF behind an
> IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> sound?

If that avoids us running into the erratum, then fine by me. I'd obviously
prefer we work-around it since we know how to, but that's up to you.

> Please also note that in our SoC we have other devices behind the same SMMU,
> like storage controller, which are not affected or related to the Errata.
> 
> BTW, ignoring DT support, are you happy with this patchset?

Yes, but I won't ack it without the above taken care of.

Will

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

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

On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
> On 23/08/2017 14:24, Will Deacon wrote:
> >On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>>>Signed-off-by: Shameer Kolothum
> >>>><shameerali.kolothum.thodi(a)huawei.com>
> >>>>>---
> >>>>>drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >>>>>1 file changed, 22 insertions(+), 5 deletions(-)
> >>>>
> >>>>Please can you also add a devicetree binding with corresponding
> >>>>documentation to enable this workaround on non-ACPI based systems too?
> >>>>It should be straightforward if you update the arm_smmu_options table.
> >>>
> >>>As I mentioned before, devicetree was a lower priority and we would definitely
> >>>submit patch to support that. Even if we update the arm_smmu_options table
> >>>with DT binding, the generic function to retrieve the MSI address regions only
> >>>works on ACPI/IORT case now.
> >>>
> >>
> >>Hi Will,
> >>
> >>Can you confirm your stance on supporting this workaround for DT as well as
> >>ACPI?
> >>
> >>For us, we now only "officially" support ACPI FW, and DT support at this
> >>point is patchy/limited. To me, adding DT support is just more errata
> >>workaround code to maintain with little useful gain.
> >
> >I basically don't like the idea of a driver that only works for one of
> >ACPI or DT yet claims to support both. I'm less fussed about functionality
> >differences (feature X is only available with firmware Y), but not working
> >around a hardware erratum that we know about is just lazy.
> >
> >So I'd prefer that we handle this in both cases, or blacklist affected
> >devices when booting with DT. Continuing as though there isn't an erratum
> >is the worst thing we can do.
> 
> OK, seems reasonable.
> 
> We would consider blacklisting the device, where/how to do is the question.
> 
> So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> in-between SMMU (driver) to provide the workaround. So my initial impression
> is that the PCI host controller would have to be blacklisted IFF behind an
> IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> sound?

If that avoids us running into the erratum, then fine by me. I'd obviously
prefer we work-around it since we know how to, but that's up to you.

> Please also note that in our SoC we have other devices behind the same SMMU,
> like storage controller, which are not affected or related to the Errata.
> 
> BTW, ignoring DT support, are you happy with this patchset?

Yes, but I won't ack it without the above taken care of.

Will

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

On 23/08/2017 17:43, Will Deacon wrote:
> On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
>> On 23/08/2017 14:24, Will Deacon wrote:
>>> On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
>>>>>>> Signed-off-by: Shameer Kolothum
>>>>>> <shameerali.kolothum.thodi@huawei.com>
>>>>>>> ---
>>>>>>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>>>>>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> Please can you also add a devicetree binding with corresponding
>>>>>> documentation to enable this workaround on non-ACPI based systems too?
>>>>>> It should be straightforward if you update the arm_smmu_options table.
>>>>>
>>>>> As I mentioned before, devicetree was a lower priority and we would definitely
>>>>> submit patch to support that. Even if we update the arm_smmu_options table
>>>>> with DT binding, the generic function to retrieve the MSI address regions only
>>>>> works on ACPI/IORT case now.
>>>>>
>>>>
>>>> Hi Will,
>>>>
>>>> Can you confirm your stance on supporting this workaround for DT as well as
>>>> ACPI?
>>>>
>>>> For us, we now only "officially" support ACPI FW, and DT support at this
>>>> point is patchy/limited. To me, adding DT support is just more errata
>>>> workaround code to maintain with little useful gain.
>>>
>>> I basically don't like the idea of a driver that only works for one of
>>> ACPI or DT yet claims to support both. I'm less fussed about functionality
>>> differences (feature X is only available with firmware Y), but not working
>>> around a hardware erratum that we know about is just lazy.
>>>
>>> So I'd prefer that we handle this in both cases, or blacklist affected
>>> devices when booting with DT. Continuing as though there isn't an erratum
>>> is the worst thing we can do.
>>
>> OK, seems reasonable.
>>
>> We would consider blacklisting the device, where/how to do is the question.
>>
>> So the errata is in the GICv3 ITS/PCI host controller, and we just use the
>> in-between SMMU (driver) to provide the workaround. So my initial impression
>> is that the PCI host controller would have to be blacklisted IFF behind an
>> IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
>> sound?
>
> If that avoids us running into the erratum, then fine by me. I'd obviously
> prefer we work-around it since we know how to, but that's up to you.

I'm surpsised that you may want more errata workaround code to maintain.

Anyway we'll check both approaches and show you how they look and go 
from there.

>
>> Please also note that in our SoC we have other devices behind the same SMMU,
>> like storage controller, which are not affected or related to the Errata.
>>
>> BTW, ignoring DT support, are you happy with this patchset?
>
> Yes, but I won't ack it without the above taken care of.

Fair enough.

>
> Will

Thanks,
John

>
> .
>

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

On 23/08/2017 17:43, Will Deacon wrote:
> On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
>> On 23/08/2017 14:24, Will Deacon wrote:
>>> On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
>>>>>>> Signed-off-by: Shameer Kolothum
>>>>>> <shameerali.kolothum.thodi@huawei.com>
>>>>>>> ---
>>>>>>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>>>>>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> Please can you also add a devicetree binding with corresponding
>>>>>> documentation to enable this workaround on non-ACPI based systems too?
>>>>>> It should be straightforward if you update the arm_smmu_options table.
>>>>>
>>>>> As I mentioned before, devicetree was a lower priority and we would definitely
>>>>> submit patch to support that. Even if we update the arm_smmu_options table
>>>>> with DT binding, the generic function to retrieve the MSI address regions only
>>>>> works on ACPI/IORT case now.
>>>>>
>>>>
>>>> Hi Will,
>>>>
>>>> Can you confirm your stance on supporting this workaround for DT as well as
>>>> ACPI?
>>>>
>>>> For us, we now only "officially" support ACPI FW, and DT support at this
>>>> point is patchy/limited. To me, adding DT support is just more errata
>>>> workaround code to maintain with little useful gain.
>>>
>>> I basically don't like the idea of a driver that only works for one of
>>> ACPI or DT yet claims to support both. I'm less fussed about functionality
>>> differences (feature X is only available with firmware Y), but not working
>>> around a hardware erratum that we know about is just lazy.
>>>
>>> So I'd prefer that we handle this in both cases, or blacklist affected
>>> devices when booting with DT. Continuing as though there isn't an erratum
>>> is the worst thing we can do.
>>
>> OK, seems reasonable.
>>
>> We would consider blacklisting the device, where/how to do is the question.
>>
>> So the errata is in the GICv3 ITS/PCI host controller, and we just use the
>> in-between SMMU (driver) to provide the workaround. So my initial impression
>> is that the PCI host controller would have to be blacklisted IFF behind an
>> IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
>> sound?
>
> If that avoids us running into the erratum, then fine by me. I'd obviously
> prefer we work-around it since we know how to, but that's up to you.

I'm surpsised that you may want more errata workaround code to maintain.

Anyway we'll check both approaches and show you how they look and go 
from there.

>
>> Please also note that in our SoC we have other devices behind the same SMMU,
>> like storage controller, which are not affected or related to the Errata.
>>
>> BTW, ignoring DT support, are you happy with this patchset?
>
> Yes, but I won't ack it without the above taken care of.

Fair enough.

>
> Will

Thanks,
John

>
> .
>

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

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

On 23/08/2017 17:43, Will Deacon wrote:
> On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
>> On 23/08/2017 14:24, Will Deacon wrote:
>>> On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
>>>>>>> Signed-off-by: Shameer Kolothum
>>>>>> <shameerali.kolothum.thodi(a)huawei.com>
>>>>>>> ---
>>>>>>> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>>>>>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> Please can you also add a devicetree binding with corresponding
>>>>>> documentation to enable this workaround on non-ACPI based systems too?
>>>>>> It should be straightforward if you update the arm_smmu_options table.
>>>>>
>>>>> As I mentioned before, devicetree was a lower priority and we would definitely
>>>>> submit patch to support that. Even if we update the arm_smmu_options table
>>>>> with DT binding, the generic function to retrieve the MSI address regions only
>>>>> works on ACPI/IORT case now.
>>>>>
>>>>
>>>> Hi Will,
>>>>
>>>> Can you confirm your stance on supporting this workaround for DT as well as
>>>> ACPI?
>>>>
>>>> For us, we now only "officially" support ACPI FW, and DT support at this
>>>> point is patchy/limited. To me, adding DT support is just more errata
>>>> workaround code to maintain with little useful gain.
>>>
>>> I basically don't like the idea of a driver that only works for one of
>>> ACPI or DT yet claims to support both. I'm less fussed about functionality
>>> differences (feature X is only available with firmware Y), but not working
>>> around a hardware erratum that we know about is just lazy.
>>>
>>> So I'd prefer that we handle this in both cases, or blacklist affected
>>> devices when booting with DT. Continuing as though there isn't an erratum
>>> is the worst thing we can do.
>>
>> OK, seems reasonable.
>>
>> We would consider blacklisting the device, where/how to do is the question.
>>
>> So the errata is in the GICv3 ITS/PCI host controller, and we just use the
>> in-between SMMU (driver) to provide the workaround. So my initial impression
>> is that the PCI host controller would have to be blacklisted IFF behind an
>> IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
>> sound?
>
> If that avoids us running into the erratum, then fine by me. I'd obviously
> prefer we work-around it since we know how to, but that's up to you.

I'm surpsised that you may want more errata workaround code to maintain.

Anyway we'll check both approaches and show you how they look and go 
from there.

>
>> Please also note that in our SoC we have other devices behind the same SMMU,
>> like storage controller, which are not affected or related to the Errata.
>>
>> BTW, ignoring DT support, are you happy with this patchset?
>
> Yes, but I won't ack it without the above taken care of.

Fair enough.

>
> Will

Thanks,
John

>
> .
>

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

On Wed, Aug 23, 2017 at 05:55:52PM +0100, John Garry wrote:
> On 23/08/2017 17:43, Will Deacon wrote:
> >On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
> >>On 23/08/2017 14:24, Will Deacon wrote:
> >>>On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>>>>>Signed-off-by: Shameer Kolothum
> >>>>>><shameerali.kolothum.thodi@huawei.com>
> >>>>>>>---
> >>>>>>>drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >>>>>>>1 file changed, 22 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>>Please can you also add a devicetree binding with corresponding
> >>>>>>documentation to enable this workaround on non-ACPI based systems too?
> >>>>>>It should be straightforward if you update the arm_smmu_options table.
> >>>>>
> >>>>>As I mentioned before, devicetree was a lower priority and we would definitely
> >>>>>submit patch to support that. Even if we update the arm_smmu_options table
> >>>>>with DT binding, the generic function to retrieve the MSI address regions only
> >>>>>works on ACPI/IORT case now.
> >>>>>
> >>>>
> >>>>Hi Will,
> >>>>
> >>>>Can you confirm your stance on supporting this workaround for DT as well as
> >>>>ACPI?
> >>>>
> >>>>For us, we now only "officially" support ACPI FW, and DT support at this
> >>>>point is patchy/limited. To me, adding DT support is just more errata
> >>>>workaround code to maintain with little useful gain.
> >>>
> >>>I basically don't like the idea of a driver that only works for one of
> >>>ACPI or DT yet claims to support both. I'm less fussed about functionality
> >>>differences (feature X is only available with firmware Y), but not working
> >>>around a hardware erratum that we know about is just lazy.
> >>>
> >>>So I'd prefer that we handle this in both cases, or blacklist affected
> >>>devices when booting with DT. Continuing as though there isn't an erratum
> >>>is the worst thing we can do.
> >>
> >>OK, seems reasonable.
> >>
> >>We would consider blacklisting the device, where/how to do is the question.
> >>
> >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> >>in-between SMMU (driver) to provide the workaround. So my initial impression
> >>is that the PCI host controller would have to be blacklisted IFF behind an
> >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> >>sound?
> >
> >If that avoids us running into the erratum, then fine by me. I'd obviously
> >prefer we work-around it since we know how to, but that's up to you.
> 
> I'm surpsised that you may want more errata workaround code to maintain.
> 
> Anyway we'll check both approaches and show you how they look and go from
> there.

Don't get me wrong, I don't dream about adding errata workarounds to the
code, but our job as an operating system is to abstract the hardware from
the user, which means dealing with its quirks whether we like it or not.

Thanks,

Will

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

On Wed, Aug 23, 2017 at 05:55:52PM +0100, John Garry wrote:
> On 23/08/2017 17:43, Will Deacon wrote:
> >On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
> >>On 23/08/2017 14:24, Will Deacon wrote:
> >>>On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>>>>>Signed-off-by: Shameer Kolothum
> >>>>>><shameerali.kolothum.thodi@huawei.com>
> >>>>>>>---
> >>>>>>>drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >>>>>>>1 file changed, 22 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>>Please can you also add a devicetree binding with corresponding
> >>>>>>documentation to enable this workaround on non-ACPI based systems too?
> >>>>>>It should be straightforward if you update the arm_smmu_options table.
> >>>>>
> >>>>>As I mentioned before, devicetree was a lower priority and we would definitely
> >>>>>submit patch to support that. Even if we update the arm_smmu_options table
> >>>>>with DT binding, the generic function to retrieve the MSI address regions only
> >>>>>works on ACPI/IORT case now.
> >>>>>
> >>>>
> >>>>Hi Will,
> >>>>
> >>>>Can you confirm your stance on supporting this workaround for DT as well as
> >>>>ACPI?
> >>>>
> >>>>For us, we now only "officially" support ACPI FW, and DT support at this
> >>>>point is patchy/limited. To me, adding DT support is just more errata
> >>>>workaround code to maintain with little useful gain.
> >>>
> >>>I basically don't like the idea of a driver that only works for one of
> >>>ACPI or DT yet claims to support both. I'm less fussed about functionality
> >>>differences (feature X is only available with firmware Y), but not working
> >>>around a hardware erratum that we know about is just lazy.
> >>>
> >>>So I'd prefer that we handle this in both cases, or blacklist affected
> >>>devices when booting with DT. Continuing as though there isn't an erratum
> >>>is the worst thing we can do.
> >>
> >>OK, seems reasonable.
> >>
> >>We would consider blacklisting the device, where/how to do is the question.
> >>
> >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> >>in-between SMMU (driver) to provide the workaround. So my initial impression
> >>is that the PCI host controller would have to be blacklisted IFF behind an
> >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> >>sound?
> >
> >If that avoids us running into the erratum, then fine by me. I'd obviously
> >prefer we work-around it since we know how to, but that's up to you.
> 
> I'm surpsised that you may want more errata workaround code to maintain.
> 
> Anyway we'll check both approaches and show you how they look and go from
> there.

Don't get me wrong, I don't dream about adding errata workarounds to the
code, but our job as an operating system is to abstract the hardware from
the user, which means dealing with its quirks whether we like it or not.

Thanks,

Will

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Will Deacon]

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

On Wed, Aug 23, 2017 at 05:55:52PM +0100, John Garry wrote:
> On 23/08/2017 17:43, Will Deacon wrote:
> >On Wed, Aug 23, 2017 at 03:29:46PM +0100, John Garry wrote:
> >>On 23/08/2017 14:24, Will Deacon wrote:
> >>>On Wed, Aug 23, 2017 at 02:17:24PM +0100, John Garry wrote:
> >>>>>>>Signed-off-by: Shameer Kolothum
> >>>>>><shameerali.kolothum.thodi(a)huawei.com>
> >>>>>>>---
> >>>>>>>drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >>>>>>>1 file changed, 22 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>>Please can you also add a devicetree binding with corresponding
> >>>>>>documentation to enable this workaround on non-ACPI based systems too?
> >>>>>>It should be straightforward if you update the arm_smmu_options table.
> >>>>>
> >>>>>As I mentioned before, devicetree was a lower priority and we would definitely
> >>>>>submit patch to support that. Even if we update the arm_smmu_options table
> >>>>>with DT binding, the generic function to retrieve the MSI address regions only
> >>>>>works on ACPI/IORT case now.
> >>>>>
> >>>>
> >>>>Hi Will,
> >>>>
> >>>>Can you confirm your stance on supporting this workaround for DT as well as
> >>>>ACPI?
> >>>>
> >>>>For us, we now only "officially" support ACPI FW, and DT support at this
> >>>>point is patchy/limited. To me, adding DT support is just more errata
> >>>>workaround code to maintain with little useful gain.
> >>>
> >>>I basically don't like the idea of a driver that only works for one of
> >>>ACPI or DT yet claims to support both. I'm less fussed about functionality
> >>>differences (feature X is only available with firmware Y), but not working
> >>>around a hardware erratum that we know about is just lazy.
> >>>
> >>>So I'd prefer that we handle this in both cases, or blacklist affected
> >>>devices when booting with DT. Continuing as though there isn't an erratum
> >>>is the worst thing we can do.
> >>
> >>OK, seems reasonable.
> >>
> >>We would consider blacklisting the device, where/how to do is the question.
> >>
> >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the
> >>in-between SMMU (driver) to provide the workaround. So my initial impression
> >>is that the PCI host controller would have to be blacklisted IFF behind an
> >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
> >>sound?
> >
> >If that avoids us running into the erratum, then fine by me. I'd obviously
> >prefer we work-around it since we know how to, but that's up to you.
> 
> I'm surpsised that you may want more errata workaround code to maintain.
> 
> Anyway we'll check both approaches and show you how they look and go from
> there.

Don't get me wrong, I don't dream about adding errata workarounds to the
code, but our job as an operating system is to abstract the hardware from
the user, which means dealing with its quirks whether we like it or not.

Thanks,

Will

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

On 24/08/2017 15:35, Will Deacon wrote:
>>>> > >>OK, seems reasonable.
>>>> > >>
>>>> > >>We would consider blacklisting the device, where/how to do is the question.
>>>> > >>
>>>> > >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the
>>>> > >>in-between SMMU (driver) to provide the workaround. So my initial impression
>>>> > >>is that the PCI host controller would have to be blacklisted IFF behind an
>>>> > >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
>>>> > >>sound?
>>> > >
>>> > >If that avoids us running into the erratum, then fine by me. I'd obviously
>>> > >prefer we work-around it since we know how to, but that's up to you.
>> >
>> > I'm surpsised that you may want more errata workaround code to maintain.
>> >
>> > Anyway we'll check both approaches and show you how they look and go from
>> > there.
> Don't get me wrong, I don't dream about adding errata workarounds to the
> code, but our job as an operating system is to abstract the hardware from
> the user, which means dealing with its quirks whether we like it or not.
>

Fine, it's ok.

For our next platform, hip08, we will provide no DT FW support, so this 
whole DT grey area should not be an issue.

And hopefully no errata also.

Much appreciated,
John

> Thanks,
>
> Will

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

On 24/08/2017 15:35, Will Deacon wrote:
>>>> > >>OK, seems reasonable.
>>>> > >>
>>>> > >>We would consider blacklisting the device, where/how to do is the question.
>>>> > >>
>>>> > >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the
>>>> > >>in-between SMMU (driver) to provide the workaround. So my initial impression
>>>> > >>is that the PCI host controller would have to be blacklisted IFF behind an
>>>> > >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
>>>> > >>sound?
>>> > >
>>> > >If that avoids us running into the erratum, then fine by me. I'd obviously
>>> > >prefer we work-around it since we know how to, but that's up to you.
>> >
>> > I'm surpsised that you may want more errata workaround code to maintain.
>> >
>> > Anyway we'll check both approaches and show you how they look and go from
>> > there.
> Don't get me wrong, I don't dream about adding errata workarounds to the
> code, but our job as an operating system is to abstract the hardware from
> the user, which means dealing with its quirks whether we like it or not.
>

Fine, it's ok.

For our next platform, hip08, we will provide no DT FW support, so this 
whole DT grey area should not be an issue.

And hopefully no errata also.

Much appreciated,
John

> Thanks,
>
> Will

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

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

On 24/08/2017 15:35, Will Deacon wrote:
>>>> > >>OK, seems reasonable.
>>>> > >>
>>>> > >>We would consider blacklisting the device, where/how to do is the question.
>>>> > >>
>>>> > >>So the errata is in the GICv3 ITS/PCI host controller, and we just use the
>>>> > >>in-between SMMU (driver) to provide the workaround. So my initial impression
>>>> > >>is that the PCI host controller would have to be blacklisted IFF behind an
>>>> > >>IOMMU for DT firmware in pcie-hisi.c or pci quirks framework. How does it
>>>> > >>sound?
>>> > >
>>> > >If that avoids us running into the erratum, then fine by me. I'd obviously
>>> > >prefer we work-around it since we know how to, but that's up to you.
>> >
>> > I'm surpsised that you may want more errata workaround code to maintain.
>> >
>> > Anyway we'll check both approaches and show you how they look and go from
>> > there.
> Don't get me wrong, I don't dream about adding errata workarounds to the
> code, but our job as an operating system is to abstract the hardware from
> the user, which means dealing with its quirks whether we like it or not.
>

Fine, it's ok.

For our next platform, hip08, we will provide no DT FW support, so this 
whole DT grey area should not be an issue.

And hopefully no errata also.

Much appreciated,
John

> Thanks,
>
> Will

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

On 10/08/2017 18:27, Will Deacon wrote:
> On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
>> The HiSilicon erratum 161010801 describes the limitation of HiSilicon
>> platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.
>>
>> 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.
>> 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.
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> Please can you also add a devicetree binding with corresponding
> documentation to enable this workaround on non-ACPI based systems too? It
> should be straightforward if you update the arm_smmu_options table.
>

Hi Will, Lorenzo, Robin,

I have created the patch to add DT support for this erratum. However, 
currently I have only added support for pci-based devices. I'm a bit 
stumped on how to add platform device support, or if we should also add 
support at all. And I would rather ask before sending the patches.

The specific issue is that I know of no platform device with an ITS 
msi-parent which we would want reserve, i.e. do not translate msi. And, 
if there were, how to do it.

The only platform devices I know off with msi ITS parents are SMMUv3 and 
mbigen. For mbigen, the msi are not translated. Actually, as I see under 
IORT solution, for mbigen we don't reserve the hw msi region as the 
SMMUv3 does not have an ID mapping. And we have no equivalent ID mapping 
property for DT SMMUv3, so cannot create an equivalent check.

So here is how the DT iommu get reserved region function with only pci 
device support looks:

/**
  * of_iommu_its_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
  * @list: Reserved region list from iommu_get_resv_regions()
  *
  * Returns: Number of reserved regions on success(0 if no associated ITS),
  *          appropriate error value otherwise.
  */
int of_iommu_its_get_resv_regions(struct device *dev, struct list_head 
*head)
{
     struct device_node *of_node = NULL;
     struct device *parent_dev;
     const __be32 *msi_map;
     int num_mappings, i, err, len, resv = 0;

     /* walk up to find the parent with a msi-map property */
     for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
         if (!parent_dev->of_node)
             continue;

         /*
          * Current logic to reserve ITS regions for only PCI root
          * complex.
          */
         msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
         if (msi_map) {
             of_node = parent_dev->of_node;
             break;
         }
     }

     if (!of_node)
         return 0;

     num_mappings = of_count_phandle_with_args(of_node, "msi-map", NULL) 
/ 4;

     for (i = 0; i < num_mappings; i++) {
         int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
         struct iommu_resv_region *region;
         struct device_node *msi_node;
         struct resource resource;
         u32 phandle;

         phandle = be32_to_cpup(msi_map + 1);
         msi_node = of_find_node_by_phandle(phandle);
         if (!msi_node)
             return -ENODEV;

         /*
          * There may be different types of msi-controller, so check
          * for ITS.
          */
         if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
             of_node_put(msi_node);
             continue;
         }

         err = of_address_to_resource(msi_node, 0, &resource);

         of_node_put(msi_node);
         if (err)
             return err;

         region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
                          IOMMU_RESV_MSI);
         if (region) {
             list_add_tail(&region->list, head);
             resv++;
         }
     }

     return (resv == num_mappings) ? resv : -ENODEV;
}

Any feedback is appreciated.

Thanks,
John


> Thanks,
>
> Will
>
> .
>

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

On 10/08/2017 18:27, Will Deacon wrote:
> On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
>> The HiSilicon erratum 161010801 describes the limitation of HiSilicon
>> platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.
>>
>> 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.
>> 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.
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> Please can you also add a devicetree binding with corresponding
> documentation to enable this workaround on non-ACPI based systems too? It
> should be straightforward if you update the arm_smmu_options table.
>

Hi Will, Lorenzo, Robin,

I have created the patch to add DT support for this erratum. However, 
currently I have only added support for pci-based devices. I'm a bit 
stumped on how to add platform device support, or if we should also add 
support at all. And I would rather ask before sending the patches.

The specific issue is that I know of no platform device with an ITS 
msi-parent which we would want reserve, i.e. do not translate msi. And, 
if there were, how to do it.

The only platform devices I know off with msi ITS parents are SMMUv3 and 
mbigen. For mbigen, the msi are not translated. Actually, as I see under 
IORT solution, for mbigen we don't reserve the hw msi region as the 
SMMUv3 does not have an ID mapping. And we have no equivalent ID mapping 
property for DT SMMUv3, so cannot create an equivalent check.

So here is how the DT iommu get reserved region function with only pci 
device support looks:

/**
  * of_iommu_its_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
  * @list: Reserved region list from iommu_get_resv_regions()
  *
  * Returns: Number of reserved regions on success(0 if no associated ITS),
  *          appropriate error value otherwise.
  */
int of_iommu_its_get_resv_regions(struct device *dev, struct list_head 
*head)
{
     struct device_node *of_node = NULL;
     struct device *parent_dev;
     const __be32 *msi_map;
     int num_mappings, i, err, len, resv = 0;

     /* walk up to find the parent with a msi-map property */
     for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
         if (!parent_dev->of_node)
             continue;

         /*
          * Current logic to reserve ITS regions for only PCI root
          * complex.
          */
         msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
         if (msi_map) {
             of_node = parent_dev->of_node;
             break;
         }
     }

     if (!of_node)
         return 0;

     num_mappings = of_count_phandle_with_args(of_node, "msi-map", NULL) 
/ 4;

     for (i = 0; i < num_mappings; i++) {
         int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
         struct iommu_resv_region *region;
         struct device_node *msi_node;
         struct resource resource;
         u32 phandle;

         phandle = be32_to_cpup(msi_map + 1);
         msi_node = of_find_node_by_phandle(phandle);
         if (!msi_node)
             return -ENODEV;

         /*
          * There may be different types of msi-controller, so check
          * for ITS.
          */
         if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
             of_node_put(msi_node);
             continue;
         }

         err = of_address_to_resource(msi_node, 0, &resource);

         of_node_put(msi_node);
         if (err)
             return err;

         region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
                          IOMMU_RESV_MSI);
         if (region) {
             list_add_tail(&region->list, head);
             resv++;
         }
     }

     return (resv == num_mappings) ? resv : -ENODEV;
}

Any feedback is appreciated.

Thanks,
John


> Thanks,
>
> Will
>
> .
>

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

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

On 10/08/2017 18:27, Will Deacon wrote:
> On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
>> The HiSilicon erratum 161010801 describes the limitation of HiSilicon
>> platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.
>>
>> 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.
>> 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.
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> Please can you also add a devicetree binding with corresponding
> documentation to enable this workaround on non-ACPI based systems too? It
> should be straightforward if you update the arm_smmu_options table.
>

Hi Will, Lorenzo, Robin,

I have created the patch to add DT support for this erratum. However, 
currently I have only added support for pci-based devices. I'm a bit 
stumped on how to add platform device support, or if we should also add 
support at all. And I would rather ask before sending the patches.

The specific issue is that I know of no platform device with an ITS 
msi-parent which we would want reserve, i.e. do not translate msi. And, 
if there were, how to do it.

The only platform devices I know off with msi ITS parents are SMMUv3 and 
mbigen. For mbigen, the msi are not translated. Actually, as I see under 
IORT solution, for mbigen we don't reserve the hw msi region as the 
SMMUv3 does not have an ID mapping. And we have no equivalent ID mapping 
property for DT SMMUv3, so cannot create an equivalent check.

So here is how the DT iommu get reserved region function with only pci 
device support looks:

/**
  * of_iommu_its_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
  * @list: Reserved region list from iommu_get_resv_regions()
  *
  * Returns: Number of reserved regions on success(0 if no associated ITS),
  *          appropriate error value otherwise.
  */
int of_iommu_its_get_resv_regions(struct device *dev, struct list_head 
*head)
{
     struct device_node *of_node = NULL;
     struct device *parent_dev;
     const __be32 *msi_map;
     int num_mappings, i, err, len, resv = 0;

     /* walk up to find the parent with a msi-map property */
     for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
         if (!parent_dev->of_node)
             continue;

         /*
          * Current logic to reserve ITS regions for only PCI root
          * complex.
          */
         msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
         if (msi_map) {
             of_node = parent_dev->of_node;
             break;
         }
     }

     if (!of_node)
         return 0;

     num_mappings = of_count_phandle_with_args(of_node, "msi-map", NULL) 
/ 4;

     for (i = 0; i < num_mappings; i++) {
         int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
         struct iommu_resv_region *region;
         struct device_node *msi_node;
         struct resource resource;
         u32 phandle;

         phandle = be32_to_cpup(msi_map + 1);
         msi_node = of_find_node_by_phandle(phandle);
         if (!msi_node)
             return -ENODEV;

         /*
          * There may be different types of msi-controller, so check
          * for ITS.
          */
         if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
             of_node_put(msi_node);
             continue;
         }

         err = of_address_to_resource(msi_node, 0, &resource);

         of_node_put(msi_node);
         if (err)
             return err;

         region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
                          IOMMU_RESV_MSI);
         if (region) {
             list_add_tail(&region->list, head);
             resv++;
         }
     }

     return (resv == num_mappings) ? resv : -ENODEV;
}

Any feedback is appreciated.

Thanks,
John


> Thanks,
>
> Will
>
> .
>

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

On Fri, Sep 01, 2017 at 09:46:00AM +0100, John Garry wrote:
> On 10/08/2017 18:27, Will Deacon wrote:
> >On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
> >>The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> >>platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.
> >>
> >>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.
> >>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.
> >>
> >>Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> >>---
> >> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >> 1 file changed, 22 insertions(+), 5 deletions(-)
> >
> >Please can you also add a devicetree binding with corresponding
> >documentation to enable this workaround on non-ACPI based systems too? It
> >should be straightforward if you update the arm_smmu_options table.
> >
> 
> Hi Will, Lorenzo, Robin,
> 
> I have created the patch to add DT support for this erratum.
> However, currently I have only added support for pci-based devices.
> I'm a bit stumped on how to add platform device support, or if we
> should also add support at all. And I would rather ask before
> sending the patches.
> 
> The specific issue is that I know of no platform device with an ITS
> msi-parent which we would want reserve, i.e. do not translate msi.
> And, if there were, how to do it.
> 
> The only platform devices I know off with msi ITS parents are SMMUv3
> and mbigen. For mbigen, the msi are not translated. Actually, as I
> see under IORT solution, for mbigen we don't reserve the hw msi
> region as the SMMUv3 does not have an ID mapping. And we have no
> equivalent ID mapping property for DT SMMUv3, so cannot create an
> equivalent check.
> 
> So here is how the DT iommu get reserved region function with only
> pci device support looks:
> 
> /**
>  * of_iommu_its_get_resv_regions - Reserved region driver helper
>  * @dev: Device from iommu_get_resv_regions()
>  * @list: Reserved region list from iommu_get_resv_regions()
>  *
>  * Returns: Number of reserved regions on success(0 if no associated ITS),
>  *          appropriate error value otherwise.
>  */
> int of_iommu_its_get_resv_regions(struct device *dev, struct
> list_head *head)
> {
>     struct device_node *of_node = NULL;
>     struct device *parent_dev;
>     const __be32 *msi_map;
>     int num_mappings, i, err, len, resv = 0;
> 
>     /* walk up to find the parent with a msi-map property */
>     for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>         if (!parent_dev->of_node)
>             continue;
> 
>         /*
>          * Current logic to reserve ITS regions for only PCI root
>          * complex.
>          */
>         msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
>         if (msi_map) {
>             of_node = parent_dev->of_node;
>             break;
>         }
>     }
> 
>     if (!of_node)
>         return 0;
> 
>     num_mappings = of_count_phandle_with_args(of_node, "msi-map",
> NULL) / 4;
> 
>     for (i = 0; i < num_mappings; i++) {
>         int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>         struct iommu_resv_region *region;
>         struct device_node *msi_node;
>         struct resource resource;
>         u32 phandle;
> 
>         phandle = be32_to_cpup(msi_map + 1);
>         msi_node = of_find_node_by_phandle(phandle);
>         if (!msi_node)
>             return -ENODEV;
> 
>         /*
>          * There may be different types of msi-controller, so check
>          * for ITS.
>          */
>         if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
>             of_node_put(msi_node);
>             continue;
>         }
> 
>         err = of_address_to_resource(msi_node, 0, &resource);
> 
>         of_node_put(msi_node);
>         if (err)
>             return err;
> 
>         region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
>                          IOMMU_RESV_MSI);
>         if (region) {
>             list_add_tail(&region->list, head);
>             resv++;
>         }
>     }
> 
>     return (resv == num_mappings) ? resv : -ENODEV;
> }
> 
> Any feedback is appreciated.

Hi John,

I appreciate it is not trivial to make the code generic, part of the
snippet above can be shared between ACPI/IORT and OF, the only sticking
point is probably the "compatible" string that has to be parameterized
since having this code in generic IOMMU layer is a bit horrible to
have it ITS specific (and it is a problem that was existing already
in the original patch with the hardcoded ITS node type or function
name).

To sum it up the hook:

- has to be called from SMMU driver in a firmware agnostic way
- somehow it has to ascertain which interrupt controller "compatible"
  (which in IORT is a node type) to match against so the hook has to
  take an id of sorts

I need to go back and have a look at the original patch to see how
we can accommodate both OF/ACPI - certainly the region reservations
code can and should be shared.

Lorenzo

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

On Fri, Sep 01, 2017 at 09:46:00AM +0100, John Garry wrote:
> On 10/08/2017 18:27, Will Deacon wrote:
> >On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
> >>The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> >>platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.
> >>
> >>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.
> >>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.
> >>
> >>Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> >>---
> >> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >> 1 file changed, 22 insertions(+), 5 deletions(-)
> >
> >Please can you also add a devicetree binding with corresponding
> >documentation to enable this workaround on non-ACPI based systems too? It
> >should be straightforward if you update the arm_smmu_options table.
> >
> 
> Hi Will, Lorenzo, Robin,
> 
> I have created the patch to add DT support for this erratum.
> However, currently I have only added support for pci-based devices.
> I'm a bit stumped on how to add platform device support, or if we
> should also add support at all. And I would rather ask before
> sending the patches.
> 
> The specific issue is that I know of no platform device with an ITS
> msi-parent which we would want reserve, i.e. do not translate msi.
> And, if there were, how to do it.
> 
> The only platform devices I know off with msi ITS parents are SMMUv3
> and mbigen. For mbigen, the msi are not translated. Actually, as I
> see under IORT solution, for mbigen we don't reserve the hw msi
> region as the SMMUv3 does not have an ID mapping. And we have no
> equivalent ID mapping property for DT SMMUv3, so cannot create an
> equivalent check.
> 
> So here is how the DT iommu get reserved region function with only
> pci device support looks:
> 
> /**
>  * of_iommu_its_get_resv_regions - Reserved region driver helper
>  * @dev: Device from iommu_get_resv_regions()
>  * @list: Reserved region list from iommu_get_resv_regions()
>  *
>  * Returns: Number of reserved regions on success(0 if no associated ITS),
>  *          appropriate error value otherwise.
>  */
> int of_iommu_its_get_resv_regions(struct device *dev, struct
> list_head *head)
> {
>     struct device_node *of_node = NULL;
>     struct device *parent_dev;
>     const __be32 *msi_map;
>     int num_mappings, i, err, len, resv = 0;
> 
>     /* walk up to find the parent with a msi-map property */
>     for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>         if (!parent_dev->of_node)
>             continue;
> 
>         /*
>          * Current logic to reserve ITS regions for only PCI root
>          * complex.
>          */
>         msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
>         if (msi_map) {
>             of_node = parent_dev->of_node;
>             break;
>         }
>     }
> 
>     if (!of_node)
>         return 0;
> 
>     num_mappings = of_count_phandle_with_args(of_node, "msi-map",
> NULL) / 4;
> 
>     for (i = 0; i < num_mappings; i++) {
>         int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>         struct iommu_resv_region *region;
>         struct device_node *msi_node;
>         struct resource resource;
>         u32 phandle;
> 
>         phandle = be32_to_cpup(msi_map + 1);
>         msi_node = of_find_node_by_phandle(phandle);
>         if (!msi_node)
>             return -ENODEV;
> 
>         /*
>          * There may be different types of msi-controller, so check
>          * for ITS.
>          */
>         if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
>             of_node_put(msi_node);
>             continue;
>         }
> 
>         err = of_address_to_resource(msi_node, 0, &resource);
> 
>         of_node_put(msi_node);
>         if (err)
>             return err;
> 
>         region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
>                          IOMMU_RESV_MSI);
>         if (region) {
>             list_add_tail(&region->list, head);
>             resv++;
>         }
>     }
> 
>     return (resv == num_mappings) ? resv : -ENODEV;
> }
> 
> Any feedback is appreciated.

Hi John,

I appreciate it is not trivial to make the code generic, part of the
snippet above can be shared between ACPI/IORT and OF, the only sticking
point is probably the "compatible" string that has to be parameterized
since having this code in generic IOMMU layer is a bit horrible to
have it ITS specific (and it is a problem that was existing already
in the original patch with the hardcoded ITS node type or function
name).

To sum it up the hook:

- has to be called from SMMU driver in a firmware agnostic way
- somehow it has to ascertain which interrupt controller "compatible"
  (which in IORT is a node type) to match against so the hook has to
  take an id of sorts

I need to go back and have a look at the original patch to see how
we can accommodate both OF/ACPI - certainly the region reservations
code can and should be shared.

Lorenzo

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

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

On Fri, Sep 01, 2017 at 09:46:00AM +0100, John Garry wrote:
> On 10/08/2017 18:27, Will Deacon wrote:
> >On Wed, Aug 09, 2017 at 11:07:15AM +0100, Shameer Kolothum wrote:
> >>The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> >>platforms Hip06/Hip07 to support the SMMU mappings for MSI transactions.
> >>
> >>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.
> >>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.
> >>
> >>Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com>
> >>---
> >> drivers/iommu/arm-smmu-v3.c | 27 ++++++++++++++++++++++-----
> >> 1 file changed, 22 insertions(+), 5 deletions(-)
> >
> >Please can you also add a devicetree binding with corresponding
> >documentation to enable this workaround on non-ACPI based systems too? It
> >should be straightforward if you update the arm_smmu_options table.
> >
> 
> Hi Will, Lorenzo, Robin,
> 
> I have created the patch to add DT support for this erratum.
> However, currently I have only added support for pci-based devices.
> I'm a bit stumped on how to add platform device support, or if we
> should also add support at all. And I would rather ask before
> sending the patches.
> 
> The specific issue is that I know of no platform device with an ITS
> msi-parent which we would want reserve, i.e. do not translate msi.
> And, if there were, how to do it.
> 
> The only platform devices I know off with msi ITS parents are SMMUv3
> and mbigen. For mbigen, the msi are not translated. Actually, as I
> see under IORT solution, for mbigen we don't reserve the hw msi
> region as the SMMUv3 does not have an ID mapping. And we have no
> equivalent ID mapping property for DT SMMUv3, so cannot create an
> equivalent check.
> 
> So here is how the DT iommu get reserved region function with only
> pci device support looks:
> 
> /**
>  * of_iommu_its_get_resv_regions - Reserved region driver helper
>  * @dev: Device from iommu_get_resv_regions()
>  * @list: Reserved region list from iommu_get_resv_regions()
>  *
>  * Returns: Number of reserved regions on success(0 if no associated ITS),
>  *          appropriate error value otherwise.
>  */
> int of_iommu_its_get_resv_regions(struct device *dev, struct
> list_head *head)
> {
>     struct device_node *of_node = NULL;
>     struct device *parent_dev;
>     const __be32 *msi_map;
>     int num_mappings, i, err, len, resv = 0;
> 
>     /* walk up to find the parent with a msi-map property */
>     for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>         if (!parent_dev->of_node)
>             continue;
> 
>         /*
>          * Current logic to reserve ITS regions for only PCI root
>          * complex.
>          */
>         msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
>         if (msi_map) {
>             of_node = parent_dev->of_node;
>             break;
>         }
>     }
> 
>     if (!of_node)
>         return 0;
> 
>     num_mappings = of_count_phandle_with_args(of_node, "msi-map",
> NULL) / 4;
> 
>     for (i = 0; i < num_mappings; i++) {
>         int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>         struct iommu_resv_region *region;
>         struct device_node *msi_node;
>         struct resource resource;
>         u32 phandle;
> 
>         phandle = be32_to_cpup(msi_map + 1);
>         msi_node = of_find_node_by_phandle(phandle);
>         if (!msi_node)
>             return -ENODEV;
> 
>         /*
>          * There may be different types of msi-controller, so check
>          * for ITS.
>          */
>         if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
>             of_node_put(msi_node);
>             continue;
>         }
> 
>         err = of_address_to_resource(msi_node, 0, &resource);
> 
>         of_node_put(msi_node);
>         if (err)
>             return err;
> 
>         region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
>                          IOMMU_RESV_MSI);
>         if (region) {
>             list_add_tail(&region->list, head);
>             resv++;
>         }
>     }
> 
>     return (resv == num_mappings) ? resv : -ENODEV;
> }
> 
> Any feedback is appreciated.

Hi John,

I appreciate it is not trivial to make the code generic, part of the
snippet above can be shared between ACPI/IORT and OF, the only sticking
point is probably the "compatible" string that has to be parameterized
since having this code in generic IOMMU layer is a bit horrible to
have it ITS specific (and it is a problem that was existing already
in the original patch with the hardcoded ITS node type or function
name).

To sum it up the hook:

- has to be called from SMMU driver in a firmware agnostic way
- somehow it has to ascertain which interrupt controller "compatible"
  (which in IORT is a node type) to match against so the hook has to
  take an id of sorts

I need to go back and have a look at the original patch to see how
we can accommodate both OF/ACPI - certainly the region reservations
code can and should be shared.

Lorenzo

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

>>
>> Hi Will, Lorenzo, Robin,
>>
>> I have created the patch to add DT support for this erratum.
>> However, currently I have only added support for pci-based devices.
>> I'm a bit stumped on how to add platform device support, or if we
>> should also add support at all. And I would rather ask before
>> sending the patches.
>>
>> The specific issue is that I know of no platform device with an ITS
>> msi-parent which we would want reserve, i.e. do not translate msi.
>> And, if there were, how to do it.
>>
>> The only platform devices I know off with msi ITS parents are SMMUv3
>> and mbigen. For mbigen, the msi are not translated. Actually, as I
>> see under IORT solution, for mbigen we don't reserve the hw msi
>> region as the SMMUv3 does not have an ID mapping. And we have no
>> equivalent ID mapping property for DT SMMUv3, so cannot create an
>> equivalent check.
>>
>> So here is how the DT iommu get reserved region function with only
>> pci device support looks:
>>
>> /**
>>  * of_iommu_its_get_resv_regions - Reserved region driver helper
>>  * @dev: Device from iommu_get_resv_regions()
>>  * @list: Reserved region list from iommu_get_resv_regions()
>>  *
>>  * Returns: Number of reserved regions on success(0 if no associated ITS),
>>  *          appropriate error value otherwise.
>>  */
>> int of_iommu_its_get_resv_regions(struct device *dev, struct
>> list_head *head)
>> {
>>     struct device_node *of_node = NULL;
>>     struct device *parent_dev;
>>     const __be32 *msi_map;
>>     int num_mappings, i, err, len, resv = 0;
>>
>>     /* walk up to find the parent with a msi-map property */
>>     for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>>         if (!parent_dev->of_node)
>>             continue;
>>
>>         /*
>>          * Current logic to reserve ITS regions for only PCI root
>>          * complex.
>>          */
>>         msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
>>         if (msi_map) {
>>             of_node = parent_dev->of_node;
>>             break;
>>         }
>>     }
>>
>>     if (!of_node)
>>         return 0;
>>
>>     num_mappings = of_count_phandle_with_args(of_node, "msi-map",
>> NULL) / 4;
>>
>>     for (i = 0; i < num_mappings; i++) {
>>         int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>>         struct iommu_resv_region *region;
>>         struct device_node *msi_node;
>>         struct resource resource;
>>         u32 phandle;
>>
>>         phandle = be32_to_cpup(msi_map + 1);
>>         msi_node = of_find_node_by_phandle(phandle);
>>         if (!msi_node)
>>             return -ENODEV;
>>
>>         /*
>>          * There may be different types of msi-controller, so check
>>          * for ITS.
>>          */
>>         if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
>>             of_node_put(msi_node);
>>             continue;
>>         }
>>
>>         err = of_address_to_resource(msi_node, 0, &resource);
>>
>>         of_node_put(msi_node);
>>         if (err)
>>             return err;
>>
>>         region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
>>                          IOMMU_RESV_MSI);
>>         if (region) {
>>             list_add_tail(&region->list, head);
>>             resv++;
>>         }
>>     }
>>
>>     return (resv == num_mappings) ? resv : -ENODEV;
>> }
>>
>> Any feedback is appreciated.
>
> Hi John,
>
> I appreciate it is not trivial to make the code generic, part of the
> snippet above can be shared between ACPI/IORT and OF, the only sticking
> point is probably the "compatible" string that has to be parameterized
> since having this code in generic IOMMU layer is a bit horrible to
> have it ITS specific (and it is a problem that was existing already
> in the original patch with the hardcoded ITS node type or function
> name).

Hi Lorenzo,

Agreed, checking the ITS compatible string in IOMMU code is not nice. 
However the function is just trying to replicate our ACPI version, which 
calls IORT code directly, and this is ITS specific. Anyway, we can make 
it generic.

>
> To sum it up the hook:
>
> - has to be called from SMMU driver in a firmware agnostic way

ok

> - somehow it has to ascertain which interrupt controller "compatible"
>   (which in IORT is a node type) to match against so the hook has to
>   take an id of sorts

OK

I will mention 2 more points after discussion on OF solution with Shameer:
- for platform device, we can add suppport by checking for the devices 
msi-parent property
- for both pci and platform device, we should check device rid for 
msi-controller also, like IORT solution

BTW, even though merge window is open, we would like to send some 
solution to the lists this week, so any outstanding topics can 
potentially be discussed at LPC next week. Let me know if you're unhappy 
about this.

All the best,
John


>
> I need to go back and have a look at the original patch to see how
> we can accommodate both OF/ACPI - certainly the region reservations
> code can and should be shared.
>
> Lorenzo
>
> .
>

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

>>
>> Hi Will, Lorenzo, Robin,
>>
>> I have created the patch to add DT support for this erratum.
>> However, currently I have only added support for pci-based devices.
>> I'm a bit stumped on how to add platform device support, or if we
>> should also add support at all. And I would rather ask before
>> sending the patches.
>>
>> The specific issue is that I know of no platform device with an ITS
>> msi-parent which we would want reserve, i.e. do not translate msi.
>> And, if there were, how to do it.
>>
>> The only platform devices I know off with msi ITS parents are SMMUv3
>> and mbigen. For mbigen, the msi are not translated. Actually, as I
>> see under IORT solution, for mbigen we don't reserve the hw msi
>> region as the SMMUv3 does not have an ID mapping. And we have no
>> equivalent ID mapping property for DT SMMUv3, so cannot create an
>> equivalent check.
>>
>> So here is how the DT iommu get reserved region function with only
>> pci device support looks:
>>
>> /**
>>  * of_iommu_its_get_resv_regions - Reserved region driver helper
>>  * @dev: Device from iommu_get_resv_regions()
>>  * @list: Reserved region list from iommu_get_resv_regions()
>>  *
>>  * Returns: Number of reserved regions on success(0 if no associated ITS),
>>  *          appropriate error value otherwise.
>>  */
>> int of_iommu_its_get_resv_regions(struct device *dev, struct
>> list_head *head)
>> {
>>     struct device_node *of_node = NULL;
>>     struct device *parent_dev;
>>     const __be32 *msi_map;
>>     int num_mappings, i, err, len, resv = 0;
>>
>>     /* walk up to find the parent with a msi-map property */
>>     for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>>         if (!parent_dev->of_node)
>>             continue;
>>
>>         /*
>>          * Current logic to reserve ITS regions for only PCI root
>>          * complex.
>>          */
>>         msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
>>         if (msi_map) {
>>             of_node = parent_dev->of_node;
>>             break;
>>         }
>>     }
>>
>>     if (!of_node)
>>         return 0;
>>
>>     num_mappings = of_count_phandle_with_args(of_node, "msi-map",
>> NULL) / 4;
>>
>>     for (i = 0; i < num_mappings; i++) {
>>         int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>>         struct iommu_resv_region *region;
>>         struct device_node *msi_node;
>>         struct resource resource;
>>         u32 phandle;
>>
>>         phandle = be32_to_cpup(msi_map + 1);
>>         msi_node = of_find_node_by_phandle(phandle);
>>         if (!msi_node)
>>             return -ENODEV;
>>
>>         /*
>>          * There may be different types of msi-controller, so check
>>          * for ITS.
>>          */
>>         if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
>>             of_node_put(msi_node);
>>             continue;
>>         }
>>
>>         err = of_address_to_resource(msi_node, 0, &resource);
>>
>>         of_node_put(msi_node);
>>         if (err)
>>             return err;
>>
>>         region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
>>                          IOMMU_RESV_MSI);
>>         if (region) {
>>             list_add_tail(&region->list, head);
>>             resv++;
>>         }
>>     }
>>
>>     return (resv == num_mappings) ? resv : -ENODEV;
>> }
>>
>> Any feedback is appreciated.
>
> Hi John,
>
> I appreciate it is not trivial to make the code generic, part of the
> snippet above can be shared between ACPI/IORT and OF, the only sticking
> point is probably the "compatible" string that has to be parameterized
> since having this code in generic IOMMU layer is a bit horrible to
> have it ITS specific (and it is a problem that was existing already
> in the original patch with the hardcoded ITS node type or function
> name).

Hi Lorenzo,

Agreed, checking the ITS compatible string in IOMMU code is not nice. 
However the function is just trying to replicate our ACPI version, which 
calls IORT code directly, and this is ITS specific. Anyway, we can make 
it generic.

>
> To sum it up the hook:
>
> - has to be called from SMMU driver in a firmware agnostic way

ok

> - somehow it has to ascertain which interrupt controller "compatible"
>   (which in IORT is a node type) to match against so the hook has to
>   take an id of sorts

OK

I will mention 2 more points after discussion on OF solution with Shameer:
- for platform device, we can add suppport by checking for the devices 
msi-parent property
- for both pci and platform device, we should check device rid for 
msi-controller also, like IORT solution

BTW, even though merge window is open, we would like to send some 
solution to the lists this week, so any outstanding topics can 
potentially be discussed at LPC next week. Let me know if you're unhappy 
about this.

All the best,
John


>
> I need to go back and have a look at the original patch to see how
> we can accommodate both OF/ACPI - certainly the region reservations
> code can and should be shared.
>
> Lorenzo
>
> .
>

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[John Garry]

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

>>
>> Hi Will, Lorenzo, Robin,
>>
>> I have created the patch to add DT support for this erratum.
>> However, currently I have only added support for pci-based devices.
>> I'm a bit stumped on how to add platform device support, or if we
>> should also add support at all. And I would rather ask before
>> sending the patches.
>>
>> The specific issue is that I know of no platform device with an ITS
>> msi-parent which we would want reserve, i.e. do not translate msi.
>> And, if there were, how to do it.
>>
>> The only platform devices I know off with msi ITS parents are SMMUv3
>> and mbigen. For mbigen, the msi are not translated. Actually, as I
>> see under IORT solution, for mbigen we don't reserve the hw msi
>> region as the SMMUv3 does not have an ID mapping. And we have no
>> equivalent ID mapping property for DT SMMUv3, so cannot create an
>> equivalent check.
>>
>> So here is how the DT iommu get reserved region function with only
>> pci device support looks:
>>
>> /**
>>  * of_iommu_its_get_resv_regions - Reserved region driver helper
>>  * @dev: Device from iommu_get_resv_regions()
>>  * @list: Reserved region list from iommu_get_resv_regions()
>>  *
>>  * Returns: Number of reserved regions on success(0 if no associated ITS),
>>  *          appropriate error value otherwise.
>>  */
>> int of_iommu_its_get_resv_regions(struct device *dev, struct
>> list_head *head)
>> {
>>     struct device_node *of_node = NULL;
>>     struct device *parent_dev;
>>     const __be32 *msi_map;
>>     int num_mappings, i, err, len, resv = 0;
>>
>>     /* walk up to find the parent with a msi-map property */
>>     for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>>         if (!parent_dev->of_node)
>>             continue;
>>
>>         /*
>>          * Current logic to reserve ITS regions for only PCI root
>>          * complex.
>>          */
>>         msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
>>         if (msi_map) {
>>             of_node = parent_dev->of_node;
>>             break;
>>         }
>>     }
>>
>>     if (!of_node)
>>         return 0;
>>
>>     num_mappings = of_count_phandle_with_args(of_node, "msi-map",
>> NULL) / 4;
>>
>>     for (i = 0; i < num_mappings; i++) {
>>         int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>>         struct iommu_resv_region *region;
>>         struct device_node *msi_node;
>>         struct resource resource;
>>         u32 phandle;
>>
>>         phandle = be32_to_cpup(msi_map + 1);
>>         msi_node = of_find_node_by_phandle(phandle);
>>         if (!msi_node)
>>             return -ENODEV;
>>
>>         /*
>>          * There may be different types of msi-controller, so check
>>          * for ITS.
>>          */
>>         if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
>>             of_node_put(msi_node);
>>             continue;
>>         }
>>
>>         err = of_address_to_resource(msi_node, 0, &resource);
>>
>>         of_node_put(msi_node);
>>         if (err)
>>             return err;
>>
>>         region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
>>                          IOMMU_RESV_MSI);
>>         if (region) {
>>             list_add_tail(&region->list, head);
>>             resv++;
>>         }
>>     }
>>
>>     return (resv == num_mappings) ? resv : -ENODEV;
>> }
>>
>> Any feedback is appreciated.
>
> Hi John,
>
> I appreciate it is not trivial to make the code generic, part of the
> snippet above can be shared between ACPI/IORT and OF, the only sticking
> point is probably the "compatible" string that has to be parameterized
> since having this code in generic IOMMU layer is a bit horrible to
> have it ITS specific (and it is a problem that was existing already
> in the original patch with the hardcoded ITS node type or function
> name).

Hi Lorenzo,

Agreed, checking the ITS compatible string in IOMMU code is not nice. 
However the function is just trying to replicate our ACPI version, which 
calls IORT code directly, and this is ITS specific. Anyway, we can make 
it generic.

>
> To sum it up the hook:
>
> - has to be called from SMMU driver in a firmware agnostic way

ok

> - somehow it has to ascertain which interrupt controller "compatible"
>   (which in IORT is a node type) to match against so the hook has to
>   take an id of sorts

OK

I will mention 2 more points after discussion on OF solution with Shameer:
- for platform device, we can add suppport by checking for the devices 
msi-parent property
- for both pci and platform device, we should check device rid for 
msi-controller also, like IORT solution

BTW, even though merge window is open, we would like to send some 
solution to the lists this week, so any outstanding topics can 
potentially be discussed at LPC next week. Let me know if you're unhappy 
about this.

All the best,
John


>
> I need to go back and have a look at the original patch to see how
> we can accommodate both OF/ACPI - certainly the region reservations
> code can and should be shared.
>
> Lorenzo
>
> .
>

Re: [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

On Tue, Sep 05, 2017 at 12:07:59PM +0100, John Garry wrote:
> >>
> >>Hi Will, Lorenzo, Robin,
> >>
> >>I have created the patch to add DT support for this erratum.
> >>However, currently I have only added support for pci-based devices.
> >>I'm a bit stumped on how to add platform device support, or if we
> >>should also add support at all. And I would rather ask before
> >>sending the patches.
> >>
> >>The specific issue is that I know of no platform device with an ITS
> >>msi-parent which we would want reserve, i.e. do not translate msi.
> >>And, if there were, how to do it.
> >>
> >>The only platform devices I know off with msi ITS parents are SMMUv3
> >>and mbigen. For mbigen, the msi are not translated. Actually, as I
> >>see under IORT solution, for mbigen we don't reserve the hw msi
> >>region as the SMMUv3 does not have an ID mapping. And we have no
> >>equivalent ID mapping property for DT SMMUv3, so cannot create an
> >>equivalent check.
> >>
> >>So here is how the DT iommu get reserved region function with only
> >>pci device support looks:
> >>
> >>/**
> >> * of_iommu_its_get_resv_regions - Reserved region driver helper
> >> * @dev: Device from iommu_get_resv_regions()
> >> * @list: Reserved region list from iommu_get_resv_regions()
> >> *
> >> * Returns: Number of reserved regions on success(0 if no associated ITS),
> >> *          appropriate error value otherwise.
> >> */
> >>int of_iommu_its_get_resv_regions(struct device *dev, struct
> >>list_head *head)
> >>{
> >>    struct device_node *of_node = NULL;
> >>    struct device *parent_dev;
> >>    const __be32 *msi_map;
> >>    int num_mappings, i, err, len, resv = 0;
> >>
> >>    /* walk up to find the parent with a msi-map property */
> >>    for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >>        if (!parent_dev->of_node)
> >>            continue;
> >>
> >>        /*
> >>         * Current logic to reserve ITS regions for only PCI root
> >>         * complex.
> >>         */
> >>        msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
> >>        if (msi_map) {
> >>            of_node = parent_dev->of_node;
> >>            break;
> >>        }
> >>    }
> >>
> >>    if (!of_node)
> >>        return 0;
> >>
> >>    num_mappings = of_count_phandle_with_args(of_node, "msi-map",
> >>NULL) / 4;
> >>
> >>    for (i = 0; i < num_mappings; i++) {
> >>        int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> >>        struct iommu_resv_region *region;
> >>        struct device_node *msi_node;
> >>        struct resource resource;
> >>        u32 phandle;
> >>
> >>        phandle = be32_to_cpup(msi_map + 1);
> >>        msi_node = of_find_node_by_phandle(phandle);
> >>        if (!msi_node)
> >>            return -ENODEV;
> >>
> >>        /*
> >>         * There may be different types of msi-controller, so check
> >>         * for ITS.
> >>         */
> >>        if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
> >>            of_node_put(msi_node);
> >>            continue;
> >>        }
> >>
> >>        err = of_address_to_resource(msi_node, 0, &resource);
> >>
> >>        of_node_put(msi_node);
> >>        if (err)
> >>            return err;
> >>
> >>        region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
> >>                         IOMMU_RESV_MSI);
> >>        if (region) {
> >>            list_add_tail(&region->list, head);
> >>            resv++;
> >>        }
> >>    }
> >>
> >>    return (resv == num_mappings) ? resv : -ENODEV;
> >>}
> >>
> >>Any feedback is appreciated.
> >
> >Hi John,
> >
> >I appreciate it is not trivial to make the code generic, part of the
> >snippet above can be shared between ACPI/IORT and OF, the only sticking
> >point is probably the "compatible" string that has to be parameterized
> >since having this code in generic IOMMU layer is a bit horrible to
> >have it ITS specific (and it is a problem that was existing already
> >in the original patch with the hardcoded ITS node type or function
> >name).
> 
> Hi Lorenzo,
> 
> Agreed, checking the ITS compatible string in IOMMU code is not
> nice. However the function is just trying to replicate our ACPI
> version, which calls IORT code directly, and this is ITS specific.
> Anyway, we can make it generic.
> 
> >
> >To sum it up the hook:
> >
> >- has to be called from SMMU driver in a firmware agnostic way
> 
> ok
> 
> >- somehow it has to ascertain which interrupt controller "compatible"
> >  (which in IORT is a node type) to match against so the hook has to
> >  take an id of sorts
> 
> OK
> 
> I will mention 2 more points after discussion on OF solution with Shameer:
> - for platform device, we can add suppport by checking for the
> devices msi-parent property
> - for both pci and platform device, we should check device rid for
> msi-controller also, like IORT solution
> 
> BTW, even though merge window is open, we would like to send some
> solution to the lists this week, so any outstanding topics can
> potentially be discussed at LPC next week. Let me know if you're
> unhappy about this.

I am happy to find a way forward for this next week, posting patches
this week would not help much with the merge window ongoing, I think
it is better to try to find a way forward and post the resulting code
after reaching an agreement, at -rc1 (or right after the IOMMU PR is
merged).

Thanks,
Lorenzo

[PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

On Tue, Sep 05, 2017 at 12:07:59PM +0100, John Garry wrote:
> >>
> >>Hi Will, Lorenzo, Robin,
> >>
> >>I have created the patch to add DT support for this erratum.
> >>However, currently I have only added support for pci-based devices.
> >>I'm a bit stumped on how to add platform device support, or if we
> >>should also add support at all. And I would rather ask before
> >>sending the patches.
> >>
> >>The specific issue is that I know of no platform device with an ITS
> >>msi-parent which we would want reserve, i.e. do not translate msi.
> >>And, if there were, how to do it.
> >>
> >>The only platform devices I know off with msi ITS parents are SMMUv3
> >>and mbigen. For mbigen, the msi are not translated. Actually, as I
> >>see under IORT solution, for mbigen we don't reserve the hw msi
> >>region as the SMMUv3 does not have an ID mapping. And we have no
> >>equivalent ID mapping property for DT SMMUv3, so cannot create an
> >>equivalent check.
> >>
> >>So here is how the DT iommu get reserved region function with only
> >>pci device support looks:
> >>
> >>/**
> >> * of_iommu_its_get_resv_regions - Reserved region driver helper
> >> * @dev: Device from iommu_get_resv_regions()
> >> * @list: Reserved region list from iommu_get_resv_regions()
> >> *
> >> * Returns: Number of reserved regions on success(0 if no associated ITS),
> >> *          appropriate error value otherwise.
> >> */
> >>int of_iommu_its_get_resv_regions(struct device *dev, struct
> >>list_head *head)
> >>{
> >>    struct device_node *of_node = NULL;
> >>    struct device *parent_dev;
> >>    const __be32 *msi_map;
> >>    int num_mappings, i, err, len, resv = 0;
> >>
> >>    /* walk up to find the parent with a msi-map property */
> >>    for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >>        if (!parent_dev->of_node)
> >>            continue;
> >>
> >>        /*
> >>         * Current logic to reserve ITS regions for only PCI root
> >>         * complex.
> >>         */
> >>        msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
> >>        if (msi_map) {
> >>            of_node = parent_dev->of_node;
> >>            break;
> >>        }
> >>    }
> >>
> >>    if (!of_node)
> >>        return 0;
> >>
> >>    num_mappings = of_count_phandle_with_args(of_node, "msi-map",
> >>NULL) / 4;
> >>
> >>    for (i = 0; i < num_mappings; i++) {
> >>        int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> >>        struct iommu_resv_region *region;
> >>        struct device_node *msi_node;
> >>        struct resource resource;
> >>        u32 phandle;
> >>
> >>        phandle = be32_to_cpup(msi_map + 1);
> >>        msi_node = of_find_node_by_phandle(phandle);
> >>        if (!msi_node)
> >>            return -ENODEV;
> >>
> >>        /*
> >>         * There may be different types of msi-controller, so check
> >>         * for ITS.
> >>         */
> >>        if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
> >>            of_node_put(msi_node);
> >>            continue;
> >>        }
> >>
> >>        err = of_address_to_resource(msi_node, 0, &resource);
> >>
> >>        of_node_put(msi_node);
> >>        if (err)
> >>            return err;
> >>
> >>        region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
> >>                         IOMMU_RESV_MSI);
> >>        if (region) {
> >>            list_add_tail(&region->list, head);
> >>            resv++;
> >>        }
> >>    }
> >>
> >>    return (resv == num_mappings) ? resv : -ENODEV;
> >>}
> >>
> >>Any feedback is appreciated.
> >
> >Hi John,
> >
> >I appreciate it is not trivial to make the code generic, part of the
> >snippet above can be shared between ACPI/IORT and OF, the only sticking
> >point is probably the "compatible" string that has to be parameterized
> >since having this code in generic IOMMU layer is a bit horrible to
> >have it ITS specific (and it is a problem that was existing already
> >in the original patch with the hardcoded ITS node type or function
> >name).
> 
> Hi Lorenzo,
> 
> Agreed, checking the ITS compatible string in IOMMU code is not
> nice. However the function is just trying to replicate our ACPI
> version, which calls IORT code directly, and this is ITS specific.
> Anyway, we can make it generic.
> 
> >
> >To sum it up the hook:
> >
> >- has to be called from SMMU driver in a firmware agnostic way
> 
> ok
> 
> >- somehow it has to ascertain which interrupt controller "compatible"
> >  (which in IORT is a node type) to match against so the hook has to
> >  take an id of sorts
> 
> OK
> 
> I will mention 2 more points after discussion on OF solution with Shameer:
> - for platform device, we can add suppport by checking for the
> devices msi-parent property
> - for both pci and platform device, we should check device rid for
> msi-controller also, like IORT solution
> 
> BTW, even though merge window is open, we would like to send some
> solution to the lists this week, so any outstanding topics can
> potentially be discussed at LPC next week. Let me know if you're
> unhappy about this.

I am happy to find a way forward for this next week, posting patches
this week would not help much with the merge window ongoing, I think
it is better to try to find a way forward and post the resulting code
after reaching an agreement, at -rc1 (or right after the IOMMU PR is
merged).

Thanks,
Lorenzo

Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

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

On Tue, Sep 05, 2017 at 12:07:59PM +0100, John Garry wrote:
> >>
> >>Hi Will, Lorenzo, Robin,
> >>
> >>I have created the patch to add DT support for this erratum.
> >>However, currently I have only added support for pci-based devices.
> >>I'm a bit stumped on how to add platform device support, or if we
> >>should also add support at all. And I would rather ask before
> >>sending the patches.
> >>
> >>The specific issue is that I know of no platform device with an ITS
> >>msi-parent which we would want reserve, i.e. do not translate msi.
> >>And, if there were, how to do it.
> >>
> >>The only platform devices I know off with msi ITS parents are SMMUv3
> >>and mbigen. For mbigen, the msi are not translated. Actually, as I
> >>see under IORT solution, for mbigen we don't reserve the hw msi
> >>region as the SMMUv3 does not have an ID mapping. And we have no
> >>equivalent ID mapping property for DT SMMUv3, so cannot create an
> >>equivalent check.
> >>
> >>So here is how the DT iommu get reserved region function with only
> >>pci device support looks:
> >>
> >>/**
> >> * of_iommu_its_get_resv_regions - Reserved region driver helper
> >> * @dev: Device from iommu_get_resv_regions()
> >> * @list: Reserved region list from iommu_get_resv_regions()
> >> *
> >> * Returns: Number of reserved regions on success(0 if no associated ITS),
> >> *          appropriate error value otherwise.
> >> */
> >>int of_iommu_its_get_resv_regions(struct device *dev, struct
> >>list_head *head)
> >>{
> >>    struct device_node *of_node = NULL;
> >>    struct device *parent_dev;
> >>    const __be32 *msi_map;
> >>    int num_mappings, i, err, len, resv = 0;
> >>
> >>    /* walk up to find the parent with a msi-map property */
> >>    for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >>        if (!parent_dev->of_node)
> >>            continue;
> >>
> >>        /*
> >>         * Current logic to reserve ITS regions for only PCI root
> >>         * complex.
> >>         */
> >>        msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
> >>        if (msi_map) {
> >>            of_node = parent_dev->of_node;
> >>            break;
> >>        }
> >>    }
> >>
> >>    if (!of_node)
> >>        return 0;
> >>
> >>    num_mappings = of_count_phandle_with_args(of_node, "msi-map",
> >>NULL) / 4;
> >>
> >>    for (i = 0; i < num_mappings; i++) {
> >>        int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> >>        struct iommu_resv_region *region;
> >>        struct device_node *msi_node;
> >>        struct resource resource;
> >>        u32 phandle;
> >>
> >>        phandle = be32_to_cpup(msi_map + 1);
> >>        msi_node = of_find_node_by_phandle(phandle);
> >>        if (!msi_node)
> >>            return -ENODEV;
> >>
> >>        /*
> >>         * There may be different types of msi-controller, so check
> >>         * for ITS.
> >>         */
> >>        if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
> >>            of_node_put(msi_node);
> >>            continue;
> >>        }
> >>
> >>        err = of_address_to_resource(msi_node, 0, &resource);
> >>
> >>        of_node_put(msi_node);
> >>        if (err)
> >>            return err;
> >>
> >>        region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
> >>                         IOMMU_RESV_MSI);
> >>        if (region) {
> >>            list_add_tail(&region->list, head);
> >>            resv++;
> >>        }
> >>    }
> >>
> >>    return (resv == num_mappings) ? resv : -ENODEV;
> >>}
> >>
> >>Any feedback is appreciated.
> >
> >Hi John,
> >
> >I appreciate it is not trivial to make the code generic, part of the
> >snippet above can be shared between ACPI/IORT and OF, the only sticking
> >point is probably the "compatible" string that has to be parameterized
> >since having this code in generic IOMMU layer is a bit horrible to
> >have it ITS specific (and it is a problem that was existing already
> >in the original patch with the hardcoded ITS node type or function
> >name).
> 
> Hi Lorenzo,
> 
> Agreed, checking the ITS compatible string in IOMMU code is not
> nice. However the function is just trying to replicate our ACPI
> version, which calls IORT code directly, and this is ITS specific.
> Anyway, we can make it generic.
> 
> >
> >To sum it up the hook:
> >
> >- has to be called from SMMU driver in a firmware agnostic way
> 
> ok
> 
> >- somehow it has to ascertain which interrupt controller "compatible"
> >  (which in IORT is a node type) to match against so the hook has to
> >  take an id of sorts
> 
> OK
> 
> I will mention 2 more points after discussion on OF solution with Shameer:
> - for platform device, we can add suppport by checking for the
> devices msi-parent property
> - for both pci and platform device, we should check device rid for
> msi-controller also, like IORT solution
> 
> BTW, even though merge window is open, we would like to send some
> solution to the lists this week, so any outstanding topics can
> potentially be discussed at LPC next week. Let me know if you're
> unhappy about this.

I am happy to find a way forward for this next week, posting patches
this week would not help much with the merge window ongoing, I think
it is better to try to find a way forward and post the resulting code
after reaching an agreement, at -rc1 (or right after the IOMMU PR is
merged).

Thanks,
Lorenzo