| Message ID | 4f5ed9d75c23a503ffe672a85cf9010569652794.1729897352.git.nicolinc@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommufd: Add vIOMMU infrastructure (Part-1) | expand |
On Mon, 28 Oct 2024 at 11:24, Zhangfei Gao <zhangfei.gao@linaro.org> wrote: > By the way, has qemu changed compared with v3? > I still got a hardware error in this version, in check Found iommufd_viommu_p2-v5 misses some patches, Simply tested ok with iommufd_viommu_p2-v5-with-rmr, (with some hacks) Thanks
On Mon, Oct 28, 2024 at 07:52:10AM -0700, Nicolin Chen wrote: > On Mon, Oct 28, 2024 at 10:03:09AM -0300, Jason Gunthorpe wrote: > > On Mon, Oct 28, 2024 at 11:24:10AM +0800, Zhangfei Gao wrote: > > > > > > +/** > > > > + * iommufd_viommu_alloc_hwpt_nested() - Get a hwpt_nested for a vIOMMU > > > > + * @viommu: vIOMMU ojbect to associate the hwpt_nested/domain with > > > > + * @user_data: user_data pointer. Must be valid > > > > + * > > > > + * Allocate a new IOMMU_DOMAIN_NESTED for a vIOMMU and return it as a NESTED > > > > + * hw_pagetable. > > > > + */ > > > > +static struct iommufd_hwpt_nested * > > > > +iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags, > > > > + const struct iommu_user_data *user_data) > > > > +{ > > > > + struct iommufd_hwpt_nested *hwpt_nested; > > > > + struct iommufd_hw_pagetable *hwpt; > > > > + int rc; > > > > + > > > > + if (flags) > > > > + return ERR_PTR(-EOPNOTSUPP); > > > > > > This check should be removed. > > > > > > When a user page fault is required, IOMMU_HWPT_FAULT_ID_VALID is set. > > > if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) { > > > > It can't just be removed.. > > > > I suspect that IOMMU_HWPT_FAULT_ID_VALID should not be set on the > > nested domain but on the parent? > > By giving another look, > > In iommufd_hwpt_paging_alloc(), we reject IOMMU_HWPT_FAULT_ID_VALID: > const u32 valid_flags = IOMMU_HWPT_ALLOC_NEST_PARENT | > IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > ... > if (flags & ~valid_flags) > return ERR_PTR(-EOPNOTSUPP); > > In iommufd_hwpt_nested_alloc(), we mask the flag away: > if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) || > !user_data->len || !ops->domain_alloc_user) > return ERR_PTR(-EOPNOTSUPP); > ... > hwpt->domain = ops->domain_alloc_user(idev->dev, > flags & ~IOMMU_HWPT_FAULT_ID_VALID, > parent->common.domain, user_data); > > Then, in the common function it has a section of > if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) { > ... > > It seems that this IOMMU_HWPT_FAULT_ID_VALID is for nested domains? OK, but ARM should be blocking it since it doesn't work there. I think we made some error here, it should have been passed in flags to the drivers and only intel should have accepted it. This suggests we should send flags down the viommu alloc domain path too. Jason
On Tue, Oct 29, 2024 at 12:27:46PM -0300, Jason Gunthorpe wrote: > On Mon, Oct 28, 2024 at 07:52:10AM -0700, Nicolin Chen wrote: > > On Mon, Oct 28, 2024 at 10:03:09AM -0300, Jason Gunthorpe wrote: > > In iommufd_hwpt_paging_alloc(), we reject IOMMU_HWPT_FAULT_ID_VALID: > > const u32 valid_flags = IOMMU_HWPT_ALLOC_NEST_PARENT | > > IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > > ... > > if (flags & ~valid_flags) > > return ERR_PTR(-EOPNOTSUPP); > > > > In iommufd_hwpt_nested_alloc(), we mask the flag away: > > if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) || > > !user_data->len || !ops->domain_alloc_user) > > return ERR_PTR(-EOPNOTSUPP); > > ... > > hwpt->domain = ops->domain_alloc_user(idev->dev, > > flags & ~IOMMU_HWPT_FAULT_ID_VALID, > > parent->common.domain, user_data); > > > > Then, in the common function it has a section of > > if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) { > > ... > > > > It seems that this IOMMU_HWPT_FAULT_ID_VALID is for nested domains? > > OK, but ARM should be blocking it since it doesn't work there. > > I think we made some error here, it should have been passed in flags > to the drivers and only intel should have accepted it. Trying to limit changes here since two parts are already quite large, I think a separate series fixing that would be nicer? > This suggests we should send flags down the viommu alloc domain path too. Ack. Will pass it in. Thanks Nicolin
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 9adf8d616796..8c9ab35eaea5 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -288,6 +288,7 @@ struct iommufd_hwpt_paging { struct iommufd_hwpt_nested { struct iommufd_hw_pagetable common; struct iommufd_hwpt_paging *parent; + struct iommufd_viommu *viommu; }; static inline bool hwpt_is_paging(struct iommufd_hw_pagetable *hwpt) diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h index 3d320d069654..717659b9fdce 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -430,7 +430,7 @@ enum iommu_hwpt_data_type { * @size: sizeof(struct iommu_hwpt_alloc) * @flags: Combination of enum iommufd_hwpt_alloc_flags * @dev_id: The device to allocate this HWPT for - * @pt_id: The IOAS or HWPT to connect this HWPT to + * @pt_id: The IOAS or HWPT or vIOMMU to connect this HWPT to * @out_hwpt_id: The ID of the new HWPT * @__reserved: Must be 0 * @data_type: One of enum iommu_hwpt_data_type @@ -449,11 +449,13 @@ enum iommu_hwpt_data_type { * IOMMU_HWPT_DATA_NONE. The HWPT can be allocated as a parent HWPT for a * nesting configuration by passing IOMMU_HWPT_ALLOC_NEST_PARENT via @flags. * - * A user-managed nested HWPT will be created from a given parent HWPT via - * @pt_id, in which the parent HWPT must be allocated previously via the - * same ioctl from a given IOAS (@pt_id). In this case, the @data_type - * must be set to a pre-defined type corresponding to an I/O page table - * type supported by the underlying IOMMU hardware. + * A user-managed nested HWPT will be created from a given vIOMMU (wrapping a + * parent HWPT) or a parent HWPT via @pt_id, in which the parent HWPT must be + * allocated previously via the same ioctl from a given IOAS (@pt_id). In this + * case, the @data_type must be set to a pre-defined type corresponding to an + * I/O page table type supported by the underlying IOMMU hardware. The device + * via @dev_id and the vIOMMU via @pt_id must be associated to the same IOMMU + * instance. * * If the @data_type is set to IOMMU_HWPT_DATA_NONE, @data_len and * @data_uptr should be zero. Otherwise, both @data_len and @data_uptr diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index d06bf6e6c19f..1df5d40c93df 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -57,7 +57,10 @@ void iommufd_hwpt_nested_destroy(struct iommufd_object *obj) container_of(obj, struct iommufd_hwpt_nested, common.obj); __iommufd_hwpt_destroy(&hwpt_nested->common); - refcount_dec(&hwpt_nested->parent->common.obj.users); + if (hwpt_nested->viommu) + refcount_dec(&hwpt_nested->viommu->obj.users); + else + refcount_dec(&hwpt_nested->parent->common.obj.users); } void iommufd_hwpt_nested_abort(struct iommufd_object *obj) @@ -260,6 +263,56 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx, return ERR_PTR(rc); } +/** + * iommufd_viommu_alloc_hwpt_nested() - Get a hwpt_nested for a vIOMMU + * @viommu: vIOMMU ojbect to associate the hwpt_nested/domain with + * @user_data: user_data pointer. Must be valid + * + * Allocate a new IOMMU_DOMAIN_NESTED for a vIOMMU and return it as a NESTED + * hw_pagetable. + */ +static struct iommufd_hwpt_nested * +iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags, + const struct iommu_user_data *user_data) +{ + struct iommufd_hwpt_nested *hwpt_nested; + struct iommufd_hw_pagetable *hwpt; + int rc; + + if (flags) + return ERR_PTR(-EOPNOTSUPP); + if (!viommu->ops || !viommu->ops->alloc_domain_nested) + return ERR_PTR(-EOPNOTSUPP); + + hwpt_nested = __iommufd_object_alloc( + viommu->ictx, hwpt_nested, IOMMUFD_OBJ_HWPT_NESTED, common.obj); + if (IS_ERR(hwpt_nested)) + return ERR_CAST(hwpt_nested); + hwpt = &hwpt_nested->common; + + hwpt_nested->viommu = viommu; + hwpt_nested->parent = viommu->hwpt; + refcount_inc(&viommu->obj.users); + + hwpt->domain = viommu->ops->alloc_domain_nested(viommu, user_data); + if (IS_ERR(hwpt->domain)) { + rc = PTR_ERR(hwpt->domain); + hwpt->domain = NULL; + goto out_abort; + } + hwpt->domain->owner = viommu->iommu_dev->ops; + + if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) { + rc = -EINVAL; + goto out_abort; + } + return hwpt_nested; + +out_abort: + iommufd_object_abort_and_destroy(viommu->ictx, &hwpt->obj); + return ERR_PTR(rc); +} + int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) { struct iommu_hwpt_alloc *cmd = ucmd->cmd; @@ -316,6 +369,22 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) goto out_unlock; } hwpt = &hwpt_nested->common; + } else if (pt_obj->type == IOMMUFD_OBJ_VIOMMU) { + struct iommufd_hwpt_nested *hwpt_nested; + struct iommufd_viommu *viommu; + + viommu = container_of(pt_obj, struct iommufd_viommu, obj); + if (viommu->iommu_dev != __iommu_get_iommu_dev(idev->dev)) { + rc = -EINVAL; + goto out_unlock; + } + hwpt_nested = iommufd_viommu_alloc_hwpt_nested( + viommu, cmd->flags, &user_data); + if (IS_ERR(hwpt_nested)) { + rc = PTR_ERR(hwpt_nested); + goto out_unlock; + } + hwpt = &hwpt_nested->common; } else { rc = -EINVAL; goto out_put_pt;
Now a vIOMMU holds a shareable nesting parent HWPT. So, it can act like that nesting parent HWPT to allocate a nested HWPT. Support that in the IOMMU_HWPT_ALLOC ioctl handler, and update its kdoc. Also, add an iommufd_hwpt_nested_alloc_for_viommu helper to allocate a nested HWPT for a vIOMMU object. Since a vIOMMU object holds the parent hwpt's refcount already, increase the refcount of the vIOMMU only. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/iommufd/iommufd_private.h | 1 + include/uapi/linux/iommufd.h | 14 ++--- drivers/iommu/iommufd/hw_pagetable.c | 71 ++++++++++++++++++++++++- 3 files changed, 79 insertions(+), 7 deletions(-)