RE: [PATCH 2/2] iommu/vt-d: Move iopf code from SVA to IOPF enabling path

Tian, Kevin
2023-02-06T03:29:07Z

> 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.