Now, access->ops can be NULL, to support an internal use case for the new
HW queue object. Since an access object in this case will be allocated by
an inernal iommufd object, the refcount on the ictx should be skipped, so
as not to deadlock the release of the ictx as it would otherwise wait for
the release of the access first during the release of the internal object
that could wait for the release of ictx:
ictx --releases--> hw_queue --releases--> access
^ |
|_________________releases________________v
Add a set of lightweight internal APIs to unlink access and ictx:
ictx --releases--> hw_queue --releases--> access
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/iommu/iommufd/iommufd_private.h | 8 ++++
drivers/iommu/iommufd/device.c | 59 +++++++++++++++++++++----
2 files changed, 58 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 4a375a8c9216..468717d5e5bc 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt,
struct iommufd_access *access, u32 iopt_access_list_id);
void iommufd_access_destroy_object(struct iommufd_object *obj);
+/* iommufd_access for internal use */
+struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx);
+#define iommufd_access_destroy_internal(ictx, access) \
+ iommufd_object_destroy_user(ictx, &(access)->obj)
+int iommufd_access_attach_internal(struct iommufd_access *access,
+ struct iommufd_ioas *ioas);
+#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
+
struct iommufd_eventq {
struct iommufd_object obj;
struct iommufd_ctx *ictx;
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 9293722b9cff..ad33f1e41a24 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
if (access->ioas)
WARN_ON(iommufd_access_change_ioas(access, NULL));
mutex_unlock(&access->ioas_lock);
- iommufd_ctx_put(access->ictx);
+ if (access->ops)
+ iommufd_ctx_put(access->ictx);
+}
+
+static struct iommufd_access *__iommufd_access_create(struct iommufd_ctx *ictx)
+{
+ struct iommufd_access *access;
+
+ /*
+ * There is no uAPI for the access object, but to keep things symmetric
+ * use the object infrastructure anyhow.
+ */
+ access = iommufd_object_alloc(ictx, access, IOMMUFD_OBJ_ACCESS);
+ if (IS_ERR(access))
+ return access;
+
+ /* The calling driver is a user until iommufd_access_destroy() */
+ refcount_inc(&access->obj.users);
+ mutex_init(&access->ioas_lock);
+ return access;
+}
+
+struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx)
+{
+ struct iommufd_access *access;
+
+ access = __iommufd_access_create(ictx);
+ if (IS_ERR(access))
+ return access;
+ access->iova_alignment = PAGE_SIZE;
+
+ iommufd_object_finalize(ictx, &access->obj);
+ return access;
}
/**
@@ -1106,11 +1138,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
{
struct iommufd_access *access;
- /*
- * There is no uAPI for the access object, but to keep things symmetric
- * use the object infrastructure anyhow.
- */
- access = iommufd_object_alloc(ictx, access, IOMMUFD_OBJ_ACCESS);
+ access = __iommufd_access_create(ictx);
if (IS_ERR(access))
return access;
@@ -1122,13 +1150,10 @@ iommufd_access_create(struct iommufd_ctx *ictx,
else
access->iova_alignment = 1;
- /* The calling driver is a user until iommufd_access_destroy() */
- refcount_inc(&access->obj.users);
access->ictx = ictx;
iommufd_ctx_get(ictx);
iommufd_object_finalize(ictx, &access->obj);
*id = access->obj.id;
- mutex_init(&access->ioas_lock);
return access;
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_create, "IOMMUFD");
@@ -1173,6 +1198,22 @@ int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, "IOMMUFD");
+int iommufd_access_attach_internal(struct iommufd_access *access,
+ struct iommufd_ioas *ioas)
+{
+ int rc;
+
+ mutex_lock(&access->ioas_lock);
+ if (WARN_ON(access->ioas)) {
+ mutex_unlock(&access->ioas_lock);
+ return -EINVAL;
+ }
+
+ rc = iommufd_access_change_ioas(access, ioas);
+ mutex_unlock(&access->ioas_lock);
+ return rc;
+}
+
int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id)
{
int rc;
--
2.43.0On Sat, Jun 14, 2025 at 12:14:32AM -0700, Nicolin Chen wrote:
> Now, access->ops can be NULL, to support an internal use case for the new
> HW queue object. Since an access object in this case will be allocated by
> an inernal iommufd object, the refcount on the ictx should be skipped, so
> as not to deadlock the release of the ictx as it would otherwise wait for
> the release of the access first during the release of the internal object
> that could wait for the release of ictx:
> ictx --releases--> hw_queue --releases--> access
> ^ |
> |_________________releases________________v
>
> Add a set of lightweight internal APIs to unlink access and ictx:
> ictx --releases--> hw_queue --releases--> access
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> drivers/iommu/iommufd/iommufd_private.h | 8 ++++
> drivers/iommu/iommufd/device.c | 59 +++++++++++++++++++++----
> 2 files changed, 58 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 4a375a8c9216..468717d5e5bc 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt,
> struct iommufd_access *access, u32 iopt_access_list_id);
> void iommufd_access_destroy_object(struct iommufd_object *obj);
>
> +/* iommufd_access for internal use */
> +struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx);
> +#define iommufd_access_destroy_internal(ictx, access) \
> + iommufd_object_destroy_user(ictx, &(access)->obj)
> +int iommufd_access_attach_internal(struct iommufd_access *access,
> + struct iommufd_ioas *ioas);
> +#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
> +
> struct iommufd_eventq {
> struct iommufd_object obj;
> struct iommufd_ctx *ictx;
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 9293722b9cff..ad33f1e41a24 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
> if (access->ioas)
> WARN_ON(iommufd_access_change_ioas(access, NULL));
> mutex_unlock(&access->ioas_lock);
> - iommufd_ctx_put(access->ictx);
> + if (access->ops)
> + iommufd_ctx_put(access->ictx);
Purely relying on access->ops being NULL feels a bit hacky to me..
> +}
> +
> +static struct iommufd_access *__iommufd_access_create(struct iommufd_ctx *ictx)
> +{
> + struct iommufd_access *access;
> +
> + /*
> + * There is no uAPI for the access object, but to keep things symmetric
> + * use the object infrastructure anyhow.
> + */
> + access = iommufd_object_alloc(ictx, access, IOMMUFD_OBJ_ACCESS);
> + if (IS_ERR(access))
> + return access;
> +
> + /* The calling driver is a user until iommufd_access_destroy() */
> + refcount_inc(&access->obj.users);
> + mutex_init(&access->ioas_lock);
> + return access;
> +}
> +
> +struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx)
> +{
> + struct iommufd_access *access;
> +
> + access = __iommufd_access_create(ictx);
> + if (IS_ERR(access))
> + return access;
> + access->iova_alignment = PAGE_SIZE;
Maybe setting acces->ictx = NULL; explicitly here would be a clear
demarcation between the new API for "internal" v/s the original one.
Else, I definitely believe we should have a comment mentioning that
access->ictx is NULL for internal.
> +
> + iommufd_object_finalize(ictx, &access->obj);
> + return access;
> }
>
> /**
> @@ -1106,11 +1138,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> {
> struct iommufd_access *access;
>
> - /*
> - * There is no uAPI for the access object, but to keep things symmetric
> - * use the object infrastructure anyhow.
> - */
> - access = iommufd_object_alloc(ictx, access, IOMMUFD_OBJ_ACCESS);
> + access = __iommufd_access_create(ictx);
> if (IS_ERR(access))
> return access;
>
> @@ -1122,13 +1150,10 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> else
> access->iova_alignment = 1;
>
> - /* The calling driver is a user until iommufd_access_destroy() */
> - refcount_inc(&access->obj.users);
> access->ictx = ictx;
> iommufd_ctx_get(ictx);
> iommufd_object_finalize(ictx, &access->obj);
> *id = access->obj.id;
> - mutex_init(&access->ioas_lock);
> return access;
> }
> EXPORT_SYMBOL_NS_GPL(iommufd_access_create, "IOMMUFD");
> @@ -1173,6 +1198,22 @@ int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> }
> EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, "IOMMUFD");
>
> +int iommufd_access_attach_internal(struct iommufd_access *access,
> + struct iommufd_ioas *ioas)
> +{
> + int rc;
> +
> + mutex_lock(&access->ioas_lock);
> + if (WARN_ON(access->ioas)) {
> + mutex_unlock(&access->ioas_lock);
> + return -EINVAL;
> + }
> +
> + rc = iommufd_access_change_ioas(access, ioas);
> + mutex_unlock(&access->ioas_lock);
> + return rc;
> +}
> +
> int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id)
> {
> int rc;
> --
> 2.43.0
>
On Sat, Jun 14, 2025 at 12:14:32AM -0700, Nicolin Chen wrote:
> Now, access->ops can be NULL, to support an internal use case for the new
> HW queue object. Since an access object in this case will be allocated by
> an inernal iommufd object, the refcount on the ictx should be skipped, so
> as not to deadlock the release of the ictx as it would otherwise wait for
> the release of the access first during the release of the internal object
> that could wait for the release of ictx:
> ictx --releases--> hw_queue --releases--> access
> ^ |
> |_________________releases________________v
>
> Add a set of lightweight internal APIs to unlink access and ictx:
> ictx --releases--> hw_queue --releases--> access
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> drivers/iommu/iommufd/iommufd_private.h | 8 ++++
> drivers/iommu/iommufd/device.c | 59 +++++++++++++++++++++----
> 2 files changed, 58 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 4a375a8c9216..468717d5e5bc 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt,
> struct iommufd_access *access, u32 iopt_access_list_id);
> void iommufd_access_destroy_object(struct iommufd_object *obj);
>
> +/* iommufd_access for internal use */
> +struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx);
> +#define iommufd_access_destroy_internal(ictx, access) \
> + iommufd_object_destroy_user(ictx, &(access)->obj)
Use a static inline please
> +int iommufd_access_attach_internal(struct iommufd_access *access,
> + struct iommufd_ioas *ioas);
> +#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
> struct iommufd_eventq {
> struct iommufd_object obj;
> struct iommufd_ctx *ictx;
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 9293722b9cff..ad33f1e41a24 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
> if (access->ioas)
> WARN_ON(iommufd_access_change_ioas(access, NULL));
> mutex_unlock(&access->ioas_lock);
> - iommufd_ctx_put(access->ictx);
> + if (access->ops)
> + iommufd_ctx_put(access->ictx);
I was hoping we could null the ictx to signal internal? That didn't
work out?
I would at least add a comment here this is filtering internal that
doesn't have ictx. Maybe a little inline 'iommufd_access_is_internal'
is appropriate. We'll be sad down the road if we need ops for
internal.
Jason
On Mon, Jun 16, 2025 at 10:37:19AM -0300, Jason Gunthorpe wrote:
> On Sat, Jun 14, 2025 at 12:14:32AM -0700, Nicolin Chen wrote:
> > Now, access->ops can be NULL, to support an internal use case for the new
> > HW queue object. Since an access object in this case will be allocated by
> > an inernal iommufd object, the refcount on the ictx should be skipped, so
> > as not to deadlock the release of the ictx as it would otherwise wait for
> > the release of the access first during the release of the internal object
> > that could wait for the release of ictx:
> > ictx --releases--> hw_queue --releases--> access
> > ^ |
> > |_________________releases________________v
> >
> > Add a set of lightweight internal APIs to unlink access and ictx:
> > ictx --releases--> hw_queue --releases--> access
> >
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > ---
> > drivers/iommu/iommufd/iommufd_private.h | 8 ++++
> > drivers/iommu/iommufd/device.c | 59 +++++++++++++++++++++----
> > 2 files changed, 58 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> > index 4a375a8c9216..468717d5e5bc 100644
> > --- a/drivers/iommu/iommufd/iommufd_private.h
> > +++ b/drivers/iommu/iommufd/iommufd_private.h
> > @@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt,
> > struct iommufd_access *access, u32 iopt_access_list_id);
> > void iommufd_access_destroy_object(struct iommufd_object *obj);
> >
> > +/* iommufd_access for internal use */
> > +struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx);
> > +#define iommufd_access_destroy_internal(ictx, access) \
> > + iommufd_object_destroy_user(ictx, &(access)->obj)
>
> Use a static inline please
>
> > +int iommufd_access_attach_internal(struct iommufd_access *access,
> > + struct iommufd_ioas *ioas);
> > +#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
>
>
> > struct iommufd_eventq {
> > struct iommufd_object obj;
> > struct iommufd_ctx *ictx;
> > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> > index 9293722b9cff..ad33f1e41a24 100644
> > --- a/drivers/iommu/iommufd/device.c
> > +++ b/drivers/iommu/iommufd/device.c
> > @@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
> > if (access->ioas)
> > WARN_ON(iommufd_access_change_ioas(access, NULL));
> > mutex_unlock(&access->ioas_lock);
> > - iommufd_ctx_put(access->ictx);
> > + if (access->ops)
> > + iommufd_ctx_put(access->ictx);
>
> I was hoping we could null the ictx to signal internal? That didn't
> work out?
access->ictx should be NULL for internal. It should have been:
+ if (access->ictx)
+ iommufd_ctx_put(access->ictx);
> I would at least add a comment here this is filtering internal that
> doesn't have ictx. Maybe a little inline 'iommufd_access_is_internal'
> is appropriate. We'll be sad down the road if we need ops for
> internal.
Yea, an inline will be cleaner. Will add that.
Thanks
Nicolin
On Mon, Jun 16, 2025 at 07:25:57PM -0700, Nicolin Chen wrote:
> On Mon, Jun 16, 2025 at 10:37:19AM -0300, Jason Gunthorpe wrote:
> > On Sat, Jun 14, 2025 at 12:14:32AM -0700, Nicolin Chen wrote:
> > > Now, access->ops can be NULL, to support an internal use case for the new
> > > HW queue object. Since an access object in this case will be allocated by
> > > an inernal iommufd object, the refcount on the ictx should be skipped, so
> > > as not to deadlock the release of the ictx as it would otherwise wait for
> > > the release of the access first during the release of the internal object
> > > that could wait for the release of ictx:
> > > ictx --releases--> hw_queue --releases--> access
> > > ^ |
> > > |_________________releases________________v
> > >
> > > Add a set of lightweight internal APIs to unlink access and ictx:
> > > ictx --releases--> hw_queue --releases--> access
> > >
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > ---
> > > drivers/iommu/iommufd/iommufd_private.h | 8 ++++
> > > drivers/iommu/iommufd/device.c | 59 +++++++++++++++++++++----
> > > 2 files changed, 58 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> > > index 4a375a8c9216..468717d5e5bc 100644
> > > --- a/drivers/iommu/iommufd/iommufd_private.h
> > > +++ b/drivers/iommu/iommufd/iommufd_private.h
> > > @@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt,
> > > struct iommufd_access *access, u32 iopt_access_list_id);
> > > void iommufd_access_destroy_object(struct iommufd_object *obj);
> > >
> > > +/* iommufd_access for internal use */
> > > +struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx);
> > > +#define iommufd_access_destroy_internal(ictx, access) \
> > > + iommufd_object_destroy_user(ictx, &(access)->obj)
> >
> > Use a static inline please
> >
> > > +int iommufd_access_attach_internal(struct iommufd_access *access,
> > > + struct iommufd_ioas *ioas);
> > > +#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
> >
> >
> > > struct iommufd_eventq {
> > > struct iommufd_object obj;
> > > struct iommufd_ctx *ictx;
> > > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> > > index 9293722b9cff..ad33f1e41a24 100644
> > > --- a/drivers/iommu/iommufd/device.c
> > > +++ b/drivers/iommu/iommufd/device.c
> > > @@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
> > > if (access->ioas)
> > > WARN_ON(iommufd_access_change_ioas(access, NULL));
> > > mutex_unlock(&access->ioas_lock);
> > > - iommufd_ctx_put(access->ictx);
> > > + if (access->ops)
> > > + iommufd_ctx_put(access->ictx);
> >
> > I was hoping we could null the ictx to signal internal? That didn't
> > work out?
>
> access->ictx should be NULL for internal. It should have been:
> + if (access->ictx)
> + iommufd_ctx_put(access->ictx);
>
Ohh sorry, just saw this. +1, I too believe this is better than relying
on access->ops being NULL.
> > I would at least add a comment here this is filtering internal that
> > doesn't have ictx. Maybe a little inline 'iommufd_access_is_internal'
> > is appropriate. We'll be sad down the road if we need ops for
> > internal.
>
> Yea, an inline will be cleaner. Will add that.
>
Ack.
Thanks,
Praan
On 6/17/25 10:25, Nicolin Chen wrote:
>>> struct iommufd_eventq {
>>> struct iommufd_object obj;
>>> struct iommufd_ctx *ictx;
>>> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
>>> index 9293722b9cff..ad33f1e41a24 100644
>>> --- a/drivers/iommu/iommufd/device.c
>>> +++ b/drivers/iommu/iommufd/device.c
>>> @@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
>>> if (access->ioas)
>>> WARN_ON(iommufd_access_change_ioas(access, NULL));
>>> mutex_unlock(&access->ioas_lock);
>>> - iommufd_ctx_put(access->ictx);
>>> + if (access->ops)
>>> + iommufd_ctx_put(access->ictx);
>> I was hoping we could null the ictx to signal internal? That didn't
>> work out?
> access->ictx should be NULL for internal. It should have been:
> + if (access->ictx)
> + iommufd_ctx_put(access->ictx);
access->ictx could be treated as user ownership token. If it's NULL,
there is no user ownership, indicating it's owned by the kernel. This is
the concept here?
Thanks,
baolu
On Tue, Jun 17, 2025 at 12:23:20PM +0800, Baolu Lu wrote:
> On 6/17/25 10:25, Nicolin Chen wrote:
> > > > struct iommufd_eventq {
> > > > struct iommufd_object obj;
> > > > struct iommufd_ctx *ictx;
> > > > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> > > > index 9293722b9cff..ad33f1e41a24 100644
> > > > --- a/drivers/iommu/iommufd/device.c
> > > > +++ b/drivers/iommu/iommufd/device.c
> > > > @@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
> > > > if (access->ioas)
> > > > WARN_ON(iommufd_access_change_ioas(access, NULL));
> > > > mutex_unlock(&access->ioas_lock);
> > > > - iommufd_ctx_put(access->ictx);
> > > > + if (access->ops)
> > > > + iommufd_ctx_put(access->ictx);
> > > I was hoping we could null the ictx to signal internal? That didn't
> > > work out?
> > access->ictx should be NULL for internal. It should have been:
> > + if (access->ictx)
> > + iommufd_ctx_put(access->ictx);
>
> access->ictx could be treated as user ownership token. If it's NULL,
> there is no user ownership, indicating it's owned by the kernel. This is
> the concept here?
Yes
Jason
© 2016 - 2025 Red Hat, Inc.