Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Thu, Oct 17, 2024 at 10:01:28AM -0700, Nicolin Chen wrote: > On Thu, Oct 17, 2024 at 01:33:59PM -0300, Jason Gunthorpe wrote: > > > > diff --git a/drivers/iommu/iommufd/viommu_api.c > > > b/drivers/iommu/iommufd/viommu_api.c > > > new file mode 100644 > > > index ..c1731f080d6b > > > --- /dev/null > > > +++ b/drivers/iommu/iommufd/viommu_api.c > > > > Let's call this file driver.c to match CONFIG_IOMMUFD_DRIVER > > Would this make its scope too large that it might feel odd to have > the iova_bitmap.c in a separate file? Think of it as the catchall ? Jason
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Thu, Oct 17, 2024 at 01:33:59PM -0300, Jason Gunthorpe wrote:
> > diff --git a/drivers/iommu/iommufd/viommu_api.c
> > b/drivers/iommu/iommufd/viommu_api.c
> > new file mode 100644
> > index ..c1731f080d6b
> > --- /dev/null
> > +++ b/drivers/iommu/iommufd/viommu_api.c
>
> Let's call this file driver.c to match CONFIG_IOMMUFD_DRIVER
Would this make its scope too large that it might feel odd to have
the iova_bitmap.c in a separate file?
> /*
> * Helpers for IOMMU driver to allocate driver structures that will be freed
> by
> * the iommufd core. The free op will be called prior to freeing the memory.
> */
> #define iommufd_viommu_alloc(ictx, drv_struct, member, viommu_ops)
> \
> ({\
> struct drv_struct *ret; \
>
> \
> static_assert(\
> __same_type(struct iommufd_viommu,\
> ((struct drv_struct *)NULL)->member));\
> static_assert(offsetof(struct drv_struct, member.obj) == 0); \
> ret = (struct drv_struct *)iommufd_object_alloc_elm( \
> ictx, sizeof(struct drv_struct), IOMMUFD_OBJ_VIOMMU); \
> ret->member.ops = viommu_ops; \
> ret; \
> })
OK. Will try with this.
Thanks
Nicolin
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Wed, Oct 09, 2024 at 09:38:03AM -0700, Nicolin Chen wrote:
> + struct iommufd_viommu *(*viommu_alloc)(struct iommu_device *iommu_dev,
> +struct iommu_domain *domain,
Let's call this parent_domain ie nesting parent
> +/*
> + * Helpers for IOMMU driver to allocate driver structures that will be freed
> by
> + * the iommufd core. Yet, a driver is responsible for its own
> struct cleanup.
'own struct cleanup via the free callback'
> diff --git a/drivers/iommu/iommufd/viommu_api.c
> b/drivers/iommu/iommufd/viommu_api.c
> new file mode 100644
> index ..c1731f080d6b
> --- /dev/null
> +++ b/drivers/iommu/iommufd/viommu_api.c
Let's call this file driver.c to match CONFIG_IOMMUFD_DRIVER
> +struct iommufd_viommu *
> +__iommufd_viommu_alloc(struct iommufd_ctx *ictx, size_t size,
> +const struct iommufd_viommu_ops *ops)
> +{
> + struct iommufd_viommu *viommu;
> + struct iommufd_object *obj;
> +
> + if (WARN_ON(size < sizeof(*viommu)))
> + return ERR_PTR(-EINVAL);
The macro does this already
> + obj = iommufd_object_alloc_elm(ictx, size, IOMMUFD_OBJ_VIOMMU);
> + if (IS_ERR(obj))
> + return ERR_CAST(obj);
> + viommu = container_of(obj, struct iommufd_viommu, obj);
And this too..
> + if (ops)
> + viommu->ops = ops;
No need for an if...
It could just be in the macro which is a bit appealing given we want
this linked into the drivers..
/*
* Helpers for IOMMU driver to allocate driver structures that will be freed by
* the iommufd core. The free op will be called prior to freeing the memory.
*/
#define iommufd_viommu_alloc(ictx, drv_struct, member, viommu_ops)\
({\
struct drv_struct *ret; \
\
static_assert(\
__same_type(struct iommufd_viommu,\
((struct drv_struct *)NULL)->member));\
static_assert(offsetof(struct drv_struct, member.obj) == 0); \
ret = (struct drv_struct *)iommufd_object_alloc_elm( \
ictx, sizeof(struct drv_struct), IOMMUFD_OBJ_VIOMMU); \
ret->member.ops = viommu_ops; \
ret; \
})
Jason
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Wed, 16 Oct 2024 at 14:52, Nicolin Chen wrote:
>
> On Wed, Oct 16, 2024 at 09:56:51AM +0800, Zhangfei Gao wrote:
> > On Wed, 16 Oct 2024 at 02:44, Nicolin Chen wrote:
> > >
> > > On Mon, Oct 14, 2024 at 07:01:40PM -0700, Nicolin Chen wrote:
> > > > On Tue, Oct 15, 2024 at 09:15:01AM +0800, Zhangfei Gao wrote:
> > > >
> > > > > > > iommufd_device_bind
> > > > > > > iommufd_device_attach
> > > > > > > iommufd_vdevice_alloc_ioctl
> > > > > > >
> > > > > > > iommufd_device_detach
> > > > > > > iommufd_device_unbind // refcount check fail
> > > > > > > iommufd_vdevice_destroy ref--
> > > > > >
> > > > > > Things should be symmetric. As you suspected, vdevice should be
> > > > > > destroyed before iommufd_device_detach.
> > > > >
> > > > > I am trying based on your for_iommufd_viommu_p2-v3 branch, do you have
> > > > > this issue?
> > > > > In checking whether close fd before unbind?
> > > >
> > > > Oops, my bad. I will provide a fix.
> > >
> > > This should fix the problem:
> > >
> > > -
> > > diff --git a/drivers/iommu/iommufd/device.c
> > > b/drivers/iommu/iommufd/device.c
> > > index 5fd3dd420290..13100cfea29d 100644
> > > --- a/drivers/iommu/iommufd/device.c
> > > +++ b/drivers/iommu/iommufd/device.c
> > > @@ -277,6 +277,11 @@ EXPORT_SYMBOL_NS_GPL(iommufd_ctx_has_group, IOMMUFD);
> > > */
> > > void iommufd_device_unbind(struct iommufd_device *idev)
> > > {
> > > + mutex_lock(&idev->igroup->lock);
> > > + /* idev->vdev object should be destroyed prior, yet just in
> > > case.. */
> > > + if (idev->vdev)
> > > + iommufd_object_remove(idev->ictx, NULL,
> > > idev->vdev->obj.id, 0);
> > > + mutex_unlock(&idev->igroup->lock);
> > > iommufd_object_destroy_user(idev->ictx, &idev->obj);
> > > }
> > > EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, IOMMUFD);
> > > -
> >
> > Not yet
> > [ 574.162112] Unable to handle kernel NULL pointer dereference at
> > virtual address 0004
> > [ 574.261102] pc : iommufd_object_remove+0x7c/0x278
> > [ 574.265795] lr : iommufd_device_unbind+0x44/0x98
> > in check
>
> Hmm, it's kinda odd it crashes inside iommufd_object_remove().
> Did you happen to change something there?
>
> The added iommufd_object_remove() is equivalent to userspace
> calling the destroy ioctl on the vDEVICE object.
>
Yes, double confirmed, it can solve the issue.
The guest can stop and run again
The Null pointer may be caused by the added debug.
Thanks Nico.
> Nicolin
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Wed, Oct 16, 2024 at 09:56:51AM +0800, Zhangfei Gao wrote:
> On Wed, 16 Oct 2024 at 02:44, Nicolin Chen wrote:
> >
> > On Mon, Oct 14, 2024 at 07:01:40PM -0700, Nicolin Chen wrote:
> > > On Tue, Oct 15, 2024 at 09:15:01AM +0800, Zhangfei Gao wrote:
> > >
> > > > > > iommufd_device_bind
> > > > > > iommufd_device_attach
> > > > > > iommufd_vdevice_alloc_ioctl
> > > > > >
> > > > > > iommufd_device_detach
> > > > > > iommufd_device_unbind // refcount check fail
> > > > > > iommufd_vdevice_destroy ref--
> > > > >
> > > > > Things should be symmetric. As you suspected, vdevice should be
> > > > > destroyed before iommufd_device_detach.
> > > >
> > > > I am trying based on your for_iommufd_viommu_p2-v3 branch, do you have
> > > > this issue?
> > > > In checking whether close fd before unbind?
> > >
> > > Oops, my bad. I will provide a fix.
> >
> > This should fix the problem:
> >
> > -
> > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> > index 5fd3dd420290..13100cfea29d 100644
> > --- a/drivers/iommu/iommufd/device.c
> > +++ b/drivers/iommu/iommufd/device.c
> > @@ -277,6 +277,11 @@ EXPORT_SYMBOL_NS_GPL(iommufd_ctx_has_group, IOMMUFD);
> > */
> > void iommufd_device_unbind(struct iommufd_device *idev)
> > {
> > + mutex_lock(&idev->igroup->lock);
> > + /* idev->vdev object should be destroyed prior, yet just in case..
> > */
> > + if (idev->vdev)
> > + iommufd_object_remove(idev->ictx, NULL, idev->vdev->obj.id,
> > 0);
> > + mutex_unlock(&idev->igroup->lock);
> > iommufd_object_destroy_user(idev->ictx, &idev->obj);
> > }
> > EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, IOMMUFD);
> > -
>
> Not yet
> [ 574.162112] Unable to handle kernel NULL pointer dereference at
> virtual address 0004
> [ 574.261102] pc : iommufd_object_remove+0x7c/0x278
> [ 574.265795] lr : iommufd_device_unbind+0x44/0x98
> in check
Hmm, it's kinda odd it crashes inside iommufd_object_remove().
Did you happen to change something there?
The added iommufd_object_remove() is equivalent to userspace
calling the destroy ioctl on the vDEVICE object.
Nicolin
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Wed, 16 Oct 2024 at 02:44, Nicolin Chen wrote:
>
> On Mon, Oct 14, 2024 at 07:01:40PM -0700, Nicolin Chen wrote:
> > On Tue, Oct 15, 2024 at 09:15:01AM +0800, Zhangfei Gao wrote:
> >
> > > > > iommufd_device_bind
> > > > > iommufd_device_attach
> > > > > iommufd_vdevice_alloc_ioctl
> > > > >
> > > > > iommufd_device_detach
> > > > > iommufd_device_unbind // refcount check fail
> > > > > iommufd_vdevice_destroy ref--
> > > >
> > > > Things should be symmetric. As you suspected, vdevice should be
> > > > destroyed before iommufd_device_detach.
> > >
> > > I am trying based on your for_iommufd_viommu_p2-v3 branch, do you have
> > > this issue?
> > > In checking whether close fd before unbind?
> >
> > Oops, my bad. I will provide a fix.
>
> This should fix the problem:
>
> -
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 5fd3dd420290..13100cfea29d 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -277,6 +277,11 @@ EXPORT_SYMBOL_NS_GPL(iommufd_ctx_has_group, IOMMUFD);
> */
> void iommufd_device_unbind(struct iommufd_device *idev)
> {
> + mutex_lock(&idev->igroup->lock);
> + /* idev->vdev object should be destroyed prior, yet just in case.. */
> + if (idev->vdev)
> + iommufd_object_remove(idev->ictx, NULL, idev->vdev->obj.id,
> 0);
> + mutex_unlock(&idev->igroup->lock);
> iommufd_object_destroy_user(idev->ictx, &idev->obj);
> }
> EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, IOMMUFD);
> -
Not yet
[ 574.162112] Unable to handle kernel NULL pointer dereference at
virtual address 0004
[ 574.261102] pc : iommufd_object_remove+0x7c/0x278
[ 574.265795] lr : iommufd_device_unbind+0x44/0x98
in check
Thanks
>
> Thanks
> Nicolin
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Mon, Oct 14, 2024 at 07:01:40PM -0700, Nicolin Chen wrote:
> On Tue, Oct 15, 2024 at 09:15:01AM +0800, Zhangfei Gao wrote:
>
> > > > iommufd_device_bind
> > > > iommufd_device_attach
> > > > iommufd_vdevice_alloc_ioctl
> > > >
> > > > iommufd_device_detach
> > > > iommufd_device_unbind // refcount check fail
> > > > iommufd_vdevice_destroy ref--
> > >
> > > Things should be symmetric. As you suspected, vdevice should be
> > > destroyed before iommufd_device_detach.
> >
> > I am trying based on your for_iommufd_viommu_p2-v3 branch, do you have
> > this issue?
> > In checking whether close fd before unbind?
>
> Oops, my bad. I will provide a fix.
This should fix the problem:
-
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 5fd3dd420290..13100cfea29d 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -277,6 +277,11 @@ EXPORT_SYMBOL_NS_GPL(iommufd_ctx_has_group, IOMMUFD);
*/
void iommufd_device_unbind(struct iommufd_device *idev)
{
+ mutex_lock(&idev->igroup->lock);
+ /* idev->vdev object should be destroyed prior, yet just in case.. */
+ if (idev->vdev)
+ iommufd_object_remove(idev->ictx, NULL, idev->vdev->obj.id, 0);
+ mutex_unlock(&idev->igroup->lock);
iommufd_object_destroy_user(idev->ictx, &idev->obj);
}
EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, IOMMUFD);
-
Thanks
Nicolin
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Tue, Oct 15, 2024 at 09:15:01AM +0800, Zhangfei Gao wrote: > > > iommufd_device_bind > > > iommufd_device_attach > > > iommufd_vdevice_alloc_ioctl > > > > > > iommufd_device_detach > > > iommufd_device_unbind // refcount check fail > > > iommufd_vdevice_destroy ref-- > > > > Things should be symmetric. As you suspected, vdevice should be > > destroyed before iommufd_device_detach. > > I am trying based on your for_iommufd_viommu_p2-v3 branch, do you have > this issue? > In checking whether close fd before unbind? Oops, my bad. I will provide a fix. Nicolin
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Mon, 14 Oct 2024 at 23:46, Nicolin Chen wrote:
>
> On Mon, Oct 14, 2024 at 03:58:55PM +0800, Zhangfei Gao wrote:
>
> > > > > > +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx
> > > > > > *ictx,
> > > > > > + size_t size,
> > > > > > + enum
> > > > > > iommufd_object_type type)
> > > > > > +{
> > > > > > + struct iommufd_object *obj;
> > > > > > + int rc;
> > > > > > +
> > > > > > + obj = kzalloc(size, GFP_KERNEL_ACCOUNT);
> > > > > > + if (!obj)
> > > > > > + return ERR_PTR(-ENOMEM);
> > > > > > + obj->type = type;
> > > > > > + /* Starts out bias'd by 1 until it is removed from the
> > > > > > xarray */
> > > > > > + refcount_set(&obj->shortterm_users, 1);
> > > > > > + refcount_set(&obj->users, 1);
> > > > >
> > > > > here set refcont 1
> > > > >
> > > > > iommufd_device_bind -> iommufd_object_alloc(ictx, idev,
> > > > > IOMMUFD_OBJ_DEVICE): refcont -> 1
> > > > > refcount_inc(&idev->obj.users); refcount -> 2
> > > > > will cause iommufd_device_unbind fail.
> > > > >
> > > > > May remove refcount_inc(&idev->obj.users) in iommufd_device_bind
> > > >
> > > > Hmm, why would it fail? Or is it failing on your system?
> > >
> > > Not sure, still in check, it may only be on my platform.
> > >
> > > it hit
> > > iommufd_object_remove:
> > > if (WARN_ON(obj != to_destroy))
> > >
> > > iommufd_device_bind refcount=2
> > > iommufd_device_attach refcount=3
> > > //still not sure which operation inc the count?
> > > iommufd_device_detach refcount=4
> > >
> >
> > Have a question,
> > when should iommufd_vdevice_destroy be called, before or after
> > iommufd_device_unbind.
>
> Before.
>
> > Now iommufd_vdevice_destroy (ref--) is after unbind, hits the if
> > (!refcount_dec_if_one(&obj->users)) check.
>
> Hmm, where do we have an iommufd_vdevice_destroy after unbind?
Looks like it is called by close fd?
[ 2480.909319] iommufd_vdevice_destroy+0xdc/0x168
[ 2480.909324] iommufd_fops_release+0xa4/0x140
[ 2480.909328] __fput+0xd0/0x2e0
[ 2480.909331] fput+0x1c/0x30
[ 2480.909333] task_work_run+0x78/0xe0
[ 2480.909337] do_exit+0x2fc/0xa50
[ 2480.909340] do_group_exit+0x3c/0xa0
[ 2480.909344] get_signal+0x96c/0x978
[ 2480.909346] do_signal+0x94/0x3a8
[ 2480.909348] do_notify_resume+0x100/0x190
>
> > iommufd_device_bind
> > iommufd_device_attach
> > iommufd_vdevice_alloc_ioctl
> >
> > iommufd_device_detach
> > iommufd_device_unbind // refcount check fail
> > iommufd_vdevice_destroy ref--
>
> Things should be symmetric. As you suspected, vdevice should be
> destroyed before iommufd_device_detach.
I am trying based on your for_iommufd_viommu_p2-v3 branch, do you have
this issue?
In checking whether close fd before unbind?
>
> A vdev is an object on top of a vIOMMU obj and an idev obj, so
> it takes a refcount from each of them. That's why idev couldn't
> unbind.
Thanks
>
> Thanks
> Nicolin
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Mon, Oct 14, 2024 at 03:58:55PM +0800, Zhangfei Gao wrote:
> > > > > +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx
> > > > > *ictx,
> > > > > + size_t size,
> > > > > + enum
> > > > > iommufd_object_type type)
> > > > > +{
> > > > > + struct iommufd_object *obj;
> > > > > + int rc;
> > > > > +
> > > > > + obj = kzalloc(size, GFP_KERNEL_ACCOUNT);
> > > > > + if (!obj)
> > > > > + return ERR_PTR(-ENOMEM);
> > > > > + obj->type = type;
> > > > > + /* Starts out bias'd by 1 until it is removed from the xarray
> > > > > */
> > > > > + refcount_set(&obj->shortterm_users, 1);
> > > > > + refcount_set(&obj->users, 1);
> > > >
> > > > here set refcont 1
> > > >
> > > > iommufd_device_bind -> iommufd_object_alloc(ictx, idev,
> > > > IOMMUFD_OBJ_DEVICE): refcont -> 1
> > > > refcount_inc(&idev->obj.users); refcount -> 2
> > > > will cause iommufd_device_unbind fail.
> > > >
> > > > May remove refcount_inc(&idev->obj.users) in iommufd_device_bind
> > >
> > > Hmm, why would it fail? Or is it failing on your system?
> >
> > Not sure, still in check, it may only be on my platform.
> >
> > it hit
> > iommufd_object_remove:
> > if (WARN_ON(obj != to_destroy))
> >
> > iommufd_device_bind refcount=2
> > iommufd_device_attach refcount=3
> > //still not sure which operation inc the count?
> > iommufd_device_detach refcount=4
> >
>
> Have a question,
> when should iommufd_vdevice_destroy be called, before or after
> iommufd_device_unbind.
Before.
> Now iommufd_vdevice_destroy (ref--) is after unbind, hits the if
> (!refcount_dec_if_one(&obj->users)) check.
Hmm, where do we have an iommufd_vdevice_destroy after unbind?
> iommufd_device_bind
> iommufd_device_attach
> iommufd_vdevice_alloc_ioctl
>
> iommufd_device_detach
> iommufd_device_unbind // refcount check fail
> iommufd_vdevice_destroy ref--
Things should be symmetric. As you suspected, vdevice should be
destroyed before iommufd_device_detach.
A vdev is an object on top of a vIOMMU obj and an idev obj, so
it takes a refcount from each of them. That's why idev couldn't
unbind.
Thanks
Nicolin
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
Hi, Nico
On Sat, 12 Oct 2024 at 18:18, Zhangfei Gao wrote:
>
> On Sat, 12 Oct 2024 at 12:49, Nicolin Chen wrote:
> >
> > On Sat, Oct 12, 2024 at 11:23:07AM +0800, Zhangfei Gao wrote:
> >
> > > > diff --git a/drivers/iommu/iommufd/viommu_api.c
> > > > b/drivers/iommu/iommufd/viommu_api.c
> > > > new file mode 100644
> > > > index ..c1731f080d6b
> > > > --- /dev/null
> > > > +++ b/drivers/iommu/iommufd/viommu_api.c
> > > > @@ -0,0 +1,57 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES
> > > > + */
> > > > +
> > > > +#include "iommufd_private.h"
> > > > +
> > > > +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx
> > > > *ictx,
> > > > + size_t size,
> > > > + enum
> > > > iommufd_object_type type)
> > > > +{
> > > > + struct iommufd_object *obj;
> > > > + int rc;
> > > > +
> > > > + obj = kzalloc(size, GFP_KERNEL_ACCOUNT);
> > > > + if (!obj)
> > > > + return ERR_PTR(-ENOMEM);
> > > > + obj->type = type;
> > > > + /* Starts out bias'd by 1 until it is removed from the xarray */
> > > > + refcount_set(&obj->shortterm_users, 1);
> > > > + refcount_set(&obj->users, 1);
> > >
> > > here set refcont 1
> > >
> > > iommufd_device_bind -> iommufd_object_alloc(ictx, idev,
> > > IOMMUFD_OBJ_DEVICE): refcont -> 1
> > > refcount_inc(&idev->obj.users); refcount -> 2
> > > will cause iommufd_device_unbind fail.
> > >
> > > May remove refcount_inc(&idev->obj.users) in iommufd_device_bind
> >
> > Hmm, why would it fail? Or is it failing on your system?
>
> Not sure, still in check, it may only be on my platform.
>
> it hit
> iommufd_object_remove:
> if (WARN_ON(obj != to_destroy))
>
> iommufd_device_bind refcount=2
> iommufd_device_attach refcount=3
> //still not sure which operation inc the count?
> iommufd_device_detach refcount=4
>
Have a question,
when should iommufd_vdevice_destroy be called, before or after
iommufd_device_unbind.
Now iommufd_vdevice_destroy (ref--) is after unbind, hits the if
(!refcount_dec_if_one(&obj->users)) check.
iommufd_device_bind
iommufd_device_attach
iommufd_vdevice_alloc_ioctl
iommufd_device_detach
iommufd_device_unbind // refcount check fail
iommufd_vdevice_destroy ref--
Thanks
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Sat, 12 Oct 2024 at 12:49, Nicolin Chen wrote:
>
> On Sat, Oct 12, 2024 at 11:23:07AM +0800, Zhangfei Gao wrote:
>
> > > diff --git a/drivers/iommu/iommufd/viommu_api.c
> > > b/drivers/iommu/iommufd/viommu_api.c
> > > new file mode 100644
> > > index ..c1731f080d6b
> > > --- /dev/null
> > > +++ b/drivers/iommu/iommufd/viommu_api.c
> > > @@ -0,0 +1,57 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES
> > > + */
> > > +
> > > +#include "iommufd_private.h"
> > > +
> > > +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx,
> > > + size_t size,
> > > + enum iommufd_object_type
> > > type)
> > > +{
> > > + struct iommufd_object *obj;
> > > + int rc;
> > > +
> > > + obj = kzalloc(size, GFP_KERNEL_ACCOUNT);
> > > + if (!obj)
> > > + return ERR_PTR(-ENOMEM);
> > > + obj->type = type;
> > > + /* Starts out bias'd by 1 until it is removed from the xarray */
> > > + refcount_set(&obj->shortterm_users, 1);
> > > + refcount_set(&obj->users, 1);
> >
> > here set refcont 1
> >
> > iommufd_device_bind -> iommufd_object_alloc(ictx, idev,
> > IOMMUFD_OBJ_DEVICE): refcont -> 1
> > refcount_inc(&idev->obj.users); refcount -> 2
> > will cause iommufd_device_unbind fail.
> >
> > May remove refcount_inc(&idev->obj.users) in iommufd_device_bind
>
> Hmm, why would it fail? Or is it failing on your system?
Not sure, still in check, it may only be on my platform.
it hit
iommufd_object_remove:
if (WARN_ON(obj != to_destroy))
iommufd_device_bind refcount=2
iommufd_device_attach refcount=3
//still not sure which operation inc the count?
iommufd_device_detach refcount=4
Thanks
>
> This patch doesn't change the function but only moved it..
>
> Thanks
> Nicolin
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Sat, Oct 12, 2024 at 11:23:07AM +0800, Zhangfei Gao wrote:
> > diff --git a/drivers/iommu/iommufd/viommu_api.c
> > b/drivers/iommu/iommufd/viommu_api.c
> > new file mode 100644
> > index ..c1731f080d6b
> > --- /dev/null
> > +++ b/drivers/iommu/iommufd/viommu_api.c
> > @@ -0,0 +1,57 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES
> > + */
> > +
> > +#include "iommufd_private.h"
> > +
> > +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx,
> > + size_t size,
> > + enum iommufd_object_type
> > type)
> > +{
> > + struct iommufd_object *obj;
> > + int rc;
> > +
> > + obj = kzalloc(size, GFP_KERNEL_ACCOUNT);
> > + if (!obj)
> > + return ERR_PTR(-ENOMEM);
> > + obj->type = type;
> > + /* Starts out bias'd by 1 until it is removed from the xarray */
> > + refcount_set(&obj->shortterm_users, 1);
> > + refcount_set(&obj->users, 1);
>
> here set refcont 1
>
> iommufd_device_bind -> iommufd_object_alloc(ictx, idev,
> IOMMUFD_OBJ_DEVICE): refcont -> 1
> refcount_inc(&idev->obj.users); refcount -> 2
> will cause iommufd_device_unbind fail.
>
> May remove refcount_inc(&idev->obj.users) in iommufd_device_bind
Hmm, why would it fail? Or is it failing on your system?
This patch doesn't change the function but only moved it..
Thanks
Nicolin
Re: [PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
On Thu, 10 Oct 2024 at 00:40, Nicolin Chen wrote:
>
> Add a new IOMMUFD_OBJ_VIOMMU with an iommufd_viommu structure to represent
> a slice of physical IOMMU device passed to or shared with a user space VM.
> This slice, now a vIOMMU object, is a group of virtualization resources of
> a physical IOMMU's, such as:
> - Security namespace for guest owned ID, e.g. guest-controlled cache tags
> - Access to a sharable nesting parent pagetable across physical IOMMUs
> - Virtualization of various platforms IDs, e.g. RIDs and others
> - Delivery of paravirtualized invalidation
> - Direct assigned invalidation queues
> - Direct assigned interrupts
> - Non-affiliated event reporting
>
> Add a new viommu_alloc op in iommu_ops, for drivers to allocate their own
> vIOMMU structures. And this allocation also needs a free(), so add struct
> iommufd_viommu_ops.
>
> To simplify a vIOMMU allocation, provide a iommufd_viommu_alloc() helper.
> It's suggested that a driver should embed a core-level viommu structure in
> its driver-level viommu struct and call the iommufd_viommu_alloc() helper,
> meanwhile the driver can also implement a viommu ops:
> struct my_driver_viommu {
> struct iommufd_viommu core;
> /* driver-owned properties/features */
>
> };
>
> static const struct iommufd_viommu_ops my_driver_viommu_ops = {
> .free = my_driver_viommu_free,
> /* future ops for virtualization features */
>
> };
>
> static struct iommufd_viommu my_driver_viommu_alloc(...)
> {
> struct my_driver_viommu *my_viommu =
> iommufd_viommu_alloc(ictx, my_driver_viommu, core,
> my_driver_viommu_ops);
> /* Init my_viommu and related HW feature */
>
> return &my_viommu->core;
> }
>
> static struct iommu_domain_ops my_driver_domain_ops = {
>
> .viommu_alloc = my_driver_viommu_alloc,
> };
>
> To make the Kernel config work between a driver and the iommufd core, put
> the for-driver allocation helpers into a new viommu_api file building with
> CONFIG_IOMMUFD_DRIVER.
>
> Signed-off-by: Nicolin Chen
> diff --git a/drivers/iommu/iommufd/viommu_api.c
> b/drivers/iommu/iommufd/viommu_api.c
> new file mode 100644
> index ..c1731f080d6b
> --- /dev/null
> +++ b/drivers/iommu/iommufd/viommu_api.c
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES
> + */
> +
> +#include "iommufd_private.h"
> +
> +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx,
> + size_t size,
> + enum iommufd_object_type type)
> +{
> + struct iommufd_object *obj;
> + int rc;
> +
> + obj = kzalloc(size, GFP_KERNEL_ACCOUNT);
> + if (!obj)
> + return ERR_PTR(-ENOMEM);
> + obj->type = type;
> + /* Starts out bias'd by 1 until it is removed from the xarray */
> + refcount_set(&obj->shortterm_users, 1);
> + refcount_set(&obj->users, 1);
here set refcont 1
iommufd_device_bind -> iommufd_object_alloc(ictx, idev,
IOMMUFD_OBJ_DEVICE): refcont -> 1
refcount_inc(&idev->obj.users); refcount -> 2
will cause iommufd_device_unbind fail.
May remove refcount_inc(&idev->obj.users) in iommufd_device_bind
Thanks
[PATCH v3 03/11] iommufd: Introduce IOMMUFD_OBJ_VIOMMU and its related struct
Add a new IOMMUFD_OBJ_VIOMMU with an iommufd_viommu structure to represent
a slice of physical IOMMU device passed to or shared with a user space VM.
This slice, now a vIOMMU object, is a group of virtualization resources of
a physical IOMMU's, such as:
- Security namespace for guest owned ID, e.g. guest-controlled cache tags
- Access to a sharable nesting parent pagetable across physical IOMMUs
- Virtualization of various platforms IDs, e.g. RIDs and others
- Delivery of paravirtualized invalidation
- Direct assigned invalidation queues
- Direct assigned interrupts
- Non-affiliated event reporting
Add a new viommu_alloc op in iommu_ops, for drivers to allocate their own
vIOMMU structures. And this allocation also needs a free(), so add struct
iommufd_viommu_ops.
To simplify a vIOMMU allocation, provide a iommufd_viommu_alloc() helper.
It's suggested that a driver should embed a core-level viommu structure in
its driver-level viommu struct and call the iommufd_viommu_alloc() helper,
meanwhile the driver can also implement a viommu ops:
struct my_driver_viommu {
struct iommufd_viommu core;
/* driver-owned properties/features */
};
static const struct iommufd_viommu_ops my_driver_viommu_ops = {
.free = my_driver_viommu_free,
/* future ops for virtualization features */
};
static struct iommufd_viommu my_driver_viommu_alloc(...)
{
struct my_driver_viommu *my_viommu =
iommufd_viommu_alloc(ictx, my_driver_viommu, core,
my_driver_viommu_ops);
/* Init my_viommu and related HW feature */
return &my_viommu->core;
}
static struct iommu_domain_ops my_driver_domain_ops = {
.viommu_alloc = my_driver_viommu_alloc,
};
To make the Kernel config work between a driver and the iommufd core, put
the for-driver allocation helpers into a new viommu_api file building with
CONFIG_IOMMUFD_DRIVER.
Signed-off-by: Nicolin Chen
---
drivers/iommu/iommufd/Makefile | 2 +-
drivers/iommu/iommufd/iommufd_private.h | 1 +
include/linux/iommu.h | 14 ++
include/linux/iommufd.h | 43 +++
drivers/iommu/iommufd/main.c| 32 --
drivers/iommu/iommufd/viommu_api.c | 57 +
6 files changed, 116 insertions(+), 33 deletions(-)
create mode 100644 drivers/iommu/iommufd/viommu_api.c
diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile
index cf4605962bea..93daedd7e5c8 100644
--- a/drivers/iommu/iommufd/Makefile
+++ b/drivers/iommu/iommufd/Makefile
@@ -12,4 +12,4 @@ iommufd-y := \
iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o
obj-$(CONFIG_IOMMUFD) += iommufd.o
-obj-$(CONFIG_IOMMUFD_DRIVER) += iova_bitmap.o
+obj-$(CONFIG_IOMMUFD_DRIVER) += iova_bitmap.o viommu_api.o
diff --git a/drivers/iommu/iommufd/iommufd_private.h
b/drivers/iommu/iommufd/iommufd_private.h
index f2f3a906eac9..6a364073f699 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -131,6 +131,7 @@ enum iommufd_object_type {
IOMMUFD_OBJ_IOAS,
IOMMUFD_OBJ_ACCESS,
IOMMUFD_OBJ_FAULT,
+ IOMMUFD_OBJ_VIOMMU,
#ifdef CONFIG_IOMMUFD_TEST
IOMMUFD_OBJ_SELFTEST,
#endif
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index c8d18f5f644e..3a50f57b0861 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -42,6 +42,8 @@ struct notifier_block;
struct iommu_sva;
struct iommu_dma_cookie;
struct iommu_fault_param;
+struct iommufd_ctx;
+struct iommufd_viommu;
#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
@@ -542,6 +544,13 @@ static inline int __iommu_copy_struct_from_user_array(
* @remove_dev_pasid: Remove any translation configurations of a specific
*pasid, so that any DMA transactions with this pasid
*will be blocked by the hardware.
+ * @viommu_alloc: Allocate an iommufd_viommu on an @iommu_dev as the group of
+ *virtualization resources shared/passed to user space IOMMU
+ *instance. Associate it with a nesting parent @domain. The
+ *@viommu_type must be defined in include/uapi/linux/iommufd.h
+ *It is suggested to call iommufd_viommu_alloc() helper for
+ *a bundled allocation of the core and the driver structures,
+ *using the given @ictx pointer.
* @pgsize_bitmap: bitmap of all possible supported page sizes
* @owner: Driver module providing these ops
* @identity_domain: An always available, always attachable identity
@@ -591,6 +600,11 @@ struct iommu_ops {
void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid,
struct iommu_domain *domain);
+ struct iommufd_viommu *(*viommu_alloc
