LinuxLists.cc - [PATCH v5 0/7] iommu: Retire bus ops

[permalink] [raw]

Subject: [PATCH v5 2/7] iommu: Decouple iommu_present() from bus ops

Much as I'd like to remove iommu_present(), the final remaining users
are proving stubbornly difficult to clean up, so kick that can down the
road and just rework it to preserve the current behaviour without
depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup
to IOMMU device registration"), any registered IOMMU instance is already
considered "present" for every entry in iommu_buses, so it's simply a
case of validating the bus and checking we have at least once IOMMU.

Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>

---

v3: Tweak to use the ops-based check rather than group-based, to
properly match the existing behaviour
v4: Just look for IOMMU instances instead of managed devices
---
drivers/iommu/iommu.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5a3ce293a5de..7bb92e8b7a49 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2000,9 +2000,28 @@ int bus_iommu_probe(const struct bus_type *bus)
return 0;
}

+/**
+ * iommu_present() - make platform-specific assumptions about an IOMMU
+ * @bus: bus to check
+ *
+ * Do not use this function. You want device_iommu_mapped() instead.
+ *
+ * Return: true if some IOMMU is present and aware of devices on the given bus;
+ * in general it may not be the only IOMMU, and it may not have anything to do
+ * with whatever device you are ultimately interested in.
+ */
bool iommu_present(const struct bus_type *bus)
{
- return bus->iommu_ops != NULL;
+ bool ret = false;
+
+ for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
+ if (iommu_buses[i] == bus) {
+ spin_lock(&iommu_device_lock);
+ ret = !list_empty(&iommu_device_list);
+ spin_unlock(&iommu_device_lock);
+ }
+ }
+ return ret;
}
EXPORT_SYMBOL_GPL(iommu_present);

--
2.39.2.101.g768bb238c484.dirty

2023-10-11 18:15:56

[permalink] [raw]

Subject: [PATCH v5 4/7] iommu: Decouple iommu_domain_alloc() from bus ops

As the final remaining piece of bus-dependent API, iommu_domain_alloc()
can now take responsibility for the "one iommu_ops per bus" rule for
itself. It turns out we can't safely make the internal allocation call
any more group-based or device-based yet - that will have to wait until
the external callers can pass the right thing - but we can at least get
as far as deriving "bus ops" based on which driver is actually managing
devices on the given bus, rather than whichever driver won the race to
register first.

This will then leave us able to convert the last of the core internals
over to the IOMMU-instance model, allow multiple drivers to register and
actually coexist (modulo the above limitation for unmanaged domain users
in the short term), and start trying to solve the long-standing
iommu_probe_device() mess.

Signed-off-by: Robin Murphy <[email protected]>

---

v5: Rewrite, de-scoping to just retrieve ops under the same assumptions
as the existing code.
---
drivers/iommu/iommu.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 578292d3b152..18667dc2ff86 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2140,12 +2140,31 @@ __iommu_group_domain_alloc(struct iommu_group *group, unsigned int type)
return __iommu_domain_alloc(dev_iommu_ops(dev), dev, type);
}

+static int __iommu_domain_alloc_dev(struct device *dev, void *data)
+{
+ const struct iommu_ops **ops = data;
+
+ if (!dev_has_iommu(dev))
+ return 0;
+
+ if (WARN_ONCE(*ops && *ops != dev_iommu_ops(dev),
+ "Multiple IOMMU drivers present for bus %s, which the public IOMMU API can't fully support yet. You will still need to disable one or more for this to work, sorry!\n",
+ dev_bus_name(dev)))
+ return -EBUSY;
+
+ *ops = dev_iommu_ops(dev);
+ return 0;
+}
+
struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
{
- if (bus == NULL || bus->iommu_ops == NULL)
+ const struct iommu_ops *ops = NULL;
+ int err = bus_for_each_dev(bus, NULL, &ops, __iommu_domain_alloc_dev);
+
+ if (err || !ops)
return NULL;
- return __iommu_domain_alloc(bus->iommu_ops, NULL,
- IOMMU_DOMAIN_UNMANAGED);
+
+ return __iommu_domain_alloc(ops, NULL, IOMMU_DOMAIN_UNMANAGED);
}
EXPORT_SYMBOL_GPL(iommu_domain_alloc);

--
2.39.2.101.g768bb238c484.dirty

2023-10-11 18:15:58

[permalink] [raw]

Subject: [PATCH v5 3/7] iommu: Validate that devices match domains

