Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On 2022/5/12 20:03, Jason Gunthorpe wrote:
On Thu, May 12, 2022 at 07:59:41PM +0800, Baolu Lu wrote:
On 2022/5/12 19:48, Jason Gunthorpe wrote:
On Thu, May 12, 2022 at 01:17:08PM +0800, Baolu Lu wrote:
On 2022/5/12 13:01, Tian, Kevin wrote:
From: Baolu Lu
Sent: Thursday, May 12, 2022 11:03 AM
On 2022/5/11 22:53, Jason Gunthorpe wrote:
Also, given the current arrangement it might make sense to have a
struct iommu_domain_sva given that no driver is wrappering this in
something else.
Fair enough. How about below wrapper?
+struct iommu_sva_domain {
+ /*
+* Common iommu domain header,*must* be put at the top
+* of the structure.
+*/
+ struct iommu_domain domain;
+ struct mm_struct *mm;
+ struct iommu_sva bond;
+}
The refcount is wrapped in bond.
I'm still not sure that bond is necessary
"bond" is the sva handle that the device drivers get through calling
iommu_sva_bind().
'bond' was required before because we didn't have a domain to wrap
the page table at that time.
Now we have a domain and it is 1:1 associated to bond. Probably
make sense now by just returning the domain as the sva handle
instead?
It also includes the device information that the domain has been
attached. So the sva_unbind() looks like this:
/**
* iommu_sva_unbind_device() - Remove a bond created with
iommu_sva_bind_device
* @handle: the handle returned by iommu_sva_bind_device()
*
* Put reference to a bond between device and address space. The device
should
* not be issuing any more transaction for this PASID. All outstanding page
* requests for this PASID must have been flushed to the IOMMU.
*/
void iommu_sva_unbind_device(struct iommu_sva *handle)
It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
if we can include the device in the unbind() interface.
Why would we have a special unbind for SVA?
It's about SVA kAPI for device drivers. The existing kAPIs include:
struct iommu_sva *iommu_sva_bind_device(struct device *dev,
struct mm_struct *mm,
void *drvdata);
void iommu_sva_unbind_device(struct iommu_sva *handle);
u32 iommu_sva_get_pasid(struct iommu_sva *handle);
This is not what we agreed the API should be. We agreed:
iommu_sva_domain_alloc()
iommu_attach_device_pasid()
iommu_detach_device_pasid()
Again, SVA should not be different from normal domain stuff.
Yes, agreed.
I am trying to achieve this in two steps. This first step focuses on
internal iommu implementation and keep the driver kAPI untouched. Then,
the second step focus on the driver APIs.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On 2022/5/12 19:51, Jason Gunthorpe wrote:
On Thu, May 12, 2022 at 11:02:39AM +0800, Baolu Lu wrote:
+ mutex_lock(&group->mutex);
+ domain = xa_load(&group->pasid_array, pasid);
+ if (domain && domain->type != type)
+ domain = NULL;
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
+
+ return domain;
This is bad locking, group->pasid_array values cannot be taken outside
the lock.
It's not iommu core, but SVA (or other feature components) that manage
the life cycle of a domain. The iommu core only provides a place to
store the domain pointer. The feature components are free to fetch their
domain pointers from iommu core as long as they are sure that the domain
is alive during use.
I'm not convinced.
I'm sorry, I may not have explained it clearly. :-)
This helper is safe for uses inside the IOMMU subsystem. We could trust
ourselves that nobody will abuse this helper to obtain domains belonging
to others as the pasid has been allocated for SVA code. No other code
should be able to setup another type of domain for this pasid. The SVA
code has its own lock mechanism to avoid using after free.
Please correct me if I missed anything. :-) By the way, I can see some
similar helpers in current IOMMU core. For example,
struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
{
struct iommu_domain *domain;
struct iommu_group *group;
group = iommu_group_get(dev);
if (!group)
return NULL;
domain = group->domain;
iommu_group_put(group);
return domain;
}
EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On Thu, May 12, 2022 at 07:59:41PM +0800, Baolu Lu wrote:
> On 2022/5/12 19:48, Jason Gunthorpe wrote:
> > On Thu, May 12, 2022 at 01:17:08PM +0800, Baolu Lu wrote:
> > > On 2022/5/12 13:01, Tian, Kevin wrote:
> > > > > From: Baolu Lu
> > > > > Sent: Thursday, May 12, 2022 11:03 AM
> > > > >
> > > > > On 2022/5/11 22:53, Jason Gunthorpe wrote:
> > > > > > > > Also, given the current arrangement it might make sense to have
> > > > > > > > a
> > > > > > > > struct iommu_domain_sva given that no driver is wrappering this
> > > > > > > > in
> > > > > > > > something else.
> > > > > > > Fair enough. How about below wrapper?
> > > > > > >
> > > > > > > +struct iommu_sva_domain {
> > > > > > > + /*
> > > > > > > +* Common iommu domain header,*must* be put at the top
> > > > > > > +* of the structure.
> > > > > > > +*/
> > > > > > > + struct iommu_domain domain;
> > > > > > > + struct mm_struct *mm;
> > > > > > > + struct iommu_sva bond;
> > > > > > > +}
> > > > > > >
> > > > > > > The refcount is wrapped in bond.
> > > > > > I'm still not sure that bond is necessary
> > > > >
> > > > > "bond" is the sva handle that the device drivers get through calling
> > > > > iommu_sva_bind().
> > > > >
> > > >
> > > > 'bond' was required before because we didn't have a domain to wrap
> > > > the page table at that time.
> > > >
> > > > Now we have a domain and it is 1:1 associated to bond. Probably
> > > > make sense now by just returning the domain as the sva handle
> > > > instead?
> > >
> > > It also includes the device information that the domain has been
> > > attached. So the sva_unbind() looks like this:
> > >
> > > /**
> > > * iommu_sva_unbind_device() - Remove a bond created with
> > > iommu_sva_bind_device
> > > * @handle: the handle returned by iommu_sva_bind_device()
> > > *
> > > * Put reference to a bond between device and address space. The device
> > > should
> > > * not be issuing any more transaction for this PASID. All outstanding
> > > page
> > > * requests for this PASID must have been flushed to the IOMMU.
> > > */
> > > void iommu_sva_unbind_device(struct iommu_sva *handle)
> > >
> > > It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
> > > if we can include the device in the unbind() interface.
> >
> > Why would we have a special unbind for SVA?
>
> It's about SVA kAPI for device drivers. The existing kAPIs include:
>
> struct iommu_sva *iommu_sva_bind_device(struct device *dev,
> struct mm_struct *mm,
> void *drvdata);
> void iommu_sva_unbind_device(struct iommu_sva *handle);
> u32 iommu_sva_get_pasid(struct iommu_sva *handle);
This is not what we agreed the API should be. We agreed:
iommu_sva_domain_alloc()
iommu_attach_device_pasid()
iommu_detach_device_pasid()
Again, SVA should not be different from normal domain stuff.
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On 2022/5/12 19:48, Jason Gunthorpe wrote:
On Thu, May 12, 2022 at 01:17:08PM +0800, Baolu Lu wrote:
On 2022/5/12 13:01, Tian, Kevin wrote:
From: Baolu Lu
Sent: Thursday, May 12, 2022 11:03 AM
On 2022/5/11 22:53, Jason Gunthorpe wrote:
Also, given the current arrangement it might make sense to have a
struct iommu_domain_sva given that no driver is wrappering this in
something else.
Fair enough. How about below wrapper?
+struct iommu_sva_domain {
+ /*
+* Common iommu domain header,*must* be put at the top
+* of the structure.
+*/
+ struct iommu_domain domain;
+ struct mm_struct *mm;
+ struct iommu_sva bond;
+}
The refcount is wrapped in bond.
I'm still not sure that bond is necessary
"bond" is the sva handle that the device drivers get through calling
iommu_sva_bind().
'bond' was required before because we didn't have a domain to wrap
the page table at that time.
Now we have a domain and it is 1:1 associated to bond. Probably
make sense now by just returning the domain as the sva handle
instead?
It also includes the device information that the domain has been
attached. So the sva_unbind() looks like this:
/**
* iommu_sva_unbind_device() - Remove a bond created with
iommu_sva_bind_device
* @handle: the handle returned by iommu_sva_bind_device()
*
* Put reference to a bond between device and address space. The device
should
* not be issuing any more transaction for this PASID. All outstanding page
* requests for this PASID must have been flushed to the IOMMU.
*/
void iommu_sva_unbind_device(struct iommu_sva *handle)
It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
if we can include the device in the unbind() interface.
Why would we have a special unbind for SVA?
It's about SVA kAPI for device drivers. The existing kAPIs include:
struct iommu_sva *iommu_sva_bind_device(struct device *dev,
struct mm_struct *mm,
void *drvdata);
void iommu_sva_unbind_device(struct iommu_sva *handle);
u32 iommu_sva_get_pasid(struct iommu_sva *handle);
SVA should not different from normal domains it should use the normal
detach flow too.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On Thu, May 12, 2022 at 11:02:39AM +0800, Baolu Lu wrote: > > > + mutex_lock(&group->mutex); > > > + domain = xa_load(&group->pasid_array, pasid); > > > + if (domain && domain->type != type) > > > + domain = NULL; > > > + mutex_unlock(&group->mutex); > > > + iommu_group_put(group); > > > + > > > + return domain; > > This is bad locking, group->pasid_array values cannot be taken outside > > the lock. > > It's not iommu core, but SVA (or other feature components) that manage > the life cycle of a domain. The iommu core only provides a place to > store the domain pointer. The feature components are free to fetch their > domain pointers from iommu core as long as they are sure that the domain > is alive during use. I'm not convinced. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On Thu, May 12, 2022 at 01:17:08PM +0800, Baolu Lu wrote:
> On 2022/5/12 13:01, Tian, Kevin wrote:
> > > From: Baolu Lu
> > > Sent: Thursday, May 12, 2022 11:03 AM
> > >
> > > On 2022/5/11 22:53, Jason Gunthorpe wrote:
> > > > > > Also, given the current arrangement it might make sense to have a
> > > > > > struct iommu_domain_sva given that no driver is wrappering this in
> > > > > > something else.
> > > > > Fair enough. How about below wrapper?
> > > > >
> > > > > +struct iommu_sva_domain {
> > > > > + /*
> > > > > +* Common iommu domain header,*must* be put at the top
> > > > > +* of the structure.
> > > > > +*/
> > > > > + struct iommu_domain domain;
> > > > > + struct mm_struct *mm;
> > > > > + struct iommu_sva bond;
> > > > > +}
> > > > >
> > > > > The refcount is wrapped in bond.
> > > > I'm still not sure that bond is necessary
> > >
> > > "bond" is the sva handle that the device drivers get through calling
> > > iommu_sva_bind().
> > >
> >
> > 'bond' was required before because we didn't have a domain to wrap
> > the page table at that time.
> >
> > Now we have a domain and it is 1:1 associated to bond. Probably
> > make sense now by just returning the domain as the sva handle
> > instead?
>
> It also includes the device information that the domain has been
> attached. So the sva_unbind() looks like this:
>
> /**
> * iommu_sva_unbind_device() - Remove a bond created with
> iommu_sva_bind_device
> * @handle: the handle returned by iommu_sva_bind_device()
> *
> * Put reference to a bond between device and address space. The device
> should
> * not be issuing any more transaction for this PASID. All outstanding page
> * requests for this PASID must have been flushed to the IOMMU.
> */
> void iommu_sva_unbind_device(struct iommu_sva *handle)
>
> It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
> if we can include the device in the unbind() interface.
Why would we have a special unbind for SVA?
SVA should not different from normal domains it should use the normal
detach flow too.
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On 2022/5/12 13:44, Tian, Kevin wrote:
From: Baolu Lu
Sent: Thursday, May 12, 2022 1:17 PM
On 2022/5/12 13:01, Tian, Kevin wrote:
From: Baolu Lu
Sent: Thursday, May 12, 2022 11:03 AM
On 2022/5/11 22:53, Jason Gunthorpe wrote:
Also, given the current arrangement it might make sense to have a
struct iommu_domain_sva given that no driver is wrappering this in
something else.
Fair enough. How about below wrapper?
+struct iommu_sva_domain {
+ /*
+* Common iommu domain header,*must* be put at the top
+* of the structure.
+*/
+ struct iommu_domain domain;
+ struct mm_struct *mm;
+ struct iommu_sva bond;
+}
The refcount is wrapped in bond.
I'm still not sure that bond is necessary
"bond" is the sva handle that the device drivers get through calling
iommu_sva_bind().
'bond' was required before because we didn't have a domain to wrap
the page table at that time.
Now we have a domain and it is 1:1 associated to bond. Probably
make sense now by just returning the domain as the sva handle
instead?
It also includes the device information that the domain has been
attached. So the sva_unbind() looks like this:
/**
* iommu_sva_unbind_device() - Remove a bond created with
iommu_sva_bind_device
* @handle: the handle returned by iommu_sva_bind_device()
*
* Put reference to a bond between device and address space. The device
should
* not be issuing any more transaction for this PASID. All outstanding page
* requests for this PASID must have been flushed to the IOMMU.
*/
void iommu_sva_unbind_device(struct iommu_sva *handle)
It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
if we can include the device in the unbind() interface.
can we just have unbind(domain, device)?
Yes. With this, we can remove bond.
This could be done in below phase 2.
Anyway, I'd expect to achieve all these in two steps:
- sva and iopf refactoring, only iommu internal changes;
- sva interface refactoring, only interface changes.
Does above work?
Best regards,
baolu
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
> From: Baolu Lu
> Sent: Thursday, May 12, 2022 1:17 PM
>
> On 2022/5/12 13:01, Tian, Kevin wrote:
> >> From: Baolu Lu
> >> Sent: Thursday, May 12, 2022 11:03 AM
> >>
> >> On 2022/5/11 22:53, Jason Gunthorpe wrote:
> > Also, given the current arrangement it might make sense to have a
> > struct iommu_domain_sva given that no driver is wrappering this in
> > something else.
> Fair enough. How about below wrapper?
>
> +struct iommu_sva_domain {
> + /*
> +* Common iommu domain header,*must* be put at the top
> +* of the structure.
> +*/
> + struct iommu_domain domain;
> + struct mm_struct *mm;
> + struct iommu_sva bond;
> +}
>
> The refcount is wrapped in bond.
> >>> I'm still not sure that bond is necessary
> >>
> >> "bond" is the sva handle that the device drivers get through calling
> >> iommu_sva_bind().
> >>
> >
> > 'bond' was required before because we didn't have a domain to wrap
> > the page table at that time.
> >
> > Now we have a domain and it is 1:1 associated to bond. Probably
> > make sense now by just returning the domain as the sva handle
> > instead?
>
> It also includes the device information that the domain has been
> attached. So the sva_unbind() looks like this:
>
> /**
> * iommu_sva_unbind_device() - Remove a bond created with
> iommu_sva_bind_device
> * @handle: the handle returned by iommu_sva_bind_device()
> *
> * Put reference to a bond between device and address space. The device
> should
> * not be issuing any more transaction for this PASID. All outstanding page
> * requests for this PASID must have been flushed to the IOMMU.
> */
> void iommu_sva_unbind_device(struct iommu_sva *handle)
>
> It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
> if we can include the device in the unbind() interface.
can we just have unbind(domain, device)?
>
> Anyway, I'd expect to achieve all these in two steps:
>
> - sva and iopf refactoring, only iommu internal changes;
> - sva interface refactoring, only interface changes.
>
> Does above work?
>
> Best regards,
> baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On 2022/5/12 13:01, Tian, Kevin wrote:
From: Baolu Lu
Sent: Thursday, May 12, 2022 11:03 AM
On 2022/5/11 22:53, Jason Gunthorpe wrote:
Also, given the current arrangement it might make sense to have a
struct iommu_domain_sva given that no driver is wrappering this in
something else.
Fair enough. How about below wrapper?
+struct iommu_sva_domain {
+ /*
+* Common iommu domain header,*must* be put at the top
+* of the structure.
+*/
+ struct iommu_domain domain;
+ struct mm_struct *mm;
+ struct iommu_sva bond;
+}
The refcount is wrapped in bond.
I'm still not sure that bond is necessary
"bond" is the sva handle that the device drivers get through calling
iommu_sva_bind().
'bond' was required before because we didn't have a domain to wrap
the page table at that time.
Now we have a domain and it is 1:1 associated to bond. Probably
make sense now by just returning the domain as the sva handle
instead?
It also includes the device information that the domain has been
attached. So the sva_unbind() looks like this:
/**
* iommu_sva_unbind_device() - Remove a bond created with
iommu_sva_bind_device
* @handle: the handle returned by iommu_sva_bind_device()
*
* Put reference to a bond between device and address space. The device
should
* not be issuing any more transaction for this PASID. All outstanding page
* requests for this PASID must have been flushed to the IOMMU.
*/
void iommu_sva_unbind_device(struct iommu_sva *handle)
It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
if we can include the device in the unbind() interface.
Anyway, I'd expect to achieve all these in two steps:
- sva and iopf refactoring, only iommu internal changes;
- sva interface refactoring, only interface changes.
Does above work?
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
> From: Baolu Lu
> Sent: Thursday, May 12, 2022 11:03 AM
>
> On 2022/5/11 22:53, Jason Gunthorpe wrote:
> >>> Also, given the current arrangement it might make sense to have a
> >>> struct iommu_domain_sva given that no driver is wrappering this in
> >>> something else.
> >> Fair enough. How about below wrapper?
> >>
> >> +struct iommu_sva_domain {
> >> + /*
> >> +* Common iommu domain header,*must* be put at the top
> >> +* of the structure.
> >> +*/
> >> + struct iommu_domain domain;
> >> + struct mm_struct *mm;
> >> + struct iommu_sva bond;
> >> +}
> >>
> >> The refcount is wrapped in bond.
> > I'm still not sure that bond is necessary
>
> "bond" is the sva handle that the device drivers get through calling
> iommu_sva_bind().
>
'bond' was required before because we didn't have a domain to wrap
the page table at that time.
Now we have a domain and it is 1:1 associated to bond. Probably
make sense now by just returning the domain as the sva handle
instead?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On 2022/5/11 22:53, Jason Gunthorpe wrote:
Assuming we leave room for multi-device groups this logic should just
be
group = iommu_group_get(dev);
if (!group)
return -ENODEV;
mutex_lock(&group->mutex);
domain = xa_load(&group->pasid_array, mm->pasid);
if (!domain || domain->type != IOMMU_DOMAIN_SVA || domain->mm != mm)
domain = iommu_sva_alloc_domain(dev, mm);
?
Agreed. As a helper in iommu core, how about making it more generic like
below?
IDK, is there more users of this? AFAIK SVA is the only place that
will be auto-sharing?
The generic thing is that components, like SVA, want to fetch the
attached domain from the iommu core.
+ mutex_lock(&group->mutex);
+ domain = xa_load(&group->pasid_array, pasid);
+ if (domain && domain->type != type)
+ domain = NULL;
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
+
+ return domain;
This is bad locking, group->pasid_array values cannot be taken outside
the lock.
It's not iommu core, but SVA (or other feature components) that manage
the life cycle of a domain. The iommu core only provides a place to
store the domain pointer. The feature components are free to fetch their
domain pointers from iommu core as long as they are sure that the domain
is alive during use.
And stick the refcount in the sva_domain
Also, given the current arrangement it might make sense to have a
struct iommu_domain_sva given that no driver is wrappering this in
something else.
Fair enough. How about below wrapper?
+struct iommu_sva_domain {
+ /*
+* Common iommu domain header,*must* be put at the top
+* of the structure.
+*/
+ struct iommu_domain domain;
+ struct mm_struct *mm;
+ struct iommu_sva bond;
+}
The refcount is wrapped in bond.
I'm still not sure that bond is necessary
"bond" is the sva handle that the device drivers get through calling
iommu_sva_bind().
But yes, something like that
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On Wed, May 11, 2022 at 03:21:31PM +0800, Baolu Lu wrote:
> On 2022/5/10 23:23, Jason Gunthorpe wrote:
> > On Tue, May 10, 2022 at 02:17:34PM +0800, Lu Baolu wrote:
> >
> > > +/**
> > > + * iommu_sva_bind_device() - Bind a process address space to a device
> > > + * @dev: the device
> > > + * @mm: the mm to bind, caller must hold a reference to mm_users
> > > + * @drvdata: opaque data pointer to pass to bind callback
> > > + *
> > > + * Create a bond between device and address space, allowing the device
> > > to access
> > > + * the mm using the returned PASID. If a bond already exists between
> > > @device and
> > > + * @mm, it is returned and an additional reference is taken. Caller must
> > > call
> > > + * iommu_sva_unbind_device() to release each reference.
> > > + *
> > > + * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called
> > > first, to
> > > + * initialize the required SVA features.
> > > + *
> > > + * On error, returns an ERR_PTR value.
> > > + */
> > > +struct iommu_sva *
> > > +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
> > > *drvdata)
> > > +{
> > > + int ret = -EINVAL;
> > > + struct iommu_sva *handle;
> > > + struct iommu_domain *domain;
> > > +
> > > + /*
> > > + * TODO: Remove the drvdata parameter after kernel PASID support is
> > > + * enabled for the idxd driver.
> > > + */
> > > + if (drvdata)
> > > + return ERR_PTR(-EOPNOTSUPP);
> >
> > Why is this being left behind? Clean up the callers too please.
>
> Okay, let me try to.
>
> >
> > > + /* Allocate mm->pasid if necessary. */
> > > + ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits) - 1);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> > > +
> > > + mutex_lock(&iommu_sva_lock);
> > > + /* Search for an existing bond. */
> > > + handle = xa_load(&dev->iommu->sva_bonds, mm->pasid);
> > > + if (handle) {
> > > + refcount_inc(&handle->users);
> > > + goto out_success;
> > > + }
> >
> > How can there be an existing bond?
> >
> > dev->iommu is per-device
> >
> > The device_group_immutable_singleton() insists on a single device
> > group
> >
> > Basically 'sva_bonds' is the same thing as the group->pasid_array.
>
> Yes, really.
>
> >
> > Assuming we leave room for multi-device groups this logic should just
> > be
> >
> > group = iommu_group_get(dev);
> > if (!group)
> > return -ENODEV;
> >
> > mutex_lock(&group->mutex);
> > domain = xa_load(&group->pasid_array, mm->pasid);
> > if (!domain || domain->type != IOMMU_DOMAIN_SVA || domain->mm != mm)
> > domain = iommu_sva_alloc_domain(dev, mm);
> >
> > ?
>
> Agreed. As a helper in iommu core, how about making it more generic like
> below?
IDK, is there more users of this? AFAIK SVA is the only place that
will be auto-sharing?
> + mutex_lock(&group->mutex);
> + domain = xa_load(&group->pasid_array, pasid);
> + if (domain && domain->type != type)
> + domain = NULL;
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
> +
> + return domain;
This is bad locking, group->pasid_array values cannot be taken outside
the lock.
> > And stick the refcount in the sva_domain
> >
> > Also, given the current arrangement it might make sense to have a
> > struct iommu_domain_sva given that no driver is wrappering this in
> > something else.
>
> Fair enough. How about below wrapper?
>
> +struct iommu_sva_domain {
> + /*
> +* Common iommu domain header, *must* be put at the top
> +* of the structure.
> +*/
> + struct iommu_domain domain;
> + struct mm_struct *mm;
> + struct iommu_sva bond;
> +}
>
> The refcount is wrapped in bond.
I'm still not sure that bond is necessary
But yes, something like that
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On 2022/5/10 23:23, Jason Gunthorpe wrote:
On Tue, May 10, 2022 at 02:17:34PM +0800, Lu Baolu wrote:
+/**
+ * iommu_sva_bind_device() - Bind a process address space to a device
+ * @dev: the device
+ * @mm: the mm to bind, caller must hold a reference to mm_users
+ * @drvdata: opaque data pointer to pass to bind callback
+ *
+ * Create a bond between device and address space, allowing the device to
access
+ * the mm using the returned PASID. If a bond already exists between @device
and
+ * @mm, it is returned and an additional reference is taken. Caller must call
+ * iommu_sva_unbind_device() to release each reference.
+ *
+ * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
+ * initialize the required SVA features.
+ *
+ * On error, returns an ERR_PTR value.
+ */
+struct iommu_sva *
+iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
+{
+ int ret = -EINVAL;
+ struct iommu_sva *handle;
+ struct iommu_domain *domain;
+
+ /*
+* TODO: Remove the drvdata parameter after kernel PASID support is
+* enabled for the idxd driver.
+*/
+ if (drvdata)
+ return ERR_PTR(-EOPNOTSUPP);
Why is this being left behind? Clean up the callers too please.
Okay, let me try to.
+ /* Allocate mm->pasid if necessary. */
+ ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits) - 1);
+ if (ret)
+ return ERR_PTR(ret);
+
+ mutex_lock(&iommu_sva_lock);
+ /* Search for an existing bond. */
+ handle = xa_load(&dev->iommu->sva_bonds, mm->pasid);
+ if (handle) {
+ refcount_inc(&handle->users);
+ goto out_success;
+ }
How can there be an existing bond?
dev->iommu is per-device
The device_group_immutable_singleton() insists on a single device
group
Basically 'sva_bonds' is the same thing as the group->pasid_array.
Yes, really.
Assuming we leave room for multi-device groups this logic should just
be
group = iommu_group_get(dev);
if (!group)
return -ENODEV;
mutex_lock(&group->mutex);
domain = xa_load(&group->pasid_array, mm->pasid);
if (!domain || domain->type != IOMMU_DOMAIN_SVA || domain->mm != mm)
domain = iommu_sva_alloc_domain(dev, mm);
?
Agreed. As a helper in iommu core, how about making it more generic like
below?
+struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
+ iosid_t pasid,
+ unsigned int type)
+{
+ struct iommu_domain *domain;
+ struct iommu_group *group;
+
+ if (!pasid_valid(pasid))
+ return NULL;
+
+ group = iommu_group_get(dev);
+ if (!group)
+ return NULL;
+
+ mutex_lock(&group->mutex);
+ domain = xa_load(&group->pasid_array, pasid);
+ if (domain && domain->type != type)
+ domain = NULL;
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
+
+ return domain;
+}
And stick the refcount in the sva_domain
Also, given the current arrangement it might make sense to have a
struct iommu_domain_sva given that no driver is wrappering this in
something else.
Fair enough. How about below wrapper?
+struct iommu_sva_domain {
+ /*
+* Common iommu domain header, *must* be put at the top
+* of the structure.
+*/
+ struct iommu_domain domain;
+ struct mm_struct *mm;
+ struct iommu_sva bond;
+}
The refcount is wrapped in bond.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On Tue, May 10, 2022 at 02:17:34PM +0800, Lu Baolu wrote:
> +/**
> + * iommu_sva_bind_device() - Bind a process address space to a device
> + * @dev: the device
> + * @mm: the mm to bind, caller must hold a reference to mm_users
> + * @drvdata: opaque data pointer to pass to bind callback
> + *
> + * Create a bond between device and address space, allowing the device to
> access
> + * the mm using the returned PASID. If a bond already exists between @device
> and
> + * @mm, it is returned and an additional reference is taken. Caller must call
> + * iommu_sva_unbind_device() to release each reference.
> + *
> + * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
> + * initialize the required SVA features.
> + *
> + * On error, returns an ERR_PTR value.
> + */
> +struct iommu_sva *
> +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
> *drvdata)
> +{
> + int ret = -EINVAL;
> + struct iommu_sva *handle;
> + struct iommu_domain *domain;
> +
> + /*
> + * TODO: Remove the drvdata parameter after kernel PASID support is
> + * enabled for the idxd driver.
> + */
> + if (drvdata)
> + return ERR_PTR(-EOPNOTSUPP);
Why is this being left behind? Clean up the callers too please.
> + /* Allocate mm->pasid if necessary. */
> + ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits) - 1);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + mutex_lock(&iommu_sva_lock);
> + /* Search for an existing bond. */
> + handle = xa_load(&dev->iommu->sva_bonds, mm->pasid);
> + if (handle) {
> + refcount_inc(&handle->users);
> + goto out_success;
> + }
How can there be an existing bond?
dev->iommu is per-device
The device_group_immutable_singleton() insists on a single device
group
Basically 'sva_bonds' is the same thing as the group->pasid_array.
Assuming we leave room for multi-device groups this logic should just
be
group = iommu_group_get(dev);
if (!group)
return -ENODEV;
mutex_lock(&group->mutex);
domain = xa_load(&group->pasid_array, mm->pasid);
if (!domain || domain->type != IOMMU_DOMAIN_SVA || domain->mm != mm)
domain = iommu_sva_alloc_domain(dev, mm);
?
And stick the refcount in the sva_domain
Also, given the current arrangement it might make sense to have a
struct iommu_domain_sva given that no driver is wrappering this in
something else.
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
The existing iommu SVA interfaces are implemented by calling the SVA
specific iommu ops provided by the IOMMU drivers. There's no need for
any SVA specific ops in iommu_ops vector anymore as we can achieve
this through the generic attach/detach_dev_pasid domain ops.
This refactors the IOMMU SVA interfaces implementation by using the
attach/detach_pasid_dev ops and align them with the concept of the
iommu domain. Put the new SVA code in the sva related file in order
to make it self-contained.
Signed-off-by: Lu Baolu
---
include/linux/iommu.h | 44 ++-
drivers/iommu/iommu-sva-lib.c | 145 ++
drivers/iommu/iommu.c | 92 -
3 files changed, 168 insertions(+), 113 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2921e634491e..5a3ef4d58b1f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -684,12 +684,6 @@ int iommu_dev_enable_feature(struct device *dev, enum
iommu_dev_features f);
int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features f);
-struct iommu_sva *iommu_sva_bind_device(struct device *dev,
- struct mm_struct *mm,
- void *drvdata);
-void iommu_sva_unbind_device(struct iommu_sva *handle);
-u32 iommu_sva_get_pasid(struct iommu_sva *handle);
-
int iommu_device_use_default_domain(struct device *dev);
void iommu_device_unuse_default_domain(struct device *dev);
@@ -1031,21 +1025,6 @@ iommu_dev_disable_feature(struct device *dev, enum
iommu_dev_features feat)
return -ENODEV;
}
-static inline struct iommu_sva *
-iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- return NULL;
-}
-
-static inline void iommu_sva_unbind_device(struct iommu_sva *handle)
-{
-}
-
-static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
-{
- return IOMMU_PASID_INVALID;
-}
-
static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
{
return NULL;
@@ -1087,6 +1066,29 @@ static inline void iommu_detach_device_pasid(struct
iommu_domain *domain,
}
#endif /* CONFIG_IOMMU_API */
+#ifdef CONFIG_IOMMU_SVA
+struct iommu_sva *iommu_sva_bind_device(struct device *dev,
+ struct mm_struct *mm,
+ void *drvdata);
+void iommu_sva_unbind_device(struct iommu_sva *handle);
+u32 iommu_sva_get_pasid(struct iommu_sva *handle);
+#else /* CONFIG_IOMMU_SVA */
+static inline struct iommu_sva *
+iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
+{
+ return NULL;
+}
+
+static inline void iommu_sva_unbind_device(struct iommu_sva *handle)
+{
+}
+
+static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
+{
+ return IOMMU_PASID_INVALID;
+}
+#endif /* CONFIG_IOMMU_SVA */
+
/**
* iommu_map_sgtable - Map the given buffer to the IOMMU domain
* @domain:The IOMMU domain to perform the mapping
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index 106506143896..e7301514f286 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -3,6 +3,8 @@
* Helpers for IOMMU drivers implementing SVA
*/
#include
+#include
+#include
#include
#include "iommu-sva-lib.h"
@@ -69,3 +71,146 @@ struct mm_struct *iommu_sva_find(ioasid_t pasid)
return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
}
EXPORT_SYMBOL_GPL(iommu_sva_find);
+
+/*
+ * IOMMU SVA driver-oriented interfaces
+ */
+static struct iommu_domain *
+iommu_sva_alloc_domain(struct device *dev, struct mm_struct *mm)
+{
+ struct bus_type *bus = dev->bus;
+ struct iommu_domain *domain;
+
+ if (!bus || !bus->iommu_ops)
+ return NULL;
+
+ domain = bus->iommu_ops->domain_alloc(IOMMU_DOMAIN_SVA);
+ if (!domain)
+ return NULL;
+
+ mmgrab(mm);
+ domain->mm = mm;
+ domain->type = IOMMU_DOMAIN_SVA;
+
+ return domain;
+}
+
+static void iommu_sva_free_domain(struct iommu_domain *domain)
+{
+ mmdrop(domain->mm);
+ iommu_domain_free(domain);
+}
+
+/**
+ * iommu_sva_bind_device() - Bind a process address space to a device
+ * @dev: the device
+ * @mm: the mm to bind, caller must hold a reference to mm_users
+ * @drvdata: opaque data pointer to pass to bind callback
+ *
+ * Create a bond between device and address space, allowing the device to
access
+ * the mm using the returned PASID. If a bond already exists between @device
and
+ * @mm, it is returned and an additional reference is taken. Caller must call
+ * iommu_sva_unbind_device() to release each reference.
+ *
+ * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
+ * initialize the required SVA features.
+ *
+ * On error, returns an ERR_PTR value.
+ */
+struct i
