diff mbox series

[v2,06/12] iommu: Make dev->fault_param static

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

Commit Message

Baolu Lu July 27, 2023, 5:48 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 could be static and 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 Aug. 3, 2023, 8:08 a.m. UTC | #1 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, July 27, 2023 1:49 PM
>
> @@ -4630,7 +4621,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));

the comment should be updated too.

> 
>  	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);

let's also move 'partial' from struct iopf_device_param into struct
iommu_fault_param. That logic is not specific to sva.

meanwhile probably iopf_device_param can be renamed to
iopf_sva_param since all the remaining fields are only used by
the sva handler.

current naming (iommu_fault_param vs. iopf_device_param) is a
bit confusing when reading related code.
Baolu Lu Aug. 4, 2023, 3:16 a.m. UTC | #2 
On 2023/8/3 16:08, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, July 27, 2023 1:49 PM
>>
>> @@ -4630,7 +4621,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));
> 
> the comment should be updated too.

Ack.

> 
>>
>>   	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);
> 
> let's also move 'partial' from struct iopf_device_param into struct
> iommu_fault_param. That logic is not specific to sva.
> 
> meanwhile probably iopf_device_param can be renamed to
> iopf_sva_param since all the remaining fields are only used by
> the sva handler.
> 
> current naming (iommu_fault_param vs. iopf_device_param) is a
> bit confusing when reading related code.

My understanding is that iommu_fault_param is for all kinds of iommu
faults. Currently they probably include recoverable IO page faults or
unrecoverable DMA faults.

While, iopf_device_param is for the recoverable IO page faults. I agree
that this naming is not specific and even confusing. Perhaps renaming it
to something like iommu_iopf_param?

Best regards,
baolu
Tian, Kevin Aug. 4, 2023, 3:56 a.m. UTC | #3 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Friday, August 4, 2023 11:17 AM
> 
> On 2023/8/3 16:08, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Thursday, July 27, 2023 1:49 PM
> >>
> >>
> >>   	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);
> >
> > let's also move 'partial' from struct iopf_device_param into struct
> > iommu_fault_param. That logic is not specific to sva.
> >
> > meanwhile probably iopf_device_param can be renamed to
> > iopf_sva_param since all the remaining fields are only used by
> > the sva handler.
> >
> > current naming (iommu_fault_param vs. iopf_device_param) is a
> > bit confusing when reading related code.
> 
> My understanding is that iommu_fault_param is for all kinds of iommu
> faults. Currently they probably include recoverable IO page faults or
> unrecoverable DMA faults.
> 
> While, iopf_device_param is for the recoverable IO page faults. I agree
> that this naming is not specific and even confusing. Perhaps renaming it
> to something like iommu_iopf_param?
> 

or just iopf_param...
Baolu Lu Aug. 4, 2023, 5:34 a.m. UTC | #4 
On 8/4/23 11:56 AM, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Friday, August 4, 2023 11:17 AM
>>
>> On 2023/8/3 16:08, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Sent: Thursday, July 27, 2023 1:49 PM
>>>>
>>>>
>>>>    	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);
>>>
>>> let's also move 'partial' from struct iopf_device_param into struct
>>> iommu_fault_param. That logic is not specific to sva.
>>>
>>> meanwhile probably iopf_device_param can be renamed to
>>> iopf_sva_param since all the remaining fields are only used by
>>> the sva handler.
>>>
>>> current naming (iommu_fault_param vs. iopf_device_param) is a
>>> bit confusing when reading related code.
>>
>> My understanding is that iommu_fault_param is for all kinds of iommu
>> faults. Currently they probably include recoverable IO page faults or
>> unrecoverable DMA faults.
>>
>> While, iopf_device_param is for the recoverable IO page faults. I agree
>> that this naming is not specific and even confusing. Perhaps renaming it
>> to something like iommu_iopf_param?
>>
> 
> or just iopf_param.

Okay.

Best regards,
baolu
Jason Gunthorpe Aug. 10, 2023, 6:20 p.m. UTC | #5 
On Thu, Jul 27, 2023 at 01:48:31PM +0800, Lu Baolu wrote:
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4ba3bb692993..3e4ff984aa85 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -302,7 +302,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;

This allocation seems pointless?

If we always allocate the fault param then just don't make it a
pointer in the first place.

The appeal of allocation would be to save a few bytes in the common
case that the driver doesn't support faulting.

Which means the driver needs to make some call to enable faulting for
a device. In this case I'd continue to lazy free on release like this
patch does.

Jason
Jason Gunthorpe Aug. 10, 2023, 6:46 p.m. UTC | #6 
On Thu, Aug 10, 2023 at 03:20:07PM -0300, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:31PM +0800, Lu Baolu wrote:
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 4ba3bb692993..3e4ff984aa85 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -302,7 +302,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;
> 
> This allocation seems pointless?
> 
> If we always allocate the fault param then just don't make it a
> pointer in the first place.
> 
> The appeal of allocation would be to save a few bytes in the common
> case that the driver doesn't support faulting.
> 
> Which means the driver needs to make some call to enable faulting for
> a device. In this case I'd continue to lazy free on release like this
> patch does.

For instance allocate the fault_param in iopf_queue_add_device() which
is the only thing that needs it.

Actually probably just merge struct iopf_device_param and
iommu_fault_param ?

When you call iopf_queue_add_device() it enables page faulting mode,
does 1 additional allocation for all additional required per-device
memory and thats it.

Jason
Baolu Lu Aug. 11, 2023, 1:43 a.m. UTC | #7 
On 2023/8/11 2:46, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2023 at 03:20:07PM -0300, Jason Gunthorpe wrote:
>> On Thu, Jul 27, 2023 at 01:48:31PM +0800, Lu Baolu wrote:
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 4ba3bb692993..3e4ff984aa85 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -302,7 +302,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;
>> This allocation seems pointless?
>>
>> If we always allocate the fault param then just don't make it a
>> pointer in the first place.
>>
>> The appeal of allocation would be to save a few bytes in the common
>> case that the driver doesn't support faulting.
>>
>> Which means the driver needs to make some call to enable faulting for
>> a device. In this case I'd continue to lazy free on release like this
>> patch does.
> For instance allocate the fault_param in iopf_queue_add_device() which
> is the only thing that needs it.
> 
> Actually probably just merge struct iopf_device_param and
> iommu_fault_param ?
> 
> When you call iopf_queue_add_device() it enables page faulting mode,
> does 1 additional allocation for all additional required per-device
> memory and thats it.

Agreed.

I originally kept the iommu_fault_param structure because I thought it
could also be used to store temporary data for unrecoverable faults,
just like the iopf_device_param structure is used for iopf. However, I
am not sure whether we actually need any temporary data for
unrecoverable fault forwarding, which doesn't require any response.

So, it's better to do like you suggested. It's cleaner and simpler.

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 d5ca2387e65c..e94e833638a7 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4587,23 +4587,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)
@@ -4630,7 +4621,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 4ba3bb692993..3e4ff984aa85 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -302,7 +302,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;
 }
 
@@ -315,6 +323,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);
 }