Before we can allow drivers to coexist, we need to make sure that one
driver's domain ops can't misinterpret another driver's dev_iommu_priv
data. To that end, add a token to the domain so we can remember how it
was allocated - for now this may as well be the device ops, since they
still correlate 1:1 with drivers. We can trust ourselves for internal
default domain attachment, so add checks to cover all the public attach
interfaces.

Reviewed-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>

---

v4: Cover iommu_attach_device_pasid() as well, and improve robustness
against theoretical attempts to attach a noiommu group.
---
drivers/iommu/iommu.c | 10 ++++++++++
include/linux/iommu.h | 1 +
2 files changed, 11 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7bb92e8b7a49..578292d3b152 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2114,6 +2114,7 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
return NULL;

domain->type = type;
+ domain->owner = ops;
/*
* If not already set, assume all sizes by default; the driver
* may override this later
@@ -2279,10 +2280,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
static int __iommu_attach_group(struct iommu_domain *domain,
struct iommu_group *group)
{
+ struct device *dev;
+
if (group->domain && group->domain != group->default_domain &&
group->domain != group->blocking_domain)
return -EBUSY;

+ dev = iommu_group_first_dev(group);
+ if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
+ return -EINVAL;
+
return __iommu_group_set_domain(group, domain);
}

@@ -3480,6 +3487,9 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
if (!group)
return -ENODEV;

+ if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
+ return -EINVAL;
+
mutex_lock(&group->mutex);
curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
if (curr) {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2d2802fb2c74..5c9560813d05 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -99,6 +99,7 @@ struct iommu_domain_geometry {
struct iommu_domain {
unsigned type;
const struct iommu_domain_ops *ops;
+ const struct iommu_ops *owner; /* Whose domain_alloc we came from */
unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
--
2.39.2.101.g768bb238c484.dirty

2023-10-11 18:16:01

[permalink] [raw]

Subject: [PATCH v5 5/7] iommu/arm-smmu: Don't register fwnode for legacy binding

When using the legacy binding we bypass the of_xlate mechanism, so avoid
registering the instance fwnodes which act as keys for that. This will
help __iommu_probe_device() to retrieve the registered ops the same way
as for x86 etc. when no fwspec has previously been set up by of_xlate.

Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/arm/arm-smmu/arm-smmu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index d6d1a2a55cc0..4b83a3adacd6 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -2161,7 +2161,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
return err;
}

- err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
+ err = iommu_device_register(&smmu->iommu, &arm_smmu_ops,
+ using_legacy_binding ? NULL : dev);
if (err) {
dev_err(dev, "Failed to register iommu\n");
iommu_device_sysfs_remove(&smmu->iommu);
--
2.39.2.101.g768bb238c484.dirty

2023-10-11 18:16:24

[permalink] [raw]

Subject: [PATCH v5 7/7] iommu: Clean up open-coded ownership checks

Some drivers already implement their own defence against the possibility
of being given someone else's device. Since this is now taken care of by
the core code (and via a slightly different path from the original
fwspec-based idea), let's clean them up.

Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 ---
drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +--------
drivers/iommu/arm/arm-smmu/qcom_iommu.c | 16 +++-------------
drivers/iommu/mtk_iommu.c | 7 +------
drivers/iommu/mtk_iommu_v1.c | 3 ---
drivers/iommu/sprd-iommu.c | 8 +-------
drivers/iommu/virtio-iommu.c | 3 ---
7 files changed, 6 insertions(+), 43 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index e82bf1c449a3..01ea307c7791 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2653,9 +2653,6 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
struct arm_smmu_master *master;
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);

- if (!fwspec || fwspec->ops != &arm_smmu_ops)
- return ERR_PTR(-ENODEV);
-
if (WARN_ON_ONCE(dev_iommu_priv_get(dev)))
return ERR_PTR(-EBUSY);

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 4b83a3adacd6..4d09c0047892 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1116,11 +1116,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
struct arm_smmu_device *smmu;
int ret;

