Hi BaoLu,
On Thu, 9 Mar 2023 10:56:39 +0800, Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
> 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
