diff mbox series

[9/9] iommu: Use fault cookie to store iopf_param

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

Commit Message

Baolu Lu July 11, 2023, 1:06 a.m. UTC 
  Remove the static iopf_param pointer from struct iommu_fault_param to
save memory.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h      |  2 --
 drivers/iommu/io-pgfault.c | 47 +++++++++++++++++++++++---------------
 2 files changed, 29 insertions(+), 20 deletions(-)

Comments

Tian, Kevin July 11, 2023, 6:26 a.m. UTC | #1 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, July 11, 2023 9:07 AM
> 
> Remove the static iopf_param pointer from struct iommu_fault_param to
> save memory.

why is there memory saving? you replace a single pointer with a xarray now...

> @@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue
> *queue, struct device *dev)
> 
>  	mutex_lock(&queue->lock);
>  	mutex_lock(&param->lock);
> -	if (!param->iopf_param) {
> -		list_add(&iopf_param->queue_list, &queue->devices);
> -		param->iopf_param = iopf_param;
> -		ret = 0;
> +	curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
> +	if (IS_ERR(curr)) {
> +		ret = PTR_ERR(curr);
> +		goto err_free;
>  	}

So although the new xarray is called a per-pasid storage, here only
slot#0 is used for sva which includes a list containing partial req's
for many pasid's. It doesn't sound clean...
Jacob Pan July 11, 2023, 10:02 p.m. UTC | #2 
Hi BaoLu,

On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> Remove the static iopf_param pointer from struct iommu_fault_param to
> save memory.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/linux/iommu.h      |  2 --
>  drivers/iommu/io-pgfault.c | 47 +++++++++++++++++++++++---------------
>  2 files changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ffd6fe1317f4..5fe37a7c5a55 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -551,7 +551,6 @@ struct iommu_fault_param {
>   * struct dev_iommu - Collection of per-device IOMMU data
>   *
>   * @fault_param: IOMMU detected device fault reporting data
> - * @iopf_param:	 I/O Page Fault queue and data
>   * @fwspec:	 IOMMU fwspec data
>   * @iommu_dev:	 IOMMU device this device is linked to
>   * @priv:	 IOMMU Driver private data
> @@ -564,7 +563,6 @@ struct iommu_fault_param {
>  struct dev_iommu {
>  	struct mutex lock;
>  	struct iommu_fault_param	*fault_param;
> -	struct iopf_device_param	*iopf_param;
>  	struct iommu_fwspec		*fwspec;
>  	struct iommu_device		*iommu_dev;
>  	void				*priv;
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 1749e0869f2e..6a3a4e08e67e 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
> struct device *dev)
>  	 * As long as we're holding param->lock, the queue can't be
> unlinked
>  	 * from the device and therefore cannot disappear.
>  	 */
> -	iopf_param = param->iopf_param;
> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
I am not sure I understand how does it know the cookie type is iopf_param
for PASID 0?

Between IOPF and IOMMUFD use of the cookie, cookie types are different,
right?

>  	if (!iopf_param)
>  		return -ENODEV;
>  
> @@ -235,7 +235,7 @@ int iopf_queue_flush_dev(struct device *dev)
>  		return -ENODEV;
>  
>  	mutex_lock(&param->lock);
> -	iopf_param = param->iopf_param;
> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
>  	if (iopf_param)
>  		flush_workqueue(iopf_param->queue->wq);
>  	else
> @@ -286,9 +286,9 @@ EXPORT_SYMBOL_GPL(iopf_queue_discard_partial);
>   */
>  int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
>  {
> -	int ret = -EBUSY;
> -	struct iopf_device_param *iopf_param;
> +	struct iopf_device_param *iopf_param, *curr;
>  	struct dev_iommu *param = dev->iommu;
> +	int ret;
>  
>  	if (!param)
>  		return -ENODEV;
> @@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue *queue,
> struct device *dev) 
>  	mutex_lock(&queue->lock);
>  	mutex_lock(&param->lock);
> -	if (!param->iopf_param) {
> -		list_add(&iopf_param->queue_list, &queue->devices);
> -		param->iopf_param = iopf_param;
> -		ret = 0;
> +	curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
> +	if (IS_ERR(curr)) {
> +		ret = PTR_ERR(curr);
> +		goto err_free;
>  	}
> +
> +	if (curr) {
> +		ret = -EBUSY;
> +		goto err_restore;
> +	}
> +	list_add(&iopf_param->queue_list, &queue->devices);
>  	mutex_unlock(&param->lock);
>  	mutex_unlock(&queue->lock);
>  
> -	if (ret)
> -		kfree(iopf_param);
> +	return 0;
> +err_restore:
> +	iommu_set_device_fault_cookie(dev, 0, curr);
> +err_free:
> +	mutex_unlock(&param->lock);
> +	mutex_unlock(&queue->lock);
> +	kfree(iopf_param);
>  
>  	return ret;
>  }
> @@ -329,7 +340,6 @@ EXPORT_SYMBOL_GPL(iopf_queue_add_device);
>   */
>  int iopf_queue_remove_device(struct iopf_queue *queue, struct device
> *dev) {
> -	int ret = -EINVAL;
>  	struct iopf_fault *iopf, *next;
>  	struct iopf_device_param *iopf_param;
>  	struct dev_iommu *param = dev->iommu;
> @@ -339,16 +349,17 @@ int iopf_queue_remove_device(struct iopf_queue
> *queue, struct device *dev) 
>  	mutex_lock(&queue->lock);
>  	mutex_lock(&param->lock);
> -	iopf_param = param->iopf_param;
> -	if (iopf_param && iopf_param->queue == queue) {
> -		list_del(&iopf_param->queue_list);
> -		param->iopf_param = NULL;
> -		ret = 0;
> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
> +	if (!iopf_param || iopf_param->queue != queue) {
> +		mutex_unlock(&param->lock);
> +		mutex_unlock(&queue->lock);
> +		return -EINVAL;
>  	}
> +
> +	list_del(&iopf_param->queue_list);
> +	iommu_set_device_fault_cookie(dev, 0, NULL);
>  	mutex_unlock(&param->lock);
>  	mutex_unlock(&queue->lock);
> -	if (ret)
> -		return ret;
>  
>  	/* Just in case some faults are still stuck */
>  	list_for_each_entry_safe(iopf, next, &iopf_param->partial, list)


Thanks,

Jacob
Baolu Lu July 12, 2023, 3:09 a.m. UTC | #3 
On 2023/7/11 14:26, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, July 11, 2023 9:07 AM
>>
>> Remove the static iopf_param pointer from struct iommu_fault_param to
>> save memory.
> 
> why is there memory saving? you replace a single pointer with a xarray now...

iopf_param is duplicate with the fault cookie. So replace it with the
fault cookie to remove duplication and save memory.

> 
>> @@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue
>> *queue, struct device *dev)
>>
>>   	mutex_lock(&queue->lock);
>>   	mutex_lock(&param->lock);
>> -	if (!param->iopf_param) {
>> -		list_add(&iopf_param->queue_list, &queue->devices);
>> -		param->iopf_param = iopf_param;
>> -		ret = 0;
>> +	curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
>> +	if (IS_ERR(curr)) {
>> +		ret = PTR_ERR(curr);
>> +		goto err_free;
>>   	}
> 
> So although the new xarray is called a per-pasid storage, here only
> slot#0 is used for sva which includes a list containing partial req's
> for many pasid's. It doesn't sound clean...

Just to make it generic so that IOMMUFD can also use it. IOMMUFD will
use it to store the per-{device, pasid} object id (and possibly other
data) so that it can be quickly retrieved in the critical fault
delivering patch.

Best regards,
baolu
Baolu Lu July 12, 2023, 3:13 a.m. UTC | #4 
On 2023/7/12 6:02, Jacob Pan wrote:
> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> wrote:
> 
>> Remove the static iopf_param pointer from struct iommu_fault_param to
>> save memory.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   include/linux/iommu.h      |  2 --
>>   drivers/iommu/io-pgfault.c | 47 +++++++++++++++++++++++---------------
>>   2 files changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index ffd6fe1317f4..5fe37a7c5a55 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -551,7 +551,6 @@ struct iommu_fault_param {
>>    * struct dev_iommu - Collection of per-device IOMMU data
>>    *
>>    * @fault_param: IOMMU detected device fault reporting data
>> - * @iopf_param:	 I/O Page Fault queue and data
>>    * @fwspec:	 IOMMU fwspec data
>>    * @iommu_dev:	 IOMMU device this device is linked to
>>    * @priv:	 IOMMU Driver private data
>> @@ -564,7 +563,6 @@ struct iommu_fault_param {
>>   struct dev_iommu {
>>   	struct mutex lock;
>>   	struct iommu_fault_param	*fault_param;
>> -	struct iopf_device_param	*iopf_param;
>>   	struct iommu_fwspec		*fwspec;
>>   	struct iommu_device		*iommu_dev;
>>   	void				*priv;
>> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
>> index 1749e0869f2e..6a3a4e08e67e 100644
>> --- a/drivers/iommu/io-pgfault.c
>> +++ b/drivers/iommu/io-pgfault.c
>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
>> struct device *dev)
>>   	 * As long as we're holding param->lock, the queue can't be
>> unlinked
>>   	 * from the device and therefore cannot disappear.
>>   	 */
>> -	iopf_param = param->iopf_param;
>> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
> I am not sure I understand how does it know the cookie type is iopf_param
> for PASID 0?
> 
> Between IOPF and IOMMUFD use of the cookie, cookie types are different,
> right?
> 

The fault cookie is managed by the code that delivers or handles the
faults. The sva and IOMMUFD paths are exclusive.

Best regards,
baolu
Tian, Kevin July 13, 2023, 3:24 a.m. UTC | #5 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, July 12, 2023 11:13 AM
> 
> On 2023/7/12 6:02, Jacob Pan wrote:
> > On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> > wrote:
> >
> >> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
> >> struct device *dev)
> >>   	 * As long as we're holding param->lock, the queue can't be
> >> unlinked
> >>   	 * from the device and therefore cannot disappear.
> >>   	 */
> >> -	iopf_param = param->iopf_param;
> >> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
> > I am not sure I understand how does it know the cookie type is iopf_param
> > for PASID 0?
> >
> > Between IOPF and IOMMUFD use of the cookie, cookie types are different,
> > right?
> >
> 
> The fault cookie is managed by the code that delivers or handles the
> faults. The sva and IOMMUFD paths are exclusive.
> 

what about siov? A siov-capable device can support sva and iommufd
simultaneously.
Baolu Lu July 13, 2023, 3:43 a.m. UTC | #6 
On 2023/7/13 11:24, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Wednesday, July 12, 2023 11:13 AM
>>
>> On 2023/7/12 6:02, Jacob Pan wrote:
>>> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<baolu.lu@linux.intel.com>
>>> wrote:
>>>
>>>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
>>>> struct device *dev)
>>>>    	 * As long as we're holding param->lock, the queue can't be
>>>> unlinked
>>>>    	 * from the device and therefore cannot disappear.
>>>>    	 */
>>>> -	iopf_param = param->iopf_param;
>>>> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
>>> I am not sure I understand how does it know the cookie type is iopf_param
>>> for PASID 0?
>>>
>>> Between IOPF and IOMMUFD use of the cookie, cookie types are different,
>>> right?
>>>
>>
>> The fault cookie is managed by the code that delivers or handles the
>> faults. The sva and IOMMUFD paths are exclusive.
>>
> 
> what about siov? A siov-capable device can support sva and iommufd
> simultaneously.

For siov case, the pasid should be global. RID and each pasid are still
exclusive, so I don't see any problem. Did I overlook anything?

Best regards,
baolu
Tian, Kevin July 13, 2023, 8:01 a.m. UTC | #7 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, July 13, 2023 11:44 AM
> 
> On 2023/7/13 11:24, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@linux.intel.com>
> >> Sent: Wednesday, July 12, 2023 11:13 AM
> >>
> >> On 2023/7/12 6:02, Jacob Pan wrote:
> >>> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> >>> wrote:
> >>>
> >>>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault
> *fault,
> >>>> struct device *dev)
> >>>>    	 * As long as we're holding param->lock, the queue can't be
> >>>> unlinked
> >>>>    	 * from the device and therefore cannot disappear.
> >>>>    	 */
> >>>> -	iopf_param = param->iopf_param;
> >>>> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
> >>> I am not sure I understand how does it know the cookie type is
> iopf_param
> >>> for PASID 0?
> >>>
> >>> Between IOPF and IOMMUFD use of the cookie, cookie types are
> different,
> >>> right?
> >>>
> >>
> >> The fault cookie is managed by the code that delivers or handles the
> >> faults. The sva and IOMMUFD paths are exclusive.
> >>
> >
> > what about siov? A siov-capable device can support sva and iommufd
> > simultaneously.
> 
> For siov case, the pasid should be global. RID and each pasid are still
> exclusive, so I don't see any problem. Did I overlook anything?
> 

they are exclusive but it's weird to see some pasids (for sva) on this
device are tracked by slot#0 while other pasids (for iommufd) occupies
per-pasid slot.

why not generalizing them given you name it as "per-pasid fault cookie"?
Baolu Lu July 14, 2023, 2:49 a.m. UTC | #8 
On 2023/7/13 16:01, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Thursday, July 13, 2023 11:44 AM
>>
>> On 2023/7/13 11:24, Tian, Kevin wrote:
>>>> From: Baolu Lu <baolu.lu@linux.intel.com>
>>>> Sent: Wednesday, July 12, 2023 11:13 AM
>>>>
>>>> On 2023/7/12 6:02, Jacob Pan wrote:
>>>>> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<baolu.lu@linux.intel.com>
>>>>> wrote:
>>>>>
>>>>>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault
>> *fault,
>>>>>> struct device *dev)
>>>>>>     	 * As long as we're holding param->lock, the queue can't be
>>>>>> unlinked
>>>>>>     	 * from the device and therefore cannot disappear.
>>>>>>     	 */
>>>>>> -	iopf_param = param->iopf_param;
>>>>>> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
>>>>> I am not sure I understand how does it know the cookie type is
>> iopf_param
>>>>> for PASID 0?
>>>>>
>>>>> Between IOPF and IOMMUFD use of the cookie, cookie types are
>> different,
>>>>> right?
>>>>>
>>>>
>>>> The fault cookie is managed by the code that delivers or handles the
>>>> faults. The sva and IOMMUFD paths are exclusive.
>>>>
>>>
>>> what about siov? A siov-capable device can support sva and iommufd
>>> simultaneously.
>>
>> For siov case, the pasid should be global. RID and each pasid are still
>> exclusive, so I don't see any problem. Did I overlook anything?
>>
> 
> they are exclusive but it's weird to see some pasids (for sva) on this
> device are tracked by slot#0 while other pasids (for iommufd) occupies
> per-pasid slot.
> 
> why not generalizing them given you name it as "per-pasid fault cookie"?

Yeah! Get your point now. At least the partial list should be per-pasid.

Let me invest more time on this.

Best regards,
baolu
diff mbox series

Patch

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ffd6fe1317f4..5fe37a7c5a55 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -551,7 +551,6 @@  struct iommu_fault_param {
  * struct dev_iommu - Collection of per-device IOMMU data
  *
  * @fault_param: IOMMU detected device fault reporting data
- * @iopf_param:	 I/O Page Fault queue and data
  * @fwspec:	 IOMMU fwspec data
  * @iommu_dev:	 IOMMU device this device is linked to
  * @priv:	 IOMMU Driver private data
@@ -564,7 +563,6 @@  struct iommu_fault_param {
 struct dev_iommu {
 	struct mutex lock;
 	struct iommu_fault_param	*fault_param;
-	struct iopf_device_param	*iopf_param;
 	struct iommu_fwspec		*fwspec;
 	struct iommu_device		*iommu_dev;
 	void				*priv;
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 1749e0869f2e..6a3a4e08e67e 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -158,7 +158,7 @@  int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
 	 * As long as we're holding param->lock, the queue can't be unlinked
 	 * from the device and therefore cannot disappear.
 	 */
-	iopf_param = param->iopf_param;
+	iopf_param = iommu_get_device_fault_cookie(dev, 0);
 	if (!iopf_param)
 		return -ENODEV;
 
@@ -235,7 +235,7 @@  int iopf_queue_flush_dev(struct device *dev)
 		return -ENODEV;
 
 	mutex_lock(&param->lock);
-	iopf_param = param->iopf_param;
+	iopf_param = iommu_get_device_fault_cookie(dev, 0);
 	if (iopf_param)
 		flush_workqueue(iopf_param->queue->wq);
 	else
@@ -286,9 +286,9 @@  EXPORT_SYMBOL_GPL(iopf_queue_discard_partial);
  */
 int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
 {
-	int ret = -EBUSY;
-	struct iopf_device_param *iopf_param;
+	struct iopf_device_param *iopf_param, *curr;
 	struct dev_iommu *param = dev->iommu;
+	int ret;
 
 	if (!param)
 		return -ENODEV;
@@ -303,16 +303,27 @@  int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
 
 	mutex_lock(&queue->lock);
 	mutex_lock(&param->lock);
-	if (!param->iopf_param) {
-		list_add(&iopf_param->queue_list, &queue->devices);
-		param->iopf_param = iopf_param;
-		ret = 0;
+	curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
+	if (IS_ERR(curr)) {
+		ret = PTR_ERR(curr);
+		goto err_free;
 	}
+
+	if (curr) {
+		ret = -EBUSY;
+		goto err_restore;
+	}
+	list_add(&iopf_param->queue_list, &queue->devices);
 	mutex_unlock(&param->lock);
 	mutex_unlock(&queue->lock);
 
-	if (ret)
-		kfree(iopf_param);
+	return 0;
+err_restore:
+	iommu_set_device_fault_cookie(dev, 0, curr);
+err_free:
+	mutex_unlock(&param->lock);
+	mutex_unlock(&queue->lock);
+	kfree(iopf_param);
 
 	return ret;
 }
@@ -329,7 +340,6 @@  EXPORT_SYMBOL_GPL(iopf_queue_add_device);
  */
 int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
 {
-	int ret = -EINVAL;
 	struct iopf_fault *iopf, *next;
 	struct iopf_device_param *iopf_param;
 	struct dev_iommu *param = dev->iommu;
@@ -339,16 +349,17 @@  int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
 
 	mutex_lock(&queue->lock);
 	mutex_lock(&param->lock);
-	iopf_param = param->iopf_param;
-	if (iopf_param && iopf_param->queue == queue) {
-		list_del(&iopf_param->queue_list);
-		param->iopf_param = NULL;
-		ret = 0;
+	iopf_param = iommu_get_device_fault_cookie(dev, 0);
+	if (!iopf_param || iopf_param->queue != queue) {
+		mutex_unlock(&param->lock);
+		mutex_unlock(&queue->lock);
+		return -EINVAL;
 	}
+
+	list_del(&iopf_param->queue_list);
+	iommu_set_device_fault_cookie(dev, 0, NULL);
 	mutex_unlock(&param->lock);
 	mutex_unlock(&queue->lock);
-	if (ret)
-		return ret;
 
 	/* Just in case some faults are still stuck */
 	list_for_each_entry_safe(iopf, next, &iopf_param->partial, list)