* [PATCH rc] iommu/sva: Restore SVA handle sharing
@ 2024-02-22 14:07 Jason Gunthorpe
2024-02-22 14:39 ` Zhangfei Gao
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2024-02-22 14:07 UTC (permalink / raw)
To: iommu, Joerg Roedel, Robin Murphy, Will Deacon
Cc: Lu Baolu, Joerg Roedel, Nicolin Chen, patches, Tina Zhang,
Vasant Hegde, Zhangfei Gao
Prior to commit 092edaddb660 ("iommu: Support mm PASID 1:n with sva
domains") the code allowed a SVA handle to be bound multiple times to the
same (mm, device) pair. This was alluded to in the kdoc comment, but we
had understood this to be more a remark about allowing multiple devices,
not a literal same-driver re-opening the same SVA.
It turns out uacce and idxd were both relying on the core code to handle
reference counting for same-device same-mm scenarios. As this looks hard
to resolve in the drivers bring it back to the core code.
The new design has changed the meaning of the domain->users refcount to
refer to the number of devices that are sharing that domain for the same
mm. This is part of the design to lift the SVA domain de-duplication out
of the drivers.
Return the old behavior by explicitly de-duplicating the struct iommu_sva
handle. The same (mm, device) will return the same handle pointer and the
core code will handle tracking this. The last unbind of the handle will
destroy it.
Fixes: 092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")
Reported-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Closes: https://lore.kernel.org/all/20240221110658.529-1-zhangfei.gao@linaro.org/
Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/iommu-sva.c | 17 +++++++++++++++++
include/linux/iommu.h | 3 +++
2 files changed, 20 insertions(+)
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index c3fc9201d0be97..7f91c8d0064b71 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,14 @@ 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) {
+ refcount_inc(&handle->users);
+ mutex_unlock(&iommu_sva_lock);
+ return handle;
+ }
+ }
+
handle = kzalloc(sizeof(*handle), GFP_KERNEL);
if (!handle) {
ret = -ENOMEM;
@@ -108,7 +117,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
if (ret)
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 +152,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,
base-commit: 510325e5ac5f45c1180189d3bfc108c54bf64544
--
2.43.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH rc] iommu/sva: Restore SVA handle sharing
2024-02-22 14:07 [PATCH rc] iommu/sva: Restore SVA handle sharing Jason Gunthorpe
@ 2024-02-22 14:39 ` Zhangfei Gao
2024-02-23 5:06 ` Baolu Lu
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Zhangfei Gao @ 2024-02-22 14:39 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: iommu, Joerg Roedel, Robin Murphy, Will Deacon, Lu Baolu,
Joerg Roedel, Nicolin Chen, patches, Tina Zhang, Vasant Hegde
On Thu, 22 Feb 2024 at 22:07, Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> Prior to commit 092edaddb660 ("iommu: Support mm PASID 1:n with sva
> domains") the code allowed a SVA handle to be bound multiple times to the
> same (mm, device) pair. This was alluded to in the kdoc comment, but we
> had understood this to be more a remark about allowing multiple devices,
> not a literal same-driver re-opening the same SVA.
>
> It turns out uacce and idxd were both relying on the core code to handle
> reference counting for same-device same-mm scenarios. As this looks hard
> to resolve in the drivers bring it back to the core code.
>
> The new design has changed the meaning of the domain->users refcount to
> refer to the number of devices that are sharing that domain for the same
> mm. This is part of the design to lift the SVA domain de-duplication out
> of the drivers.
>
> Return the old behavior by explicitly de-duplicating the struct iommu_sva
> handle. The same (mm, device) will return the same handle pointer and the
> core code will handle tracking this. The last unbind of the handle will
> destroy it.
>
> Fixes: 092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")
> Reported-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Closes: https://lore.kernel.org/all/20240221110658.529-1-zhangfei.gao@linaro.org/
> Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Thanks Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH rc] iommu/sva: Restore SVA handle sharing
2024-02-22 14:07 [PATCH rc] iommu/sva: Restore SVA handle sharing Jason Gunthorpe
2024-02-22 14:39 ` Zhangfei Gao
@ 2024-02-23 5:06 ` Baolu Lu
2024-02-23 15:45 ` Joerg Roedel
2024-02-26 8:38 ` Zhangfei Gao
3 siblings, 0 replies; 6+ messages in thread
From: Baolu Lu @ 2024-02-23 5:06 UTC (permalink / raw)
To: Jason Gunthorpe, iommu, Joerg Roedel, Robin Murphy, Will Deacon
Cc: baolu.lu, Joerg Roedel, Nicolin Chen, patches, Tina Zhang,
Vasant Hegde, Zhangfei Gao
On 2024/2/22 22:07, Jason Gunthorpe wrote:
> Prior to commit 092edaddb660 ("iommu: Support mm PASID 1:n with sva
> domains") the code allowed a SVA handle to be bound multiple times to the
> same (mm, device) pair. This was alluded to in the kdoc comment, but we
> had understood this to be more a remark about allowing multiple devices,
> not a literal same-driver re-opening the same SVA.
>
> It turns out uacce and idxd were both relying on the core code to handle
> reference counting for same-device same-mm scenarios. As this looks hard
> to resolve in the drivers bring it back to the core code.
>
> The new design has changed the meaning of the domain->users refcount to
> refer to the number of devices that are sharing that domain for the same
> mm. This is part of the design to lift the SVA domain de-duplication out
> of the drivers.
>
> Return the old behavior by explicitly de-duplicating the struct iommu_sva
> handle. The same (mm, device) will return the same handle pointer and the
> core code will handle tracking this. The last unbind of the handle will
> destroy it.
>
> Fixes: 092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")
> Reported-by: Zhangfei Gao<zhangfei.gao@linaro.org>
> Closes:https://lore.kernel.org/all/20240221110658.529-1-zhangfei.gao@linaro.org/
> Tested-by: Zhangfei Gao<zhangfei.gao@linaro.org>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Best regards,
baolu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH rc] iommu/sva: Restore SVA handle sharing
2024-02-22 14:07 [PATCH rc] iommu/sva: Restore SVA handle sharing Jason Gunthorpe
2024-02-22 14:39 ` Zhangfei Gao
2024-02-23 5:06 ` Baolu Lu
@ 2024-02-23 15:45 ` Joerg Roedel
2024-02-26 8:38 ` Zhangfei Gao
3 siblings, 0 replies; 6+ messages in thread
From: Joerg Roedel @ 2024-02-23 15:45 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: iommu, Robin Murphy, Will Deacon, Lu Baolu, Joerg Roedel,
Nicolin Chen, patches, Tina Zhang, Vasant Hegde, Zhangfei Gao
On Thu, Feb 22, 2024 at 10:07:41AM -0400, Jason Gunthorpe wrote:
> drivers/iommu/iommu-sva.c | 17 +++++++++++++++++
> include/linux/iommu.h | 3 +++
> 2 files changed, 20 insertions(+)
Applied, thanks Jason.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH rc] iommu/sva: Restore SVA handle sharing
2024-02-22 14:07 [PATCH rc] iommu/sva: Restore SVA handle sharing Jason Gunthorpe
` (2 preceding siblings ...)
2024-02-23 15:45 ` Joerg Roedel
@ 2024-02-26 8:38 ` Zhangfei Gao
2024-02-26 13:07 ` Jason Gunthorpe
3 siblings, 1 reply; 6+ messages in thread
From: Zhangfei Gao @ 2024-02-26 8:38 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: iommu, Joerg Roedel, Robin Murphy, Will Deacon, Lu Baolu,
Joerg Roedel, Nicolin Chen, patches, Tina Zhang, Vasant Hegde
On Thu, 22 Feb 2024 at 22:07, Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> Prior to commit 092edaddb660 ("iommu: Support mm PASID 1:n with sva
> domains") the code allowed a SVA handle to be bound multiple times to the
> same (mm, device) pair. This was alluded to in the kdoc comment, but we
> had understood this to be more a remark about allowing multiple devices,
> not a literal same-driver re-opening the same SVA.
>
> It turns out uacce and idxd were both relying on the core code to handle
> reference counting for same-device same-mm scenarios. As this looks hard
> to resolve in the drivers bring it back to the core code.
>
> The new design has changed the meaning of the domain->users refcount to
> refer to the number of devices that are sharing that domain for the same
> mm. This is part of the design to lift the SVA domain de-duplication out
> of the drivers.
>
> Return the old behavior by explicitly de-duplicating the struct iommu_sva
> handle. The same (mm, device) will return the same handle pointer and the
> core code will handle tracking this. The last unbind of the handle will
> destroy it.
>
> Fixes: 092edaddb660 ("iommu: Support mm PASID 1:n with sva domains")
> Reported-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Closes: https://lore.kernel.org/all/20240221110658.529-1-zhangfei.gao@linaro.org/
> Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> drivers/iommu/iommu-sva.c | 17 +++++++++++++++++
> include/linux/iommu.h | 3 +++
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index c3fc9201d0be97..7f91c8d0064b71 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,14 @@ 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) {
> + refcount_inc(&handle->users);
> + mutex_unlock(&iommu_sva_lock);
> + return handle;
> + }
> + }
> +
> handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> if (!handle) {
> ret = -ENOMEM;
> @@ -108,7 +117,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
> if (ret)
> 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 +152,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,
>
> base-commit: 510325e5ac5f45c1180189d3bfc108c54bf64544
> --
> 2.43.2
>
Hi, Jason
I am testing on 6.8-rc6 with more test and found one issue, sorry about this.
When test openssl rsa, it will use two devices, one is sec, the other is hpre.
I found the second handle can not be shared since bind_dev directly
goto out but not add_list.
So still need a minor fix like this
iommu/sva: Fix SVA handle sharing in multi device case
iommu_sva_bind_device will directly goto out in multi-device
case when found existing domain, ignoring list_add handle,
which causes the handle to fail to be shared.
Fixs: 65d4418c5002 (iommu/sva: Restore SVA handle sharing)
Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 7f91c8d0064b..b1f3a864e124 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -117,11 +117,10 @@ struct iommu_sva *iommu_sva_bind_device(struct
device *dev, struct mm_struct *mm
if (ret)
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:
+ refcount_set(&handle->users, 1);
+ list_add(&handle->handle_item, &mm->iommu_mm->sva_handles);
mutex_unlock(&iommu_sva_lock);
handle->dev = dev;
handle->domain = domain;
What do you think.
Thanks
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH rc] iommu/sva: Restore SVA handle sharing
2024-02-26 8:38 ` Zhangfei Gao
@ 2024-02-26 13:07 ` Jason Gunthorpe
0 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2024-02-26 13:07 UTC (permalink / raw)
To: Zhangfei Gao
Cc: iommu, Joerg Roedel, Robin Murphy, Will Deacon, Lu Baolu,
Joerg Roedel, Nicolin Chen, patches, Tina Zhang, Vasant Hegde
On Mon, Feb 26, 2024 at 04:38:52PM +0800, Zhangfei Gao wrote:
> Hi, Jason
>
> I am testing on 6.8-rc6 with more test and found one issue, sorry about this.
> When test openssl rsa, it will use two devices, one is sec, the other is hpre.
> I found the second handle can not be shared since bind_dev directly
> goto out but not add_list.
>
> So still need a minor fix like this
>
> iommu/sva: Fix SVA handle sharing in multi device case
>
>
> iommu_sva_bind_device will directly goto out in multi-device
> case when found existing domain, ignoring list_add handle,
> which causes the handle to fail to be shared.
>
> Fixs: 65d4418c5002 (iommu/sva: Restore SVA handle sharing)
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 7f91c8d0064b..b1f3a864e124 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -117,11 +117,10 @@ struct iommu_sva *iommu_sva_bind_device(struct
> device *dev, struct mm_struct *mm
> if (ret)
> 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:
> + refcount_set(&handle->users, 1);
> + list_add(&handle->handle_item, &mm->iommu_mm->sva_handles);
> mutex_unlock(&iommu_sva_lock);
> handle->dev = dev;
> handle->domain = domain;
>
> What do you think.
Yes, please send a patch
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-26 13:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-22 14:07 [PATCH rc] iommu/sva: Restore SVA handle sharing Jason Gunthorpe
2024-02-22 14:39 ` Zhangfei Gao
2024-02-23 5:06 ` Baolu Lu
2024-02-23 15:45 ` Joerg Roedel
2024-02-26 8:38 ` Zhangfei Gao
2024-02-26 13:07 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).