[4/5] iommu: Support mm PASID 1:n with sva domains
Commit Message
Each mm bound to devices gets a PASID and corresponding sva domains
allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
field of the mm. The PASID is released in __mmdrop(), while a sva domain
is released when on one is using it (the reference count is decremented
in iommu_sva_unbind_device()).
Since the required info of PASID and sva domains is kept in struct
iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
field in mm struct. The sva domain list is protected by iommu_sva_lock.
Besides, this patch removes mm_pasid_init(), as with the introduced
iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
drivers/iommu/iommu-sva.c | 38 +++++++++++++++++++++++++-------------
include/linux/iommu.h | 10 +++-------
kernel/fork.c | 1 -
3 files changed, 28 insertions(+), 21 deletions(-)
Comments
On Tue, Aug 08, 2023 at 03:49:43PM +0800, Tina Zhang wrote:
> Each mm bound to devices gets a PASID and corresponding sva domains
> allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
> field of the mm. The PASID is released in __mmdrop(), while a sva domain
> is released when on one is using it (the reference count is decremented
> in iommu_sva_unbind_device()).
>
> Since the required info of PASID and sva domains is kept in struct
> iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
> field in mm struct. The sva domain list is protected by iommu_sva_lock.
>
> Besides, this patch removes mm_pasid_init(), as with the introduced
> iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.
>
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
> drivers/iommu/iommu-sva.c | 38 +++++++++++++++++++++++++-------------
> include/linux/iommu.h | 10 +++-------
> kernel/fork.c | 1 -
> 3 files changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 0a4a1ed40814..35366f51ad27 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -15,6 +15,7 @@ static DEFINE_IDA(iommu_global_pasid_ida);
> /* Allocate a PASID for the mm within range (inclusive) */
> static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> {
> + struct iommu_mm_data *iommu_mm = NULL;
> int ret = 0;
>
> if (min == IOMMU_PASID_INVALID ||
> @@ -33,11 +34,22 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
> goto out;
> }
>
> + iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL);
> + if (!iommu_mm) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
> - if (ret < 0)
> + if (ret < 0) {
> + kfree(iommu_mm);
> goto out;
> + }
> +
> + iommu_mm->pasid = ret;
> + mm->iommu_mm = iommu_mm;
> + INIT_LIST_HEAD(&mm->iommu_mm->sva_domains);
>
> - mm->pasid = ret;
> ret = 0;
> out:
> mutex_unlock(&iommu_sva_lock);
> @@ -82,16 +94,12 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
Lets please rework this function into two parts
The first should be 'iommu_sva_alloc_domain()'
It should do the list searching and user management. The usual
'iommu_domain_free()' should be modified to clean it up.
Then you have the 'alloc global/enqcmd pasid' function
Then bind is just callling alloc sva domain, alloc global pasid,
set_dev_pasid in a sequence.
It will make this much more understandable what all the parts are
supposed to be doing.
Jason
On 8/8/23 23:19, Jason Gunthorpe wrote:
> On Tue, Aug 08, 2023 at 12:12:42PM -0300, Jason Gunthorpe wrote:
>
>> Lets please rework this function into two parts
>>
>> The first should be 'iommu_sva_alloc_domain()'
>>
>> It should do the list searching and user management. The usual
>> 'iommu_domain_free()' should be modified to clean it up.
>
> I suppose we can't quite do this fully since the domain allocation
> needs to use set_dev_pasid to detect compatability :( This is a
> similar problem we had with iommufd too.
>
> Still, the 'alloc and set_dev_pasid' should be in one function and it
> should be undone with iommu_detach_device_pasid() and
> iommu_domain_free().
>
> The helper bind/unbind functions should wrapper that sequence.
Make sense. I'll refine the error handling paths in bind/unbind
functions in the next version.
Thanks,
-Tina
>
> Jason
@@ -15,6 +15,7 @@ static DEFINE_IDA(iommu_global_pasid_ida);
/* Allocate a PASID for the mm within range (inclusive) */
static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
{
+ struct iommu_mm_data *iommu_mm = NULL;
int ret = 0;
if (min == IOMMU_PASID_INVALID ||
@@ -33,11 +34,22 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
goto out;
}
+ iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL);
+ if (!iommu_mm) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
- if (ret < 0)
+ if (ret < 0) {
+ kfree(iommu_mm);
goto out;
+ }
+
+ iommu_mm->pasid = ret;
+ mm->iommu_mm = iommu_mm;
+ INIT_LIST_HEAD(&mm->iommu_mm->sva_domains);
- mm->pasid = ret;
ret = 0;
out:
mutex_unlock(&iommu_sva_lock);
@@ -82,16 +94,12 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
mutex_lock(&iommu_sva_lock);
/* Search for an existing domain. */
- domain = iommu_get_domain_for_dev_pasid(dev, mm_get_pasid(mm),
- IOMMU_DOMAIN_SVA);
- if (IS_ERR(domain)) {
- ret = PTR_ERR(domain);
- goto out_unlock;
- }
-
- if (domain) {
- domain->users++;
- goto out;
+ list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
+ ret = iommu_attach_device_pasid(domain, dev, mm_get_pasid(mm));
+ if (!ret) {
+ domain->users++;
+ goto out;
+ }
}
/* Allocate a new domain and set it on device pasid. */
@@ -105,6 +113,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
if (ret)
goto out_free_domain;
domain->users = 1;
+ list_add(&domain->next, &mm->iommu_mm->sva_domains);
+
out:
mutex_unlock(&iommu_sva_lock);
handle->dev = dev;
@@ -137,8 +147,9 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
struct device *dev = handle->dev;
mutex_lock(&iommu_sva_lock);
+ iommu_detach_device_pasid(domain, dev, pasid);
if (--domain->users == 0) {
- iommu_detach_device_pasid(domain, dev, pasid);
+ list_del(&domain->next);
iommu_domain_free(domain);
}
mutex_unlock(&iommu_sva_lock);
@@ -218,4 +229,5 @@ void mm_pasid_drop(struct mm_struct *mm)
return;
ida_free(&iommu_global_pasid_ida, mm_get_pasid(mm));
+ kfree(mm->iommu_mm);
}
@@ -109,6 +109,7 @@ struct iommu_domain {
struct { /* IOMMU_DOMAIN_SVA */
struct mm_struct *mm;
int users;
+ struct list_head next;
};
};
};
@@ -1177,17 +1178,13 @@ static inline bool tegra_dev_iommu_get_stream_id(struct device *dev, u32 *stream
}
#ifdef CONFIG_IOMMU_SVA
-static inline void mm_pasid_init(struct mm_struct *mm)
-{
- mm->pasid = IOMMU_PASID_INVALID;
-}
static inline bool mm_valid_pasid(struct mm_struct *mm)
{
- return mm->pasid != IOMMU_PASID_INVALID;
+ return mm->iommu_mm ? true : false;
}
static inline u32 mm_get_pasid(struct mm_struct *mm)
{
- return mm->pasid;
+ return mm->iommu_mm ? mm->iommu_mm->pasid : IOMMU_PASID_INVALID;
}
void mm_pasid_drop(struct mm_struct *mm);
struct iommu_sva *iommu_sva_bind_device(struct device *dev,
@@ -1209,7 +1206,6 @@ static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
{
return IOMMU_PASID_INVALID;
}
-static inline void mm_pasid_init(struct mm_struct *mm) {}
static inline bool mm_valid_pasid(struct mm_struct *mm) { return false; }
static inline u32 mm_get_pasid(struct mm_struct *mm)
{
@@ -1274,7 +1274,6 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
mm_init_cpumask(mm);
mm_init_aio(mm);
mm_init_owner(mm, p);
- mm_pasid_init(mm);
RCU_INIT_POINTER(mm->exe_file, NULL);
mmu_notifier_subscriptions_init(mm);
init_tlb_flush_pending(mm);