diff mbox series

[v4,1/2] acpi:iort: Add an IORT helper function to reserve HW ITS address regions for IOMMU drivers

Message ID 20170725111732.41792-2-shameerali.kolothum.thodi@huawei.com
State New
Headers show
Series iommu/smmu-v3: Workaround for hisilicon 161010801 erratum(reserve HW MSI) | expand

Commit Message

Shameerali Kolothum Thodi July 25, 2017, 11:17 a.m. UTC 
The helper function retrieves ITS address regions through IORT
device <-> ITS mappings and reserves it so that these regions
will not be translated by IOMMU and will be excluded from IOVA
allocations. IOMMU drivers can use this to implement their
.get_resv_regions callback.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

---
 drivers/acpi/arm64/iort.c        | 91 ++++++++++++++++++++++++++++++++++++++--
 drivers/irqchip/irq-gic-v3-its.c |  3 +-
 include/linux/acpi_iort.h        |  8 +++-
 3 files changed, 97 insertions(+), 5 deletions(-)

-- 
1.9.1


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

Comments

Lorenzo Pieralisi July 25, 2017, 5:11 p.m. UTC | #1 
On Tue, Jul 25, 2017 at 12:17:31PM +0100, Shameer Kolothum wrote:
> The helper function retrieves ITS address regions through IORT

> device <-> ITS mappings and reserves it so that these regions

> will not be translated by IOMMU and will be excluded from IOVA

> allocations. IOMMU drivers can use this to implement their

> .get_resv_regions callback.

> 

> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> ---

>  drivers/acpi/arm64/iort.c        | 91 ++++++++++++++++++++++++++++++++++++++--

>  drivers/irqchip/irq-gic-v3-its.c |  3 +-

>  include/linux/acpi_iort.h        |  8 +++-

>  3 files changed, 97 insertions(+), 5 deletions(-)

> 

> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c

> index a3215ee..e28f30c 100644

> --- a/drivers/acpi/arm64/iort.c

> +++ b/drivers/acpi/arm64/iort.c

> @@ -39,6 +39,7 @@

>  struct iort_its_msi_chip {

>  	struct list_head	list;

>  	struct fwnode_handle	*fw_node;

> +	phys_addr_t		base_addr;

>  	u32			translation_id;

>  };

>  

> @@ -136,14 +137,16 @@ typedef acpi_status (*iort_find_node_callback)

>  static DEFINE_SPINLOCK(iort_msi_chip_lock);

>  

>  /**

> - * iort_register_domain_token() - register domain token and related ITS ID

> - * to the list from where we can get it back later on.

> + * iort_register_domain_token() - register domain token along with related

> + * ITS ID and base address to the list from where we can get it back later on.

>   * @trans_id: ITS ID.

> + * @base: ITS base address.

>   * @fw_node: Domain token.

>   *

>   * Returns: 0 on success, -ENOMEM if no memory when allocating list element

>   */

> -int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)

> +int iort_register_domain_token(int trans_id, phys_addr_t base,

> +			       struct fwnode_handle *fw_node)

>  {

>  	struct iort_its_msi_chip *its_msi_chip;

>  

> @@ -153,6 +156,7 @@ int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)

>  

>  	its_msi_chip->fw_node = fw_node;

>  	its_msi_chip->translation_id = trans_id;

> +	its_msi_chip->base_addr = base;

>  

>  	spin_lock(&iort_msi_chip_lock);

>  	list_add(&its_msi_chip->list, &iort_msi_chip_list);

> @@ -481,6 +485,24 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)

>  	return -ENODEV;

>  }

>  

> +static int iort_find_its_base(u32 its_id, phys_addr_t *base)


You have to tag it as __maybe_unused for the !IOMMU_API case.

> +{

> +	struct iort_its_msi_chip *its_msi_chip;

> +	bool match = false;

> +

> +	spin_lock(&iort_msi_chip_lock);

> +	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {

> +		if (its_msi_chip->translation_id == its_id) {

> +			*base = its_msi_chip->base_addr;

> +			match = true;

> +			break;

> +		}

> +	}

> +	spin_unlock(&iort_msi_chip_lock);

> +

> +	return match ? 0 : -ENODEV;

> +}

> +

>  /**

>   * iort_dev_find_its_id() - Find the ITS identifier for a device

>   * @dev: The device.

> @@ -639,6 +661,67 @@ int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)

>  

>  	return err;

>  }

> +

> +/**

> + * iort_iommu_its_get_resv_regions - Reserved region driver helper

> + * @dev: Device from iommu_get_resv_regions()

> + * @list: Reserved region list from iommu_get_resv_regions()

> + *

> + * Returns: Number of reserved regions on success(0 if no associated ITS),

> + *          appropriate error value otherwise.

> + *

> + * IOMMU drivers can use this to implement their .get_resv_regions callback

> + * for reserving the HW ITS address regions.


Stale comment.

> + */

> +int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)

> +{

> +	int i;

> +	struct acpi_iort_its_group *its;

> +	struct acpi_iort_node *node, *its_node = NULL;

> +	int resv = 0;


Nit: int i, resv = 0;

I can make these changes but I suspect this series will go via IOMMU
tree, let me know how you want to handle it.

Lorenzo

> +	node = iort_find_dev_node(dev);

> +	if (!node)

> +		return -ENODEV;

> +

> +	if (dev_is_pci(dev)) {

> +		u32 rid;

> +

> +		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, &rid);

> +		its_node = iort_node_map_id(node, rid, NULL, IORT_MSI_TYPE);

> +	} else {

> +		for (i = 0; i < node->mapping_count; i++) {

> +			its_node = iort_node_map_platform_id(node, NULL,

> +							 IORT_MSI_TYPE, i);

> +			if (its_node)

> +				break;

> +		}

> +	}

> +

> +	if (!its_node)

> +		return 0;

> +

> +	/* Move to ITS specific data */

> +	its = (struct acpi_iort_its_group *)its_node->node_data;

> +

> +	for (i = 0; i < its->its_count; i++) {

> +		phys_addr_t base;

> +

> +		if (!iort_find_its_base(its->identifiers[i], &base)) {

> +			int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;

> +			struct iommu_resv_region *region;

> +

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

> +							 IOMMU_RESV_MSI);

> +			if (region) {

> +				list_add_tail(&region->list, head);

> +				resv++;

> +			}

> +		}

> +	}

> +

> +	return (resv == its->its_count) ? resv : -ENODEV;

> +}

>  #else

>  static inline

>  const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)

> @@ -646,6 +729,8 @@ const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)

>  static inline

>  int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)

>  { return 0; }

> +int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)

> +{ return -ENODEV; }

>  #endif

>  

>  static const struct iommu_ops *iort_iommu_xlate(struct device *dev,

> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c

> index 6893287..77322b3 100644

> --- a/drivers/irqchip/irq-gic-v3-its.c

> +++ b/drivers/irqchip/irq-gic-v3-its.c

> @@ -1928,7 +1928,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,

>  		return -ENOMEM;

>  	}

>  

> -	err = iort_register_domain_token(its_entry->translation_id, dom_handle);

> +	err = iort_register_domain_token(its_entry->translation_id, res.start,

> +					 dom_handle);

>  	if (err) {

>  		pr_err("ITS@%pa: Unable to register GICv3 ITS domain token (ITS ID %d) to IORT\n",

>  		       &res.start, its_entry->translation_id);

> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h

> index 8379d40..56bb6c7 100644

> --- a/include/linux/acpi_iort.h

> +++ b/include/linux/acpi_iort.h

> @@ -26,7 +26,8 @@

>  #define IORT_IRQ_MASK(irq)		(irq & 0xffffffffULL)

>  #define IORT_IRQ_TRIGGER_MASK(irq)	((irq >> 32) & 0xffffffffULL)

>  

> -int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);

> +int iort_register_domain_token(int trans_id, phys_addr_t base,

> +			       struct fwnode_handle *fw_node);

>  void iort_deregister_domain_token(int trans_id);

