| Message ID | b0ee53af3f59602834e67ddf86c748ca304da175.1712978213.git.nicolinc@nvidia.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add Tegra241 (Grace) CMDQV Support (part 2/2) | expand |
On Fri, Apr 12, 2024 at 08:47:09PM -0700, Nicolin Chen wrote: > +/** > + * struct iommu_vqueue_alloc - ioctl(IOMMU_VQUEUE_ALLOC) > + * @size: sizeof(struct iommu_vqueue_alloc) > + * @flags: Must be 0 > + * @viommu_id: viommu ID to associate the virtual queue with > + * @out_vqueue_id: The ID of the new virtual queue > + * @data_type: One of enum iommu_vqueue_data_type > + * @data_len: Length of the type specific data > + * @data_uptr: User pointer to the type specific data > + * > + * Allocate an virtual queue object for driver-specific HW-accelerated queue > + */ > + > +struct iommu_vqueue_alloc { > + __u32 size; > + __u32 flags; > + __u32 viommu_id; > + __u32 out_vqueue_id; > + __u32 data_type; > + __u32 data_len; > + __aligned_u64 data_uptr; Some of the iommus will want an IPA here not a user pointer. I think it is fine API wise, we'd just add a flag to indicate data_uptr is an IPA. Jason
On Sun, May 12, 2024 at 12:02:21PM -0300, Jason Gunthorpe wrote: > On Fri, Apr 12, 2024 at 08:47:09PM -0700, Nicolin Chen wrote: > > > +/** > > + * struct iommu_vqueue_alloc - ioctl(IOMMU_VQUEUE_ALLOC) > > + * @size: sizeof(struct iommu_vqueue_alloc) > > + * @flags: Must be 0 > > + * @viommu_id: viommu ID to associate the virtual queue with > > + * @out_vqueue_id: The ID of the new virtual queue > > + * @data_type: One of enum iommu_vqueue_data_type > > + * @data_len: Length of the type specific data > > + * @data_uptr: User pointer to the type specific data > > + * > > + * Allocate an virtual queue object for driver-specific HW-accelerated queue > > + */ > > + > > +struct iommu_vqueue_alloc { > > + __u32 size; > > + __u32 flags; > > + __u32 viommu_id; > > + __u32 out_vqueue_id; > > + __u32 data_type; > > + __u32 data_len; > > + __aligned_u64 data_uptr; > > Some of the iommus will want an IPA here not a user pointer. I think > it is fine API wise, we'd just add a flag to indicate data_uptr is an > IPA. Ack. Nicolin
On Sun, May 12, 2024 at 09:41:19PM -0700, Nicolin Chen wrote: > On Sun, May 12, 2024 at 12:02:21PM -0300, Jason Gunthorpe wrote: > > On Fri, Apr 12, 2024 at 08:47:09PM -0700, Nicolin Chen wrote: > > > > > +/** > > > + * struct iommu_vqueue_alloc - ioctl(IOMMU_VQUEUE_ALLOC) > > > + * @size: sizeof(struct iommu_vqueue_alloc) > > > + * @flags: Must be 0 > > > + * @viommu_id: viommu ID to associate the virtual queue with > > > + * @out_vqueue_id: The ID of the new virtual queue > > > + * @data_type: One of enum iommu_vqueue_data_type > > > + * @data_len: Length of the type specific data > > > + * @data_uptr: User pointer to the type specific data > > > + * > > > + * Allocate an virtual queue object for driver-specific HW-accelerated queue > > > + */ > > > + > > > +struct iommu_vqueue_alloc { > > > + __u32 size; > > > + __u32 flags; > > > + __u32 viommu_id; > > > + __u32 out_vqueue_id; > > > + __u32 data_type; > > > + __u32 data_len; > > > + __aligned_u64 data_uptr; > > > > Some of the iommus will want an IPA here not a user pointer. I think > > it is fine API wise, we'd just add a flag to indicate data_uptr is an > > IPA. > > Ack. To be clear, add a flag someday, no change needed Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Sunday, May 12, 2024 11:02 PM > > On Fri, Apr 12, 2024 at 08:47:09PM -0700, Nicolin Chen wrote: > > > +/** > > + * struct iommu_vqueue_alloc - ioctl(IOMMU_VQUEUE_ALLOC) > > + * @size: sizeof(struct iommu_vqueue_alloc) > > + * @flags: Must be 0 > > + * @viommu_id: viommu ID to associate the virtual queue with > > + * @out_vqueue_id: The ID of the new virtual queue > > + * @data_type: One of enum iommu_vqueue_data_type > > + * @data_len: Length of the type specific data > > + * @data_uptr: User pointer to the type specific data > > + * > > + * Allocate an virtual queue object for driver-specific HW-accelerated > queue > > + */ > > + > > +struct iommu_vqueue_alloc { > > + __u32 size; > > + __u32 flags; > > + __u32 viommu_id; > > + __u32 out_vqueue_id; > > + __u32 data_type; > > + __u32 data_len; > > + __aligned_u64 data_uptr; > > Some of the iommus will want an IPA here not a user pointer. I think > it is fine API wise, we'd just add a flag to indicate data_uptr is an > IPA. > Presumably each driver will create its own type data which can include any IPA field as vcmdq_base in this patch?
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Saturday, April 13, 2024 11:47 AM > + > +/** > + * struct iommu_vqueue_tegra241_cmdqv - NVIDIA Tegra241's Virtual > Command Queue > + * for its CMDQV Extension for ARM SMMUv3 > + * (IOMMU_VQUEUE_DATA_TEGRA241_CMDQV) > + * @vcmdq_id: logical ID of a virtual command queue in the VIOMMU > instance > + * @vcmdq_log2size: (1 << @vcmdq_log2size) will be the size of the vcmdq > + * @vcmdq_base: guest physical address (IPA) to the vcmdq base address > + */ > +struct iommu_vqueue_tegra241_cmdqv { > + __u32 vcmdq_id; > + __u32 vcmdq_log2size; > + __aligned_u64 vcmdq_base; > +}; Is there restriction that log2size cannot exceed 12? According to patch14: + q_base = arm_smmu_domain_ipa_to_pa(vintf->s2_domain, arg.vcmdq_base); + vcmdq->cmdq.q.q_base = q_base & VCMDQ_ADDR; + vcmdq->cmdq.q.q_base |= arg.vcmdq_log2size; It only converts the IPA to HPA for the starting page, assuming continuous host pages backing a guest cmd queue including multiple pages. but what guarantees it?
On Thu, May 23, 2024 at 06:57:15AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Sunday, May 12, 2024 11:02 PM > > > > On Fri, Apr 12, 2024 at 08:47:09PM -0700, Nicolin Chen wrote: > > > > > +/** > > > + * struct iommu_vqueue_alloc - ioctl(IOMMU_VQUEUE_ALLOC) > > > + * @size: sizeof(struct iommu_vqueue_alloc) > > > + * @flags: Must be 0 > > > + * @viommu_id: viommu ID to associate the virtual queue with > > > + * @out_vqueue_id: The ID of the new virtual queue > > > + * @data_type: One of enum iommu_vqueue_data_type > > > + * @data_len: Length of the type specific data > > > + * @data_uptr: User pointer to the type specific data > > > + * > > > + * Allocate an virtual queue object for driver-specific HW-accelerated > > queue > > > + */ > > > + > > > +struct iommu_vqueue_alloc { > > > + __u32 size; > > > + __u32 flags; > > > + __u32 viommu_id; > > > + __u32 out_vqueue_id; > > > + __u32 data_type; > > > + __u32 data_len; > > > + __aligned_u64 data_uptr; > > > > Some of the iommus will want an IPA here not a user pointer. I think > > it is fine API wise, we'd just add a flag to indicate data_uptr is an > > IPA. > > > > Presumably each driver will create its own type data which can > include any IPA field as vcmdq_base in this patch? You mean putting a base address field in this common vqueue structure? Hmm, I think we could, assuming every queue must have a set of base and size info in the guest level. Thanks Nicolin
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Friday, May 24, 2024 12:42 PM > > On Thu, May 23, 2024 at 06:57:15AM +0000, Tian, Kevin wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Sunday, May 12, 2024 11:02 PM > > > > > > On Fri, Apr 12, 2024 at 08:47:09PM -0700, Nicolin Chen wrote: > > > > > > > +/** > > > > + * struct iommu_vqueue_alloc - ioctl(IOMMU_VQUEUE_ALLOC) > > > > + * @size: sizeof(struct iommu_vqueue_alloc) > > > > + * @flags: Must be 0 > > > > + * @viommu_id: viommu ID to associate the virtual queue with > > > > + * @out_vqueue_id: The ID of the new virtual queue > > > > + * @data_type: One of enum iommu_vqueue_data_type > > > > + * @data_len: Length of the type specific data > > > > + * @data_uptr: User pointer to the type specific data > > > > + * > > > > + * Allocate an virtual queue object for driver-specific HW-accelerated > > > queue > > > > + */ > > > > + > > > > +struct iommu_vqueue_alloc { > > > > + __u32 size; > > > > + __u32 flags; > > > > + __u32 viommu_id; > > > > + __u32 out_vqueue_id; > > > > + __u32 data_type; > > > > + __u32 data_len; > > > > + __aligned_u64 data_uptr; > > > > > > Some of the iommus will want an IPA here not a user pointer. I think > > > it is fine API wise, we'd just add a flag to indicate data_uptr is an > > > IPA. > > > > > > > Presumably each driver will create its own type data which can > > include any IPA field as vcmdq_base in this patch? > > You mean putting a base address field in this common vqueue > structure? Hmm, I think we could, assuming every queue must > have a set of base and size info in the guest level. > We could, but my original point was just that it's confusing to change the meaning of data_uptr. Let's stick it to be driver defined data and any driver-specific IPA field is defined in that data. Of course if we think the base address is common enough it can be moved to the common vqueue structure too.
On Fri, May 24, 2024 at 05:26:50AM +0000, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > Sent: Friday, May 24, 2024 12:42 PM > > > > On Thu, May 23, 2024 at 06:57:15AM +0000, Tian, Kevin wrote: > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > Sent: Sunday, May 12, 2024 11:02 PM > > > > > > > > On Fri, Apr 12, 2024 at 08:47:09PM -0700, Nicolin Chen wrote: > > > > > > > > > +/** > > > > > + * struct iommu_vqueue_alloc - ioctl(IOMMU_VQUEUE_ALLOC) > > > > > + * @size: sizeof(struct iommu_vqueue_alloc) > > > > > + * @flags: Must be 0 > > > > > + * @viommu_id: viommu ID to associate the virtual queue with > > > > > + * @out_vqueue_id: The ID of the new virtual queue > > > > > + * @data_type: One of enum iommu_vqueue_data_type > > > > > + * @data_len: Length of the type specific data > > > > > + * @data_uptr: User pointer to the type specific data > > > > > + * > > > > > + * Allocate an virtual queue object for driver-specific HW-accelerated > > > > queue > > > > > + */ > > > > > + > > > > > +struct iommu_vqueue_alloc { > > > > > + __u32 size; > > > > > + __u32 flags; > > > > > + __u32 viommu_id; > > > > > + __u32 out_vqueue_id; > > > > > + __u32 data_type; > > > > > + __u32 data_len; > > > > > + __aligned_u64 data_uptr; > > > > > > > > Some of the iommus will want an IPA here not a user pointer. I think > > > > it is fine API wise, we'd just add a flag to indicate data_uptr is an > > > > IPA. > > > > > > > > > > Presumably each driver will create its own type data which can > > > include any IPA field as vcmdq_base in this patch? > > > > You mean putting a base address field in this common vqueue > > structure? Hmm, I think we could, assuming every queue must > > have a set of base and size info in the guest level. > > > > We could, but my original point was just that it's confusing to > change the meaning of data_uptr. Let's stick it to be driver defined > data and any driver-specific IPA field is defined in that data. Oh, that's copied from IOMMU_HWPT_ALLOC: * @data_uptr: User pointer to the type specific data And it actually makes sense to me to be "type specific" since a driver (AMD VIOMMU for example) might have two different vqueue types (invalidation queue v.s. fault queue)? Thanks Nicolin
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 9ba8f4ecc221..c994f2db3052 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -436,6 +436,8 @@ iommufd_get_viommu(struct iommufd_ucmd *ucmd, u32 id) int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd); void iommufd_viommu_destroy(struct iommufd_object *obj); int iommufd_viommu_set_device_id(struct iommufd_ucmd *ucmd); +int iommufd_vqueue_alloc_ioctl(struct iommufd_ucmd *ucmd); +void iommufd_vqueue_destroy(struct iommufd_object *obj); #ifdef CONFIG_IOMMUFD_TEST int iommufd_test(struct iommufd_ucmd *ucmd); diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c index 16efc3346a2a..96ef81530809 100644 --- a/drivers/iommu/iommufd/main.c +++ b/drivers/iommu/iommufd/main.c @@ -324,6 +324,7 @@ union ucmd_buffer { struct iommu_ioas_alloc alloc; struct iommu_ioas_allow_iovas allow_iovas; struct iommu_viommu_alloc viommu; + struct iommu_vqueue_alloc vqueue; struct iommu_ioas_copy ioas_copy; struct iommu_ioas_iova_ranges iova_ranges; struct iommu_ioas_map map; @@ -383,6 +384,8 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = { struct iommu_viommu_alloc, out_viommu_id), IOCTL_OP(IOMMU_VIOMMU_SET_DEV_ID, iommufd_viommu_set_device_id, struct iommu_viommu_set_dev_id, id), + IOCTL_OP(IOMMU_VQUEUE_ALLOC, iommufd_vqueue_alloc_ioctl, + struct iommu_vqueue_alloc, data_uptr), #ifdef CONFIG_IOMMUFD_TEST IOCTL_OP(IOMMU_TEST_CMD, iommufd_test, struct iommu_test_cmd, last), #endif @@ -518,6 +521,9 @@ static const struct iommufd_object_ops iommufd_object_ops[] = { [IOMMUFD_OBJ_VIOMMU] = { .destroy = iommufd_viommu_destroy, }, + [IOMMUFD_OBJ_VQUEUE] = { + .destroy = iommufd_vqueue_destroy, + }, #ifdef CONFIG_IOMMUFD_TEST [IOMMUFD_OBJ_SELFTEST] = { .destroy = iommufd_selftest_destroy, diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c index 8894878c1e73..56c9ea818bfa 100644 --- a/drivers/iommu/iommufd/viommu.c +++ b/drivers/iommu/iommufd/viommu.c @@ -161,3 +161,75 @@ int iommufd_viommu_set_device_id(struct iommufd_ucmd *ucmd) iommufd_put_object(ucmd->ictx, &idev->obj); return rc; } + +void iommufd_vqueue_destroy(struct iommufd_object *obj) +{ + struct iommufd_vqueue *vqueue = + container_of(obj, struct iommufd_vqueue, obj); + struct iommufd_viommu *viommu = vqueue->viommu; + + if (viommu->ops->vqueue_free) + viommu->ops->vqueue_free(vqueue); + refcount_dec(&viommu->obj.users); +} + +int iommufd_vqueue_alloc_ioctl(struct iommufd_ucmd *ucmd) +{ + struct iommu_vqueue_alloc *cmd = ucmd->cmd; + const struct iommu_user_data user_data = { + .type = cmd->data_type, + .uptr = u64_to_user_ptr(cmd->data_uptr), + .len = cmd->data_len, + }; + struct iommufd_vqueue *vqueue; + struct iommufd_viommu *viommu; + int rc; + + if (cmd->flags) + return -EOPNOTSUPP; + if (!cmd->data_len) + return -EINVAL; + + viommu = iommufd_get_viommu(ucmd, cmd->viommu_id); + if (IS_ERR(viommu)) + return PTR_ERR(viommu); + + if (!viommu->ops || !viommu->ops->vqueue_alloc) { + rc = -EOPNOTSUPP; + goto out_put_viommu; + } + + vqueue = viommu->ops->vqueue_alloc( + viommu, user_data.len ? &user_data : NULL); + if (IS_ERR(vqueue)) { + rc = PTR_ERR(vqueue); + goto out_put_viommu; + } + + /* iommufd_object_finalize will store the vqueue->obj.id */ + rc = xa_alloc(&ucmd->ictx->objects, &vqueue->obj.id, XA_ZERO_ENTRY, + xa_limit_31b, GFP_KERNEL_ACCOUNT); + if (rc) + goto out_free; + + vqueue->obj.type = IOMMUFD_OBJ_VQUEUE; + + vqueue->ictx = ucmd->ictx; + vqueue->viommu = viommu; + cmd->out_vqueue_id = vqueue->obj.id; + rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); + if (rc) + goto out_erase_xa; + iommufd_object_finalize(ucmd->ictx, &vqueue->obj); + refcount_inc(&viommu->obj.users); + goto out_put_viommu; + +out_erase_xa: + xa_erase(&ucmd->ictx->objects, vqueue->obj.id); +out_free: + if (viommu->ops->vqueue_free) + viommu->ops->vqueue_free(vqueue); +out_put_viommu: + iommufd_put_object(ucmd->ictx, &viommu->obj); + return rc; +} diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index 5b97b04aa145..707b6d4b20a3 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -31,6 +31,7 @@ enum iommufd_object_type { IOMMUFD_OBJ_IOAS, IOMMUFD_OBJ_ACCESS, IOMMUFD_OBJ_VIOMMU, + IOMMUFD_OBJ_VQUEUE, #ifdef CONFIG_IOMMUFD_TEST IOMMUFD_OBJ_SELFTEST, #endif diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h index eaa192de63d3..95abe3100e3b 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -52,6 +52,7 @@ enum { IOMMUFD_CMD_HWPT_INVALIDATE, IOMMUFD_CMD_VIOMMU_ALLOC, IOMMUFD_CMD_VIOMMU_SET_DEV_ID, + IOMMUFD_CMD_VQUEUE_ALLOC, }; /** @@ -742,4 +743,51 @@ struct iommu_viommu_set_dev_id { __aligned_u64 id; }; #define IOMMU_VIOMMU_SET_DEV_ID _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VIOMMU_SET_DEV_ID) + +/** + * struct iommu_vqueue_tegra241_cmdqv - NVIDIA Tegra241's Virtual Command Queue + * for its CMDQV Extension for ARM SMMUv3 + * (IOMMU_VQUEUE_DATA_TEGRA241_CMDQV) + * @vcmdq_id: logical ID of a virtual command queue in the VIOMMU instance + * @vcmdq_log2size: (1 << @vcmdq_log2size) will be the size of the vcmdq + * @vcmdq_base: guest physical address (IPA) to the vcmdq base address + */ +struct iommu_vqueue_tegra241_cmdqv { + __u32 vcmdq_id; + __u32 vcmdq_log2size; + __aligned_u64 vcmdq_base; +}; + +/** + * enum iommu_vqueue_data_type - VQUEUE Data Type + * @IOMMU_VQUEUE_DATA_TEGRA241_CMDQV: NVIDIA Tegra241 CMDQV Extension for SMMUv3 + */ +enum iommu_vqueue_data_type { + IOMMU_VQUEUE_DATA_TEGRA241_CMDQV, +}; + +/** + * struct iommu_vqueue_alloc - ioctl(IOMMU_VQUEUE_ALLOC) + * @size: sizeof(struct iommu_vqueue_alloc) + * @flags: Must be 0 + * @viommu_id: viommu ID to associate the virtual queue with + * @out_vqueue_id: The ID of the new virtual queue + * @data_type: One of enum iommu_vqueue_data_type + * @data_len: Length of the type specific data + * @data_uptr: User pointer to the type specific data + * + * Allocate an virtual queue object for driver-specific HW-accelerated queue + */ + +struct iommu_vqueue_alloc { + __u32 size; + __u32 flags; + __u32 viommu_id; + __u32 out_vqueue_id; + __u32 data_type; + __u32 data_len; + __aligned_u64 data_uptr; +}; +#define IOMMU_VQUEUE_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VQUEUE_ALLOC) + #endif
Introduce a new IOMMUFD_OBJ_VQUEU to represent a virtual command queue instance. And add a new ioctl for user space to allocate it. As an initial version, ddd IOMMU_VQUEUE_DATA_TEGRA241_CMDQV to the enum iommu_vqueue_data_type and the corresponding iommu_vqueue_tegra241_cmdqv data structure. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/iommufd/iommufd_private.h | 2 + drivers/iommu/iommufd/main.c | 6 +++ drivers/iommu/iommufd/viommu.c | 72 +++++++++++++++++++++++++ include/linux/iommufd.h | 1 + include/uapi/linux/iommufd.h | 48 +++++++++++++++++ 5 files changed, 129 insertions(+)