- if (!fwspec || fwspec->ops != &arm_smmu_ops) {
- dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
- return -ENXIO;
- }
-
/*
* FIXME: The arch/arm DMA API code tries to attach devices to its own
* domains between of_xlate() and probe_device() - we have no way to cope
@@ -1357,10 +1352,8 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
fwspec = dev_iommu_fwspec_get(dev);
if (ret)
goto out_free;
- } else if (fwspec && fwspec->ops == &arm_smmu_ops) {
- smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
} else {
- return ERR_PTR(-ENODEV);
+ smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
}

ret = -EINVAL;
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 97b2122032b2..33f3c870086c 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -79,16 +79,6 @@ static struct qcom_iommu_domain *to_qcom_iommu_domain(struct iommu_domain *dom)

static const struct iommu_ops qcom_iommu_ops;

-static struct qcom_iommu_dev * to_iommu(struct device *dev)
-{
- struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-
- if (!fwspec || fwspec->ops != &qcom_iommu_ops)
- return NULL;
-
- return dev_iommu_priv_get(dev);
-}
-
static struct qcom_iommu_ctx * to_ctx(struct qcom_iommu_domain *d, unsigned asid)
{
struct qcom_iommu_dev *qcom_iommu = d->iommu;
@@ -372,7 +362,7 @@ static void qcom_iommu_domain_free(struct iommu_domain *domain)

static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
{
- struct qcom_iommu_dev *qcom_iommu = to_iommu(dev);
+ struct qcom_iommu_dev *qcom_iommu = dev_iommu_priv_get(dev);
struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);
int ret;

@@ -404,7 +394,7 @@ static int qcom_iommu_identity_attach(struct iommu_domain *identity_domain,
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
struct qcom_iommu_domain *qcom_domain;
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
- struct qcom_iommu_dev *qcom_iommu = to_iommu(dev);
+ struct qcom_iommu_dev *qcom_iommu = dev_iommu_priv_get(dev);
unsigned int i;

if (domain == identity_domain || !domain)
@@ -535,7 +525,7 @@ static bool qcom_iommu_capable(struct device *dev, enum iommu_cap cap)

static struct iommu_device *qcom_iommu_probe_device(struct device *dev)
{
- struct qcom_iommu_dev *qcom_iommu = to_iommu(dev);
+ struct qcom_iommu_dev *qcom_iommu = dev_iommu_priv_get(dev);
struct device_link *link;

if (!qcom_iommu)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 19ef50221c93..0a3698c33e19 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -863,16 +863,11 @@ static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
static struct iommu_device *mtk_iommu_probe_device(struct device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
- struct mtk_iommu_data *data;
+ struct mtk_iommu_data *data = dev_iommu_priv_get(dev);
struct device_link *link;
struct device *larbdev;
unsigned int larbid, larbidx, i;

- if (!fwspec || fwspec->ops != &mtk_iommu_ops)
- return ERR_PTR(-ENODEV); /* Not a iommu client device */
-
- data = dev_iommu_priv_get(dev);
-
if (!MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM))
return &data->iommu;

diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 67e044c1a7d9..25b41222abae 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -481,9 +481,6 @@ static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev)
idx++;
}

- if (!fwspec || fwspec->ops != &mtk_iommu_v1_ops)
- return ERR_PTR(-ENODEV); /* Not a iommu client device */
-
data = dev_iommu_priv_get(dev);

/* Link the consumer device with the smi-larb device(supplier) */
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index 9c33ea6903f6..b15a8fe7ae8a 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -384,13 +384,7 @@ static phys_addr_t sprd_iommu_iova_to_phys(struct iommu_domain *domain,

static struct iommu_device *sprd_iommu_probe_device(struct device *dev)
{
- struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
- struct sprd_iommu_device *sdev;
-
- if (!fwspec || fwspec->ops != &sprd_iommu_ops)
- return ERR_PTR(-ENODEV);
-
- sdev = dev_iommu_priv_get(dev);
+ struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);

return &sdev->iommu;
}
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 17dcd826f5c2..bb2e795a80d0 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -969,9 +969,6 @@ static struct iommu_device *viommu_probe_device(struct device *dev)
struct viommu_dev *viommu = NULL;
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);

- if (!fwspec || fwspec->ops != &viommu_ops)
- return ERR_PTR(-ENODEV);
-
viommu = viommu_get_by_fwnode(fwspec->iommu_fwnode);
if (!viommu)
return ERR_PTR(-ENODEV);
--
2.39.2.101.g768bb238c484.dirty

2023-10-11 23:38:59

[permalink] [raw]

Subject: Re: [PATCH v5 4/7] iommu: Decouple iommu_domain_alloc() from bus ops

On Wed, Oct 11, 2023 at 07:14:51PM +0100, Robin Murphy wrote:
> As the final remaining piece of bus-dependent API, iommu_domain_alloc()
> can now take responsibility for the "one iommu_ops per bus" rule for
> itself. It turns out we can't safely make the internal allocation call
> any more group-based or device-based yet - that will have to wait until
> the external callers can pass the right thing - but we can at least get
> as far as deriving "bus ops" based on which driver is actually managing
> devices on the given bus, rather than whichever driver won the race to
> register first.
>
> This will then leave us able to convert the last of the core internals
> over to the IOMMU-instance model, allow multiple drivers to register and
> actually coexist (modulo the above limitation for unmanaged domain users
> in the short term), and start trying to solve the long-standing
> iommu_probe_device() mess.
>
> Signed-off-by: Robin Murphy <[email protected]>
>
> ---
>
> v5: Rewrite, de-scoping to just retrieve ops under the same assumptions
> as the existing code.
> ---
> drivers/iommu/iommu.c | 25 ++++++++++++++++++++++---
> 1 file changed, 22 insertions(+), 3 deletions(-)

