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
iommu_attach/detach_device_pasid interfaces and align them with the
concept of the SVA iommu domain. Put the new SVA code in the SVA
related file in order to make it self-contained.
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jean-Philippe Brucker <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Tested-by: Zhangfei Gao <[email protected]>
Tested-by: Tony Zhu <[email protected]>
---
include/linux/iommu.h | 43 ++++++-------
drivers/iommu/iommu-sva-lib.c | 110 ++++++++++++++++++++++++++++++++++
drivers/iommu/iommu.c | 91 ----------------------------
3 files changed, 133 insertions(+), 111 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index c45236d95355..f8427e5e0bff 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -647,6 +647,7 @@ struct iommu_fwspec {
*/
struct iommu_sva {
struct device *dev;
+ struct iommu_domain *domain;
};
int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
@@ -688,11 +689,6 @@ void iommu_release_device(struct device *dev);
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);
-struct iommu_sva *iommu_sva_bind_device(struct device *dev,
- struct mm_struct *mm);
-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);
@@ -1033,21 +1029,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)
-{
- 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;
@@ -1125,4 +1106,26 @@ void iommu_debugfs_setup(void);
static inline void iommu_debugfs_setup(void) {}
#endif
+#ifdef CONFIG_IOMMU_SVA
+struct iommu_sva *iommu_sva_bind_device(struct device *dev,
+ struct mm_struct *mm);
+void iommu_sva_unbind_device(struct iommu_sva *handle);
+u32 iommu_sva_get_pasid(struct iommu_sva *handle);
+#else
+static inline struct iommu_sva *
+iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
+{
+ 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 */
+
#endif /* __LINUX_IOMMU_H */
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index 106506143896..197f68602439 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -4,6 +4,7 @@
*/
#include <linux/mutex.h>
#include <linux/sched/mm.h>
+#include <linux/iommu.h>
#include "iommu-sva-lib.h"
@@ -69,3 +70,112 @@ 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_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
+ *
+ * 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)
+{
+ struct iommu_domain *domain;
+ struct iommu_sva *handle;
+ ioasid_t max_pasids;
+ int ret;
+
+ max_pasids = dev->iommu->max_pasids;
+ if (!max_pasids)
+ return ERR_PTR(-EOPNOTSUPP);
+
+ /* Allocate mm->pasid if necessary. */
+ ret = iommu_sva_alloc_pasid(mm, 1, max_pasids - 1);
+ if (ret)
+ return ERR_PTR(ret);
+
+ handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+ if (!handle)
+ return ERR_PTR(-ENOMEM);
+
+ mutex_lock(&iommu_sva_lock);
+ /* Search for an existing domain. */
+ domain = iommu_get_domain_for_dev_pasid(dev, mm->pasid,
+ IOMMU_DOMAIN_SVA);
+ if (IS_ERR(domain)) {
+ ret = PTR_ERR(domain);
+ goto out_unlock;
+ }
+
+ if (domain) {
+ domain->users++;
+ goto out;
+ }
+
+ /* Allocate a new domain and set it on device pasid. */
+ domain = iommu_sva_domain_alloc(dev, mm);
+ if (!domain) {
+ ret = -ENOMEM;
+ goto out_unlock;
+ }
+
+ ret = iommu_attach_device_pasid(domain, dev, mm->pasid);
+ if (ret)
+ goto out_free_domain;
+ domain->users = 1;
+out:
+ mutex_unlock(&iommu_sva_lock);
+ handle->dev = dev;
+ handle->domain = domain;
+
+ return handle;
+
+out_free_domain:
+ iommu_domain_free(domain);
+out_unlock:
+ mutex_unlock(&iommu_sva_lock);
+ kfree(handle);
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
+
+/**
+ * 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)
+{
+ struct iommu_domain *domain = handle->domain;
+ ioasid_t pasid = domain->mm->pasid;
+ struct device *dev = handle->dev;
+
+ mutex_lock(&iommu_sva_lock);
+ if (--domain->users == 0) {
+ iommu_detach_device_pasid(domain, dev, pasid);
+ iommu_domain_free(domain);
+ }
+ mutex_unlock(&iommu_sva_lock);
+ kfree(handle);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
+
+u32 iommu_sva_get_pasid(struct iommu_sva *handle)
+{
+ struct iommu_domain *domain = handle->domain;
+
+ return domain->mm->pasid;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2d0a1688725a..bc6b1d65ba2e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2776,97 +2776,6 @@ int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
}
EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
-/**
- * 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 it
- *
- * 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)
-{
- struct iommu_group *group;
- struct iommu_sva *handle = ERR_PTR(-EINVAL);
- const struct iommu_ops *ops = dev_iommu_ops(dev);
-
- if (!ops->sva_bind)
- return ERR_PTR(-ENODEV);
-
- group = iommu_group_get(dev);
- if (!group)
- return ERR_PTR(-ENODEV);
-
- /* Ensure device count and domain don't change while we're binding */
- mutex_lock(&group->mutex);
-
- /*
- * To keep things simple, SVA currently doesn't support IOMMU groups
- * with more than one device. Existing SVA-capable systems are not
- * affected by the problems that required IOMMU groups (lack of ACS
- * isolation, device ID aliasing and other hardware issues).
- */
- if (iommu_group_device_count(group) != 1)
- goto out_unlock;
-
- handle = ops->sva_bind(dev, mm);
-
-out_unlock:
- mutex_unlock(&group->mutex);
- iommu_group_put(group);
-
- return handle;
-}
-EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
-
-/**
- * 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)
-{
- struct iommu_group *group;
- struct device *dev = handle->dev;
- const struct iommu_ops *ops = dev_iommu_ops(dev);
-
- if (!ops->sva_unbind)
- return;
-
- group = iommu_group_get(dev);
- if (!group)
- return;
-
- mutex_lock(&group->mutex);
- ops->sva_unbind(handle);
- mutex_unlock(&group->mutex);
-
- iommu_group_put(group);
-}
-EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
-
-u32 iommu_sva_get_pasid(struct iommu_sva *handle)
-{
- const struct iommu_ops *ops = dev_iommu_ops(handle->dev);
-
- if (!ops->sva_get_pasid)
- return IOMMU_PASID_INVALID;
-
- return ops->sva_get_pasid(handle);
-}
-EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
-
/*
* Changes the default domain of an iommu group that has *only* one device
*
--
2.25.1
On Tue, Sep 06, 2022 at 08:44:54PM +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
> + *
> + * 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.
This isn't true anymore. How about storing handle in the domain?
(Maybe also drop my Reviewed-by tags since this has changed significantly,
I tend to ignore patches that have them)
Thanks,
Jean
> + *
> + * 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)
> +{
> + struct iommu_domain *domain;
> + struct iommu_sva *handle;
> + ioasid_t max_pasids;
> + int ret;
> +
> + max_pasids = dev->iommu->max_pasids;
> + if (!max_pasids)
> + return ERR_PTR(-EOPNOTSUPP);
> +
> + /* Allocate mm->pasid if necessary. */
> + ret = iommu_sva_alloc_pasid(mm, 1, max_pasids - 1);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> + if (!handle)
> + return ERR_PTR(-ENOMEM);
> +
> + mutex_lock(&iommu_sva_lock);
> + /* Search for an existing domain. */
> + domain = iommu_get_domain_for_dev_pasid(dev, mm->pasid,
> + IOMMU_DOMAIN_SVA);
> + if (IS_ERR(domain)) {
> + ret = PTR_ERR(domain);
> + goto out_unlock;
> + }
> +
> + if (domain) {
> + domain->users++;
> + goto out;
> + }
> +
> + /* Allocate a new domain and set it on device pasid. */
> + domain = iommu_sva_domain_alloc(dev, mm);
> + if (!domain) {
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + ret = iommu_attach_device_pasid(domain, dev, mm->pasid);
> + if (ret)
> + goto out_free_domain;
> + domain->users = 1;
> +out:
> + mutex_unlock(&iommu_sva_lock);
> + handle->dev = dev;
> + handle->domain = domain;
> +
> + return handle;
> +
> +out_free_domain:
> + iommu_domain_free(domain);
> +out_unlock:
> + mutex_unlock(&iommu_sva_lock);
> + kfree(handle);
> +
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
Hi Jean,
On 2022/9/7 0:36, Jean-Philippe Brucker wrote:
> On Tue, Sep 06, 2022 at 08:44:54PM +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
>> + *
>> + * 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.
> This isn't true anymore. How about storing handle in the domain?
Yes, agreed. How about making the comments like this:
/**
* 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
*
* Create a bond between device and address space, allowing the device to
* access the mm using the pasid returned by iommu_sva_get_pasid(). If a
* bond already exists between @device and @mm, an additional internal
* reference is taken. The reference will be released when the caller calls
* iommu_sva_unbind_device().
Storing the handle in the domain looks odd. Conceptually an iommu domain
represents a hardware page table and the SVA handle represents a
relationship between device and the page table for a consumer. It's
better to make them separated.
In a separated series, probably we can discuss the possibility of
removing handle from the driver APIs. Just simply return the sva domain
instead.
struct iommu_domain *iommu_sva_bind_device(struct device *dev,
struct mm_struct *mm);
void iommu_sva_unbind_device(struct device *dev,
struct iommu_domain *domain);
u32 iommu_sva_get_pasid(struct iommu_domain *domain);
If you think it's appropriate, I can send out the code for discussion.
>
> (Maybe also drop my Reviewed-by tags since this has changed significantly,
> I tend to ignore patches that have them)
I am sorry that after your review, the SVA domain and attach/detach
device pasid interfaces have undergone some changes. They mainly exist
in the following patches. Can you please help to take a look.
iommu/sva: Refactoring iommu_sva_bind/unbind_device()
arm-smmu-v3/sva: Add SVA domain support
iommu: Add IOMMU SVA domain support
iommu: Add attach/detach_dev_pasid iommu interfaces
Best regards,
baolu
Hi Baolu,
On Wed, Sep 07, 2022 at 09:27:30AM +0800, Baolu Lu wrote:
> Hi Jean,
>
> On 2022/9/7 0:36, Jean-Philippe Brucker wrote:
> > On Tue, Sep 06, 2022 at 08:44:54PM +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
> > > + *
> > > + * 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.
> > This isn't true anymore. How about storing handle in the domain?
>
> Yes, agreed. How about making the comments like this:
>
> /**
> * 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
> *
> * Create a bond between device and address space, allowing the device to
> * access the mm using the pasid returned by iommu_sva_get_pasid(). If a
> * bond already exists between @device and @mm, an additional internal
> * reference is taken. The reference will be released when the caller calls
> * iommu_sva_unbind_device().
Sure, that works. I'd keep "Caller must call iommu_sva_unbind_device()
to release each reference"
>
> Storing the handle in the domain looks odd. Conceptually an iommu domain
> represents a hardware page table and the SVA handle represents a
> relationship between device and the page table for a consumer. It's
> better to make them separated.
Right
>
> In a separated series, probably we can discuss the possibility of
> removing handle from the driver APIs. Just simply return the sva domain
> instead.
>
> struct iommu_domain *iommu_sva_bind_device(struct device *dev,
> struct mm_struct *mm);
> void iommu_sva_unbind_device(struct device *dev,
> struct iommu_domain *domain);
> u32 iommu_sva_get_pasid(struct iommu_domain *domain);
>
> If you think it's appropriate, I can send out the code for discussion.
Yes, I don't see a reason to keep struct iommu_sva at the moment.
I believe we needed to keep track of bonds themselves for sva_ops and
driver data but those are gone now.
Is iommu_domain still going to represent both a device context (whole
PASID table) and individual address spaces, or are you planning to move
away from that? What happens when a driver does:
d1 = iommu_domain_alloc()
iommu_attach_device(d1)
d2 = iommu_sva_bind_device()
iommu_detach_device(d1)
Does detach
(a) only disable the non-PASID address space?
(b) disable everything?
(c) fail because the driver didn't unbind first?
I'm asking because the SMMU driver is still using smmu_domain to represent
all address spaces + the non-PASID one, and using the same type
"iommu_domain" for the new object makes things unreadable. I think
internally we'll want to use distinct variable names, something like
"domain" and "address_space". If (a) is not the direction you're going,
then it may be worth renaming the API as well.
I'm also not sure why set_dev_pasid() is a domain_ops of the SVA domain,
but acts on the parent domain which contains the PASID table. Shouldn't it
be an IOMMU op like remove_dev_pasid(), or on the parent domain?
Thanks,
Jean
>
> >
> > (Maybe also drop my Reviewed-by tags since this has changed significantly,
> > I tend to ignore patches that have them)
>
> I am sorry that after your review, the SVA domain and attach/detach
> device pasid interfaces have undergone some changes. They mainly exist
> in the following patches. Can you please help to take a look.
>
> iommu/sva: Refactoring iommu_sva_bind/unbind_device()
> arm-smmu-v3/sva: Add SVA domain support
> iommu: Add IOMMU SVA domain support
> iommu: Add attach/detach_dev_pasid iommu interfaces
>
> Best regards,
> baolu
On Wed, Sep 07, 2022 at 10:54:54AM +0100, Jean-Philippe Brucker wrote:
> Is iommu_domain still going to represent both a device context (whole
> PASID table) and individual address spaces, or are you planning to move
> away from that? What happens when a driver does:
>
> d1 = iommu_domain_alloc()
> iommu_attach_device(d1)
> d2 = iommu_sva_bind_device()
> iommu_detach_device(d1)
>
> Does detach
> (a) only disable the non-PASID address space?
> (b) disable everything?
> (c) fail because the driver didn't unbind first?
I think it must be (a), considering how everything is defined and the
needs for vIOMMU emulation.
If it is any other option then we have a problem of what to do if the
guest VM asks to change the page table associated with the RID while a
PASID is attached.
> I'm asking because the SMMU driver is still using smmu_domain to represent
> all address spaces + the non-PASID one, and using the same type
> "iommu_domain" for the new object makes things unreadable. I think
> internally we'll want to use distinct variable names, something like
> "domain" and "address_space". If (a) is not the direction you're going,
> then it may be worth renaming the API as well.
>
> I'm also not sure why set_dev_pasid() is a domain_ops of the SVA domain,
> but acts on the parent domain which contains the PASID table. Shouldn't it
> be an IOMMU op like remove_dev_pasid(), or on the parent domain?
There is no "parent domain"
PASID or RID+PASID are completely equal concepts for binding.
If you are thinking "parent domain" because SMMU is storing the PASID
table in the RID's iommu_domain, then I think that is a misplacement
in the SMMU driver...
The PASID table belongs in the iommu driver's per-group data
structure. The iommu domain should only have the actual IOPTEs.
Otherwise everything blows up if you attach an iommu_domain to two
RIDs - the API demands that every RID gets its own PASID mapping, even
if the RID shares iommu_domains. We do not have an API to share PASID
tables.
Thus the PASID table is NOT part of the iommu_domain.
The exception will be for nested translation where we will have a
special ARM iommu_domain that contains the PASID table in userspace
memory. When this domain is attached it will logically claim the RID
and every PASID and thus disable the PASID API for that RID.
Remember also that an UNMANAGED iommu_domain should be attachable to
many PASID's and RID's concurrently.
Jason
On Wed, Sep 07, 2022 at 02:33:11PM -0300, Jason Gunthorpe wrote:
> On Wed, Sep 07, 2022 at 10:54:54AM +0100, Jean-Philippe Brucker wrote:
>
> > Is iommu_domain still going to represent both a device context (whole
> > PASID table) and individual address spaces, or are you planning to move
> > away from that? What happens when a driver does:
> >
> > d1 = iommu_domain_alloc()
> > iommu_attach_device(d1)
> > d2 = iommu_sva_bind_device()
> > iommu_detach_device(d1)
> >
> > Does detach
> > (a) only disable the non-PASID address space?
> > (b) disable everything?
> > (c) fail because the driver didn't unbind first?
>
> I think it must be (a), considering how everything is defined and the
> needs for vIOMMU emulation.
Yes (a) is probably better. The SMMU driver currently implements (c) to
ensure that you can't switch device driver without unbinding everything
first, and we should keep that check somewhere
>
> If it is any other option then we have a problem of what to do if the
> guest VM asks to change the page table associated with the RID while a
> PASID is attached.
>
> > I'm asking because the SMMU driver is still using smmu_domain to represent
> > all address spaces + the non-PASID one, and using the same type
> > "iommu_domain" for the new object makes things unreadable. I think
> > internally we'll want to use distinct variable names, something like
> > "domain" and "address_space". If (a) is not the direction you're going,
> > then it may be worth renaming the API as well.
> >
> > I'm also not sure why set_dev_pasid() is a domain_ops of the SVA domain,
> > but acts on the parent domain which contains the PASID table. Shouldn't it
> > be an IOMMU op like remove_dev_pasid(), or on the parent domain?
>
> There is no "parent domain"
>
> PASID or RID+PASID are completely equal concepts for binding.
>
> If you are thinking "parent domain" because SMMU is storing the PASID
> table in the RID's iommu_domain, then I think that is a misplacement
> in the SMMU driver...
>
> The PASID table belongs in the iommu driver's per-group data
> structure. The iommu domain should only have the actual IOPTEs.
>
> Otherwise everything blows up if you attach an iommu_domain to two
> RIDs - the API demands that every RID gets its own PASID mapping, even
> if the RID shares iommu_domains. We do not have an API to share PASID
> tables.
Well, we still do since SMMU implements it. Changing the API is fine, but
someone will need to rework the SMMU driver to align with the new meaning
of iommu_domain. I can take a stab if no one volunteers but probably not
before next year.
Thanks,
Jean
>
> Thus the PASID table is NOT part of the iommu_domain.
>
> The exception will be for nested translation where we will have a
> special ARM iommu_domain that contains the PASID table in userspace
> memory. When this domain is attached it will logically claim the RID
> and every PASID and thus disable the PASID API for that RID.
>
> Remember also that an UNMANAGED iommu_domain should be attachable to
> many PASID's and RID's concurrently.
>
> Jason
On Thu, Sep 08, 2022 at 05:25:32PM +0100, Jean-Philippe Brucker wrote:
> On Wed, Sep 07, 2022 at 02:33:11PM -0300, Jason Gunthorpe wrote:
> > On Wed, Sep 07, 2022 at 10:54:54AM +0100, Jean-Philippe Brucker wrote:
> >
> > > Is iommu_domain still going to represent both a device context (whole
> > > PASID table) and individual address spaces, or are you planning to move
> > > away from that? What happens when a driver does:
> > >
> > > d1 = iommu_domain_alloc()
> > > iommu_attach_device(d1)
> > > d2 = iommu_sva_bind_device()
> > > iommu_detach_device(d1)
> > >
> > > Does detach
> > > (a) only disable the non-PASID address space?
> > > (b) disable everything?
> > > (c) fail because the driver didn't unbind first?
> >
> > I think it must be (a), considering how everything is defined and the
> > needs for vIOMMU emulation.
>
> Yes (a) is probably better. The SMMU driver currently implements (c) to
> ensure that you can't switch device driver without unbinding everything
> first, and we should keep that check somewhere
Yes, the owner stuff is a logical place to put that, when ownership
is all released the PASID table of the group must be empty. Lu?
> > Otherwise everything blows up if you attach an iommu_domain to two
> > RIDs - the API demands that every RID gets its own PASID mapping, even
> > if the RID shares iommu_domains. We do not have an API to share PASID
> > tables.
>
> Well, we still do since SMMU implements it. Changing the API is fine, but
> someone will need to rework the SMMU driver to align with the new meaning
> of iommu_domain. I can take a stab if no one volunteers but probably not
> before next year.
I think given the current progression it is OK for this series to
leave the SMMU driver with a bit of a different view of how the API
should work. The main target here is the in-kernel drivers and they
won't trigger these cases.
As we extend things into userspace PASID support we may want to block
SMMU PASID support through iommfd/vfio until it is updated.
I've been thinking on and off about this - we likely need to have a
conformance test suite as part of iommufd that is specifically for
checking that the iommu driver meets the API. So it would do things
like exercise scenario (a), attaching domains to all sorts of
combinations, and so on.
We know this from other driver subsystems that drivers can implement
the uAPI incorrectly and it causes a lot of pain for userspace.
Jason
On 2022/9/9 0:41, Jason Gunthorpe wrote:
> On Thu, Sep 08, 2022 at 05:25:32PM +0100, Jean-Philippe Brucker wrote:
>> On Wed, Sep 07, 2022 at 02:33:11PM -0300, Jason Gunthorpe wrote:
>>> On Wed, Sep 07, 2022 at 10:54:54AM +0100, Jean-Philippe Brucker wrote:
>>>
>>>> Is iommu_domain still going to represent both a device context (whole
>>>> PASID table) and individual address spaces, or are you planning to move
>>>> away from that? What happens when a driver does:
>>>>
>>>> d1 = iommu_domain_alloc()
>>>> iommu_attach_device(d1)
>>>> d2 = iommu_sva_bind_device()
>>>> iommu_detach_device(d1)
>>>>
>>>> Does detach
>>>> (a) only disable the non-PASID address space?
>>>> (b) disable everything?
>>>> (c) fail because the driver didn't unbind first?
>>> I think it must be (a), considering how everything is defined and the
>>> needs for vIOMMU emulation.
>> Yes (a) is probably better. The SMMU driver currently implements (c) to
>> ensure that you can't switch device driver without unbinding everything
>> first, and we should keep that check somewhere
> Yes, the owner stuff is a logical place to put that, when ownership
> is all released the PASID table of the group must be empty. Lu?
I agree. The ownership is about the whole device (more precisely,
group), including the RID and PASIDs. When the ownership is converted,
the pasid array must be empty. I will add code in this series to enforce
this. Thanks for pointing out this.
Best regards,
baolu
On Tue, Sep 06, 2022 at 08:44:54PM +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
> + *
> + * 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.
Thsi is something else that needs cleaning up. IOMMU_DEV_FEAT_SVA
shouldn't exist.
We need to figure out an appropriate way to allow PRI. IMHO the domain
attach should do this, domains that require PRI should be distinct
from domains that don't. When a PRI domain is attached the HW should
be enabled to do PRI. The domain itself should carry the fault ops/etc
that the caller supplies to respond to the PRI.
That is something to address in the PRI series though..
Reviewed-by: Jason Gunthorpe <[email protected]>
Jason
On 2022/9/23 0:00, Jason Gunthorpe wrote:
> On Tue, Sep 06, 2022 at 08:44:54PM +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
>> + *
>> + * 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.
> Thsi is something else that needs cleaning up. IOMMU_DEV_FEAT_SVA
> shouldn't exist.
>
> We need to figure out an appropriate way to allow PRI. IMHO the domain
> attach should do this, domains that require PRI should be distinct
> from domains that don't. When a PRI domain is attached the HW should
> be enabled to do PRI. The domain itself should carry the fault ops/etc
> that the caller supplies to respond to the PRI.
>
> That is something to address in the PRI series though..
From Intel IOMMU driver's point of view, with above done,
IOMMU_DEV_FEAT_SVA could be removed. However, it will take more time to
consider other needs.
>
> Reviewed-by: Jason Gunthorpe<[email protected]>
Best regards,
baolu