[2/2] iommu/vt-d: Move iopf code from SVA to IOPF enabling path
Commit Message
Generally enabling IOMMU_DEV_FEAT_SVA requires IOMMU_DEV_FEAT_IOPF, but
some devices manage I/O Page Faults themselves instead of relying on the
IOMMU. Move IOPF related code from SVA to IOPF enabling path to make the
driver work for devices that manage IOPF themselves.
For the device drivers that relies on the IOMMU for IOPF through PCI/PRI,
IOMMU_DEV_FEAT_IOPF must be enabled before and disabled after
IOMMU_DEV_FEAT_SVA.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/intel/iommu.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
Comments
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Friday, February 3, 2023 4:45 PM
>
> Generally enabling IOMMU_DEV_FEAT_SVA requires
> IOMMU_DEV_FEAT_IOPF, but
> some devices manage I/O Page Faults themselves instead of relying on the
> IOMMU. Move IOPF related code from SVA to IOPF enabling path to make
> the
> driver work for devices that manage IOPF themselves.
>
> For the device drivers that relies on the IOMMU for IOPF through PCI/PRI,
> IOMMU_DEV_FEAT_IOPF must be enabled before and disabled after
> IOMMU_DEV_FEAT_SVA.
ARM still handles this differently:
arm_smmu_master_enable_sva()
arm_smmu_master_sva_enable_iopf():
{
/*
* Drivers for devices supporting PRI or stall should enable IOPF first.
* Others have device-specific fault handlers and don't need IOPF.
*/
if (!arm_smmu_master_iopf_supported(master))
return 0;
if (!master->iopf_enabled)
return -EINVAL;
}
i.e. device specific IOPF is allowed only when PRI or stall is not supported.
it's different from what this patch does to allow device specific IOPF even
when PRI is supported.
should we make them consistent given SVA/IOPF capabilities are general
iommu definitions or fine to leave each iommu driver with different
restriction?
>
> - ret = iopf_queue_add_device(iommu->iopf_queue, dev);
> - if (!ret)
> - ret = iommu_register_device_fault_handler(dev,
> iommu_queue_iopf, dev);
> -
> - return ret;
> + return 0;
> }
here and below...
> + ret = iopf_queue_add_device(info->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(info->iommu->iopf_queue, dev);
> +
> + return ret;
> }
...indicate a bug fix on error handling. better to have the fix as
a separate patch and then move code.
On 2023/2/6 11:28, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Friday, February 3, 2023 4:45 PM
>>
>> Generally enabling IOMMU_DEV_FEAT_SVA requires
>> IOMMU_DEV_FEAT_IOPF, but
>> some devices manage I/O Page Faults themselves instead of relying on the
>> IOMMU. Move IOPF related code from SVA to IOPF enabling path to make
>> the
>> driver work for devices that manage IOPF themselves.
>>
>> For the device drivers that relies on the IOMMU for IOPF through PCI/PRI,
>> IOMMU_DEV_FEAT_IOPF must be enabled before and disabled after
>> IOMMU_DEV_FEAT_SVA.
>
> ARM still handles this differently:
>
> arm_smmu_master_enable_sva()
> arm_smmu_master_sva_enable_iopf():
> {
> /*
> * Drivers for devices supporting PRI or stall should enable IOPF first.
> * Others have device-specific fault handlers and don't need IOPF.
> */
> if (!arm_smmu_master_iopf_supported(master))
> return 0;
>
> if (!master->iopf_enabled)
> return -EINVAL;
> }
>
> i.e. device specific IOPF is allowed only when PRI or stall is not supported.
>
> it's different from what this patch does to allow device specific IOPF even
> when PRI is supported.
>
> should we make them consistent given SVA/IOPF capabilities are general
> iommu definitions or fine to leave each iommu driver with different
> restriction?
Good point! I prefer the former. I will add a check in sva enabling path
and return failure if device supports PRI but not enabled (that
implies device has its specific IOPF handling).
>
>>
>> - ret = iopf_queue_add_device(iommu->iopf_queue, dev);
>> - if (!ret)
>> - ret = iommu_register_device_fault_handler(dev,
>> iommu_queue_iopf, dev);
>> -
>> - return ret;
>> + return 0;
>> }
>
> here and below...
>
>> + ret = iopf_queue_add_device(info->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(info->iommu->iopf_queue, dev);
>> +
>> + return ret;
>> }
>
> ...indicate a bug fix on error handling. better to have the fix as
> a separate patch and then move code.
>
Yes. I will post a fix patch before this move.
Best regards,
baolu
@@ -4632,7 +4632,6 @@ static int intel_iommu_enable_sva(struct device *dev)
{
struct device_domain_info *info = dev_iommu_priv_get(dev);
struct intel_iommu *iommu;
- int ret;
if (!info || dmar_disabled)
return -EINVAL;
@@ -4644,17 +4643,13 @@ static int intel_iommu_enable_sva(struct device *dev)
if (!(iommu->flags & VTD_FLAG_SVM_CAPABLE))
return -ENODEV;
- if (!info->pasid_enabled || !info->pri_enabled || !info->ats_enabled)
+ if (!info->pasid_enabled)
return -EINVAL;
- ret = iopf_queue_add_device(iommu->iopf_queue, dev);
- if (!ret)
- ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
-
- return ret;
+ return 0;
}
-static int intel_iommu_disable_sva(struct device *dev)
+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;
@@ -4670,11 +4665,20 @@ static int intel_iommu_disable_sva(struct device *dev)
static int intel_iommu_enable_iopf(struct device *dev)
{
struct device_domain_info *info = dev_iommu_priv_get(dev);
+ int ret;
- if (info && info->pri_supported)
- return 0;
+ if (!info || !info->ats_enabled || !info->pri_enabled)
+ return -ENODEV;
- return -ENODEV;
+ ret = iopf_queue_add_device(info->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(info->iommu->iopf_queue, dev);
+
+ return ret;
}
static int
@@ -4697,10 +4701,10 @@ intel_iommu_dev_disable_feat(struct device *dev, enum iommu_dev_features feat)
{
switch (feat) {
case IOMMU_DEV_FEAT_IOPF:
- return 0;
+ return intel_iommu_disable_iopf(dev);
case IOMMU_DEV_FEAT_SVA:
- return intel_iommu_disable_sva(dev);
+ return 0;
default:
return -ENODEV;