[PATCH v3 5/5] Add two entries:

[Lad, Prabhakar]

Hi,

Thank you for the feedback.

On Wed, Jan 25, 2023 at 8:36 PM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>
> On 25 Jan 2023, at 20:14, Jessica Clarke <jrtc27 at jrtc27.com> wrote:
> >
> > On 25 Jan 2023, at 19:42, Lad, Prabhakar <prabhakar.csengg at gmail.com> wrote:
> >>
> >> Hi Himanshu,
> >>
> >> On Thu, Jan 19, 2023 at 3:19 PM Himanshu Chauhan
> >> <hchauhan at ventanamicro.com> wrote:
> >>>
> >>> 1. TEXT: fw_start to _fw_rw_offset with RX permissions
> >>> 2. DATA: _fw_rw_offset to fw_size with RW permissions
> >>>
> >>> These permissions are still not enforced from M-mode but lay
> >>> the ground work for enforcing them for M-mode. SU-mode don't
> >>> have any access to these regions.
> >>>
> >>> Sample output:
> >>> Domain0 Region01  : 0x0000000080000000-0x000000008001ffff M: (R,X) S/U: ()
> >>> Domain0 Region02  : 0x0000000080020000-0x000000008003ffff M: (R,W) S/U: ()
> >>>
> >>> Signed-off-by: Himanshu Chauhan <hchauhan at ventanamicro.com>
> >>> ---
> >>> lib/sbi/sbi_domain.c | 24 ++++++++++++++++++++++--
> >>> 1 file changed, 22 insertions(+), 2 deletions(-)
> >>>
> >> After updating to the latest master, RZ/Five SoC doesn't boot up.
> >> Bisecting pointed me to this commit 230278d "lib: sbi: Add separate
> >> entries for firmware RX and RW regions"
> >>
> >> I get no prints from OpenSBI ; the only prints I get are from SPL.
> >> After reverting this patch I get RZ/Five working as expected (log
> >> below). Do you see any reason why this fails on RZ/Five ?
> >>
> >> U-Boot SPL 2023.01-00206-g1026336bc6 (Jan 25 2023 - 19:36:23 +0000)
> >> Trying to boot from NOR
> >>
> >> OpenSBI v1.2-40-g031b6db
> >>  ____                    _____ ____ _____
> >> / __ \                  / ____|  _ \_   _|
> >> | |  | |_ __   ___ _ __ | (___ | |_) || |
> >> | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> >> | |__| | |_) |  __/ | | |____) | |_) || |_
> >> \____/| .__/ \___|_| |_|_____/|____/_____|
> >>       | |
> >>       |_|
> >>
> >> Platform Name             : Renesas SMARC EVK based on r9a07g043f01
> >> Platform Features         : medeleg
> >> Platform HART Count       : 1
> >> Platform IPI Device       : andes_plicsw
> >> Platform Timer Device     : andes_plmt @ 12000000Hz
> >> Platform Console Device   : renesas_scif
> >> Platform HSM Device       : ---
> >> Platform PMU Device       : ---
> >> Platform Reboot Device    : ---
> >> Platform Shutdown Device  : ---
> >> Firmware Base             : 0x44000000
> >> Firmware Size             : 228 KB
> >> Firmware RW Offset        : 0xffffffffc4020000
> >
> > This is clearly nonsense. What does readelf -Ws show for _fw_rw_offset?
> >
> > Worth noting that 0x44000000 + 0xffffffffc4020000 = 0x8020000, so it
> > sounds like relocation inconsistency to me somehow…
>
> Ok, figured it out, this is an LLD vs BFD linker script difference. For
> whatever reason, BFD decides that . - _fw_start is not an absolute
> address, whereas LLD gives you the more sensible (or at least what we
> want here) behaviour, probably because . = in the various output
> section statements makes it a relative address, but _fw_start remains
> absolute, and so it thinks you’re trying to compute relative -
> absolute, which gives a relative address. This then means that the
> value located at __fw_rw_offset is subject to relocation and so loading
> at an address other than 0x80000000 breaks things like you see (your
> observed _fw_rw_offset value is out by 0x44000000 - 0x80000000). The
> following diff should fix it (I’ve verified it with a toy example with
> BFD that reproduced the symbol not being absolute); let me know if it
> does and I’ll make a proper patch out of it.
>
> Jess
>
> diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS
> index 9a1304e..918357d 100644
> --- a/firmware/fw_base.ldS
> +++ b/firmware/fw_base.ldS
> @@ -64,7 +64,7 @@
>         . = ALIGN(1 << LOG2CEIL((SIZEOF(.rodata) + SIZEOF(.text)
>                                 + SIZEOF(.dynsym) + SIZEOF(.rela.dyn))));
>
> -       PROVIDE(_fw_rw_offset = (.  - _fw_start));
> +       PROVIDE(_fw_rw_offset = ABSOLUTE(. - _fw_start));
>
>         /* Beginning of the read-write data sections */
>
>
I did try the above diff and was able to see the same issue.

After this change:
readelf -Ws build/platform/generic/firmware/fw_dynamic.elf | grep -w
_fw_rw_offset
   805: 0000000000020000     0 NOTYPE  GLOBAL DEFAULT  ABS _fw_rw_offset

Before this change:
$ readelf -Ws build/platform/generic/firmware/fw_dynamic.elf | grep -w
_fw_rw_offset
   805: 0000000000020000     0 NOTYPE  GLOBAL DEFAULT    8 _fw_rw_offset

Cheers,
Prabhakar