Message-ID: <CALHNRZ--ZUxqrXHEnizXC8ddHC5LFA10oH+CgQmOcTt+cJ1CWw@mail.gmail.com>
Date: Wed, 28 May 2025 12:35:16 -0500
From: Aaron Kling <webgeek1234@...il.com>
To: Thierry Reding <thierry.reding@...il.com>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	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 Thu, May 8, 2025 at 4:27 PM Thierry Reding <thierry.reding@...il.com> wrote:
>
> On Mon, Apr 28, 2025 at 08:21:55PM -0500, Aaron Kling wrote:
> > On Sun, Apr 20, 2025 at 8:45 PM Aaron Kling <webgeek1234@...il.com> wrote:
> > >
> > > 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.
> > >
> > In lieu of a response from the Tegra subsystem maintainers, I can only
> > hazard an assumption, Krzysztof. I presume the pstore carveout is
> > bootloader controlled because various stages of the boot stack can
> > dynamically allocate memory, and this became bootloader controlled to
> > prevent any of those from overwriting pstore. I worry about hardcoding
> > an address in the kernel dt, then finding out later that there's an
> > in-use configuration that overwrites or corrupts that section of ram
> > during boot. What are your thoughts on this? And is there any way for
> > this patch to proceed?
>
> I haven't been able to find anything out about this yet. Generally it's
> difficult to get the bootloaders updated for these devices. Tegra194 and
> Tegra234 may be new enough to make an update eventually go into a
> release, but for Tegra186 and older, I honestly wouldn't hold my
> breath.
>
> Thierry

Krzysztof, based on this response, is there any way or form that the
Tegra186 part of this could be submitted? I can drop the newer
platforms from this patch if Thierry can get a response to his other
reply about how the bootloader could conform.

Sincerely,
Aaron

Powered by blists - more mailing lists