[PATCH RFCv2 05/13] iommu: Turn fault_data to iommufd private pointer

Nicolin Chen posted 13 patches 1 year ago
There is a newer version of this series
[PATCH RFCv2 05/13] iommu: Turn fault_data to iommufd private pointer
Posted by Nicolin Chen 1 year ago
A "fault_data" was added exclusively for the iommufd_fault_iopf_handler()
used by IOPF/PRI use cases, along with the attach_handle. Now, the iommufd
version of sw_msi function will resue the attach_handle and fault_data for
a non-fault case.

Rename "fault_data" to "iommufd_hwpt" so as not to confine it to a "fault"
case. Move it into a union to be the iommufd private pointer. A following
patch will move the iova_cookie to the union for dma-iommu too, after the
iommufd_sw_msi implementation is added.

Since we have two unions now, add some simple comments for readability.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 include/linux/iommu.h                | 6 ++++--
 drivers/iommu/iommufd/fault.c        | 2 +-
 drivers/iommu/iommufd/hw_pagetable.c | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 423fdfa6b3bb..b6526d734f30 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -224,8 +224,10 @@ struct iommu_domain {
 		      phys_addr_t msi_addr);
 #endif
 
-	void *fault_data;
-	union {
+	union { /* Pointer usable by owner of the domain */
+		struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
+	};
+	union { /* Fault handler */
 		struct {
 			iommu_fault_handler_t handler;
 			void *handler_token;
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index 1fe804e28a86..06aa83a75e94 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -431,7 +431,7 @@ int iommufd_fault_iopf_handler(struct iopf_group *group)
 	struct iommufd_hw_pagetable *hwpt;
 	struct iommufd_fault *fault;
 
-	hwpt = group->attach_handle->domain->fault_data;
+	hwpt = group->attach_handle->domain->iommufd_hwpt;
 	fault = hwpt->fault;
 
 	mutex_lock(&fault->mutex);
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index ce03c3804651..f7c0d7b214b6 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -402,10 +402,10 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
 		}
 		hwpt->fault = fault;
 		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
-		hwpt->domain->fault_data = hwpt;
 		refcount_inc(&fault->obj.users);
 		iommufd_put_object(ucmd->ictx, &fault->obj);
 	}
+	hwpt->domain->iommufd_hwpt = hwpt;
 
 	cmd->out_hwpt_id = hwpt->obj.id;
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
-- 
2.43.0
Re: [PATCH RFCv2 05/13] iommu: Turn fault_data to iommufd private pointer
Posted by Eric Auger 1 year ago


On 1/11/25 4:32 AM, Nicolin Chen wrote:
> A "fault_data" was added exclusively for the iommufd_fault_iopf_handler()
> used by IOPF/PRI use cases, along with the attach_handle. Now, the iommufd
> version of sw_msi function will resue the attach_handle and fault_data for
reuse
> a non-fault case.
>
> Rename "fault_data" to "iommufd_hwpt" so as not to confine it to a "fault"
> case. Move it into a union to be the iommufd private pointer. A following
> patch will move the iova_cookie to the union for dma-iommu too, after the
> iommufd_sw_msi implementation is added.
>
> Since we have two unions now, add some simple comments for readability.
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  include/linux/iommu.h                | 6 ++++--
>  drivers/iommu/iommufd/fault.c        | 2 +-
>  drivers/iommu/iommufd/hw_pagetable.c | 2 +-
>  3 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 423fdfa6b3bb..b6526d734f30 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -224,8 +224,10 @@ struct iommu_domain {
>  		      phys_addr_t msi_addr);
>  #endif
>  
> -	void *fault_data;
> -	union {
> +	union { /* Pointer usable by owner of the domain */
> +		struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd */
> +	};
> +	union { /* Fault handler */
>  		struct {
>  			iommu_fault_handler_t handler;
>  			void *handler_token;
> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
> index 1fe804e28a86..06aa83a75e94 100644
> --- a/drivers/iommu/iommufd/fault.c
> +++ b/drivers/iommu/iommufd/fault.c
> @@ -431,7 +431,7 @@ int iommufd_fault_iopf_handler(struct iopf_group *group)
>  	struct iommufd_hw_pagetable *hwpt;
>  	struct iommufd_fault *fault;
>  
> -	hwpt = group->attach_handle->domain->fault_data;
> +	hwpt = group->attach_handle->domain->iommufd_hwpt;
>  	fault = hwpt->fault;
>  
>  	mutex_lock(&fault->mutex);
> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> index ce03c3804651..f7c0d7b214b6 100644
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -402,10 +402,10 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
>  		}
>  		hwpt->fault = fault;
>  		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
> -		hwpt->domain->fault_data = hwpt;
>  		refcount_inc(&fault->obj.users);
>  		iommufd_put_object(ucmd->ictx, &fault->obj);
>  	}
> +	hwpt->domain->iommufd_hwpt = hwpt;
don't we want to reset this somewhere on release path?

Eric
>  
>  	cmd->out_hwpt_id = hwpt->obj.id;
>  	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
Re: [PATCH RFCv2 05/13] iommu: Turn fault_data to iommufd private pointer
Posted by Nicolin Chen 1 year ago
Hi Eric,

On Wed, Jan 29, 2025 at 01:40:54PM +0100, Eric Auger wrote:
> On 1/11/25 4:32 AM, Nicolin Chen wrote:
> > A "fault_data" was added exclusively for the iommufd_fault_iopf_handler()
> > used by IOPF/PRI use cases, along with the attach_handle. Now, the iommufd
> > version of sw_msi function will resue the attach_handle and fault_data for
> reuse

Ack.

> > diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> > index ce03c3804651..f7c0d7b214b6 100644
> > --- a/drivers/iommu/iommufd/hw_pagetable.c
> > +++ b/drivers/iommu/iommufd/hw_pagetable.c
> > @@ -402,10 +402,10 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
> >  		}
> >  		hwpt->fault = fault;
> >  		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
> > -		hwpt->domain->fault_data = hwpt;
> >  		refcount_inc(&fault->obj.users);
> >  		iommufd_put_object(ucmd->ictx, &fault->obj);
> >  	}
> > +	hwpt->domain->iommufd_hwpt = hwpt;