Reviewed-by: Jason Gunthorpe <[email protected]>

FWIW, I was thinking afterwords that domain_alloc_paging() probably
should have been:

(domain_alloc_paging *)(struct iommu_device *iommu, struct iommu_group *group)

Most drivers can use the iommu and ignore the group, a few special
ones can do the needed reduce operation across the group.

We can get to that later when we get deeper into the PASID troubles,
it also requires the deferal of the domain creation like the bus code
probe does but the fwnode path doesn't :\

Jason

2023-10-12 06:09:07

by Baolu Lu

[permalink] [raw]

Subject: Re: [PATCH v5 2/7] iommu: Decouple iommu_present() from bus ops

On 10/12/23 2:14 AM, Robin Murphy wrote:
> Much as I'd like to remove iommu_present(), the final remaining users
> are proving stubbornly difficult to clean up, so kick that can down the
> road and just rework it to preserve the current behaviour without
> depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup

The iommu_present() is only used in below two drivers.

$ git grep iommu_present
drivers/gpu/drm/mediatek/mtk_drm_drv.c: if
(!iommu_present(&platform_bus_type))
drivers/gpu/drm/tegra/drm.c: if (host1x_drm_wants_iommu(dev) &&
iommu_present(&platform_bus_type)) {

Both are platform drivers and have the device pointer passed in. Just
out of curiosity, why not replacing them with device_iommu_mapped()
instead? Sorry if I overlooked previous discussion.

Best regards,
baolu

> to IOMMU device registration"), any registered IOMMU instance is already
> considered "present" for every entry in iommu_buses, so it's simply a
> case of validating the bus and checking we have at least once IOMMU.
>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
>
> ---
>
> v3: Tweak to use the ops-based check rather than group-based, to
> properly match the existing behaviour
> v4: Just look for IOMMU instances instead of managed devices
> ---
> drivers/iommu/iommu.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 5a3ce293a5de..7bb92e8b7a49 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2000,9 +2000,28 @@ int bus_iommu_probe(const struct bus_type *bus)
> return 0;
> }
>
> +/**
> + * iommu_present() - make platform-specific assumptions about an IOMMU
> + * @bus: bus to check
> + *
> + * Do not use this function. You want device_iommu_mapped() instead.
> + *
> + * Return: true if some IOMMU is present and aware of devices on the given bus;
> + * in general it may not be the only IOMMU, and it may not have anything to do
> + * with whatever device you are ultimately interested in.
> + */
> bool iommu_present(const struct bus_type *bus)
> {
> - return bus->iommu_ops != NULL;
> + bool ret = false;
> +
> + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
> + if (iommu_buses[i] == bus) {
> + spin_lock(&iommu_device_lock);
> + ret = !list_empty(&iommu_device_list);
> + spin_unlock(&iommu_device_lock);
> + }
> + }
> + return ret;
> }
> EXPORT_SYMBOL_GPL(iommu_present);
>

2023-10-12 11:40:56

[permalink] [raw]

Subject: Re: [PATCH v5 2/7] iommu: Decouple iommu_present() from bus ops

