* [PATCH v2] iommu: Fix iommu_sva_bind_device to the same domain
@ 2024-02-21 11:06 Zhangfei Gao
2024-02-21 13:17 ` Jason Gunthorpe
0 siblings, 1 reply; 4+ messages in thread
From: Zhangfei Gao @ 2024-02-21 11:06 UTC (permalink / raw)
To: Joerg Roedel, Will Deacon, jean-philippe, Jason Gunthorpe,
baolu.lu, Zhang, Tina, kevin.tian
Cc: iommu, linux-kernel, Zhangfei Gao
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 <zhangfei.gao@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iommu: Fix iommu_sva_bind_device to the same domain
2024-02-21 11:06 [PATCH v2] iommu: Fix iommu_sva_bind_device to the same domain Zhangfei Gao
@ 2024-02-21 13:17 ` Jason Gunthorpe
2024-02-21 16:17 ` Zhangfei Gao
0 siblings, 1 reply; 4+ messages in thread
From: Jason Gunthorpe @ 2024-02-21 13:17 UTC (permalink / raw)
To: Zhangfei Gao
Cc: Joerg Roedel, Will Deacon, jean-philippe, baolu.lu, Zhang, Tina,
kevin.tian, iommu, linux-kernel
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 <zhangfei.gao@linaro.org>
> ---
> 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,
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iommu: Fix iommu_sva_bind_device to the same domain
2024-02-21 13:17 ` Jason Gunthorpe
@ 2024-02-21 16:17 ` Zhangfei Gao
2024-02-22 4:11 ` Zhangfei Gao
0 siblings, 1 reply; 4+ messages in thread
From: Zhangfei Gao @ 2024-02-21 16:17 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Joerg Roedel, Will Deacon, jean-philippe, baolu.lu, Zhang, Tina,
kevin.tian, iommu, linux-kernel
On Wed, 21 Feb 2024 at 21:17, Jason Gunthorpe <jgg@nvidia.com> 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 <zhangfei.gao@linaro.org>
> > ---
> > 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iommu: Fix iommu_sva_bind_device to the same domain
2024-02-21 16:17 ` Zhangfei Gao
@ 2024-02-22 4:11 ` Zhangfei Gao
0 siblings, 0 replies; 4+ messages in thread
From: Zhangfei Gao @ 2024-02-22 4:11 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Joerg Roedel, Will Deacon, jean-philippe, baolu.lu, Zhang, Tina,
kevin.tian, iommu, linux-kernel
On Thu, 22 Feb 2024 at 00:17, Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
>
> On Wed, 21 Feb 2024 at 21:17, Jason Gunthorpe <jgg@nvidia.com> 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 <zhangfei.gao@linaro.org>
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-02-22 4:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-21 11:06 [PATCH v2] iommu: Fix iommu_sva_bind_device to the same domain Zhangfei Gao
2024-02-21 13:17 ` Jason Gunthorpe
2024-02-21 16:17 ` Zhangfei Gao
2024-02-22 4:11 ` Zhangfei Gao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.