[PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable

Lu Baolu posted 9 patches 1 year, 7 months ago
There is a newer version of this series
[PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable
Posted by Lu Baolu 1 year, 7 months ago
When allocating a user iommufd_hw_pagetable, the user space is allowed to
associate a fault object with the hw_pagetable by specifying the fault
object ID in the page table allocation data and setting the
IOMMU_HWPT_FAULT_ID_VALID flag bit.

On a successful return of hwpt allocation, the user can retrieve and
respond to page faults by reading and writing the file interface of the
fault object.

Once a fault object has been associated with a hwpt, the hwpt is
iopf-capable, indicated by fault_capable set to true. Attaching,
detaching, or replacing an iopf-capable hwpt to an RID or PASID will
differ from those that are not iopf-capable.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommufd/iommufd_private.h |  9 +++++++
 include/uapi/linux/iommufd.h            |  8 ++++++
 drivers/iommu/iommufd/fault.c           | 17 ++++++++++++
 drivers/iommu/iommufd/hw_pagetable.c    | 35 +++++++++++++++++++------
 drivers/iommu/iommufd/main.c            |  2 +-
 5 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 18c8d7d38dcd..100c7b585e7e 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -449,8 +449,17 @@ struct iommufd_fault {
 	struct wait_queue_head wait_queue;
 };
 
+static inline struct iommufd_fault *
+iommufd_get_fault(struct iommufd_ucmd *ucmd, u32 id)
+{
+	return container_of(iommufd_get_object(ucmd->ictx, id,
+					       IOMMUFD_OBJ_FAULT),
+			    struct iommufd_fault, obj);
+}
+
 int iommufd_fault_alloc(struct iommufd_ucmd *ucmd);
 void iommufd_fault_destroy(struct iommufd_object *obj);
+int iommufd_fault_iopf_handler(struct iopf_group *group);
 
 int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
 				    struct iommufd_device *idev);
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 1819b28e9e6b..3d566c1ffcc6 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -357,10 +357,13 @@ struct iommu_vfio_ioas {
  *                                the parent HWPT in a nesting configuration.
  * @IOMMU_HWPT_ALLOC_DIRTY_TRACKING: Dirty tracking support for device IOMMU is
  *                                   enforced on device attachment
+ * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation data is
+ *                             valid.
  */
 enum iommufd_hwpt_alloc_flags {
 	IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
 	IOMMU_HWPT_ALLOC_DIRTY_TRACKING = 1 << 1,
+	IOMMU_HWPT_FAULT_ID_VALID = 1 << 2,
 };
 
 /**
@@ -412,6 +415,9 @@ enum iommu_hwpt_data_type {
  * @data_type: One of enum iommu_hwpt_data_type
  * @data_len: Length of the type specific data
  * @data_uptr: User pointer to the type specific data
+ * @fault_id: The ID of IOMMUFD_FAULT object. Valid only if flags field of
+ *            IOMMU_HWPT_FAULT_ID_VALID is set.
+ * @__reserved2: Padding to 64-bit alignment. Must be 0.
  *
  * Explicitly allocate a hardware page table object. This is the same object
  * type that is returned by iommufd_device_attach() and represents the
@@ -442,6 +448,8 @@ struct iommu_hwpt_alloc {
 	__u32 data_type;
 	__u32 data_len;
 	__aligned_u64 data_uptr;
+	__u32 fault_id;
+	__u32 __reserved2;
 };
 #define IOMMU_HWPT_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_ALLOC)
 
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index 6357229bf3b4..802d0f819b22 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -354,3 +354,20 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 
 	return rc;
 }
+
+int iommufd_fault_iopf_handler(struct iopf_group *group)
+{
+	struct iommufd_hw_pagetable *hwpt;
+	struct iommufd_fault *fault;
+
+	hwpt = group->domain->fault_data;
+	fault = hwpt->fault;
+
+	mutex_lock(&fault->mutex);
+	list_add_tail(&group->node, &fault->deliver);
+	mutex_unlock(&fault->mutex);
+
+	wake_up_interruptible(&fault->wait_queue);
+
+	return 0;
+}
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 33d142f8057d..f7d05e12dea1 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -8,6 +8,15 @@
 #include "../iommu-priv.h"
 #include "iommufd_private.h"
 
+static void __iommufd_hwpt_destroy(struct iommufd_hw_pagetable *hwpt)
+{
+	if (hwpt->domain)
+		iommu_domain_free(hwpt->domain);
+
+	if (hwpt->fault)
+		iommufd_put_object(hwpt->fault->ictx, &hwpt->fault->obj);
+}
+
 void iommufd_hwpt_paging_destroy(struct iommufd_object *obj)
 {
 	struct iommufd_hwpt_paging *hwpt_paging =
@@ -22,9 +31,7 @@ void iommufd_hwpt_paging_destroy(struct iommufd_object *obj)
 					 hwpt_paging->common.domain);
 	}
 
-	if (hwpt_paging->common.domain)
-		iommu_domain_free(hwpt_paging->common.domain);
-
+	__iommufd_hwpt_destroy(&hwpt_paging->common);
 	refcount_dec(&hwpt_paging->ioas->obj.users);
 }
 
@@ -49,9 +56,7 @@ void iommufd_hwpt_nested_destroy(struct iommufd_object *obj)
 	struct iommufd_hwpt_nested *hwpt_nested =
 		container_of(obj, struct iommufd_hwpt_nested, common.obj);
 
-	if (hwpt_nested->common.domain)
-		iommu_domain_free(hwpt_nested->common.domain);
-
+	__iommufd_hwpt_destroy(&hwpt_nested->common);
 	refcount_dec(&hwpt_nested->parent->common.obj.users);
 }
 
@@ -213,7 +218,8 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
 	struct iommufd_hw_pagetable *hwpt;
 	int rc;
 
-	if (flags || !user_data->len || !ops->domain_alloc_user)
+	if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) ||
+	    !user_data->len || !ops->domain_alloc_user)
 		return ERR_PTR(-EOPNOTSUPP);
 	if (parent->auto_domain || !parent->nest_parent)
 		return ERR_PTR(-EINVAL);
@@ -227,7 +233,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
 	refcount_inc(&parent->common.obj.users);
 	hwpt_nested->parent = parent;
 
-	hwpt->domain = ops->domain_alloc_user(idev->dev, flags,
+	hwpt->domain = ops->domain_alloc_user(idev->dev, 0,
 					      parent->common.domain, user_data);
 	if (IS_ERR(hwpt->domain)) {
 		rc = PTR_ERR(hwpt->domain);
@@ -308,6 +314,19 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
 		goto out_put_pt;
 	}
 
+	if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
+		struct iommufd_fault *fault;
+
+		fault = iommufd_get_fault(ucmd, cmd->fault_id);
+		if (IS_ERR(fault)) {
+			rc = PTR_ERR(fault);
+			goto out_hwpt;
+		}
+		hwpt->fault = fault;
+		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
+		hwpt->domain->fault_data = hwpt;
+	}
+
 	cmd->out_hwpt_id = hwpt->obj.id;
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 	if (rc)
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 792db077d33e..d8414ee9feae 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -359,7 +359,7 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
 	IOCTL_OP(IOMMU_GET_HW_INFO, iommufd_get_hw_info, struct iommu_hw_info,
 		 __reserved),
 	IOCTL_OP(IOMMU_HWPT_ALLOC, iommufd_hwpt_alloc, struct iommu_hwpt_alloc,
-		 __reserved),
+		 __reserved2),
 	IOCTL_OP(IOMMU_HWPT_GET_DIRTY_BITMAP, iommufd_hwpt_get_dirty_bitmap,
 		 struct iommu_hwpt_get_dirty_bitmap, data),
 	IOCTL_OP(IOMMU_HWPT_INVALIDATE, iommufd_hwpt_invalidate,
-- 
2.34.1
RE: [PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable
Posted by Tian, Kevin 1 year, 7 months ago
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, April 30, 2024 10:57 PM
>
> @@ -227,7 +233,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx
> *ictx,
>  	refcount_inc(&parent->common.obj.users);
>  	hwpt_nested->parent = parent;
> 
> -	hwpt->domain = ops->domain_alloc_user(idev->dev, flags,
> +	hwpt->domain = ops->domain_alloc_user(idev->dev, 0,
>  					      parent->common.domain,
> user_data);

it reads slightly better to clear the fault bit and still pass in flags.

> @@ -308,6 +314,19 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd
> *ucmd)
>  		goto out_put_pt;
>  	}
> 
> +	if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
> +		struct iommufd_fault *fault;
> +
> +		fault = iommufd_get_fault(ucmd, cmd->fault_id);
> +		if (IS_ERR(fault)) {
> +			rc = PTR_ERR(fault);
> +			goto out_hwpt;
> +		}
> +		hwpt->fault = fault;
> +		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
> +		hwpt->domain->fault_data = hwpt;
> +	}

this is nesting specific. why not moving it to the nested_alloc()?
Re: [PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable
Posted by Baolu Lu 1 year, 7 months ago
On 5/15/24 4:50 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, April 30, 2024 10:57 PM
>>
>> @@ -227,7 +233,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx
>> *ictx,
>>   	refcount_inc(&parent->common.obj.users);
>>   	hwpt_nested->parent = parent;
>>
>> -	hwpt->domain = ops->domain_alloc_user(idev->dev, flags,
>> +	hwpt->domain = ops->domain_alloc_user(idev->dev, 0,
>>   					      parent->common.domain,
>> user_data);
> 
> it reads slightly better to clear the fault bit and still pass in flags.
> 

Done.

-       hwpt->domain = ops->domain_alloc_user(idev->dev, 0,
+       hwpt->domain = ops->domain_alloc_user(idev->dev,
+                                             flags & 
~IOMMU_HWPT_FAULT_ID_VALID,
                                               parent->common.domain, 
user_data);

>> @@ -308,6 +314,19 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd
>> *ucmd)
>>   		goto out_put_pt;
>>   	}
>>
>> +	if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
>> +		struct iommufd_fault *fault;
>> +
>> +		fault = iommufd_get_fault(ucmd, cmd->fault_id);
>> +		if (IS_ERR(fault)) {
>> +			rc = PTR_ERR(fault);
>> +			goto out_hwpt;
>> +		}
>> +		hwpt->fault = fault;
>> +		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
>> +		hwpt->domain->fault_data = hwpt;
>> +	}
> 
> this is nesting specific. why not moving it to the nested_alloc()?

Nesting is currently a use case for userspace I/O page faults, but this
design should be general enough to support other scenarios as well.

Best regards,
baolu
RE: [PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable
Posted by Tian, Kevin 1 year, 7 months ago
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Monday, May 20, 2024 10:19 AM
> 
> On 5/15/24 4:50 PM, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Tuesday, April 30, 2024 10:57 PM
> >>
> >> @@ -308,6 +314,19 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd
> >> *ucmd)
> >>   		goto out_put_pt;
> >>   	}
> >>
> >> +	if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
> >> +		struct iommufd_fault *fault;
> >> +
> >> +		fault = iommufd_get_fault(ucmd, cmd->fault_id);
> >> +		if (IS_ERR(fault)) {
> >> +			rc = PTR_ERR(fault);
> >> +			goto out_hwpt;
> >> +		}
> >> +		hwpt->fault = fault;
> >> +		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
> >> +		hwpt->domain->fault_data = hwpt;
> >> +	}
> >
> > this is nesting specific. why not moving it to the nested_alloc()?
> 
> Nesting is currently a use case for userspace I/O page faults, but this
> design should be general enough to support other scenarios as well.
> 

Do we allow user page table w/o nesting?

What would be a scenario in which the user doesn't manage the
page table but still want to handle the I/O page fault? The fault
should always be delivered to the owner managing the page table...
Re: [PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable
Posted by Jason Gunthorpe 1 year, 6 months ago
On Mon, May 20, 2024 at 03:39:54AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Monday, May 20, 2024 10:19 AM
> > 
> > On 5/15/24 4:50 PM, Tian, Kevin wrote:
> > >> From: Lu Baolu <baolu.lu@linux.intel.com>
> > >> Sent: Tuesday, April 30, 2024 10:57 PM
> > >>
> > >> @@ -308,6 +314,19 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd
> > >> *ucmd)
> > >>   		goto out_put_pt;
> > >>   	}
> > >>
> > >> +	if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
> > >> +		struct iommufd_fault *fault;
> > >> +
> > >> +		fault = iommufd_get_fault(ucmd, cmd->fault_id);
> > >> +		if (IS_ERR(fault)) {
> > >> +			rc = PTR_ERR(fault);
> > >> +			goto out_hwpt;
> > >> +		}
> > >> +		hwpt->fault = fault;
> > >> +		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
> > >> +		hwpt->domain->fault_data = hwpt;
> > >> +	}
> > >
> > > this is nesting specific. why not moving it to the nested_alloc()?
> > 
> > Nesting is currently a use case for userspace I/O page faults, but this
> > design should be general enough to support other scenarios as well.
> 
> Do we allow user page table w/o nesting?
> 
> What would be a scenario in which the user doesn't manage the
> page table but still want to handle the I/O page fault? The fault
> should always be delivered to the owner managing the page table...

userspace always manages the page table, either it updates the IOPTE
directly in a nest or it calls iommufd map operations.

Ideally the driver will allow PRI on normal cases, although it will
probably never be used.

Jason
RE: [PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable
Posted by Tian, Kevin 1 year, 6 months ago
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Friday, May 24, 2024 10:25 PM
> 
> On Mon, May 20, 2024 at 03:39:54AM +0000, Tian, Kevin wrote:
> > > From: Baolu Lu <baolu.lu@linux.intel.com>
> > > Sent: Monday, May 20, 2024 10:19 AM
> > >
> > > On 5/15/24 4:50 PM, Tian, Kevin wrote:
> > > >> From: Lu Baolu <baolu.lu@linux.intel.com>
> > > >> Sent: Tuesday, April 30, 2024 10:57 PM
> > > >>
> > > >> @@ -308,6 +314,19 @@ int iommufd_hwpt_alloc(struct
> iommufd_ucmd
> > > >> *ucmd)
> > > >>   		goto out_put_pt;
> > > >>   	}
> > > >>
> > > >> +	if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
> > > >> +		struct iommufd_fault *fault;
> > > >> +
> > > >> +		fault = iommufd_get_fault(ucmd, cmd->fault_id);
> > > >> +		if (IS_ERR(fault)) {
> > > >> +			rc = PTR_ERR(fault);
> > > >> +			goto out_hwpt;
> > > >> +		}
> > > >> +		hwpt->fault = fault;
> > > >> +		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
> > > >> +		hwpt->domain->fault_data = hwpt;
> > > >> +	}
> > > >
> > > > this is nesting specific. why not moving it to the nested_alloc()?
> > >
> > > Nesting is currently a use case for userspace I/O page faults, but this
> > > design should be general enough to support other scenarios as well.
> >
> > Do we allow user page table w/o nesting?
> >
> > What would be a scenario in which the user doesn't manage the
> > page table but still want to handle the I/O page fault? The fault
> > should always be delivered to the owner managing the page table...
> 
> userspace always manages the page table, either it updates the IOPTE
> directly in a nest or it calls iommufd map operations.
> 
> Ideally the driver will allow PRI on normal cases, although it will
> probably never be used.
> 

But now it's done in a half way.

valid_flags in normal cases doesn't accept a fault ID. but we then
handle the fault ID flag generally above.

I'd like to see a consistent message throughout the path. 
Re: [PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable
Posted by Baolu Lu 1 year, 6 months ago
On 5/27/24 9:33 AM, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@ziepe.ca>
>> Sent: Friday, May 24, 2024 10:25 PM
>>
>> On Mon, May 20, 2024 at 03:39:54AM +0000, Tian, Kevin wrote:
>>>> From: Baolu Lu <baolu.lu@linux.intel.com>
>>>> Sent: Monday, May 20, 2024 10:19 AM
>>>>
>>>> On 5/15/24 4:50 PM, Tian, Kevin wrote:
>>>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>>>> Sent: Tuesday, April 30, 2024 10:57 PM
>>>>>>
>>>>>> @@ -308,6 +314,19 @@ int iommufd_hwpt_alloc(struct
>> iommufd_ucmd
>>>>>> *ucmd)
>>>>>>    		goto out_put_pt;
>>>>>>    	}
>>>>>>
>>>>>> +	if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
>>>>>> +		struct iommufd_fault *fault;
>>>>>> +
>>>>>> +		fault = iommufd_get_fault(ucmd, cmd->fault_id);
>>>>>> +		if (IS_ERR(fault)) {
>>>>>> +			rc = PTR_ERR(fault);
>>>>>> +			goto out_hwpt;
>>>>>> +		}
>>>>>> +		hwpt->fault = fault;
>>>>>> +		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
>>>>>> +		hwpt->domain->fault_data = hwpt;
>>>>>> +	}
>>>>>
>>>>> this is nesting specific. why not moving it to the nested_alloc()?
>>>>
>>>> Nesting is currently a use case for userspace I/O page faults, but this
>>>> design should be general enough to support other scenarios as well.
>>>
>>> Do we allow user page table w/o nesting?
>>>
>>> What would be a scenario in which the user doesn't manage the
>>> page table but still want to handle the I/O page fault? The fault
>>> should always be delivered to the owner managing the page table...
>>
>> userspace always manages the page table, either it updates the IOPTE
>> directly in a nest or it calls iommufd map operations.
>>
>> Ideally the driver will allow PRI on normal cases, although it will
>> probably never be used.
>>
> 
> But now it's done in a half way.
> 
> valid_flags in normal cases doesn't accept a fault ID. but we then
> handle the fault ID flag generally above.
> 
> I'd like to see a consistent message throughout the path.

Okay, I see. I think valid_flags logic is doing the right thing. It
indicates that user space page fault on a paging hwpt is not supported
yet, but it leaves the room to grow it in the future.

I will post v6 of this series soon to address some obvious issues
identified during this v5 review cycle. Thanks to all review comments.

Best regards,
baolu
Re: [PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable
Posted by Baolu Lu 1 year, 7 months ago
On 5/20/24 11:39 AM, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Monday, May 20, 2024 10:19 AM
>>
>> On 5/15/24 4:50 PM, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Sent: Tuesday, April 30, 2024 10:57 PM
>>>>
>>>> @@ -308,6 +314,19 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd
>>>> *ucmd)
>>>>    		goto out_put_pt;
>>>>    	}
>>>>
>>>> +	if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
>>>> +		struct iommufd_fault *fault;
>>>> +
>>>> +		fault = iommufd_get_fault(ucmd, cmd->fault_id);
>>>> +		if (IS_ERR(fault)) {
>>>> +			rc = PTR_ERR(fault);
>>>> +			goto out_hwpt;
>>>> +		}
>>>> +		hwpt->fault = fault;
>>>> +		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
>>>> +		hwpt->domain->fault_data = hwpt;
>>>> +	}
>>>
>>> this is nesting specific. why not moving it to the nested_alloc()?
>>
>> Nesting is currently a use case for userspace I/O page faults, but this
>> design should be general enough to support other scenarios as well.
>>
> 
> Do we allow user page table w/o nesting?
> 
> What would be a scenario in which the user doesn't manage the
> page table but still want to handle the I/O page fault? The fault
> should always be delivered to the owner managing the page table...

I am not sure. But if nesting is the only case for user page table, it's
fine to move above code to the nested_alloc helper.

Best regards,
baolu
Re: [PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable
Posted by Jason Gunthorpe 1 year, 7 months ago
On Tue, Apr 30, 2024 at 10:57:08PM +0800, Lu Baolu wrote:
>  /**
> @@ -412,6 +415,9 @@ enum iommu_hwpt_data_type {
>   * @data_type: One of enum iommu_hwpt_data_type
>   * @data_len: Length of the type specific data
>   * @data_uptr: User pointer to the type specific data
> + * @fault_id: The ID of IOMMUFD_FAULT object. Valid only if flags field of
> + *            IOMMU_HWPT_FAULT_ID_VALID is set.
> + * @__reserved2: Padding to 64-bit alignment. Must be 0.
>   *
>   * Explicitly allocate a hardware page table object. This is the same object
>   * type that is returned by iommufd_device_attach() and represents the
> @@ -442,6 +448,8 @@ struct iommu_hwpt_alloc {
>  	__u32 data_type;
>  	__u32 data_len;
>  	__aligned_u64 data_uptr;
> +	__u32 fault_id;
> +	__u32 __reserved2;
>  };
>  #define IOMMU_HWPT_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_ALLOC)

[..]

> @@ -359,7 +359,7 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
>  	IOCTL_OP(IOMMU_GET_HW_INFO, iommufd_get_hw_info, struct iommu_hw_info,
>  		 __reserved),
>  	IOCTL_OP(IOMMU_HWPT_ALLOC, iommufd_hwpt_alloc, struct iommu_hwpt_alloc,
> -		 __reserved),
> +		 __reserved2),

This is now how the back compat mechanism works. The value here is the
absolute minimum size, it should never increase. The first __reserved
is always the right value.

If you change it then old userspace that doesn't include the fault_id
will stop working.

Jason
Re: [PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable
Posted by Baolu Lu 1 year, 7 months ago
On 5/8/24 8:25 AM, Jason Gunthorpe wrote:
> On Tue, Apr 30, 2024 at 10:57:08PM +0800, Lu Baolu wrote:
>>   /**
>> @@ -412,6 +415,9 @@ enum iommu_hwpt_data_type {
>>    * @data_type: One of enum iommu_hwpt_data_type
>>    * @data_len: Length of the type specific data
>>    * @data_uptr: User pointer to the type specific data
>> + * @fault_id: The ID of IOMMUFD_FAULT object. Valid only if flags field of
>> + *            IOMMU_HWPT_FAULT_ID_VALID is set.
>> + * @__reserved2: Padding to 64-bit alignment. Must be 0.
>>    *
>>    * Explicitly allocate a hardware page table object. This is the same object
>>    * type that is returned by iommufd_device_attach() and represents the
>> @@ -442,6 +448,8 @@ struct iommu_hwpt_alloc {
>>   	__u32 data_type;
>>   	__u32 data_len;
>>   	__aligned_u64 data_uptr;
>> +	__u32 fault_id;
>> +	__u32 __reserved2;
>>   };
>>   #define IOMMU_HWPT_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_ALLOC)
> [..]
> 
>> @@ -359,7 +359,7 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
>>   	IOCTL_OP(IOMMU_GET_HW_INFO, iommufd_get_hw_info, struct iommu_hw_info,
>>   		 __reserved),
>>   	IOCTL_OP(IOMMU_HWPT_ALLOC, iommufd_hwpt_alloc, struct iommu_hwpt_alloc,
>> -		 __reserved),
>> +		 __reserved2),
> This is now how the back compat mechanism works. The value here is the
> absolute minimum size, it should never increase. The first __reserved
> is always the right value.
> 
> If you change it then old userspace that doesn't include the fault_id
> will stop working.

Yeah! I will remove this change.

Best regards,
baolu