Peripheral Image Loader support for Qualcomm SoCs running Linux host at EL2

[PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Mukesh Ojha 1 month, 2 weeks ago

Most Qualcomm platforms feature a proprietary hypervisor (such as Gunyah
or QHEE), which typically handles IOMMU configuration. This includes
mapping memory regions and device memory resources for remote processors
by intercepting qcom_scm_pas_auth_and_reset() calls. These mappings are
later removed during teardown. Additionally, SHM bridge setup is required
to enable memory protection for both remoteproc metadata and its memory
regions.

When the hypervisor is absent, the operating system must perform these
configurations instead.

Support for handling IOMMU and SHM setup in the absence of a hypervisor
is now in place. Extend the Iris driver to enable this functionality on
platforms where IOMMU is managed by Linux (i.e., non-Gunyah, non-QHEE).

Additionally, the Iris driver must map the firmware and its required
resources to the firmware SID, which is now specified via the device tree.

Co-developed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/media/platform/qcom/iris/iris_core.c  |   9 +-
 drivers/media/platform/qcom/iris/iris_core.h  |   6 +
 .../media/platform/qcom/iris/iris_firmware.c  | 156 ++++++++++++++++--
 .../media/platform/qcom/iris/iris_firmware.h  |   2 +
 4 files changed, 155 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/qcom/iris/iris_core.c b/drivers/media/platform/qcom/iris/iris_core.c
index 0fa0a3b549a2..57417d4d7e05 100644
--- a/drivers/media/platform/qcom/iris/iris_core.c
+++ b/drivers/media/platform/qcom/iris/iris_core.c
@@ -17,6 +17,7 @@ void iris_core_deinit(struct iris_core *core)
 	mutex_lock(&core->lock);
 	iris_fw_unload(core);
 	iris_vpu_power_off(core);
+	iris_fw_deinit(core);
 	iris_hfi_queues_deinit(core);
 	core->state = IRIS_CORE_DEINIT;
 	mutex_unlock(&core->lock);
@@ -65,10 +66,14 @@ int iris_core_init(struct iris_core *core)
 	if (ret)
 		goto error_queue_deinit;
 
-	ret = iris_fw_load(core);
+	ret = iris_fw_init(core);
 	if (ret)
 		goto error_power_off;
 
+	ret = iris_fw_load(core);
+	if (ret)
+		goto error_firmware_deinit;
+
 	ret = iris_vpu_boot_firmware(core);
 	if (ret)
 		goto error_unload_fw;
@@ -83,6 +88,8 @@ int iris_core_init(struct iris_core *core)
 
 error_unload_fw:
 	iris_fw_unload(core);
+error_firmware_deinit:
+	iris_fw_deinit(core);
 error_power_off:
 	iris_vpu_power_off(core);
 error_queue_deinit:
diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h
index aeeac32a1f6d..57eeefb38f22 100644
--- a/drivers/media/platform/qcom/iris/iris_core.h
+++ b/drivers/media/platform/qcom/iris/iris_core.h
@@ -73,6 +73,12 @@ struct iris_core {
 	int					irq;
 	struct v4l2_device			v4l2_dev;
 	struct video_device			*vdev_dec;
+	bool					has_iommu;
+	struct video_firmware {
+		struct device *dev;
+		struct qcom_scm_pas_ctx *ctx;
+		struct iommu_domain *iommu_domain;
+	} fw;
 	const struct v4l2_file_operations	*iris_v4l2_file_ops;
 	const struct v4l2_ioctl_ops		*iris_v4l2_ioctl_ops;
 	const struct vb2_ops			*iris_vb2_ops;
diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
index f1b5cd56db32..e3f2fe5c9d7a 100644
--- a/drivers/media/platform/qcom/iris/iris_firmware.c
+++ b/drivers/media/platform/qcom/iris/iris_firmware.c
@@ -3,10 +3,18 @@
  * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
+#include <linux/device.h>
 #include <linux/firmware.h>
-#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/kernel.h>
+#include <linux/iommu.h>
+#include <linux/io.h>
+#include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/sizes.h>
 #include <linux/soc/qcom/mdt_loader.h>
 
 #include "iris_core.h"
@@ -17,15 +25,14 @@
 static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
 {
 	u32 pas_id = core->iris_platform_data->pas_id;
+	struct qcom_scm_pas_ctx *ctx;
 	const struct firmware *firmware = NULL;
 	struct device *dev = core->dev;
-	struct reserved_mem *rmem;
-	struct device_node *node;
-	phys_addr_t mem_phys;
-	size_t res_size;
-	ssize_t fw_size;
-	void *mem_virt;
-	int ret;
+	struct reserved_mem *rmem = NULL;
+	struct device_node *node = NULL;
+	ssize_t fw_size = 0;
+	void *mem_virt = NULL;
+	int ret = 0;
 
 	if (strlen(fw_name) >= MAX_FIRMWARE_NAME_SIZE - 4)
 		return -EINVAL;
@@ -39,36 +46,64 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
 	if (!rmem)
 		return -EINVAL;
 
-	mem_phys = rmem->base;
-	res_size = rmem->size;
+	if (core->has_iommu)
+		dev = core->fw.dev;
 
+	ctx = qcom_scm_pas_ctx_init(dev, pas_id, rmem->base, rmem->size, false);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->has_iommu = core->has_iommu;
 	ret = request_firmware(&firmware, fw_name, dev);
 	if (ret)
 		return ret;
 
 	fw_size = qcom_mdt_get_size(firmware);
-	if (fw_size < 0 || res_size < (size_t)fw_size) {
+	if (fw_size < 0 || rmem->size < (size_t)fw_size) {
 		ret = -EINVAL;
 		goto err_release_fw;
 	}
 
-	mem_virt = memremap(mem_phys, res_size, MEMREMAP_WC);
+	mem_virt = memremap(rmem->base, rmem->size, MEMREMAP_WC);
 	if (!mem_virt) {
 		ret = -ENOMEM;
 		goto err_release_fw;
 	}
 
-	ret = qcom_mdt_load(dev, firmware, fw_name,
-			    pas_id, mem_virt, mem_phys, res_size, NULL);
+	ret = qcom_mdt_pas_load(ctx, firmware, fw_name, mem_virt, NULL);
 	if (ret)
 		goto err_mem_unmap;
 
-	ret = qcom_scm_pas_auth_and_reset(pas_id);
+	if (core->has_iommu) {
+		ret = iommu_map(core->fw.iommu_domain, 0, rmem->base, rmem->size,
+				IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV, GFP_KERNEL);
+		if (ret)
+			goto err_mem_unmap;
+
+		/*
+		 * Firmware has no support for resource table for now, so, lets
+		 * pass NULL and zero for input resource table and input resource
+		 * table respectively.
+		 */
+		ret = qcom_mdt_pas_map_devmem_rscs(ctx, core->fw.iommu_domain, NULL, 0);
+		if (ret)
+			goto err_unmap_carveout;
+	}
+
+	ret = qcom_scm_pas_prepare_and_auth_reset(ctx);
 	if (ret)
-		goto err_mem_unmap;
+		goto err_unmap_devmem_rscs;
+
+	core->fw.ctx = ctx;
 
 	return ret;
 
+err_unmap_devmem_rscs:
+	if (core->has_iommu)
+		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
+err_unmap_carveout:
+	if (core->has_iommu)
+		iommu_unmap(core->fw.iommu_domain, 0, rmem->size);
 err_mem_unmap:
 	memunmap(mem_virt);
 err_release_fw:
@@ -109,10 +144,97 @@ int iris_fw_load(struct iris_core *core)
 
 int iris_fw_unload(struct iris_core *core)
 {
-	return qcom_scm_pas_shutdown(core->iris_platform_data->pas_id);
+	struct qcom_scm_pas_ctx *ctx;
+	int ret;
+
+	ctx = core->fw.ctx;
+	ret = qcom_scm_pas_shutdown(ctx->peripheral);
+	if (core->has_iommu) {
+		iommu_unmap(core->fw.iommu_domain, 0, ctx->mem_size);
+		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
+	}
+
+	return ret;
 }
 
 int iris_set_hw_state(struct iris_core *core, bool resume)
 {
 	return qcom_scm_set_remote_state(resume, 0);
 }
+
+int iris_fw_init(struct iris_core *core)
+{
+	struct platform_device_info info;
+	struct iommu_domain *iommu_dom;
+	struct platform_device *pdev;
+	struct device_node *np;
+	int ret;
+
+	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
+	if (!np)
+		return 0;
+
+	core->has_iommu = true;
+	memset(&info, 0, sizeof(info));
+	info.fwnode = &np->fwnode;
+	info.parent = core->dev;
+	info.name = np->name;
+	info.dma_mask = DMA_BIT_MASK(32);
+
+	pdev = platform_device_register_full(&info);
+	if (IS_ERR(pdev)) {
+		of_node_put(np);
+		return PTR_ERR(pdev);
+	}
+
+	pdev->dev.of_node = np;
+
+	ret = of_dma_configure(&pdev->dev, np, true);
+	if (ret) {
+		dev_err(core->dev, "failed to allocate domain\n");
+		goto err_unregister;
+	}
+
+	core->fw.dev = &pdev->dev;
+
+	iommu_dom = iommu_get_domain_for_dev(core->fw.dev);
+	if (!iommu_dom) {
+		dev_err(core->fw.dev, "Failed to get iommu domain\n");
+		ret = -EINVAL;
+		goto err_iommu_free;
+	}
+
+	ret = iommu_attach_device(iommu_dom, core->fw.dev);
+	if (ret) {
+		dev_err(core->fw.dev, "could not attach device\n");
+		goto err_iommu_free;
+	}
+
+	core->fw.iommu_domain = iommu_dom;
+
+	of_node_put(np);
+
+	return 0;
+
+err_iommu_free:
+	iommu_domain_free(iommu_dom);
+err_unregister:
+	platform_device_unregister(pdev);
+	of_node_put(np);
+	return ret;
+}
+
+void iris_fw_deinit(struct iris_core *core)
+{
+	struct iommu_domain *iommu_dom;
+
+	if (!core->has_iommu)
+		return;
+
+	iommu_dom = core->fw.iommu_domain;
+	iommu_detach_device(iommu_dom, core->fw.dev);
+	iommu_domain_free(iommu_dom);
+
+	core->fw.iommu_domain = NULL;
+	platform_device_unregister(to_platform_device(core->fw.dev));
+}
diff --git a/drivers/media/platform/qcom/iris/iris_firmware.h b/drivers/media/platform/qcom/iris/iris_firmware.h
index e833ecd34887..adde46109966 100644
--- a/drivers/media/platform/qcom/iris/iris_firmware.h
+++ b/drivers/media/platform/qcom/iris/iris_firmware.h
@@ -11,5 +11,7 @@ struct iris_core;
 int iris_fw_load(struct iris_core *core);
 int iris_fw_unload(struct iris_core *core);
 int iris_set_hw_state(struct iris_core *core, bool resume);
+int iris_fw_init(struct iris_core *core);
+void iris_fw_deinit(struct iris_core *core);
 
 #endif
-- 
2.50.1

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Krzysztof Kozlowski 1 month, 1 week ago

On 19/08/2025 18:54, Mukesh Ojha wrote:
> +int iris_fw_init(struct iris_core *core)
> +{
> +	struct platform_device_info info;
> +	struct iommu_domain *iommu_dom;
> +	struct platform_device *pdev;
> +	struct device_node *np;
> +	int ret;
> +
> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");

Undocumented ABI.

If you tested your DTS, you would notice warnings.

Best regards,
Krzysztof

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Mukesh Ojha 1 month, 1 week ago

On Fri, Aug 22, 2025 at 10:45:50AM +0200, Krzysztof Kozlowski wrote:
> On 19/08/2025 18:54, Mukesh Ojha wrote:
> > +int iris_fw_init(struct iris_core *core)
> > +{
> > +	struct platform_device_info info;
> > +	struct iommu_domain *iommu_dom;
> > +	struct platform_device *pdev;
> > +	struct device_node *np;
> > +	int ret;
> > +
> > +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> 
> Undocumented ABI.
> 
> If you tested your DTS, you would notice warnings.

qcom,venus-common.yaml is documenting video-firmware and getting included in
Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml

> 
> Best regards,
> Krzysztof
> 

-- 
-Mukesh Ojha

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Krzysztof Kozlowski 1 month, 1 week ago

On 22/08/2025 17:13, Mukesh Ojha wrote:
> On Fri, Aug 22, 2025 at 10:45:50AM +0200, Krzysztof Kozlowski wrote:
>> On 19/08/2025 18:54, Mukesh Ojha wrote:
>>> +int iris_fw_init(struct iris_core *core)
>>> +{
>>> +	struct platform_device_info info;
>>> +	struct iommu_domain *iommu_dom;
>>> +	struct platform_device *pdev;
>>> +	struct device_node *np;
>>> +	int ret;
>>> +
>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
>>
>> Undocumented ABI.
>>
>> If you tested your DTS, you would notice warnings.
> 
> qcom,venus-common.yaml is documenting video-firmware and getting included in
> Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml

Uh, why? Why does qcom keep this legacy pattern also for iris?

Best regards,
Krzysztof

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Krzysztof Kozlowski 1 month, 1 week ago

On 23/08/2025 17:41, Krzysztof Kozlowski wrote:
> On 22/08/2025 17:13, Mukesh Ojha wrote:
>> On Fri, Aug 22, 2025 at 10:45:50AM +0200, Krzysztof Kozlowski wrote:
>>> On 19/08/2025 18:54, Mukesh Ojha wrote:
>>>> +int iris_fw_init(struct iris_core *core)
>>>> +{
>>>> +	struct platform_device_info info;
>>>> +	struct iommu_domain *iommu_dom;
>>>> +	struct platform_device *pdev;
>>>> +	struct device_node *np;
>>>> +	int ret;
>>>> +
>>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
>>>
>>> Undocumented ABI.
>>>
>>> If you tested your DTS, you would notice warnings.
>>
>> qcom,venus-common.yaml is documenting video-firmware and getting included in
>> Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> 
> Uh, why? Why does qcom keep this legacy pattern also for iris?

I will remove it btw, because it is a fake device node (not for a real
device).

Best regards,
Krzysztof

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Krzysztof Kozlowski 1 month, 1 week ago

On 23/08/2025 17:46, Krzysztof Kozlowski wrote:
> On 23/08/2025 17:41, Krzysztof Kozlowski wrote:
>> On 22/08/2025 17:13, Mukesh Ojha wrote:
>>> On Fri, Aug 22, 2025 at 10:45:50AM +0200, Krzysztof Kozlowski wrote:
>>>> On 19/08/2025 18:54, Mukesh Ojha wrote:
>>>>> +int iris_fw_init(struct iris_core *core)
>>>>> +{
>>>>> +	struct platform_device_info info;
>>>>> +	struct iommu_domain *iommu_dom;
>>>>> +	struct platform_device *pdev;
>>>>> +	struct device_node *np;
>>>>> +	int ret;
>>>>> +
>>>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
>>>>
>>>> Undocumented ABI.
>>>>
>>>> If you tested your DTS, you would notice warnings.
>>>
>>> qcom,venus-common.yaml is documenting video-firmware and getting included in
>>> Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
>>
>> Uh, why? Why does qcom keep this legacy pattern also for iris?
> 
> I will remove it btw, because it is a fake device node (not for a real
> device).


So also to clarify: drop it here, because it will be undocumented.
Anyway such fake node was NAKed in other context.

Best regards,
Krzysztof

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Bryan O'Donoghue 1 month, 2 weeks ago

On 19/08/2025 17:54, Mukesh Ojha wrote:
> Most Qualcomm platforms feature a proprietary hypervisor (such as Gunyah
> or QHEE), which typically handles IOMMU configuration. This includes
> mapping memory regions and device memory resources for remote processors
> by intercepting qcom_scm_pas_auth_and_reset() calls. These mappings are
> later removed during teardown. Additionally, SHM bridge setup is required
> to enable memory protection for both remoteproc metadata and its memory
> regions.
> 
> When the hypervisor is absent, the operating system must perform these
> configurations instead.
> 
> Support for handling IOMMU and SHM setup in the absence of a hypervisor
> is now in place. Extend the Iris driver to enable this functionality on
> platforms where IOMMU is managed by Linux (i.e., non-Gunyah, non-QHEE).
> 
> Additionally, the Iris driver must map the firmware and its required
> resources to the firmware SID, which is now specified via the device tree.
> 
> Co-developed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/iris/iris_core.c  |   9 +-
>   drivers/media/platform/qcom/iris/iris_core.h  |   6 +
>   .../media/platform/qcom/iris/iris_firmware.c  | 156 ++++++++++++++++--
>   .../media/platform/qcom/iris/iris_firmware.h  |   2 +
>   4 files changed, 155 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_core.c b/drivers/media/platform/qcom/iris/iris_core.c
> index 0fa0a3b549a2..57417d4d7e05 100644
> --- a/drivers/media/platform/qcom/iris/iris_core.c
> +++ b/drivers/media/platform/qcom/iris/iris_core.c
> @@ -17,6 +17,7 @@ void iris_core_deinit(struct iris_core *core)
>   	mutex_lock(&core->lock);
>   	iris_fw_unload(core);
>   	iris_vpu_power_off(core);
> +	iris_fw_deinit(core);
>   	iris_hfi_queues_deinit(core);
>   	core->state = IRIS_CORE_DEINIT;
>   	mutex_unlock(&core->lock);
> @@ -65,10 +66,14 @@ int iris_core_init(struct iris_core *core)
>   	if (ret)
>   		goto error_queue_deinit;
>   
> -	ret = iris_fw_load(core);
> +	ret = iris_fw_init(core);
>   	if (ret)
>   		goto error_power_off;
>   
> +	ret = iris_fw_load(core);
> +	if (ret)
> +		goto error_firmware_deinit;
> +
>   	ret = iris_vpu_boot_firmware(core);
>   	if (ret)
>   		goto error_unload_fw;
> @@ -83,6 +88,8 @@ int iris_core_init(struct iris_core *core)
>   
>   error_unload_fw:
>   	iris_fw_unload(core);
> +error_firmware_deinit:
> +	iris_fw_deinit(core);
>   error_power_off:
>   	iris_vpu_power_off(core);
>   error_queue_deinit:
> diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h
> index aeeac32a1f6d..57eeefb38f22 100644
> --- a/drivers/media/platform/qcom/iris/iris_core.h
> +++ b/drivers/media/platform/qcom/iris/iris_core.h
> @@ -73,6 +73,12 @@ struct iris_core {
>   	int					irq;
>   	struct v4l2_device			v4l2_dev;
>   	struct video_device			*vdev_dec;
> +	bool					has_iommu;
> +	struct video_firmware {
> +		struct device *dev;
> +		struct qcom_scm_pas_ctx *ctx;
> +		struct iommu_domain *iommu_domain;
> +	} fw;
>   	const struct v4l2_file_operations	*iris_v4l2_file_ops;
>   	const struct v4l2_ioctl_ops		*iris_v4l2_ioctl_ops;
>   	const struct vb2_ops			*iris_vb2_ops;
> diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> index f1b5cd56db32..e3f2fe5c9d7a 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
> @@ -3,10 +3,18 @@
>    * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>    */
>   
> +#include <linux/device.h>
>   #include <linux/firmware.h>
> -#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/kernel.h>
> +#include <linux/iommu.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
>   #include <linux/of_address.h>
>   #include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/sizes.h>
>   #include <linux/soc/qcom/mdt_loader.h>
>   
>   #include "iris_core.h"
> @@ -17,15 +25,14 @@
>   static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
>   {
>   	u32 pas_id = core->iris_platform_data->pas_id;
> +	struct qcom_scm_pas_ctx *ctx;
>   	const struct firmware *firmware = NULL;
>   	struct device *dev = core->dev;
> -	struct reserved_mem *rmem;
> -	struct device_node *node;
> -	phys_addr_t mem_phys;
> -	size_t res_size;
> -	ssize_t fw_size;
> -	void *mem_virt;
> -	int ret;
> +	struct reserved_mem *rmem = NULL;
> +	struct device_node *node = NULL;
> +	ssize_t fw_size = 0;
> +	void *mem_virt = NULL;
> +	int ret = 0;
>   
>   	if (strlen(fw_name) >= MAX_FIRMWARE_NAME_SIZE - 4)
>   		return -EINVAL;
> @@ -39,36 +46,64 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
>   	if (!rmem)
>   		return -EINVAL;
>   
> -	mem_phys = rmem->base;
> -	res_size = rmem->size;
> +	if (core->has_iommu)
> +		dev = core->fw.dev;
>   
> +	ctx = qcom_scm_pas_ctx_init(dev, pas_id, rmem->base, rmem->size, false);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->has_iommu = core->has_iommu;
>   	ret = request_firmware(&firmware, fw_name, dev);
>   	if (ret)
>   		return ret;
>   
>   	fw_size = qcom_mdt_get_size(firmware);
> -	if (fw_size < 0 || res_size < (size_t)fw_size) {
> +	if (fw_size < 0 || rmem->size < (size_t)fw_size) {
>   		ret = -EINVAL;
>   		goto err_release_fw;
>   	}
>   
> -	mem_virt = memremap(mem_phys, res_size, MEMREMAP_WC);
> +	mem_virt = memremap(rmem->base, rmem->size, MEMREMAP_WC);
>   	if (!mem_virt) {
>   		ret = -ENOMEM;
>   		goto err_release_fw;
>   	}
>   
> -	ret = qcom_mdt_load(dev, firmware, fw_name,
> -			    pas_id, mem_virt, mem_phys, res_size, NULL);
> +	ret = qcom_mdt_pas_load(ctx, firmware, fw_name, mem_virt, NULL);
>   	if (ret)
>   		goto err_mem_unmap;
>   
> -	ret = qcom_scm_pas_auth_and_reset(pas_id);
> +	if (core->has_iommu) {
> +		ret = iommu_map(core->fw.iommu_domain, 0, rmem->base, rmem->size,
> +				IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV, GFP_KERNEL);
> +		if (ret)
> +			goto err_mem_unmap;
> +
> +		/*
> +		 * Firmware has no support for resource table for now, so, lets
> +		 * pass NULL and zero for input resource table and input resource
> +		 * table respectively.
> +		 */
> +		ret = qcom_mdt_pas_map_devmem_rscs(ctx, core->fw.iommu_domain, NULL, 0);
> +		if (ret)
> +			goto err_unmap_carveout;
> +	}
> +
> +	ret = qcom_scm_pas_prepare_and_auth_reset(ctx);
>   	if (ret)
> -		goto err_mem_unmap;
> +		goto err_unmap_devmem_rscs;
> +
> +	core->fw.ctx = ctx;
>   
>   	return ret;
>   
> +err_unmap_devmem_rscs:
> +	if (core->has_iommu)
> +		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
> +err_unmap_carveout:
> +	if (core->has_iommu)
> +		iommu_unmap(core->fw.iommu_domain, 0, rmem->size);
>   err_mem_unmap:
>   	memunmap(mem_virt);
>   err_release_fw:
> @@ -109,10 +144,97 @@ int iris_fw_load(struct iris_core *core)
>   
>   int iris_fw_unload(struct iris_core *core)
>   {
> -	return qcom_scm_pas_shutdown(core->iris_platform_data->pas_id);
> +	struct qcom_scm_pas_ctx *ctx;
> +	int ret;
> +
> +	ctx = core->fw.ctx;
> +	ret = qcom_scm_pas_shutdown(ctx->peripheral);
> +	if (core->has_iommu) {
> +		iommu_unmap(core->fw.iommu_domain, 0, ctx->mem_size);
> +		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
> +	}
> +
> +	return ret;
>   }
>   
>   int iris_set_hw_state(struct iris_core *core, bool resume)
>   {
>   	return qcom_scm_set_remote_state(resume, 0);
>   }
> +
> +int iris_fw_init(struct iris_core *core)
> +{
> +	struct platform_device_info info;
> +	struct iommu_domain *iommu_dom;
> +	struct platform_device *pdev;
> +	struct device_node *np;
> +	int ret;
> +
> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> +	if (!np)
> +		return 0;
> +
> +	core->has_iommu = true;

You set has_iommu = true and then you check this flag throughout this 
patch..


> +	memset(&info, 0, sizeof(info));
> +	info.fwnode = &np->fwnode;
> +	info.parent = core->dev;
> +	info.name = np->name;
> +	info.dma_mask = DMA_BIT_MASK(32);
> +
> +	pdev = platform_device_register_full(&info);
> +	if (IS_ERR(pdev)) {
> +		of_node_put(np);
> +		return PTR_ERR(pdev);
> +	}
> +
> +	pdev->dev.of_node = np;
> +
> +	ret = of_dma_configure(&pdev->dev, np, true);
> +	if (ret) {
> +		dev_err(core->dev, "failed to allocate domain\n");
> +		goto err_unregister;
> +	}
> +
> +	core->fw.dev = &pdev->dev;
> +
> +	iommu_dom = iommu_get_domain_for_dev(core->fw.dev);
> +	if (!iommu_dom) {
> +		dev_err(core->fw.dev, "Failed to get iommu domain\n");
> +		ret = -EINVAL;
> +		goto err_iommu_free;
> +	}
> +
> +	ret = iommu_attach_device(iommu_dom, core->fw.dev);
> +	if (ret) {
> +		dev_err(core->fw.dev, "could not attach device\n");
> +		goto err_iommu_free;
> +	}
> +
> +	core->fw.iommu_domain = iommu_dom;
> +
> +	of_node_put(np);
> +
> +	return 0;
> +
> +err_iommu_free:
> +	iommu_domain_free(iommu_dom);
> +err_unregister:
> +	platform_device_unregister(pdev);
> +	of_node_put(np);
> +	return ret;
> +}
> +
> +void iris_fw_deinit(struct iris_core *core)
> +{
> +	struct iommu_domain *iommu_dom;
> +
> +	if (!core->has_iommu)

This is fixed in your code is it not ?

You presumably are going to change this later otherwise suggest dropping

---
bod

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Mukesh Ojha 1 month, 2 weeks ago

On Wed, Aug 20, 2025 at 12:33:44PM +0100, Bryan O'Donoghue wrote:
> On 19/08/2025 17:54, Mukesh Ojha wrote:
> > Most Qualcomm platforms feature a proprietary hypervisor (such as Gunyah
> > or QHEE), which typically handles IOMMU configuration. This includes
> > mapping memory regions and device memory resources for remote processors
> > by intercepting qcom_scm_pas_auth_and_reset() calls. These mappings are
> > later removed during teardown. Additionally, SHM bridge setup is required
> > to enable memory protection for both remoteproc metadata and its memory
> > regions.
> > 
> > When the hypervisor is absent, the operating system must perform these
> > configurations instead.
> > 
> > Support for handling IOMMU and SHM setup in the absence of a hypervisor
> > is now in place. Extend the Iris driver to enable this functionality on
> > platforms where IOMMU is managed by Linux (i.e., non-Gunyah, non-QHEE).
> > 
> > Additionally, the Iris driver must map the firmware and its required
> > resources to the firmware SID, which is now specified via the device tree.
> > 
> > Co-developed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> > ---
> >   drivers/media/platform/qcom/iris/iris_core.c  |   9 +-
> >   drivers/media/platform/qcom/iris/iris_core.h  |   6 +
> >   .../media/platform/qcom/iris/iris_firmware.c  | 156 ++++++++++++++++--
> >   .../media/platform/qcom/iris/iris_firmware.h  |   2 +
> >   4 files changed, 155 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/platform/qcom/iris/iris_core.c b/drivers/media/platform/qcom/iris/iris_core.c
> > index 0fa0a3b549a2..57417d4d7e05 100644
> > --- a/drivers/media/platform/qcom/iris/iris_core.c
> > +++ b/drivers/media/platform/qcom/iris/iris_core.c
> > @@ -17,6 +17,7 @@ void iris_core_deinit(struct iris_core *core)
> >   	mutex_lock(&core->lock);
> >   	iris_fw_unload(core);
> >   	iris_vpu_power_off(core);
> > +	iris_fw_deinit(core);
> >   	iris_hfi_queues_deinit(core);
> >   	core->state = IRIS_CORE_DEINIT;
> >   	mutex_unlock(&core->lock);
> > @@ -65,10 +66,14 @@ int iris_core_init(struct iris_core *core)
> >   	if (ret)
> >   		goto error_queue_deinit;
> > -	ret = iris_fw_load(core);
> > +	ret = iris_fw_init(core);
> >   	if (ret)
> >   		goto error_power_off;
> > +	ret = iris_fw_load(core);
> > +	if (ret)
> > +		goto error_firmware_deinit;
> > +
> >   	ret = iris_vpu_boot_firmware(core);
> >   	if (ret)
> >   		goto error_unload_fw;
> > @@ -83,6 +88,8 @@ int iris_core_init(struct iris_core *core)
> >   error_unload_fw:
> >   	iris_fw_unload(core);
> > +error_firmware_deinit:
> > +	iris_fw_deinit(core);
> >   error_power_off:
> >   	iris_vpu_power_off(core);
> >   error_queue_deinit:
> > diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h
> > index aeeac32a1f6d..57eeefb38f22 100644
> > --- a/drivers/media/platform/qcom/iris/iris_core.h
> > +++ b/drivers/media/platform/qcom/iris/iris_core.h
> > @@ -73,6 +73,12 @@ struct iris_core {
> >   	int					irq;
> >   	struct v4l2_device			v4l2_dev;
> >   	struct video_device			*vdev_dec;
> > +	bool					has_iommu;
> > +	struct video_firmware {
> > +		struct device *dev;
> > +		struct qcom_scm_pas_ctx *ctx;
> > +		struct iommu_domain *iommu_domain;
> > +	} fw;
> >   	const struct v4l2_file_operations	*iris_v4l2_file_ops;
> >   	const struct v4l2_ioctl_ops		*iris_v4l2_ioctl_ops;
> >   	const struct vb2_ops			*iris_vb2_ops;
> > diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> > index f1b5cd56db32..e3f2fe5c9d7a 100644
> > --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> > +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
> > @@ -3,10 +3,18 @@
> >    * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> >    */
> > +#include <linux/device.h>
> >   #include <linux/firmware.h>
> > -#include <linux/firmware/qcom/qcom_scm.h>
> > +#include <linux/kernel.h>
> > +#include <linux/iommu.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> >   #include <linux/of_address.h>
> >   #include <linux/of_reserved_mem.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of_device.h>
> > +#include <linux/firmware/qcom/qcom_scm.h>
> > +#include <linux/sizes.h>
> >   #include <linux/soc/qcom/mdt_loader.h>
> >   #include "iris_core.h"
> > @@ -17,15 +25,14 @@
> >   static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> >   {
> >   	u32 pas_id = core->iris_platform_data->pas_id;
> > +	struct qcom_scm_pas_ctx *ctx;
> >   	const struct firmware *firmware = NULL;
> >   	struct device *dev = core->dev;
> > -	struct reserved_mem *rmem;
> > -	struct device_node *node;
> > -	phys_addr_t mem_phys;
> > -	size_t res_size;
> > -	ssize_t fw_size;
> > -	void *mem_virt;
> > -	int ret;
> > +	struct reserved_mem *rmem = NULL;
> > +	struct device_node *node = NULL;
> > +	ssize_t fw_size = 0;
> > +	void *mem_virt = NULL;
> > +	int ret = 0;
> >   	if (strlen(fw_name) >= MAX_FIRMWARE_NAME_SIZE - 4)
> >   		return -EINVAL;
> > @@ -39,36 +46,64 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> >   	if (!rmem)
> >   		return -EINVAL;
> > -	mem_phys = rmem->base;
> > -	res_size = rmem->size;
> > +	if (core->has_iommu)
> > +		dev = core->fw.dev;
> > +	ctx = qcom_scm_pas_ctx_init(dev, pas_id, rmem->base, rmem->size, false);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +
> > +	ctx->has_iommu = core->has_iommu;
> >   	ret = request_firmware(&firmware, fw_name, dev);
> >   	if (ret)
> >   		return ret;
> >   	fw_size = qcom_mdt_get_size(firmware);
> > -	if (fw_size < 0 || res_size < (size_t)fw_size) {
> > +	if (fw_size < 0 || rmem->size < (size_t)fw_size) {
> >   		ret = -EINVAL;
> >   		goto err_release_fw;
> >   	}
> > -	mem_virt = memremap(mem_phys, res_size, MEMREMAP_WC);
> > +	mem_virt = memremap(rmem->base, rmem->size, MEMREMAP_WC);
> >   	if (!mem_virt) {
> >   		ret = -ENOMEM;
> >   		goto err_release_fw;
> >   	}
> > -	ret = qcom_mdt_load(dev, firmware, fw_name,
> > -			    pas_id, mem_virt, mem_phys, res_size, NULL);
> > +	ret = qcom_mdt_pas_load(ctx, firmware, fw_name, mem_virt, NULL);
> >   	if (ret)
> >   		goto err_mem_unmap;
> > -	ret = qcom_scm_pas_auth_and_reset(pas_id);
> > +	if (core->has_iommu) {
> > +		ret = iommu_map(core->fw.iommu_domain, 0, rmem->base, rmem->size,
> > +				IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV, GFP_KERNEL);
> > +		if (ret)
> > +			goto err_mem_unmap;
> > +
> > +		/*
> > +		 * Firmware has no support for resource table for now, so, lets
> > +		 * pass NULL and zero for input resource table and input resource
> > +		 * table respectively.
> > +		 */
> > +		ret = qcom_mdt_pas_map_devmem_rscs(ctx, core->fw.iommu_domain, NULL, 0);
> > +		if (ret)
> > +			goto err_unmap_carveout;
> > +	}
> > +
> > +	ret = qcom_scm_pas_prepare_and_auth_reset(ctx);
> >   	if (ret)
> > -		goto err_mem_unmap;
> > +		goto err_unmap_devmem_rscs;
> > +
> > +	core->fw.ctx = ctx;
> >   	return ret;
> > +err_unmap_devmem_rscs:
> > +	if (core->has_iommu)
> > +		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
> > +err_unmap_carveout:
> > +	if (core->has_iommu)
> > +		iommu_unmap(core->fw.iommu_domain, 0, rmem->size);
> >   err_mem_unmap:
> >   	memunmap(mem_virt);
> >   err_release_fw:
> > @@ -109,10 +144,97 @@ int iris_fw_load(struct iris_core *core)
> >   int iris_fw_unload(struct iris_core *core)
> >   {
> > -	return qcom_scm_pas_shutdown(core->iris_platform_data->pas_id);
> > +	struct qcom_scm_pas_ctx *ctx;
> > +	int ret;
> > +
> > +	ctx = core->fw.ctx;
> > +	ret = qcom_scm_pas_shutdown(ctx->peripheral);
> > +	if (core->has_iommu) {
> > +		iommu_unmap(core->fw.iommu_domain, 0, ctx->mem_size);
> > +		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
> > +	}
> > +
> > +	return ret;
> >   }
> >   int iris_set_hw_state(struct iris_core *core, bool resume)
> >   {
> >   	return qcom_scm_set_remote_state(resume, 0);
> >   }
> > +
> > +int iris_fw_init(struct iris_core *core)
> > +{
> > +	struct platform_device_info info;
> > +	struct iommu_domain *iommu_dom;
> > +	struct platform_device *pdev;
> > +	struct device_node *np;
> > +	int ret;
> > +
> > +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > +	if (!np)
> > +		return 0;
> > +
> > +	core->has_iommu = true;
> 
> You set has_iommu = true and then you check this flag throughout this
> patch..
> 
> 
> > +	memset(&info, 0, sizeof(info));
> > +	info.fwnode = &np->fwnode;
> > +	info.parent = core->dev;
> > +	info.name = np->name;
> > +	info.dma_mask = DMA_BIT_MASK(32);
> > +
> > +	pdev = platform_device_register_full(&info);
> > +	if (IS_ERR(pdev)) {
> > +		of_node_put(np);
> > +		return PTR_ERR(pdev);
> > +	}
> > +
> > +	pdev->dev.of_node = np;
> > +
> > +	ret = of_dma_configure(&pdev->dev, np, true);
> > +	if (ret) {
> > +		dev_err(core->dev, "failed to allocate domain\n");
> > +		goto err_unregister;
> > +	}
> > +
> > +	core->fw.dev = &pdev->dev;
> > +
> > +	iommu_dom = iommu_get_domain_for_dev(core->fw.dev);
> > +	if (!iommu_dom) {
> > +		dev_err(core->fw.dev, "Failed to get iommu domain\n");
> > +		ret = -EINVAL;
> > +		goto err_iommu_free;
> > +	}
> > +
> > +	ret = iommu_attach_device(iommu_dom, core->fw.dev);
> > +	if (ret) {
> > +		dev_err(core->fw.dev, "could not attach device\n");
> > +		goto err_iommu_free;
> > +	}
> > +
> > +	core->fw.iommu_domain = iommu_dom;
> > +
> > +	of_node_put(np);
> > +
> > +	return 0;
> > +
> > +err_iommu_free:
> > +	iommu_domain_free(iommu_dom);
> > +err_unregister:
> > +	platform_device_unregister(pdev);
> > +	of_node_put(np);
> > +	return ret;
> > +}
> > +
> > +void iris_fw_deinit(struct iris_core *core)
> > +{
> > +	struct iommu_domain *iommu_dom;
> > +
> > +	if (!core->has_iommu)
> 
> This is fixed in your code is it not ?
> 
> You presumably are going to change this later otherwise suggest dropping

Sure.

> 
> ---
> bod

-- 
-Mukesh Ojha

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Stephan Gerhold 1 month, 2 weeks ago

On Tue, Aug 19, 2025 at 10:24:46PM +0530, Mukesh Ojha wrote:
> Most Qualcomm platforms feature a proprietary hypervisor (such as Gunyah
> or QHEE), which typically handles IOMMU configuration. This includes
> mapping memory regions and device memory resources for remote processors
> by intercepting qcom_scm_pas_auth_and_reset() calls. These mappings are
> later removed during teardown. Additionally, SHM bridge setup is required
> to enable memory protection for both remoteproc metadata and its memory
> regions.
> 
> When the hypervisor is absent, the operating system must perform these
> configurations instead.
> 
> Support for handling IOMMU and SHM setup in the absence of a hypervisor
> is now in place. Extend the Iris driver to enable this functionality on
> platforms where IOMMU is managed by Linux (i.e., non-Gunyah, non-QHEE).
> 
> Additionally, the Iris driver must map the firmware and its required
> resources to the firmware SID, which is now specified via the device tree.
> 
> Co-developed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
>  drivers/media/platform/qcom/iris/iris_core.c  |   9 +-
>  drivers/media/platform/qcom/iris/iris_core.h  |   6 +
>  .../media/platform/qcom/iris/iris_firmware.c  | 156 ++++++++++++++++--
>  .../media/platform/qcom/iris/iris_firmware.h  |   2 +
>  4 files changed, 155 insertions(+), 18 deletions(-)
> 
> [...]
> diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> index f1b5cd56db32..e3f2fe5c9d7a 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
> @@ -3,10 +3,18 @@
>   * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
> +#include <linux/device.h>
>  #include <linux/firmware.h>
> -#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/kernel.h>
> +#include <linux/iommu.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/sizes.h>
>  #include <linux/soc/qcom/mdt_loader.h>
>  
>  #include "iris_core.h"
> @@ -17,15 +25,14 @@
>  static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
>  {
>  	u32 pas_id = core->iris_platform_data->pas_id;
> +	struct qcom_scm_pas_ctx *ctx;
>  	const struct firmware *firmware = NULL;
>  	struct device *dev = core->dev;
> -	struct reserved_mem *rmem;
> -	struct device_node *node;
> -	phys_addr_t mem_phys;
> -	size_t res_size;
> -	ssize_t fw_size;
> -	void *mem_virt;
> -	int ret;
> +	struct reserved_mem *rmem = NULL;
> +	struct device_node *node = NULL;
> +	ssize_t fw_size = 0;
> +	void *mem_virt = NULL;
> +	int ret = 0;
>  
>  	if (strlen(fw_name) >= MAX_FIRMWARE_NAME_SIZE - 4)
>  		return -EINVAL;
> @@ -39,36 +46,64 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
>  	if (!rmem)
>  		return -EINVAL;
>  
> -	mem_phys = rmem->base;
> -	res_size = rmem->size;
> +	if (core->has_iommu)
> +		dev = core->fw.dev;
>  
> +	ctx = qcom_scm_pas_ctx_init(dev, pas_id, rmem->base, rmem->size, false);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->has_iommu = core->has_iommu;
>  	ret = request_firmware(&firmware, fw_name, dev);
>  	if (ret)
>  		return ret;
>  
>  	fw_size = qcom_mdt_get_size(firmware);
> -	if (fw_size < 0 || res_size < (size_t)fw_size) {
> +	if (fw_size < 0 || rmem->size < (size_t)fw_size) {
>  		ret = -EINVAL;
>  		goto err_release_fw;
>  	}
>  
> -	mem_virt = memremap(mem_phys, res_size, MEMREMAP_WC);
> +	mem_virt = memremap(rmem->base, rmem->size, MEMREMAP_WC);
>  	if (!mem_virt) {
>  		ret = -ENOMEM;
>  		goto err_release_fw;
>  	}
>  
> -	ret = qcom_mdt_load(dev, firmware, fw_name,
> -			    pas_id, mem_virt, mem_phys, res_size, NULL);
> +	ret = qcom_mdt_pas_load(ctx, firmware, fw_name, mem_virt, NULL);
>  	if (ret)
>  		goto err_mem_unmap;
>  
> -	ret = qcom_scm_pas_auth_and_reset(pas_id);
> +	if (core->has_iommu) {
> +		ret = iommu_map(core->fw.iommu_domain, 0, rmem->base, rmem->size,
> +				IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV, GFP_KERNEL);

What is the use case for IOMMU_PRIV here? You don't have this flag for
the qcom_q6v5_pas change.

> +		if (ret)
> +			goto err_mem_unmap;
> +
> +		/*
> +		 * Firmware has no support for resource table for now, so, lets
> +		 * pass NULL and zero for input resource table and input resource
> +		 * table respectively.
> +		 */
> +		ret = qcom_mdt_pas_map_devmem_rscs(ctx, core->fw.iommu_domain, NULL, 0);
> +		if (ret)
> +			goto err_unmap_carveout;
> +	}
> +
> +	ret = qcom_scm_pas_prepare_and_auth_reset(ctx);
>  	if (ret)
> -		goto err_mem_unmap;
> +		goto err_unmap_devmem_rscs;
> +
> +	core->fw.ctx = ctx;
>  
>  	return ret;
>  
> +err_unmap_devmem_rscs:
> +	if (core->has_iommu)
> +		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
> +err_unmap_carveout:
> +	if (core->has_iommu)
> +		iommu_unmap(core->fw.iommu_domain, 0, rmem->size);
>  err_mem_unmap:
>  	memunmap(mem_virt);
>  err_release_fw:
> @@ -109,10 +144,97 @@ int iris_fw_load(struct iris_core *core)
>  
>  int iris_fw_unload(struct iris_core *core)
>  {
> -	return qcom_scm_pas_shutdown(core->iris_platform_data->pas_id);
> +	struct qcom_scm_pas_ctx *ctx;
> +	int ret;
> +
> +	ctx = core->fw.ctx;
> +	ret = qcom_scm_pas_shutdown(ctx->peripheral);
> +	if (core->has_iommu) {
> +		iommu_unmap(core->fw.iommu_domain, 0, ctx->mem_size);
> +		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
> +	}
> +
> +	return ret;
>  }
>  
>  int iris_set_hw_state(struct iris_core *core, bool resume)
>  {
>  	return qcom_scm_set_remote_state(resume, 0);
>  }
> +
> +int iris_fw_init(struct iris_core *core)
> +{
> +	struct platform_device_info info;
> +	struct iommu_domain *iommu_dom;
> +	struct platform_device *pdev;
> +	struct device_node *np;
> +	int ret;
> +
> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> +	if (!np)
> +		return 0;

You need a dt-bindings change for this as well. This is documented only
for Venus.

Thanks,
Stephan

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Mukesh Ojha 1 month, 2 weeks ago

On Wed, Aug 20, 2025 at 10:46:31AM +0200, Stephan Gerhold wrote:
> On Tue, Aug 19, 2025 at 10:24:46PM +0530, Mukesh Ojha wrote:
> > Most Qualcomm platforms feature a proprietary hypervisor (such as Gunyah
> > or QHEE), which typically handles IOMMU configuration. This includes
> > mapping memory regions and device memory resources for remote processors
> > by intercepting qcom_scm_pas_auth_and_reset() calls. These mappings are
> > later removed during teardown. Additionally, SHM bridge setup is required
> > to enable memory protection for both remoteproc metadata and its memory
> > regions.
> > 
> > When the hypervisor is absent, the operating system must perform these
> > configurations instead.
> > 
> > Support for handling IOMMU and SHM setup in the absence of a hypervisor
> > is now in place. Extend the Iris driver to enable this functionality on
> > platforms where IOMMU is managed by Linux (i.e., non-Gunyah, non-QHEE).
> > 
> > Additionally, the Iris driver must map the firmware and its required
> > resources to the firmware SID, which is now specified via the device tree.
> > 
> > Co-developed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> > ---
> >  drivers/media/platform/qcom/iris/iris_core.c  |   9 +-
> >  drivers/media/platform/qcom/iris/iris_core.h  |   6 +
> >  .../media/platform/qcom/iris/iris_firmware.c  | 156 ++++++++++++++++--
> >  .../media/platform/qcom/iris/iris_firmware.h  |   2 +
> >  4 files changed, 155 insertions(+), 18 deletions(-)
> > 
> > [...]
> > diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> > index f1b5cd56db32..e3f2fe5c9d7a 100644
> > --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> > +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
> > @@ -3,10 +3,18 @@
> >   * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> >   */
> >  
> > +#include <linux/device.h>
> >  #include <linux/firmware.h>
> > -#include <linux/firmware/qcom/qcom_scm.h>
> > +#include <linux/kernel.h>
> > +#include <linux/iommu.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of_reserved_mem.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of_device.h>
> > +#include <linux/firmware/qcom/qcom_scm.h>
> > +#include <linux/sizes.h>
> >  #include <linux/soc/qcom/mdt_loader.h>
> >  
> >  #include "iris_core.h"
> > @@ -17,15 +25,14 @@
> >  static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> >  {
> >  	u32 pas_id = core->iris_platform_data->pas_id;
> > +	struct qcom_scm_pas_ctx *ctx;
> >  	const struct firmware *firmware = NULL;
> >  	struct device *dev = core->dev;
> > -	struct reserved_mem *rmem;
> > -	struct device_node *node;
> > -	phys_addr_t mem_phys;
> > -	size_t res_size;
> > -	ssize_t fw_size;
> > -	void *mem_virt;
> > -	int ret;
> > +	struct reserved_mem *rmem = NULL;
> > +	struct device_node *node = NULL;
> > +	ssize_t fw_size = 0;
> > +	void *mem_virt = NULL;
> > +	int ret = 0;
> >  
> >  	if (strlen(fw_name) >= MAX_FIRMWARE_NAME_SIZE - 4)
> >  		return -EINVAL;
> > @@ -39,36 +46,64 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> >  	if (!rmem)
> >  		return -EINVAL;
> >  
> > -	mem_phys = rmem->base;
> > -	res_size = rmem->size;
> > +	if (core->has_iommu)
> > +		dev = core->fw.dev;
> >  
> > +	ctx = qcom_scm_pas_ctx_init(dev, pas_id, rmem->base, rmem->size, false);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +
> > +	ctx->has_iommu = core->has_iommu;
> >  	ret = request_firmware(&firmware, fw_name, dev);
> >  	if (ret)
> >  		return ret;
> >  
> >  	fw_size = qcom_mdt_get_size(firmware);
> > -	if (fw_size < 0 || res_size < (size_t)fw_size) {
> > +	if (fw_size < 0 || rmem->size < (size_t)fw_size) {
> >  		ret = -EINVAL;
> >  		goto err_release_fw;
> >  	}
> >  
> > -	mem_virt = memremap(mem_phys, res_size, MEMREMAP_WC);
> > +	mem_virt = memremap(rmem->base, rmem->size, MEMREMAP_WC);
> >  	if (!mem_virt) {
> >  		ret = -ENOMEM;
> >  		goto err_release_fw;
> >  	}
> >  
> > -	ret = qcom_mdt_load(dev, firmware, fw_name,
> > -			    pas_id, mem_virt, mem_phys, res_size, NULL);
> > +	ret = qcom_mdt_pas_load(ctx, firmware, fw_name, mem_virt, NULL);
> >  	if (ret)
> >  		goto err_mem_unmap;
> >  
> > -	ret = qcom_scm_pas_auth_and_reset(pas_id);
> > +	if (core->has_iommu) {
> > +		ret = iommu_map(core->fw.iommu_domain, 0, rmem->base, rmem->size,
> > +				IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV, GFP_KERNEL);
> 
> What is the use case for IOMMU_PRIV here? You don't have this flag for
> the qcom_q6v5_pas change.

This is there for historic regions, I may not have complete information about why
is it required but the reference is taken from venus support for chrome.

 
> > +		if (ret)
> > +			goto err_mem_unmap;
> > +
> > +		/*
> > +		 * Firmware has no support for resource table for now, so, lets
> > +		 * pass NULL and zero for input resource table and input resource
> > +		 * table respectively.
> > +		 */
> > +		ret = qcom_mdt_pas_map_devmem_rscs(ctx, core->fw.iommu_domain, NULL, 0);
> > +		if (ret)
> > +			goto err_unmap_carveout;
> > +	}
> > +
> > +	ret = qcom_scm_pas_prepare_and_auth_reset(ctx);
> >  	if (ret)
> > -		goto err_mem_unmap;
> > +		goto err_unmap_devmem_rscs;
> > +
> > +	core->fw.ctx = ctx;
> >  
> >  	return ret;
> >  
> > +err_unmap_devmem_rscs:
> > +	if (core->has_iommu)
> > +		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
> > +err_unmap_carveout:
> > +	if (core->has_iommu)
> > +		iommu_unmap(core->fw.iommu_domain, 0, rmem->size);
> >  err_mem_unmap:
> >  	memunmap(mem_virt);
> >  err_release_fw:
> > @@ -109,10 +144,97 @@ int iris_fw_load(struct iris_core *core)
> >  
> >  int iris_fw_unload(struct iris_core *core)
> >  {
> > -	return qcom_scm_pas_shutdown(core->iris_platform_data->pas_id);
> > +	struct qcom_scm_pas_ctx *ctx;
> > +	int ret;
> > +
> > +	ctx = core->fw.ctx;
> > +	ret = qcom_scm_pas_shutdown(ctx->peripheral);
> > +	if (core->has_iommu) {
> > +		iommu_unmap(core->fw.iommu_domain, 0, ctx->mem_size);
> > +		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
> > +	}
> > +
> > +	return ret;
> >  }
> >  
> >  int iris_set_hw_state(struct iris_core *core, bool resume)
> >  {
> >  	return qcom_scm_set_remote_state(resume, 0);
> >  }
> > +
> > +int iris_fw_init(struct iris_core *core)
> > +{
> > +	struct platform_device_info info;
> > +	struct iommu_domain *iommu_dom;
> > +	struct platform_device *pdev;
> > +	struct device_node *np;
> > +	int ret;
> > +
> > +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > +	if (!np)
> > +		return 0;
> 
> You need a dt-bindings change for this as well. This is documented only
> for Venus.

You are right, wanted to send device tree and binding support separately.
But if required, will add with the series in the next version.

> 
> Thanks,
> Stephan

-- 
-Mukesh Ojha

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Stephan Gerhold 1 month, 2 weeks ago

On Wed, Aug 20, 2025 at 05:26:59PM +0530, Mukesh Ojha wrote:
> On Wed, Aug 20, 2025 at 10:46:31AM +0200, Stephan Gerhold wrote:
> > On Tue, Aug 19, 2025 at 10:24:46PM +0530, Mukesh Ojha wrote:
> > > Most Qualcomm platforms feature a proprietary hypervisor (such as Gunyah
> > > or QHEE), which typically handles IOMMU configuration. This includes
> > > mapping memory regions and device memory resources for remote processors
> > > by intercepting qcom_scm_pas_auth_and_reset() calls. These mappings are
> > > later removed during teardown. Additionally, SHM bridge setup is required
> > > to enable memory protection for both remoteproc metadata and its memory
> > > regions.
> > > 
> > > When the hypervisor is absent, the operating system must perform these
> > > configurations instead.
> > > 
> > > Support for handling IOMMU and SHM setup in the absence of a hypervisor
> > > is now in place. Extend the Iris driver to enable this functionality on
> > > platforms where IOMMU is managed by Linux (i.e., non-Gunyah, non-QHEE).
> > > 
> > > Additionally, the Iris driver must map the firmware and its required
> > > resources to the firmware SID, which is now specified via the device tree.
> > > 
> > > Co-developed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> > > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> > > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> > > ---
> > >  drivers/media/platform/qcom/iris/iris_core.c  |   9 +-
> > >  drivers/media/platform/qcom/iris/iris_core.h  |   6 +
> > >  .../media/platform/qcom/iris/iris_firmware.c  | 156 ++++++++++++++++--
> > >  .../media/platform/qcom/iris/iris_firmware.h  |   2 +
> > >  4 files changed, 155 insertions(+), 18 deletions(-)
> > > 
> > > [...]
> > > diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> > > index f1b5cd56db32..e3f2fe5c9d7a 100644
> > > --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> > > +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
> > > @@ -3,10 +3,18 @@
> > >   * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > >   */
> > >  
> > > +#include <linux/device.h>
> > >  #include <linux/firmware.h>
> > > -#include <linux/firmware/qcom/qcom_scm.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/iommu.h>
> > > +#include <linux/io.h>
> > > +#include <linux/of.h>
> > >  #include <linux/of_address.h>
> > >  #include <linux/of_reserved_mem.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/firmware/qcom/qcom_scm.h>
> > > +#include <linux/sizes.h>
> > >  #include <linux/soc/qcom/mdt_loader.h>
> > >  
> > >  #include "iris_core.h"
> > > @@ -17,15 +25,14 @@
> > >  static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> > >  {
> > >  	u32 pas_id = core->iris_platform_data->pas_id;
> > > +	struct qcom_scm_pas_ctx *ctx;
> > >  	const struct firmware *firmware = NULL;
> > >  	struct device *dev = core->dev;
> > > -	struct reserved_mem *rmem;
> > > -	struct device_node *node;
> > > -	phys_addr_t mem_phys;
> > > -	size_t res_size;
> > > -	ssize_t fw_size;
> > > -	void *mem_virt;
> > > -	int ret;
> > > +	struct reserved_mem *rmem = NULL;
> > > +	struct device_node *node = NULL;
> > > +	ssize_t fw_size = 0;
> > > +	void *mem_virt = NULL;
> > > +	int ret = 0;
> > >  
> > >  	if (strlen(fw_name) >= MAX_FIRMWARE_NAME_SIZE - 4)
> > >  		return -EINVAL;
> > > @@ -39,36 +46,64 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> > >  	if (!rmem)
> > >  		return -EINVAL;
> > >  
> > > -	mem_phys = rmem->base;
> > > -	res_size = rmem->size;
> > > +	if (core->has_iommu)
> > > +		dev = core->fw.dev;
> > >  
> > > +	ctx = qcom_scm_pas_ctx_init(dev, pas_id, rmem->base, rmem->size, false);
> > > +	if (!ctx)
> > > +		return -ENOMEM;
> > > +
> > > +	ctx->has_iommu = core->has_iommu;
> > >  	ret = request_firmware(&firmware, fw_name, dev);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > >  	fw_size = qcom_mdt_get_size(firmware);
> > > -	if (fw_size < 0 || res_size < (size_t)fw_size) {
> > > +	if (fw_size < 0 || rmem->size < (size_t)fw_size) {
> > >  		ret = -EINVAL;
> > >  		goto err_release_fw;
> > >  	}
> > >  
> > > -	mem_virt = memremap(mem_phys, res_size, MEMREMAP_WC);
> > > +	mem_virt = memremap(rmem->base, rmem->size, MEMREMAP_WC);
> > >  	if (!mem_virt) {
> > >  		ret = -ENOMEM;
> > >  		goto err_release_fw;
> > >  	}
> > >  
> > > -	ret = qcom_mdt_load(dev, firmware, fw_name,
> > > -			    pas_id, mem_virt, mem_phys, res_size, NULL);
> > > +	ret = qcom_mdt_pas_load(ctx, firmware, fw_name, mem_virt, NULL);
> > >  	if (ret)
> > >  		goto err_mem_unmap;
> > >  
> > > -	ret = qcom_scm_pas_auth_and_reset(pas_id);
> > > +	if (core->has_iommu) {
> > > +		ret = iommu_map(core->fw.iommu_domain, 0, rmem->base, rmem->size,
> > > +				IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV, GFP_KERNEL);
> > 
> > What is the use case for IOMMU_PRIV here? You don't have this flag for
> > the qcom_q6v5_pas change.
> 
> This is there for historic regions, I may not have complete information about why
> is it required but the reference is taken from venus support for chrome.
> 

Setting IOMMU_PRIV results in omitting the ARM_LPAE_PTE_AP_UNPRIV bit in
the IOMMU page tables - have you checked if QHEE sets this? Ideally we
want to do the same QHEE would normally do.

Also, please add a define for the 0 numbere here similar to

#define VENUS_FW_START_ADDR		0x0

It's quite hard to see that this is not an identity-mapping like for
qcom_q6v5_pas.

>  
> > > +		if (ret)
> > > +			goto err_mem_unmap;
> > > +
> > > +		/*
> > > +		 * Firmware has no support for resource table for now, so, lets
> > > +		 * pass NULL and zero for input resource table and input resource
> > > +		 * table respectively.
> > > +		 */
> > > +		ret = qcom_mdt_pas_map_devmem_rscs(ctx, core->fw.iommu_domain, NULL, 0);
> > > +		if (ret)
> > > +			goto err_unmap_carveout;
> > > +	}
> > > +
> > > +	ret = qcom_scm_pas_prepare_and_auth_reset(ctx);
> > >  	if (ret)
> > > -		goto err_mem_unmap;
> > > +		goto err_unmap_devmem_rscs;
> > > +
> > > +	core->fw.ctx = ctx;
> > >  
> > >  	return ret;
> > >  
> > > +err_unmap_devmem_rscs:
> > > +	if (core->has_iommu)
> > > +		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
> > > +err_unmap_carveout:
> > > +	if (core->has_iommu)
> > > +		iommu_unmap(core->fw.iommu_domain, 0, rmem->size);
> > >  err_mem_unmap:
> > >  	memunmap(mem_virt);
> > >  err_release_fw:
> > > @@ -109,10 +144,97 @@ int iris_fw_load(struct iris_core *core)
> > >  
> > >  int iris_fw_unload(struct iris_core *core)
> > >  {
> > > -	return qcom_scm_pas_shutdown(core->iris_platform_data->pas_id);
> > > +	struct qcom_scm_pas_ctx *ctx;
> > > +	int ret;
> > > +
> > > +	ctx = core->fw.ctx;
> > > +	ret = qcom_scm_pas_shutdown(ctx->peripheral);
> > > +	if (core->has_iommu) {
> > > +		iommu_unmap(core->fw.iommu_domain, 0, ctx->mem_size);
> > > +		qcom_mdt_pas_unmap_devmem_rscs(ctx, core->fw.iommu_domain);
> > > +	}
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  int iris_set_hw_state(struct iris_core *core, bool resume)
> > >  {
> > >  	return qcom_scm_set_remote_state(resume, 0);
> > >  }
> > > +
> > > +int iris_fw_init(struct iris_core *core)
> > > +{
> > > +	struct platform_device_info info;
> > > +	struct iommu_domain *iommu_dom;
> > > +	struct platform_device *pdev;
> > > +	struct device_node *np;
> > > +	int ret;
> > > +
> > > +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > > +	if (!np)
> > > +		return 0;
> > 
> > You need a dt-bindings change for this as well. This is documented only
> > for Venus.
> 
> You are right, wanted to send device tree and binding support separately.
> But if required, will add with the series in the next version.
> 

You can send device tree changes separately, but dt-binding changes
always need to come before the driver changes.

Thanks,
Stephan

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Vikash Garodia 1 month, 1 week ago

On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
>>>> +int iris_fw_init(struct iris_core *core)
>>>> +{
>>>> +	struct platform_device_info info;
>>>> +	struct iommu_domain *iommu_dom;
>>>> +	struct platform_device *pdev;
>>>> +	struct device_node *np;
>>>> +	int ret;
>>>> +
>>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
>>>> +	if (!np)
>>>> +		return 0;
>>> You need a dt-bindings change for this as well. This is documented only
>>> for Venus.
>> You are right, wanted to send device tree and binding support separately.
>> But if required, will add with the series in the next version.
>>
> You can send device tree changes separately, but dt-binding changes
> always need to come before the driver changes.

Do you mean to update the examples section[1] with the firmware subnode,
something similar to venus schema[2] ?

Regards,
Vikash

[1]
https://elixir.bootlin.com/linux/v6.17-rc2/source/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml#L109

[2]
https://elixir.bootlin.com/linux/v6.17-rc2/source/Documentation/devicetree/bindings/media/qcom,sc7280-venus.yaml#L128

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Stephan Gerhold 1 month, 1 week ago

On Fri, Aug 22, 2025 at 09:56:49AM +0530, Vikash Garodia wrote:
> On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
> >>>> +int iris_fw_init(struct iris_core *core)
> >>>> +{
> >>>> +	struct platform_device_info info;
> >>>> +	struct iommu_domain *iommu_dom;
> >>>> +	struct platform_device *pdev;
> >>>> +	struct device_node *np;
> >>>> +	int ret;
> >>>> +
> >>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> >>>> +	if (!np)
> >>>> +		return 0;
> >>> You need a dt-bindings change for this as well. This is documented only
> >>> for Venus.
> >> You are right, wanted to send device tree and binding support separately.
> >> But if required, will add with the series in the next version.
> >>
> > You can send device tree changes separately, but dt-binding changes
> > always need to come before the driver changes.
> 
> Do you mean to update the examples section[1] with the firmware subnode,
> something similar to venus schema[2] ?
> 

Sorry, I missed the fact that the "video-firmware" subnode is already
documented for iris as well through qcom,venus-common.yaml (which is
included for qcom,sm8550-iris). I don't think it's strictly required to
add every possibility to the examples of the schema, since we'll also
have the actual DTBs later to test this part of the schema.

I would recommend to extend the description of the "video-firmware" node
in qcom,venus-common.yaml a bit. You do use the reset functionality of
TrustZone, so the description there doesn't fit for your use case.

I think we will also have to figure out how to handle the old
"ChromeOS"/"non_tz" use case (that resets Iris directly with the
registers) vs the EL2 PAS use case (that resets Iris in TZ but still
handles IOMMU from Linux). Simply checking for the presence of the
"video-firmware" node is not enough, because that doesn't tell us if the
PAS support is present in TZ.

I have been experimenting with a similar patch that copies the "non_tz"
code paths from Venus into Iris. We need this to upstream the Iris DT
patch for X1E without regressing the community-contributed x1-el2.dtso,
which doesn't have functional PAS when running in EL2.

Perhaps we could check for __qcom_scm_is_call_available() with the new
QCOM_SCM_PIL_PAS_GET_RSCTABLE to choose between invoking reset via PAS
or directly with the registers. I don't have a device with the new
firmware to verify if that works.

I'll try to send out my patch soon, so you can better see the context.

Thanks,
Stephan

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Mukesh Ojha 1 month, 1 week ago

On Fri, Aug 22, 2025 at 10:46:20AM +0200, Stephan Gerhold wrote:
> On Fri, Aug 22, 2025 at 09:56:49AM +0530, Vikash Garodia wrote:
> > On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
> > >>>> +int iris_fw_init(struct iris_core *core)
> > >>>> +{
> > >>>> +	struct platform_device_info info;
> > >>>> +	struct iommu_domain *iommu_dom;
> > >>>> +	struct platform_device *pdev;
> > >>>> +	struct device_node *np;
> > >>>> +	int ret;
> > >>>> +
> > >>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > >>>> +	if (!np)
> > >>>> +		return 0;
> > >>> You need a dt-bindings change for this as well. This is documented only
> > >>> for Venus.
> > >> You are right, wanted to send device tree and binding support separately.
> > >> But if required, will add with the series in the next version.
> > >>
> > > You can send device tree changes separately, but dt-binding changes
> > > always need to come before the driver changes.
> > 
> > Do you mean to update the examples section[1] with the firmware subnode,
> > something similar to venus schema[2] ?
> > 
> 
> Sorry, I missed the fact that the "video-firmware" subnode is already
> documented for iris as well through qcom,venus-common.yaml (which is
> included for qcom,sm8550-iris). I don't think it's strictly required to
> add every possibility to the examples of the schema, since we'll also
> have the actual DTBs later to test this part of the schema.
> 
> I would recommend to extend the description of the "video-firmware" node
> in qcom,venus-common.yaml a bit. You do use the reset functionality of
> TrustZone, so the description there doesn't fit for your use case.
> 
> I think we will also have to figure out how to handle the old
> "ChromeOS"/"non_tz" use case (that resets Iris directly with the
> registers) vs the EL2 PAS use case (that resets Iris in TZ but still
> handles IOMMU from Linux). Simply checking for the presence of the
> "video-firmware" node is not enough, because that doesn't tell us if the
> PAS support is present in TZ.
> 
> I have been experimenting with a similar patch that copies the "non_tz"
> code paths from Venus into Iris. We need this to upstream the Iris DT
> patch for X1E without regressing the community-contributed x1-el2.dtso,
> which doesn't have functional PAS when running in EL2.
> 
> Perhaps we could check for __qcom_scm_is_call_available() with the new
> QCOM_SCM_PIL_PAS_GET_RSCTABLE to choose between invoking reset via PAS
> or directly with the registers. I don't have a device with the new
> firmware to verify if that works.

You can check QCOM_SCM_PIL_PAS_GET_RSCTABLE with __qcom_scm_is_call_available() 
but there is a possibility that QCOM_SCM_PIL_PAS_GET_RSCTABLE SMC call will be
used even for Gunyah. So, I believe, __qcom_scm_is_call_available() and
video-firmware's iommu property is also important.

> 
> I'll try to send out my patch soon, so you can better see the context.

Are you saying that you are going to send patch to support IRIS on
x1-el2.dtso in non-secure way i.e., non-PAS way.

> 
> Thanks,
> Stephan

-- 
-Mukesh Ojha

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Stephan Gerhold 1 month, 1 week ago

On Fri, Aug 22, 2025 at 08:36:11PM +0530, Mukesh Ojha wrote:
> On Fri, Aug 22, 2025 at 10:46:20AM +0200, Stephan Gerhold wrote:
> > On Fri, Aug 22, 2025 at 09:56:49AM +0530, Vikash Garodia wrote:
> > > On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
> > > >>>> +int iris_fw_init(struct iris_core *core)
> > > >>>> +{
> > > >>>> +	struct platform_device_info info;
> > > >>>> +	struct iommu_domain *iommu_dom;
> > > >>>> +	struct platform_device *pdev;
> > > >>>> +	struct device_node *np;
> > > >>>> +	int ret;
> > > >>>> +
> > > >>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > > >>>> +	if (!np)
> > > >>>> +		return 0;
> > > >>> You need a dt-bindings change for this as well. This is documented only
> > > >>> for Venus.
> > > >> You are right, wanted to send device tree and binding support separately.
> > > >> But if required, will add with the series in the next version.
> > > >>
> > > > You can send device tree changes separately, but dt-binding changes
> > > > always need to come before the driver changes.
> > > 
> > > Do you mean to update the examples section[1] with the firmware subnode,
> > > something similar to venus schema[2] ?
> > > 
> > 
> > Sorry, I missed the fact that the "video-firmware" subnode is already
> > documented for iris as well through qcom,venus-common.yaml (which is
> > included for qcom,sm8550-iris). I don't think it's strictly required to
> > add every possibility to the examples of the schema, since we'll also
> > have the actual DTBs later to test this part of the schema.
> > 
> > I would recommend to extend the description of the "video-firmware" node
> > in qcom,venus-common.yaml a bit. You do use the reset functionality of
> > TrustZone, so the description there doesn't fit for your use case.
> > 
> > I think we will also have to figure out how to handle the old
> > "ChromeOS"/"non_tz" use case (that resets Iris directly with the
> > registers) vs the EL2 PAS use case (that resets Iris in TZ but still
> > handles IOMMU from Linux). Simply checking for the presence of the
> > "video-firmware" node is not enough, because that doesn't tell us if the
> > PAS support is present in TZ.
> > 
> > I have been experimenting with a similar patch that copies the "non_tz"
> > code paths from Venus into Iris. We need this to upstream the Iris DT
> > patch for X1E without regressing the community-contributed x1-el2.dtso,
> > which doesn't have functional PAS when running in EL2.
> > 
> > Perhaps we could check for __qcom_scm_is_call_available() with the new
> > QCOM_SCM_PIL_PAS_GET_RSCTABLE to choose between invoking reset via PAS
> > or directly with the registers. I don't have a device with the new
> > firmware to verify if that works.
> 
> You can check QCOM_SCM_PIL_PAS_GET_RSCTABLE with __qcom_scm_is_call_available() 
> but there is a possibility that QCOM_SCM_PIL_PAS_GET_RSCTABLE SMC call will be
> used even for Gunyah. So, I believe, __qcom_scm_is_call_available() and
> video-firmware's iommu property is also important.
> 

Yeah, this sounds good.

> > 
> > I'll try to send out my patch soon, so you can better see the context.
> 
> Are you saying that you are going to send patch to support IRIS on
> x1-el2.dtso in non-secure way i.e., non-PAS way.
> 

The background is the following: I have a pending patch to add iris to
x1e80100.dtsi, but that currently breaks x1-el2.dtso. My original plan
was to disable &iris in x1-el2.dtso (because the PAS way seems to be
just broken), but then I saw that e.g. sc7180-el2.dtso does have working
Venus with the "video-firmware" node. Copy-pasting the "no_tz"(/non-PAS)
code as-is from venus into iris works just fine for x1-el2.dtso, so
disabling &iris in x1-el2.dtso just because the "no_tz" code is
currently missing in iris doesn't sound right.

As far as I understand the approach you use in this series does not work
without the TZ changes for older platforms like X1E(?), so adding that
code in iris seems to be the best way to move forward.

I started working on a patch for this a while ago, it just needs a bit
more cleanup. I'll try to finish it up and post it so we can discuss it
further. I think the IOMMU management in my patch would even work as-is
for you, you would just need to toggle a boolean to use the PAS instead
of accessing the registers directly.

Thanks,
Stephan

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Mukesh Ojha 1 month, 1 week ago

On Fri, Aug 22, 2025 at 06:26:19PM +0200, Stephan Gerhold wrote:
> On Fri, Aug 22, 2025 at 08:36:11PM +0530, Mukesh Ojha wrote:
> > On Fri, Aug 22, 2025 at 10:46:20AM +0200, Stephan Gerhold wrote:
> > > On Fri, Aug 22, 2025 at 09:56:49AM +0530, Vikash Garodia wrote:
> > > > On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
> > > > >>>> +int iris_fw_init(struct iris_core *core)
> > > > >>>> +{
> > > > >>>> +	struct platform_device_info info;
> > > > >>>> +	struct iommu_domain *iommu_dom;
> > > > >>>> +	struct platform_device *pdev;
> > > > >>>> +	struct device_node *np;
> > > > >>>> +	int ret;
> > > > >>>> +
> > > > >>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > > > >>>> +	if (!np)
> > > > >>>> +		return 0;
> > > > >>> You need a dt-bindings change for this as well. This is documented only
> > > > >>> for Venus.
> > > > >> You are right, wanted to send device tree and binding support separately.
> > > > >> But if required, will add with the series in the next version.
> > > > >>
> > > > > You can send device tree changes separately, but dt-binding changes
> > > > > always need to come before the driver changes.
> > > > 
> > > > Do you mean to update the examples section[1] with the firmware subnode,
> > > > something similar to venus schema[2] ?
> > > > 
> > > 
> > > Sorry, I missed the fact that the "video-firmware" subnode is already
> > > documented for iris as well through qcom,venus-common.yaml (which is
> > > included for qcom,sm8550-iris). I don't think it's strictly required to
> > > add every possibility to the examples of the schema, since we'll also
> > > have the actual DTBs later to test this part of the schema.
> > > 
> > > I would recommend to extend the description of the "video-firmware" node
> > > in qcom,venus-common.yaml a bit. You do use the reset functionality of
> > > TrustZone, so the description there doesn't fit for your use case.
> > > 
> > > I think we will also have to figure out how to handle the old
> > > "ChromeOS"/"non_tz" use case (that resets Iris directly with the
> > > registers) vs the EL2 PAS use case (that resets Iris in TZ but still
> > > handles IOMMU from Linux). Simply checking for the presence of the
> > > "video-firmware" node is not enough, because that doesn't tell us if the
> > > PAS support is present in TZ.
> > > 
> > > I have been experimenting with a similar patch that copies the "non_tz"
> > > code paths from Venus into Iris. We need this to upstream the Iris DT
> > > patch for X1E without regressing the community-contributed x1-el2.dtso,
> > > which doesn't have functional PAS when running in EL2.
> > > 
> > > Perhaps we could check for __qcom_scm_is_call_available() with the new
> > > QCOM_SCM_PIL_PAS_GET_RSCTABLE to choose between invoking reset via PAS
> > > or directly with the registers. I don't have a device with the new
> > > firmware to verify if that works.
> > 
> > You can check QCOM_SCM_PIL_PAS_GET_RSCTABLE with __qcom_scm_is_call_available() 
> > but there is a possibility that QCOM_SCM_PIL_PAS_GET_RSCTABLE SMC call will be
> > used even for Gunyah. So, I believe, __qcom_scm_is_call_available() and
> > video-firmware's iommu property is also important.
> > 
> 
> Yeah, this sounds good.
> 
> > > 
> > > I'll try to send out my patch soon, so you can better see the context.
> > 
> > Are you saying that you are going to send patch to support IRIS on
> > x1-el2.dtso in non-secure way i.e., non-PAS way.
> > 
> 
> The background is the following: I have a pending patch to add iris to
> x1e80100.dtsi, but that currently breaks x1-el2.dtso. My original plan
> was to disable &iris in x1-el2.dtso (because the PAS way seems to be
> just broken), but then I saw that e.g. sc7180-el2.dtso does have working
> Venus with the "video-firmware" node. Copy-pasting the "no_tz"(/non-PAS)
> code as-is from venus into iris works just fine for x1-el2.dtso, so
> disabling &iris in x1-el2.dtso just because the "no_tz" code is
> currently missing in iris doesn't sound right.
> 
> As far as I understand the approach you use in this series does not work
> without the TZ changes for older platforms like X1E(?), so adding that
> code in iris seems to be the best way to move forward.

Yes, this series has dependency on firmware and will not work for older
platforms.

> 
> I started working on a patch for this a while ago, it just needs a bit
> more cleanup. I'll try to finish it up and post it so we can discuss it
> further. I think the IOMMU management in my patch would even work as-is
> for you, you would just need to toggle a boolean to use the PAS instead
> of accessing the registers directly.

Sounds like a plan.
Thanks, please cc me when you send the patches; So, I could test along
with my changes and make dependency on it.

> 
> Thanks,
> Stephan

-- 
-Mukesh Ojha

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Stephan Gerhold 1 month, 1 week ago

On Fri, Aug 22, 2025 at 10:10:30PM +0530, Mukesh Ojha wrote:
> On Fri, Aug 22, 2025 at 06:26:19PM +0200, Stephan Gerhold wrote:
> > On Fri, Aug 22, 2025 at 08:36:11PM +0530, Mukesh Ojha wrote:
> > > On Fri, Aug 22, 2025 at 10:46:20AM +0200, Stephan Gerhold wrote:
> > > > On Fri, Aug 22, 2025 at 09:56:49AM +0530, Vikash Garodia wrote:
> > > > > On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
> > > > > >>>> +int iris_fw_init(struct iris_core *core)
> > > > > >>>> +{
> > > > > >>>> +	struct platform_device_info info;
> > > > > >>>> +	struct iommu_domain *iommu_dom;
> > > > > >>>> +	struct platform_device *pdev;
> > > > > >>>> +	struct device_node *np;
> > > > > >>>> +	int ret;
> > > > > >>>> +
> > > > > >>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > > > > >>>> +	if (!np)
> > > > > >>>> +		return 0;
> > > > > >>> You need a dt-bindings change for this as well. This is documented only
> > > > > >>> for Venus.
> > > > > >> You are right, wanted to send device tree and binding support separately.
> > > > > >> But if required, will add with the series in the next version.
> > > > > >>
> > > > > > You can send device tree changes separately, but dt-binding changes
> > > > > > always need to come before the driver changes.
> > > > > 
> > > > > Do you mean to update the examples section[1] with the firmware subnode,
> > > > > something similar to venus schema[2] ?
> > > > > 
> > > > 
> > > > Sorry, I missed the fact that the "video-firmware" subnode is already
> > > > documented for iris as well through qcom,venus-common.yaml (which is
> > > > included for qcom,sm8550-iris). I don't think it's strictly required to
> > > > add every possibility to the examples of the schema, since we'll also
> > > > have the actual DTBs later to test this part of the schema.
> > > > 
> > > > I would recommend to extend the description of the "video-firmware" node
> > > > in qcom,venus-common.yaml a bit. You do use the reset functionality of
> > > > TrustZone, so the description there doesn't fit for your use case.
> > > > 
> > > > I think we will also have to figure out how to handle the old
> > > > "ChromeOS"/"non_tz" use case (that resets Iris directly with the
> > > > registers) vs the EL2 PAS use case (that resets Iris in TZ but still
> > > > handles IOMMU from Linux). Simply checking for the presence of the
> > > > "video-firmware" node is not enough, because that doesn't tell us if the
> > > > PAS support is present in TZ.
> > > > 
> > > > I have been experimenting with a similar patch that copies the "non_tz"
> > > > code paths from Venus into Iris. We need this to upstream the Iris DT
> > > > patch for X1E without regressing the community-contributed x1-el2.dtso,
> > > > which doesn't have functional PAS when running in EL2.
> > > > 
> > > > Perhaps we could check for __qcom_scm_is_call_available() with the new
> > > > QCOM_SCM_PIL_PAS_GET_RSCTABLE to choose between invoking reset via PAS
> > > > or directly with the registers. I don't have a device with the new
> > > > firmware to verify if that works.
> > > 
> > > You can check QCOM_SCM_PIL_PAS_GET_RSCTABLE with __qcom_scm_is_call_available() 
> > > but there is a possibility that QCOM_SCM_PIL_PAS_GET_RSCTABLE SMC call will be
> > > used even for Gunyah. So, I believe, __qcom_scm_is_call_available() and
> > > video-firmware's iommu property is also important.
> > > 
> > 
> > Yeah, this sounds good.
> > 
> > > > 
> > > > I'll try to send out my patch soon, so you can better see the context.
> > > 
> > > Are you saying that you are going to send patch to support IRIS on
> > > x1-el2.dtso in non-secure way i.e., non-PAS way.
> > > 
> > 
> > The background is the following: I have a pending patch to add iris to
> > x1e80100.dtsi, but that currently breaks x1-el2.dtso. My original plan
> > was to disable &iris in x1-el2.dtso (because the PAS way seems to be
> > just broken), but then I saw that e.g. sc7180-el2.dtso does have working
> > Venus with the "video-firmware" node. Copy-pasting the "no_tz"(/non-PAS)
> > code as-is from venus into iris works just fine for x1-el2.dtso, so
> > disabling &iris in x1-el2.dtso just because the "no_tz" code is
> > currently missing in iris doesn't sound right.
> > 
> > As far as I understand the approach you use in this series does not work
> > without the TZ changes for older platforms like X1E(?), so adding that
> > code in iris seems to be the best way to move forward.
> 
> Yes, this series has dependency on firmware and will not work for older
> platforms.
> 
> > 
> > I started working on a patch for this a while ago, it just needs a bit
> > more cleanup. I'll try to finish it up and post it so we can discuss it
> > further. I think the IOMMU management in my patch would even work as-is
> > for you, you would just need to toggle a boolean to use the PAS instead
> > of accessing the registers directly.
> 
> Sounds like a plan.
> Thanks, please cc me when you send the patches; So, I could test along
> with my changes and make dependency on it.
> 

Krzysztof raised the concern that we shouldn't model the IOMMU specifier
for the firmware using a "video-firmware" subnode [1], similar to the
discussion for the "non-pixel" subnode recently [2].

I mostly finished up the cleanup of my patch, but I don't see any point
in posting it without an alternative proposal for the dt-bindings. For
this case, I think a simple property like

	firmware-iommus = <&apps_smmu ...>;

instead of

	video-firmware {
		iommus = <&apps_smmu ...>;
	};

could perhaps work. (XYZ-iommus isn't standardized at the moment, but I
think something like XYZ-gpios would make sense in this case. There are
many other possible approaches as well though.)

Unfortunately, I won't have enough time in the next weeks to fully
implement and propose an alternative. I'm assuming you still have
ongoing work for supporting the "non-pixel" IOMMU, perhaps your new
approach can be adapted for video-firmware as well?

I've pushed my current patch to a branch in case it helps. It's similar
to yours, but it has no external dependencies except for a fix in iris
I sent recently ("media: iris: Fix firmware reference leak and unmap
memory after load" [3]). You could use the non-PAS use case as a basis
to add the initial implementation in iris independent of this larger
patch series.

https://git.codelinaro.org/stephan.gerhold/linux/-/commit/1e068f5864d958ab9e807e6e3772b778cd0edea8.patch

For the PAS+IOMMU use case, it should be enough to set core->use_tz to
true, plus any changes needed for the SHM bridge (and maybe resource
table). The IOMMU management is independent from core->use_tz.

I'm also happy to add the non-PAS approach later on top of your changes,
whatever works best for you. :)

Thanks,
Stephan

[1]: https://lore.kernel.org/r/20250823155349.22344-2-krzysztof.kozlowski@linaro.org/
[2]: https://lore.kernel.org/r/20250627-video_cb-v3-0-51e18c0ffbce@quicinc.com/T/
[3]: https://lore.kernel.org/r/20250818-iris-firmware-leak-v1-1-1e3f9b8d31ce@linaro.org/

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Mukesh Ojha 1 month, 1 week ago

On Sat, Aug 23, 2025 at 10:43:52PM +0200, Stephan Gerhold wrote:
> On Fri, Aug 22, 2025 at 10:10:30PM +0530, Mukesh Ojha wrote:
> > On Fri, Aug 22, 2025 at 06:26:19PM +0200, Stephan Gerhold wrote:
> > > On Fri, Aug 22, 2025 at 08:36:11PM +0530, Mukesh Ojha wrote:
> > > > On Fri, Aug 22, 2025 at 10:46:20AM +0200, Stephan Gerhold wrote:
> > > > > On Fri, Aug 22, 2025 at 09:56:49AM +0530, Vikash Garodia wrote:
> > > > > > On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
> > > > > > >>>> +int iris_fw_init(struct iris_core *core)
> > > > > > >>>> +{
> > > > > > >>>> +	struct platform_device_info info;
> > > > > > >>>> +	struct iommu_domain *iommu_dom;
> > > > > > >>>> +	struct platform_device *pdev;
> > > > > > >>>> +	struct device_node *np;
> > > > > > >>>> +	int ret;
> > > > > > >>>> +
> > > > > > >>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > > > > > >>>> +	if (!np)
> > > > > > >>>> +		return 0;
> > > > > > >>> You need a dt-bindings change for this as well. This is documented only
> > > > > > >>> for Venus.
> > > > > > >> You are right, wanted to send device tree and binding support separately.
> > > > > > >> But if required, will add with the series in the next version.
> > > > > > >>
> > > > > > > You can send device tree changes separately, but dt-binding changes
> > > > > > > always need to come before the driver changes.
> > > > > > 
> > > > > > Do you mean to update the examples section[1] with the firmware subnode,
> > > > > > something similar to venus schema[2] ?
> > > > > > 
> > > > > 
> > > > > Sorry, I missed the fact that the "video-firmware" subnode is already
> > > > > documented for iris as well through qcom,venus-common.yaml (which is
> > > > > included for qcom,sm8550-iris). I don't think it's strictly required to
> > > > > add every possibility to the examples of the schema, since we'll also
> > > > > have the actual DTBs later to test this part of the schema.
> > > > > 
> > > > > I would recommend to extend the description of the "video-firmware" node
> > > > > in qcom,venus-common.yaml a bit. You do use the reset functionality of
> > > > > TrustZone, so the description there doesn't fit for your use case.
> > > > > 
> > > > > I think we will also have to figure out how to handle the old
> > > > > "ChromeOS"/"non_tz" use case (that resets Iris directly with the
> > > > > registers) vs the EL2 PAS use case (that resets Iris in TZ but still
> > > > > handles IOMMU from Linux). Simply checking for the presence of the
> > > > > "video-firmware" node is not enough, because that doesn't tell us if the
> > > > > PAS support is present in TZ.
> > > > > 
> > > > > I have been experimenting with a similar patch that copies the "non_tz"
> > > > > code paths from Venus into Iris. We need this to upstream the Iris DT
> > > > > patch for X1E without regressing the community-contributed x1-el2.dtso,
> > > > > which doesn't have functional PAS when running in EL2.
> > > > > 
> > > > > Perhaps we could check for __qcom_scm_is_call_available() with the new
> > > > > QCOM_SCM_PIL_PAS_GET_RSCTABLE to choose between invoking reset via PAS
> > > > > or directly with the registers. I don't have a device with the new
> > > > > firmware to verify if that works.
> > > > 
> > > > You can check QCOM_SCM_PIL_PAS_GET_RSCTABLE with __qcom_scm_is_call_available() 
> > > > but there is a possibility that QCOM_SCM_PIL_PAS_GET_RSCTABLE SMC call will be
> > > > used even for Gunyah. So, I believe, __qcom_scm_is_call_available() and
> > > > video-firmware's iommu property is also important.
> > > > 
> > > 
> > > Yeah, this sounds good.
> > > 
> > > > > 
> > > > > I'll try to send out my patch soon, so you can better see the context.
> > > > 
> > > > Are you saying that you are going to send patch to support IRIS on
> > > > x1-el2.dtso in non-secure way i.e., non-PAS way.
> > > > 
> > > 
> > > The background is the following: I have a pending patch to add iris to
> > > x1e80100.dtsi, but that currently breaks x1-el2.dtso. My original plan
> > > was to disable &iris in x1-el2.dtso (because the PAS way seems to be
> > > just broken), but then I saw that e.g. sc7180-el2.dtso does have working
> > > Venus with the "video-firmware" node. Copy-pasting the "no_tz"(/non-PAS)
> > > code as-is from venus into iris works just fine for x1-el2.dtso, so
> > > disabling &iris in x1-el2.dtso just because the "no_tz" code is
> > > currently missing in iris doesn't sound right.
> > > 
> > > As far as I understand the approach you use in this series does not work
> > > without the TZ changes for older platforms like X1E(?), so adding that
> > > code in iris seems to be the best way to move forward.
> > 
> > Yes, this series has dependency on firmware and will not work for older
> > platforms.
> > 
> > > 
> > > I started working on a patch for this a while ago, it just needs a bit
> > > more cleanup. I'll try to finish it up and post it so we can discuss it
> > > further. I think the IOMMU management in my patch would even work as-is
> > > for you, you would just need to toggle a boolean to use the PAS instead
> > > of accessing the registers directly.
> > 
> > Sounds like a plan.
> > Thanks, please cc me when you send the patches; So, I could test along
> > with my changes and make dependency on it.
> > 
> 
> Krzysztof raised the concern that we shouldn't model the IOMMU specifier
> for the firmware using a "video-firmware" subnode [1], similar to the
> discussion for the "non-pixel" subnode recently [2].
> 
> I mostly finished up the cleanup of my patch, but I don't see any point
> in posting it without an alternative proposal for the dt-bindings. For
> this case, I think a simple property like
> 
> 	firmware-iommus = <&apps_smmu ...>;
> 
> instead of
> 
> 	video-firmware {
> 		iommus = <&apps_smmu ...>;
> 	};
> 
> could perhaps work. (XYZ-iommus isn't standardized at the moment, but I
> think something like XYZ-gpios would make sense in this case. There are
> many other possible approaches as well though.)
> 
> Unfortunately, I won't have enough time in the next weeks to fully
> implement and propose an alternative. I'm assuming you still have
> ongoing work for supporting the "non-pixel" IOMMU, perhaps your new
> approach can be adapted for video-firmware as well?

I believe, non-pixel case a bit different and thats not depends on whether
it is PAS or non-PAS.

However, I liked the idea about introducing something similar to -gpios
for -iommus as could pottentially solves at least this issue. Here, we need
to create a platform device and its domain based on firmware-iommu
property.

So, its required change in device link to put supplier/consumer dependency
and addition of firmware-iommu binding for IRIS and little of changes
over your existing changes.

But I have doubt, whether @Krzysztof would be fine with it ?

> 
> I've pushed my current patch to a branch in case it helps. It's similar
> to yours, but it has no external dependencies except for a fix in iris
> I sent recently ("media: iris: Fix firmware reference leak and unmap
> memory after load" [3]). You could use the non-PAS use case as a basis
> to add the initial implementation in iris independent of this larger
> patch series.

Thanks.

> 
> https://git.codelinaro.org/stephan.gerhold/linux/-/commit/1e068f5864d958ab9e807e6e3772b778cd0edea8.patch
> 
> For the PAS+IOMMU use case, it should be enough to set core->use_tz to
> true, plus any changes needed for the SHM bridge (and maybe resource
> table). The IOMMU management is independent from core->use_tz.
> 
> I'm also happy to add the non-PAS approach later on top of your changes,
> whatever works best for you. :)
> 
> Thanks,
> Stephan
> 
> [1]: https://lore.kernel.org/r/20250823155349.22344-2-krzysztof.kozlowski@linaro.org/
> [2]: https://lore.kernel.org/r/20250627-video_cb-v3-0-51e18c0ffbce@quicinc.com/T/
> [3]: https://lore.kernel.org/r/20250818-iris-firmware-leak-v1-1-1e3f9b8d31ce@linaro.org/

-- 
-Mukesh Ojha

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Stephan Gerhold 3 weeks, 4 days ago

On Mon, Aug 25, 2025 at 04:49:56PM +0530, Mukesh Ojha wrote:
> On Sat, Aug 23, 2025 at 10:43:52PM +0200, Stephan Gerhold wrote:
> > On Fri, Aug 22, 2025 at 10:10:30PM +0530, Mukesh Ojha wrote:
> > > On Fri, Aug 22, 2025 at 06:26:19PM +0200, Stephan Gerhold wrote:
> > > > On Fri, Aug 22, 2025 at 08:36:11PM +0530, Mukesh Ojha wrote:
> > > > > On Fri, Aug 22, 2025 at 10:46:20AM +0200, Stephan Gerhold wrote:
> > > > > > On Fri, Aug 22, 2025 at 09:56:49AM +0530, Vikash Garodia wrote:
> > > > > > > On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
> > > > > > > >>>> +int iris_fw_init(struct iris_core *core)
> > > > > > > >>>> +{
> > > > > > > >>>> +	struct platform_device_info info;
> > > > > > > >>>> +	struct iommu_domain *iommu_dom;
> > > > > > > >>>> +	struct platform_device *pdev;
> > > > > > > >>>> +	struct device_node *np;
> > > > > > > >>>> +	int ret;
> > > > > > > >>>> +
> > > > > > > >>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > > > > > > >>>> +	if (!np)
> > > > > > > >>>> +		return 0;
> > > > > > > >>> You need a dt-bindings change for this as well. This is documented only
> > > > > > > >>> for Venus.
> > > > > > > >> You are right, wanted to send device tree and binding support separately.
> > > > > > > >> But if required, will add with the series in the next version.
> > > > > > > >>
> > > > > > > > You can send device tree changes separately, but dt-binding changes
> > > > > > > > always need to come before the driver changes.
> > > > > > > 
> > > > > > > Do you mean to update the examples section[1] with the firmware subnode,
> > > > > > > something similar to venus schema[2] ?
> > > > > > > 
> > > > > > 
> > > > > > Sorry, I missed the fact that the "video-firmware" subnode is already
> > > > > > documented for iris as well through qcom,venus-common.yaml (which is
> > > > > > included for qcom,sm8550-iris). I don't think it's strictly required to
> > > > > > add every possibility to the examples of the schema, since we'll also
> > > > > > have the actual DTBs later to test this part of the schema.
> > > > > > 
> > > > > > I would recommend to extend the description of the "video-firmware" node
> > > > > > in qcom,venus-common.yaml a bit. You do use the reset functionality of
> > > > > > TrustZone, so the description there doesn't fit for your use case.
> > > > > > 
> > > > > > I think we will also have to figure out how to handle the old
> > > > > > "ChromeOS"/"non_tz" use case (that resets Iris directly with the
> > > > > > registers) vs the EL2 PAS use case (that resets Iris in TZ but still
> > > > > > handles IOMMU from Linux). Simply checking for the presence of the
> > > > > > "video-firmware" node is not enough, because that doesn't tell us if the
> > > > > > PAS support is present in TZ.
> > > > > > 
> > > > > > I have been experimenting with a similar patch that copies the "non_tz"
> > > > > > code paths from Venus into Iris. We need this to upstream the Iris DT
> > > > > > patch for X1E without regressing the community-contributed x1-el2.dtso,
> > > > > > which doesn't have functional PAS when running in EL2.
> > > > > > 
> > > > > > Perhaps we could check for __qcom_scm_is_call_available() with the new
> > > > > > QCOM_SCM_PIL_PAS_GET_RSCTABLE to choose between invoking reset via PAS
> > > > > > or directly with the registers. I don't have a device with the new
> > > > > > firmware to verify if that works.
> > > > > 
> > > > > You can check QCOM_SCM_PIL_PAS_GET_RSCTABLE with __qcom_scm_is_call_available() 
> > > > > but there is a possibility that QCOM_SCM_PIL_PAS_GET_RSCTABLE SMC call will be
> > > > > used even for Gunyah. So, I believe, __qcom_scm_is_call_available() and
> > > > > video-firmware's iommu property is also important.
> > > > > 
> > > > 
> > > > Yeah, this sounds good.
> > > > 
> > > > > > 
> > > > > > I'll try to send out my patch soon, so you can better see the context.
> > > > > 
> > > > > Are you saying that you are going to send patch to support IRIS on
> > > > > x1-el2.dtso in non-secure way i.e., non-PAS way.
> > > > > 
> > > > 
> > > > The background is the following: I have a pending patch to add iris to
> > > > x1e80100.dtsi, but that currently breaks x1-el2.dtso. My original plan
> > > > was to disable &iris in x1-el2.dtso (because the PAS way seems to be
> > > > just broken), but then I saw that e.g. sc7180-el2.dtso does have working
> > > > Venus with the "video-firmware" node. Copy-pasting the "no_tz"(/non-PAS)
> > > > code as-is from venus into iris works just fine for x1-el2.dtso, so
> > > > disabling &iris in x1-el2.dtso just because the "no_tz" code is
> > > > currently missing in iris doesn't sound right.
> > > > 
> > > > As far as I understand the approach you use in this series does not work
> > > > without the TZ changes for older platforms like X1E(?), so adding that
> > > > code in iris seems to be the best way to move forward.
> > > 
> > > Yes, this series has dependency on firmware and will not work for older
> > > platforms.
> > > 
> > > > 
> > > > I started working on a patch for this a while ago, it just needs a bit
> > > > more cleanup. I'll try to finish it up and post it so we can discuss it
> > > > further. I think the IOMMU management in my patch would even work as-is
> > > > for you, you would just need to toggle a boolean to use the PAS instead
> > > > of accessing the registers directly.
> > > 
> > > Sounds like a plan.
> > > Thanks, please cc me when you send the patches; So, I could test along
> > > with my changes and make dependency on it.
> > > 
> > 
> > Krzysztof raised the concern that we shouldn't model the IOMMU specifier
> > for the firmware using a "video-firmware" subnode [1], similar to the
> > discussion for the "non-pixel" subnode recently [2].
> > 
> > I mostly finished up the cleanup of my patch, but I don't see any point
> > in posting it without an alternative proposal for the dt-bindings. For
> > this case, I think a simple property like
> > 
> > 	firmware-iommus = <&apps_smmu ...>;
> > 
> > instead of
> > 
> > 	video-firmware {
> > 		iommus = <&apps_smmu ...>;
> > 	};
> > 
> > could perhaps work. (XYZ-iommus isn't standardized at the moment, but I
> > think something like XYZ-gpios would make sense in this case. There are
> > many other possible approaches as well though.)
> > 
> > Unfortunately, I won't have enough time in the next weeks to fully
> > implement and propose an alternative. I'm assuming you still have
> > ongoing work for supporting the "non-pixel" IOMMU, perhaps your new
> > approach can be adapted for video-firmware as well?
> 
> I believe, non-pixel case a bit different and thats not depends on whether
> it is PAS or non-PAS.
> 
> However, I liked the idea about introducing something similar to -gpios
> for -iommus as could pottentially solves at least this issue. Here, we need
> to create a platform device and its domain based on firmware-iommu
> property.
> 
> So, its required change in device link to put supplier/consumer dependency
> and addition of firmware-iommu binding for IRIS and little of changes
> over your existing changes.
> 
> But I have doubt, whether @Krzysztof would be fine with it ?
> 

Krzysztof isn't on Cc here so I wouldn't expect him to reply. :-)
I'm not sure if it's helpful to add him in the middle of the discussion
either (at least without proper summary of the problem description).

I think it would be best to prepare a patch series with the motivation
properly described. If making the actual implementation (to create the
platform device etc) is too much work it could also be sent as RFC with
only the dt-bindings.

Have you continued working on this to unblock adding the IOMMU needed
for the IRIS firmware?

Thanks,
Stephan

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Mukesh Ojha 3 weeks, 3 days ago

On Mon, Sep 08, 2025 at 10:23:31AM +0200, Stephan Gerhold wrote:
> On Mon, Aug 25, 2025 at 04:49:56PM +0530, Mukesh Ojha wrote:
> > On Sat, Aug 23, 2025 at 10:43:52PM +0200, Stephan Gerhold wrote:
> > > On Fri, Aug 22, 2025 at 10:10:30PM +0530, Mukesh Ojha wrote:
> > > > On Fri, Aug 22, 2025 at 06:26:19PM +0200, Stephan Gerhold wrote:
> > > > > On Fri, Aug 22, 2025 at 08:36:11PM +0530, Mukesh Ojha wrote:
> > > > > > On Fri, Aug 22, 2025 at 10:46:20AM +0200, Stephan Gerhold wrote:
> > > > > > > On Fri, Aug 22, 2025 at 09:56:49AM +0530, Vikash Garodia wrote:
> > > > > > > > On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
> > > > > > > > >>>> +int iris_fw_init(struct iris_core *core)
> > > > > > > > >>>> +{
> > > > > > > > >>>> +	struct platform_device_info info;
> > > > > > > > >>>> +	struct iommu_domain *iommu_dom;
> > > > > > > > >>>> +	struct platform_device *pdev;
> > > > > > > > >>>> +	struct device_node *np;
> > > > > > > > >>>> +	int ret;
> > > > > > > > >>>> +
> > > > > > > > >>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > > > > > > > >>>> +	if (!np)
> > > > > > > > >>>> +		return 0;
> > > > > > > > >>> You need a dt-bindings change for this as well. This is documented only
> > > > > > > > >>> for Venus.
> > > > > > > > >> You are right, wanted to send device tree and binding support separately.
> > > > > > > > >> But if required, will add with the series in the next version.
> > > > > > > > >>
> > > > > > > > > You can send device tree changes separately, but dt-binding changes
> > > > > > > > > always need to come before the driver changes.
> > > > > > > > 
> > > > > > > > Do you mean to update the examples section[1] with the firmware subnode,
> > > > > > > > something similar to venus schema[2] ?
> > > > > > > > 
> > > > > > > 
> > > > > > > Sorry, I missed the fact that the "video-firmware" subnode is already
> > > > > > > documented for iris as well through qcom,venus-common.yaml (which is
> > > > > > > included for qcom,sm8550-iris). I don't think it's strictly required to
> > > > > > > add every possibility to the examples of the schema, since we'll also
> > > > > > > have the actual DTBs later to test this part of the schema.
> > > > > > > 
> > > > > > > I would recommend to extend the description of the "video-firmware" node
> > > > > > > in qcom,venus-common.yaml a bit. You do use the reset functionality of
> > > > > > > TrustZone, so the description there doesn't fit for your use case.
> > > > > > > 
> > > > > > > I think we will also have to figure out how to handle the old
> > > > > > > "ChromeOS"/"non_tz" use case (that resets Iris directly with the
> > > > > > > registers) vs the EL2 PAS use case (that resets Iris in TZ but still
> > > > > > > handles IOMMU from Linux). Simply checking for the presence of the
> > > > > > > "video-firmware" node is not enough, because that doesn't tell us if the
> > > > > > > PAS support is present in TZ.
> > > > > > > 
> > > > > > > I have been experimenting with a similar patch that copies the "non_tz"
> > > > > > > code paths from Venus into Iris. We need this to upstream the Iris DT
> > > > > > > patch for X1E without regressing the community-contributed x1-el2.dtso,
> > > > > > > which doesn't have functional PAS when running in EL2.
> > > > > > > 
> > > > > > > Perhaps we could check for __qcom_scm_is_call_available() with the new
> > > > > > > QCOM_SCM_PIL_PAS_GET_RSCTABLE to choose between invoking reset via PAS
> > > > > > > or directly with the registers. I don't have a device with the new
> > > > > > > firmware to verify if that works.
> > > > > > 
> > > > > > You can check QCOM_SCM_PIL_PAS_GET_RSCTABLE with __qcom_scm_is_call_available() 
> > > > > > but there is a possibility that QCOM_SCM_PIL_PAS_GET_RSCTABLE SMC call will be
> > > > > > used even for Gunyah. So, I believe, __qcom_scm_is_call_available() and
> > > > > > video-firmware's iommu property is also important.
> > > > > > 
> > > > > 
> > > > > Yeah, this sounds good.
> > > > > 
> > > > > > > 
> > > > > > > I'll try to send out my patch soon, so you can better see the context.
> > > > > > 
> > > > > > Are you saying that you are going to send patch to support IRIS on
> > > > > > x1-el2.dtso in non-secure way i.e., non-PAS way.
> > > > > > 
> > > > > 
> > > > > The background is the following: I have a pending patch to add iris to
> > > > > x1e80100.dtsi, but that currently breaks x1-el2.dtso. My original plan
> > > > > was to disable &iris in x1-el2.dtso (because the PAS way seems to be
> > > > > just broken), but then I saw that e.g. sc7180-el2.dtso does have working
> > > > > Venus with the "video-firmware" node. Copy-pasting the "no_tz"(/non-PAS)
> > > > > code as-is from venus into iris works just fine for x1-el2.dtso, so
> > > > > disabling &iris in x1-el2.dtso just because the "no_tz" code is
> > > > > currently missing in iris doesn't sound right.
> > > > > 
> > > > > As far as I understand the approach you use in this series does not work
> > > > > without the TZ changes for older platforms like X1E(?), so adding that
> > > > > code in iris seems to be the best way to move forward.
> > > > 
> > > > Yes, this series has dependency on firmware and will not work for older
> > > > platforms.
> > > > 
> > > > > 
> > > > > I started working on a patch for this a while ago, it just needs a bit
> > > > > more cleanup. I'll try to finish it up and post it so we can discuss it
> > > > > further. I think the IOMMU management in my patch would even work as-is
> > > > > for you, you would just need to toggle a boolean to use the PAS instead
> > > > > of accessing the registers directly.
> > > > 
> > > > Sounds like a plan.
> > > > Thanks, please cc me when you send the patches; So, I could test along
> > > > with my changes and make dependency on it.
> > > > 
> > > 
> > > Krzysztof raised the concern that we shouldn't model the IOMMU specifier
> > > for the firmware using a "video-firmware" subnode [1], similar to the
> > > discussion for the "non-pixel" subnode recently [2].
> > > 
> > > I mostly finished up the cleanup of my patch, but I don't see any point
> > > in posting it without an alternative proposal for the dt-bindings. For
> > > this case, I think a simple property like
> > > 
> > > 	firmware-iommus = <&apps_smmu ...>;
> > > 
> > > instead of
> > > 
> > > 	video-firmware {
> > > 		iommus = <&apps_smmu ...>;
> > > 	};
> > > 
> > > could perhaps work. (XYZ-iommus isn't standardized at the moment, but I
> > > think something like XYZ-gpios would make sense in this case. There are
> > > many other possible approaches as well though.)
> > > 
> > > Unfortunately, I won't have enough time in the next weeks to fully
> > > implement and propose an alternative. I'm assuming you still have
> > > ongoing work for supporting the "non-pixel" IOMMU, perhaps your new
> > > approach can be adapted for video-firmware as well?
> > 
> > I believe, non-pixel case a bit different and thats not depends on whether
> > it is PAS or non-PAS.
> > 
> > However, I liked the idea about introducing something similar to -gpios
> > for -iommus as could pottentially solves at least this issue. Here, we need
> > to create a platform device and its domain based on firmware-iommu
> > property.
> > 
> > So, its required change in device link to put supplier/consumer dependency
> > and addition of firmware-iommu binding for IRIS and little of changes
> > over your existing changes.
> > 
> > But I have doubt, whether @Krzysztof would be fine with it ?
> > 
> 
> Krzysztof isn't on Cc here so I wouldn't expect him to reply. :-)
> I'm not sure if it's helpful to add him in the middle of the discussion
> either (at least without proper summary of the problem description).
> 
> I think it would be best to prepare a patch series with the motivation
> properly described. If making the actual implementation (to create the
> platform device etc) is too much work it could also be sent as RFC with
> only the dt-bindings.
> 
> Have you continued working on this to unblock adding the IOMMU needed
> for the IRIS firmware?

We are discussing on this internally and if this can be taken along with
non-pixel case or not, will come back on this.

If it takes too much, will drop video support for now in next version.

> 
> Thanks,
> Stephan

-- 
-Mukesh Ojha

Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with IOMMU managed by Linux

Posted by Bryan O'Donoghue 3 weeks, 3 days ago

On 09/09/2025 13:19, Mukesh Ojha wrote:
>> Have you continued working on this to unblock adding the IOMMU needed
>> for the IRIS firmware?
> We are discussing on this internally and if this can be taken along with
> non-pixel case or not, will come back on this.
> 
> If it takes too much, will drop video support for now in next version.

For preference I'd rather get something that can be applied to -next so 
that we can enable PIL loading on current kernels instead of waiting an 
indeterminate time for an iommu non-pixel resolution.

i.e. we are better of with stuff that works now instead of gating 
ourselves forever on a fix that may be months or years off.

---
bod
the optimist