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
