The accelerator device can provide multi-queue and bind to
the same domain in multi-thread for better performance,
and domain refcount takes care of it.
'commit 092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")'
removes the possibility, so fix it
Fixs: '092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")'
Signed-off-by: Zhangfei Gao <[email protected]>
---
v2: Instead of checking ret == -EBUSY,
change iommu_attach_device_pasid return value from -EBUSY to 0
when pasid entry is found, and refcount++ when return
drivers/iommu/iommu-sva.c | 2 +-
drivers/iommu/iommu.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index c3fc9201d0be..20b232c7675d 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -141,8 +141,8 @@ 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, iommu_mm->pasid);
if (--domain->users == 0) {
+ iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
list_del(&domain->next);
iommu_domain_free(domain);
}
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d14413916f93..a16ade93db25 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3551,7 +3551,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
mutex_lock(&group->mutex);
curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
if (curr) {
- ret = xa_err(curr) ? : -EBUSY;
+ ret = xa_err(curr) ? : 0;
goto out_unlock;
}
--
2.34.1
On Wed, Feb 21, 2024 at 11:06:58AM +0000, Zhangfei Gao wrote:
> The accelerator device can provide multi-queue and bind to
> the same domain in multi-thread for better performance,
> and domain refcount takes care of it.
>
> 'commit 092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")'
> removes the possibility, so fix it
>
> Fixs: '092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")'
> Signed-off-by: Zhangfei Gao <[email protected]>
> ---
> v2: Instead of checking ret == -EBUSY,
> change iommu_attach_device_pasid return value from -EBUSY to 0
> when pasid entry is found, and refcount++ when return
>
> drivers/iommu/iommu-sva.c | 2 +-
> drivers/iommu/iommu.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index c3fc9201d0be..20b232c7675d 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -141,8 +141,8 @@ 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, iommu_mm->pasid);
> if (--domain->users == 0) {
> + iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
> list_del(&domain->next);
> iommu_domain_free(domain);
> }
The users refcount is not to provide for sharing of the same PASID it
is to provide for sharing the domain across devices. This change would
break that because we loose the 'dev' that needs to be detached in a
multi-device case if we don't immediately call detach_device_pasid
here.
You'd need to build something much more complicated here to allow
PASID sharing.
I wonder if this case is common enough to warrant the core code to get
involved. I suppose maybe, does idxd have the same problem? It can
only open it's cdev once because of this - that doesn't seem like what
the code intends for a non-wq_dedicated?
More like this:
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index c3fc9201d0be97..aec11e5cde6b0e 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -41,6 +41,7 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de
}
iommu_mm->pasid = pasid;
INIT_LIST_HEAD(&iommu_mm->sva_domains);
+ INIT_LIST_HEAD(&iommu_mm->sva_handles);
/*
* Make sure the write to mm->iommu_mm is not reordered in front of
* initialization to iommu_mm fields. If it does, readers may see a
@@ -82,6 +83,13 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
goto out_unlock;
}
+ list_for_each_entry(handle, &mm->iommu_mm->sva_handles, handle_item) {
+ if (handle->dev == dev && handle->domain->mm == mm) {
+ refcount_inc(&handle->users);
+ return handle;
+ }
+ }
+
handle = kzalloc(sizeof(*handle), GFP_KERNEL);
if (!handle) {
ret = -ENOMEM;
@@ -109,6 +117,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
goto out_free_domain;
domain->users = 1;
list_add(&domain->next, &mm->iommu_mm->sva_domains);
+ list_add(&handle->handle_item, &mm->iommu_mm->sva_handles);
out:
mutex_unlock(&iommu_sva_lock);
@@ -141,6 +150,12 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
struct device *dev = handle->dev;
mutex_lock(&iommu_sva_lock);
+ if (!refcount_dec_and_test(&handle->users)) {
+ mutex_unlock(&iommu_sva_lock);
+ return;
+ }
+ list_del(&handle->handle_item);
+
iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
if (--domain->users == 0) {
list_del(&domain->next);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 1ea2a820e1eb03..5e27cb3a3be99b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -892,11 +892,14 @@ struct iommu_fwspec {
struct iommu_sva {
struct device *dev;
struct iommu_domain *domain;
+ struct list_head handle_item;
+ refcount_t users;
};
struct iommu_mm_data {
u32 pasid;
struct list_head sva_domains;
+ struct list_head sva_handles;
};
int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
On Wed, 21 Feb 2024 at 21:17, Jason Gunthorpe <[email protected]> wrote:
>
> On Wed, Feb 21, 2024 at 11:06:58AM +0000, Zhangfei Gao wrote:
> > The accelerator device can provide multi-queue and bind to
> > the same domain in multi-thread for better performance,
> > and domain refcount takes care of it.
> >
> > 'commit 092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")'
> > removes the possibility, so fix it
> >
> > Fixs: '092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")'
> > Signed-off-by: Zhangfei Gao <[email protected]>
> > ---
> > v2: Instead of checking ret == -EBUSY,
> > change iommu_attach_device_pasid return value from -EBUSY to 0
> > when pasid entry is found, and refcount++ when return
> >
> > drivers/iommu/iommu-sva.c | 2 +-
> > drivers/iommu/iommu.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index c3fc9201d0be..20b232c7675d 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -141,8 +141,8 @@ 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, iommu_mm->pasid);
> > if (--domain->users == 0) {
> > + iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
> > list_del(&domain->next);
> > iommu_domain_free(domain);
> > }
>
> The users refcount is not to provide for sharing of the same PASID it
> is to provide for sharing the domain across devices. This change would
> break that because we loose the 'dev' that needs to be detached in a
> multi-device case if we don't immediately call detach_device_pasid
> here.
>
> You'd need to build something much more complicated here to allow
> PASID sharing.
>
> I wonder if this case is common enough to warrant the core code to get
> involved. I suppose maybe, does idxd have the same problem? It can
> only open it's cdev once because of this - that doesn't seem like what
> the code intends for a non-wq_dedicated?
>
> More like this:
Hi, Jason
Only added two lines change, and tested ok.
The different with before is same handle is returned, and the handle
itself has refcount.
While before different handle is returned,
Not think about any issue now, I think it is OK.
Could you send the patch, will add tested-by then.
>
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index c3fc9201d0be97..aec11e5cde6b0e 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -41,6 +41,7 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de
> }
> iommu_mm->pasid = pasid;
> INIT_LIST_HEAD(&iommu_mm->sva_domains);
> + INIT_LIST_HEAD(&iommu_mm->sva_handles);
> /*
> * Make sure the write to mm->iommu_mm is not reordered in front of
> * initialization to iommu_mm fields. If it does, readers may see a
> @@ -82,6 +83,13 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
> goto out_unlock;
> }
>
> + list_for_each_entry(handle, &mm->iommu_mm->sva_handles, handle_item) {
> + if (handle->dev == dev && handle->domain->mm == mm) {
> + refcount_inc(&handle->users);
mutex_unlock(&iommu_sva_lock);
> + return handle;
> + }
> + }
> +
> handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> if (!handle) {
> ret = -ENOMEM;
> @@ -109,6 +117,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
> goto out_free_domain;
> domain->users = 1;
refcount_set(&handle->users, 1);
> list_add(&domain->next, &mm->iommu_mm->sva_domains);
> + list_add(&handle->handle_item, &mm->iommu_mm->sva_handles);
>
> out:
> mutex_unlock(&iommu_sva_lock);
> @@ -141,6 +150,12 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
> struct device *dev = handle->dev;
>
> mutex_lock(&iommu_sva_lock);
> + if (!refcount_dec_and_test(&handle->users)) {
> + mutex_unlock(&iommu_sva_lock);
> + return;
> + }
> + list_del(&handle->handle_item);
> +
> iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
> if (--domain->users == 0) {
> list_del(&domain->next);
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 1ea2a820e1eb03..5e27cb3a3be99b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -892,11 +892,14 @@ struct iommu_fwspec {
> struct iommu_sva {
> struct device *dev;
> struct iommu_domain *domain;
> + struct list_head handle_item;
> + refcount_t users;
> };
>
> struct iommu_mm_data {
> u32 pasid;
> struct list_head sva_domains;
> + struct list_head sva_handles;
> };
>
> int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
Thanks
On Thu, 22 Feb 2024 at 00:17, Zhangfei Gao <[email protected]> wrote:
>
> On Wed, 21 Feb 2024 at 21:17, Jason Gunthorpe <[email protected]> wrote:
> >
> > On Wed, Feb 21, 2024 at 11:06:58AM +0000, Zhangfei Gao wrote:
> > > The accelerator device can provide multi-queue and bind to
> > > the same domain in multi-thread for better performance,
> > > and domain refcount takes care of it.
> > >
> > > 'commit 092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")'
> > > removes the possibility, so fix it
> > >
> > > Fixs: '092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")'
> > > Signed-off-by: Zhangfei Gao <[email protected]>
> > > ---
> > > v2: Instead of checking ret == -EBUSY,
> > > change iommu_attach_device_pasid return value from -EBUSY to 0
> > > when pasid entry is found, and refcount++ when return
> > >
> > > drivers/iommu/iommu-sva.c | 2 +-
> > > drivers/iommu/iommu.c | 2 +-
> > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > > index c3fc9201d0be..20b232c7675d 100644
> > > --- a/drivers/iommu/iommu-sva.c
> > > +++ b/drivers/iommu/iommu-sva.c
> > > @@ -141,8 +141,8 @@ 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, iommu_mm->pasid);
> > > if (--domain->users == 0) {
> > > + iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
> > > list_del(&domain->next);
> > > iommu_domain_free(domain);
> > > }
> >
> > The users refcount is not to provide for sharing of the same PASID it
> > is to provide for sharing the domain across devices. This change would
> > break that because we loose the 'dev' that needs to be detached in a
> > multi-device case if we don't immediately call detach_device_pasid
> > here.
> >
> > You'd need to build something much more complicated here to allow
> > PASID sharing.
> >
> > I wonder if this case is common enough to warrant the core code to get
> > involved. I suppose maybe, does idxd have the same problem? It can
> > only open it's cdev once because of this - that doesn't seem like what
> > the code intends for a non-wq_dedicated?
> >
> > More like this:
>
> Hi, Jason
>
> Only added two lines change, and tested ok.
> The different with before is same handle is returned, and the handle
> itself has refcount.
> While before different handle is returned,
> Not think about any issue now, I think it is OK.
>
> Could you send the patch, will add tested-by then.
Or would you mind I send the patch on behalf of you?
The limitation breaks our existing system :(
https://github.com/Linaro/linux-kernel-uadk/commit/4c48330faf727303e3127c9ee6fbf56d885b4297
Thanks
>
> >
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index c3fc9201d0be97..aec11e5cde6b0e 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -41,6 +41,7 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de
> > }
> > iommu_mm->pasid = pasid;
> > INIT_LIST_HEAD(&iommu_mm->sva_domains);
> > + INIT_LIST_HEAD(&iommu_mm->sva_handles);
> > /*
> > * Make sure the write to mm->iommu_mm is not reordered in front of
> > * initialization to iommu_mm fields. If it does, readers may see a
> > @@ -82,6 +83,13 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
> > goto out_unlock;
> > }
> >
> > + list_for_each_entry(handle, &mm->iommu_mm->sva_handles, handle_item) {
> > + if (handle->dev == dev && handle->domain->mm == mm) {
> > + refcount_inc(&handle->users);
>
> mutex_unlock(&iommu_sva_lock);
>
>
> > + return handle;
> > + }
> > + }
> > +
> > handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> > if (!handle) {
> > ret = -ENOMEM;
> > @@ -109,6 +117,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
> > goto out_free_domain;
> > domain->users = 1;
>
> refcount_set(&handle->users, 1);
>
>
> > list_add(&domain->next, &mm->iommu_mm->sva_domains);
> > + list_add(&handle->handle_item, &mm->iommu_mm->sva_handles);
> >
> > out:
> > mutex_unlock(&iommu_sva_lock);
> > @@ -141,6 +150,12 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
> > struct device *dev = handle->dev;
> >
> > mutex_lock(&iommu_sva_lock);
> > + if (!refcount_dec_and_test(&handle->users)) {
> > + mutex_unlock(&iommu_sva_lock);
> > + return;
> > + }
> > + list_del(&handle->handle_item);
> > +
> > iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
> > if (--domain->users == 0) {
> > list_del(&domain->next);
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 1ea2a820e1eb03..5e27cb3a3be99b 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -892,11 +892,14 @@ struct iommu_fwspec {
> > struct iommu_sva {
> > struct device *dev;
> > struct iommu_domain *domain;
> > + struct list_head handle_item;
> > + refcount_t users;
> > };
> >
> > struct iommu_mm_data {
> > u32 pasid;
> > struct list_head sva_domains;
> > + struct list_head sva_handles;
> > };
> >
> > int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>
> Thanks