On 2023-10-12 07:05, Baolu Lu wrote:
> On 10/12/23 2:14 AM, Robin Murphy wrote:
>> Much as I'd like to remove iommu_present(), the final remaining users
>> are proving stubbornly difficult to clean up, so kick that can down the
>> road and just rework it to preserve the current behaviour without
>> depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup
>
> The iommu_present() is only used in below two drivers.
>
> $ git grep iommu_present
> drivers/gpu/drm/mediatek/mtk_drm_drv.c: if
> (!iommu_present(&platform_bus_type))
> drivers/gpu/drm/tegra/drm.c:    if (host1x_drm_wants_iommu(dev) &&
> iommu_present(&platform_bus_type)) {
>
> Both are platform drivers and have the device pointer passed in. Just
> out of curiosity, why not replacing them with device_iommu_mapped()
> instead? Sorry if I overlooked previous discussion.

Yes, we've already gone round in circles on this several times, that's
why it's explicitly called out as "stubbornly difficult" in the commit
message. The Mediatek one is entirely redundant, but it seems I have yet
to figure out the right CC list to get anyone to care about that
patch[1]. The Tegra one is making some non-obvious assumptions to
actually check on behalf of some *other* devices, even when the one to
hand may not be using the IOMMU itself[2]. That case is what the new
kerneldoc alludes to.

My hope is to eventually punt this into the Tegra driver itself
(probably at the point when it needs something similar for
iommu_domain_alloc() as well), however previous experience has taught me
that trying to coordinate cross-subsystem work with drm-misc is an
ordeal best avoided until there is no possible alternative.

Thanks,
Robin.

[1] https://patchwork.freedesktop.org/patch/536273/
[2]
https://lore.kernel.org/linux-iommu/[email protected]/

>
> Best regards,
> baolu
>
>> to IOMMU device registration"), any registered IOMMU instance is already
>> considered "present" for every entry in iommu_buses, so it's simply a
>> case of validating the bus and checking we have at least once IOMMU.
>>
>> Reviewed-by: Jason Gunthorpe <[email protected]>
>> Signed-off-by: Robin Murphy <[email protected]>
>>
>> ---
>>
>> v3: Tweak to use the ops-based check rather than group-based, to
>>      properly match the existing behaviour
>> v4: Just look for IOMMU instances instead of managed devices
>> ---
>> drivers/iommu/iommu.c | 21 ++++++++++++++++++++-
>> 1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 5a3ce293a5de..7bb92e8b7a49 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2000,9 +2000,28 @@ int bus_iommu_probe(const struct bus_type *bus)
>>       return 0;
>> }
>> +/**
>> + * iommu_present() - make platform-specific assumptions about an IOMMU
>> + * @bus: bus to check
>> + *
>> + * Do not use this function. You want device_iommu_mapped() instead.
>> + *
>> + * Return: true if some IOMMU is present and aware of devices on the
>> given bus;
>> + * in general it may not be the only IOMMU, and it may not have
>> anything to do
>> + * with whatever device you are ultimately interested in.
>> + */
>> bool iommu_present(const struct bus_type *bus)
>> {
>> -    return bus->iommu_ops != NULL;
>> +    bool ret = false;
>> +
>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
>> +        if (iommu_buses[i] == bus) {
>> +            spin_lock(&iommu_device_lock);
>> +            ret = !list_empty(&iommu_device_list);
>> +            spin_unlock(&iommu_device_lock);
>> +        }
>> +    }
>> +    return ret;
>> }
>> EXPORT_SYMBOL_GPL(iommu_present);

2023-10-12 12:38:26

[permalink] [raw]

Subject: Re: [PATCH v5 2/7] iommu: Decouple iommu_present() from bus ops