>  struct fwnode_handle *iort_find_domain_token(int trans_id);

>  #ifdef CONFIG_ACPI_IORT

> @@ -38,8 +39,10 @@

>  /* IOMMU interface */

>  void iort_set_dma_mask(struct device *dev);

>  const struct iommu_ops *iort_iommu_configure(struct device *dev);

> +int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head);

>  #else

>  static inline void acpi_iort_init(void) { }

> +static inline bool iort_node_match(u8 type) { return false; }

>  static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)

>  { return req_id; }

>  static inline struct irq_domain *iort_get_device_domain(struct device *dev,

> @@ -51,6 +54,9 @@ static inline void iort_set_dma_mask(struct device *dev) { }

>  static inline

>  const struct iommu_ops *iort_iommu_configure(struct device *dev)

>  { return NULL; }

> +static inline

> +int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)

> +{ return -ENODEV; }

>  #endif

>  

>  #endif /* __ACPI_IORT_H__ */

> -- 

> 1.9.1

> 

> 

> --

> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in

> the body of a message to majordomo@vger.kernel.org

> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robin Murphy July 25, 2017, 5:32 p.m. UTC | #2 
On 25/07/17 18:11, Lorenzo Pieralisi wrote:
> On Tue, Jul 25, 2017 at 12:17:31PM +0100, Shameer Kolothum wrote:

>> The helper function retrieves ITS address regions through IORT

>> device <-> ITS mappings and reserves it so that these regions

>> will not be translated by IOMMU and will be excluded from IOVA

>> allocations. IOMMU drivers can use this to implement their

>> .get_resv_regions callback.

>>

>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

>> ---

>>  drivers/acpi/arm64/iort.c        | 91 ++++++++++++++++++++++++++++++++++++++--

>>  drivers/irqchip/irq-gic-v3-its.c |  3 +-

>>  include/linux/acpi_iort.h        |  8 +++-

>>  3 files changed, 97 insertions(+), 5 deletions(-)

>>

>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c

>> index a3215ee..e28f30c 100644

>> --- a/drivers/acpi/arm64/iort.c

>> +++ b/drivers/acpi/arm64/iort.c

>> @@ -39,6 +39,7 @@

>>  struct iort_its_msi_chip {

>>  	struct list_head	list;

>>  	struct fwnode_handle	*fw_node;

>> +	phys_addr_t		base_addr;

>>  	u32			translation_id;

>>  };

>>  

>> @@ -136,14 +137,16 @@ typedef acpi_status (*iort_find_node_callback)

>>  static DEFINE_SPINLOCK(iort_msi_chip_lock);

>>  

>>  /**

>> - * iort_register_domain_token() - register domain token and related ITS ID

>> - * to the list from where we can get it back later on.

>> + * iort_register_domain_token() - register domain token along with related

>> + * ITS ID and base address to the list from where we can get it back later on.

>>   * @trans_id: ITS ID.

>> + * @base: ITS base address.

>>   * @fw_node: Domain token.

>>   *

>>   * Returns: 0 on success, -ENOMEM if no memory when allocating list element

>>   */

>> -int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)

>> +int iort_register_domain_token(int trans_id, phys_addr_t base,

>> +			       struct fwnode_handle *fw_node)

>>  {

>>  	struct iort_its_msi_chip *its_msi_chip;

>>  

>> @@ -153,6 +156,7 @@ int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)

>>  

>>  	its_msi_chip->fw_node = fw_node;

>>  	its_msi_chip->translation_id = trans_id;

>> +	its_msi_chip->base_addr = base;

>>  

>>  	spin_lock(&iort_msi_chip_lock);

>>  	list_add(&its_msi_chip->list, &iort_msi_chip_list);

>> @@ -481,6 +485,24 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)

>>  	return -ENODEV;

>>  }

>>  

>> +static int iort_find_its_base(u32 its_id, phys_addr_t *base)

> 

> You have to tag it as __maybe_unused for the !IOMMU_API case.

> 

>> +{

>> +	struct iort_its_msi_chip *its_msi_chip;

>> +	bool match = false;

>> +

>> +	spin_lock(&iort_msi_chip_lock);

>> +	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {

>> +		if (its_msi_chip->translation_id == its_id) {

>> +			*base = its_msi_chip->base_addr;

>> +			match = true;

>> +			break;

>> +		}

>> +	}

>> +	spin_unlock(&iort_msi_chip_lock);

>> +

>> +	return match ? 0 : -ENODEV;

>> +}

>> +

>>  /**

>>   * iort_dev_find_its_id() - Find the ITS identifier for a device

>>   * @dev: The device.

>> @@ -639,6 +661,67 @@ int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)

>>  

>>  	return err;

>>  }

>> +

>> +/**

>> + * iort_iommu_its_get_resv_regions - Reserved region driver helper

>> + * @dev: Device from iommu_get_resv_regions()

>> + * @list: Reserved region list from iommu_get_resv_regions()

>> + *

>> + * Returns: Number of reserved regions on success(0 if no associated ITS),

>> + *          appropriate error value otherwise.

>> + *

>> + * IOMMU drivers can use this to implement their .get_resv_regions callback

>> + * for reserving the HW ITS address regions.

> 

> Stale comment.

> 

>> + */

>> +int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)

>> +{

>> +	int i;

>> +	struct acpi_iort_its_group *its;

>> +	struct acpi_iort_node *node, *its_node = NULL;

>> +	int resv = 0;

> 

> Nit: int i, resv = 0;

> 

> I can make these changes but I suspect this series will go via IOMMU

> tree, let me know how you want to handle it.

> 

> Lorenzo

> 

>> +	node = iort_find_dev_node(dev);

>> +	if (!node)

>> +		return -ENODEV;

>> +


I'd suggest we also want a comment here to clarify that we're currently
assuming straightforward topologies where all mappings for a given root
complex/named component target the same ITS group. Otherwise we're going
to need somewhat more logic to iterate the its_node processing over
every mapping (or every alias in the PCI case), but avoid creating
duplicate entries.

Robin.

>> +	if (dev_is_pci(dev)) {

>> +		u32 rid;

>> +

>> +		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, &rid);

>> +		its_node = iort_node_map_id(node, rid, NULL, IORT_MSI_TYPE);

>> +	} else {

>> +		for (i = 0; i < node->mapping_count; i++) {

>> +			its_node = iort_node_map_platform_id(node, NULL,

>> +							 IORT_MSI_TYPE, i);

>> +			if (its_node)

>> +				break;

>> +		}

>> +	}

>> +

>> +	if (!its_node)

>> +		return 0;

>> +

>> +	/* Move to ITS specific data */

>> +	its = (struct acpi_iort_its_group *)its_node->node_data;

>> +

>> +	for (i = 0; i < its->its_count; i++) {

>> +		phys_addr_t base;

>> +

>> +		if (!iort_find_its_base(its->identifiers[i], &base)) {

>> +			int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;

>> +			struct iommu_resv_region *region;

>> +

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

>> +							 IOMMU_RESV_MSI);

>> +			if (region) {

>> +				list_add_tail(&region->list, head);

>> +				resv++;

>> +			}

>> +		}

>> +	}

>> +

>> +	return (resv == its->its_count) ? resv : -ENODEV;

>> +}

>>  #else

>>  static inline

>>  const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)

>> @@ -646,6 +729,8 @@ const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)

>>  static inline

>>  int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)

>>  { return 0; }

>> +int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)

>> +{ return -ENODEV; }

>>  #endif

>>  

>>  static const struct iommu_ops *iort_iommu_xlate(struct device *dev,

>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c

>> index 6893287..77322b3 100644

>> --- a/drivers/irqchip/irq-gic-v3-its.c

>> +++ b/drivers/irqchip/irq-gic-v3-its.c

>> @@ -1928,7 +1928,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,

>>  		return -ENOMEM;

>>  	}

>>  

>> -	err = iort_register_domain_token(its_entry->translation_id, dom_handle);

