Hi Jordan,
Thanks for the patches, I finally got around testing them with SMMUv3.
It's an important feature, arguably more than SVA itself. I could pick
this one as part of the SVA series, what do you think?
Although I probably would have done the same, I dislike the interface
because it forces us to duplicate functions and IOMMU ops. The list is
small but growing:
iommu_map
iommu_map_sg
iommu_unmap
iommu_unmap_fast
iommu_iova_to_phys
iommu_tlb_range_add
iommu_flush_tlb_all
Each of these and their associated IOMMU op will have an iommu_sva_X
counterpart that takes one different argument. Modifying these functions
to take both a domain and a PASID argument would be more elegant. Or as
an intermediate solution, perhaps we could only change the IOMMU ops to
take an additional argument, like you did for map_sg?
In any case it requires invasive changes in lots of drivers and we can
always tidy up later, so unless Joerg has a preference I'd keep the
duplicates for now.
However, having to lookup pasid-to-io_mm on every map/unmap call is
cumbersome, especially since map/unmap are supposed to be as fast as
possible. iommu_sva_alloc_pasid should return a structure representing
the PASID instead of the value alone. The io_mm structure seems like a
good fit, and the device driver can access io_mm->pasid directly or via
an io_mm_get_pasid() function.
The new functions would then be:
struct io_mm *iommu_sva_alloc_pasid(domain, dev)
void iommu_sva_free_pasid(domain, io_mm)
int iommu_sva_map(io_mm, iova, paddr, size, prot)
size_t iommu_map_sg(io_mm, iova, sg, nents, prot)
size_t iommu_sva_unmap(io_mm, iova, size)
size_t iommu_sva_unmap_fast(io_mm, iova, size)
phys_addr_t iommu_sva_iova_to_phys(io_mm, iova)
void iommu_sva_flush_tlb_all(io_mm)
void iommu_sva_tlb_range_add(io_mm, iova, size)
A few more comments inline
On 18/05/18 22:34, Jordan Crouse wrote:
> Some older SMMU implementations that do not have a fully featured
> hardware PASID features have alternate workarounds for using multiple
> pagetables. For example, MSM GPUs have logic to automatically switch the
> user pagetable from hardware by writing the context bank directly.
The comment may be a bit too specific, sva_map/sva_unmap is also useful
for PASID-capable IOMMUs
> Support private PASIDs by creating a new io-pgtable instance map it
> to a PASID and provide the APIs for drivers to populate it manually.
>
> Signed-off-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>
> ---
[...]
> +int iommu_sva_alloc_pasid(struct iommu_domain *domain, struct device *dev)
> +{
> + int ret, pasid;
> + struct io_mm *io_mm;
> + struct iommu_sva_param *param = dev->iommu_param->sva_param;
We need a NULL check on the param, to ensure that the driver called
sva_device_init first.
> +
> + if (!domain->ops->mm_attach || !domain->ops->mm_detach)
> + return -ENODEV;
> +
> + if (domain->ops->mm_alloc)
I'd rather make mm_alloc and mm_free mandatory, but if we do make them
optional, then we need to check that both mm_alloc and mm_free are
present, or both absent.
> + io_mm = domain->ops->mm_alloc(domain, NULL, 0);
> + else
> + io_mm = kzalloc(sizeof(*io_mm), GFP_KERNEL);
> +
> + if (IS_ERR(io_mm))
> + return PTR_ERR(io_mm);
> + if (!io_mm)
> + return -ENOMEM;
> +
> + io_mm->domain = domain;
> + io_mm->type = IO_TYPE_PRIVATE;
This could be a IOMMU_SVA_FEAT_PRIVATE flag
> +
> + idr_preload(GFP_KERNEL);
> + spin_lock(&iommu_sva_lock);
> + pasid = idr_alloc_cyclic(&iommu_pasid_idr, io_mm, param->min_pasid,
> + param->max_pasid + 1, GFP_ATOMIC);
> + io_mm->pasid = pasid;
> + spin_unlock(&iommu_sva_lock);
> + idr_preload_end();
> +
> + if (pasid < 0) {
> + kfree(io_mm);
> + return pasid;
> + }
> +
> + ret = domain->ops->mm_attach(domain, dev, io_mm, false);
attach_domain should be true, otherwise the SMMUv3 driver won't write
the PASID table. But we should probably go through io_mm_attach here, to
make sure that PASID contexts are added to the mm list and cleaned up by
unbind_dev_all()
> +size_t iommu_sva_unmap(int pasid, unsigned long iova, size_t size)
> +{
> + struct io_mm *io_mm = get_io_mm(pasid);
> +
> + if (!io_mm || io_mm->type != IO_TYPE_PRIVATE)
> + return -ENODEV;
> +
> + return __iommu_unmap(io_mm->domain, &pasid, iova, size, false);
sync must be true here, and false in the unmap_fast() variant
> +}
> +EXPORT_SYMBOL_GPL(iommu_sva_unmap);
> +
> +void iommu_sva_free_pasid(int pasid, struct device *dev)
> +{
> + struct io_mm *io_mm = get_io_mm(pasid);
> + struct iommu_domain *domain;
> +
> + if (!io_mm || io_mm->type != IO_TYPE_PRIVATE)
> + return;
> +
> + domain = io_mm->domain;
> +
> + domain->ops->mm_detach(domain, dev, io_mm, false);
Here too detach_domain should be true
> @@ -1841,16 +1854,23 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
>
> /* unroll mapping in case something went wrong */
> if (ret)
> - iommu_unmap(domain, orig_iova, orig_size - size);
> + __iommu_unmap(domain, pasid, orig_iova, orig_size - size,
> + pasid ? false : true);
sync should be true
> - if (unlikely(ops->unmap == NULL ||
> - domain->pgsize_bitmap == 0UL))
> - return 0;
> + if (unlikely(domain->pgsize_bitmap == 0UL))
> + return -0;
spurious '-'
Thanks,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