> don't we want to reset this somewhere on release path?

We do iommu_domain_free() entirely on HWPT's release path.

This basically sets the domain's "owner data" as Jason remarked:
https://lore.kernel.org/linux-iommu/20250113164037.GO5556@nvidia.com/

Thanks
Nicolin
RE: [PATCH RFCv2 05/13] iommu: Turn fault_data to iommufd private pointer
Posted by Tian, Kevin 1 year ago
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Saturday, January 11, 2025 11:32 AM
> 
> @@ -224,8 +224,10 @@ struct iommu_domain {
>  		      phys_addr_t msi_addr);
>  #endif
> 
> -	void *fault_data;
> -	union {
> +	union { /* Pointer usable by owner of the domain */
> +		struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd
> */
> +	};
> +	union { /* Fault handler */

hmm is it better to rename it as "void *private;" and let the caller
do type conversion?
Re: [PATCH RFCv2 05/13] iommu: Turn fault_data to iommufd private pointer
Posted by Jason Gunthorpe 1 year ago
On Thu, Jan 23, 2025 at 09:54:38AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Saturday, January 11, 2025 11:32 AM
> > 
> > @@ -224,8 +224,10 @@ struct iommu_domain {
> >  		      phys_addr_t msi_addr);
> >  #endif
> > 
> > -	void *fault_data;
> > -	union {
> > +	union { /* Pointer usable by owner of the domain */
> > +		struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd
> > */
> > +	};
> > +	union { /* Fault handler */
> 
> hmm is it better to rename it as "void *private;" and let the caller
> do type conversion?

I like the type safety, the union can hold other in-kernel users with
their proper types and this discourages drivers from inventing weird
things..

Jason