Re: [PATCH v5 08/10] arm64: tegra: Add VPR placeholder node on Tegra234

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The DT template comments out the `no-map` property to satisfy schema validation for resizable VPRs, but fails to instruct the firmware to add it back for fixed VPRs. Without `no-map`, the kernel will map the hardware-protected Video Protection Region into the CPU linear map, leading to fatal SError panics from speculative execution.
- [Medium] Mismatch between the commit message instructions and the DT template regarding the `reg` property. The commit message instructs firmware to remove `reg` for resizable VPRs, but it has already been commented out in the code.
--

--- Patch [8]: [PATCH v5 08/10] arm64: tegra: Add VPR placeholder node on Tegra234 ---
commit 19202d4b4a42362e9866fdb06d4d5ebfaf462002
Author: Thierry Reding <[email protected]>

arm64: tegra: Add VPR placeholder node on Tegra234

This commit adds a placeholder device tree node for the Video Protection Region
(VPR) on Tegra234. It provides properties for both fixed and resizable VPR
configurations, expecting early firmware to finalize the node.

> Similarly, if the VPR is
> resizable, the firmware should remove the "reg" property since it is no
> longer needed.

[Severity: Medium]
Is this commit message instruction accurate for the final version of the patch?

The DT template in arch/arm64/boot/dts/nvidia/tegra234.dtsi comments out the
`reg` property by default. If early firmware strictly follows this commit
message and attempts to delete the `reg` property (e.g., using fdt_delprop()),
could it fail and potentially abort boot if it doesn't handle the missing
property gracefully?

> diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> index 8e0c51e496e20..52ff11873580d 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> @@ -29,6 +29,45 @@ aliases {

[ ... ]

> +			/*
> +			 * The below assumes resizable VPR by default. If the
> +			 * firmwares sets up fixed VPR, it is responsible for
> +			 * adding the missing "reg" property, removing any of
> +			 * the unused properties, as well as adding a unit-
> +			 * address matching the "reg" property.
> +			 */
> +
> +			/* fixed VPR */
> +			/*
> +			reg = <0x0 0x0 0x0 0x0>;
> +			no-map;
> +			*/

[Severity: High]
Does the firmware also need to be instructed to add the `no-map` property back
for fixed VPR configurations?

Since `no-map` is completely commented out and omitted from the instructional
comment in the vpr node, firmware might not add it. Without `no-map`, the
kernel's of_reserved_mem_init() will use memblock_reserve() instead of
memblock_mark_nomap().

Could this cause a regression where the kernel maps the hardware-protected Video
Protection Region into the CPU linear map, leading to a fatal SError panic from
speculative execution?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8