[RFCv2 0/2] iommu/smmu-v3: Workaround for hisilicon 161010801 erratum(reserve HW MSI)

[RFCv2 0/2] iommu/smmu-v3: Workaround for hisilicon 161010801 erratum(reserve HW MSI)

[shameer]

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.

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.

To implement this quirk, the following changes are incorporated:

1. Added a generic helper function to IORT code to retrieve the
   associated ITS base address from a device IORT node.
2. Added quirk to SMMUv3 to retrieve the HW ITS address and replace
   the default SW MSI reserve address based on the IORT SMMU model.

This is based on the following patches:
1. https://patchwork.kernel.org/patch/9740733/
2. https://patchwork.kernel.org/patch/9730491/

Thanks,
Shameer

RFC v1 ---> 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 (2):
  acpi:iort: Add new helper function to retrieve ITS base addr from IORT
    node
  iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

 drivers/acpi/arm64/iort.c        | 47 +++++++++++++++++++++++++++++++++++---
 drivers/iommu/arm-smmu-v3.c      | 49 +++++++++++++++++++++++++++++++++++++---
 drivers/irqchip/irq-gic-v3-its.c |  3 ++-
 include/linux/acpi_iort.h        |  8 ++++++-
 4 files changed, 99 insertions(+), 8 deletions(-)

-- 
1.9.1

[RFCv2 0/2] iommu/smmu-v3: Workaround for hisilicon 161010801 erratum(reserve HW MSI)

[shameer]

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.

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.

To implement this quirk, the following changes are incorporated:

1. Added a generic helper function to IORT code to retrieve the
   associated ITS base address from a device IORT node.
2. Added quirk to SMMUv3 to retrieve the HW ITS address and replace
   the default SW MSI reserve address based on the IORT SMMU model.

This is based on the following patches:
1. https://patchwork.kernel.org/patch/9740733/
2. https://patchwork.kernel.org/patch/9730491/

Thanks,
Shameer

RFC v1 ---> 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 (2):
  acpi:iort: Add new helper function to retrieve ITS base addr from IORT
    node
  iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

 drivers/acpi/arm64/iort.c        | 47 +++++++++++++++++++++++++++++++++++---
 drivers/iommu/arm-smmu-v3.c      | 49 +++++++++++++++++++++++++++++++++++++---
 drivers/irqchip/irq-gic-v3-its.c |  3 ++-
 include/linux/acpi_iort.h        |  8 ++++++-
 4 files changed, 99 insertions(+), 8 deletions(-)

-- 
1.9.1

[RFCv2 1/2] acpi:iort: Add new helper function to retrieve ITS base addr from dev IORT node

[shameer]

This provides a helper function to find and retrieve the ITS
base address from the ID mappings array reference of a device
IORT node(if any).

This is used in the subsequent patch to retrieve the ITS base 
address associated with a pci dev IORT node.

Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
---
 drivers/acpi/arm64/iort.c        | 47 +++++++++++++++++++++++++++++++++++++---
 drivers/irqchip/irq-gic-v3-its.c |  3 ++-
 include/linux/acpi_iort.h        |  8 ++++++-
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index c5fecf9..12d7347 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -34,6 +34,7 @@
 struct iort_its_msi_chip {
 	struct list_head	list;
 	struct fwnode_handle	*fw_node;
+	u64			base_addr;
 	u32			translation_id;
 };
 
@@ -132,13 +133,14 @@ typedef acpi_status (*iort_find_node_callback)
 
 /**
  * iort_register_domain_token() - register domain token and related ITS ID
- * to the list from where we can get it back later on.
+ * and base address to the list from where we can get it back later on.
  * @trans_id: ITS ID.
  * @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, u64 base,
+			       struct fwnode_handle *fw_node)
 {
 	struct iort_its_msi_chip *its_msi_chip;
 
@@ -148,6 +150,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);
@@ -370,7 +373,6 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 
 		if (!node->mapping_offset || !node->mapping_count)
 			goto fail_map;
-
 		map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
 				   node->mapping_offset);
 
@@ -491,6 +493,45 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
 	return -ENODEV;
 }
 
+int iort_dev_find_its_base(struct device *dev, u32 req_id,
+			   unsigned int idx, u64 *its_base)
+{
+	struct acpi_iort_its_group *its;
+	struct acpi_iort_node *node;
+	struct iort_its_msi_chip *its_msi_chip;
+	u32  trans_id;
+
+	node = iort_find_dev_node(dev);
+	if (!node)
+		return -ENXIO;
+
+	node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE);
+	if (!node)
+		return -ENXIO;
+
+	/* Move to ITS specific data */
+	its = (struct acpi_iort_its_group *)node->node_data;
+	if (idx > its->its_count) {
+		dev_err(dev, "requested ITS ID index [%d] is greater than available [%d]\n",
+			idx, its->its_count);
+		return -ENXIO;
+	}
+
+	trans_id = its->identifiers[idx];
+
+	spin_lock(&iort_msi_chip_lock);
+	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {
+		if (its_msi_chip->translation_id == trans_id) {
+			*its_base = its_msi_chip->base_addr;
+			spin_unlock(&iort_msi_chip_lock);
+			return 0;
+		}
+	}
+	spin_unlock(&iort_msi_chip_lock);
+
+	return -ENXIO;
+}
+
 /**
  * iort_dev_find_its_id() - Find the ITS identifier for a device
  * @dev: The device.
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 45ea1933..c45a2ad 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1854,7 +1854,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 3ff9ace..bf7b53d 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, u64 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
@@ -36,6 +37,8 @@
 struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
 void acpi_configure_pmsi_domain(struct device *dev);
 int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
+int iort_dev_find_its_base(struct device *dev, u32 req_id,
+			   unsigned int idx, u64 *its_base);
 /* IOMMU interface */
 void iort_set_dma_mask(struct device *dev);
 const struct iommu_ops *iort_iommu_configure(struct device *dev);
@@ -48,6 +51,9 @@ static inline struct irq_domain *iort_get_device_domain(struct device *dev,
 							u32 req_id)
 { return NULL; }
 static inline void acpi_configure_pmsi_domain(struct device *dev) { }
+int iort_dev_find_its_base(struct device *dev, u32 req_id,
+			   unsigned int idx, u64 *its_base)
+{ return -ENOSYS; }
 /* IOMMU interface */
 static inline void iort_set_dma_mask(struct device *dev) { }
 static inline
-- 
1.9.1

[RFCv2 1/2] acpi:iort: Add new helper function to retrieve ITS base addr from dev IORT node

[shameer]

This provides a helper function to find and retrieve the ITS
base address from the ID mappings array reference of a device
IORT node(if any).

This is used in the subsequent patch to retrieve the ITS base 
address associated with a pci dev IORT node.

Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
---
 drivers/acpi/arm64/iort.c        | 47 +++++++++++++++++++++++++++++++++++++---
 drivers/irqchip/irq-gic-v3-its.c |  3 ++-
 include/linux/acpi_iort.h        |  8 ++++++-
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index c5fecf9..12d7347 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -34,6 +34,7 @@
 struct iort_its_msi_chip {
 	struct list_head	list;
 	struct fwnode_handle	*fw_node;
+	u64			base_addr;
 	u32			translation_id;
 };
 
@@ -132,13 +133,14 @@ typedef acpi_status (*iort_find_node_callback)
 
 /**
  * iort_register_domain_token() - register domain token and related ITS ID
- * to the list from where we can get it back later on.
+ * and base address to the list from where we can get it back later on.
  * @trans_id: ITS ID.
  * @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, u64 base,
+			       struct fwnode_handle *fw_node)
 {
 	struct iort_its_msi_chip *its_msi_chip;
 
@@ -148,6 +150,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);
@@ -370,7 +373,6 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 
 		if (!node->mapping_offset || !node->mapping_count)
 			goto fail_map;
-
 		map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
 				   node->mapping_offset);
 
@@ -491,6 +493,45 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
 	return -ENODEV;
 }
 
+int iort_dev_find_its_base(struct device *dev, u32 req_id,
+			   unsigned int idx, u64 *its_base)
+{
+	struct acpi_iort_its_group *its;
+	struct acpi_iort_node *node;
+	struct iort_its_msi_chip *its_msi_chip;
+	u32  trans_id;
+
+	node = iort_find_dev_node(dev);
+	if (!node)
+		return -ENXIO;
+
+	node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE);
+	if (!node)
+		return -ENXIO;
+
+	/* Move to ITS specific data */
+	its = (struct acpi_iort_its_group *)node->node_data;
+	if (idx > its->its_count) {
+		dev_err(dev, "requested ITS ID index [%d] is greater than available [%d]\n",
+			idx, its->its_count);
+		return -ENXIO;
+	}
+
+	trans_id = its->identifiers[idx];
+
+	spin_lock(&iort_msi_chip_lock);
+	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {
+		if (its_msi_chip->translation_id == trans_id) {
+			*its_base = its_msi_chip->base_addr;
+			spin_unlock(&iort_msi_chip_lock);
+			return 0;
+		}
+	}
+	spin_unlock(&iort_msi_chip_lock);
+
+	return -ENXIO;
+}
+
 /**
  * iort_dev_find_its_id() - Find the ITS identifier for a device
  * @dev: The device.
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 45ea1933..c45a2ad 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1854,7 +1854,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 3ff9ace..bf7b53d 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, u64 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
@@ -36,6 +37,8 @@
 struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
 void acpi_configure_pmsi_domain(struct device *dev);
 int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
+int iort_dev_find_its_base(struct device *dev, u32 req_id,
+			   unsigned int idx, u64 *its_base);
 /* IOMMU interface */
 void iort_set_dma_mask(struct device *dev);
 const struct iommu_ops *iort_iommu_configure(struct device *dev);
@@ -48,6 +51,9 @@ static inline struct irq_domain *iort_get_device_domain(struct device *dev,
 							u32 req_id)
 { return NULL; }
 static inline void acpi_configure_pmsi_domain(struct device *dev) { }
+int iort_dev_find_its_base(struct device *dev, u32 req_id,
+			   unsigned int idx, u64 *its_base)
+{ return -ENOSYS; }
 /* IOMMU interface */
 static inline void iort_set_dma_mask(struct device *dev) { }
 static inline
-- 
1.9.1

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[shameer]

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.

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

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

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index abe4b88..3767526 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -597,6 +597,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;
@@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
 
 static struct iommu_ops arm_smmu_ops;
 
+#ifdef CONFIG_ACPI
+static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device *dev)
+{
+	struct iommu_resv_region *region;
+	struct	irq_domain *irq_dom;
+	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+	u64	base;
+
+	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
+	if (irq_dom) {
+		int	ret;
+		u32	rid;
+
+		rid = pci_msi_domain_get_msi_rid(irq_dom, to_pci_dev(dev));
+		ret = iort_dev_find_its_base(dev, rid, 0, &base);
+		if (!ret) {
+			dev_info(dev, "SMMUv3:HW MSI resv addr 0x%pa\n", &base);
+			region = iommu_alloc_resv_region(base, SZ_128K,
+							 prot, IOMMU_RESV_MSI);
+			return region;
+		}
+	}
+
+	return NULL;
+}
+#else
+static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int arm_smmu_add_device(struct device *dev)
 {
 	int i, ret;
@@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 static void arm_smmu_get_resv_regions(struct device *dev,
 				      struct list_head *head)
 {
-	struct iommu_resv_region *region;
+	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+	struct iommu_resv_region *region = NULL;
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+	struct arm_smmu_device *smmu;
+
+	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
 
-	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
-					 prot, IOMMU_RESV_SW_MSI);
+	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI) &&
+		      dev_is_pci(dev))
+		region = arm_smmu_acpi_alloc_hw_msi(dev);
+
+	if (!region)
+		region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
+						 prot, IOMMU_RESV_SW_MSI);
 	if (!region)
 		return;
 
