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

[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

[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
 

回复： 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]

[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

[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

[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

[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

[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

[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

[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

[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

[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