Re: [PATCH 0/6] of: iommu-map parsing for multi-cell IOMMU
From: Robin Murphy
Date: Mon Nov 24 2025 - 11:51:34 EST
On 2025-11-12 2:42 pm, Charan Teja Kalla wrote:
On 11/11/2025 11:57 PM, Charan Teja Kalla wrote:
On 11/5/2025 10:58 PM, Robin Murphy wrote:
Thanks a lot Robin for taking a look and sorry for the delayed reply asThe other motivation for this patchset is the below usecase.Hmm, I did actually have a quick go at this the other week too, and
USECASE [1]:
------------
Video IP, 32bit, have 2 hardware sub blocks(or can be called as
functions) called as pixel and nonpixel blocks, that does decode and
encode of the video stream. These logical blocks are configured to
generate different stream IDs.
With the classical approach of representing all sids with iommus= end up
in using a single translation context limited to the 4GB. There are
video usecases which needs larger IOVA space, like higher concurrent
video sessions(eg: 32 session and 192MB per session) where 4GB of IOVA
is not sufficient.
For this case, each functionality is represented in the firmware(device
tree) by the 'rid' field of the iommu-map property and the video driver
creates sub platform devices for each of this functionality and call
into IOMMU configuration. Each rid(function id) in the dt property
indicates the bit that can be associated by the driver passed input id.
Example:
iommu {
#iommu-cells = 2;
};
video-codec@foobar {
compatible = "qcom,video";
iommus = <&apps_smmu 0x1234 0xca>;
iommu-map= <0x1 &iommu 0x1940 0x0 0x1>,
<0x1 &iommu 0x1941 0x0 0x1>,
<0x2 &iommu 0x1942 0x0 0x1>,
<0x4 &iommu 0x1943 0x0 0x1>,
<0x4 &iommu 0x1944 0x0 0x1>;
};
video-driver:
#define PIXEL_FUNC (1)
#define NON_PIXEL_FUNC (2)
#define SECURE_FUNC (4)
case1: All these functionalities requires individual contexts.
Create 3 subdevices for each of this function and call
of_dma_configure_id(..,id), id = 0x1, 0x2, 0x4.
Case2: Secure and non-secure functionalities require individual
contexts. Create 2 subdevices and call of_dma_configure_id(..,id), id =
0x3(bitmap of pixel and non-pixel), 0x4 (secure).
Credits: to Dmitry for thorough discussions on the RFC patch and major
help in getting the consenus on this approach, to Konrad & Bjorn for
offline discussions and reviews, to Robin for his inputs on IOMMU front,
to Bod, Rob and Krzysztof for all valuable inputs.
[1] https://lore.kernel.org/all/20250627-video_cb-
v3-0-51e18c0ffbce@xxxxxxxxxxx/
[2] https://lore.kernel.org/all/20250928171718.436440-1-
charan.kalla@xxxxxxxxxxxxxxxx/#r
Charan Teja Kalla (6):
of: create a wrapper for of_map_id()
of: introduce wrapper function to query the cell count
of: parse #<name>-cells property to get the cell count
of: detect and handle legacy iommu-map parsing
of: add infra to parse iommu-map per IOMMU cell count
of: use correct iommu-map parsing logic from of_iommu layer
drivers/iommu/of_iommu.c | 59 +++++++--
drivers/of/base.c | 269 +++++++++++++++++++++++++++++++++++----
include/linux/of.h | 19 +++
3 files changed, 314 insertions(+), 33 deletions(-)
while I though it was a bit clunky, it was still significantly simpler
than this seems to be...
FWIW: https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu-map - I
I was on vacation.
stripped code_snippet from your patch:
offset = 0;
out_base = map + offset + 2;
id_off = masked_id - id_base;
if (masked_id < id_base || id_off >= id_len)
continue;
for (int i = 0; id_out && i < cells; i++)
id_out[i] = id_off + be32_to_cpu(out_base[i]);
seems way cleaner than mine...
Actually, we also have a case of a device emitting 2 distinct
identifiers, eg: a device is emitting 0x1940, 0x1944 and 0x1A20 sids and
attached to a single context bank. If I use mask to cover all these sids
in a single iommu-map entry, it does overlap with other device SID.
I don't think that patch you shared can be used to cover the above, or
it is?
No, the point of my patch is just to correctly respect #cells and hopefully discourage any further abuse of Linux's current behaviour. The fact that Linux interprets multiple mappings for the same input ID as a set of equivalent choices to pick one of, rather than a set that must all be maintained in parallel, is an orthogonal concern. Again there's not necessarily one right answer there.
Hence I resorted to the approach where RID used as the bitmap of indices
to cover such cases for platform devices, which it seems you clearly
didn't like....otherwise, any otherway we can handle such cases?
Eww, if the intent of that was to bake further abuse into DT ABI to bodge around Linux behaviour then I double-NAK it even harder :)
Hi Robin,
Don't want to bother you with my ideas, but I can't think of other ways
to handle such cases of multi-map than the below. I just tried this code on
Qemu on top of your patches(with some nit compilation fixes) and just checked
if devices are added to the iommu groups.
My initial thought would be to either add an index argument so that callers can keep count and request the Nth match if they want to - like we do in various resource management APIs, for instance - or go all the way and convert the existing target and id_out complexity into usage-specific callbacks too.
----------------------8888---------------------------------------------
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index a511ecf21fcd..ac005e70de7d 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -16,6 +16,7 @@
#include <linux/pci.h>
#include <linux/slab.h>
#include <linux/fsl/mc.h>
+#include <linux/platform_device.h>
#include "iommu-priv.h"
@@ -41,6 +42,18 @@ static int of_iommu_xlate(struct device *dev,
return ret;
}
+static int of_iommu_configure_cb(void *arg, u32 *id_out)
+{
+ struct of_phandle_args *iommu_spec =
+ (struct of_phandle_args *)((void *)id_out - offsetof(struct of_phandle_args, args));
Not sure whether to be impressed or disgusted... If we are to take a callback approach then it should probably standardise on passing a full of_phandle_args to encode the map output. Particularly given what I've just noticed below...
+ struct device *dev = arg;
+ int err;
+
+ err = of_iommu_xlate(dev, iommu_spec);
+ of_node_put(iommu_spec->np);
+ return err;
+}
+
static int of_iommu_configure_dev_id(struct device_node *master_np,
struct device *dev,
const u32 *id)
@@ -48,12 +61,10 @@ static int of_iommu_configure_dev_id(struct device_node *master_np,
struct of_phandle_args iommu_spec = { .args_count = 1 };
Oh dear, I totally overlooked this, and off the top of my head I'm not sure it's simple to fix :(
So it's still not actually working as intended. Oh well, I did say it was untested...
Thanks,
Robin.