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>
---
 arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
index 8b924812322c..326f84f746cb 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
@@ -68,6 +68,16 @@ l2: l2-cache0 {
 		};
 	};
 
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		cfe-stub@0 {
+			reg = <0 0 0 0x10000>;
+		};
+	};
+
 	axi@81000000 {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.44.2

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?

-- 
Florian

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.

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

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