Re: [PATCH RFC v2 01/14] iommu: arm-smmu-qcom: Configure lpac device with split address space

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iommu/arm-smmu-qcom: qcom_adreno_smmu_alloc_context_bank() shifts GMU to cb 2
- [Medium] iommu/arm-smmu-qcom: undocumented qcom,lpac DT compatible breaks DT ABI
--

commit 89b03a5b08cd9912b4e1abfb59372ce6c00d8186
Author: Anna Maniscalco <anna.maniscalco2000@gmail.com>

iommu: arm-smmu-qcom: Configure lpac device with split address space

This commit configures the LPAC device to use a split address space by
matching the "qcom,lpac" compatible string, recognizing stream ID 1 in
qcom_adreno_smmu_is_gpu_device(), and reserving context banks 0 and 1 for
GPU and LPAC devices.

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index edd41b5a3b6ac..48a590aaeb5eb 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -35,6 +35,8 @@
>  static const struct of_device_id qcom_smmu_actlr_client_of_match[] = {
>  	{ .compatible = "qcom,adreno",
>  			.data = (const void *) (PREFETCH_DEEP | CPRE | CMTLB) },
> +	{ .compatible = "qcom,lpac",
> +			.data = (const void *) (PREFETCH_DEEP | CPRE | CMTLB) },

[Severity: Medium]
Does adding "qcom,lpac" here and in qcom_smmu_client_of_match[] introduce
an undocumented Device Tree compatible string and break Device Tree
backwards compatibility?

Neither this commit nor the follow-up commits in this series document
"qcom,lpac" in Documentation/devicetree/bindings/, and
Documentation/devicetree/bindings/display/msm/gpu.yaml sets
additionalProperties: false:

Documentation/devicetree/bindings/display/msm/gpu.yaml:
  required:
    - compatible
    - reg
    - interrupts

  additionalProperties: false

which causes dtbs_check validation to fail when an lpac child node is added
under the GPU node in "arm64: dts: qcom: sm8650: move smmu sid 1 to new
lpac device".

Furthermore, existing upstream Device Trees (such as sm8350.dtsi,
sc8280xp.dtsi, sm8450.dtsi, sm8550.dtsi, sm8650.dtsi, hamoa.dtsi,
kodiak.dtsi, lemans.dtsi, and monaco.dtsi) already assign Stream ID 1
directly to the main "qcom,adreno" GPU node:

arch/arm64/boot/dts/qcom/sm8650.dtsi:
  iommus = <&adreno_smmu 0 0x0>,
           <&adreno_smmu 1 0x0>;

When booting with an existing sm8650 DTB that lacks the new "qcom,lpac"
child node, gpu->lpac_pdev is NULL and context bank 1 is never allocated,
yet the later commits "firmware: qcom: scm: Configure LPAC aperture",
"drm/msm: initialize LPAC ring", and "drm/msm: enable LPAC on gen70900"
gate LPAC hardware initialization on ADRENO_QUIRK_LPAC rather than
gpu->lpac_pdev.

[ ... ]

> @@ -305,9 +309,9 @@ static int qcom_adreno_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_doma
>  	 */
>  	if (qcom_adreno_smmu_is_gpu_device(dev)) {
>  		start = 0;
> -		count = 1;
> +		count = 2;
>  	} else {
> -		start = 1;
> +		start = 2;
>  		count = smmu->num_context_banks;
>  	}

[Severity: High]
Won't unconditionally changing start from 1 to 2 in the else branch of
qcom_adreno_smmu_alloc_context_bank() shift the Adreno GMU ("qcom,adreno-gmu",
SID 5) from context bank 1 to context bank 2 across all Adreno platforms,
and break non-GPU context bank allocation on qcom,sdm630-smmu-v2?

During Adreno GMU initialization on Adreno 6xx/7xx/8xx SoCs, the call chain:

  a6xx_gpu_init() ->
    a6xx_gmu_init() ->
      a6xx_gmu_memory_probe() ->
        msm_iommu_new(gmu->dev, NULL, 0) ->
          iommu_attach_device() ->
            arm_smmu_attach_dev() ->
              arm_smmu_init_domain_context() ->
                qcom_adreno_smmu_alloc_context_bank()

evaluates gmu->dev (SID 5), which takes the else branch (both here and after
the later commit "iommu: arm-smmu-qcom: Fixed mapping between sid and cb for
gpu and lpac") and allocates cbndx 2 instead of cbndx 1, leaving cbndx 1
unused on all SoCs or DTBs without a separate "qcom,lpac" device.

As noted in commit afc95681c30689 ("iommu/arm-smmu-qcom: Add missing GMU
entry to match table"), firmware on platforms such as sc8280xp expects
context bank 1 to be assigned to the GMU; assigning context bank 2 causes
the SMMU to wedge and the GPU to hang without memory access when a GPU
translation fault occurs.

Additionally, on qcom,sdm630-smmu-v2, qcom_adreno_smmuv2_cfg_probe() limits
smmu->num_context_banks to 2:

drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:qcom_adreno_smmuv2_cfg_probe() {
	/* TZ protects several last context banks, hide them from Linux */
	if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm630-smmu-v2") &&
	    smmu->num_context_banks == 5)
		smmu->num_context_banks = 2;
}

With start = 2 and count = 2, __arm_smmu_alloc_bitmap(smmu->context_map, 2, 2)
has an empty allocation range and will unconditionally return -ENOSPC for
any non-GPU device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-descriptive-name-lpac-upstream-v2-0-a21a5c850961@gmail.com?part=1