@@ -2611,6 +2653,7 @@ static void parse_driver_acpi_options(struct acpi_iort_smmu_v3 *iort_smmu,
 	switch (iort_smmu->model) {
 	case ACPI_IORT_SMMU_HISILICON_HI161X:
 		smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH;
+		smmu->options |= ARM_SMMU_OPT_RESV_HW_MSI;
 		break;
 	default:
 		break;
-- 
1.9.1

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[shameer]

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.

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

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

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index abe4b88..3767526 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -597,6 +597,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;
@@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
 
 static struct iommu_ops arm_smmu_ops;
 
+#ifdef CONFIG_ACPI
+static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device *dev)
+{
+	struct iommu_resv_region *region;
+	struct	irq_domain *irq_dom;
+	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+	u64	base;
+
+	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
+	if (irq_dom) {
+		int	ret;
+		u32	rid;
+
+		rid = pci_msi_domain_get_msi_rid(irq_dom, to_pci_dev(dev));
+		ret = iort_dev_find_its_base(dev, rid, 0, &base);
+		if (!ret) {
+			dev_info(dev, "SMMUv3:HW MSI resv addr 0x%pa\n", &base);
+			region = iommu_alloc_resv_region(base, SZ_128K,
+							 prot, IOMMU_RESV_MSI);
+			return region;
+		}
+	}
+
+	return NULL;
+}
+#else
+static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int arm_smmu_add_device(struct device *dev)
 {
 	int i, ret;
@@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 static void arm_smmu_get_resv_regions(struct device *dev,
 				      struct list_head *head)
 {
-	struct iommu_resv_region *region;
+	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+	struct iommu_resv_region *region = NULL;
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+	struct arm_smmu_device *smmu;
+
+	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
 
-	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
-					 prot, IOMMU_RESV_SW_MSI);
+	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI) &&
+		      dev_is_pci(dev))
+		region = arm_smmu_acpi_alloc_hw_msi(dev);
+
+	if (!region)
+		region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
+						 prot, IOMMU_RESV_SW_MSI);
 	if (!region)
 		return;
 
