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