Message-ID: <CAH5Ym4gUfhh3ZkPY8i8eVNzWOpfk+ibqAgdPuT3i0H0FsF5t8Q@mail.gmail.com>
Date: Fri, 4 Oct 2024 11:23:34 -0700
From: Sam Edwards <cfsworks@...il.com>
To: Florian Fainelli <florian.fainelli@...adcom.com>
Cc: Rafał Miłecki <rafal@...ecki.pl>, 
	William Zhang <william.zhang@...adcom.com>, Anand Gore <anand.gore@...adcom.com>, 
	Kursad Oney <kursad.oney@...adcom.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: dts: broadcom: bcmbca: bcm4908: Reserve CFE stub area

On Thu, Oct 3, 2024 at 3:41 PM Florian Fainelli
<florian.fainelli@...adcom.com> wrote:
>
> On 10/3/24 14:30, Sam Edwards wrote:
> > The CFE bootloader places a stub program at 0x0000-0xFFFF to hold the
> > secondary CPUs until the boot CPU writes the release address. If Linux
> > overwrites this program before execution reaches smp_prepare_cpus(), the
> > secondary CPUs may become inaccessible.
> >
> > This is only a problem with CFE, and then only until the secondary CPUs
> > are brought online. However, since it is such a small amount of memory,
> > it is easiest to reserve it unconditionally.
> >
> > Therefore, add a /reserved-memory node to bcm4908.dtsi to protect this
> > critical memory region.
> >
> > Signed-off-by: Sam Edwards <CFSworks@...il.com>
>
> Not objecting to the solution, but should not this be moved to a
> per-board DTS given that there are boards using CFE, and some using
> u-boot + ARM TF that are unlikely to suffer from that problem?

Hi Florian,

I think I share your same gut feeling: this is bootloader-reserved
memory, not something claimed by a driver or belonging to a device. If
the bootloader is going to leave some code or structures resident in
memory after handing off control to Linux, it's the responsibility of
the bootloader to claim that memory by splicing in a reserved-memory
DT node, and CFE isn't doing that. So I think we're very much in
"Linux-side workaround for a proprietary-blob bug" territory.

I don't know if it makes much more sense to put this in the
board-specific .dts files; as I understand it, the architecture of CFE
is somewhat unique in that CFERAM (containing the actual "bootloader"
part) is included in the firmware image. That means that whether CFE
or CFEROM-loaded-U-Boot is the thing kicking off Linux is up to the
creator of the firmware image, rather than the device manufacturer.

My reasoning for including this in the SoC-level .dtsi is threefold:
- The .dtsi is specifying enable-method and cpu-release-addr for the
CPUs, which also concern the Linux-to-bootloader protocol and should
customarily be synthesized by the bootloader. U-Boot picks "psci,"
overriding the FDT-specified default: so the .dtsi is already assuming
CFE.
- The .dtsi is also picking 0xfff8 as the fixed location to put the
secondary-core entry point. I've noticed that CFE walks the FDT to
learn cpu-release-addr (rather than writing the property): so the
.dtsi is also already assuming that this region of memory is reserved;
this patch just makes that explicit.
- 64K of reserved memory is so tiny compared to the hundreds of MBs
typically available on these boards, so I felt that the unconditional
memory cost was an acceptable trade-off to save affected users the
troubleshooting.

If you happen to know of a DT property that tells Linux to unreserve
the memory once fully booted, I'd gladly use that, but I didn't find
such a thing when I looked.

Since CFE's stub program appears to be very small, would you be more
amenable to a patch that moves the address at 0xfff8 to 0xff8 and
reserves only 4K (one page) instead? I hadn't thought to try it before
now but it should work.

Best,
Sam

>
> --
> Florian

Powered by blists - more mailing lists