RE: [PATCH v10 1/3] ACPI/IORT: Add msi address regions reservation helper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@xxxxxxx]
> Sent: Monday, December 11, 2017 2:45 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>
> Cc: will.deacon@xxxxxxx; robin.murphy@xxxxxxx; marc.zyngier@xxxxxxx;
> joro@xxxxxxxxxx; John Garry <john.garry@xxxxxxxxxx>; xuwei (O)
> <xuwei5@xxxxxxxxxxxxx>; Guohanjun (Hanjun Guo) <guohanjun@xxxxxxxxxx>;
> iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> acpi@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Linuxarm
> <linuxarm@xxxxxxxxxx>
> Subject: Re: [PATCH v10 1/3] ACPI/IORT: Add msi address regions reservation
> helper
> 
> On Wed, Nov 29, 2017 at 02:14:47PM +0000, Shameer Kolothum wrote:
> > On some platforms msi parent 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 a helper function that retrieves ITS address regions - the msi
> > parent - 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. The function checks for the smmu model number and
> > only applies the msi reservation if the platform requires it.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx>
> > ---
> >  drivers/acpi/arm64/iort.c        | 133
> ++++++++++++++++++++++++++++++++++++++-
> >  drivers/irqchip/irq-gic-v3-its.c |   3 +-
> >  include/linux/acpi_iort.h        |   7 ++-
> >  3 files changed, 138 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index 95255ec..1c5fc36 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;
> >  };
> >
> > @@ -161,14 +162,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;
> >
> > @@ -178,6 +181,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);
> > @@ -581,6 +585,24 @@ int iort_pmsi_get_dev_id(struct device *dev, u32
> *dev_id)
> >  	return -ENODEV;
> >  }
> >
> > +static int __maybe_unused iort_find_its_base(u32 its_id, phys_addr_t *base)
> > +{
> > +	struct iort_its_msi_chip *its_msi_chip;
> > +	bool match = false;
> > +
> > +	spin_lock(&iort_msi_chip_lock);
> > +	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {
> > +		if (its_msi_chip->translation_id == its_id) {
> > +			*base = its_msi_chip->base_addr;
> > +			match = true;
> > +			break;
> > +		}
> > +	}
> > +	spin_unlock(&iort_msi_chip_lock);
> > +
> > +	return match ? 0 : -ENODEV;
> 
> Nit: if you need to return an int, use it as the "match" variable,
> there is no point in using a bool.
> 
> int ret = -ENODEV;
> 
> if (its_msi_chip->translation_id == its_id) {
> 	...
> 	ret = 0;
> 	break;
> }
> 
> return ret;
> 
> > +}
> > +
> >  /**
> >   * iort_dev_find_its_id() - Find the ITS identifier for a device
> >   * @dev: The device.
> > @@ -740,6 +762,38 @@ static int __maybe_unused __get_pci_rid(struct
> pci_dev *pdev, u16 alias,
> >  	return 0;
> >  }
> >
> > +static bool __maybe_unused iort_hw_msi_resv_enable(struct device *dev,
> > +					struct acpi_iort_node *node)
> > +{
> > +	struct iort_fwnode *curr;
> > +	struct acpi_iort_node *iommu = NULL;
> > +	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > +
> > +	if (WARN_ON(!fwspec || !fwspec->iommu_fwnode))
> 
> Remove WARN_ON() here, it seems excessive.
> 
> > +		return false;
> > +
> > +	spin_lock(&iort_fwnode_lock);
> > +	list_for_each_entry(curr, &iort_fwnode_list, list) {
> > +		if (curr->fwnode == fwspec->iommu_fwnode) {
> > +			iommu = curr->iort_node;
> > +			break;
> > +		}
> > +	}
> > +	spin_unlock(&iort_fwnode_lock);
> 
> iort_get_iort_node() ?
> 
> > +	if (iommu && (iommu->type == ACPI_IORT_NODE_SMMU_V3)) {
> > +		struct acpi_iort_smmu_v3 *smmu;
> > +
> > +		smmu = (struct acpi_iort_smmu_v3 *)iommu->node_data;
> > +		if (smmu->model ==
> ACPI_IORT_SMMU_V3_HISILICON_HI161X) {
> > +			dev_notice(dev, "Enabling HiSilicon erratum
> 161010801\n");
> > +			return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
> >  			       struct fwnode_handle *fwnode,
> >  			       const struct iommu_ops *ops)
> > @@ -782,6 +836,77 @@ static inline int iort_add_device_replay(const struct
> iommu_ops *ops,
> >
> >  	return err;
> >  }
> > +
> > +/**
> > + * iort_iommu_msi_get_resv_regions - Reserved region driver helper
> > + * @dev: Device from iommu_get_resv_regions()
> > + * @head: Reserved region list from iommu_get_resv_regions()
> > + *
> > + * Returns: Number of msi reserved regions on success (0 if platform
> > + *          doesn't require the reservation or no associated msi regions),
> > + *          appropriate error value otherwise. The ITS interrupt translation
> > + *          space (ITS_base + 0x010000) associated with the device are the
> > + *          msi reserved regions.
> > + */
> > +int iort_iommu_msi_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;
> 
> You may not need node, see below.
> 
> > +	if (!iort_hw_msi_resv_enable(dev, node))
> 
> node is passed but effectively not used by the function.
> 
> > +		return 0;
> > +
> > +	/*
> > +	 * 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;
> > +		}
> > +	}
> 
> Ok. So, how about doing:
> 
> for (i = 0; i < dev->iommu_fwspec->num_ids; i++) {
> 	its_node = iort_node_map_id(iommu_iort_node, dev->iommu_fwspec-
> >ids[i],
> 				    NULL, IORT_MSI_TYPE);
> 	if (its_node)
> 		break;
> }
> 
> which removes the need for dev_is_pci() check and simplifies a bit, is
> there any issue with that ?
> 
> iommu_iort_node can be returned by iort_hw_msi_resv_enable(), that
> you can rename accordingly.

Thanks Lorenzo. I will address the comments and send the revised patch
after testing.

Much appreciated,
Shameer
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux