diff mbox series

[v2,5/5] iommu/vt-d: Move PRI handling to IOPF feature path

Message ID 20230309025639.26109-6-baolu.lu@linux.intel.com
State New
Headers
Series Refactor code for non-PRI IOPF |

Commit Message

Baolu Lu March 9, 2023, 2:56 a.m. UTC 
  PRI is only used for IOPF. With this move, the PCI/PRI feature could be
controlled by the device driver through iommu_dev_enable/disable_feature()
interfaces.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 59 ++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 20 deletions(-)

Comments

Tian, Kevin March 16, 2023, 7:17 a.m. UTC | #1 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, March 9, 2023 10:57 AM
> 
> @@ -4689,17 +4704,21 @@ static int intel_iommu_disable_iopf(struct device
> *dev)
>  {
>  	struct device_domain_info *info = dev_iommu_priv_get(dev);
>  	struct intel_iommu *iommu = info->iommu;
> -	int ret;
> 
> -	ret = iommu_unregister_device_fault_handler(dev);
> -	if (ret)
> -		return ret;
> +	if (!info->pri_enabled)
> +		return -EINVAL;
> 
> -	ret = iopf_queue_remove_device(iommu->iopf_queue, dev);
> -	if (ret)
> -		iommu_register_device_fault_handler(dev,
> iommu_queue_iopf, dev);
> +	pci_disable_pri(to_pci_dev(dev));
> +	info->pri_enabled = 0;
> 
> -	return ret;
> +	/*
> +	 * With pri_enabled checked, unregistering fault handler and
> +	 * removing device from iopf queue should never fail.
> +	 */
> +	iommu_unregister_device_fault_handler(dev);
> +	iopf_queue_remove_device(iommu->iopf_queue, dev);
> +
> +	return 0;
>  }

PCIe spec says that clearing the enable bit doesn't mean in-fly
page requests are completed:
--
Enable (E) - This field, when set, indicates that the Page Request
Interface is allowed to make page requests. If this field is Clear,
the Page Request Interface is not allowed to issue page requests.
If both this field and the Stopped field are Clear, then the Page
Request Interface will not issue new page requests, but has
outstanding page requests that have been transmitted or are
queued for transmission
Baolu Lu March 16, 2023, 8:17 a.m. UTC | #2 
On 2023/3/16 15:17, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, March 9, 2023 10:57 AM
>>
>> @@ -4689,17 +4704,21 @@ static int intel_iommu_disable_iopf(struct device
>> *dev)
>>   {
>>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>>   	struct intel_iommu *iommu = info->iommu;
>> -	int ret;
>>
>> -	ret = iommu_unregister_device_fault_handler(dev);
>> -	if (ret)
>> -		return ret;
>> +	if (!info->pri_enabled)
>> +		return -EINVAL;
>>
>> -	ret = iopf_queue_remove_device(iommu->iopf_queue, dev);
>> -	if (ret)
>> -		iommu_register_device_fault_handler(dev,
>> iommu_queue_iopf, dev);
>> +	pci_disable_pri(to_pci_dev(dev));
>> +	info->pri_enabled = 0;
>>
>> -	return ret;
>> +	/*
>> +	 * With pri_enabled checked, unregistering fault handler and
>> +	 * removing device from iopf queue should never fail.
>> +	 */
>> +	iommu_unregister_device_fault_handler(dev);
>> +	iopf_queue_remove_device(iommu->iopf_queue, dev);
>> +
>> +	return 0;
>>   }
> 
> PCIe spec says that clearing the enable bit doesn't mean in-fly
> page requests are completed:
> --
> Enable (E) - This field, when set, indicates that the Page Request
> Interface is allowed to make page requests. If this field is Clear,
> the Page Request Interface is not allowed to issue page requests.
> If both this field and the Stopped field are Clear, then the Page
> Request Interface will not issue new page requests, but has
> outstanding page requests that have been transmitted or are
> queued for transmission

Yes. So the iommu driver should drain the in-fly PRQs.

The Intel VT-d implementation drains the PRQs when any PASID is unbound
from the iommu domain (see intel_svm_drain_prq()) before reuse. Before
disabling iopf, the device driver should unbind pasid and disable sva,
so when it comes here, the PRQ should have been drained.