On Thu, Oct 12, 2023 at 12:40:01PM +0100, Robin Murphy wrote:
> On 2023-10-12 07:05, Baolu Lu wrote:
> > On 10/12/23 2:14 AM, Robin Murphy wrote:
> > > Much as I'd like to remove iommu_present(), the final remaining users
> > > are proving stubbornly difficult to clean up, so kick that can down the
> > > road and just rework it to preserve the current behaviour without
> > > depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup
> >
> > The iommu_present() is only used in below two drivers.
> >
> > $ git grep iommu_present
> > drivers/gpu/drm/mediatek/mtk_drm_drv.c: if
> > (!iommu_present(&platform_bus_type))
> > drivers/gpu/drm/tegra/drm.c: if (host1x_drm_wants_iommu(dev) &&
> > iommu_present(&platform_bus_type)) {
> >
> > Both are platform drivers and have the device pointer passed in. Just
> > out of curiosity, why not replacing them with device_iommu_mapped()
> > instead? Sorry if I overlooked previous discussion.
>
> Yes, we've already gone round in circles on this several times, that's why
> it's explicitly called out as "stubbornly difficult" in the commit message.
> The Mediatek one is entirely redundant, but it seems I have yet to figure
> out the right CC list to get anyone to care about that patch[1].

Please just have Joerg take such a trivial patch, there is no reason
we need to torture outselves because DRM side is not behaving well. :(

Jason

2023-10-12 12:56:29

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH v5 5/7] iommu/arm-smmu: Don't register fwnode for legacy binding

On Wed, Oct 11, 2023 at 07:14:52PM +0100, Robin Murphy wrote:
> When using the legacy binding we bypass the of_xlate mechanism, so avoid
> registering the instance fwnodes which act as keys for that. This will
> help __iommu_probe_device() to retrieve the registered ops the same way
> as for x86 etc. when no fwspec has previously been set up by of_xlate.
>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index d6d1a2a55cc0..4b83a3adacd6 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -2161,7 +2161,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> return err;
> }
>
> - err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
> + err = iommu_device_register(&smmu->iommu, &arm_smmu_ops,
> + using_legacy_binding ? NULL : dev);
> if (err) {
> dev_err(dev, "Failed to register iommu\n");
> iommu_device_sysfs_remove(&smmu->iommu);
> --
> 2.39.2.101.g768bb238c484.dirty

Acked-by: Will Deacon <[email protected]>

Will

2023-10-12 12:58:04

by Baolu Lu

[permalink] [raw]

Subject: Re: [PATCH v5 2/7] iommu: Decouple iommu_present() from bus ops

On 2023/10/12 20:37, Jason Gunthorpe wrote:
> On Thu, Oct 12, 2023 at 12:40:01PM +0100, Robin Murphy wrote:
>> On 2023-10-12 07:05, Baolu Lu wrote:
>>> On 10/12/23 2:14 AM, Robin Murphy wrote:
>>>> Much as I'd like to remove iommu_present(), the final remaining users
>>>> are proving stubbornly difficult to clean up, so kick that can down the
>>>> road and just rework it to preserve the current behaviour without
>>>> depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup
>>>
>>> The iommu_present() is only used in below two drivers.
>>>
>>> $ git grep iommu_present
>>> drivers/gpu/drm/mediatek/mtk_drm_drv.c: if
>>> (!iommu_present(&platform_bus_type))
>>> drivers/gpu/drm/tegra/drm.c: if (host1x_drm_wants_iommu(dev) &&
>>> iommu_present(&platform_bus_type)) {
>>>
>>> Both are platform drivers and have the device pointer passed in. Just
>>> out of curiosity, why not replacing them with device_iommu_mapped()
>>> instead? Sorry if I overlooked previous discussion.
>>
>> Yes, we've already gone round in circles on this several times, that's why
>> it's explicitly called out as "stubbornly difficult" in the commit message.
>> The Mediatek one is entirely redundant, but it seems I have yet to figure
>> out the right CC list to get anyone to care about that patch[1].

I see now. Thanks for the explanation.

>
> Please just have Joerg take such a trivial patch, there is no reason
> we need to torture outselves because DRM side is not behaving well. :(

I was not object to the patch. Just want to make sure that I understand
the reason why device_iommu_mapped() can't be used in those two drivers.

It's fine to me. I will add my r-b.

Best regards,
baolu

2023-10-12 12:58:10

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH v5 7/7] iommu: Clean up open-coded ownership checks

On Wed, Oct 11, 2023 at 07:14:54PM +0100, Robin Murphy wrote:
> Some drivers already implement their own defence against the possibility
> of being given someone else's device. Since this is now taken care of by
> the core code (and via a slightly different path from the original
> fwspec-based idea), let's clean them up.
>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 ---
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +--------
> drivers/iommu/arm/arm-smmu/qcom_iommu.c | 16 +++-------------
> drivers/iommu/mtk_iommu.c | 7 +------
> drivers/iommu/mtk_iommu_v1.c | 3 ---
> drivers/iommu/sprd-iommu.c | 8 +-------
> drivers/iommu/virtio-iommu.c | 3 ---
> 7 files changed, 6 insertions(+), 43 deletions(-)

Acked-by: Will Deacon <[email protected]>

Will

2023-10-12 12:59:13

by Baolu Lu

[permalink] [raw]

Subject: Re: [PATCH v5 2/7] iommu: Decouple iommu_present() from bus ops

On 2023/10/12 2:14, Robin Murphy wrote:
> Much as I'd like to remove iommu_present(), the final remaining users
> are proving stubbornly difficult to clean up, so kick that can down the
> road and just rework it to preserve the current behaviour without
> depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup
> to IOMMU device registration"), any registered IOMMU instance is already
> considered "present" for every entry in iommu_buses, so it's simply a
> case of validating the bus and checking we have at least once IOMMU.
>
> Reviewed-by: Jason Gunthorpe<[email protected]>
> Signed-off-by: Robin Murphy<[email protected]>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2023-10-18 23:07:23

[permalink] [raw]

Subject: Re: [PATCH v5 2/7] iommu: Decouple iommu_present() from bus ops

On Wed, Oct 11, 2023 at 07:14:49PM +0100, Robin Murphy wrote:
> Much as I'd like to remove iommu_present(), the final remaining users
> are proving stubbornly difficult to clean up, so kick that can down the
> road and just rework it to preserve the current behaviour without
> depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup
> to IOMMU device registration"), any registered IOMMU instance is already
> considered "present" for every entry in iommu_buses, so it's simply a
> case of validating the bus and checking we have at least once IOMMU.
>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
>
> ---
>
> v3: Tweak to use the ops-based check rather than group-based, to
> properly match the existing behaviour
> v4: Just look for IOMMU instances instead of managed devices
> ---

Reviewed-by: Jerry Snitselaar <[email protected]>

2023-10-18 23:16:19

[permalink] [raw]

Subject: Re: [PATCH v5 3/7] iommu: Validate that devices match domains

On Wed, Oct 11, 2023 at 07:14:50PM +0100, Robin Murphy wrote:
> Before we can allow drivers to coexist, we need to make sure that one
> driver's domain ops can't misinterpret another driver's dev_iommu_priv
> data. To that end, add a token to the domain so we can remember how it
> was allocated - for now this may as well be the device ops, since they
> still correlate 1:1 with drivers. We can trust ourselves for internal
> default domain attachment, so add checks to cover all the public attach
> interfaces.
>
> Reviewed-by: Lu Baolu <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
>
> ---
>
> v4: Cover iommu_attach_device_pasid() as well, and improve robustness
> against theoretical attempts to attach a noiommu group.
> ---

Reviewed-by: Jerry Snitselaar <[email protected]>

2023-10-18 23:19:14

[permalink] [raw]

Subject: Re: [PATCH v5 4/7] iommu: Decouple iommu_domain_alloc() from bus ops

2023-10-18 23:31:05

[permalink] [raw]

Subject: Re: [PATCH v5 5/7] iommu/arm-smmu: Don't register fwnode for legacy binding

2023-10-18 23:41:52

[permalink] [raw]

Subject: Re: [PATCH v5 7/7] iommu: Clean up open-coded ownership checks

2023-10-24 18:52:24

[permalink] [raw]

Subject: Re: [PATCH v5 3/7] iommu: Validate that devices match domains

On Wed, Oct 11, 2023 at 07:14:50PM +0100, Robin Murphy wrote:

> @@ -2279,10 +2280,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
> static int __iommu_attach_group(struct iommu_domain *domain,
> struct iommu_group *group)
> {
> + struct device *dev;
> +
> if (group->domain && group->domain != group->default_domain &&
> group->domain != group->blocking_domain)
> return -EBUSY;
>
> + dev = iommu_group_first_dev(group);
> + if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
> + return -EINVAL;

I was thinking about this later, how does this work for the global
static domains? domain->owner will not be set?

if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
return ops->identity_domain;
else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
return ops->blocked_domain;

Seems like it will break everything?

I suggest we just put a simple void * tag in the const domain->ops at
compile time to indicate the owning driver.

Jason

2023-10-25 12:40:16

[permalink] [raw]

Subject: Re: [PATCH v5 3/7] iommu: Validate that devices match domains

On 24/10/2023 7:52 pm, Jason Gunthorpe wrote:
> On Wed, Oct 11, 2023 at 07:14:50PM +0100, Robin Murphy wrote:
>
>> @@ -2279,10 +2280,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
>> static int __iommu_attach_group(struct iommu_domain *domain,
>> struct iommu_group *group)
>> {
>> + struct device *dev;
>> +
>> if (group->domain && group->domain != group->default_domain &&
>> group->domain != group->blocking_domain)
>> return -EBUSY;
>>
>> + dev = iommu_group_first_dev(group);
>> + if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
>> + return -EINVAL;
>
> I was thinking about this later, how does this work for the global
> static domains? domain->owner will not be set?
>
> if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
> return ops->identity_domain;
> else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
> return ops->blocked_domain;
>
> Seems like it will break everything?

I don't believe it makes any significant difference - as the commit
message points out, this validation is only applied at the public
interface boundaries of iommu_attach_group(), iommu_attach_device(), and
iommu_attach_device_pasid(), which are only expected to be operating on
explicitly-allocated unmanaged domains. For internal default domain
attachment, the domain is initially derived from the device/group itself
so we know it's appropriate by construction.

I guess this *would* now prevent some external caller reaching in and
trying to attach something to some other group's identity default
domain, but frankly it feels like making that fail would be no bad thing
anyway.

Thanks,
Robin.

2023-10-25 12:55:53

[permalink] [raw]

Subject: Re: [PATCH v5 3/7] iommu: Validate that devices match domains

On Wed, Oct 25, 2023 at 01:39:56PM +0100, Robin Murphy wrote:
> On 24/10/2023 7:52 pm, Jason Gunthorpe wrote:
> > On Wed, Oct 11, 2023 at 07:14:50PM +0100, Robin Murphy wrote:
> >
> > > @@ -2279,10 +2280,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
> > > static int __iommu_attach_group(struct iommu_domain *domain,
> > > struct iommu_group *group)
> > > {
> > > + struct device *dev;
> > > +
> > > if (group->domain && group->domain != group->default_domain &&
> > > group->domain != group->blocking_domain)
> > > return -EBUSY;
> > > + dev = iommu_group_first_dev(group);
> > > + if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
> > > + return -EINVAL;
> >
> > I was thinking about this later, how does this work for the global
> > static domains? domain->owner will not be set?
> >
> > if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
> > return ops->identity_domain;
> > else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
> > return ops->blocked_domain;
> >
> > Seems like it will break everything?
>
> I don't believe it makes any significant difference - as the commit message
> points out, this validation is only applied at the public interface
> boundaries of iommu_attach_group(), iommu_attach_device(),

Oh, making it only work for on domain type seems kind of hacky..

If that is the intention maybe the owner set should be moved into
iommu_domain_alloc() with a little comment noting that it is limited
to work in only a few cases?

I certainly didn't understand from the commit message to mean it was
only actually working for one domain type and this also blocks using
other types with the public interface.

> and iommu_attach_device_pasid(), which are only expected to be
> operating on explicitly-allocated unmanaged domains.

We have nesting now in the iommufd branch, and SVA will come soon for
these APIs.

Regardless this will clash with the iommufd branch for this reason so
I guess it needs to wait till rc1.

Thanks,
Jason

2023-10-25 16:07:01

[permalink] [raw]

Subject: Re: [PATCH v5 3/7] iommu: Validate that devices match domains

On 25/10/2023 1:55 pm, Jason Gunthorpe wrote:
> On Wed, Oct 25, 2023 at 01:39:56PM +0100, Robin Murphy wrote:
>> On 24/10/2023 7:52 pm, Jason Gunthorpe wrote:
>>> On Wed, Oct 11, 2023 at 07:14:50PM +0100, Robin Murphy wrote:
>>>
>>>> @@ -2279,10 +2280,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
>>>> static int __iommu_attach_group(struct iommu_domain *domain,
>>>> struct iommu_group *group)
>>>> {
>>>> + struct device *dev;
>>>> +
>>>> if (group->domain && group->domain != group->default_domain &&
>>>> group->domain != group->blocking_domain)
>>>> return -EBUSY;
>>>> + dev = iommu_group_first_dev(group);
>>>> + if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
>>>> + return -EINVAL;
>>>
>>> I was thinking about this later, how does this work for the global
>>> static domains? domain->owner will not be set?
>>>
>>> if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
>>> return ops->identity_domain;
>>> else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
>>> return ops->blocked_domain;
>>>
>>> Seems like it will break everything?
>>
>> I don't believe it makes any significant difference - as the commit message
>> points out, this validation is only applied at the public interface
>> boundaries of iommu_attach_group(), iommu_attach_device(),
>
> Oh, making it only work for on domain type seems kind of hacky..
>
> If that is the intention maybe the owner set should be moved into
> iommu_domain_alloc() with a little comment noting that it is limited
> to work in only a few cases?
>
> I certainly didn't understand from the commit message to mean it was
> only actually working for one domain type and this also blocks using
> other types with the public interface.

It's not about one particular domain type, it's about the scope of what
we consider valid usage. External API users should almost always be
attaching to their own domain which they have allocated, however we also
tolerate co-attaching additional groups to the same DMA domain in rare
cases where it's reasonable. The fact is that those users cannot
allocate blocking or identity domains, and I can't see that they would
ever have any legitimate business trying to do anything with them
anyway. So although yes, we technically lose some functionality once
this intersects with the static domain optimisation, it's only
questionable functionality which was never explicitly intended anyway.

I mean, what would be the valid purpose of trying to attach group A to
group B's identity domain, even if they *were* backed by the same
driver? At best it's pointless if group A also has its own identity
domain already, otherwise at worst it's a deliberate attempt to
circumvent a default domain policy imposed by the IOMMU core.

>> and iommu_attach_device_pasid(), which are only expected to be
>> operating on explicitly-allocated unmanaged domains.
>
> We have nesting now in the iommufd branch, and SVA will come soon for
> these APIs.
>
> Regardless this will clash with the iommufd branch for this reason so
> I guess it needs to wait till rc1.

Sigh, back on the shelf it goes then...

Thanks,
Robin.

2023-10-25 16:16:05