>> +	err = iort_register_domain_token(its_entry->translation_id, res.start,

>> +					 dom_handle);

>>  	if (err) {

>>  		pr_err("ITS@%pa: Unable to register GICv3 ITS domain token (ITS ID %d) to IORT\n",

>>  		       &res.start, its_entry->translation_id);

>> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h

>> index 8379d40..56bb6c7 100644

>> --- a/include/linux/acpi_iort.h

>> +++ b/include/linux/acpi_iort.h

>> @@ -26,7 +26,8 @@

>>  #define IORT_IRQ_MASK(irq)		(irq & 0xffffffffULL)

>>  #define IORT_IRQ_TRIGGER_MASK(irq)	((irq >> 32) & 0xffffffffULL)

>>  

>> -int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);

>> +int iort_register_domain_token(int trans_id, phys_addr_t base,

>> +			       struct fwnode_handle *fw_node);

>>  void iort_deregister_domain_token(int trans_id);

>>  struct fwnode_handle *iort_find_domain_token(int trans_id);

>>  #ifdef CONFIG_ACPI_IORT

>> @@ -38,8 +39,10 @@

>>  /* IOMMU interface */

>>  void iort_set_dma_mask(struct device *dev);

>>  const struct iommu_ops *iort_iommu_configure(struct device *dev);

>> +int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head);

>>  #else

>>  static inline void acpi_iort_init(void) { }

>> +static inline bool iort_node_match(u8 type) { return false; }

>>  static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)

>>  { return req_id; }

>>  static inline struct irq_domain *iort_get_device_domain(struct device *dev,

>> @@ -51,6 +54,9 @@ static inline void iort_set_dma_mask(struct device *dev) { }

>>  static inline

>>  const struct iommu_ops *iort_iommu_configure(struct device *dev)

>>  { return NULL; }

>> +static inline

>> +int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)

>> +{ return -ENODEV; }

>>  #endif

>>  

>>  #endif /* __ACPI_IORT_H__ */

>> -- 

>> 1.9.1

>>

>>

>> --

>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in

>> the body of a message to majordomo@vger.kernel.org

>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi July 26, 2017, 9:52 a.m. UTC | #3 
On Tue, Jul 25, 2017 at 06:32:40PM +0100, Robin Murphy wrote:
> On 25/07/17 18:11, Lorenzo Pieralisi wrote:

> > On Tue, Jul 25, 2017 at 12:17:31PM +0100, Shameer Kolothum wrote:

> >> The helper function retrieves ITS address regions through IORT

> >> device <-> ITS mappings and reserves it so that these regions

> >> will not be translated by IOMMU and will be excluded from IOVA

> >> allocations. IOMMU drivers can use this to implement their

> >> .get_resv_regions callback.

> >>

> >> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> >> ---

> >>  drivers/acpi/arm64/iort.c        | 91 ++++++++++++++++++++++++++++++++++++++--

> >>  drivers/irqchip/irq-gic-v3-its.c |  3 +-

> >>  include/linux/acpi_iort.h        |  8 +++-

> >>  3 files changed, 97 insertions(+), 5 deletions(-)

> >>

> >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c

> >> index a3215ee..e28f30c 100644

> >> --- a/drivers/acpi/arm64/iort.c

> >> +++ b/drivers/acpi/arm64/iort.c

> >> @@ -39,6 +39,7 @@

> >>  struct iort_its_msi_chip {

> >>  	struct list_head	list;

> >>  	struct fwnode_handle	*fw_node;

> >> +	phys_addr_t		base_addr;

> >>  	u32			translation_id;

> >>  };

> >>  

> >> @@ -136,14 +137,16 @@ typedef acpi_status (*iort_find_node_callback)

> >>  static DEFINE_SPINLOCK(iort_msi_chip_lock);

> >>  

> >>  /**

> >> - * iort_register_domain_token() - register domain token and related ITS ID

> >> - * to the list from where we can get it back later on.

> >> + * iort_register_domain_token() - register domain token along with related

> >> + * ITS ID and base address to the list from where we can get it back later on.

> >>   * @trans_id: ITS ID.

> >> + * @base: ITS base address.

> >>   * @fw_node: Domain token.

> >>   *

> >>   * Returns: 0 on success, -ENOMEM if no memory when allocating list element

> >>   */

> >> -int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)

> >> +int iort_register_domain_token(int trans_id, phys_addr_t base,

> >> +			       struct fwnode_handle *fw_node)

> >>  {

> >>  	struct iort_its_msi_chip *its_msi_chip;

> >>  

> >> @@ -153,6 +156,7 @@ int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)

> >>  

> >>  	its_msi_chip->fw_node = fw_node;

> >>  	its_msi_chip->translation_id = trans_id;

> >> +	its_msi_chip->base_addr = base;

> >>  

> >>  	spin_lock(&iort_msi_chip_lock);

> >>  	list_add(&its_msi_chip->list, &iort_msi_chip_list);

> >> @@ -481,6 +485,24 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)

> >>  	return -ENODEV;

> >>  }

> >>  

> >> +static int iort_find_its_base(u32 its_id, phys_addr_t *base)

> > 

> > You have to tag it as __maybe_unused for the !IOMMU_API case.

> > 

> >> +{

> >> +	struct iort_its_msi_chip *its_msi_chip;

> >> +	bool match = false;

> >> +

> >> +	spin_lock(&iort_msi_chip_lock);

> >> +	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {

> >> +		if (its_msi_chip->translation_id == its_id) {

> >> +			*base = its_msi_chip->base_addr;

> >> +			match = true;

> >> +			break;

> >> +		}

> >> +	}

> >> +	spin_unlock(&iort_msi_chip_lock);

> >> +

> >> +	return match ? 0 : -ENODEV;

> >> +}

> >> +

> >>  /**

> >>   * iort_dev_find_its_id() - Find the ITS identifier for a device

> >>   * @dev: The device.

> >> @@ -639,6 +661,67 @@ int iort_add_device_replay(const struct iommu_ops *ops, struct device *dev)

> >>  

> >>  	return err;

> >>  }

> >> +

> >> +/**

> >> + * iort_iommu_its_get_resv_regions - Reserved region driver helper

> >> + * @dev: Device from iommu_get_resv_regions()

> >> + * @list: Reserved region list from iommu_get_resv_regions()

> >> + *

> >> + * Returns: Number of reserved regions on success(0 if no associated ITS),

> >> + *          appropriate error value otherwise.

> >> + *

> >> + * IOMMU drivers can use this to implement their .get_resv_regions callback

> >> + * for reserving the HW ITS address regions.

> > 

> > Stale comment.

> > 

> >> + */

> >> +int iort_iommu_its_get_resv_regions(struct device *dev, struct list_head *head)

> >> +{

> >> +	int i;

> >> +	struct acpi_iort_its_group *its;

> >> +	struct acpi_iort_node *node, *its_node = NULL;

> >> +	int resv = 0;

> > 

> > Nit: int i, resv = 0;

> > 

> > I can make these changes but I suspect this series will go via IOMMU

> > tree, let me know how you want to handle it.

> > 

> > Lorenzo

> > 

> >> +	node = iort_find_dev_node(dev);

> >> +	if (!node)

> >> +		return -ENODEV;

> >> +

> 

> I'd suggest we also want a comment here to clarify that we're currently

> assuming straightforward topologies where all mappings for a given root

> complex/named component target the same ITS group. Otherwise we're going

> to need somewhat more logic to iterate the its_node processing over

> every mapping (or every alias in the PCI case), but avoid creating

> duplicate entries.


You have a point and we have time to update the code. Short of reserving
all ITS regions for every device that maps to one at least, we could (even
pre-compute instead of looking it up on the fly) create a list of ITS
identifiers a given IORT node may map to and use that to reserve the
regions.

Thoughts ?

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shameerali Kolothum Thodi July 27, 2017, 9:13 a.m. UTC | #4 
> -----Original Message-----

> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]

> Sent: Wednesday, July 26, 2017 10:52 AM

> To: Robin Murphy

