[08/12] iommufd/device: Report supported hwpt_types
Commit Message
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(-)
Comments
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
@@ -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));
@@ -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;
}
@@ -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;
@@ -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,
@@ -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)