On Thu, Oct 26, 2017 at 10:11:58AM +0000, Shameerali Kolothum Thodi wrote:
[...]
> > > >>> +int iommu_dma_get_msi_resv_regions(struct device *dev, struct
> > > list_head
> > > >> *list)
> > > >>> +{
> > > >>> + if (!is_of_node(dev->iommu_fwspec->iommu_fwnode))
> > > >>> + return iort_iommu_msi_get_resv_regions(dev, list);
> > >
> > > Either this call knows how to do the right thing for any platform and
> > > should be made from iommu_dma_get_reserved_regions() directly, or it's
> > > tightly coupled to the HiSilicon quirk in the SMMUv3 driver and
> > > iommu-dma doesn't need to know - the middle ground presented here is
> > > surely the worst of both worlds.
> >
> > Right. I think we have discussed this earlier[1] and had a v4 based on invoking
> > the iort_iommu_its_get_resv_regions() within the
> > iommu_dma_get_resv_regions().
> > But later as you rightly pointed out, we were not checking for platforms which
> > requires this quirk inside the iort code and that will break the platforms which
> > are happy with MSI translations. Hence moved to the current implementation
> > in v6 after this discussion here[2]
> >
> > And earlier I think in the v3 version we had the function called from smmu
> > driver directly and the feedback was that it should be abstracted from the
> > driver.
> >
> > May be it is still possible to move the function call inside the
> > iommu_dma_get_resv_regions() and do the smmu model check inside the iort
> > helper function and selectively apply the HW MSI reservations.
> >
> > But I think it is much neater if we can invoke the iort_get_msi_regions()
> > directly from SMMUv3 based on the model.
> >
> > Thoughts?
>
> As we still don’t have a clear resolution on how to invoke the
> iort_iommu_msi_get_resv_regions(), I have gone back and attempted to move the
> smmu model check inside the iort code. This means the function will selectively
> apply HW MSI reservation based on the platform and also the function can be
> invoked from the iommu_dma_get_resv_regions() directly.
>
> Could you please take a look at the below snippet and let me know your feedback.
> Hope we can make some progress on this series.
>
> Thanks,
> Shameer
>
> -->8--
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 876c0e1..a27233d 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -619,6 +619,39 @@ 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 acpi_iort_node *iommu = NULL;
> + int i;
> +
> + if (dev_is_pci(dev)) {
> + u32 rid;
> +
> + pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, &rid);
> + iommu = iort_node_map_id(node, rid, NULL, IORT_IOMMU_TYPE);
> + } else {
> + for (i = 0; i < node->mapping_count; i++) {
> + iommu = iort_node_map_platform_id(node, NULL,
> + IORT_IOMMU_TYPE, i);
> + if (iommu)
> + break;
> + }
> + }
You do not need (and I do not want this code) to do the mapping again.
You have the fwnode (ie dev->iommu_fwspec) corresponding to the IOMMU,
you can retrieve the SMMU IORT node by a simple look-up and carry out the
check below.
It would be simpler to set an option in the SMMUv3 driver but then
you go back to square one with DT/ACPI SMMUv3 driver awareness so, if,
with the change above this can make the generic approach work (ie Robin
is happy with it) I am fine with this IORT update as well.
Lorenzo
> +
> + 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)
> @@ -682,6 +715,9 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
> if (!node)
> return -ENODEV;
>
> + if (!iort_hw_msi_resv_enable(dev, node))
> + 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
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 9d1cebe..67c6e30 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -19,6 +19,7 @@
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include <linux/acpi_iort.h>
> #include <linux/device.h>
> #include <linux/dma-iommu.h>
> #include <linux/gfp.h>
> @@ -174,6 +175,10 @@ void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)
> struct pci_host_bridge *bridge;
> struct resource_entry *window;
>
> + if (!is_of_node(dev->iommu_fwspec->iommu_fwnode) &&
> + iort_iommu_msi_get_resv_regions(dev, list) < 0)
> + return;
> +
> if (!dev_is_pci(dev))
> return;
>
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 182a577..88f17c9 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -56,7 +56,7 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
> { return NULL; }
> static inline
> int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
> -{ return -ENODEV; }
> +{ return 0; }
> #endif
>
> #endif /* __ACPI_IORT_H__ */
>
> -->8--
>
>
>
