diff mbox series

[5/9] iommu: Make fault_param generic

Message ID 20230711010642.19707-6-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 
  The iommu faults, including recoverable faults (IO page faults) and
unrecoverable faults (DMA faults), are generic to all devices. The
iommu faults could possibly be triggered for every device.

The fault_param pointer under struct dev_iommu is the per-device fault
data. Therefore, the fault_param pointer should be allocated during
iommu device probe and freed when the device is released.

With this done, the individual iommu drivers that support iopf have no
need to call iommu_[un]register_device_fault_handler() any more.
This will make it easier for the iommu drivers to support iopf, and it
will also make the fault_param allocation and free simpler.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c    | 13 +------------
 drivers/iommu/intel/iommu.c                    | 18 ++++--------------
 drivers/iommu/iommu.c                          | 14 ++++++++++++++
 3 files changed, 19 insertions(+), 26 deletions(-)

Comments

Tian, Kevin July 11, 2023, 6:14 a.m. UTC | #1 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, July 11, 2023 9:07 AM
> 
> @@ -299,7 +299,15 @@ static int dev_iommu_get(struct device *dev)
>  		return -ENOMEM;
> 
>  	mutex_init(&param->lock);
> +	param->fault_param = kzalloc(sizeof(*param->fault_param),
> GFP_KERNEL);
> +	if (!param->fault_param) {
> +		kfree(param);
> +		return -ENOMEM;
> +	}
> +	mutex_init(&param->fault_param->lock);
> +	INIT_LIST_HEAD(&param->fault_param->faults);
>  	dev->iommu = param;
> +
>  	return 0;
>  }

Upon above changes is it slightly cleaner to call it dev_iommu_init()
to better pair with dev_iommu_free()?
Jacob Pan July 11, 2023, 9:31 p.m. UTC | #2 
Hi BaoLu,

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

> The iommu faults, including recoverable faults (IO page faults) and
> unrecoverable faults (DMA faults), are generic to all devices. The
> iommu faults could possibly be triggered for every device.
> 
> The fault_param pointer under struct dev_iommu is the per-device fault
> data. Therefore, the fault_param pointer should be allocated during
> iommu device probe and freed when the device is released.
> 
> With this done, the individual iommu drivers that support iopf have no
> need to call iommu_[un]register_device_fault_handler() any more.
> This will make it easier for the iommu drivers to support iopf, and it
> will also make the fault_param allocation and free simpler.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c    | 13 +------------
>  drivers/iommu/intel/iommu.c                    | 18 ++++--------------
>  drivers/iommu/iommu.c                          | 14 ++++++++++++++
>  3 files changed, 19 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index
> a5a63b1c947e..fa8ab9d413f8 100644 ---
> a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -437,7 +437,6 @@
> bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master) 
>  static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master
> *master) {
> -	int ret;
>  	struct device *dev = master->dev;
>  
>  	/*
> @@ -450,16 +449,7 @@ static int arm_smmu_master_sva_enable_iopf(struct
> arm_smmu_master *master) if (!master->iopf_enabled)
>  		return -EINVAL;
>  
> -	ret = iopf_queue_add_device(master->smmu->evtq.iopf, dev);
> -	if (ret)
> -		return ret;
> -
> -	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,
> dev);
> -	if (ret) {
> -		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> -		return ret;
> -	}
> -	return 0;
> +	return iopf_queue_add_device(master->smmu->evtq.iopf, dev);
>  }
>  
>  static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master
> *master) @@ -469,7 +459,6 @@ static void
> arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master) if
> (!master->iopf_enabled) return;
>  
> -	iommu_unregister_device_fault_handler(dev);
>  	iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
>  }
>  
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 5c8c5cdc36cf..22e43db20252 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4594,23 +4594,14 @@ static int intel_iommu_enable_iopf(struct device
> *dev) if (ret)
>  		return ret;
>  
> -	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,
> dev);
> -	if (ret)
> -		goto iopf_remove_device;
> -
>  	ret = pci_enable_pri(pdev, PRQ_DEPTH);
> -	if (ret)
> -		goto iopf_unregister_handler;
> +	if (ret) {
> +		iopf_queue_remove_device(iommu->iopf_queue, dev);
> +		return ret;
> +	}
>  	info->pri_enabled = 1;
>  
>  	return 0;
> -
> -iopf_unregister_handler:
> -	iommu_unregister_device_fault_handler(dev);
> -iopf_remove_device:
> -	iopf_queue_remove_device(iommu->iopf_queue, dev);
> -
> -	return ret;
>  }
>  
>  static int intel_iommu_disable_iopf(struct device *dev)
> @@ -4637,7 +4628,6 @@ static int intel_iommu_disable_iopf(struct device
> *dev)
>  	 * fault handler and removing device from iopf queue should never
>  	 * fail.
>  	 */
> -	WARN_ON(iommu_unregister_device_fault_handler(dev));
>  	WARN_ON(iopf_queue_remove_device(iommu->iopf_queue, dev));
>  
>  	return 0;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 65895b987e22..8d1f0935ea71 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -299,7 +299,15 @@ static int dev_iommu_get(struct device *dev)
>  		return -ENOMEM;
>  
>  	mutex_init(&param->lock);
> +	param->fault_param = kzalloc(sizeof(*param->fault_param),
> GFP_KERNEL);
since fault_param is _always_ allocated/freed along with param, can we merge
into one allocation? i.e.
 struct dev_iommu {
        struct mutex lock;
-       struct iommu_fault_param        *fault_param;
+       struct iommu_fault_param        fault_param;


> +	if (!param->fault_param) {
> +		kfree(param);
> +		return -ENOMEM;
> +	}
> +	mutex_init(&param->fault_param->lock);
> +	INIT_LIST_HEAD(&param->fault_param->faults);
>  	dev->iommu = param;
> +
>  	return 0;
>  }
>  
> @@ -312,6 +320,12 @@ static void dev_iommu_free(struct device *dev)
>  		fwnode_handle_put(param->fwspec->iommu_fwnode);
>  		kfree(param->fwspec);
>  	}
> +	/*
> +	 * All pending faults should have been drained before
> +	 * device release.
> +	 */
> +	WARN_ON_ONCE(!list_empty(&param->fault_param->faults));
> +	kfree(param->fault_param);
>  	kfree(param);
>  }
>  


