| Message ID | 20241003213007.1339811-1-CFSworks@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: broadcom: bcmbca: bcm4908: Reserve CFE stub area | expand |
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?
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!
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>;
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(+)