> Cc: Shameerali Kolothum Thodi; marc.zyngier@arm.com;

> sudeep.holla@arm.com; will.deacon@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: [PATCH v4 1/2] acpi:iort: Add an IORT helper function to reserve

> HW ITS address regions for IOMMU drivers

> 

> On Tue, Jul 25, 2017 at 06:32:40PM +0100, Robin Murphy wrote:

> > On 25/07/17 18:11, Lorenzo Pieralisi wrote:

> > > On Tue, Jul 25, 2017 at 12:17:31PM +0100, Shameer Kolothum wrote:

> > >> The helper function retrieves ITS address regions through IORT

> > >> device <-> ITS mappings and reserves it so that these regions

> > >> will not be translated by IOMMU and will be excluded from IOVA

> > >> allocations. IOMMU drivers can use this to implement their

> > >> .get_resv_regions callback.

> > >>

> > >> Signed-off-by: Shameer Kolothum

> <shameerali.kolothum.thodi@huawei.com>

> > >> ---

> > >>  drivers/acpi/arm64/iort.c        | 91

> ++++++++++++++++++++++++++++++++++++++--

> > >>  drivers/irqchip/irq-gic-v3-its.c |  3 +-

> > >>  include/linux/acpi_iort.h        |  8 +++-

> > >>  3 files changed, 97 insertions(+), 5 deletions(-)

> > >>

> > >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c

> > >> index a3215ee..e28f30c 100644

> > >> --- a/drivers/acpi/arm64/iort.c

> > >> +++ b/drivers/acpi/arm64/iort.c

> > >> @@ -39,6 +39,7 @@

> > >>  struct iort_its_msi_chip {

> > >>  	struct list_head	list;

> > >>  	struct fwnode_handle	*fw_node;

> > >> +	phys_addr_t		base_addr;

> > >>  	u32			translation_id;

> > >>  };

> > >>

> > >> @@ -136,14 +137,16 @@ typedef acpi_status

> (*iort_find_node_callback)

> > >>  static DEFINE_SPINLOCK(iort_msi_chip_lock);

> > >>

> > >>  /**

> > >> - * iort_register_domain_token() - register domain token and related

> ITS ID

> > >> - * to the list from where we can get it back later on.

> > >> + * iort_register_domain_token() - register domain token along with

> related

> > >> + * ITS ID and base address to the list from where we can get it back

> later on.

> > >>   * @trans_id: ITS ID.

> > >> + * @base: ITS base address.

> > >>   * @fw_node: Domain token.

> > >>   *

> > >>   * Returns: 0 on success, -ENOMEM if no memory when allocating list

> element

> > >>   */

> > >> -int iort_register_domain_token(int trans_id, struct fwnode_handle

> *fw_node)

> > >> +int iort_register_domain_token(int trans_id, phys_addr_t base,

> > >> +			       struct fwnode_handle *fw_node)

> > >>  {

> > >>  	struct iort_its_msi_chip *its_msi_chip;

> > >>

> > >> @@ -153,6 +156,7 @@ int iort_register_domain_token(int trans_id,

> struct fwnode_handle *fw_node)

> > >>

> > >>  	its_msi_chip->fw_node = fw_node;

> > >>  	its_msi_chip->translation_id = trans_id;

> > >> +	its_msi_chip->base_addr = base;

> > >>

> > >>  	spin_lock(&iort_msi_chip_lock);

> > >>  	list_add(&its_msi_chip->list, &iort_msi_chip_list);

> > >> @@ -481,6 +485,24 @@ int iort_pmsi_get_dev_id(struct device *dev,

> u32 *dev_id)

> > >>  	return -ENODEV;

> > >>  }

> > >>

> > >> +static int iort_find_its_base(u32 its_id, phys_addr_t *base)

> > >

> > > You have to tag it as __maybe_unused for the !IOMMU_API case.

> > >

> > >> +{

> > >> +	struct iort_its_msi_chip *its_msi_chip;

> > >> +	bool match = false;

> > >> +

> > >> +	spin_lock(&iort_msi_chip_lock);

> > >> +	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {

> > >> +		if (its_msi_chip->translation_id == its_id) {

> > >> +			*base = its_msi_chip->base_addr;

> > >> +			match = true;

> > >> +			break;

> > >> +		}

> > >> +	}

> > >> +	spin_unlock(&iort_msi_chip_lock);

> > >> +

> > >> +	return match ? 0 : -ENODEV;

> > >> +}

> > >> +

> > >>  /**

> > >>   * iort_dev_find_its_id() - Find the ITS identifier for a device

> > >>   * @dev: The device.

> > >> @@ -639,6 +661,67 @@ int iort_add_device_replay(const struct

> iommu_ops *ops, struct device *dev)

> > >>

> > >>  	return err;

> > >>  }

> > >> +

> > >> +/**

> > >> + * iort_iommu_its_get_resv_regions - Reserved region driver helper

> > >> + * @dev: Device from iommu_get_resv_regions()

> > >> + * @list: Reserved region list from iommu_get_resv_regions()

> > >> + *

> > >> + * Returns: Number of reserved regions on success(0 if no associated

> ITS),

> > >> + *          appropriate error value otherwise.

> > >> + *

> > >> + * IOMMU drivers can use this to implement their .get_resv_regions

> callback

> > >> + * for reserving the HW ITS address regions.

> > >

> > > Stale comment.

> > >

> > >> + */

> > >> +int iort_iommu_its_get_resv_regions(struct device *dev, struct

> list_head *head)