@@ -2611,6 +2653,7 @@ static void parse_driver_acpi_options(struct acpi_iort_smmu_v3 *iort_smmu,
 	switch (iort_smmu->model) {
 	case ACPI_IORT_SMMU_HISILICON_HI161X:
 		smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH;
+		smmu->options |= ARM_SMMU_OPT_RESV_HW_MSI;
 		break;
 	default:
 		break;
-- 
1.9.1

Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

On Wed, May 31, 2017 at 03:32:13PM +0100, shameer 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.
> 
> The HW ITS address region associated with the dev is retrieved
> using a new helper function added in the IORT code.

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

> Signed-off-by: shameer <shameerali.kolothum.thodi-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu-v3.c | 49 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index abe4b88..3767526 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -597,6 +597,7 @@ struct arm_smmu_device {
>  	u32				features;
>  
>  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> +#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
>  	u32				options;
>  
>  	struct arm_smmu_cmdq		cmdq;
> @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>  
>  static struct iommu_ops arm_smmu_ops;
>  
> +#ifdef CONFIG_ACPI
> +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device *dev)
> +{
> +	struct iommu_resv_region *region;
> +	struct	irq_domain *irq_dom;
> +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +	u64	base;

phys_addr_t

> +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> +	if (irq_dom) {
> +		int	ret;
> +		u32	rid;
> +
> +		rid = pci_msi_domain_get_msi_rid(irq_dom, to_pci_dev(dev));
> +		ret = iort_dev_find_its_base(dev, rid, 0, &base);

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

> +		if (!ret) {
> +			dev_info(dev, "SMMUv3:HW MSI resv addr 0x%pa\n", &base);
> +			region = iommu_alloc_resv_region(base, SZ_128K,
> +							 prot, IOMMU_RESV_MSI);
> +			return region;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +#else
> +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  static int arm_smmu_add_device(struct device *dev)
>  {
>  	int i, ret;
> @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
>  static void arm_smmu_get_resv_regions(struct device *dev,
>  				      struct list_head *head)
>  {
> -	struct iommu_resv_region *region;
> +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +	struct iommu_resv_region *region = NULL;
>  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +	struct arm_smmu_device *smmu;
> +
> +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
>  
> -	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> -					 prot, IOMMU_RESV_SW_MSI);
> +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI) &&
> +		      dev_is_pci(dev))
> +		region = arm_smmu_acpi_alloc_hw_msi(dev);

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

Lorenzo

> +	if (!region)
> +		region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> +						 prot, IOMMU_RESV_SW_MSI);
>  	if (!region)
>  		return;
>  
> @@ -2611,6 +2653,7 @@ static void parse_driver_acpi_options(struct acpi_iort_smmu_v3 *iort_smmu,
>  	switch (iort_smmu->model) {
>  	case ACPI_IORT_SMMU_HISILICON_HI161X:
>  		smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH;
> +		smmu->options |= ARM_SMMU_OPT_RESV_HW_MSI;
>  		break;
>  	default:
>  		break;
> -- 
> 1.9.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

On Wed, May 31, 2017 at 03:32:13PM +0100, shameer 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.
> 
> The HW ITS address region associated with the dev is retrieved
> using a new helper function added in the IORT code.

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

> Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/iommu/arm-smmu-v3.c | 49 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index abe4b88..3767526 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -597,6 +597,7 @@ struct arm_smmu_device {
>  	u32				features;
>  
>  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> +#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
>  	u32				options;
>  
>  	struct arm_smmu_cmdq		cmdq;
> @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>  
>  static struct iommu_ops arm_smmu_ops;
>  
> +#ifdef CONFIG_ACPI
> +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device *dev)
> +{
> +	struct iommu_resv_region *region;
> +	struct	irq_domain *irq_dom;
> +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +	u64	base;

phys_addr_t

> +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> +	if (irq_dom) {
> +		int	ret;
> +		u32	rid;
> +
> +		rid = pci_msi_domain_get_msi_rid(irq_dom, to_pci_dev(dev));
> +		ret = iort_dev_find_its_base(dev, rid, 0, &base);

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

> +		if (!ret) {
> +			dev_info(dev, "SMMUv3:HW MSI resv addr 0x%pa\n", &base);
> +			region = iommu_alloc_resv_region(base, SZ_128K,
> +							 prot, IOMMU_RESV_MSI);
> +			return region;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +#else
> +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  static int arm_smmu_add_device(struct device *dev)
>  {
>  	int i, ret;
> @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
>  static void arm_smmu_get_resv_regions(struct device *dev,
>  				      struct list_head *head)
>  {
> -	struct iommu_resv_region *region;
> +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +	struct iommu_resv_region *region = NULL;
>  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +	struct arm_smmu_device *smmu;
> +
> +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
>  
> -	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> -					 prot, IOMMU_RESV_SW_MSI);
> +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI) &&
> +		      dev_is_pci(dev))
> +		region = arm_smmu_acpi_alloc_hw_msi(dev);

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

Lorenzo

> +	if (!region)
> +		region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> +						 prot, IOMMU_RESV_SW_MSI);
>  	if (!region)
>  		return;
>  
> @@ -2611,6 +2653,7 @@ static void parse_driver_acpi_options(struct acpi_iort_smmu_v3 *iort_smmu,
>  	switch (iort_smmu->model) {
>  	case ACPI_IORT_SMMU_HISILICON_HI161X:
>  		smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH;
> +		smmu->options |= ARM_SMMU_OPT_RESV_HW_MSI;
>  		break;
>  	default:
>  		break;
> -- 
> 1.9.1
> 
> 
> --
> 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: [RFCv2 1/2] acpi:iort: Add new helper function to retrieve ITS base addr from dev IORT node

[Lorenzo Pieralisi]

On Wed, May 31, 2017 at 03:32:12PM +0100, shameer wrote:
> This provides a helper function to find and retrieve the ITS
> base address from the ID mappings array reference of a device
> IORT node(if any).
> 
> This is used in the subsequent patch to retrieve the ITS base 
> address associated with a pci dev IORT node.

"Add an IORT helper function to retrieve the ITS data through IORT
device<->ITS mappings".

> Signed-off-by: shameer <shameerali.kolothum.thodi-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/acpi/arm64/iort.c        | 47 +++++++++++++++++++++++++++++++++++++---
>  drivers/irqchip/irq-gic-v3-its.c |  3 ++-
>  include/linux/acpi_iort.h        |  8 ++++++-
>  3 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index c5fecf9..12d7347 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -34,6 +34,7 @@
>  struct iort_its_msi_chip {
>  	struct list_head	list;
>  	struct fwnode_handle	*fw_node;
> +	u64			base_addr;

phys_addr_t

>  	u32			translation_id;
>  };
>  
> @@ -132,13 +133,14 @@ typedef acpi_status (*iort_find_node_callback)
>  
>  /**
>   * iort_register_domain_token() - register domain token and related ITS ID
> - * to the list from where we can get it back later on.
> + * and base address to the list from where we can get it back later on.
>   * @trans_id: ITS ID.

Missing something here

>   * @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, u64 base,

phys_addr_t

> +			       struct fwnode_handle *fw_node)
>  {
>  	struct iort_its_msi_chip *its_msi_chip;
>  
> @@ -148,6 +150,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);
> @@ -370,7 +373,6 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>  
>  		if (!node->mapping_offset || !node->mapping_count)
>  			goto fail_map;
> -

Unrelated change.

>  		map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
>  				   node->mapping_offset);
>  
> @@ -491,6 +493,45 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
>  	return -ENODEV;
>  }
>  
> +int iort_dev_find_its_base(struct device *dev, u32 req_id,
> +			   unsigned int idx, u64 *its_base)
> +{
> +	struct acpi_iort_its_group *its;
> +	struct acpi_iort_node *node;
> +	struct iort_its_msi_chip *its_msi_chip;
> +	u32  trans_id;
> +
> +	node = iort_find_dev_node(dev);
> +	if (!node)
> +		return -ENXIO;

-ENODEV, throughout

> +
> +	node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE);
> +	if (!node)
> +		return -ENXIO;
> +
> +	/* Move to ITS specific data */
> +	its = (struct acpi_iort_its_group *)node->node_data;
> +	if (idx > its->its_count) {
> +		dev_err(dev, "requested ITS ID index [%d] is greater than available [%d]\n",
> +			idx, its->its_count);
> +		return -ENXIO;
> +	}
> +
> +	trans_id = its->identifiers[idx];
> +
> +	spin_lock(&iort_msi_chip_lock);
> +	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {
> +		if (its_msi_chip->translation_id == trans_id) {
> +			*its_base = its_msi_chip->base_addr;
> +			spin_unlock(&iort_msi_chip_lock);
> +			return 0;
> +		}
> +	}
> +	spin_unlock(&iort_msi_chip_lock);
> +
> +	return -ENXIO;

Cosmetics:

	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 == trans_id) {
			*its_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.
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 45ea1933..c45a2ad 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1854,7 +1854,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 3ff9ace..bf7b53d 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, u64 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
> @@ -36,6 +37,8 @@
>  struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
>  void acpi_configure_pmsi_domain(struct device *dev);
>  int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
> +int iort_dev_find_its_base(struct device *dev, u32 req_id,
> +			   unsigned int idx, u64 *its_base);
>  /* IOMMU interface */
>  void iort_set_dma_mask(struct device *dev);
>  const struct iommu_ops *iort_iommu_configure(struct device *dev);
> @@ -48,6 +51,9 @@ static inline struct irq_domain *iort_get_device_domain(struct device *dev,
>  							u32 req_id)
>  { return NULL; }
>  static inline void acpi_configure_pmsi_domain(struct device *dev) { }
> +int iort_dev_find_its_base(struct device *dev, u32 req_id,
> +			   unsigned int idx, u64 *its_base)
> +{ return -ENOSYS; }

-ENODEV

Thanks,
Lorenzo

[RFCv2 1/2] acpi:iort: Add new helper function to retrieve ITS base addr from dev IORT node

[Lorenzo Pieralisi]

On Wed, May 31, 2017 at 03:32:12PM +0100, shameer wrote:
> This provides a helper function to find and retrieve the ITS
> base address from the ID mappings array reference of a device
> IORT node(if any).
> 
> This is used in the subsequent patch to retrieve the ITS base 
> address associated with a pci dev IORT node.

"Add an IORT helper function to retrieve the ITS data through IORT
device<->ITS mappings".

> Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/acpi/arm64/iort.c        | 47 +++++++++++++++++++++++++++++++++++++---
>  drivers/irqchip/irq-gic-v3-its.c |  3 ++-
>  include/linux/acpi_iort.h        |  8 ++++++-
>  3 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index c5fecf9..12d7347 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -34,6 +34,7 @@
>  struct iort_its_msi_chip {
>  	struct list_head	list;
>  	struct fwnode_handle	*fw_node;
> +	u64			base_addr;

phys_addr_t

>  	u32			translation_id;
>  };
>  
> @@ -132,13 +133,14 @@ typedef acpi_status (*iort_find_node_callback)
>  
>  /**
>   * iort_register_domain_token() - register domain token and related ITS ID
> - * to the list from where we can get it back later on.
> + * and base address to the list from where we can get it back later on.
>   * @trans_id: ITS ID.

Missing something here

>   * @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, u64 base,

phys_addr_t

> +			       struct fwnode_handle *fw_node)
>  {
>  	struct iort_its_msi_chip *its_msi_chip;
>  
> @@ -148,6 +150,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);
> @@ -370,7 +373,6 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>  
>  		if (!node->mapping_offset || !node->mapping_count)
>  			goto fail_map;
> -

Unrelated change.

>  		map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
>  				   node->mapping_offset);
>  
> @@ -491,6 +493,45 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
>  	return -ENODEV;
>  }
>  
> +int iort_dev_find_its_base(struct device *dev, u32 req_id,
> +			   unsigned int idx, u64 *its_base)
> +{
> +	struct acpi_iort_its_group *its;
> +	struct acpi_iort_node *node;
> +	struct iort_its_msi_chip *its_msi_chip;
> +	u32  trans_id;
> +
> +	node = iort_find_dev_node(dev);
> +	if (!node)
> +		return -ENXIO;

-ENODEV, throughout

> +
> +	node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE);
> +	if (!node)
> +		return -ENXIO;
> +
> +	/* Move to ITS specific data */
> +	its = (struct acpi_iort_its_group *)node->node_data;
> +	if (idx > its->its_count) {
> +		dev_err(dev, "requested ITS ID index [%d] is greater than available [%d]\n",
> +			idx, its->its_count);
> +		return -ENXIO;
> +	}
> +
> +	trans_id = its->identifiers[idx];
> +
> +	spin_lock(&iort_msi_chip_lock);
> +	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {
> +		if (its_msi_chip->translation_id == trans_id) {
> +			*its_base = its_msi_chip->base_addr;
> +			spin_unlock(&iort_msi_chip_lock);
> +			return 0;
> +		}
> +	}
> +	spin_unlock(&iort_msi_chip_lock);
> +
> +	return -ENXIO;

Cosmetics:

	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 == trans_id) {
			*its_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.
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 45ea1933..c45a2ad 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1854,7 +1854,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 3ff9ace..bf7b53d 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, u64 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
> @@ -36,6 +37,8 @@
>  struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
>  void acpi_configure_pmsi_domain(struct device *dev);
>  int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
> +int iort_dev_find_its_base(struct device *dev, u32 req_id,
> +			   unsigned int idx, u64 *its_base);
>  /* IOMMU interface */
>  void iort_set_dma_mask(struct device *dev);
>  const struct iommu_ops *iort_iommu_configure(struct device *dev);
> @@ -48,6 +51,9 @@ static inline struct irq_domain *iort_get_device_domain(struct device *dev,
>  							u32 req_id)
>  { return NULL; }
>  static inline void acpi_configure_pmsi_domain(struct device *dev) { }
> +int iort_dev_find_its_base(struct device *dev, u32 req_id,
> +			   unsigned int idx, u64 *its_base)
> +{ return -ENOSYS; }

-ENODEV

Thanks,
Lorenzo

Re: 回复： Alibaba-kernel

[Jacob Pan]

On Tue, 6 Jun 2017 13:25:00 +0800
"若翾" <eva6688@vip.qq.com> wrote:

> Hi，Jacob，
> Thank you for your reply，
> alibaba in  San Mateo ，
> Do you have a WeChat? You can add me
> WeChat15900779704
> 
I see, I am not planning to relocate at the moment but I will keep it
in mind.

Thanks,
> 
> 
> 
> 
> ------------------
> 
> Eva wang  
> 中国 上海郊区 | BAT高科技公司高端技术类岗位招聘
> 互联网 | 某知名公司
> 
> 
> 
>  
> 
> 
> 
> 
> ------------------ 原始邮件 ------------------
> 发件人: "jacob.jun.pan";<jacob.jun.pan@linux.intel.com>;
> 发送时间: 2017年6月6日(星期二) 中午1:06
> 收件人: "若翾"<eva6688@vip.qq.com>; 
> 抄送: "jacob.jun.pan"<jacob.jun.pan@linux.intel.com>; 
> 主题: Re: Alibaba-kernel
> 
> 
> 
> On Mon, 5 Jun 2017 18:35:27 +0800
> "若翾" <eva6688@vip.qq.com> wrote:
> 
> > 您好，我是猎头eva。
> >  
> > 很冒昧的打扰您,想和您聊一下阿里的机会。  
> >  
> >  
> >  阿里巴巴首席技术官，行癫发出通知：阿里整合9个事业部加原有的中间件技术部组成新的基础设施事业群，携手共同打造具备全球竞争力的阿里巴巴经济体软硬件基础设施。                                
> >   
> >  成立系统软件事业部
> >  由平台架构部团队、AIS的JVM、Kernel团队共同组成，向毕玄汇报。
> >   
> >  目前我们需要高端的技术管理型leader／技术型资深研发专家和高级专家/
> > 行业的标杆和领军人物 1）虚拟化领域：对xen/KVM/Vmware熟悉的。
> > 2）OS内核领域：熟悉Linux内核机制：内存、调度、文件系统、网络等；熟练使用多种Linux内核调试分析工具(kgdb,
> > crash等)；性能分析工具（gcover, blktrace, ftrace）等经验；
> > 
> >  
> > 
> >  事业部介绍：
> >  阿里巴巴操作系统研发团队负责全集团的服务器操作系统以及Linux内核的研发与产品化。
> >  团队针对阿里巴巴各业务的需求，新技术的发展，新硬件的引入，在内核与操作系统等基础领域进行研究创新。
> >  目前已经形成alios与alikernel等多个产品。加入我们，你可以跟传说中的多隆大神一起工作，你可以不断挑战基础软件领域的新技术，你可以向社区提Patch，你的工作可以应用到阿里巴巴的数十万台服务器上。阿里巴巴操作系统研发团队，期待你的加入。 
> >   Team Introduction； 
> >  The Operation System R&D Team of Alibaba Group is responsible for
> > research & development and production of the server operation system
> > and linux kernel . If you are interested in challenging and
> > edge-cutting technologies in foundational software domain, please
> > don't hesitate to join us. 
> >   
> Hi Eva,
> 
> Thanks for reaching out, sounds like great opportunity. What are the
> US locations? I am based in Portland Oregon.
> 
> Jacob
> >  
> > 
> >  地点：美国、北京、深圳、杭州
> > 
> >  JD参考介绍
> >  系统软件事业部-Linux内核研发专家
> >  Responsibilities： 
> >  1·Responsible for Linux kernel development 
> >  2·Responsible for Linux kernel related tools development 
> >  3·Responsible for Linux kernel security patching 
> >  4·Linux kernel debugging
> >  岗位要求
> >  1·Expertise in at least one kernel sub-system, for examples,
> > network, file system (ext4, xfs), memory management, process
> > scheduler 2·Five years or more of Linux kernel development
> > experiences 3·Knowledgeable about x86 or arm 4·Experienced with
> > kernel debugging 5·Kernel community contributor as a plus
> > 
> >  Linux内核测试专家
> >  具体职责： 
> >  1.
> > 根据业务需求制定测试方案，测试执行和结果分析，保证操作系统的高质量部署交付。
> > 2. 负责linux内核测试系统、测试工具和测试集的开发。 3.
> > 对内核各子模块性能数据采集和量化分析，发现性能瓶颈，提出解决方案。岗位要求
> >  1. 五年以上软件开发或测试软件开发经验。 
> >  2.熟悉linux操作系统各组件原理，了解内核开发工具和调试工具的运用，有内核开发或调优经验的优先。 
> >  3. 精通至少一到两种脚本语言和C语言的开发。 
> >  4. 具备复杂需求及技术的研究能力，善于进行任务分解，抓住关键点。 
> >  5. 良好的沟通协调能力和团队合作意识，能承受大的工作压力。
> >  
> >  
> >  
> >   
> >  Eva Wang
> >  consultant - Shanghai
> >  Mobile:15900779704
> >  Weixin:15900779704
> >   
> >  ------------------
> >   
> > 
> >  Eva wang  
> >  中国 上海郊区 | BAT高端技术类岗位招聘
> >  互联网 | 知名互联网公司  
> [Jacob Pan]
[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

回复： Alibaba-kernel

[Jacob Pan]

On Tue, 6 Jun 2017 13:25:00 +0800
"??" <eva6688@vip.qq.com> wrote:

> Hi?Jacob?
> Thank you for your reply?
> alibaba in  San Mateo ?
> Do you have a WeChat? You can add me
> WeChat15900779704
> 
I see, I am not planning to relocate at the moment but I will keep it
in mind.

Thanks,
> 
> 
> 
> 
> ------------------
> 
> Eva wang  
> ?? ???? | BAT??????????????
> ??? | ?????
> 
> 
> 
>  
> 
> 
> 
> 
> ------------------ ???? ------------------
> ???: "jacob.jun.pan";<jacob.jun.pan@linux.intel.com>;
> ????: 2017?6?6?(???) ??1:06
> ???: "??"<eva6688@vip.qq.com>; 
> ??: "jacob.jun.pan"<jacob.jun.pan@linux.intel.com>; 
> ??: Re: Alibaba-kernel
> 
> 
> 
> On Mon, 5 Jun 2017 18:35:27 +0800
> "??" <eva6688@vip.qq.com> wrote:
> 
> > ???????eva?
> >  
> > ???????,????????????  
> >  
> >  
> >  ?????????????????????9???????????????????????????????????????????????????????                                
> >   
> >  ?????????
> >  ?????????AIS?JVM?Kernel?????????????
> >   
> >  ??????????????leader???????????????/
> > ?????????? 1????????xen/KVM/Vmware????
> > 2?OS???????Linux??????????????????????????Linux????????(kgdb,
> > crash?)????????gcover, blktrace, ftrace?????
> > 
> >  
> > 
> >  ??????
> >  ???????????????????????????Linux??????????
> >  ?????????????????????????????????????????????????
> >  ??????alios?alikernel?????????????????????????????????????????????????????Patch???????????????????????????????????????????? 
> >   Team Introduction? 
> >  The Operation System R&D Team of Alibaba Group is responsible for
> > research & development and production of the server operation system
> > and linux kernel . If you are interested in challenging and
> > edge-cutting technologies in foundational software domain, please
> > don't hesitate to join us. 
> >   
> Hi Eva,
> 
> Thanks for reaching out, sounds like great opportunity. What are the
> US locations? I am based in Portland Oregon.
> 
> Jacob
> >  
> > 
> >  ??????????????
> > 
> >  JD????
> >  ???????-Linux??????
> >  Responsibilities? 
> >  1?Responsible for Linux kernel development 
> >  2?Responsible for Linux kernel related tools development 
> >  3?Responsible for Linux kernel security patching 
> >  4?Linux kernel debugging
> >  ????
> >  1?Expertise in at least one kernel sub-system, for examples,
> > network, file system (ext4, xfs), memory management, process
> > scheduler 2?Five years or more of Linux kernel development
> > experiences 3?Knowledgeable about x86 or arm 4?Experienced with
> > kernel debugging 5?Kernel community contributor as a plus
> > 
> >  Linux??????
> >  ????? 
> >  1.
> > ??????????????????????????????????????
> > 2. ??linux??????????????????? 3.
> > ?????????????????????????????????????
> >  1. ?????????????????? 
> >  2.??linux????????????????????????????????????????? 
> >  3. ?????????????C?????? 
> >  4. ?????????????????????????????? 
> >  5. ???????????????????????????
> >  
> >  
> >  
> >   
> >  Eva Wang
> >  consultant - Shanghai
> >  Mobile:15900779704
> >  Weixin:15900779704
> >   
> >  ------------------
> >   
> > 
> >  Eva wang  
> >  ?? ???? | BAT?????????
> >  ??? | ???????  
> [Jacob Pan]
[Jacob Pan]

RE: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

Hi Lorenzo,

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org]
> Sent: Tuesday, June 06, 2017 2:56 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier-5wv7dgnIgG8@public.gmane.org; sudeep.holla-5wv7dgnIgG8@public.gmane.org; will.deacon-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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > 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.
> >
> > The HW ITS address region associated with the dev is retrieved using a
> > new helper function added in the IORT code.
> 
> Remove or rephrase last paragraph, it reads as if you are adding an IORT
> helper function in this patch but you actually aren't.

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

> > Signed-off-by: shameer <shameerali.kolothum.thodi-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/iommu/arm-smmu-v3.c | 49
> > ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 46 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-
> v3.c
> > index abe4b88..3767526 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> >  	u32				features;
> >
> >  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> > +#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
> >  	u32				options;
> >
> >  	struct arm_smmu_cmdq		cmdq;
> > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > arm_smmu_device *smmu, u32 sid)
> >
> >  static struct iommu_ops arm_smmu_ops;
> >
> > +#ifdef CONFIG_ACPI
> > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > +device *dev) {
> > +	struct iommu_resv_region *region;
> > +	struct	irq_domain *irq_dom;
> > +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > +	u64	base;
> 
> phys_addr_t

Ok.

> > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > +	if (irq_dom) {
> > +		int	ret;
> > +		u32	rid;
> > +
> > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> to_pci_dev(dev));
> > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> 
> Well, here we use ITS id 0 which is fine as long as code in IORT uses the same
> policy for getting the irq_domain (ie we want to reserve the ITS address
> space that is actually used by the device to send IRQs not a a different one) it
> is just a heads-up because I find this confusing.

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

I am not sure, how we can get the index info  though theoretically It is possible for
the ITS group node having multiple ITSs.
 
> > +		if (!ret) {
> > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> 0x%pa\n", &base);
> > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > +							 prot,
> IOMMU_RESV_MSI);
> > +			return region;
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> > +#else
> > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > +device *dev) {
> > +	return NULL;
> > +}
> > +#endif
> > +
> >  static int arm_smmu_add_device(struct device *dev)  {
> >  	int i, ret;
> > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > *dev, struct of_phandle_args *args)  static void
> arm_smmu_get_resv_regions(struct device *dev,
> >  				      struct list_head *head)
> >  {
> > -	struct iommu_resv_region *region;
> > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > +	struct iommu_resv_region *region = NULL;
> >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > +	struct arm_smmu_device *smmu;
> > +
> > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> >
> > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> MSI_IOVA_LENGTH,
> > -					 prot, IOMMU_RESV_SW_MSI);
> > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> &&
> > +		      dev_is_pci(dev))
> > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> 
> Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?

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

Thanks,
Shameer

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

Hi Lorenzo,

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: Tuesday, June 06, 2017 2:56 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier at arm.com; sudeep.holla at arm.com; will.deacon at arm.com;
> robin.murphy at arm.com; hanjun.guo at linaro.org; Gabriele Paoloni; John
> Garry; iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org; linux-acpi at vger.kernel.org; devel at acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > 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.
> >
> > The HW ITS address region associated with the dev is retrieved using a
> > new helper function added in the IORT code.
> 
> Remove or rephrase last paragraph, it reads as if you are adding an IORT
> helper function in this patch but you actually aren't.

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

> > Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  drivers/iommu/arm-smmu-v3.c | 49
> > ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 46 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-
> v3.c
> > index abe4b88..3767526 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> >  	u32				features;
> >
> >  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> > +#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
> >  	u32				options;
> >
> >  	struct arm_smmu_cmdq		cmdq;
> > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > arm_smmu_device *smmu, u32 sid)
> >
> >  static struct iommu_ops arm_smmu_ops;
> >
> > +#ifdef CONFIG_ACPI
> > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > +device *dev) {
> > +	struct iommu_resv_region *region;
> > +	struct	irq_domain *irq_dom;
> > +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > +	u64	base;
> 
> phys_addr_t

Ok.

> > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > +	if (irq_dom) {
> > +		int	ret;
> > +		u32	rid;
> > +
> > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> to_pci_dev(dev));
> > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> 
> Well, here we use ITS id 0 which is fine as long as code in IORT uses the same
> policy for getting the irq_domain (ie we want to reserve the ITS address
> space that is actually used by the device to send IRQs not a a different one) it
> is just a heads-up because I find this confusing.

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

I am not sure, how we can get the index info  though theoretically It is possible for
the ITS group node having multiple ITSs.
 
> > +		if (!ret) {
> > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> 0x%pa\n", &base);
> > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > +							 prot,
> IOMMU_RESV_MSI);
> > +			return region;
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> > +#else
> > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > +device *dev) {
> > +	return NULL;
> > +}
> > +#endif
> > +
> >  static int arm_smmu_add_device(struct device *dev)  {
> >  	int i, ret;
> > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > *dev, struct of_phandle_args *args)  static void
> arm_smmu_get_resv_regions(struct device *dev,
> >  				      struct list_head *head)
> >  {
> > -	struct iommu_resv_region *region;
> > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > +	struct iommu_resv_region *region = NULL;
> >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > +	struct arm_smmu_device *smmu;
> > +
> > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> >
> > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> MSI_IOVA_LENGTH,
> > -					 prot, IOMMU_RESV_SW_MSI);
> > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> &&
> > +		      dev_is_pci(dev))
> > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> 
> Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?

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

Thanks,
Shameer

RE: [RFCv2 1/2] acpi:iort: Add new helper function to retrieve ITS base addr from dev IORT node

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org]
> Sent: Tuesday, June 06, 2017 3:10 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier-5wv7dgnIgG8@public.gmane.org; sudeep.holla-5wv7dgnIgG8@public.gmane.org; will.deacon-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: [RFCv2 1/2] acpi:iort: Add new helper function to retrieve ITS
> base addr from dev IORT node
> 
> On Wed, May 31, 2017 at 03:32:12PM +0100, shameer wrote:
> > This provides a helper function to find and retrieve the ITS base
> > address from the ID mappings array reference of a device IORT node(if
> > any).
> >
> > This is used in the subsequent patch to retrieve the ITS base address
> > associated with a pci dev IORT node.
> 
> "Add an IORT helper function to retrieve the ITS data through IORT device<-
> >ITS mappings".
> 
> > Signed-off-by: shameer <shameerali.kolothum.thodi-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/acpi/arm64/iort.c        | 47
> +++++++++++++++++++++++++++++++++++++---
> >  drivers/irqchip/irq-gic-v3-its.c |  3 ++-
> >  include/linux/acpi_iort.h        |  8 ++++++-
> >  3 files changed, 53 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index c5fecf9..12d7347 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -34,6 +34,7 @@
> >  struct iort_its_msi_chip {
> >  	struct list_head	list;
> >  	struct fwnode_handle	*fw_node;
> > +	u64			base_addr;
> 
> phys_addr_t
> 
> >  	u32			translation_id;
> >  };
> >
> > @@ -132,13 +133,14 @@ typedef acpi_status (*iort_find_node_callback)
> >
> >  /**
> >   * iort_register_domain_token() - register domain token and related
> > ITS ID
> > - * to the list from where we can get it back later on.
> > + * and base address to the list from where we can get it back later on.
> >   * @trans_id: ITS ID.
> 
> Missing something here
> 
> >   * @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, u64 base,
> 
> phys_addr_t
> 
> > +			       struct fwnode_handle *fw_node)
> >  {
> >  	struct iort_its_msi_chip *its_msi_chip;
> >
> > @@ -148,6 +150,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); @@ -370,7 +373,6
> > @@ static struct acpi_iort_node *iort_node_map_id(struct
> > acpi_iort_node *node,
> >
> >  		if (!node->mapping_offset || !node->mapping_count)
> >  			goto fail_map;
> > -
> 
> Unrelated change.
> 
> >  		map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
> >  				   node->mapping_offset);
> >
> > @@ -491,6 +493,45 @@ int iort_pmsi_get_dev_id(struct device *dev, u32
> *dev_id)
> >  	return -ENODEV;
> >  }
> >
> > +int iort_dev_find_its_base(struct device *dev, u32 req_id,
> > +			   unsigned int idx, u64 *its_base) {
> > +	struct acpi_iort_its_group *its;
> > +	struct acpi_iort_node *node;
> > +	struct iort_its_msi_chip *its_msi_chip;
> > +	u32  trans_id;
> > +
> > +	node = iort_find_dev_node(dev);
> > +	if (!node)
> > +		return -ENXIO;
> 
> -ENODEV, throughout

Ok. The reason for -ENXIO use is that this function is based on the existing
iort_dev_find_its_id() and it returns -ENXIO for those error cases.

I will incorporate all the other comments in the next revision.

Many thanks,
Shameer
 

[RFCv2 1/2] acpi:iort: Add new helper function to retrieve ITS base addr from dev IORT node

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: Tuesday, June 06, 2017 3:10 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier at arm.com; sudeep.holla at arm.com; will.deacon at arm.com;
> robin.murphy at arm.com; hanjun.guo at linaro.org; Gabriele Paoloni; John
> Garry; iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org; linux-acpi at vger.kernel.org; devel at acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [RFCv2 1/2] acpi:iort: Add new helper function to retrieve ITS
> base addr from dev IORT node
> 
> On Wed, May 31, 2017 at 03:32:12PM +0100, shameer wrote:
> > This provides a helper function to find and retrieve the ITS base
> > address from the ID mappings array reference of a device IORT node(if
> > any).
> >
> > This is used in the subsequent patch to retrieve the ITS base address
> > associated with a pci dev IORT node.
> 
> "Add an IORT helper function to retrieve the ITS data through IORT device<-
> >ITS mappings".
> 
> > Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  drivers/acpi/arm64/iort.c        | 47
> +++++++++++++++++++++++++++++++++++++---
> >  drivers/irqchip/irq-gic-v3-its.c |  3 ++-
> >  include/linux/acpi_iort.h        |  8 ++++++-
> >  3 files changed, 53 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index c5fecf9..12d7347 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -34,6 +34,7 @@
> >  struct iort_its_msi_chip {
> >  	struct list_head	list;
> >  	struct fwnode_handle	*fw_node;
> > +	u64			base_addr;
> 
> phys_addr_t
> 
> >  	u32			translation_id;
> >  };
> >
> > @@ -132,13 +133,14 @@ typedef acpi_status (*iort_find_node_callback)
> >
> >  /**
> >   * iort_register_domain_token() - register domain token and related
> > ITS ID
> > - * to the list from where we can get it back later on.
> > + * and base address to the list from where we can get it back later on.
> >   * @trans_id: ITS ID.
> 
> Missing something here
> 
> >   * @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, u64 base,
> 
> phys_addr_t
> 
> > +			       struct fwnode_handle *fw_node)
> >  {
> >  	struct iort_its_msi_chip *its_msi_chip;
> >
> > @@ -148,6 +150,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); @@ -370,7 +373,6
> > @@ static struct acpi_iort_node *iort_node_map_id(struct
> > acpi_iort_node *node,
> >
> >  		if (!node->mapping_offset || !node->mapping_count)
> >  			goto fail_map;
> > -
> 
> Unrelated change.
> 
> >  		map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
> >  				   node->mapping_offset);
> >
> > @@ -491,6 +493,45 @@ int iort_pmsi_get_dev_id(struct device *dev, u32
> *dev_id)
> >  	return -ENODEV;
> >  }
> >
> > +int iort_dev_find_its_base(struct device *dev, u32 req_id,
> > +			   unsigned int idx, u64 *its_base) {
> > +	struct acpi_iort_its_group *its;
> > +	struct acpi_iort_node *node;
> > +	struct iort_its_msi_chip *its_msi_chip;
> > +	u32  trans_id;
> > +
> > +	node = iort_find_dev_node(dev);
> > +	if (!node)
> > +		return -ENXIO;
> 
> -ENODEV, throughout

Ok. The reason for -ENXIO use is that this function is based on the existing
iort_dev_find_its_id() and it returns -ENXIO for those error cases.

I will incorporate all the other comments in the next revision.

Many thanks,
Shameer
 

Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi wrote:
> Hi Lorenzo,
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> > Sent: Tuesday, June 06, 2017 2:56 PM
> > To: Shameerali Kolothum Thodi
> > Cc: marc.zyngier@arm.com; sudeep.holla@arm.com; will.deacon@arm.com;
> > robin.murphy@arm.com; hanjun.guo@linaro.org; Gabriele Paoloni; John
> > Garry; iommu@lists.linux-foundation.org; linux-arm-
> > kernel@lists.infradead.org; linux-acpi@vger.kernel.org; devel@acpica.org;
> > Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> > Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> > erratum 161010801
> > 
> > On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > > 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.
> > >
> > > The HW ITS address region associated with the dev is retrieved using a
> > > new helper function added in the IORT code.
> > 
> > Remove or rephrase last paragraph, it reads as if you are adding an IORT
> > helper function in this patch but you actually aren't.
> 
> Thanks for going through this patch series. I will remove this in next version.
> 
> > > Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > >  drivers/iommu/arm-smmu-v3.c | 49
> > > ++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 46 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-
> > v3.c
> > > index abe4b88..3767526 100644
> > > --- a/drivers/iommu/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> > >  	u32				features;
> > >
> > >  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> > > +#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
> > >  	u32				options;
> > >
> > >  	struct arm_smmu_cmdq		cmdq;
> > > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > > arm_smmu_device *smmu, u32 sid)
> > >
> > >  static struct iommu_ops arm_smmu_ops;
> > >
> > > +#ifdef CONFIG_ACPI
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > +	struct iommu_resv_region *region;
> > > +	struct	irq_domain *irq_dom;
> > > +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > +	u64	base;
> > 
> > phys_addr_t
> 
> Ok.
> 
> > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > +	if (irq_dom) {
> > > +		int	ret;
> > > +		u32	rid;
> > > +
> > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > to_pci_dev(dev));
> > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > 
> > Well, here we use ITS id 0 which is fine as long as code in IORT uses the same
> > policy for getting the irq_domain (ie we want to reserve the ITS address
> > space that is actually used by the device to send IRQs not a a different one) it
> > is just a heads-up because I find this confusing.
> 
> Ok. Just to make it clear, 0 is the index into the ITS identifier list.
> I noted that iort_get_device_domain() uses index 0 while retrieving the ITS identifier.
> May be use the same approach here as well? ie, remove the index from function call?
> 
> I am not sure, how we can get the index info  though theoretically It is possible for
> the ITS group node having multiple ITSs.

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

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

> > > +		if (!ret) {
> > > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> > 0x%pa\n", &base);
> > > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > > +							 prot,
> > IOMMU_RESV_MSI);
> > > +			return region;
> > > +		}
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +#else
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > +	return NULL;
> > > +}
> > > +#endif
> > > +
> > >  static int arm_smmu_add_device(struct device *dev)  {
> > >  	int i, ret;
> > > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > > *dev, struct of_phandle_args *args)  static void
> > arm_smmu_get_resv_regions(struct device *dev,
> > >  				      struct list_head *head)
> > >  {
> > > -	struct iommu_resv_region *region;
> > > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > > +	struct iommu_resv_region *region = NULL;
> > >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > +	struct arm_smmu_device *smmu;
> > > +
> > > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> > >
> > > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> > MSI_IOVA_LENGTH,
> > > -					 prot, IOMMU_RESV_SW_MSI);
> > > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> > &&
> > > +		      dev_is_pci(dev))
> > > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> > 
> > Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?
> 
> It is just that PCIe devices won't be functional on this platforms as the endpoint will 
> be configured with ITS IOVA address. May be I should add some dev_warn() here.

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

Lorenzo

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi wrote:
> Hi Lorenzo,
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> > Sent: Tuesday, June 06, 2017 2:56 PM
> > To: Shameerali Kolothum Thodi
> > Cc: marc.zyngier at arm.com; sudeep.holla at arm.com; will.deacon at arm.com;
> > robin.murphy at arm.com; hanjun.guo at linaro.org; Gabriele Paoloni; John
> > Garry; iommu at lists.linux-foundation.org; linux-arm-
> > kernel at lists.infradead.org; linux-acpi at vger.kernel.org; devel at acpica.org;
> > Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> > Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> > erratum 161010801
> > 
> > On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > > 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.
> > >
> > > The HW ITS address region associated with the dev is retrieved using a
> > > new helper function added in the IORT code.
> > 
> > Remove or rephrase last paragraph, it reads as if you are adding an IORT
> > helper function in this patch but you actually aren't.
> 
> Thanks for going through this patch series. I will remove this in next version.
> 
> > > Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > >  drivers/iommu/arm-smmu-v3.c | 49
> > > ++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 46 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-
> > v3.c
> > > index abe4b88..3767526 100644
> > > --- a/drivers/iommu/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> > >  	u32				features;
> > >
> > >  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> > > +#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
> > >  	u32				options;
> > >
> > >  	struct arm_smmu_cmdq		cmdq;
> > > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > > arm_smmu_device *smmu, u32 sid)
> > >
> > >  static struct iommu_ops arm_smmu_ops;
> > >
> > > +#ifdef CONFIG_ACPI
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > +	struct iommu_resv_region *region;
> > > +	struct	irq_domain *irq_dom;
> > > +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > +	u64	base;
> > 
> > phys_addr_t
> 
> Ok.
> 
> > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > +	if (irq_dom) {
> > > +		int	ret;
> > > +		u32	rid;
> > > +
> > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > to_pci_dev(dev));
> > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > 
> > Well, here we use ITS id 0 which is fine as long as code in IORT uses the same
> > policy for getting the irq_domain (ie we want to reserve the ITS address
> > space that is actually used by the device to send IRQs not a a different one) it
> > is just a heads-up because I find this confusing.
> 
> Ok. Just to make it clear, 0 is the index into the ITS identifier list.
> I noted that iort_get_device_domain() uses index 0 while retrieving the ITS identifier.
> May be use the same approach here as well? ie, remove the index from function call?
> 
> I am not sure, how we can get the index info  though theoretically It is possible for
> the ITS group node having multiple ITSs.

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

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

> > > +		if (!ret) {
> > > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> > 0x%pa\n", &base);
> > > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > > +							 prot,
> > IOMMU_RESV_MSI);
> > > +			return region;
> > > +		}
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +#else
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > +	return NULL;
> > > +}
> > > +#endif
> > > +
> > >  static int arm_smmu_add_device(struct device *dev)  {
> > >  	int i, ret;
> > > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > > *dev, struct of_phandle_args *args)  static void
> > arm_smmu_get_resv_regions(struct device *dev,
> > >  				      struct list_head *head)
> > >  {
> > > -	struct iommu_resv_region *region;
> > > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > > +	struct iommu_resv_region *region = NULL;
> > >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > +	struct arm_smmu_device *smmu;
> > > +
> > > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> > >
> > > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> > MSI_IOVA_LENGTH,
> > > -					 prot, IOMMU_RESV_SW_MSI);
> > > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> > &&
> > > +		      dev_is_pci(dev))
> > > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> > 
> > Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?
> 
> It is just that PCIe devices won't be functional on this platforms as the endpoint will 
> be configured with ITS IOVA address. May be I should add some dev_warn() here.

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

Lorenzo

Re: [Devel] [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

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

On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi wrote:
> Hi Lorenzo,
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi(a)arm.com]
> > Sent: Tuesday, June 06, 2017 2:56 PM
> > To: Shameerali Kolothum Thodi
> > Cc: marc.zyngier(a)arm.com; sudeep.holla(a)arm.com; will.deacon(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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> > erratum 161010801
> > 
> > On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > > 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.
> > >
> > > The HW ITS address region associated with the dev is retrieved using a
> > > new helper function added in the IORT code.
> > 
> > Remove or rephrase last paragraph, it reads as if you are adding an IORT
> > helper function in this patch but you actually aren't.
> 
> Thanks for going through this patch series. I will remove this in next version.
> 
> > > Signed-off-by: shameer <shameerali.kolothum.thodi(a)huawei.com>
> > > ---
> > >  drivers/iommu/arm-smmu-v3.c | 49
> > > ++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 46 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-
> > v3.c
> > > index abe4b88..3767526 100644
> > > --- a/drivers/iommu/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> > >  	u32				features;
> > >
> > >  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> > > +#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
> > >  	u32				options;
> > >
> > >  	struct arm_smmu_cmdq		cmdq;
> > > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > > arm_smmu_device *smmu, u32 sid)
> > >
> > >  static struct iommu_ops arm_smmu_ops;
> > >
> > > +#ifdef CONFIG_ACPI
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > +	struct iommu_resv_region *region;
> > > +	struct	irq_domain *irq_dom;
> > > +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > +	u64	base;
> > 
> > phys_addr_t
> 
> Ok.
> 
> > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > +	if (irq_dom) {
> > > +		int	ret;
> > > +		u32	rid;
> > > +
> > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > to_pci_dev(dev));
> > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > 
> > Well, here we use ITS id 0 which is fine as long as code in IORT uses the same
> > policy for getting the irq_domain (ie we want to reserve the ITS address
> > space that is actually used by the device to send IRQs not a a different one) it
> > is just a heads-up because I find this confusing.
> 
> Ok. Just to make it clear, 0 is the index into the ITS identifier list.
> I noted that iort_get_device_domain() uses index 0 while retrieving the ITS identifier.
> May be use the same approach here as well? ie, remove the index from function call?
> 
> I am not sure, how we can get the index info  though theoretically It is possible for
> the ITS group node having multiple ITSs.

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

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

> > > +		if (!ret) {
> > > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> > 0x%pa\n", &base);
> > > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > > +							 prot,
> > IOMMU_RESV_MSI);
> > > +			return region;
> > > +		}
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +#else
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > +	return NULL;
> > > +}
> > > +#endif
> > > +
> > >  static int arm_smmu_add_device(struct device *dev)  {
> > >  	int i, ret;
> > > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > > *dev, struct of_phandle_args *args)  static void
> > arm_smmu_get_resv_regions(struct device *dev,
> > >  				      struct list_head *head)
> > >  {
> > > -	struct iommu_resv_region *region;
> > > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > > +	struct iommu_resv_region *region = NULL;
> > >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > +	struct arm_smmu_device *smmu;
> > > +
> > > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> > >
> > > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> > MSI_IOVA_LENGTH,
> > > -					 prot, IOMMU_RESV_SW_MSI);
> > > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> > &&
> > > +		      dev_is_pci(dev))
> > > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> > 
> > Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?
> 
> It is just that PCIe devices won't be functional on this platforms as the endpoint will 
> be configured with ITS IOVA address. May be I should add some dev_warn() here.

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

Lorenzo

Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

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

[...]

> > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > +	if (irq_dom) {
> > > +		int	ret;
> > > +		u32	rid;
> > > +
> > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > to_pci_dev(dev));
> > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > 
> > Well, here we use ITS id 0 which is fine as long as code in IORT uses the same
> > policy for getting the irq_domain (ie we want to reserve the ITS address
> > space that is actually used by the device to send IRQs not a a different one) it
> > is just a heads-up because I find this confusing.
> 
> Ok. Just to make it clear, 0 is the index into the ITS identifier
> list.  I noted that iort_get_device_domain() uses index 0 while
> retrieving the ITS identifier.  May be use the same approach here as
> well? ie, remove the index from function call?
> 
> I am not sure, how we can get the index info  though theoretically It
> is possible for the ITS group node having multiple ITSs.

Actually I think it would make sense to reserve ALL ITS regions a device
may be mapped to instead of just index 0 (ie in your case it is
equivalent); this leaves us some leeway as to choose which ITS the
device will be actually mapped to and this code does not have to care.

Lorenzo

>  
> > > +		if (!ret) {
> > > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> > 0x%pa\n", &base);
> > > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > > +							 prot,
> > IOMMU_RESV_MSI);
> > > +			return region;
> > > +		}
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +#else
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > +	return NULL;
> > > +}
> > > +#endif
> > > +
> > >  static int arm_smmu_add_device(struct device *dev)  {
> > >  	int i, ret;
> > > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > > *dev, struct of_phandle_args *args)  static void
> > arm_smmu_get_resv_regions(struct device *dev,
> > >  				      struct list_head *head)
> > >  {
> > > -	struct iommu_resv_region *region;
> > > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > > +	struct iommu_resv_region *region = NULL;
> > >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > +	struct arm_smmu_device *smmu;
> > > +
> > > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> > >
> > > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> > MSI_IOVA_LENGTH,
> > > -					 prot, IOMMU_RESV_SW_MSI);
> > > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> > &&
> > > +		      dev_is_pci(dev))
> > > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> > 
> > Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?
> 
> It is just that PCIe devices won't be functional on this platforms as the endpoint will 
> be configured with ITS IOVA address. May be I should add some dev_warn() here.
> 
> Thanks,
> Shameer

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

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

[...]

> > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > +	if (irq_dom) {
> > > +		int	ret;
> > > +		u32	rid;
> > > +
> > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > to_pci_dev(dev));
> > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > 
> > Well, here we use ITS id 0 which is fine as long as code in IORT uses the same
> > policy for getting the irq_domain (ie we want to reserve the ITS address
> > space that is actually used by the device to send IRQs not a a different one) it
> > is just a heads-up because I find this confusing.
> 
> Ok. Just to make it clear, 0 is the index into the ITS identifier
> list.  I noted that iort_get_device_domain() uses index 0 while
> retrieving the ITS identifier.  May be use the same approach here as
> well? ie, remove the index from function call?
> 
> I am not sure, how we can get the index info  though theoretically It
> is possible for the ITS group node having multiple ITSs.

Actually I think it would make sense to reserve ALL ITS regions a device
may be mapped to instead of just index 0 (ie in your case it is
equivalent); this leaves us some leeway as to choose which ITS the
device will be actually mapped to and this code does not have to care.

Lorenzo

>  
> > > +		if (!ret) {
> > > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> > 0x%pa\n", &base);
> > > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > > +							 prot,
> > IOMMU_RESV_MSI);
> > > +			return region;
> > > +		}
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +#else
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > +	return NULL;
> > > +}
> > > +#endif
> > > +
> > >  static int arm_smmu_add_device(struct device *dev)  {
> > >  	int i, ret;
> > > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > > *dev, struct of_phandle_args *args)  static void
> > arm_smmu_get_resv_regions(struct device *dev,
> > >  				      struct list_head *head)
> > >  {
> > > -	struct iommu_resv_region *region;
> > > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > > +	struct iommu_resv_region *region = NULL;
> > >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > +	struct arm_smmu_device *smmu;
> > > +
> > > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> > >
> > > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> > MSI_IOVA_LENGTH,
> > > -					 prot, IOMMU_RESV_SW_MSI);
> > > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> > &&
> > > +		      dev_is_pci(dev))
> > > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> > 
> > Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?
> 
> It is just that PCIe devices won't be functional on this platforms as the endpoint will 
> be configured with ITS IOVA address. May be I should add some dev_warn() here.
> 
> Thanks,
> Shameer

Re: [Devel] [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

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

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

[...]

> > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > +	if (irq_dom) {
> > > +		int	ret;
> > > +		u32	rid;
> > > +
> > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > to_pci_dev(dev));
> > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > 
> > Well, here we use ITS id 0 which is fine as long as code in IORT uses the same
> > policy for getting the irq_domain (ie we want to reserve the ITS address
> > space that is actually used by the device to send IRQs not a a different one) it
> > is just a heads-up because I find this confusing.
> 
> Ok. Just to make it clear, 0 is the index into the ITS identifier
> list.  I noted that iort_get_device_domain() uses index 0 while
> retrieving the ITS identifier.  May be use the same approach here as
> well? ie, remove the index from function call?
> 
> I am not sure, how we can get the index info  though theoretically It
> is possible for the ITS group node having multiple ITSs.

Actually I think it would make sense to reserve ALL ITS regions a device
may be mapped to instead of just index 0 (ie in your case it is
equivalent); this leaves us some leeway as to choose which ITS the
device will be actually mapped to and this code does not have to care.

Lorenzo

>  
> > > +		if (!ret) {
> > > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> > 0x%pa\n", &base);
> > > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > > +							 prot,
> > IOMMU_RESV_MSI);
> > > +			return region;
> > > +		}
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +#else
> > > +static struct iommu_resv_region *arm_smmu_acpi_alloc_hw_msi(struct
> > > +device *dev) {
> > > +	return NULL;
> > > +}
> > > +#endif
> > > +
> > >  static int arm_smmu_add_device(struct device *dev)  {
> > >  	int i, ret;
> > > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > > *dev, struct of_phandle_args *args)  static void
> > arm_smmu_get_resv_regions(struct device *dev,
> > >  				      struct list_head *head)
> > >  {
> > > -	struct iommu_resv_region *region;
> > > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > > +	struct iommu_resv_region *region = NULL;
> > >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > +	struct arm_smmu_device *smmu;
> > > +
> > > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> > >
> > > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> > MSI_IOVA_LENGTH,
> > > -					 prot, IOMMU_RESV_SW_MSI);
> > > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> > &&
> > > +		      dev_is_pci(dev))
> > > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> > 
> > Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL here ?
> 
> It is just that PCIe devices won't be functional on this platforms as the endpoint will 
> be configured with ITS IOVA address. May be I should add some dev_warn() here.
> 
> Thanks,
> Shameer

RE: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org]
> Sent: Thursday, June 08, 2017 9:49 AM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier-5wv7dgnIgG8@public.gmane.org; sudeep.holla-5wv7dgnIgG8@public.gmane.org; will.deacon-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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> wrote:
> 
> [...]
> 
> > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > +	if (irq_dom) {
> > > > +		int	ret;
> > > > +		u32	rid;
> > > > +
> > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > to_pci_dev(dev));
> > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > >
> > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > uses the same policy for getting the irq_domain (ie we want to
> > > reserve the ITS address space that is actually used by the device to
> > > send IRQs not a a different one) it is just a heads-up because I find this
> confusing.
> >
> > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > list.  I noted that iort_get_device_domain() uses index 0 while
> > retrieving the ITS identifier.  May be use the same approach here as
> > well? ie, remove the index from function call?
> >
> > I am not sure, how we can get the index info  though theoretically It
> > is possible for the ITS group node having multiple ITSs.
> 
> Actually I think it would make sense to reserve ALL ITS regions a device may
> be mapped to instead of just index 0 (ie in your case it is equivalent); this
> leaves us some leeway as to choose which ITS the device will be actually
> mapped to and this code does not have to care.

Ok. That make sense. Just a quick one, is it ok to add another helper function in
iort code to retrieve the its->its_count then? 

Thanks,
Shameer

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: Thursday, June 08, 2017 9:49 AM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier at arm.com; sudeep.holla at arm.com; will.deacon at arm.com;
> robin.murphy at arm.com; hanjun.guo at linaro.org; Gabriele Paoloni; John
> Garry; iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org; linux-acpi at vger.kernel.org; devel at acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> wrote:
> 
> [...]
> 
> > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > +	if (irq_dom) {
> > > > +		int	ret;
> > > > +		u32	rid;
> > > > +
> > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > to_pci_dev(dev));
> > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > >
> > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > uses the same policy for getting the irq_domain (ie we want to
> > > reserve the ITS address space that is actually used by the device to
> > > send IRQs not a a different one) it is just a heads-up because I find this
> confusing.
> >
> > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > list.  I noted that iort_get_device_domain() uses index 0 while
> > retrieving the ITS identifier.  May be use the same approach here as
> > well? ie, remove the index from function call?
> >
> > I am not sure, how we can get the index info  though theoretically It
> > is possible for the ITS group node having multiple ITSs.
> 
> Actually I think it would make sense to reserve ALL ITS regions a device may
> be mapped to instead of just index 0 (ie in your case it is equivalent); this
> leaves us some leeway as to choose which ITS the device will be actually
> mapped to and this code does not have to care.

Ok. That make sense. Just a quick one, is it ok to add another helper function in
iort code to retrieve the its->its_count then? 

Thanks,
Shameer

Re: [Devel] [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

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



> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi(a)arm.com]
> Sent: Thursday, June 08, 2017 9:49 AM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier(a)arm.com; sudeep.holla(a)arm.com; will.deacon(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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> wrote:
> 
> [...]
> 
> > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > +	if (irq_dom) {
> > > > +		int	ret;
> > > > +		u32	rid;
> > > > +
> > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > to_pci_dev(dev));
> > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > >
> > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > uses the same policy for getting the irq_domain (ie we want to
> > > reserve the ITS address space that is actually used by the device to
> > > send IRQs not a a different one) it is just a heads-up because I find this
> confusing.
> >
> > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > list.  I noted that iort_get_device_domain() uses index 0 while
> > retrieving the ITS identifier.  May be use the same approach here as
> > well? ie, remove the index from function call?
> >
> > I am not sure, how we can get the index info  though theoretically It
> > is possible for the ITS group node having multiple ITSs.
> 
> Actually I think it would make sense to reserve ALL ITS regions a device may
> be mapped to instead of just index 0 (ie in your case it is equivalent); this
> leaves us some leeway as to choose which ITS the device will be actually
> mapped to and this code does not have to care.

Ok. That make sense. Just a quick one, is it ok to add another helper function in
iort code to retrieve the its->its_count then? 

Thanks,
Shameer

RE: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org]
> Sent: Wednesday, June 07, 2017 6:16 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier-5wv7dgnIgG8@public.gmane.org; sudeep.holla-5wv7dgnIgG8@public.gmane.org; will.deacon-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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> wrote:
> > Hi Lorenzo,
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org]
> > > Sent: Tuesday, June 06, 2017 2:56 PM
> > > To: Shameerali Kolothum Thodi
> > > Cc: marc.zyngier-5wv7dgnIgG8@public.gmane.org; sudeep.holla-5wv7dgnIgG8@public.gmane.org;
> will.deacon-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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon
> > > erratum 161010801
> > >
> > > On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > > > 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.
> > > >
> > > > The HW ITS address region associated with the dev is retrieved using a
> > > > new helper function added in the IORT code.
> > >
> > > Remove or rephrase last paragraph, it reads as if you are adding an IORT
> > > helper function in this patch but you actually aren't.
> >
> > Thanks for going through this patch series. I will remove this in next
> version.
> >
> > > > Signed-off-by: shameer <shameerali.kolothum.thodi-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  drivers/iommu/arm-smmu-v3.c | 49
> > > > ++++++++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 46 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-
> smmu-
> > > v3.c
> > > > index abe4b88..3767526 100644
> > > > --- a/drivers/iommu/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> > > >  	u32				features;
> > > >
> > > >  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> > > > +#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
> > > >  	u32				options;
> > > >
> > > >  	struct arm_smmu_cmdq		cmdq;
> > > > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > > > arm_smmu_device *smmu, u32 sid)
> > > >
> > > >  static struct iommu_ops arm_smmu_ops;
> > > >
> > > > +#ifdef CONFIG_ACPI
> > > > +static struct iommu_resv_region
> *arm_smmu_acpi_alloc_hw_msi(struct
> > > > +device *dev) {
> > > > +	struct iommu_resv_region *region;
> > > > +	struct	irq_domain *irq_dom;
> > > > +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > > +	u64	base;
> > >
> > > phys_addr_t
> >
> > Ok.
> >
> > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > +	if (irq_dom) {
> > > > +		int	ret;
> > > > +		u32	rid;
> > > > +
> > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > to_pci_dev(dev));
> > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > >
> > > Well, here we use ITS id 0 which is fine as long as code in IORT uses the
> same
> > > policy for getting the irq_domain (ie we want to reserve the ITS address
> > > space that is actually used by the device to send IRQs not a a different
> one) it
> > > is just a heads-up because I find this confusing.
> >
> > Ok. Just to make it clear, 0 is the index into the ITS identifier list.
> > I noted that iort_get_device_domain() uses index 0 while retrieving the ITS
> identifier.
> > May be use the same approach here as well? ie, remove the index from
> function call?
> >
> > I am not sure, how we can get the index info  though theoretically It is
> possible for
> > the ITS group node having multiple ITSs.
> 
> Yes, it would be ideal to avoid the look-up through the ITS index and
> just reuse the ITS node associated with the MSI domain because I do not
> want this quirk to force the ITS domain allocation policy (what I mean
> I do not want to be tied to index 0 if for any reason we change
> the allocation in IORT for normal ITS<->device mapping).
> 
> I will have a further look to see if we can improve the code to
> this extent.
> 
> > > > +		if (!ret) {
> > > > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> > > 0x%pa\n", &base);
> > > > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > > > +							 prot,
> > > IOMMU_RESV_MSI);
> > > > +			return region;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return NULL;
> > > > +}
> > > > +#else
> > > > +static struct iommu_resv_region
> *arm_smmu_acpi_alloc_hw_msi(struct
> > > > +device *dev) {
> > > > +	return NULL;
> > > > +}
> > > > +#endif
> > > > +
> > > >  static int arm_smmu_add_device(struct device *dev)  {
> > > >  	int i, ret;
> > > > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > > > *dev, struct of_phandle_args *args)  static void
> > > arm_smmu_get_resv_regions(struct device *dev,
> > > >  				      struct list_head *head)
> > > >  {
> > > > -	struct iommu_resv_region *region;
> > > > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > > > +	struct iommu_resv_region *region = NULL;
> > > >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > > +	struct arm_smmu_device *smmu;
> > > > +
> > > > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> > > >
> > > > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> > > MSI_IOVA_LENGTH,
> > > > -					 prot, IOMMU_RESV_SW_MSI);
> > > > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> > > &&
> > > > +		      dev_is_pci(dev))
> > > > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> > >
> > > Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL
> here ?
> >
> > It is just that PCIe devices won't be functional on this platforms as the
> endpoint will
> > be configured with ITS IOVA address. May be I should add some
> dev_warn() here.
> 
> Well yes and also I am not sure that if arm_smmu_acpi_alloc_hw_msi()
> fails you should allocate the SW_MSI region I am not sure I understand
> the logic, so you should add a warning and just return on failure right ?

True. There is no point in reserving the SW_MSI on these platforms. I think
it's better to return.

Shameer

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: Wednesday, June 07, 2017 6:16 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier at arm.com; sudeep.holla at arm.com; will.deacon at arm.com;
> robin.murphy at arm.com; hanjun.guo at linaro.org; Gabriele Paoloni; John
> Garry; iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org; linux-acpi at vger.kernel.org; devel at acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> wrote:
> > Hi Lorenzo,
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> > > Sent: Tuesday, June 06, 2017 2:56 PM
> > > To: Shameerali Kolothum Thodi
> > > Cc: marc.zyngier at arm.com; sudeep.holla at arm.com;
> will.deacon at arm.com;
> > > robin.murphy at arm.com; hanjun.guo at linaro.org; Gabriele Paoloni; John
> > > Garry; iommu at lists.linux-foundation.org; linux-arm-
> > > kernel at lists.infradead.org; linux-acpi at vger.kernel.org;
> devel at acpica.org;
> > > Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> > > Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon
> > > erratum 161010801
> > >
> > > On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > > > 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.
> > > >
> > > > The HW ITS address region associated with the dev is retrieved using a
> > > > new helper function added in the IORT code.
> > >
> > > Remove or rephrase last paragraph, it reads as if you are adding an IORT
> > > helper function in this patch but you actually aren't.
> >
> > Thanks for going through this patch series. I will remove this in next
> version.
> >
> > > > Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
> > > > ---
> > > >  drivers/iommu/arm-smmu-v3.c | 49
> > > > ++++++++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 46 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-
> smmu-
> > > v3.c
> > > > index abe4b88..3767526 100644
> > > > --- a/drivers/iommu/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> > > >  	u32				features;
> > > >
> > > >  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> > > > +#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
> > > >  	u32				options;
> > > >
> > > >  	struct arm_smmu_cmdq		cmdq;
> > > > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > > > arm_smmu_device *smmu, u32 sid)
> > > >
> > > >  static struct iommu_ops arm_smmu_ops;
> > > >
> > > > +#ifdef CONFIG_ACPI
> > > > +static struct iommu_resv_region
> *arm_smmu_acpi_alloc_hw_msi(struct
> > > > +device *dev) {
> > > > +	struct iommu_resv_region *region;
> > > > +	struct	irq_domain *irq_dom;
> > > > +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > > +	u64	base;
> > >
> > > phys_addr_t
> >
> > Ok.
> >
> > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > +	if (irq_dom) {
> > > > +		int	ret;
> > > > +		u32	rid;
> > > > +
> > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > to_pci_dev(dev));
> > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > >
> > > Well, here we use ITS id 0 which is fine as long as code in IORT uses the
> same
> > > policy for getting the irq_domain (ie we want to reserve the ITS address
> > > space that is actually used by the device to send IRQs not a a different
> one) it
> > > is just a heads-up because I find this confusing.
> >
> > Ok. Just to make it clear, 0 is the index into the ITS identifier list.
> > I noted that iort_get_device_domain() uses index 0 while retrieving the ITS
> identifier.
> > May be use the same approach here as well? ie, remove the index from
> function call?
> >
> > I am not sure, how we can get the index info  though theoretically It is
> possible for
> > the ITS group node having multiple ITSs.
> 
> Yes, it would be ideal to avoid the look-up through the ITS index and
> just reuse the ITS node associated with the MSI domain because I do not
> want this quirk to force the ITS domain allocation policy (what I mean
> I do not want to be tied to index 0 if for any reason we change
> the allocation in IORT for normal ITS<->device mapping).
> 
> I will have a further look to see if we can improve the code to
> this extent.
> 
> > > > +		if (!ret) {
> > > > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> > > 0x%pa\n", &base);
> > > > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > > > +							 prot,
> > > IOMMU_RESV_MSI);
> > > > +			return region;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return NULL;
> > > > +}
> > > > +#else
> > > > +static struct iommu_resv_region
> *arm_smmu_acpi_alloc_hw_msi(struct
> > > > +device *dev) {
> > > > +	return NULL;
> > > > +}
> > > > +#endif
> > > > +
> > > >  static int arm_smmu_add_device(struct device *dev)  {
> > > >  	int i, ret;
> > > > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > > > *dev, struct of_phandle_args *args)  static void
> > > arm_smmu_get_resv_regions(struct device *dev,
> > > >  				      struct list_head *head)
> > > >  {
> > > > -	struct iommu_resv_region *region;
> > > > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > > > +	struct iommu_resv_region *region = NULL;
> > > >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > > +	struct arm_smmu_device *smmu;
> > > > +
> > > > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> > > >
> > > > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> > > MSI_IOVA_LENGTH,
> > > > -					 prot, IOMMU_RESV_SW_MSI);
> > > > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> > > &&
> > > > +		      dev_is_pci(dev))
> > > > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> > >
> > > Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL
> here ?
> >
> > It is just that PCIe devices won't be functional on this platforms as the
> endpoint will
> > be configured with ITS IOVA address. May be I should add some
> dev_warn() here.
> 
> Well yes and also I am not sure that if arm_smmu_acpi_alloc_hw_msi()
> fails you should allocate the SW_MSI region I am not sure I understand
> the logic, so you should add a warning and just return on failure right ?

True. There is no point in reserving the SW_MSI on these platforms. I think
it's better to return.

Shameer

Re: [Devel] [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

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



> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi(a)arm.com]
> Sent: Wednesday, June 07, 2017 6:16 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier(a)arm.com; sudeep.holla(a)arm.com; will.deacon(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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> wrote:
> > Hi Lorenzo,
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi(a)arm.com]
> > > Sent: Tuesday, June 06, 2017 2:56 PM
> > > To: Shameerali Kolothum Thodi
> > > Cc: marc.zyngier(a)arm.com; sudeep.holla(a)arm.com;
> will.deacon(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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon
> > > erratum 161010801
> > >
> > > On Wed, May 31, 2017 at 03:32:13PM +0100, shameer wrote:
> > > > 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.
> > > >
> > > > The HW ITS address region associated with the dev is retrieved using a
> > > > new helper function added in the IORT code.
> > >
> > > Remove or rephrase last paragraph, it reads as if you are adding an IORT
> > > helper function in this patch but you actually aren't.
> >
> > Thanks for going through this patch series. I will remove this in next
> version.
> >
> > > > Signed-off-by: shameer <shameerali.kolothum.thodi(a)huawei.com>
> > > > ---
> > > >  drivers/iommu/arm-smmu-v3.c | 49
> > > > ++++++++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 46 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-
> smmu-
> > > v3.c
> > > > index abe4b88..3767526 100644
> > > > --- a/drivers/iommu/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > > @@ -597,6 +597,7 @@ struct arm_smmu_device {
> > > >  	u32				features;
> > > >
> > > >  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> > > > +#define ARM_SMMU_OPT_RESV_HW_MSI	(1 << 1)
> > > >  	u32				options;
> > > >
> > > >  	struct arm_smmu_cmdq		cmdq;
> > > > @@ -1755,6 +1756,38 @@ static bool arm_smmu_sid_in_range(struct
> > > > arm_smmu_device *smmu, u32 sid)
> > > >
> > > >  static struct iommu_ops arm_smmu_ops;
> > > >
> > > > +#ifdef CONFIG_ACPI
> > > > +static struct iommu_resv_region
> *arm_smmu_acpi_alloc_hw_msi(struct
> > > > +device *dev) {
> > > > +	struct iommu_resv_region *region;
> > > > +	struct	irq_domain *irq_dom;
> > > > +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > > +	u64	base;
> > >
> > > phys_addr_t
> >
> > Ok.
> >
> > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > +	if (irq_dom) {
> > > > +		int	ret;
> > > > +		u32	rid;
> > > > +
> > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > to_pci_dev(dev));
> > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > >
> > > Well, here we use ITS id 0 which is fine as long as code in IORT uses the
> same
> > > policy for getting the irq_domain (ie we want to reserve the ITS address
> > > space that is actually used by the device to send IRQs not a a different
> one) it
> > > is just a heads-up because I find this confusing.
> >
> > Ok. Just to make it clear, 0 is the index into the ITS identifier list.
> > I noted that iort_get_device_domain() uses index 0 while retrieving the ITS
> identifier.
> > May be use the same approach here as well? ie, remove the index from
> function call?
> >
> > I am not sure, how we can get the index info  though theoretically It is
> possible for
> > the ITS group node having multiple ITSs.
> 
> Yes, it would be ideal to avoid the look-up through the ITS index and
> just reuse the ITS node associated with the MSI domain because I do not
> want this quirk to force the ITS domain allocation policy (what I mean
> I do not want to be tied to index 0 if for any reason we change
> the allocation in IORT for normal ITS<->device mapping).
> 
> I will have a further look to see if we can improve the code to
> this extent.
> 
> > > > +		if (!ret) {
> > > > +			dev_info(dev, "SMMUv3:HW MSI resv addr
> > > 0x%pa\n", &base);
> > > > +			region = iommu_alloc_resv_region(base, SZ_128K,
> > > > +							 prot,
> > > IOMMU_RESV_MSI);
> > > > +			return region;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return NULL;
> > > > +}
> > > > +#else
> > > > +static struct iommu_resv_region
> *arm_smmu_acpi_alloc_hw_msi(struct
> > > > +device *dev) {
> > > > +	return NULL;
> > > > +}
> > > > +#endif
> > > > +
> > > >  static int arm_smmu_add_device(struct device *dev)  {
> > > >  	int i, ret;
> > > > @@ -1903,11 +1936,20 @@ static int arm_smmu_of_xlate(struct device
> > > > *dev, struct of_phandle_args *args)  static void
> > > arm_smmu_get_resv_regions(struct device *dev,
> > > >  				      struct list_head *head)
> > > >  {
> > > > -	struct iommu_resv_region *region;
> > > > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > > > +	struct iommu_resv_region *region = NULL;
> > > >  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> > > > +	struct arm_smmu_device *smmu;
> > > > +
> > > > +	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> > > >
> > > > -	region = iommu_alloc_resv_region(MSI_IOVA_BASE,
> > > MSI_IOVA_LENGTH,
> > > > -					 prot, IOMMU_RESV_SW_MSI);
> > > > +	if (smmu && (smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)
> > > &&
> > > > +		      dev_is_pci(dev))
> > > > +		region = arm_smmu_acpi_alloc_hw_msi(dev);
> > >
> > > Is it safe to carry on if arm_smmu_acpi_alloc_hw_msi() returns NULL
> here ?
> >
> > It is just that PCIe devices won't be functional on this platforms as the
> endpoint will
> > be configured with ITS IOVA address. May be I should add some
> dev_warn() here.
> 
> Well yes and also I am not sure that if arm_smmu_acpi_alloc_hw_msi()
> fails you should allocate the SW_MSI region I am not sure I understand
> the logic, so you should add a warning and just return on failure right ?

True. There is no point in reserving the SW_MSI on these platforms. I think
it's better to return.

Shameer

Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

On Thu, Jun 08, 2017 at 09:09:28AM +0000, Shameerali Kolothum Thodi wrote:
> 
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> > Sent: Thursday, June 08, 2017 9:49 AM
> > To: Shameerali Kolothum Thodi
> > Cc: marc.zyngier@arm.com; sudeep.holla@arm.com; will.deacon@arm.com;
> > robin.murphy@arm.com; hanjun.guo@linaro.org; Gabriele Paoloni; John
> > Garry; iommu@lists.linux-foundation.org; linux-arm-
> > kernel@lists.infradead.org; linux-acpi@vger.kernel.org; devel@acpica.org;
> > Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> > Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> > erratum 161010801
> > 
> > On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> > wrote:
> > 
> > [...]
> > 
> > > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > > +	if (irq_dom) {
> > > > > +		int	ret;
> > > > > +		u32	rid;
> > > > > +
> > > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > > to_pci_dev(dev));
> > > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > > >
> > > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > > uses the same policy for getting the irq_domain (ie we want to
> > > > reserve the ITS address space that is actually used by the device to
> > > > send IRQs not a a different one) it is just a heads-up because I find this
> > confusing.
> > >
> > > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > > list.  I noted that iort_get_device_domain() uses index 0 while
> > > retrieving the ITS identifier.  May be use the same approach here as
> > > well? ie, remove the index from function call?
> > >
> > > I am not sure, how we can get the index info  though theoretically It
> > > is possible for the ITS group node having multiple ITSs.
> > 
> > Actually I think it would make sense to reserve ALL ITS regions a device may
> > be mapped to instead of just index 0 (ie in your case it is equivalent); this
> > leaves us some leeway as to choose which ITS the device will be actually
> > mapped to and this code does not have to care.
> 
> Ok. That make sense. Just a quick one, is it ok to add another helper function in
> iort code to retrieve the its->its_count then? 

While at it, given that the pci API code to retrieve domain and rid falls
back to IORT anyway, I would add the whole reservation to IORT (mind,
it depends on IOMMU_API) as one function instead of fiddling about with
indexes.

Side note: why Hilisicon dts upstream (eg hip07.dtsi) report ITS size
as 256K ? I was just checking whether the ITS reg map size is system
dependent and I bumped into them, I suspect there may be some dts
patching needed here.

Lorenzo

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

On Thu, Jun 08, 2017 at 09:09:28AM +0000, Shameerali Kolothum Thodi wrote:
> 
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> > Sent: Thursday, June 08, 2017 9:49 AM
> > To: Shameerali Kolothum Thodi
> > Cc: marc.zyngier at arm.com; sudeep.holla at arm.com; will.deacon at arm.com;
> > robin.murphy at arm.com; hanjun.guo at linaro.org; Gabriele Paoloni; John
> > Garry; iommu at lists.linux-foundation.org; linux-arm-
> > kernel at lists.infradead.org; linux-acpi at vger.kernel.org; devel at acpica.org;
> > Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> > Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> > erratum 161010801
> > 
> > On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> > wrote:
> > 
> > [...]
> > 
> > > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > > +	if (irq_dom) {
> > > > > +		int	ret;
> > > > > +		u32	rid;
> > > > > +
> > > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > > to_pci_dev(dev));
> > > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > > >
> > > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > > uses the same policy for getting the irq_domain (ie we want to
> > > > reserve the ITS address space that is actually used by the device to
> > > > send IRQs not a a different one) it is just a heads-up because I find this
> > confusing.
> > >
> > > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > > list.  I noted that iort_get_device_domain() uses index 0 while
> > > retrieving the ITS identifier.  May be use the same approach here as
> > > well? ie, remove the index from function call?
> > >
> > > I am not sure, how we can get the index info  though theoretically It
> > > is possible for the ITS group node having multiple ITSs.
> > 
> > Actually I think it would make sense to reserve ALL ITS regions a device may
> > be mapped to instead of just index 0 (ie in your case it is equivalent); this
> > leaves us some leeway as to choose which ITS the device will be actually
> > mapped to and this code does not have to care.
> 
> Ok. That make sense. Just a quick one, is it ok to add another helper function in
> iort code to retrieve the its->its_count then? 

While at it, given that the pci API code to retrieve domain and rid falls
back to IORT anyway, I would add the whole reservation to IORT (mind,
it depends on IOMMU_API) as one function instead of fiddling about with
indexes.

Side note: why Hilisicon dts upstream (eg hip07.dtsi) report ITS size
as 256K ? I was just checking whether the ITS reg map size is system
dependent and I bumped into them, I suspect there may be some dts
patching needed here.

Lorenzo

Re: [Devel] [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi]

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

On Thu, Jun 08, 2017 at 09:09:28AM +0000, Shameerali Kolothum Thodi wrote:
> 
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi(a)arm.com]
> > Sent: Thursday, June 08, 2017 9:49 AM
> > To: Shameerali Kolothum Thodi
> > Cc: marc.zyngier(a)arm.com; sudeep.holla(a)arm.com; will.deacon(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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> > erratum 161010801
> > 
> > On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> > wrote:
> > 
> > [...]
> > 
> > > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > > +	if (irq_dom) {
> > > > > +		int	ret;
> > > > > +		u32	rid;
> > > > > +
> > > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > > to_pci_dev(dev));
> > > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > > >
> > > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > > uses the same policy for getting the irq_domain (ie we want to
> > > > reserve the ITS address space that is actually used by the device to
> > > > send IRQs not a a different one) it is just a heads-up because I find this
> > confusing.
> > >
> > > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > > list.  I noted that iort_get_device_domain() uses index 0 while
> > > retrieving the ITS identifier.  May be use the same approach here as
> > > well? ie, remove the index from function call?
> > >
> > > I am not sure, how we can get the index info  though theoretically It
> > > is possible for the ITS group node having multiple ITSs.
> > 
> > Actually I think it would make sense to reserve ALL ITS regions a device may
> > be mapped to instead of just index 0 (ie in your case it is equivalent); this
> > leaves us some leeway as to choose which ITS the device will be actually
> > mapped to and this code does not have to care.
> 
> Ok. That make sense. Just a quick one, is it ok to add another helper function in
> iort code to retrieve the its->its_count then? 

While at it, given that the pci API code to retrieve domain and rid falls
back to IORT anyway, I would add the whole reservation to IORT (mind,
it depends on IOMMU_API) as one function instead of fiddling about with
indexes.

Side note: why Hilisicon dts upstream (eg hip07.dtsi) report ITS size
as 256K ? I was just checking whether the ITS reg map size is system
dependent and I bumped into them, I suspect there may be some dts
patching needed here.

Lorenzo

RE: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org]
> Sent: Thursday, June 08, 2017 11:15 AM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier-5wv7dgnIgG8@public.gmane.org; sudeep.holla-5wv7dgnIgG8@public.gmane.org; will.deacon-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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Thu, Jun 08, 2017 at 09:09:28AM +0000, Shameerali Kolothum Thodi
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org]
> > > Sent: Thursday, June 08, 2017 9:49 AM
> > > To: Shameerali Kolothum Thodi
> > > Cc: marc.zyngier-5wv7dgnIgG8@public.gmane.org; sudeep.holla-5wv7dgnIgG8@public.gmane.org;
> will.deacon-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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon
> > > erratum 161010801
> > >
> > > On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> > > wrote:
> > >
> > > [...]
> > >
> > > > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > > > +	if (irq_dom) {
> > > > > > +		int	ret;
> > > > > > +		u32	rid;
> > > > > > +
> > > > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > > > to_pci_dev(dev));
> > > > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > > > >
> > > > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > > > uses the same policy for getting the irq_domain (ie we want to
> > > > > reserve the ITS address space that is actually used by the device to
> > > > > send IRQs not a a different one) it is just a heads-up because I find
> this
> > > confusing.
> > > >
> > > > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > > > list.  I noted that iort_get_device_domain() uses index 0 while
> > > > retrieving the ITS identifier.  May be use the same approach here as
> > > > well? ie, remove the index from function call?
> > > >
> > > > I am not sure, how we can get the index info  though theoretically It
> > > > is possible for the ITS group node having multiple ITSs.
> > >
> > > Actually I think it would make sense to reserve ALL ITS regions a device
> may
> > > be mapped to instead of just index 0 (ie in your case it is equivalent); this
> > > leaves us some leeway as to choose which ITS the device will be actually
> > > mapped to and this code does not have to care.
> >
> > Ok. That make sense. Just a quick one, is it ok to add another helper
> function in
> > iort code to retrieve the its->its_count then?
> 
> While at it, given that the pci API code to retrieve domain and rid falls
> back to IORT anyway, I would add the whole reservation to IORT (mind,
> it depends on IOMMU_API) as one function instead of fiddling about with
> indexes.

Ok. I will take a look at this. 

> Side note: why Hilisicon dts upstream (eg hip07.dtsi) report ITS size
> as 256K ? I was just checking whether the ITS reg map size is system
> dependent and I bumped into them, I suspect there may be some dts
> patching needed here.
> 

Thanks for catching this. I will check with the team and update.

Shameer

[RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: Thursday, June 08, 2017 11:15 AM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier at arm.com; sudeep.holla at arm.com; will.deacon at arm.com;
> robin.murphy at arm.com; hanjun.guo at linaro.org; Gabriele Paoloni; John
> Garry; iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org; linux-acpi at vger.kernel.org; devel at acpica.org;
> Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Thu, Jun 08, 2017 at 09:09:28AM +0000, Shameerali Kolothum Thodi
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> > > Sent: Thursday, June 08, 2017 9:49 AM
> > > To: Shameerali Kolothum Thodi
> > > Cc: marc.zyngier at arm.com; sudeep.holla at arm.com;
> will.deacon at arm.com;
> > > robin.murphy at arm.com; hanjun.guo at linaro.org; Gabriele Paoloni; John
> > > Garry; iommu at lists.linux-foundation.org; linux-arm-
> > > kernel at lists.infradead.org; linux-acpi at vger.kernel.org;
> devel at acpica.org;
> > > Linuxarm; Wangzhou (B); Guohanjun (Hanjun Guo)
> > > Subject: Re: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon
> > > erratum 161010801
> > >
> > > On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> > > wrote:
> > >
> > > [...]
> > >
> > > > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > > > +	if (irq_dom) {
> > > > > > +		int	ret;
> > > > > > +		u32	rid;
> > > > > > +
> > > > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > > > to_pci_dev(dev));
> > > > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > > > >
> > > > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > > > uses the same policy for getting the irq_domain (ie we want to
> > > > > reserve the ITS address space that is actually used by the device to
> > > > > send IRQs not a a different one) it is just a heads-up because I find
> this
> > > confusing.
> > > >
> > > > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > > > list.  I noted that iort_get_device_domain() uses index 0 while
> > > > retrieving the ITS identifier.  May be use the same approach here as
> > > > well? ie, remove the index from function call?
> > > >
> > > > I am not sure, how we can get the index info  though theoretically It
> > > > is possible for the ITS group node having multiple ITSs.
> > >
> > > Actually I think it would make sense to reserve ALL ITS regions a device
> may
> > > be mapped to instead of just index 0 (ie in your case it is equivalent); this
> > > leaves us some leeway as to choose which ITS the device will be actually
> > > mapped to and this code does not have to care.
> >
> > Ok. That make sense. Just a quick one, is it ok to add another helper
> function in
> > iort code to retrieve the its->its_count then?
> 
> While at it, given that the pci API code to retrieve domain and rid falls
> back to IORT anyway, I would add the whole reservation to IORT (mind,
> it depends on IOMMU_API) as one function instead of fiddling about with
> indexes.

Ok. I will take a look at this. 

> Side note: why Hilisicon dts upstream (eg hip07.dtsi) report ITS size
> as 256K ? I was just checking whether the ITS reg map size is system
> dependent and I bumped into them, I suspect there may be some dts
> patching needed here.
> 

Thanks for catching this. I will check with the team and update.

Shameer

Re: [Devel] [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

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



> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi(a)arm.com]
> Sent: Thursday, June 08, 2017 11:15 AM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier(a)arm.com; sudeep.holla(a)arm.com; will.deacon(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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon
> erratum 161010801
> 
> On Thu, Jun 08, 2017 at 09:09:28AM +0000, Shameerali Kolothum Thodi
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi(a)arm.com]
> > > Sent: Thursday, June 08, 2017 9:49 AM
> > > To: Shameerali Kolothum Thodi
> > > Cc: marc.zyngier(a)arm.com; sudeep.holla(a)arm.com;
> will.deacon(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: [RFCv2 2/2] iommu/arm-smmu-v3:Enable ACPI based
> HiSilicon
> > > erratum 161010801
> > >
> > > On Tue, Jun 06, 2017 at 03:01:36PM +0000, Shameerali Kolothum Thodi
> > > wrote:
> > >
> > > [...]
> > >
> > > > > > +	irq_dom = pci_msi_get_device_domain(to_pci_dev(dev));
> > > > > > +	if (irq_dom) {
> > > > > > +		int	ret;
> > > > > > +		u32	rid;
> > > > > > +
> > > > > > +		rid = pci_msi_domain_get_msi_rid(irq_dom,
> > > > > to_pci_dev(dev));
> > > > > > +		ret = iort_dev_find_its_base(dev, rid, 0, &base);
> > > > >
> > > > > Well, here we use ITS id 0 which is fine as long as code in IORT
> > > > > uses the same policy for getting the irq_domain (ie we want to
> > > > > reserve the ITS address space that is actually used by the device to
> > > > > send IRQs not a a different one) it is just a heads-up because I find
> this
> > > confusing.
> > > >
> > > > Ok. Just to make it clear, 0 is the index into the ITS identifier
> > > > list.  I noted that iort_get_device_domain() uses index 0 while
> > > > retrieving the ITS identifier.  May be use the same approach here as
> > > > well? ie, remove the index from function call?
> > > >
> > > > I am not sure, how we can get the index info  though theoretically It
> > > > is possible for the ITS group node having multiple ITSs.
> > >
> > > Actually I think it would make sense to reserve ALL ITS regions a device
> may
> > > be mapped to instead of just index 0 (ie in your case it is equivalent); this
> > > leaves us some leeway as to choose which ITS the device will be actually
> > > mapped to and this code does not have to care.
> >
> > Ok. That make sense. Just a quick one, is it ok to add another helper
> function in
> > iort code to retrieve the its->its_count then?
> 
> While at it, given that the pci API code to retrieve domain and rid falls
> back to IORT anyway, I would add the whole reservation to IORT (mind,
> it depends on IOMMU_API) as one function instead of fiddling about with
> indexes.

Ok. I will take a look at this. 

> Side note: why Hilisicon dts upstream (eg hip07.dtsi) report ITS size
> as 256K ? I was just checking whether the ITS reg map size is system
> dependent and I bumped into them, I suspect there may be some dts
> patching needed here.
> 

Thanks for catching this. I will check with the team and update.

Shameer