Thanks,

Jacob
Baolu Lu July 12, 2023, 2:43 a.m. UTC | #3 
On 2023/7/11 14:14, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, July 11, 2023 9:07 AM
>>
>> @@ -299,7 +299,15 @@ static int dev_iommu_get(struct device *dev)
>>   		return -ENOMEM;
>>
>>   	mutex_init(&param->lock);
>> +	param->fault_param = kzalloc(sizeof(*param->fault_param),
>> GFP_KERNEL);
>> +	if (!param->fault_param) {
>> +		kfree(param);
>> +		return -ENOMEM;
>> +	}
>> +	mutex_init(&param->fault_param->lock);
>> +	INIT_LIST_HEAD(&param->fault_param->faults);
>>   	dev->iommu = param;
>> +
>>   	return 0;
>>   }
> 
> Upon above changes is it slightly cleaner to call it dev_iommu_init()
> to better pair with dev_iommu_free()?

dev_iommu_init() was introduced in Jason's series. It's not landed yet.

https://lore.kernel.org/linux-iommu/0-v3-328044aa278c+45e49-iommu_probe_jgg@nvidia.com/

Sure. Should refine this code after above patches are landed.

Best regards,
baolu
Baolu Lu July 12, 2023, 3:02 a.m. UTC | #4 
On 2023/7/12 5:31, Jacob Pan wrote:
> On Tue, 11 Jul 2023 09:06:38 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> wrote:
> 
>> The iommu faults, including recoverable faults (IO page faults) and
>> unrecoverable faults (DMA faults), are generic to all devices. The
>> iommu faults could possibly be triggered for every device.
>>
>> The fault_param pointer under struct dev_iommu is the per-device fault
>> data. Therefore, the fault_param pointer should be allocated during
>> iommu device probe and freed when the device is released.
>>
>> With this done, the individual iommu drivers that support iopf have no
>> need to call iommu_[un]register_device_fault_handler() any more.
>> This will make it easier for the iommu drivers to support iopf, and it
>> will also make the fault_param allocation and free simpler.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c    | 13 +------------
>>   drivers/iommu/intel/iommu.c                    | 18 ++++--------------
>>   drivers/iommu/iommu.c                          | 14 ++++++++++++++
>>   3 files changed, 19 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index
>> a5a63b1c947e..fa8ab9d413f8 100644 ---
>> a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++
>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -437,7 +437,6 @@
>> bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master)
>>   static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master
>> *master) {
>> -	int ret;
>>   	struct device *dev = master->dev;
>>   
>>   	/*
>> @@ -450,16 +449,7 @@ static int arm_smmu_master_sva_enable_iopf(struct
>> arm_smmu_master *master) if (!master->iopf_enabled)
>>   		return -EINVAL;
>>   
>> -	ret = iopf_queue_add_device(master->smmu->evtq.iopf, dev);
>> -	if (ret)
>> -		return ret;
>> -
>> -	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,
>> dev);
>> -	if (ret) {
>> -		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
>> -		return ret;
>> -	}
>> -	return 0;
>> +	return iopf_queue_add_device(master->smmu->evtq.iopf, dev);
>>   }
>>   
>>   static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master
>> *master) @@ -469,7 +459,6 @@ static void
>> arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master) if
>> (!master->iopf_enabled) return;
>>   
>> -	iommu_unregister_device_fault_handler(dev);
>>   	iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
>>   }
>>   
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 5c8c5cdc36cf..22e43db20252 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -4594,23 +4594,14 @@ static int intel_iommu_enable_iopf(struct device
>> *dev) if (ret)
>>   		return ret;
>>   
>> -	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,
>> dev);
>> -	if (ret)
>> -		goto iopf_remove_device;
>> -
>>   	ret = pci_enable_pri(pdev, PRQ_DEPTH);
>> -	if (ret)
>> -		goto iopf_unregister_handler;
>> +	if (ret) {
>> +		iopf_queue_remove_device(iommu->iopf_queue, dev);
>> +		return ret;
>> +	}
>>   	info->pri_enabled = 1;
>>   
>>   	return 0;
>> -
>> -iopf_unregister_handler:
>> -	iommu_unregister_device_fault_handler(dev);
>> -iopf_remove_device:
>> -	iopf_queue_remove_device(iommu->iopf_queue, dev);
>> -
>> -	return ret;
>>   }
>>   
>>   static int intel_iommu_disable_iopf(struct device *dev)
>> @@ -4637,7 +4628,6 @@ static int intel_iommu_disable_iopf(struct device
>> *dev)
>>   	 * fault handler and removing device from iopf queue should never
>>   	 * fail.
>>   	 */
>> -	WARN_ON(iommu_unregister_device_fault_handler(dev));
>>   	WARN_ON(iopf_queue_remove_device(iommu->iopf_queue, dev));
>>   
>>   	return 0;
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 65895b987e22..8d1f0935ea71 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -299,7 +299,15 @@ static int dev_iommu_get(struct device *dev)
>>   		return -ENOMEM;
>>   
>>   	mutex_init(&param->lock);
>> +	param->fault_param = kzalloc(sizeof(*param->fault_param),
>> GFP_KERNEL);
> since fault_param is_always_  allocated/freed along with param, can we merge
> into one allocation? i.e.
>   struct dev_iommu {
>          struct mutex lock;
> -       struct iommu_fault_param        *fault_param;
> +       struct iommu_fault_param        fault_param;

I am not pretty sure about the change in this patch. It's a simple-and-
stupid way. But it also wastes memory for devices that have not pri-
capable domain attached.

So probably it's better to allocate fault_param at the place where a
real pri-capable domain is attached to the device?

Best regards,
baolu
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index a5a63b1c947e..fa8ab9d413f8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -437,7 +437,6 @@  bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master)
 
 static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master)
 {
-	int ret;
 	struct device *dev = master->dev;
 
 	/*
@@ -450,16 +449,7 @@  static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master)
 	if (!master->iopf_enabled)
 		return -EINVAL;
 
-	ret = iopf_queue_add_device(master->smmu->evtq.iopf, dev);
-	if (ret)
-		return ret;
-
-	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
-	if (ret) {
-		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
-		return ret;
-	}
-	return 0;
+	return iopf_queue_add_device(master->smmu->evtq.iopf, dev);
 }
 
 static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master)
@@ -469,7 +459,6 @@  static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master)
 	if (!master->iopf_enabled)
 		return;
 
-	iommu_unregister_device_fault_handler(dev);
 	iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
 }
 
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 5c8c5cdc36cf..22e43db20252 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4594,23 +4594,14 @@  static int intel_iommu_enable_iopf(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
-	if (ret)
-		goto iopf_remove_device;
-
 	ret = pci_enable_pri(pdev, PRQ_DEPTH);
-	if (ret)
-		goto iopf_unregister_handler;
+	if (ret) {
+		iopf_queue_remove_device(iommu->iopf_queue, dev);
+		return ret;
+	}
 	info->pri_enabled = 1;
 
 	return 0;
-
-iopf_unregister_handler:
-	iommu_unregister_device_fault_handler(dev);
-iopf_remove_device:
-	iopf_queue_remove_device(iommu->iopf_queue, dev);
-
-	return ret;
 }
 
 static int intel_iommu_disable_iopf(struct device *dev)
@@ -4637,7 +4628,6 @@  static int intel_iommu_disable_iopf(struct device *dev)
 	 * fault handler and removing device from iopf queue should never
 	 * fail.
 	 */
-	WARN_ON(iommu_unregister_device_fault_handler(dev));
 	WARN_ON(iopf_queue_remove_device(iommu->iopf_queue, dev));
 
 	return 0;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 65895b987e22..8d1f0935ea71 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -299,7 +299,15 @@  static int dev_iommu_get(struct device *dev)
 		return -ENOMEM;
 
 	mutex_init(&param->lock);
+	param->fault_param = kzalloc(sizeof(*param->fault_param), GFP_KERNEL);
+	if (!param->fault_param) {
+		kfree(param);
+		return -ENOMEM;
+	}
+	mutex_init(&param->fault_param->lock);
+	INIT_LIST_HEAD(&param->fault_param->faults);
 	dev->iommu = param;
+
 	return 0;
 }
 
@@ -312,6 +320,12 @@  static void dev_iommu_free(struct device *dev)
 		fwnode_handle_put(param->fwspec->iommu_fwnode);
 		kfree(param->fwspec);
 	}
+	/*
+	 * All pending faults should have been drained before
+	 * device release.
+	 */
+	WARN_ON_ONCE(!list_empty(&param->fault_param->faults));
+	kfree(param->fault_param);
 	kfree(param);
 }