| Message ID | 5288cec9804e7e394be3b7de6b246d8ca9c4792a.1746757630.git.nicolinc@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | iommufd: Add vIOMMU infrastructure (Part-4 HW QUEUE) | expand |
On Fri, May 16, 2025 at 10:28:45AM -0300, Jason Gunthorpe wrote: > On Thu, May 15, 2025 at 07:05:45PM -0700, Nicolin Chen wrote: > > > Otherwise, they would be something like: > > // in my_viommu_alloc() > > my_viommu = iommufd_viommu_alloc(ucmd, ...); > > iommufd_viommu_alloc_mmap(ucmd->ictx, my_viommu, ...); > > iommufd_viommu_destroy_mmap(ucmd->ictx, my_viommu, ...); > > // in my_viommu_destory() > > iommufd_viommu_destroy_mmap(my_viommu->ictx, my_viommu ...); > > Yes, this is how the other objects work.. > > > That being said, I ended up doing something more in this patch > > for the ucmd rework and Kevin's comments: > > > > diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h > > index 98fe5a49c9606..ddd144031af5d 100644 > > --- a/drivers/iommu/iommufd/iommufd_private.h > > +++ b/drivers/iommu/iommufd/iommufd_private.h > > @@ -136,14 +136,6 @@ int iopt_pin_pages(struct io_pagetable *iopt, unsigned long iova, > > void iopt_unpin_pages(struct io_pagetable *iopt, unsigned long iova, > > unsigned long length); > > > > -struct iommufd_ucmd { > > - struct iommufd_ctx *ictx; > > - void __user *ubuffer; > > - u32 user_size; > > - void *cmd; > > - struct iommufd_object *alloced_obj; > > -}; > > You don't need to move this unless you are using inlines. Just use a > forward declaration. Since we forward ucmd now, ictx is in the ucmd so we need this structure for: - if (!IS_ERR(ret)) \ + if (!IS_ERR(ret)) { \ ret->member.ops = viommu_ops; \ + ret->member.ictx = ucmd->ictx; \ + } \ Or, we could have a smaller: #define ucmd_to_ictx(ucmd) (struct iommufd_ctx *)(ucmd) and add a line of comments in struct iommufd_ucmd: struct iommufd_ucmd { - struct iommufd_ctx *ictx; + struct iommufd_ctx *ictx; /* Do not move; must be the first member */ void __user *ubuffer; u32 user_size; void *cmd; struct iommufd_object *alloced_obj; }; Thanks Nicolin
On Mon, May 26, 2025 at 10:30:46AM -0300, Jason Gunthorpe wrote: > On Fri, May 16, 2025 at 01:56:26PM -0700, Nicolin Chen wrote: > > > > You don't need to move this unless you are using inlines. Just use a > > > forward declaration. > > > > Since we forward ucmd now, ictx is in the ucmd so we need this > > structure for: > > > > - if (!IS_ERR(ret)) \ > > + if (!IS_ERR(ret)) { \ > > ret->member.ops = viommu_ops; \ > > + ret->member.ictx = ucmd->ictx; \ > > + } \ > > De-inline more of that function probably.. > > Also seem my other remarks about not storing ictx so much.. I found that all other ictx pointers in vdev/hw_queue are unused, as the core simply gets an ictx from their viommu pointers. This means that only this viommu allocator here needs such a storing. With that, how about a change like this v.s. inline: [ Sol-1: Store ucmd ] ---------------------------------------------------------------------------- diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c index 2b30627d1d8ed..513af9ba04f17 100644 --- a/drivers/iommu/iommufd/viommu.c +++ b/drivers/iommu/iommufd/viommu.c @@ -60,9 +60,15 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd) goto out_put_hwpt; } + /* Verify if the driver correctly calls iommufd_viommu_alloc() */ + static_assert(offsetof(typeof(*ucmd), ictx) == 0); + if (WARN_ON_ONCE(viommu->ictx != ucmd->ictx)) { + rc = -EINVAL; + goto out_put_hwpt; + } + xa_init(&viommu->vdevs); viommu->type = cmd->type; - viommu->ictx = ucmd->ictx; viommu->hwpt = hwpt_paging; refcount_inc(&viommu->hwpt->common.obj.users); INIT_LIST_HEAD(&viommu->veventqs); diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index d74c97feb9b52..6843c748a6f74 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -104,7 +104,10 @@ void iommufd_ctx_get(struct iommufd_ctx *ictx); struct iommufd_viommu { struct iommufd_object obj; - struct iommufd_ctx *ictx; + union { + struct iommufd_ctx *ictx; + struct iommufd_ucmd *ucmd; /* for allocation only */ + }; struct iommu_device *iommu_dev; struct iommufd_hwpt_paging *hwpt; @@ -262,8 +265,10 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu, \ ret = (drv_struct *)__iommufd_object_alloc_ucmd( \ ucmd, ret, IOMMUFD_OBJ_VIOMMU, member.obj); \ - if (!IS_ERR(ret)) \ + if (!IS_ERR(ret)) { \ ret->member.ops = viommu_ops; \ + ret->member.ucmd = ucmd; \ + } \ ret; \ }) #endif ---------------------------------------------------------------------------- [ Sol-2: De-inline ] ---------------------------------------------------------------------------- diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c index 28dec9e097204..0adb1a552e457 100644 --- a/drivers/iommu/iommufd/driver.c +++ b/drivers/iommu/iommufd/driver.c @@ -54,6 +54,23 @@ struct iommufd_object *_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, } EXPORT_SYMBOL_NS_GPL(_iommufd_object_alloc_ucmd, "IOMMUFD"); +struct iommufd_viommu * +_iommufd_viommu_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size, + const struct iommufd_viommu_ops *viommu_ops) +{ + struct iommufd_viommu *viommu; + + static_assert(offsetof(typeof(*viommu), obj) == 0); + viommu = (struct iommufd_viommu *)_iommufd_object_alloc_ucmd( + ucmd, size, IOMMUFD_OBJ_VIOMMU); + if (IS_ERR(viommu)) + return viommu; + viommu->ops = viommu_ops; + viommu->ictx = ucmd->ictx; + return viommu; +} +EXPORT_SYMBOL_NS_GPL(_iommufd_viommu_alloc_ucmd, "IOMMUFD"); + /* Caller should xa_lock(&viommu->vdevs) to protect the return value */ struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu, unsigned long vdev_id) diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c index 2b30627d1d8ed..259fcfd85b1bd 100644 --- a/drivers/iommu/iommufd/viommu.c +++ b/drivers/iommu/iommufd/viommu.c @@ -60,9 +60,14 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd) goto out_put_hwpt; } + /* Verify if the driver correctly calls iommufd_viommu_alloc() */ + if (WARN_ON_ONCE(viommu->ictx != ucmd->ictx)) { + rc = -EINVAL; + goto out_put_hwpt; + } + xa_init(&viommu->vdevs); viommu->type = cmd->type; - viommu->ictx = ucmd->ictx; viommu->hwpt = hwpt_paging; refcount_inc(&viommu->hwpt->common.obj.users); INIT_LIST_HEAD(&viommu->veventqs); diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index d74c97feb9b52..50ac3a5703520 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -209,6 +209,9 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, struct iommufd_object * _iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size, enum iommufd_object_type type); +struct iommufd_viommu * +_iommufd_viommu_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size, + const struct iommufd_viommu_ops *viommu_ops); struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu, unsigned long vdev_id); int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu, @@ -231,6 +234,13 @@ _iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size, return ERR_PTR(-EOPNOTSUPP); } +static inline struct iommufd_viommu * +_iommufd_viommu_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size, + const struct iommufd_viommu_ops *viommu_ops) +{ + return ERR_PTR(-EOPNOTSUPP); +} + static inline struct device * iommufd_viommu_find_dev(struct iommufd_viommu *viommu, unsigned long vdev_id) { @@ -257,13 +267,7 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu, * the iommufd core. The free op will be called prior to freeing the memory. */ #define iommufd_viommu_alloc(ucmd, drv_struct, member, viommu_ops) \ - ({ \ - drv_struct *ret; \ - \ - ret = (drv_struct *)__iommufd_object_alloc_ucmd( \ - ucmd, ret, IOMMUFD_OBJ_VIOMMU, member.obj); \ - if (!IS_ERR(ret)) \ - ret->member.ops = viommu_ops; \ - ret; \ - }) + container_of(_iommufd_viommu_alloc_ucmd( \ + ucmd, sizeof(drv_struct), viommu_ops), \ + typeof(drv_struct), member) #endif ---------------------------------------------------------------------------- Thanks Nicolin
On Tue, May 27, 2025 at 11:41:19AM -0700, Nicolin Chen wrote: > On Mon, May 26, 2025 at 10:30:46AM -0300, Jason Gunthorpe wrote: > > On Fri, May 16, 2025 at 01:56:26PM -0700, Nicolin Chen wrote: > > > > > > You don't need to move this unless you are using inlines. Just use a > > > > forward declaration. > > > > > > Since we forward ucmd now, ictx is in the ucmd so we need this > > > structure for: > > > > > > - if (!IS_ERR(ret)) \ > > > + if (!IS_ERR(ret)) { \ > > > ret->member.ops = viommu_ops; \ > > > + ret->member.ictx = ucmd->ictx; \ > > > + } \ > > > > De-inline more of that function probably.. > > > > Also seem my other remarks about not storing ictx so much.. > > I found that all other ictx pointers in vdev/hw_queue are unused, > as the core simply gets an ictx from their viommu pointers. This > means that only this viommu allocator here needs such a storing. > > With that, how about a change like this v.s. inline: So I think the other suggestion with init vs alloc supersedes this and makes it all pretty clean? Jason
On Fri, May 30, 2025 at 03:27:23PM -0300, Jason Gunthorpe wrote: > On Tue, May 27, 2025 at 11:41:19AM -0700, Nicolin Chen wrote: > > On Mon, May 26, 2025 at 10:30:46AM -0300, Jason Gunthorpe wrote: > > > On Fri, May 16, 2025 at 01:56:26PM -0700, Nicolin Chen wrote: > > > > > > > > You don't need to move this unless you are using inlines. Just use a > > > > > forward declaration. > > > > > > > > Since we forward ucmd now, ictx is in the ucmd so we need this > > > > structure for: > > > > > > > > - if (!IS_ERR(ret)) \ > > > > + if (!IS_ERR(ret)) { \ > > > > ret->member.ops = viommu_ops; \ > > > > + ret->member.ictx = ucmd->ictx; \ > > > > + } \ > > > > > > De-inline more of that function probably.. > > > > > > Also seem my other remarks about not storing ictx so much.. > > > > I found that all other ictx pointers in vdev/hw_queue are unused, > > as the core simply gets an ictx from their viommu pointers. This > > means that only this viommu allocator here needs such a storing. > > > > With that, how about a change like this v.s. inline: > > So I think the other suggestion with init vs alloc supersedes this and > makes it all pretty clean? I believe so! Nicolin
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index 422eda95d19e..dc6535e848df 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -262,8 +262,10 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu, static_assert(offsetof(drv_struct, member.obj) == 0); \ ret = (drv_struct *)_iommufd_object_alloc( \ ictx, sizeof(drv_struct), IOMMUFD_OBJ_VIOMMU); \ - if (!IS_ERR(ret)) \ + if (!IS_ERR(ret)) { \ ret->member.ops = viommu_ops; \ + ret->member.ictx = ictx; \ + } \ ret; \ })
When an IOMMU driver calls iommufd_viommu_alloc(), it must pass in an ictx pointer as the underlying _iommufd_object_alloc() helper function requires that to allocate a new object. However, neither the iommufd_viommu_alloc() nor its underlying _iommufd_object_alloc() saves the ictx in the allocated viommu object, although viommu could hold an ictx pointer. When the IOMMU driver wants to use another iommufd function passing in the allocated viommu, it could have avoided passing in the ictx pointer again, if viommu->ictx is valid. Save ictx to viommu->ictx in the iommufd_viommu_alloc(), in order to ease a new vIOMMU-based helper that would then get the ictx from viommu->ictx. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- include/linux/iommufd.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)