[PATCH v3 5/5] Add two entries:

[Jessica Clarke]

On 25 Jan 2023, at 20:56, Jessica Clarke <jrtc27 at jrtc27.com> wrote:
> 
> 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).

I guess you could be getting further and hanging for some other reason.
Despite the sbi_printf calls, sbi_domain_init will never actually print
anything, because it’s run before sbi_console_init and thus the prints
are just slow no-ops.

What do you see with the problematic patch reverted but my fix applied?

Jess