Re: [PATCH v2] arm64: dts: qcom: kaanapali: Add memory-region for audio PD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm64: dts: qcom: kaanapali: shared-dma-pool on no-map ADSP remote heap
--

commit efc182312f70843b0020c655e0df8d4588f2efb8
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

arm64: dts: qcom: kaanapali: Add memory-region for audio PD

This commit adds a reserved memory region and destination vmids for the
kaanapali audio pd remote heap and links them to the adsp fastrpc node.

> diff --git a/arch/arm64/boot/dts/qcom/kaanapali.dtsi b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> index 131fd2a16c545..8a1b53e9f73b7 100644
> --- a/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> @@ -451,6 +451,14 @@ rmtfs_mem: rmtfs@d7c00000 {
>  			qcom,client-id = <1>;
>  			qcom,vmid = <QCOM_SCM_VMID_MSS_MSA>;
>  		};
> +
> +		adsp_rpc_remote_heap_mem: adsp-rpc-remote-heap {
> +			compatible = "shared-dma-pool";
> +			alloc-ranges = <0x0 0x0 0x1 0x0>;
> +			no-map;
> +			alignment = <0x0 0x400000>;
> +			size = <0x0 0x800000>;
> +		};

[Severity: High]
Should compatible = "shared-dma-pool" be dropped from
adsp_rpc_remote_heap_mem now that this node uses no-map?

In v2, adsp_rpc_remote_heap_mem was changed to no-map because since commit
9981fbe599e8 ("misc: fastrpc: Allocate entire reserved memory for Audio PD
in probe"), fastrpc_rpmsg_probe() -> fastrpc_init_reserved_mem() in
drivers/misc/fastrpc.c retrieves the entire reserved-memory region via
of_reserved_mem_region_to_resource() and reassigns ownership of the whole
region away from QCOM_SCM_VMID_HLOS to QCOM_SCM_VMID_LPASS and
QCOM_SCM_VMID_ADSP_HEAP via qcom_scm_assign_mem():

drivers/misc/fastrpc.c:fastrpc_init_reserved_mem() {
    ...
	src_perms = BIT(QCOM_SCM_VMID_HLOS);
	return qcom_scm_assign_mem(res.start, resource_size(&res), &src_perms,
				   cctx->vmperms, cctx->vmcount);
}

However, keeping compatible = "shared-dma-pool" alongside no-map causes
__reserved_mem_alloc_size() -> fdt_init_reserved_mem_node() ->
__reserved_mem_init_node() in drivers/of/of_reserved_mem.c to fall through
rmem_cma_setup() in kernel/dma/contiguous.c (which returns -ENODEV when
reusable is absent) and bind rmem_dma_setup() in kernel/dma/coherent.c with
rmem->ops = &rmem_dma_ops.

When the adsp fastrpc rpmsg device probes in fastrpc_rpmsg_probe() in
drivers/misc/fastrpc.c, devm_of_reserved_mem_device_init(rdev) runs before
fastrpc_init_reserved_mem() and dma_set_mask_and_coherent():

drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() {
    ...
	if (devm_of_reserved_mem_device_init(rdev))
		dev_dbg(rdev, "no reserved DMA memory for FASTRPC\n");
    ...
	err = fastrpc_init_reserved_mem(data);
    ...
	err = dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32));
}

This invokes rmem_dma_device_init() in kernel/dma/coherent.c:

kernel/dma/coherent.c:rmem_dma_device_init() {
	struct dma_coherent_mem *mem = rmem->priv;

	if (!mem) {
		mem = dma_init_coherent_memory(rmem->base, rmem->base,
					       rmem->size, true);
		if (IS_ERR(mem))
			return PTR_ERR(mem);
		rmem->priv = mem;
	}

	/* Warn if the device potentially can't use the reserved memory */
	if (mem->device_base + rmem->size - 1 >
	    min_not_zero(dev->coherent_dma_mask, dev->bus_dma_limit))
		dev_warn(dev, "reserved memory is beyond device's set DMA address range\n");

	return dma_assign_coherent_memory(dev, mem);
}

Here, dma_init_coherent_memory() calls memremap(..., MEMREMAP_WC) in
kernel/iomem.c -> ioremap_wc() in arch/arm64/include/asm/io.h, creating a
live 8 MiB stage-1 normal non-cacheable (PROT_NORMAL_NC / MT_NORMAL_NC)
kernel virtual mapping in mem->virt_base despite no-map, and caches mem in
rmem->priv where rmem_dma_device_release() in kernel/dma/coherent.c never
unmaps it.

In addition, because rdev->coherent_dma_mask and rdev->bus_dma_limit are
still 0 at that point in fastrpc_rpmsg_probe(), rmem_dma_device_init()
unconditionally triggers the dev_warn() about the reserved memory being
beyond the device's set dma address range, and attaches rdev->dma_mem.

Once fastrpc_init_reserved_mem() revokes QCOM_SCM_VMID_HLOS stage-2 access
on the 8 MiB region via qcom_scm_assign_mem(), won't leaving an active
stage-1 PROT_NORMAL_NC mapping and coherent pool attached to rdev expose
the kernel to a stage-2 permission fault upon speculative cpu reads or
allocations through fastrpc_remote_heap_alloc() -> dma_alloc_coherent() ->
__dma_alloc_from_coherent() in kernel/dma/coherent.c?

Omitting compatible = "shared-dma-pool" (matching the no-map
adsp_rpc_remote_heap_mem nodes in hamoa.dtsi, lemans.dtsi, and monaco.dtsi,
as well as dynamic no-map regions using size, alignment, and alloc-ranges
such as adsp_mem in msm8916.dtsi and mdata_mem in sdm845.dtsi) would still
let __reserved_mem_alloc_size() allocate and mark the 8 MiB region nomap
while leaving rmem->ops as NULL so only of_reserved_mem_region_to_resource()
consumes it.

>  	};
>  
>  	smp2p-adsp {
> @@ -2755,6 +2763,9 @@ fastrpc {
>  					compatible = "qcom,kaanapali-fastrpc";
>  					qcom,glink-channels = "fastrpcglink-apps-dsp";
>  					label = "adsp";
> +					memory-region = <&adsp_rpc_remote_heap_mem>;
> +					qcom,vmids = <QCOM_SCM_VMID_LPASS
> +						      QCOM_SCM_VMID_ADSP_HEAP>;
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-knp-rh-v2-1-4e2f8e16b607@oss.qualcomm.com?part=1