arm64: dts: qcom: agatti: Fix IOMMU DT properties
Subsystems (9)
PATCH 1/2 arm64: dts: qcom: agatti: Fix IOMMU DT properties
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
The DT changes should be backwards compatible with legacy QHEE based firmware stack too.
--- arch/arm64/boot/dts/qcom/agatti.dtsi | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
arch/arm64/boot/dts/qcom/agatti.dtsi +3 -8
@@ -1613,8 +1613,7 @@ gpu: gpu@5900000 { &bimc SLAVE_EBI1 RPM_ALWAYS_TAG>; interconnect-names = "gfx-mem"; - iommus = <&adreno_smmu 0 1>, - <&adreno_smmu 2 0>; + iommus = <&adreno_smmu 0 1>; operating-points-v2 = <&gpu_opp_table>; power-domains = <&rpmpd QCM2290_VDDCX>; qcom,gmu = <&gmu_wrapper>;
@@ -1895,8 +1894,7 @@ mdss: display-subsystem@5e00000 { power-domains = <&dispcc MDSS_GDSC>; - iommus = <&apps_smmu 0x420 0x2>, - <&apps_smmu 0x421 0x0>; + iommus = <&apps_smmu 0x420 0x2>; interconnects = <&mmrt_virt MASTER_MDP0 RPM_ALWAYS_TAG &bimc SLAVE_EBI1 RPM_ALWAYS_TAG>, <&bimc MASTER_APPSS_PROC RPM_ALWAYS_TAG
@@ -2339,10 +2337,7 @@ venus: video-codec@5a00000 { memory-region = <&pil_video_mem>; iommus = <&apps_smmu 0x860 0x0>, - <&apps_smmu 0x880 0x0>, - <&apps_smmu 0x861 0x04>, - <&apps_smmu 0x863 0x0>, - <&apps_smmu 0x804 0xe0>; + <&apps_smmu 0x880 0x0>; interconnects = <&mmnrt_virt MASTER_VIDEO_P0 RPM_ALWAYS_TAG &bimc SLAVE_EBI1 RPM_ALWAYS_TAG>,
Signature · 1 line
2.51.0
PATCH 2/2 arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
--- arch/arm64/boot/dts/qcom/agatti.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm64/boot/dts/qcom/agatti.dtsi +2 -2
@@ -298,8 +298,8 @@ reserved_memory: reserved-memory { #size-cells = <2>; ranges; - hyp_mem: hyp@45700000 { - reg = <0x0 0x45700000 0x0 0x600000>; + hyp_mem: hyp@45600000 { + reg = <0x0 0x45600000 0x0 0x700000>; no-map; };
Signature · 1 line
2.51.0
Re: [PATCH 2/2] arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
Quoted 17 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
Signed-off-by Sumit Garg <[email protected]>--- arch/arm64/boot/dts/qcom/agatti.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm64/boot/dts/qcom/agatti.dtsi +0
-0
arch/arm64/boot/dts/qcom/agatti.dtsi reserved_memory: reserved-memory { · line 302
Quoted 7 lines
arch/arm64/boot/dts/qcom/agatti.dtsi +2
-2@@ -298,8 +298,8 @@ reserved_memory: reserved-memory { #size-cells = <2>; ranges; - hyp_mem: hyp@45700000 { - reg = <0x0 0x45700000 0x0 0x600000>; + hyp_mem: hyp@45600000 { + reg = <0x0 0x45600000 0x0 0x700000>;
Losing another 1M of RAM on an often-2G platform hurts a little..
Konrad
Re: [PATCH 2/2] arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
In reply to Sumit GargQuoted 17 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
Signed-off-by Sumit Garg <[email protected]>--- arch/arm64/boot/dts/qcom/agatti.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm64/boot/dts/qcom/agatti.dtsi +0
-0
arch/arm64/boot/dts/qcom/agatti.dtsi reserved_memory: reserved-memory { · line 302
Quoted 7 lines
arch/arm64/boot/dts/qcom/agatti.dtsi +2
-2@@ -298,8 +298,8 @@ reserved_memory: reserved-memory { #size-cells = <2>; ranges; - hyp_mem: hyp@45700000 { - reg = <0x0 0x45700000 0x0 0x600000>; + hyp_mem: hyp@45600000 { + reg = <0x0 0x45600000 0x0 0x700000>;
On a second thought, please add a comment like:
/* Gunyah requires the start of the region to be 2MiB-aligned, QHEE doesn't care */
to prevent someone "fixing" it in the future
Konrad
Re: [PATCH 2/2] arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
Quoted 28 lines · Konrad Dybcio
Quoted 17 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
Signed-off-by Sumit Garg <[email protected]>--- arch/arm64/boot/dts/qcom/agatti.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm64/boot/dts/qcom/agatti.dtsi +0
-0arch/arm64/boot/dts/qcom/agatti.dtsi reserved_memory: reserved-memory { · line 302
Quoted 7 lines
arch/arm64/boot/dts/qcom/agatti.dtsi +2
-2@@ -298,8 +298,8 @@ reserved_memory: reserved-memory { #size-cells = <2>; ranges; - hyp_mem: hyp@45700000 { - reg = <0x0 0x45700000 0x0 0x600000>; + hyp_mem: hyp@45600000 { + reg = <0x0 0x45600000 0x0 0x700000>;On a second thought, please add a comment like:
Quoted 3 lines
/* Gunyah requires the start of the region to be 2MiB-aligned, QHEE doesn't care */
to prevent someone "fixing" it in the future
Sounds reasonable, I will add this comment on the next spin. I hope that I can keep you review tag with comment added.
-Sumit
Re: [PATCH 2/2] arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
Quoted 29 lines · Sumit Garg
Quoted 28 lines · Konrad Dybcio
Quoted 17 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
Signed-off-by Sumit Garg <[email protected]>--- arch/arm64/boot/dts/qcom/agatti.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm64/boot/dts/qcom/agatti.dtsi +0
-0arch/arm64/boot/dts/qcom/agatti.dtsi reserved_memory: reserved-memory { · line 302
Quoted 7 lines
arch/arm64/boot/dts/qcom/agatti.dtsi +2
-2@@ -298,8 +298,8 @@ reserved_memory: reserved-memory { #size-cells = <2>; ranges; - hyp_mem: hyp@45700000 { - reg = <0x0 0x45700000 0x0 0x600000>; + hyp_mem: hyp@45600000 { + reg = <0x0 0x45600000 0x0 0x700000>;On a second thought, please add a comment like:
Quoted 6 lines
Quoted 3 lines
/* Gunyah requires the start of the region to be 2MiB-aligned, QHEE doesn't care */
to prevent someone "fixing" it in the future
Sounds reasonable, I will add this comment on the next spin. I hope that I can keep you review tag with comment added.
Yeah, although as pointed out by the bot you'll need some bindings updates to match
Konrad
Bjorn Andersson
maintainerRe: [PATCH 2/2] arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
In reply to Sumit GargQuoted 7 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
How come this isn't conveyed to the operating system using the UEFI memory map?
Regards,
Bjorn
Quoted 23 lines
Signed-off-by Sumit Garg <[email protected]>--- arch/arm64/boot/dts/qcom/agatti.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm64/boot/dts/qcom/agatti.dtsi +2
-2@@ -298,8 +298,8 @@ reserved_memory: reserved-memory { #size-cells = <2>; ranges; - hyp_mem: hyp@45700000 { - reg = <0x0 0x45700000 0x0 0x600000>; + hyp_mem: hyp@45600000 { + reg = <0x0 0x45600000 0x0 0x700000>; no-map; };Signature · 1 line
2.51.0
Re: [PATCH 2/2] arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
Quoted 12 lines · Bjorn Andersson
Quoted 7 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
How come this isn't conveyed to the operating system using the UEFI memory map?
I agree that with EFI boot, the information is getting conveyed via EFI memory map. But there will be non-EFI boot scenarios as well in case of U-Boot. And moreover I suppose we need to keep the reserved memory ranges in DT updated to reflect the actual memory map.
-Sumit
Re: [PATCH 2/2] arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
Quoted 10 lines · Sumit Garg
Quoted 9 lines · Bjorn Andersson
Quoted 7 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
Quoted 8 lines
Quoted 3 lines
How come this isn't conveyed to the operating system using the UEFI memory map?
I agree that with EFI boot, the information is getting conveyed via EFI memory map. But there will be non-EFI boot scenarios as well in case of U-Boot. And moreover I suppose we need to keep the reserved memory ranges in DT updated to reflect the actual memory map.
Can U-Boot not do the same by altering /reserved-memory or /memory/reg?
Konrad
Re: [PATCH 2/2] arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
Quoted 15 lines · Konrad Dybcio
Quoted 14 lines · Sumit Garg
Quoted 12 lines · Bjorn Andersson
Quoted 7 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
How come this isn't conveyed to the operating system using the UEFI memory map?
Quoted 6 lines
Quoted 4 lines
I agree that with EFI boot, the information is getting conveyed via EFI memory map. But there will be non-EFI boot scenarios as well in case of U-Boot. And moreover I suppose we need to keep the reserved memory ranges in DT updated to reflect the actual memory map.
Can U-Boot not do the same by altering /reserved-memory or /memory/reg?
I suppose you are referring to DT fixups here, we generally try to keep them to a minimum required in U-Boot.
BTW, don't we want to keep reserved memory ranges updated in DT? Or we plan to drop them altogether?
-Sumit
Re: [PATCH 2/2] arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
Quoted 21 lines · Sumit Garg
Quoted 20 lines · Konrad Dybcio
Quoted 18 lines · Sumit Garg
Quoted 12 lines · Bjorn Andersson
Quoted 7 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
How come this isn't conveyed to the operating system using the UEFI memory map?
I agree that with EFI boot, the information is getting conveyed via EFI memory map. But there will be non-EFI boot scenarios as well in case of U-Boot. And moreover I suppose we need to keep the reserved memory ranges in DT updated to reflect the actual memory map.
Quoted 7 lines
Quoted 1 line
Can U-Boot not do the same by altering /reserved-memory or /memory/reg?
I suppose you are referring to DT fixups here, we generally try to keep them to a minimum required in U-Boot.
BTW, don't we want to keep reserved memory ranges updated in DT? Or we plan to drop them altogether?
Generally I believe they're only a necessary evil..
Since Gunyah-on-Agatti doesn't seem to have been released yet, it would seem like a sane idea to fix the issue in the firmware. If it was, well..
On UEFI-enabled targets, we can largely rely on the reservations coming from there, only adding some carveouts for e.g. PIL regions and SMEM (not because they're not resereved, but because we need a pointer to them)
Konrad
Re: [PATCH 2/2] arm64: dts: qcom: agatti: Address Gunyah memory alignment needs
Quoted 30 lines · Konrad Dybcio
Quoted 21 lines · Sumit Garg
Quoted 20 lines · Konrad Dybcio
Quoted 18 lines · Sumit Garg
Quoted 12 lines · Bjorn Andersson
Quoted 7 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Gunyah hypervisor requires it's memory start address to be 2MB aligned. So the address map for Agatti is updated to incorporate that requirement. This should be a backwards compatible DT change which should work with legacy QHEE based firmware stack too.
How come this isn't conveyed to the operating system using the UEFI memory map?
I agree that with EFI boot, the information is getting conveyed via EFI memory map. But there will be non-EFI boot scenarios as well in case of U-Boot. And moreover I suppose we need to keep the reserved memory ranges in DT updated to reflect the actual memory map.
Quoted 7 lines
Quoted 1 line
Can U-Boot not do the same by altering /reserved-memory or /memory/reg?
I suppose you are referring to DT fixups here, we generally try to keep them to a minimum required in U-Boot.
BTW, don't we want to keep reserved memory ranges updated in DT? Or we plan to drop them altogether?
Quoted 9 lines
Generally I believe they're only a necessary evil..
Since Gunyah-on-Agatti doesn't seem to have been released yet, it would seem like a sane idea to fix the issue in the firmware. If it was, well..
On UEFI-enabled targets, we can largely rely on the reservations coming from there, only adding some carveouts for e.g. PIL regions and SMEM (not because they're not resereved, but because we need a pointer to them)
Fair enough, I don't see an immediate need for this from UEFI boot perspective. Let me drop this patch then.
TBH, I am also not a fan of putting the memory regions configuration in DT which can change based on evolving requirments.
-Sumit
Quoted 11 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
Quoted 6 lines
The DT changes should be backwards compatible with legacy QHEE based firmware stack too.
Suggested-by Prakash Gupta <[email protected]>Signed-off-by Sumit Garg <[email protected]>---
This will need to be resolved by something like iommu-map anyway
Konrad
Quoted 10 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
We are porting the underlaying layer. Can we make it to behave in a way similar to the _existing_ software? If DT was a part of the firmware, it would have been fine to perform such updates. But as it is not, we really should try not to break the interface boundary between firmware and the kernel.
I'm fine with the change, but I think we should make sure that Gunyah doesn't break existing device trees.
Quoted 10 lines
The DT changes should be backwards compatible with legacy QHEE based firmware stack too.
Suggested-by Prakash Gupta <[email protected]>Signed-off-by Sumit Garg <[email protected]>--- arch/arm64/boot/dts/qcom/agatti.dtsi | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
Signature · 2 lines
With best wishes Dmitry
Quoted 12 lines · Dmitry Baryshkov
Quoted 10 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
Quoted 5 lines
We are porting the underlaying layer. Can we make it to behave in a way similar to the _existing_ software? If DT was a part of the firmware, it would have been fine to perform such updates. But as it is not, we really should try not to break the interface boundary between firmware and the kernel.
I support your viewpoint. But in this current case Linux kernel VMID mapping for secure context bank stream IDs is incorrect. As Konrad mentioned in the other thread that for secure media streaming use-cases, it rather requires proper representation via iommu-map such that appropriate VMID can be mapped.
Quoted 3 lines
I'm fine with the change, but I think we should make sure that Gunyah doesn't break existing device trees.
Sure, this change isn't breaking any DT ABI but brings compliance to usage of stream IDs.
-Sumit
Quoted 16 lines
Quoted 11 lines
The DT changes should be backwards compatible with legacy QHEE based firmware stack too.
Suggested-by Prakash Gupta <[email protected]>Signed-off-by Sumit Garg <[email protected]>--- arch/arm64/boot/dts/qcom/agatti.dtsi | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)Signature · 2 lines
With best wishes Dmitry
Quoted 19 lines · Sumit Garg
Quoted 12 lines · Dmitry Baryshkov
Quoted 10 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
Quoted 5 lines
We are porting the underlaying layer. Can we make it to behave in a way similar to the _existing_ software? If DT was a part of the firmware, it would have been fine to perform such updates. But as it is not, we really should try not to break the interface boundary between firmware and the kernel.
Quoted 4 lines
I support your viewpoint. But in this current case Linux kernel VMID mapping for secure context bank stream IDs is incorrect. As Konrad mentioned in the other thread that for secure media streaming use-cases, it rather requires proper representation via iommu-map such that appropriate VMID can be mapped.
Yes, I understand this part. I'm basically suggesting that Gunyah should ignore this SID when programming the actual SMMU (probably like QHEE did).
Quoted 7 lines
Quoted 4 lines
I'm fine with the change, but I think we should make sure that Gunyah doesn't break existing device trees.
Sure, this change isn't breaking any DT ABI but brings compliance to usage of stream IDs.
Actually, it does, as you can see from Rob's email. You didn't perform make dtbs_check while submitting this patch.
Signature · 2 lines
With best wishes Dmitry
Quoted 20 lines · Dmitry Baryshkov
Quoted 19 lines · Sumit Garg
Quoted 17 lines · Dmitry Baryshkov
Quoted 10 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
We are porting the underlaying layer. Can we make it to behave in a way similar to the _existing_ software? If DT was a part of the firmware, it would have been fine to perform such updates. But as it is not, we really should try not to break the interface boundary between firmware and the kernel.
Quoted 8 lines
Quoted 4 lines
I support your viewpoint. But in this current case Linux kernel VMID mapping for secure context bank stream IDs is incorrect. As Konrad mentioned in the other thread that for secure media streaming use-cases, it rather requires proper representation via iommu-map such that appropriate VMID can be mapped.
Yes, I understand this part. I'm basically suggesting that Gunyah should ignore this SID when programming the actual SMMU (probably like QHEE did).
TBH, I generally dislike as to why the firmware based hypervisor behaviour should be encoded in the DT. One can always argue that Gunyah is doing the right thing given the purpose of SMMU stream IDs. And if Gunyah has to change that behaviour since Agatti DT was based on QHEE behaviour then the same behaviour will get replicated on all other Qcom SoCs.
So, I would suggest that kernel should follow the expected behaviour which is to not program HLOS VMID for stream IDs dedicated for secure context bank.
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
I'm fine with the change, but I think we should make sure that Gunyah doesn't break existing device trees.
Sure, this change isn't breaking any DT ABI but brings compliance to usage of stream IDs.
Actually, it does, as you can see from Rob's email. You didn't perform make
dtbs_checkwhile submitting this patch.
Okay, I missed that. Looks like the GPU DT node already defined minItems for the iommus property but the others didn't. I will add following change in the next version to fix the warnings.
Irrespective on that, the DT forwards and backwards ABI compatibility is being maintained with the changes being proposed in this patch-set.
Documentation/devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml +1 -0
@@ -33,6 +33,7 @@ properties: - const: core iommus: + minItems: 1 maxItems: 2 interconnects:
Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml +1 -0
@@ -42,6 +42,7 @@ properties: - const: vcodec0_bus iommus: + minItems: 2 maxItems: 5 interconnects:
-Sumit
Bjorn Andersson
maintainerQuoted 26 lines · Sumit Garg
Quoted 25 lines · Dmitry Baryshkov
Quoted 23 lines · Sumit Garg
Quoted 12 lines · Dmitry Baryshkov
Quoted 10 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
Quoted 5 lines
We are porting the underlaying layer. Can we make it to behave in a way similar to the _existing_ software? If DT was a part of the firmware, it would have been fine to perform such updates. But as it is not, we really should try not to break the interface boundary between firmware and the kernel.
I support your viewpoint. But in this current case Linux kernel VMID mapping for secure context bank stream IDs is incorrect. As Konrad mentioned in the other thread that for secure media streaming use-cases, it rather requires proper representation via iommu-map such that appropriate VMID can be mapped.
Quoted 6 lines
Quoted 3 lines
Yes, I understand this part. I'm basically suggesting that Gunyah should ignore this SID when programming the actual SMMU (probably like QHEE did).
TBH, I generally dislike as to why the firmware based hypervisor behaviour should be encoded in the DT.
The DeviceTree describes the world to the operating system. If there is a hypervisor (or secure world, or other configuration) that is affecting the (hardware or firmware) interfaces that needs to either be discoverable at runtime, or represented in the description of those interfaces.
Quoted 6 lines
One can always argue that Gunyah
is doing the right thing given the purpose of SMMU stream IDs. And if Gunyah has to change that behaviour since Agatti DT was based on QHEE behaviour then the same behaviour will get replicated on all other Qcom SoCs.
Yes, you can see that mess in sdm845, sc7180, sc7280, and the various *-el2.dtso; all cases where we need to describe the variations of the hardware and firmware interfaces presented to the operating system.
Quoted 4 lines
So, I would suggest that kernel should follow the expected behaviour which is to not program HLOS VMID for stream IDs dedicated for secure context bank.
I agree. As discussed elsewhere, we might still need to manage the secure context banks in some cases, but that needs special handling anyways.
Moreover, it's two context banks (unsecure and secure), but the current description defines a single context bank capturing both the unsecure and secure streams.
Regards,
Bjorn
Quoted 45 lines
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
I'm fine with the change, but I think we should make sure that Gunyah doesn't break existing device trees.
Sure, this change isn't breaking any DT ABI but brings compliance to usage of stream IDs.
Actually, it does, as you can see from Rob's email. You didn't perform make
dtbs_checkwhile submitting this patch.Okay, I missed that. Looks like the GPU DT node already defined minItems for the iommus property but the others didn't. I will add following change in the next version to fix the warnings.
Irrespective on that, the DT forwards and backwards ABI compatibility is being maintained with the changes being proposed in this patch-set.
Documentation/devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml +1
-0@@ -33,6 +33,7 @@ properties: - const: core iommus: + minItems: 1 maxItems: 2 interconnects:Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml +1
-1@@ -42,6 +42,7 @@ properties: - const: vcodec0_bus iommus: + minItems: 2 maxItems: 5 interconnects: -Sumit
Quoted 31 lines · Bjorn Andersson
Quoted 30 lines · Sumit Garg
Quoted 28 lines · Dmitry Baryshkov
Quoted 19 lines · Sumit Garg
Quoted 12 lines · Dmitry Baryshkov
Quoted 10 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
Quoted 5 lines
We are porting the underlaying layer. Can we make it to behave in a way similar to the _existing_ software? If DT was a part of the firmware, it would have been fine to perform such updates. But as it is not, we really should try not to break the interface boundary between firmware and the kernel.
Quoted 4 lines
I support your viewpoint. But in this current case Linux kernel VMID mapping for secure context bank stream IDs is incorrect. As Konrad mentioned in the other thread that for secure media streaming use-cases, it rather requires proper representation via iommu-map such that appropriate VMID can be mapped.
Yes, I understand this part. I'm basically suggesting that Gunyah should ignore this SID when programming the actual SMMU (probably like QHEE did).
Quoted 8 lines
Quoted 2 lines
TBH, I generally dislike as to why the firmware based hypervisor behaviour should be encoded in the DT.
The DeviceTree describes the world to the operating system. If there is a hypervisor (or secure world, or other configuration) that is affecting the (hardware or firmware) interfaces that needs to either be discoverable at runtime, or represented in the description of those interfaces.
I would always favour discoverable firmware interfaces like EFI or TEE/TZ based interfaces rather than encoding such information in the DT.
Quoted 11 lines
Quoted 7 lines
One can always argue that Gunyah
is doing the right thing given the purpose of SMMU stream IDs. And if Gunyah has to change that behaviour since Agatti DT was based on QHEE behaviour then the same behaviour will get replicated on all other Qcom SoCs.Yes, you can see that mess in sdm845, sc7180, sc7280, and the various *-el2.dtso; all cases where we need to describe the variations of the hardware and firmware interfaces presented to the operating system.
AFAIU, the major problem here is that the DT for Qcom platforms is a subset of the hardware information for the OS to initially only run at non-secure EL1. Then came in requirment that we need to run Linux at EL2 for which the information wasn't there in DT leading to this *-el2.dtso mess.
I would rather like to see the DT descibing the overall hardware applicable to the apps processor rather than a subset such that the TZ, Hypervisor and the kernel can consume the same DT based on ownership hints.
I suppose the Qcom IOMMU driver can be the first one to consume such DT information based on the hints. However, this is something which is out of scope for this patch-set which tries to fix DT as per existing expectations.
Quoted 6 lines
Quoted 5 lines
So, I would suggest that kernel should follow the expected behaviour which is to not program HLOS VMID for stream IDs dedicated for secure context bank.
Quoted 7 lines
I agree. As discussed elsewhere, we might still need to manage the secure context banks in some cases, but that needs special handling anyways.
Moreover, it's two context banks (unsecure and secure), but the current description defines a single context bank capturing both the unsecure and secure streams.
Agree, I will spin up a v2 with DT bindings changes.
-Sumit
Quoted 20 lines · Dmitry Baryshkov
Quoted 19 lines · Sumit Garg
Quoted 17 lines · Dmitry Baryshkov
Quoted 10 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
We are porting the underlaying layer. Can we make it to behave in a way similar to the _existing_ software? If DT was a part of the firmware, it would have been fine to perform such updates. But as it is not, we really should try not to break the interface boundary between firmware and the kernel.
Quoted 8 lines
Quoted 4 lines
I support your viewpoint. But in this current case Linux kernel VMID mapping for secure context bank stream IDs is incorrect. As Konrad mentioned in the other thread that for secure media streaming use-cases, it rather requires proper representation via iommu-map such that appropriate VMID can be mapped.
Yes, I understand this part. I'm basically suggesting that Gunyah should ignore this SID when programming the actual SMMU (probably like QHEE did).
But the existing DT description for Agatti GPU is buggy. SID 2 is for secure access from GPU and it should not be mixed with non-secure access. We haven't hit any issue so far because the KMD doesn't have support for secure usecase yet.
At least for the GPU property, I feel that the right thing to do is to correct the buggy description.
-Akhil
Quoted 12 lines
Quoted 8 lines
Quoted 5 lines
I'm fine with the change, but I think we should make sure that Gunyah doesn't break existing device trees.
Sure, this change isn't breaking any DT ABI but brings compliance to usage of stream IDs.
Actually, it does, as you can see from Rob's email. You didn't perform make
dtbs_checkwhile submitting this patch.
Quoted 30 lines · Akhil P Oommen
Quoted 20 lines · Dmitry Baryshkov
Quoted 19 lines · Sumit Garg
Quoted 17 lines · Dmitry Baryshkov
Quoted 10 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
We are porting the underlaying layer. Can we make it to behave in a way similar to the _existing_ software? If DT was a part of the firmware, it would have been fine to perform such updates. But as it is not, we really should try not to break the interface boundary between firmware and the kernel.
Quoted 8 lines
Quoted 4 lines
I support your viewpoint. But in this current case Linux kernel VMID mapping for secure context bank stream IDs is incorrect. As Konrad mentioned in the other thread that for secure media streaming use-cases, it rather requires proper representation via iommu-map such that appropriate VMID can be mapped.
Yes, I understand this part. I'm basically suggesting that Gunyah should ignore this SID when programming the actual SMMU (probably like QHEE did).
Quoted 7 lines
But the existing DT description for Agatti GPU is buggy. SID 2 is for secure access from GPU and it should not be mixed with non-secure access. We haven't hit any issue so far because the KMD doesn't have support for secure usecase yet.
At least for the GPU property, I feel that the right thing to do is to correct the buggy description.
I do not doubt the change, we should not be mapping secure SIDs. However I very much dislike the fact that changing the bootloader makes us perform this change.
Quoted 16 lines
-Akhil
Quoted 12 lines
Quoted 8 lines
Quoted 5 lines
I'm fine with the change, but I think we should make sure that Gunyah doesn't break existing device trees.
Sure, this change isn't breaking any DT ABI but brings compliance to usage of stream IDs.
Actually, it does, as you can see from Rob's email. You didn't perform make
dtbs_checkwhile submitting this patch.
Signature · 2 lines
With best wishes Dmitry
Rob Herring
maintainerQuoted 11 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
Quoted 9 lines
The DT changes should be backwards compatible with legacy QHEE based firmware stack too.
Suggested-by Prakash Gupta <[email protected]>Signed-off-by Sumit Garg <[email protected]>--- arch/arm64/boot/dts/qcom/agatti.dtsi | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
My bot found new DTB warnings on the .dts files added or changed in this series.
Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments.
If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date:
pip3 install dtschema --upgrade
This patch series was applied (using b4) to base:
Base: attempting to guess base-commit...
Base: tags/next-20251219 (exact match)
Base: tags/next-20251219 (use --merge-base to override)
If this is not the correct base, please add 'base-commit' tag (or use b4 which does this automatically)
New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/qcom/' for [email protected]:
arch/arm64/boot/dts/qcom/qrb2210-rb1.dtb: display-subsystem@5e00000 (qcom,qcm2290-mdss): iommus: [[38, 1056, 2]] is too short
from schema $id: http://devicetree.org/schemas/display/msm/qcom,qcm2290-mdss.yamlarch/arm64/boot/dts/qcom/qrb2210-rb1.dtb: video-codec@5a00000 (qcom,qcm2290-venus): iommus: [[38, 2144, 0], [38, 2176, 0]] is too short
from schema $id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml
Quoted 11 lines · Sumit Garg
From: Sumit Garg <[email protected]>
Fix IOMMU DT propeties for GPU, display and video peripherals via dropping SMMU stream IDs which relates to secure context bank.
This problem only surfaced when the Gunyah based firmware stack is ported on Agatti replacing the legacy QHEE based firmware stack. Assigning Linux kernel (HLOS) VMID to secure context bank stream IDs is treated as a fault by Gunyah hypervisor which were previously ignored by QHEE hypervisor.
Quoted 5 lines
The DT changes should be backwards compatible with legacy QHEE based firmware stack too.
Suggested-by Prakash Gupta <[email protected]>Signed-off-by Sumit Garg <[email protected]>
-Akhil
Quoted 40 lines
--- arch/arm64/boot/dts/qcom/agatti.dtsi | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)arch/arm64/boot/dts/qcom/agatti.dtsi +3
-8@@ -1613,8 +1613,7 @@ gpu: gpu@5900000 { &bimc SLAVE_EBI1 RPM_ALWAYS_TAG>; interconnect-names = "gfx-mem"; - iommus = <&adreno_smmu 0 1>, - <&adreno_smmu 2 0>; + iommus = <&adreno_smmu 0 1>; operating-points-v2 = <&gpu_opp_table>; power-domains = <&rpmpd QCM2290_VDDCX>; qcom,gmu = <&gmu_wrapper>;@@ -1895,8 +1894,7 @@ mdss: display-subsystem@5e00000 { power-domains = <&dispcc MDSS_GDSC>; - iommus = <&apps_smmu 0x420 0x2>, - <&apps_smmu 0x421 0x0>; + iommus = <&apps_smmu 0x420 0x2>; interconnects = <&mmrt_virt MASTER_MDP0 RPM_ALWAYS_TAG &bimc SLAVE_EBI1 RPM_ALWAYS_TAG>, <&bimc MASTER_APPSS_PROC RPM_ALWAYS_TAG@@ -2339,10 +2337,7 @@ venus: video-codec@5a00000 { memory-region = <&pil_video_mem>; iommus = <&apps_smmu 0x860 0x0>, - <&apps_smmu 0x880 0x0>, - <&apps_smmu 0x861 0x04>, - <&apps_smmu 0x863 0x0>, - <&apps_smmu 0x804 0xe0>; + <&apps_smmu 0x880 0x0>; interconnects = <&mmnrt_virt MASTER_VIDEO_P0 RPM_ALWAYS_TAG &bimc SLAVE_EBI1 RPM_ALWAYS_TAG>,