| Message ID | 20230309080910.607396-9-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommufd: Add nesting infrastructure | expand |
On 3/9/23 4:09 PM, Yi Liu wrote: > This provides a way for userspace to probe the supported hwpt data > types by kernel. Currently, kernel only supports IOMMU_HWPT_TYPE_DEFAULT, > new types would be added per vendor drivers' extension. > > Userspace that wants to allocate hw_pagetable with user data should check > this. While for the allocation without user data, no need for it. It is > supported by default. > > Co-developed-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > drivers/iommu/iommufd/device.c | 1 + > drivers/iommu/iommufd/hw_pagetable.c | 18 +++++++++++++++--- > drivers/iommu/iommufd/iommufd_private.h | 2 ++ > drivers/iommu/iommufd/main.c | 2 +- > include/uapi/linux/iommufd.h | 8 ++++++++ > 5 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c > index 19cd6df46c6a..0328071dcac1 100644 > --- a/drivers/iommu/iommufd/device.c > +++ b/drivers/iommu/iommufd/device.c > @@ -322,6 +322,7 @@ int iommufd_device_get_hw_info(struct iommufd_ucmd *ucmd) > > cmd->out_data_type = ops->driver_type; > cmd->data_len = length; > + cmd->out_hwpt_type_bitmap = iommufd_hwpt_type_bitmaps[ops->driver_type]; > > rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); > > diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c > index 67facca98de1..160712256c64 100644 > --- a/drivers/iommu/iommufd/hw_pagetable.c > +++ b/drivers/iommu/iommufd/hw_pagetable.c > @@ -173,6 +173,14 @@ static const size_t iommufd_hwpt_alloc_data_size[] = { > [IOMMU_HWPT_TYPE_DEFAULT] = 0, > }; > > +/* > + * bitmaps of supported hwpt types of by underlying iommu, indexed > + * by ops->driver_type which is one of enum iommu_hw_info_type. > + */ > +const u64 iommufd_hwpt_type_bitmaps[] = { > + [IOMMU_HW_INFO_TYPE_DEFAULT] = BIT_ULL(IOMMU_HWPT_TYPE_DEFAULT), > +}; I am a bit confused here. Why do you need this array? What I read is that you want to convert ops->driver_type to a bit position in cmd->out_hwpt_type_bitmap. Am I getting it right? If so, why not just cmd->out_hwpt_type_bitmap = BIT_ULL(ops->driver_type); ? Best regards, baolu
On Fri, Mar 10, 2023 at 11:30:04AM +0800, Baolu Lu wrote: > External email: Use caution opening links or attachments > > > On 3/9/23 4:09 PM, Yi Liu wrote: > > This provides a way for userspace to probe the supported hwpt data > > types by kernel. Currently, kernel only supports IOMMU_HWPT_TYPE_DEFAULT, > > new types would be added per vendor drivers' extension. > > > > Userspace that wants to allocate hw_pagetable with user data should check > > this. While for the allocation without user data, no need for it. It is > > supported by default. > > > > Co-developed-by: Nicolin Chen <nicolinc@nvidia.com> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > --- > > drivers/iommu/iommufd/device.c | 1 + > > drivers/iommu/iommufd/hw_pagetable.c | 18 +++++++++++++++--- > > drivers/iommu/iommufd/iommufd_private.h | 2 ++ > > drivers/iommu/iommufd/main.c | 2 +- > > include/uapi/linux/iommufd.h | 8 ++++++++ > > 5 files changed, 27 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c > > index 19cd6df46c6a..0328071dcac1 100644 > > --- a/drivers/iommu/iommufd/device.c > > +++ b/drivers/iommu/iommufd/device.c > > @@ -322,6 +322,7 @@ int iommufd_device_get_hw_info(struct iommufd_ucmd *ucmd) > > > > cmd->out_data_type = ops->driver_type; > > cmd->data_len = length; > > + cmd->out_hwpt_type_bitmap = iommufd_hwpt_type_bitmaps[ops->driver_type]; > > > > rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); > > > > diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c > > index 67facca98de1..160712256c64 100644 > > --- a/drivers/iommu/iommufd/hw_pagetable.c > > +++ b/drivers/iommu/iommufd/hw_pagetable.c > > @@ -173,6 +173,14 @@ static const size_t iommufd_hwpt_alloc_data_size[] = { > > [IOMMU_HWPT_TYPE_DEFAULT] = 0, > > }; > > > > +/* > > + * bitmaps of supported hwpt types of by underlying iommu, indexed > > + * by ops->driver_type which is one of enum iommu_hw_info_type. > > + */ > > +const u64 iommufd_hwpt_type_bitmaps[] = { > > + [IOMMU_HW_INFO_TYPE_DEFAULT] = BIT_ULL(IOMMU_HWPT_TYPE_DEFAULT), > > +}; > > I am a bit confused here. Why do you need this array? What I read is > that you want to convert ops->driver_type to a bit position in > cmd->out_hwpt_type_bitmap. > > Am I getting it right? > > If so, why not just > cmd->out_hwpt_type_bitmap = BIT_ULL(ops->driver_type); > > ? A driver_type would be IOMMUFD_HW_INFO_TYPExx. What's inside the BIT_ULL is IOMMUFD_HWPT_TYPE_*. It seems to get a bit confusing after several rounds of renaming though. And they do seem to be a bit of duplications at the actual values, at least for now. I recall that we only had one enum for the types, yet later did a separation into driver_types and hwpt_types after a discussion regarding future PASID support? Thanks Nic
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Friday, March 10, 2023 3:10 PM > > On Fri, Mar 10, 2023 at 11:30:04AM +0800, Baolu Lu wrote: > > External email: Use caution opening links or attachments > > > > > > On 3/9/23 4:09 PM, Yi Liu wrote: > > > This provides a way for userspace to probe the supported hwpt data > > > types by kernel. Currently, kernel only supports > IOMMU_HWPT_TYPE_DEFAULT, > > > new types would be added per vendor drivers' extension. > > > > > > Userspace that wants to allocate hw_pagetable with user data should > check > > > this. While for the allocation without user data, no need for it. It is > > > supported by default. > > > > > > Co-developed-by: Nicolin Chen <nicolinc@nvidia.com> > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > > --- > > > drivers/iommu/iommufd/device.c | 1 + > > > drivers/iommu/iommufd/hw_pagetable.c | 18 +++++++++++++++--- > > > drivers/iommu/iommufd/iommufd_private.h | 2 ++ > > > drivers/iommu/iommufd/main.c | 2 +- > > > include/uapi/linux/iommufd.h | 8 ++++++++ > > > 5 files changed, 27 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/iommu/iommufd/device.c > b/drivers/iommu/iommufd/device.c > > > index 19cd6df46c6a..0328071dcac1 100644 > > > --- a/drivers/iommu/iommufd/device.c > > > +++ b/drivers/iommu/iommufd/device.c > > > @@ -322,6 +322,7 @@ int iommufd_device_get_hw_info(struct > iommufd_ucmd *ucmd) > > > > > > cmd->out_data_type = ops->driver_type; > > > cmd->data_len = length; > > > + cmd->out_hwpt_type_bitmap = > iommufd_hwpt_type_bitmaps[ops->driver_type]; > > > > > > rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); > > > > > > diff --git a/drivers/iommu/iommufd/hw_pagetable.c > b/drivers/iommu/iommufd/hw_pagetable.c > > > index 67facca98de1..160712256c64 100644 > > > --- a/drivers/iommu/iommufd/hw_pagetable.c > > > +++ b/drivers/iommu/iommufd/hw_pagetable.c > > > @@ -173,6 +173,14 @@ static const size_t > iommufd_hwpt_alloc_data_size[] = { > > > [IOMMU_HWPT_TYPE_DEFAULT] = 0, > > > }; > > > > > > +/* > > > + * bitmaps of supported hwpt types of by underlying iommu, indexed > > > + * by ops->driver_type which is one of enum iommu_hw_info_type. > > > + */ > > > +const u64 iommufd_hwpt_type_bitmaps[] = { > > > + [IOMMU_HW_INFO_TYPE_DEFAULT] = > BIT_ULL(IOMMU_HWPT_TYPE_DEFAULT), > > > +}; > > > > I am a bit confused here. Why do you need this array? What I read is > > that you want to convert ops->driver_type to a bit position in > > cmd->out_hwpt_type_bitmap. > > > > Am I getting it right? > > > > If so, why not just > > cmd->out_hwpt_type_bitmap = BIT_ULL(ops->driver_type); > > > > ? The reason is for future extensions. If future usages need different types of user data to allocate hwpt, it can define a new type and corresponding data structure. Such new usages may be using new vendor-specific page tables or vendor-agnostic usages like Re-use of the KVM page table in the IOMMU mentioned by IOMMUFD basic series. https://lore.kernel.org/kvm/0-v6-a196d26f289e+11787-iommufd_jgg@nvidia.com/ > A driver_type would be IOMMUFD_HW_INFO_TYPExx. What's inside the > BIT_ULL is IOMMUFD_HWPT_TYPE_*. It seems to get a bit confusing > after several rounds of renaming though. And they do seem to be > a bit of duplications at the actual values, at least for now. For now, vendor drivers only have one stage-1 page table format. But in the future, it may change per new page table format introduction and new usage. Regards, Yi Liu
On Fri, Mar 10, 2023 at 07:39:00AM +0000, Liu, Yi L wrote: > External email: Use caution opening links or attachments > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > Sent: Friday, March 10, 2023 3:10 PM > > > > On Fri, Mar 10, 2023 at 11:30:04AM +0800, Baolu Lu wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > On 3/9/23 4:09 PM, Yi Liu wrote: > > > > This provides a way for userspace to probe the supported hwpt data > > > > types by kernel. Currently, kernel only supports > > IOMMU_HWPT_TYPE_DEFAULT, > > > > new types would be added per vendor drivers' extension. > > > > > > > > Userspace that wants to allocate hw_pagetable with user data should > > check > > > > this. While for the allocation without user data, no need for it. It is > > > > supported by default. > > > > > > > > Co-developed-by: Nicolin Chen <nicolinc@nvidia.com> > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > > > --- > > > > drivers/iommu/iommufd/device.c | 1 + > > > > drivers/iommu/iommufd/hw_pagetable.c | 18 +++++++++++++++--- > > > > drivers/iommu/iommufd/iommufd_private.h | 2 ++ > > > > drivers/iommu/iommufd/main.c | 2 +- > > > > include/uapi/linux/iommufd.h | 8 ++++++++ > > > > 5 files changed, 27 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/iommu/iommufd/device.c > > b/drivers/iommu/iommufd/device.c > > > > index 19cd6df46c6a..0328071dcac1 100644 > > > > --- a/drivers/iommu/iommufd/device.c > > > > +++ b/drivers/iommu/iommufd/device.c > > > > @@ -322,6 +322,7 @@ int iommufd_device_get_hw_info(struct > > iommufd_ucmd *ucmd) > > > > > > > > cmd->out_data_type = ops->driver_type; > > > > cmd->data_len = length; > > > > + cmd->out_hwpt_type_bitmap = > > iommufd_hwpt_type_bitmaps[ops->driver_type]; > > > > > > > > rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); > > > > > > > > diff --git a/drivers/iommu/iommufd/hw_pagetable.c > > b/drivers/iommu/iommufd/hw_pagetable.c > > > > index 67facca98de1..160712256c64 100644 > > > > --- a/drivers/iommu/iommufd/hw_pagetable.c > > > > +++ b/drivers/iommu/iommufd/hw_pagetable.c > > > > @@ -173,6 +173,14 @@ static const size_t > > iommufd_hwpt_alloc_data_size[] = { > > > > [IOMMU_HWPT_TYPE_DEFAULT] = 0, > > > > }; > > > > > > > > +/* > > > > + * bitmaps of supported hwpt types of by underlying iommu, indexed > > > > + * by ops->driver_type which is one of enum iommu_hw_info_type. > > > > + */ > > > > +const u64 iommufd_hwpt_type_bitmaps[] = { > > > > + [IOMMU_HW_INFO_TYPE_DEFAULT] = > > BIT_ULL(IOMMU_HWPT_TYPE_DEFAULT), > > > > +}; > > > > > > I am a bit confused here. Why do you need this array? What I read is > > > that you want to convert ops->driver_type to a bit position in > > > cmd->out_hwpt_type_bitmap. > > > > > > Am I getting it right? > > > > > > If so, why not just > > > cmd->out_hwpt_type_bitmap = BIT_ULL(ops->driver_type); > > > > > > ? > > The reason is for future extensions. If future usages need different types > of user data to allocate hwpt, it can define a new type and corresponding > data structure. Such new usages may be using new vendor-specific page > tables or vendor-agnostic usages like Re-use of the KVM page table in > the IOMMU mentioned by IOMMUFD basic series. > > https://lore.kernel.org/kvm/0-v6-a196d26f289e+11787-iommufd_jgg@nvidia.com/ > > > A driver_type would be IOMMUFD_HW_INFO_TYPExx. What's inside the > > BIT_ULL is IOMMUFD_HWPT_TYPE_*. It seems to get a bit confusing > > after several rounds of renaming though. And they do seem to be > > a bit of duplications at the actual values, at least for now. > > For now, vendor drivers only have one stage-1 page table format. > But in the future, it may change per new page table format > introduction and new usage. Yea, that's what I thought too. Yet, I am wondering a bit if it'd be better to have an ops->hwpt_type in the drivers, v.s. maintaining a potentially big chunk of the array here. Thanks Nic
On Thu, Mar 09, 2023 at 11:45:05PM -0800, Nicolin Chen wrote: > Yea, that's what I thought too. Yet, I am wondering a bit if > it'd be better to have an ops->hwpt_type in the drivers, v.s. > maintaining a potentially big chunk of the array here. It probably is, some ops->iommufd_data.XX is not a bad idea Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Saturday, March 11, 2023 1:53 AM > > On Thu, Mar 09, 2023 at 11:45:05PM -0800, Nicolin Chen wrote: > > > Yea, that's what I thought too. Yet, I am wondering a bit if > > it'd be better to have an ops->hwpt_type in the drivers, v.s. > > maintaining a potentially big chunk of the array here. > > It probably is, some ops->iommufd_data.XX is not a bad idea Will try. Regards, Yi Liu
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 19cd6df46c6a..0328071dcac1 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -322,6 +322,7 @@ int iommufd_device_get_hw_info(struct iommufd_ucmd *ucmd) cmd->out_data_type = ops->driver_type; cmd->data_len = length; + cmd->out_hwpt_type_bitmap = iommufd_hwpt_type_bitmaps[ops->driver_type]; rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index 67facca98de1..160712256c64 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -173,6 +173,14 @@ static const size_t iommufd_hwpt_alloc_data_size[] = { [IOMMU_HWPT_TYPE_DEFAULT] = 0, }; +/* + * bitmaps of supported hwpt types of by underlying iommu, indexed + * by ops->driver_type which is one of enum iommu_hw_info_type. + */ +const u64 iommufd_hwpt_type_bitmaps[] = { + [IOMMU_HW_INFO_TYPE_DEFAULT] = BIT_ULL(IOMMU_HWPT_TYPE_DEFAULT), +}; + int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) { struct iommu_hwpt_alloc *cmd = ucmd->cmd; @@ -182,7 +190,7 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) struct iommufd_ioas *ioas; const struct iommu_ops *ops; void *data = NULL; - u32 klen; + u32 driver_type, klen; int rc; if (cmd->__reserved || cmd->flags) @@ -198,8 +206,12 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) goto out_put_idev; } - /* Only support IOMMU_HWPT_TYPE_DEFAULT for now */ - if (cmd->data_type != IOMMU_HWPT_TYPE_DEFAULT) { + driver_type = ops->driver_type; + + /* data_type should be a supported type by the driver */ + if (WARN_ON(driver_type >= ARRAY_SIZE(iommufd_hwpt_type_bitmaps)) || + !((1 << cmd->data_type) & + iommufd_hwpt_type_bitmaps[driver_type])) { rc = -EINVAL; goto out_put_idev; } diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index d879264d1acf..164ccfc2e6e0 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -321,6 +321,8 @@ iommufd_get_device(struct iommufd_ucmd *ucmd, u32 id) void iommufd_device_destroy(struct iommufd_object *obj); int iommufd_device_get_hw_info(struct iommufd_ucmd *ucmd); +extern const u64 iommufd_hwpt_type_bitmaps[]; + struct iommufd_access { struct iommufd_object obj; struct iommufd_ctx *ictx; diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c index 2cf45f65b637..7ec3ceac01b3 100644 --- a/drivers/iommu/iommufd/main.c +++ b/drivers/iommu/iommufd/main.c @@ -298,7 +298,7 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = { IOCTL_OP(IOMMU_HWPT_ALLOC, iommufd_hwpt_alloc, struct iommu_hwpt_alloc, data_uptr), IOCTL_OP(IOMMU_DEVICE_GET_HW_INFO, iommufd_device_get_hw_info, - struct iommu_hw_info, __reserved), + struct iommu_hw_info, out_hwpt_type_bitmap), IOCTL_OP(IOMMU_HWPT_INVALIDATE, iommufd_hwpt_invalidate, struct iommu_hwpt_invalidate, data_uptr), IOCTL_OP(IOMMU_IOAS_ALLOC, iommufd_ioas_alloc_ioctl, diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h index d0962c41f8d6..e2eff9c56ab3 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -421,6 +421,8 @@ enum iommu_hw_info_type { * @out_data_type: Output the iommu hardware info type, it is one of * enum iommu_hw_info_type. * @__reserved: Must be 0 + * @out_hwpt_type_bitmap: Output the supported page table type. Each + * bit is defined in enum iommu_hwpt_type. * * Query the hardware iommu information for given device which has been * bound to iommufd. @data_len is the size of the buffer which captures @@ -435,6 +437,11 @@ enum iommu_hw_info_type { * The @out_data_type will be filled if the ioctl succeeds. It would * be used to decode the data filled in the buffer pointed by @data_ptr. * + * @out_hwpt_type_bitmap reports the supported hwpt types. This differs + * per the @out_data_type. Userspace should check it before allocating a + * user-managed hw_pagetable with user data, unless it allocates a default + * hw_pagetable that does not need user data. + * * This is only available for the physical devices bound to iommufd as * only physical devices can have hardware IOMMU. */ @@ -446,6 +453,7 @@ struct iommu_hw_info { __aligned_u64 data_ptr; __u32 out_data_type; __u32 __reserved; + __aligned_u64 out_hwpt_type_bitmap; }; #define IOMMU_DEVICE_GET_HW_INFO _IO(IOMMUFD_TYPE, IOMMUFD_CMD_DEVICE_GET_HW_INFO)