[PATCH v3 5/5] Add two entries:

[Jessica Clarke]

On 25 Jan 2023, at 20:53, Lad, Prabhakar <prabhakar.csengg at gmail.com> wrote:
> 
> 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

Are you sure you tested the new file? That looks like the exact change
I was intending and should have an effect at run time on platforms that
load OpenSBI to an address other than 0x80000000 (well, whatever the
config set FW_TEXT_START to, which is that for generic by default).

Jess