Re: [PATCH] arm64: dts: broadcom: bcmbca: bcm4908: Reserve CFE stub area

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 10/4/24 11:23, Sam Edwards wrote:
> On Thu, Oct 3, 2024 at 3:41 PM Florian Fainelli
> <florian.fainelli@broadcom.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@gmail.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.

Not aware of such a thing, and I am not questioning the need to reserve 
memory, that need is quite clear. What I was questioning is making this 
a SoC specific entry because we do have a variety of boards supported 
out there, some with CFE, some with u-boot.

I suppose it is safer that way however, regardless of the boot loader 
being used, and therefore I have no problem taking this patch as-is.

> 
> 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.

If a smaller reservation works, sure, why not!
-- 
Florian