> > >> +{

> > >> +	int i;

> > >> +	struct acpi_iort_its_group *its;

> > >> +	struct acpi_iort_node *node, *its_node = NULL;

> > >> +	int resv = 0;

> > >

> > > Nit: int i, resv = 0;

> > >

> > > I can make these changes but I suspect this series will go via IOMMU

> > > tree, let me know how you want to handle it.

> > >

> > > Lorenzo

> > >

> > >> +	node = iort_find_dev_node(dev);

> > >> +	if (!node)

> > >> +		return -ENODEV;

> > >> +

> >

> > I'd suggest we also want a comment here to clarify that we're currently

> > assuming straightforward topologies where all mappings for a given root

> > complex/named component target the same ITS group. Otherwise we're

> going

> > to need somewhat more logic to iterate the its_node processing over

> > every mapping (or every alias in the PCI case), but avoid creating

> > duplicate entries.

> 

> You have a point and we have time to update the code. Short of reserving

> all ITS regions for every device that maps to one at least, we could (even

> pre-compute instead of looking it up on the fly) create a list of ITS

> identifiers a given IORT node may map to and use that to reserve the

> regions.


I am trying to understand the use case scenario discussed here. Apologies
if it is a dumb query. 

My understanding is that, it is possible to have a PCI  RC iort node mapped to
multiple ITS group nodes.  That is perfectly fine and given a dev input RID we 
can identify the ITS group the device points to using - iort_node_map_id().

But the above discussion seems to suggest that there might be situations where
we have to go through all the mapped ITS groups and identify all the ITSs associated
with the RC.  Clearly I am missing something.

Appreciate your help.

Thanks,
Shameer

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi July 27, 2017, 10:12 a.m. UTC | #5 
On Thu, Jul 27, 2017 at 09:13:42AM +0000, Shameerali Kolothum Thodi wrote:

[...]

> > > >> +int iort_iommu_its_get_resv_regions(struct device *dev, struct

> > list_head *head)

> > > >> +{

> > > >> +	int i;

> > > >> +	struct acpi_iort_its_group *its;

> > > >> +	struct acpi_iort_node *node, *its_node = NULL;

> > > >> +	int resv = 0;

> > > >

> > > > Nit: int i, resv = 0;

> > > >

> > > > I can make these changes but I suspect this series will go via IOMMU

> > > > tree, let me know how you want to handle it.

> > > >

> > > > Lorenzo

> > > >

> > > >> +	node = iort_find_dev_node(dev);

> > > >> +	if (!node)

> > > >> +		return -ENODEV;

> > > >> +

> > >

> > > I'd suggest we also want a comment here to clarify that we're currently

> > > assuming straightforward topologies where all mappings for a given root

> > > complex/named component target the same ITS group. Otherwise we're

> > going

> > > to need somewhat more logic to iterate the its_node processing over

> > > every mapping (or every alias in the PCI case), but avoid creating

> > > duplicate entries.

> > 

> > You have a point and we have time to update the code. Short of reserving

> > all ITS regions for every device that maps to one at least, we could (even

> > pre-compute instead of looking it up on the fly) create a list of ITS

> > identifiers a given IORT node may map to and use that to reserve the

> > regions.

> 

> I am trying to understand the use case scenario discussed here.

> Apologies if it is a dumb query. 

> 

> My understanding is that, it is possible to have a PCI  RC iort node

> mapped to multiple ITS group nodes.  That is perfectly fine and given

> a dev input RID we can identify the ITS group the device points to

> using - iort_node_map_id().

> 

> But the above discussion seems to suggest that there might be

> situations where we have to go through all the mapped ITS groups and

> identify all the ITSs associated with the RC.  Clearly I am missing

> something.


I reckon Robin was referring to this:

https://patchwork.kernel.org/patch/9757911/

Does this help ?

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robin Murphy July 27, 2017, 11:13 a.m. UTC | #6 
On 27/07/17 10:13, Shameerali Kolothum Thodi wrote:
> 

> 

>> -----Original Message-----

>> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]

>> Sent: Wednesday, July 26, 2017 10:52 AM

>> To: Robin Murphy

>> Cc: Shameerali Kolothum Thodi; marc.zyngier@arm.com;

>> sudeep.holla@arm.com; will.deacon@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: [PATCH v4 1/2] acpi:iort: Add an IORT helper function to reserve

>> HW ITS address regions for IOMMU drivers

>>

>> On Tue, Jul 25, 2017 at 06:32:40PM +0100, Robin Murphy wrote:

>>> On 25/07/17 18:11, Lorenzo Pieralisi wrote:

>>>> On Tue, Jul 25, 2017 at 12:17:31PM +0100, Shameer Kolothum wrote:

>>>>> The helper function retrieves ITS address regions through IORT

>>>>> device <-> ITS mappings and reserves it so that these regions

>>>>> will not be translated by IOMMU and will be excluded from IOVA

>>>>> allocations. IOMMU drivers can use this to implement their

>>>>> .get_resv_regions callback.

>>>>>

>>>>> Signed-off-by: Shameer Kolothum

>> <shameerali.kolothum.thodi@huawei.com>

>>>>> ---

>>>>>  drivers/acpi/arm64/iort.c        | 91

>> ++++++++++++++++++++++++++++++++++++++--

>>>>>  drivers/irqchip/irq-gic-v3-its.c |  3 +-

>>>>>  include/linux/acpi_iort.h        |  8 +++-

>>>>>  3 files changed, 97 insertions(+), 5 deletions(-)

>>>>>

>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c

>>>>> index a3215ee..e28f30c 100644

>>>>> --- a/drivers/acpi/arm64/iort.c

>>>>> +++ b/drivers/acpi/arm64/iort.c

>>>>> @@ -39,6 +39,7 @@

>>>>>  struct iort_its_msi_chip {

>>>>>  	struct list_head	list;

>>>>>  	struct fwnode_handle	*fw_node;

>>>>> +	phys_addr_t		base_addr;

>>>>>  	u32			translation_id;

>>>>>  };

>>>>>

>>>>> @@ -136,14 +137,16 @@ typedef acpi_status

>> (*iort_find_node_callback)

>>>>>  static DEFINE_SPINLOCK(iort_msi_chip_lock);

>>>>>

>>>>>  /**

>>>>> - * iort_register_domain_token() - register domain token and related

>> ITS ID

>>>>> - * to the list from where we can get it back later on.

>>>>> + * iort_register_domain_token() - register domain token along with

>> related

>>>>> + * ITS ID and base address to the list from where we can get it back

>> later on.

>>>>>   * @trans_id: ITS ID.

>>>>> + * @base: ITS base address.

>>>>>   * @fw_node: Domain token.

>>>>>   *

>>>>>   * Returns: 0 on success, -ENOMEM if no memory when allocating list

>> element

>>>>>   */

>>>>> -int iort_register_domain_token(int trans_id, struct fwnode_handle

>> *fw_node)

>>>>> +int iort_register_domain_token(int trans_id, phys_addr_t base,

>>>>> +			       struct fwnode_handle *fw_node)

>>>>>  {

>>>>>  	struct iort_its_msi_chip *its_msi_chip;

>>>>>

>>>>> @@ -153,6 +156,7 @@ int iort_register_domain_token(int trans_id,

>> struct fwnode_handle *fw_node)

>>>>>

>>>>>  	its_msi_chip->fw_node = fw_node;

>>>>>  	its_msi_chip->translation_id = trans_id;

>>>>> +	its_msi_chip->base_addr = base;

>>>>>

>>>>>  	spin_lock(&iort_msi_chip_lock);

>>>>>  	list_add(&its_msi_chip->list, &iort_msi_chip_list);

>>>>> @@ -481,6 +485,24 @@ int iort_pmsi_get_dev_id(struct device *dev,

>> u32 *dev_id)

>>>>>  	return -ENODEV;

>>>>>  }

>>>>>

>>>>> +static int iort_find_its_base(u32 its_id, phys_addr_t *base)

>>>>

>>>> You have to tag it as __maybe_unused for the !IOMMU_API case.

>>>>

>>>>> +{

>>>>> +	struct iort_its_msi_chip *its_msi_chip;

>>>>> +	bool match = false;

>>>>> +

>>>>> +	spin_lock(&iort_msi_chip_lock);

>>>>> +	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {

>>>>> +		if (its_msi_chip->translation_id == its_id) {

>>>>> +			*base = its_msi_chip->base_addr;

>>>>> +			match = true;

>>>>> +			break;

>>>>> +		}

>>>>> +	}

>>>>> +	spin_unlock(&iort_msi_chip_lock);

>>>>> +

>>>>> +	return match ? 0 : -ENODEV;

>>>>> +}

>>>>> +

>>>>>  /**

>>>>>   * iort_dev_find_its_id() - Find the ITS identifier for a device

>>>>>   * @dev: The device.

>>>>> @@ -639,6 +661,67 @@ int iort_add_device_replay(const struct

>> iommu_ops *ops, struct device *dev)

>>>>>

>>>>>  	return err;

>>>>>  }

>>>>> +

>>>>> +/**

>>>>> + * iort_iommu_its_get_resv_regions - Reserved region driver helper

>>>>> + * @dev: Device from iommu_get_resv_regions()

>>>>> + * @list: Reserved region list from iommu_get_resv_regions()

>>>>> + *

>>>>> + * Returns: Number of reserved regions on success(0 if no associated

>> ITS),

>>>>> + *          appropriate error value otherwise.

>>>>> + *

>>>>> + * IOMMU drivers can use this to implement their .get_resv_regions

>> callback

>>>>> + * for reserving the HW ITS address regions.

>>>>

>>>> Stale comment.

>>>>

>>>>> + */

>>>>> +int iort_iommu_its_get_resv_regions(struct device *dev, struct

>> list_head *head)

>>>>> +{

>>>>> +	int i;

>>>>> +	struct acpi_iort_its_group *its;

>>>>> +	struct acpi_iort_node *node, *its_node = NULL;

>>>>> +	int resv = 0;

>>>>

>>>> Nit: int i, resv = 0;

>>>>

>>>> I can make these changes but I suspect this series will go via IOMMU

>>>> tree, let me know how you want to handle it.

>>>>

>>>> Lorenzo

>>>>

>>>>> +	node = iort_find_dev_node(dev);

>>>>> +	if (!node)

>>>>> +		return -ENODEV;

>>>>> +

>>>

>>> I'd suggest we also want a comment here to clarify that we're currently

>>> assuming straightforward topologies where all mappings for a given root

>>> complex/named component target the same ITS group. Otherwise we're

>> going

>>> to need somewhat more logic to iterate the its_node processing over

>>> every mapping (or every alias in the PCI case), but avoid creating

>>> duplicate entries.

>>

>> You have a point and we have time to update the code. Short of reserving

>> all ITS regions for every device that maps to one at least, we could (even

>> pre-compute instead of looking it up on the fly) create a list of ITS

>> identifiers a given IORT node may map to and use that to reserve the

>> regions.

> 

> I am trying to understand the use case scenario discussed here. Apologies

> if it is a dumb query. 

> 

> My understanding is that, it is possible to have a PCI  RC iort node mapped to

> multiple ITS group nodes.  That is perfectly fine and given a dev input RID we 

> can identify the ITS group the device points to using - iort_node_map_id().

> 

> But the above discussion seems to suggest that there might be situations where

> we have to go through all the mapped ITS groups and identify all the ITSs associated

> with the RC.  Clearly I am missing something.


I was mostly thinking of a situation like this:

+----Node 0-----+  +----Node 1-----+
|  [CPU 0..n]   |  |  [CPU n+1..]  |
| [ITS group 0] |  | [ITS group 1] |
+---------------+  +---------------+
        ^                  ^
         \_______  _______/
                 \/
            +--Node 2--+
            |  [SMMU]  |
            |     ^    |
            |     |    |
            | [Device] |
            +----------+

where the (named component) device has IDs for both ITS groups (to help
optimise affining, or allow physically hotplugging CPU nodes, or
whatever - I'm hypothesising here ;)).  A generic IORT function isn't in
a position to decide *which* ITS region the device may be targeting at
any given time, so can only correctly describe both.

I'm perfectly happy not to even try to support such crazy configurations
until they actually exist, if ever; I'd just prefer to document whatever
assumptions we do make, so that we don't have to remember or re-derive
them when looking at the code in future.

Robin.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shameerali Kolothum Thodi July 27, 2017, 1:26 p.m. UTC | #7 
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogUm9iaW4gTXVycGh5IFtt
YWlsdG86cm9iaW4ubXVycGh5QGFybS5jb21dDQo+IFNlbnQ6IFRodXJzZGF5LCBKdWx5IDI3LCAy
MDE3IDEyOjEzIFBNDQo+IFRvOiBTaGFtZWVyYWxpIEtvbG90aHVtIFRob2RpOyBMb3JlbnpvIFBp
ZXJhbGlzaQ0KPiBDYzogR3VvaGFuanVuIChIYW5qdW4gR3VvKTsgR2FicmllbGUgUGFvbG9uaTsg
bWFyYy56eW5naWVyQGFybS5jb207DQo+IEpvaG4gR2Fycnk7IHdpbGwuZGVhY29uQGFybS5jb207
IExpbnV4YXJtOyBsaW51eC1hY3BpQHZnZXIua2VybmVsLm9yZzsNCj4gaW9tbXVAbGlzdHMubGlu
dXgtZm91bmRhdGlvbi5vcmc7IGhhbmp1bi5ndW9AbGluYXJvLm9yZzsgV2FuZ3pob3UgKEIpOw0K
PiBzdWRlZXAuaG9sbGFAYXJtLmNvbTsgbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQu
b3JnOw0KPiBkZXZlbEBhY3BpY2Eub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjQgMS8yXSBh
Y3BpOmlvcnQ6IEFkZCBhbiBJT1JUIGhlbHBlciBmdW5jdGlvbiB0byByZXNlcnZlDQo+IEhXIElU
UyBhZGRyZXNzIHJlZ2lvbnMgZm9yIElPTU1VIGRyaXZlcnMNCj4gDQpbLi4uXQ0KDQo+ID4+Pj4g
SSBjYW4gbWFrZSB0aGVzZSBjaGFuZ2VzIGJ1dCBJIHN1c3BlY3QgdGhpcyBzZXJpZXMgd2lsbCBn
byB2aWEgSU9NTVUNCj4gPj4+PiB0cmVlLCBsZXQgbWUga25vdyBob3cgeW91IHdhbnQgdG8gaGFu
ZGxlIGl0Lg0KPiA+Pj4+DQo+ID4+Pj4gTG9yZW56bw0KPiA+Pj4+DQo+ID4+Pj4+ICsJbm9kZSA9
IGlvcnRfZmluZF9kZXZfbm9kZShkZXYpOw0KPiA+Pj4+PiArCWlmICghbm9kZSkNCj4gPj4+Pj4g
KwkJcmV0dXJuIC1FTk9ERVY7DQo+ID4+Pj4+ICsNCj4gPj4+DQo+ID4+PiBJJ2Qgc3VnZ2VzdCB3
ZSBhbHNvIHdhbnQgYSBjb21tZW50IGhlcmUgdG8gY2xhcmlmeSB0aGF0IHdlJ3JlIGN1cnJlbnRs
eQ0KPiA+Pj4gYXNzdW1pbmcgc3RyYWlnaHRmb3J3YXJkIHRvcG9sb2dpZXMgd2hlcmUgYWxsIG1h
cHBpbmdzIGZvciBhIGdpdmVuIHJvb3QNCj4gPj4+IGNvbXBsZXgvbmFtZWQgY29tcG9uZW50IHRh
cmdldCB0aGUgc2FtZSBJVFMgZ3JvdXAuIE90aGVyd2lzZQ0KPiB3ZSdyZQ0KPiA+PiBnb2luZw0K
PiA+Pj4gdG8gbmVlZCBzb21ld2hhdCBtb3JlIGxvZ2ljIHRvIGl0ZXJhdGUgdGhlIGl0c19ub2Rl
IHByb2Nlc3Npbmcgb3Zlcg0KPiA+Pj4gZXZlcnkgbWFwcGluZyAob3IgZXZlcnkgYWxpYXMgaW4g
dGhlIFBDSSBjYXNlKSwgYnV0IGF2b2lkIGNyZWF0aW5nDQo+ID4+PiBkdXBsaWNhdGUgZW50cmll
cy4NCj4gPj4NCj4gPj4gWW91IGhhdmUgYSBwb2ludCBhbmQgd2UgaGF2ZSB0aW1lIHRvIHVwZGF0
ZSB0aGUgY29kZS4gU2hvcnQgb2YgcmVzZXJ2aW5nDQo+ID4+IGFsbCBJVFMgcmVnaW9ucyBmb3Ig
ZXZlcnkgZGV2aWNlIHRoYXQgbWFwcyB0byBvbmUgYXQgbGVhc3QsIHdlIGNvdWxkIChldmVuDQo+
ID4+IHByZS1jb21wdXRlIGluc3RlYWQgb2YgbG9va2luZyBpdCB1cCBvbiB0aGUgZmx5KSBjcmVh
dGUgYSBsaXN0IG9mIElUUw0KPiA+PiBpZGVudGlmaWVycyBhIGdpdmVuIElPUlQgbm9kZSBtYXkg
bWFwIHRvIGFuZCB1c2UgdGhhdCB0byByZXNlcnZlIHRoZQ0KPiA+PiByZWdpb25zLg0KPiA+DQo+
ID4gSSBhbSB0cnlpbmcgdG8gdW5kZXJzdGFuZCB0aGUgdXNlIGNhc2Ugc2NlbmFyaW8gZGlzY3Vz
c2VkIGhlcmUuIEFwb2xvZ2llcw0KPiA+IGlmIGl0IGlzIGEgZHVtYiBxdWVyeS4NCj4gPg0KPiA+
IE15IHVuZGVyc3RhbmRpbmcgaXMgdGhhdCwgaXQgaXMgcG9zc2libGUgdG8gaGF2ZSBhIFBDSSAg
UkMgaW9ydCBub2RlIG1hcHBlZA0KPiB0bw0KPiA+IG11bHRpcGxlIElUUyBncm91cCBub2Rlcy4g
IFRoYXQgaXMgcGVyZmVjdGx5IGZpbmUgYW5kIGdpdmVuIGEgZGV2IGlucHV0IFJJRA0KPiB3ZQ0K
PiA+IGNhbiBpZGVudGlmeSB0aGUgSVRTIGdyb3VwIHRoZSBkZXZpY2UgcG9pbnRzIHRvIHVzaW5n
IC0gaW9ydF9ub2RlX21hcF9pZCgpLg0KPiA+DQo+ID4gQnV0IHRoZSBhYm92ZSBkaXNjdXNzaW9u
IHNlZW1zIHRvIHN1Z2dlc3QgdGhhdCB0aGVyZSBtaWdodCBiZSBzaXR1YXRpb25zDQo+IHdoZXJl
DQo+ID4gd2UgaGF2ZSB0byBnbyB0aHJvdWdoIGFsbCB0aGUgbWFwcGVkIElUUyBncm91cHMgYW5k
IGlkZW50aWZ5IGFsbCB0aGUgSVRTcw0KPiBhc3NvY2lhdGVkDQo+ID4gd2l0aCB0aGUgUkMuICBD
bGVhcmx5IEkgYW0gbWlzc2luZyBzb21ldGhpbmcuDQo+IA0KPiBJIHdhcyBtb3N0bHkgdGhpbmtp
bmcgb2YgYSBzaXR1YXRpb24gbGlrZSB0aGlzOg0KPiANCj4gKy0tLS1Ob2RlIDAtLS0tLSsgICst
LS0tTm9kZSAxLS0tLS0rDQo+IHwgIFtDUFUgMC4ubl0gICB8ICB8ICBbQ1BVIG4rMS4uXSAgfA0K
PiB8IFtJVFMgZ3JvdXAgMF0gfCAgfCBbSVRTIGdyb3VwIDFdIHwNCj4gKy0tLS0tLS0tLS0tLS0t
LSsgICstLS0tLS0tLS0tLS0tLS0rDQo+ICAgICAgICAgXiAgICAgICAgICAgICAgICAgIF4NCj4g
ICAgICAgICAgXF9fX19fX18gIF9fX19fX18vDQo+ICAgICAgICAgICAgICAgICAgXC8NCj4gICAg
ICAgICAgICAgKy0tTm9kZSAyLS0rDQo+ICAgICAgICAgICAgIHwgIFtTTU1VXSAgfA0KPiAgICAg
ICAgICAgICB8ICAgICBeICAgIHwNCj4gICAgICAgICAgICAgfCAgICAgfCAgICB8DQo+ICAgICAg
ICAgICAgIHwgW0RldmljZV0gfA0KPiAgICAgICAgICAgICArLS0tLS0tLS0tLSsNCj4gDQo+IHdo
ZXJlIHRoZSAobmFtZWQgY29tcG9uZW50KSBkZXZpY2UgaGFzIElEcyBmb3IgYm90aCBJVFMgZ3Jv
dXBzICh0byBoZWxwDQo+IG9wdGltaXNlIGFmZmluaW5nLCBvciBhbGxvdyBwaHlzaWNhbGx5IGhv
dHBsdWdnaW5nIENQVSBub2Rlcywgb3INCj4gd2hhdGV2ZXIgLSBJJ20gaHlwb3RoZXNpc2luZyBo
ZXJlIDspKS4gIEEgZ2VuZXJpYyBJT1JUIGZ1bmN0aW9uIGlzbid0IGluDQo+IGEgcG9zaXRpb24g
dG8gZGVjaWRlICp3aGljaCogSVRTIHJlZ2lvbiB0aGUgZGV2aWNlIG1heSBiZSB0YXJnZXRpbmcg
YXQNCj4gYW55IGdpdmVuIHRpbWUsIHNvIGNhbiBvbmx5IGNvcnJlY3RseSBkZXNjcmliZSBib3Ro
Lg0KDQpUaGFua3MgUm9iaW4uIFRoYXQgbWFrZXMgaXQgY2xlYXIuDQogDQo+IEknbSBwZXJmZWN0
bHkgaGFwcHkgbm90IHRvIGV2ZW4gdHJ5IHRvIHN1cHBvcnQgc3VjaCBjcmF6eSBjb25maWd1cmF0
aW9ucw0KPiB1bnRpbCB0aGV5IGFjdHVhbGx5IGV4aXN0LCBpZiBldmVyOyBJJ2QganVzdCBwcmVm
ZXIgdG8gZG9jdW1lbnQgd2hhdGV2ZXINCj4gYXNzdW1wdGlvbnMgd2UgZG8gbWFrZSwgc28gdGhh
dCB3ZSBkb24ndCBoYXZlIHRvIHJlbWVtYmVyIG9yIHJlLWRlcml2ZQ0KPiB0aGVtIHdoZW4gbG9v
a2luZyBhdCB0aGUgY29kZSBpbiBmdXR1cmUuDQoNClNvIEkgdGhpbmsgdGhlIGNvbmNsdXNpb24g
aGVyZSBpcyB3ZSB3aWxsIGRvY3VtZW50IHRoZSBhc3N1bXB0aW9uIHRoYXQgd2UgYXJlDQpvbmx5
IHRha2luZyBjYXJlIG9mIHRoZSBzdHJhaWdodGZvcndhcmQgdG9wb2xvZ2llcyBmb3Igbm93Lg0K
DQpIaSBMb3JlbnpvLA0KSWYgeW91IGFyZSBvayB3aXRoIHRoZSBhYm92ZSwgcGxlYXNlIGxldCBt
ZSBrbm93IGlmIGl0IG1ha2Ugc2Vuc2UgdG8gc2VuZCBvdXQNCmEgdjUgd2l0aCB0aGlzIGFuZCB5
b3VyIG90aGVyIGNvbW1lbnRzIG9yIHlvdSBjYW4gdGFrZSBjYXJlIG9mIHRoZW0uIEkgYW0gZmlu
ZQ0KZWl0aGVyIHdheS4NCg0KTWFueSB0aGFua3MsDQpTaGFtZWVyDQoNCg0KDQoNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi July 28, 2017, 9:57 a.m. UTC | #8 
On Thu, Jul 27, 2017 at 01:26:14PM +0000, Shameerali Kolothum Thodi wrote:
> 

> 

> > -----Original Message-----

> > From: Robin Murphy [mailto:robin.murphy@arm.com]

> > Sent: Thursday, July 27, 2017 12:13 PM

> > To: Shameerali Kolothum Thodi; Lorenzo Pieralisi

> > Cc: Guohanjun (Hanjun Guo); Gabriele Paoloni; marc.zyngier@arm.com;

> > John Garry; will.deacon@arm.com; Linuxarm; linux-acpi@vger.kernel.org;

> > iommu@lists.linux-foundation.org; hanjun.guo@linaro.org; Wangzhou (B);

> > sudeep.holla@arm.com; linux-arm-kernel@lists.infradead.org;

> > devel@acpica.org

> > Subject: Re: [PATCH v4 1/2] acpi:iort: Add an IORT helper function to reserve

> > HW ITS address regions for IOMMU drivers

> > 

> [...]

> 

> > >>>> I can make these changes but I suspect this series will go via IOMMU

> > >>>> tree, let me know how you want to handle it.

> > >>>>

> > >>>> Lorenzo

> > >>>>

> > >>>>> +	node = iort_find_dev_node(dev);

> > >>>>> +	if (!node)

> > >>>>> +		return -ENODEV;

> > >>>>> +

> > >>>

> > >>> I'd suggest we also want a comment here to clarify that we're currently

> > >>> assuming straightforward topologies where all mappings for a given root

> > >>> complex/named component target the same ITS group. Otherwise

> > we're

> > >> going

> > >>> to need somewhat more logic to iterate the its_node processing over

> > >>> every mapping (or every alias in the PCI case), but avoid creating

> > >>> duplicate entries.

> > >>

> > >> You have a point and we have time to update the code. Short of reserving

> > >> all ITS regions for every device that maps to one at least, we could (even

> > >> pre-compute instead of looking it up on the fly) create a list of ITS

> > >> identifiers a given IORT node may map to and use that to reserve the

> > >> regions.

> > >

> > > I am trying to understand the use case scenario discussed here. Apologies

> > > if it is a dumb query.

> > >

> > > My understanding is that, it is possible to have a PCI  RC iort node mapped

> > to

> > > multiple ITS group nodes.  That is perfectly fine and given a dev input RID

> > we

> > > can identify the ITS group the device points to using - iort_node_map_id().

> > >

> > > But the above discussion seems to suggest that there might be situations

> > where

> > > we have to go through all the mapped ITS groups and identify all the ITSs

> > associated

> > > with the RC.  Clearly I am missing something.

> > 

> > I was mostly thinking of a situation like this:

> > 

> > +----Node 0-----+  +----Node 1-----+

> > |  [CPU 0..n]   |  |  [CPU n+1..]  |

> > | [ITS group 0] |  | [ITS group 1] |

> > +---------------+  +---------------+

> >         ^                  ^

> >          \_______  _______/

> >                  \/

> >             +--Node 2--+

> >             |  [SMMU]  |

> >             |     ^    |

> >             |     |    |

> >             | [Device] |

> >             +----------+

> > 

> > where the (named component) device has IDs for both ITS groups (to help

> > optimise affining, or allow physically hotplugging CPU nodes, or

> > whatever - I'm hypothesising here ;)).  A generic IORT function isn't in

> > a position to decide *which* ITS region the device may be targeting at

> > any given time, so can only correctly describe both.

> 

> Thanks Robin. That makes it clear.

>  

> > I'm perfectly happy not to even try to support such crazy configurations

> > until they actually exist, if ever; I'd just prefer to document whatever

> > assumptions we do make, so that we don't have to remember or re-derive

> > them when looking at the code in future.

> 

> So I think the conclusion here is we will document the assumption that we are

> only taking care of the straightforward topologies for now.

> 

> Hi Lorenzo,

> If you are ok with the above, please let me know if it make sense to send out

> a v5 with this and your other comments or you can take care of them. I am fine

> either way.


I added below what should be the final patch - please have a look test and
post it as part of v5 that should hopefully be final.

Heads-up: I noticed this contains irqchip changes too so care must be
taken for cross-tree dependencies.

-- >8 --
Subject: [PATCH] ACPI/IORT: Add ITS address regions reservation helper

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

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

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

[lorenzo.pieralisi@arm.com: updated commit log/added comments]
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

---
 drivers/acpi/arm64/iort.c        | 95 ++++++++++++++++++++++++++++++++++++++--
 drivers/irqchip/irq-gic-v3-its.c |  3 +-
 include/linux/acpi_iort.h        |  8 +++-
 3 files changed, 101 insertions(+), 5 deletions(-)

-- 
2.10.0

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

> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]

> Sent: Friday, July 28, 2017 10:58 AM

> To: Shameerali Kolothum Thodi

> Cc: Robin Murphy; Guohanjun (Hanjun Guo); Gabriele Paoloni;

> marc.zyngier@arm.com; John Garry; will.deacon@arm.com; Linuxarm; linux-

> acpi@vger.kernel.org; iommu@lists.linux-foundation.org;

> hanjun.guo@linaro.org; Wangzhou (B); sudeep.holla@arm.com; linux-arm-

> kernel@lists.infradead.org; devel@acpica.org

> Subject: Re: [PATCH v4 1/2] acpi:iort: Add an IORT helper function to reserve

> HW ITS address regions for IOMMU drivers

> 

> On Thu, Jul 27, 2017 at 01:26:14PM +0000, Shameerali Kolothum Thodi wrote:

> >

> >

> > > -----Original Message-----

> > > From: Robin Murphy [mailto:robin.murphy@arm.com]

> > > Sent: Thursday, July 27, 2017 12:13 PM

> > > To: Shameerali Kolothum Thodi; Lorenzo Pieralisi

> > > Cc: Guohanjun (Hanjun Guo); Gabriele Paoloni; marc.zyngier@arm.com;

> > > John Garry; will.deacon@arm.com; Linuxarm;

> > > linux-acpi@vger.kernel.org; iommu@lists.linux-foundation.org;

> > > hanjun.guo@linaro.org; Wangzhou (B); sudeep.holla@arm.com;

> > > linux-arm-kernel@lists.infradead.org;

> > > devel@acpica.org

> > > Subject: Re: [PATCH v4 1/2] acpi:iort: Add an IORT helper function

> > > to reserve HW ITS address regions for IOMMU drivers

> > >

> > [...]

> >

> > > >>>> I can make these changes but I suspect this series will go via

> > > >>>> IOMMU tree, let me know how you want to handle it.

> > > >>>>

> > > >>>> Lorenzo

> > > >>>>

> > > >>>>> +	node = iort_find_dev_node(dev);

> > > >>>>> +	if (!node)

> > > >>>>> +		return -ENODEV;

> > > >>>>> +

> > > >>>

> > > >>> I'd suggest we also want a comment here to clarify that we're

> > > >>> currently assuming straightforward topologies where all mappings

> > > >>> for a given root complex/named component target the same ITS

> > > >>> group. Otherwise

> > > we're

> > > >> going

> > > >>> to need somewhat more logic to iterate the its_node processing

> > > >>> over every mapping (or every alias in the PCI case), but avoid

> > > >>> creating duplicate entries.

> > > >>

> > > >> You have a point and we have time to update the code. Short of

> > > >> reserving all ITS regions for every device that maps to one at

> > > >> least, we could (even pre-compute instead of looking it up on the

> > > >> fly) create a list of ITS identifiers a given IORT node may map

> > > >> to and use that to reserve the regions.

> > > >

> > > > I am trying to understand the use case scenario discussed here.

> > > > Apologies if it is a dumb query.

> > > >

> > > > My understanding is that, it is possible to have a PCI  RC iort

> > > > node mapped

> > > to

> > > > multiple ITS group nodes.  That is perfectly fine and given a dev

> > > > input RID

> > > we

> > > > can identify the ITS group the device points to using -

> iort_node_map_id().

> > > >

> > > > But the above discussion seems to suggest that there might be

> > > > situations

> > > where

> > > > we have to go through all the mapped ITS groups and identify all

> > > > the ITSs

> > > associated

> > > > with the RC.  Clearly I am missing something.

> > >

> > > I was mostly thinking of a situation like this:

> > >

> > > +----Node 0-----+  +----Node 1-----+

> > > |  [CPU 0..n]   |  |  [CPU n+1..]  |

> > > | [ITS group 0] |  | [ITS group 1] |

> > > +---------------+  +---------------+

> > >         ^                  ^

> > >          \_______  _______/

> > >                  \/

> > >             +--Node 2--+

> > >             |  [SMMU]  |

> > >             |     ^    |

> > >             |     |    |

> > >             | [Device] |

> > >             +----------+

> > >

> > > where the (named component) device has IDs for both ITS groups (to

> > > help optimise affining, or allow physically hotplugging CPU nodes,

> > > or whatever - I'm hypothesising here ;)).  A generic IORT function

> > > isn't in a position to decide *which* ITS region the device may be

> > > targeting at any given time, so can only correctly describe both.

> >

> > Thanks Robin. That makes it clear.

> >

> > > I'm perfectly happy not to even try to support such crazy

> > > configurations until they actually exist, if ever; I'd just prefer

> > > to document whatever assumptions we do make, so that we don't have

> > > to remember or re-derive them when looking at the code in future.

> >

> > So I think the conclusion here is we will document the assumption that

> > we are only taking care of the straightforward topologies for now.

> >

> > Hi Lorenzo,

> > If you are ok with the above, please let me know if it make sense to

> > send out a v5 with this and your other comments or you can take care

> > of them. I am fine either way.

> 

> I added below what should be the final patch - please have a look test and

> post it as part of v5 that should hopefully be final.


Thanks Lorenzo. Will do that.
Shameer
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

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