Message-ID: <CALHNRZ-YZg3cKzRBMGaxRpejFMLSpOOz-FPQEaQVXFpFao40WA@mail.gmail.com>
Date: Sun, 20 Apr 2025 20:45:38 -0500
From: Aaron Kling <webgeek1234@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Thierry Reding <thierry.reding@...il.com>, 
	Jonathan Hunter <jonathanh@...dia.com>, Kees Cook <kees@...nel.org>, Tony Luck <tony.luck@...el.com>, 
	"Guilherme G. Piccoli" <gpiccoli@...lia.com>, devicetree@...r.kernel.org, 
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

On Tue, Apr 8, 2025 at 3:49 AM Aaron Kling <webgeek1234@...il.com> wrote:
>
> On Tue, Apr 8, 2025 at 3:17 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> >
> > On 08/04/2025 09:35, Aaron Kling wrote:
> > > On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> > >>
> > >> On 07/04/2025 18:00, Aaron Kling wrote:
> > >>> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> > >>>>
> > >>>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> > >>>>> From: Aaron Kling <webgeek1234@...il.com>
> > >>>>>
> > >>>>> This allows using pstore on all such platforms. There are some
> > >>>>> differences per arch:
> > >>>>>
> > >>>>> * Tegra132: Flounder does not appear to enumerate pstore and I do not
> > >>>>>   have access to norrin, thus Tegra132 is left out of this commit.
> > >>>>> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
> > >>>>>   relying on a dowstream driver to allocate the carveout, hence this
> > >>>>>   hardcodes a location matching what the downstream driver picks.
> > >>>>> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
> > >>>>>   size in a node specifically named /reserved-memory/ramoops_carveout,
> > >>>>>   thus these cannot be renamed.
> > >>>>> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
> > >>>>>   compatible, however the dt still does not know the address, so keeping
> > >>>>>   the node name consistent on Tegra186 and newer.
> > >>>>>
> > >>>>> Signed-off-by: Aaron Kling <webgeek1234@...il.com>
> > >>>>> ---
> > >>>>>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
> > >>>>>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
> > >>>>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
> > >>>>>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
> > >>>>>  4 files changed, 61 insertions(+)
> > >>>>>
> > >>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > >>>>> index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
> > >>>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > >>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > >>>>> @@ -2051,6 +2051,22 @@ pmu-denver {
> > >>>>>               interrupt-affinity = <&denver_0 &denver_1>;
> > >>>>>       };
> > >>>>>
> > >>>>> +     reserved-memory {
> > >>>>> +             #address-cells = <2>;
> > >>>>> +             #size-cells = <2>;
> > >>>>> +             ranges;
> > >>>>> +
> > >>>>> +             ramoops_carveout {
> > >>>>
> > >>>> Please follow DTS coding style for name, so this is probably only ramoops.
> > >>>
> > >>> As per the commit message regarding tegra186: bootloader fills in the
> > >>> address and size in a node specifically named
> > >>> /reserved-memory/ramoops_carveout, thus these cannot be renamed.
> > >>
> > >> That's not a reason to introduce issues. Bootloader is supposed to
> > >> follow same conventions or use aliases or labels (depending on the node).
> > >>
> > >> If bootloader adds junk, does it mean we have to accept that junk?
> > >>
> > >>>
> > >>>>
> > >>>> It does not look like you tested the DTS against bindings. Please run
> > >>>> `make dtbs_check W=1` (see
> > >>>> Documentation/devicetree/bindings/writing-schema.rst or
> > >>>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> > >>>> for instructions).
> > >>>> Maybe you need to update your dtschema and yamllint. Don't rely on
> > >>>> distro packages for dtschema and be sure you are using the latest
> > >>>> released dtschema.
> > >>>
> > >>> The bot is reporting that the reg field is missing from the added
> > >>> ramoops nodes on t186, t194, and t234. However, as also mentioned in
> > >>> the commit message, this is intentional because it is expected for the
> > >>> bootloader to fill that in. It is not known at dt compile time. Is
> > >>> there a way to mark this as intentional, so dtschema doesn't flag it?
> > >>
> > >> Fix your bootloader or chain load some normal one, like U-Boot.
> > > How would chainloading a second bootloader 'fix' previous stage
> > > bootloaders trampling on an out-of-sync hardcoded reserved-memory
> > > address? It's possible for carveout addresses and sizes to change. Not
> > > from boot to boot on the same version of the Nvidia bootloader, but
> > > potentially from one version to another. Depending on if the
> > > bootloader was configured with different carveout sizes.
> > >
> > > There is precedence for this. When blind cleanup was done on arm
> > > device trees, a chromebook broke because the memory node has to be
> > > named exactly '/memory' [0]. How is this any different from that case?
> >
> > That was an existing node, so ABI.
> >
> > > These nodes are an ABI to an existing bootloader. Carveouts on these
> >
> > You add new ABI, which I object to.
> >
> > > archs are set up in bl1 or bl2, which are not source available. I
> > > could potentially hardcode things for myself in bl33, which is source
> > > available, but the earlier stages could still overwrite any chosen
> > > block depending on how carveouts are configured. But even then, that
> > > will not change the behaviour of the vast majority of units that use a
> > > fully prebuilt boot stack direct from Nvidia. My intent here is for
> > > pstore to work on such units without users needing to use a custom
> > > bootloader.
> > I understand your goal. What I still do not understand, why bootloader
> > even bothers with ramoops carveout. It shouldn't and you should just
> > ignore whatever bootloader provides, no?
>
> Mmm, I actually don't have the answer to this. Ramoops carveout
> handling was added to t186 and t194 in cboot for L4T r32.7.3, fairly
> late in the life cycle. But it has always been in edk2 for t194 and
> t234 afaik. I could hazard some guesses, but don't have any
> documentation on why the decision was made. Maybe Thierry or Jonathan
> could chime in on why this was done.
>

Friendly reminder to the Tegra maintainers about this question.

Sincerely,
Aaron Kling

Powered by blists - more mailing lists