| Message ID | 20220826121141.50743-13-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu: SVA and IOPF refactoring | expand |
On Fri, Aug 26, 2022 at 08:11:36PM +0800, Lu Baolu wrote: > +static const struct iommu_domain_ops arm_smmu_sva_domain_ops = { > + .set_dev_pasid = arm_smmu_sva_set_dev_pasid, Do we want to permit drivers to not allow a SVA domain to be set on a RID? It seems like a weird restriction to me Jason
On 2022/8/26 22:56, Jason Gunthorpe wrote: > On Fri, Aug 26, 2022 at 08:11:36PM +0800, Lu Baolu wrote: > >> +static const struct iommu_domain_ops arm_smmu_sva_domain_ops = { >> + .set_dev_pasid = arm_smmu_sva_set_dev_pasid, > Do we want to permit drivers to not allow a SVA domain to be set on a > RID? > > It seems like a weird restriction to me Conceptually as long as the page table is compatible and user pages are pinned (or I/O page fault is supported), the device drivers are valid to set SVA domain to a RID. But I don't see a real use case as far as I can see. A reasonable use case is sharing EPT between KVM and IOMMU. That demands a new type of domain and implements its own .set_dev for page table attachment. Best regards, baolu
On Sun, Aug 28, 2022 at 09:57:21PM +0800, Baolu Lu wrote: > On 2022/8/26 22:56, Jason Gunthorpe wrote: > > On Fri, Aug 26, 2022 at 08:11:36PM +0800, Lu Baolu wrote: > > > > > +static const struct iommu_domain_ops arm_smmu_sva_domain_ops = { > > > + .set_dev_pasid = arm_smmu_sva_set_dev_pasid, > > Do we want to permit drivers to not allow a SVA domain to be set on a > > RID? > > > > It seems like a weird restriction to me > > Conceptually as long as the page table is compatible and user pages are > pinned (or I/O page fault is supported), the device drivers are valid to > set SVA domain to a RID. But I don't see a real use case as far as I can > see. It may be interesting for something like DPDK type applications where having the entire process address space mapped SVA to the device could be quite nice. You, currently, give up interrupts, but perhaps that is solvable in some way. So, IDK.. I wouldn't dismiss it entirely but I wouldn't do a bunch of work to support it either. > A reasonable use case is sharing EPT between KVM and IOMMU. That demands > a new type of domain and implements its own .set_dev for page table > attachment. Not everything is virtualization :) Jason
On 2022/8/30 01:29, Jason Gunthorpe wrote: > On Sun, Aug 28, 2022 at 09:57:21PM +0800, Baolu Lu wrote: >> On 2022/8/26 22:56, Jason Gunthorpe wrote: >>> On Fri, Aug 26, 2022 at 08:11:36PM +0800, Lu Baolu wrote: >>> >>>> +static const struct iommu_domain_ops arm_smmu_sva_domain_ops = { >>>> + .set_dev_pasid = arm_smmu_sva_set_dev_pasid, >>> Do we want to permit drivers to not allow a SVA domain to be set on a >>> RID? >>> >>> It seems like a weird restriction to me >> Conceptually as long as the page table is compatible and user pages are >> pinned (or I/O page fault is supported), the device drivers are valid to >> set SVA domain to a RID. But I don't see a real use case as far as I can >> see. > It may be interesting for something like DPDK type applications where > having the entire process address space mapped SVA to the device could > be quite nice. > > You, currently, give up interrupts, but perhaps that is solvable in some > way. > > So, IDK.. I wouldn't dismiss it entirely but I wouldn't do a bunch of > work to support it either. Then we can do this through the set_dev callback, as it's the right callback to set a domain to the RID, right? Not sure whether it worth a new type of domain. The current implementation doesn't prevent us from achieving this in the future anyway. > >> A reasonable use case is sharing EPT between KVM and IOMMU. That demands >> a new type of domain and implements its own .set_dev for page table >> attachment. > Not everything is virtualization:) Yes. Fair enough. :-) Best regards, baolu
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index d2ba86470c42..c9544b656756 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -758,6 +758,9 @@ struct iommu_sva *arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm); void arm_smmu_sva_unbind(struct iommu_sva *handle); u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle); void arm_smmu_sva_notifier_synchronize(void); +struct iommu_domain *arm_smmu_sva_domain_alloc(void); +void arm_smmu_sva_block_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t id); #else /* CONFIG_ARM_SMMU_V3_SVA */ static inline bool arm_smmu_sva_supported(struct arm_smmu_device *smmu) { @@ -803,5 +806,15 @@ static inline u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle) } static inline void arm_smmu_sva_notifier_synchronize(void) {} + +static inline struct iommu_domain *arm_smmu_sva_domain_alloc(void) +{ + return NULL; +} + +static inline void arm_smmu_sva_block_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t id) +{ +} #endif /* CONFIG_ARM_SMMU_V3_SVA */ #endif /* _ARM_SMMU_V3_H */ diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index f155d406c5d5..953ba19a1af8 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -549,3 +549,64 @@ void arm_smmu_sva_notifier_synchronize(void) */ mmu_notifier_synchronize(); } + +void arm_smmu_sva_block_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t id) +{ + struct mm_struct *mm = domain->mm; + struct arm_smmu_bond *bond = NULL, *t; + struct arm_smmu_master *master = dev_iommu_priv_get(dev); + + mutex_lock(&sva_lock); + list_for_each_entry(t, &master->bonds, list) { + if (t->mm == mm) { + bond = t; + break; + } + } + + if (!WARN_ON(!bond) && refcount_dec_and_test(&bond->refs)) { + list_del(&bond->list); + arm_smmu_mmu_notifier_put(bond->smmu_mn); + kfree(bond); + } + mutex_unlock(&sva_lock); +} + +static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t id) +{ + int ret = 0; + struct iommu_sva *handle; + struct mm_struct *mm = domain->mm; + + mutex_lock(&sva_lock); + handle = __arm_smmu_sva_bind(dev, mm); + if (IS_ERR(handle)) + ret = PTR_ERR(handle); + mutex_unlock(&sva_lock); + + return ret; +} + +static void arm_smmu_sva_domain_free(struct iommu_domain *domain) +{ + kfree(domain); +} + +static const struct iommu_domain_ops arm_smmu_sva_domain_ops = { + .set_dev_pasid = arm_smmu_sva_set_dev_pasid, + .free = arm_smmu_sva_domain_free +}; + +struct iommu_domain *arm_smmu_sva_domain_alloc(void) +{ + struct iommu_domain *domain; + + domain = kzalloc(sizeof(*domain), GFP_KERNEL); + if (!domain) + return NULL; + domain->ops = &arm_smmu_sva_domain_ops; + + return domain; +} diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 5520a9607758..a27ce693cc65 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2015,9 +2015,25 @@ static int blocking_domain_attach_dev(struct iommu_domain *domain, return 0; } +static int blocking_domain_set_dev_pasid(struct iommu_domain *_domain, + struct device *dev, ioasid_t pasid) +{ + struct iommu_domain *domain; + + domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA); + if (IS_ERR(domain)) + return PTR_ERR(domain); + + if (domain) + arm_smmu_sva_block_dev_pasid(domain, dev, pasid); + + return 0; +} + static struct iommu_domain blocking_domain = { .ops = &(const struct iommu_domain_ops) { - .attach_dev = blocking_domain_attach_dev + .attach_dev = blocking_domain_attach_dev, + .set_dev_pasid = blocking_domain_set_dev_pasid } }; @@ -2028,6 +2044,9 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) if (type == IOMMU_DOMAIN_BLOCKED) return &blocking_domain; + if (type == IOMMU_DOMAIN_SVA) + return arm_smmu_sva_domain_alloc(); + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_DMA_FQ &&