diff mbox series

[v5,09/12] iommu: Make iommu_queue_iopf() more generic

Message ID 20230914085638.17307-10-baolu.lu@linux.intel.com
State New
Headers
Series iommu: Prepare to deliver page faults to user space |

Commit Message

Baolu Lu Sept. 14, 2023, 8:56 a.m. UTC 
  Make iommu_queue_iopf() more generic by making the iopf_group a minimal
set of iopf's that an iopf handler of domain should handle and respond
to. Add domain parameter to struct iopf_group so that the handler can
retrieve and use it directly.

Change iommu_queue_iopf() to forward groups of iopf's to the domain's
iopf handler. This is also a necessary step to decouple the sva iopf
handling code from this interface.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h      |  4 ++--
 drivers/iommu/iommu-sva.h  |  6 ++---
 drivers/iommu/io-pgfault.c | 49 ++++++++++++++++++++++++++++----------
 drivers/iommu/iommu-sva.c  |  3 +--
 4 files changed, 42 insertions(+), 20 deletions(-)

Comments

Liu, Jingqi Sept. 21, 2023, 3:25 p.m. UTC | #1 
On 9/14/2023 4:56 PM, Lu Baolu wrote:
> Make iommu_queue_iopf() more generic by making the iopf_group a minimal
> set of iopf's that an iopf handler of domain should handle and respond
> to. Add domain parameter to struct iopf_group so that the handler can
> retrieve and use it directly.
>
> Change iommu_queue_iopf() to forward groups of iopf's to the domain's
> iopf handler. This is also a necessary step to decouple the sva iopf
> handling code from this interface.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   include/linux/iommu.h      |  4 ++--
>   drivers/iommu/iommu-sva.h  |  6 ++---
>   drivers/iommu/io-pgfault.c | 49 ++++++++++++++++++++++++++++----------
>   drivers/iommu/iommu-sva.c  |  3 +--
>   4 files changed, 42 insertions(+), 20 deletions(-)
>
......