Perhaps I can add below comments to make this clear:

         /*
          * PCIe spec states that by clearing PRI enable bit, the Page
          * Request Interface will not issue new page requests, but has
          * outstanding page requests that have been transmitted or are
          * queued for transmission. This is supposed to be called after
          * the device driver has stopped DMA, all PASIDs have been
          * unbound and the outstanding PRQs have been drained.
          */

Best regards,
baolu
Tian, Kevin March 17, 2023, 12:06 a.m. UTC | #3 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, March 16, 2023 4:17 PM
> 
> On 2023/3/16 15:17, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Thursday, March 9, 2023 10:57 AM
> >>
> >> @@ -4689,17 +4704,21 @@ static int intel_iommu_disable_iopf(struct
> device
> >> *dev)
> >>   {
> >>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
> >>   	struct intel_iommu *iommu = info->iommu;
> >> -	int ret;
> >>
> >> -	ret = iommu_unregister_device_fault_handler(dev);
> >> -	if (ret)
> >> -		return ret;
> >> +	if (!info->pri_enabled)
> >> +		return -EINVAL;
> >>
> >> -	ret = iopf_queue_remove_device(iommu->iopf_queue, dev);
> >> -	if (ret)
> >> -		iommu_register_device_fault_handler(dev,
> >> iommu_queue_iopf, dev);
> >> +	pci_disable_pri(to_pci_dev(dev));
> >> +	info->pri_enabled = 0;
> >>
> >> -	return ret;
> >> +	/*
> >> +	 * With pri_enabled checked, unregistering fault handler and
> >> +	 * removing device from iopf queue should never fail.
> >> +	 */
> >> +	iommu_unregister_device_fault_handler(dev);
> >> +	iopf_queue_remove_device(iommu->iopf_queue, dev);
> >> +
> >> +	return 0;
> >>   }
> >
> > PCIe spec says that clearing the enable bit doesn't mean in-fly
> > page requests are completed:
> > --
> > Enable (E) - This field, when set, indicates that the Page Request
> > Interface is allowed to make page requests. If this field is Clear,
> > the Page Request Interface is not allowed to issue page requests.
> > If both this field and the Stopped field are Clear, then the Page
> > Request Interface will not issue new page requests, but has
> > outstanding page requests that have been transmitted or are
> > queued for transmission
> 
> Yes. So the iommu driver should drain the in-fly PRQs.
> 
> The Intel VT-d implementation drains the PRQs when any PASID is unbound
> from the iommu domain (see intel_svm_drain_prq()) before reuse. Before
> disabling iopf, the device driver should unbind pasid and disable sva,
> so when it comes here, the PRQ should have been drained.
> 
> Perhaps I can add below comments to make this clear:
> 
>          /*
>           * PCIe spec states that by clearing PRI enable bit, the Page
>           * Request Interface will not issue new page requests, but has
>           * outstanding page requests that have been transmitted or are
>           * queued for transmission. This is supposed to be called after
>           * the device driver has stopped DMA, all PASIDs have been
>           * unbound and the outstanding PRQs have been drained.
>           */
> 

this is fine. But it should be a separate patch which removes
check of return value. It's not caused by this PRI handling move
patch.
Baolu Lu March 17, 2023, 12:47 a.m. UTC | #4 
On 2023/3/17 8:06, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Thursday, March 16, 2023 4:17 PM
>>
>> On 2023/3/16 15:17, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Sent: Thursday, March 9, 2023 10:57 AM
>>>>
>>>> @@ -4689,17 +4704,21 @@ static int intel_iommu_disable_iopf(struct
>> device
>>>> *dev)
>>>>    {
>>>>    	struct device_domain_info *info = dev_iommu_priv_get(dev);
>>>>    	struct intel_iommu *iommu = info->iommu;
>>>> -	int ret;
>>>>
>>>> -	ret = iommu_unregister_device_fault_handler(dev);
>>>> -	if (ret)
>>>> -		return ret;
>>>> +	if (!info->pri_enabled)
>>>> +		return -EINVAL;
>>>>
>>>> -	ret = iopf_queue_remove_device(iommu->iopf_queue, dev);
>>>> -	if (ret)
>>>> -		iommu_register_device_fault_handler(dev,
>>>> iommu_queue_iopf, dev);
>>>> +	pci_disable_pri(to_pci_dev(dev));
>>>> +	info->pri_enabled = 0;
>>>>
>>>> -	return ret;
>>>> +	/*
>>>> +	 * With pri_enabled checked, unregistering fault handler and
>>>> +	 * removing device from iopf queue should never fail.
>>>> +	 */
>>>> +	iommu_unregister_device_fault_handler(dev);
>>>> +	iopf_queue_remove_device(iommu->iopf_queue, dev);
>>>> +
>>>> +	return 0;
>>>>    }
>>>
>>> PCIe spec says that clearing the enable bit doesn't mean in-fly
>>> page requests are completed:
>>> --
>>> Enable (E) - This field, when set, indicates that the Page Request
>>> Interface is allowed to make page requests. If this field is Clear,
>>> the Page Request Interface is not allowed to issue page requests.
>>> If both this field and the Stopped field are Clear, then the Page
>>> Request Interface will not issue new page requests, but has
>>> outstanding page requests that have been transmitted or are
>>> queued for transmission
>>
>> Yes. So the iommu driver should drain the in-fly PRQs.
>>
>> The Intel VT-d implementation drains the PRQs when any PASID is unbound
>> from the iommu domain (see intel_svm_drain_prq()) before reuse. Before
>> disabling iopf, the device driver should unbind pasid and disable sva,
>> so when it comes here, the PRQ should have been drained.
>>
>> Perhaps I can add below comments to make this clear:
>>
>>           /*
>>            * PCIe spec states that by clearing PRI enable bit, the Page
>>            * Request Interface will not issue new page requests, but has
>>            * outstanding page requests that have been transmitted or are
>>            * queued for transmission. This is supposed to be called after
>>            * the device driver has stopped DMA, all PASIDs have been
>>            * unbound and the outstanding PRQs have been drained.
>>            */
>>
> 
> this is fine. But it should be a separate patch which removes
> check of return value. It's not caused by this PRI handling move
> patch.

Okay, that will be clearer.

Best regards,
baolu
Jacob Pan March 20, 2023, 4:28 p.m. UTC | #5 
Hi BaoLu,

On Thu,  9 Mar 2023 10:56:39 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> PRI is only used for IOPF. With this move, the PCI/PRI feature could be
> controlled by the device driver through iommu_dev_enable/disable_feature()
> interfaces.
This move is good for DMA API PASID as well, it will not turn on PRI when
enabling PASID, ATS cap.

Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com> 

> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 59 ++++++++++++++++++++++++-------------
>  1 file changed, 39 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index fb64ab8358a9..4ed32bde4287 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1415,11 +1415,6 @@ static void iommu_enable_pci_caps(struct
> device_domain_info *info) if (info->pasid_supported &&
> !pci_enable_pasid(pdev, info->pasid_supported & ~1)) info->pasid_enabled
> = 1; 
> -	if (info->pri_supported &&
> -	    (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) :
> 1)  &&
> -	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, PRQ_DEPTH))
> -		info->pri_enabled = 1;
> -
>  	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>  	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>  		info->ats_enabled = 1;
> @@ -1442,11 +1437,6 @@ static void iommu_disable_pci_caps(struct
> device_domain_info *info) domain_update_iotlb(info->domain);
>  	}
>  
> -	if (info->pri_enabled) {
> -		pci_disable_pri(pdev);
> -		info->pri_enabled = 0;
> -	}
> -
>  	if (info->pasid_enabled) {
>  		pci_disable_pasid(pdev);
>  		info->pasid_enabled = 0;
> @@ -4664,23 +4654,48 @@ static int intel_iommu_enable_sva(struct device
> *dev) 
>  static int intel_iommu_enable_iopf(struct device *dev)
>  {
> +	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
>  	struct device_domain_info *info = dev_iommu_priv_get(dev);
>  	struct intel_iommu *iommu;
>  	int ret;
>  
> -	if (!info || !info->ats_enabled || !info->pri_enabled)
> +	if (!pdev || !info || !info->ats_enabled || !info->pri_supported)
>  		return -ENODEV;
> +
> +	if (info->pri_enabled)
> +		return -EBUSY;
> +
>  	iommu = info->iommu;
>  	if (!iommu)
>  		return -EINVAL;
>  
> +	/* PASID is required in PRG Response Message. */
> +	if (info->pasid_enabled && !pci_prg_resp_pasid_required(pdev))
> +		return -EINVAL;
> +
> +	ret = pci_reset_pri(pdev);
> +	if (ret)
> +		return ret;
> +
>  	ret = iopf_queue_add_device(iommu->iopf_queue, dev);
>  	if (ret)
>  		return ret;
>  
>  	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,
> dev); if (ret)
> -		iopf_queue_remove_device(iommu->iopf_queue, dev);
> +		goto iopf_remove_device;
> +
> +	ret = pci_enable_pri(pdev, PRQ_DEPTH);
> +	if (ret)
> +		goto iopf_unregister_handler;
> +	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;
>  }
> @@ -4689,17 +4704,21 @@ static int intel_iommu_disable_iopf(struct device
> *dev) {
>  	struct device_domain_info *info = dev_iommu_priv_get(dev);
>  	struct intel_iommu *iommu = info->iommu;
> -	int ret;
>  
> -	ret = iommu_unregister_device_fault_handler(dev);
> -	if (ret)
> -		return ret;
> +	if (!info->pri_enabled)
> +		return -EINVAL;
>  
> -	ret = iopf_queue_remove_device(iommu->iopf_queue, dev);
> -	if (ret)
> -		iommu_register_device_fault_handler(dev,
> iommu_queue_iopf, dev);
> +	pci_disable_pri(to_pci_dev(dev));
> +	info->pri_enabled = 0;
>  
> -	return ret;
> +	/*
> +	 * With pri_enabled checked, unregistering fault handler and
> +	 * removing device from iopf queue should never fail.
> +	 */
> +	iommu_unregister_device_fault_handler(dev);
> +	iopf_queue_remove_device(iommu->iopf_queue, dev);
> +
> +	return 0;
>  }
>  
>  static int


Thanks,

Jacob
diff mbox series

Patch

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index fb64ab8358a9..4ed32bde4287 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1415,11 +1415,6 @@  static void iommu_enable_pci_caps(struct device_domain_info *info)
 	if (info->pasid_supported && !pci_enable_pasid(pdev, info->pasid_supported & ~1))
 		info->pasid_enabled = 1;
 
-	if (info->pri_supported &&
-	    (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1)  &&
-	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, PRQ_DEPTH))
-		info->pri_enabled = 1;
-
 	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
 	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
 		info->ats_enabled = 1;
@@ -1442,11 +1437,6 @@  static void iommu_disable_pci_caps(struct device_domain_info *info)
 		domain_update_iotlb(info->domain);
 	}
 
-	if (info->pri_enabled) {
-		pci_disable_pri(pdev);
-		info->pri_enabled = 0;
-	}
-
 	if (info->pasid_enabled) {
 		pci_disable_pasid(pdev);
 		info->pasid_enabled = 0;
@@ -4664,23 +4654,48 @@  static int intel_iommu_enable_sva(struct device *dev)
 
 static int intel_iommu_enable_iopf(struct device *dev)
 {
+	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu;
 	int ret;
 
-	if (!info || !info->ats_enabled || !info->pri_enabled)
+	if (!pdev || !info || !info->ats_enabled || !info->pri_supported)
 		return -ENODEV;
+
+	if (info->pri_enabled)
+		return -EBUSY;
+
 	iommu = info->iommu;
 	if (!iommu)
 		return -EINVAL;
 
+	/* PASID is required in PRG Response Message. */
+	if (info->pasid_enabled && !pci_prg_resp_pasid_required(pdev))
+		return -EINVAL;
+
+	ret = pci_reset_pri(pdev);
+	if (ret)
+		return ret;
+
 	ret = iopf_queue_add_device(iommu->iopf_queue, dev);
 	if (ret)
 		return ret;
 
 	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
 	if (ret)
-		iopf_queue_remove_device(iommu->iopf_queue, dev);
+		goto iopf_remove_device;
+
+	ret = pci_enable_pri(pdev, PRQ_DEPTH);
+	if (ret)
+		goto iopf_unregister_handler;
+	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;
 }
@@ -4689,17 +4704,21 @@  static int intel_iommu_disable_iopf(struct device *dev)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
-	int ret;
 
-	ret = iommu_unregister_device_fault_handler(dev);
-	if (ret)
-		return ret;
+	if (!info->pri_enabled)
+		return -EINVAL;
 
-	ret = iopf_queue_remove_device(iommu->iopf_queue, dev);
-	if (ret)
-		iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
+	pci_disable_pri(to_pci_dev(dev));
+	info->pri_enabled = 0;
 
-	return ret;
+	/*
+	 * With pri_enabled checked, unregistering fault handler and
+	 * removing device from iopf queue should never fail.
+	 */
+	iommu_unregister_device_fault_handler(dev);
+	iopf_queue_remove_device(iommu->iopf_queue, dev);
+
+	return 0;
 }
 
 static int