On Fri, May 19, 2023 at 8:42 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
>
> This is a step toward making __iommu_probe_device() self contained.
>
> It should, under proper locking, check if the device is already associated
> with an iommu driver and resolve parallel probes. All but one of the
> callers open code this test using two different means, but they all
> rely on dev->iommu_group.
>
> Currently the bus_iommu_probe()/probe_iommu_group() and
> probe_acpi_namespace_devices() rejects already probed devices with an
> unlocked read of dev->iommu_group. The OF and ACPI "replay" functions use
> device_iommu_mapped() which is the same read without the pointless
> refcount.
>
> Move this test into __iommu_probe_device() and put it under the
> iommu_probe_device_lock. The store to dev->iommu_group is in
> iommu_group_add_device() which is also called under this lock for iommu
> driver devices, making it properly locked.
>
> The only path that didn't have this check is the hotplug path triggered by
> BUS_NOTIFY_ADD_DEVICE. The only way to get dev->iommu_group assigned
> outside the probe path is via iommu_group_add_device(). Today the only
> caller is VFIO no-iommu which never associates with an iommu driver. Thus
> adding this additional check is safe.
>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
for the ACPI change in scan.c.
> ---
> drivers/acpi/scan.c | 2 +-
> drivers/iommu/intel/iommu.c | 7 -------
> drivers/iommu/iommu.c | 19 +++++++++----------
> drivers/iommu/of_iommu.c | 2 +-
> 4 files changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 0c6f06abe3f47f..945866f3bd8ebd 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1579,7 +1579,7 @@ static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
> * If we have reason to believe the IOMMU driver missed the initial
> * iommu_probe_device() call for dev, replay it to get things in order.
> */
> - if (!err && dev->bus && !device_iommu_mapped(dev))
> + if (!err && dev->bus)
> err = iommu_probe_device(dev);
>
> /* Ignore all other errors apart from EPROBE_DEFER */
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index b871a6afd80321..3c37b50c121c2d 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3763,7 +3763,6 @@ static int __init probe_acpi_namespace_devices(void)
> for_each_active_dev_scope(drhd->devices,
> drhd->devices_cnt, i, dev) {
> struct acpi_device_physical_node *pn;
> - struct iommu_group *group;
> struct acpi_device *adev;
>
> if (dev->bus != &acpi_bus_type)
> @@ -3773,12 +3772,6 @@ static int __init probe_acpi_namespace_devices(void)
> mutex_lock(&adev->physical_node_lock);
> list_for_each_entry(pn,
> &adev->physical_node_list, node) {
> - group = iommu_group_get(pn->dev);
> - if (group) {
> - iommu_group_put(group);
> - continue;
> - }
> -
> ret = iommu_probe_device(pn->dev);
> if (ret)
> break;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4d7010f2b260a7..6d4d6a4d692893 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -351,9 +351,16 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
> * but for now enforcing a simple global ordering is fine.
> */
> mutex_lock(&iommu_probe_device_lock);
> +
> + /* Device is probed already if in a group */
> + if (dev->iommu_group) {
> + ret = 0;
> + goto out_unlock;
> + }
> +
> if (!dev_iommu_get(dev)) {
> ret = -ENOMEM;
> - goto err_unlock;
> + goto out_unlock;
> }
>
> if (!try_module_get(ops->owner)) {
> @@ -399,7 +406,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
> err_free:
> dev_iommu_free(dev);
>
> -err_unlock:
> +out_unlock:
> mutex_unlock(&iommu_probe_device_lock);
>
> return ret;
> @@ -1711,16 +1718,8 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
> static int probe_iommu_group(struct device *dev, void *data)
> {
> struct list_head *group_list = data;
> - struct iommu_group *group;
> int ret;
>
> - /* Device is probed already if in a group */
> - group = iommu_group_get(dev);
> - if (group) {
> - iommu_group_put(group);
> - return 0;
> - }
> -
> ret = __iommu_probe_device(dev, group_list);
> if (ret == -ENODEV)
> ret = 0;
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 40f57d293a79d4..157b286e36bf3a 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -159,7 +159,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> * If we have reason to believe the IOMMU driver missed the initial
> * probe for dev, replay it to get things in order.
> */
> - if (!err && dev->bus && !device_iommu_mapped(dev))
> + if (!err && dev->bus)
> err = iommu_probe_device(dev);
>
> /* Ignore all other errors apart from EPROBE_DEFER */
> --
> 2.40.1
>