> @@ -112,6 +110,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
>   {
>   	int ret;
>   	struct iopf_group *group;
> +	struct iommu_domain *domain;
>   	struct iopf_fault *iopf, *next;
>   	struct iommu_fault_param *iopf_param;
>   	struct dev_iommu *param = dev->iommu;
> @@ -143,6 +142,19 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
>   		return 0;
>   	}
>   
> +	if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
> +		domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
> +	else
> +		domain = iommu_get_domain_for_dev(dev);
> +
> +	if (!domain || !domain->iopf_handler) {

Does it need to check if 'domain' is error ?  Like below:

          if (!domain || IS_ERR(domain) || !domain->iopf_handler)
Thanks,
Jingqi
Jason Gunthorpe Sept. 21, 2023, 11:34 p.m. UTC | #2 
On Thu, Sep 21, 2023 at 11:25:56PM +0800, Liu, Jingqi wrote:
> 
> On 9/14/2023 4:56 PM, Lu Baolu wrote:
> > Make iommu_queue_iopf() more generic by making the iopf_group a minimal
> > set of iopf's that an iopf handler of domain should handle and respond
> > to. Add domain parameter to struct iopf_group so that the handler can
> > retrieve and use it directly.
> > 
> > Change iommu_queue_iopf() to forward groups of iopf's to the domain's
> > iopf handler. This is also a necessary step to decouple the sva iopf
> > handling code from this interface.
> > 
> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > ---
> >   include/linux/iommu.h      |  4 ++--
> >   drivers/iommu/iommu-sva.h  |  6 ++---
> >   drivers/iommu/io-pgfault.c | 49 ++++++++++++++++++++++++++++----------
> >   drivers/iommu/iommu-sva.c  |  3 +--
> >   4 files changed, 42 insertions(+), 20 deletions(-)
> > 
> ......
> 
> > @@ -112,6 +110,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
> >   {
> >   	int ret;
> >   	struct iopf_group *group;
> > +	struct iommu_domain *domain;
> >   	struct iopf_fault *iopf, *next;
> >   	struct iommu_fault_param *iopf_param;
> >   	struct dev_iommu *param = dev->iommu;
> > @@ -143,6 +142,19 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
> >   		return 0;
> >   	}
> > +	if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
> > +		domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
> > +	else
> > +		domain = iommu_get_domain_for_dev(dev);
> > +
> > +	if (!domain || !domain->iopf_handler) {
> 
> Does it need to check if 'domain' is error ?  Like below:
> 
>          if (!domain || IS_ERR(domain) || !domain->iopf_handler)

Urk, yes, but not like that

The IF needs to be moved into the else block as each individual
function has its own return convention.

Jason
Baolu Lu Sept. 22, 2023, 2:44 a.m. UTC | #3 
On 9/22/23 7:34 AM, Jason Gunthorpe wrote:
> On Thu, Sep 21, 2023 at 11:25:56PM +0800, Liu, Jingqi wrote:
>>
>> On 9/14/2023 4:56 PM, Lu Baolu wrote:
>>> Make iommu_queue_iopf() more generic by making the iopf_group a minimal
>>> set of iopf's that an iopf handler of domain should handle and respond
>>> to. Add domain parameter to struct iopf_group so that the handler can
>>> retrieve and use it directly.
>>>
>>> Change iommu_queue_iopf() to forward groups of iopf's to the domain's
>>> iopf handler. This is also a necessary step to decouple the sva iopf
>>> handling code from this interface.
>>>
>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>> ---
>>>    include/linux/iommu.h      |  4 ++--
>>>    drivers/iommu/iommu-sva.h  |  6 ++---
>>>    drivers/iommu/io-pgfault.c | 49 ++++++++++++++++++++++++++++----------
>>>    drivers/iommu/iommu-sva.c  |  3 +--
>>>    4 files changed, 42 insertions(+), 20 deletions(-)
>>>
>> ......
>>
>>> @@ -112,6 +110,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
>>>    {
>>>    	int ret;
>>>    	struct iopf_group *group;
>>> +	struct iommu_domain *domain;
>>>    	struct iopf_fault *iopf, *next;
>>>    	struct iommu_fault_param *iopf_param;
>>>    	struct dev_iommu *param = dev->iommu;
>>> @@ -143,6 +142,19 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
>>>    		return 0;
>>>    	}
>>> +	if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
>>> +		domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
>>> +	else
>>> +		domain = iommu_get_domain_for_dev(dev);
>>> +
>>> +	if (!domain || !domain->iopf_handler) {
>>
>> Does it need to check if 'domain' is error ?  Like below:
>>
>>           if (!domain || IS_ERR(domain) || !domain->iopf_handler)
> 
> Urk, yes, but not like that
> 
> The IF needs to be moved into the else block as each individual
> function has its own return convention.

iommu_get_domain_for_dev_pasid() returns an ERR_PTR only if the matching
domain type is specified (non-zero).

Adding IS_ERR(domain) in the else block will make the code more
readable. Alternatively we can put a comment around above code to
explain that ERR_PTR is not a case here.

Best regards,
baolu
Jason Gunthorpe Sept. 22, 2023, 12:43 p.m. UTC | #4 
On Fri, Sep 22, 2023 at 10:44:45AM +0800, Baolu Lu wrote:

> > > > @@ -112,6 +110,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
> > > >    {
> > > >    	int ret;
> > > >    	struct iopf_group *group;
> > > > +	struct iommu_domain *domain;
> > > >    	struct iopf_fault *iopf, *next;
> > > >    	struct iommu_fault_param *iopf_param;
> > > >    	struct dev_iommu *param = dev->iommu;
> > > > @@ -143,6 +142,19 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
> > > >    		return 0;
> > > >    	}
> > > > +	if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
> > > > +		domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
> > > > +	else
> > > > +		domain = iommu_get_domain_for_dev(dev);
> > > > +
> > > > +	if (!domain || !domain->iopf_handler) {
> > > 
> > > Does it need to check if 'domain' is error ?  Like below:
> > > 
> > >           if (!domain || IS_ERR(domain) || !domain->iopf_handler)
> > 
> > Urk, yes, but not like that
> > 
> > The IF needs to be moved into the else block as each individual
> > function has its own return convention.
> 
> iommu_get_domain_for_dev_pasid() returns an ERR_PTR only if the matching
> domain type is specified (non-zero).
> 
> Adding IS_ERR(domain) in the else block will make the code more
> readable. Alternatively we can put a comment around above code to
> explain that ERR_PTR is not a case here.

You should check it because you'll probably get a static tool
complaint otherwise

Jason
Baolu Lu Sept. 22, 2023, 12:47 p.m. UTC | #5 
On 2023/9/22 20:43, Jason Gunthorpe wrote:
> On Fri, Sep 22, 2023 at 10:44:45AM +0800, Baolu Lu wrote:
> 
>>>>> @@ -112,6 +110,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
>>>>>     {
>>>>>     	int ret;
>>>>>     	struct iopf_group *group;
>>>>> +	struct iommu_domain *domain;
>>>>>     	struct iopf_fault *iopf, *next;
>>>>>     	struct iommu_fault_param *iopf_param;
>>>>>     	struct dev_iommu *param = dev->iommu;
>>>>> @@ -143,6 +142,19 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
>>>>>     		return 0;
>>>>>     	}
>>>>> +	if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
>>>>> +		domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
>>>>> +	else
>>>>> +		domain = iommu_get_domain_for_dev(dev);
>>>>> +
>>>>> +	if (!domain || !domain->iopf_handler) {
>>>> Does it need to check if 'domain' is error ?  Like below:
>>>>
>>>>            if (!domain || IS_ERR(domain) || !domain->iopf_handler)
>>> Urk, yes, but not like that
>>>
>>> The IF needs to be moved into the else block as each individual
>>> function has its own return convention.
>> iommu_get_domain_for_dev_pasid() returns an ERR_PTR only if the matching
>> domain type is specified (non-zero).
>>
>> Adding IS_ERR(domain) in the else block will make the code more
>> readable. Alternatively we can put a comment around above code to
>> explain that ERR_PTR is not a case here.
> You should check it because you'll probably get a static tool
> complaint otherwise

Okay, got you.

Best regards,
baolu
Tian, Kevin Sept. 25, 2023, 6:49 a.m. UTC | #6 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, September 14, 2023 4:57 PM
> 
> Make iommu_queue_iopf() more generic by making the iopf_group a
> minimal
> set of iopf's that an iopf handler of domain should handle and respond
> to. Add domain parameter to struct iopf_group so that the handler can
> retrieve and use it directly.
> 
> Change iommu_queue_iopf() to forward groups of iopf's to the domain's
> iopf handler. This is also a necessary step to decouple the sva iopf
> handling code from this interface.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

With the fix of domain error check:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
diff mbox series

Patch

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b179594fc378..da47813c2e4c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -128,6 +128,7 @@  struct iopf_group {
 	struct list_head faults;
 	struct work_struct work;
 	struct device *dev;
+	struct iommu_domain *domain;
 };
 
 /**
@@ -197,8 +198,7 @@  struct iommu_domain {
 	unsigned long pgsize_bitmap;	/* Bitmap of page sizes in use */
 	struct iommu_domain_geometry geometry;
 	struct iommu_dma_cookie *iova_cookie;
-	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
-						      void *data);
+	int (*iopf_handler)(struct iopf_group *group);
 	void *fault_data;
 	union {
 		struct {
diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
index de7819c796ce..27c8da115b41 100644
--- a/drivers/iommu/iommu-sva.h
+++ b/drivers/iommu/iommu-sva.h
@@ -22,8 +22,7 @@  int iopf_queue_flush_dev(struct device *dev);
 struct iopf_queue *iopf_queue_alloc(const char *name);
 void iopf_queue_free(struct iopf_queue *queue);
 int iopf_queue_discard_partial(struct iopf_queue *queue);
-enum iommu_page_response_code
-iommu_sva_handle_iopf(struct iommu_fault *fault, void *data);
+int iommu_sva_handle_iopf(struct iopf_group *group);
 
 #else /* CONFIG_IOMMU_SVA */
 static inline int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
@@ -62,8 +61,7 @@  static inline int iopf_queue_discard_partial(struct iopf_queue *queue)
 	return -ENODEV;
 }
 
-static inline enum iommu_page_response_code
-iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
+static inline int iommu_sva_handle_iopf(struct iopf_group *group)
 {
 	return IOMMU_PAGE_RESP_INVALID;
 }
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 09e05f483b4f..6533f9d0e37b 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -13,6 +13,9 @@ 
 
 #include "iommu-sva.h"
 
+enum iommu_page_response_code
+iommu_sva_handle_mm(struct iommu_fault *fault, struct mm_struct *mm);
+
 static void iopf_free_group(struct iopf_group *group)
 {
 	struct iopf_fault *iopf, *next;
@@ -45,23 +48,18 @@  static void iopf_handler(struct work_struct *work)
 {
 	struct iopf_fault *iopf;
 	struct iopf_group *group;
-	struct iommu_domain *domain;
 	enum iommu_page_response_code status = IOMMU_PAGE_RESP_SUCCESS;
 
 	group = container_of(work, struct iopf_group, work);
-	domain = iommu_get_domain_for_dev_pasid(group->dev,
-				group->last_fault.fault.prm.pasid, 0);
-	if (!domain || !domain->iopf_handler)
-		status = IOMMU_PAGE_RESP_INVALID;
-
 	list_for_each_entry(iopf, &group->faults, list) {
 		/*
 		 * For the moment, errors are sticky: don't handle subsequent
 		 * faults in the group if there is an error.
 		 */
-		if (status == IOMMU_PAGE_RESP_SUCCESS)
-			status = domain->iopf_handler(&iopf->fault,
-						      domain->fault_data);
+		if (status != IOMMU_PAGE_RESP_SUCCESS)
+			break;
+
+		status = iommu_sva_handle_mm(&iopf->fault, group->domain->mm);
 	}
 
 	iopf_complete_group(group->dev, &group->last_fault, status);
@@ -112,6 +110,7 @@  int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
 {
 	int ret;
 	struct iopf_group *group;
+	struct iommu_domain *domain;
 	struct iopf_fault *iopf, *next;
 	struct iommu_fault_param *iopf_param;
 	struct dev_iommu *param = dev->iommu;
@@ -143,6 +142,19 @@  int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
 		return 0;
 	}
 
+	if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
+		domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
+	else
+		domain = iommu_get_domain_for_dev(dev);
+
+	if (!domain || !domain->iopf_handler) {
+		dev_warn_ratelimited(dev,
+			"iopf (pasid %d) without domain attached or handler installed\n",
+			 fault->prm.pasid);
+		ret = -ENODEV;
+		goto cleanup_partial;
+	}
+
 	group = kzalloc(sizeof(*group), GFP_KERNEL);
 	if (!group) {
 		/*
@@ -157,8 +169,8 @@  int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
 	group->dev = dev;
 	group->last_fault.fault = *fault;
 	INIT_LIST_HEAD(&group->faults);
+	group->domain = domain;
 	list_add(&group->last_fault.list, &group->faults);
-	INIT_WORK(&group->work, iopf_handler);
 
 	/* See if we have partial faults for this group */
 	list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
@@ -167,9 +179,11 @@  int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
 			list_move(&iopf->list, &group->faults);
 	}
 
-	queue_work(iopf_param->queue->wq, &group->work);
-	return 0;
+	ret = domain->iopf_handler(group);
+	if (ret)
+		iopf_free_group(group);
 
+	return ret;
 cleanup_partial:
 	list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
 		if (iopf->fault.prm.grpid == fault->prm.grpid) {
@@ -181,6 +195,17 @@  int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_queue_iopf);
 
+int iommu_sva_handle_iopf(struct iopf_group *group)
+{
+	struct iommu_fault_param *fault_param = group->dev->iommu->fault_param;
+
+	INIT_WORK(&group->work, iopf_handler);
+	if (!queue_work(fault_param->queue->wq, &group->work))
+		return -EBUSY;
+
+	return 0;
+}
+
 /**
  * iopf_queue_flush_dev - Ensure that all queued faults have been processed
  * @dev: the endpoint whose faults need to be flushed.
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index b78671a8a914..ba0d5b7e106a 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -149,11 +149,10 @@  EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
  * I/O page fault handler for SVA
  */
 enum iommu_page_response_code
-iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
+iommu_sva_handle_mm(struct iommu_fault *fault, struct mm_struct *mm)
 {
 	vm_fault_t ret;
 	struct vm_area_struct *vma;
-	struct mm_struct *mm = data;
 	unsigned int access_flags = 0;
 	unsigned int fault_flags = FAULT_FLAG_REMOTE;
 	struct iommu_fault_page_request *prm = &fault->prm;