[PATCH v4] firmware: remove FW_TEXT_START

[Clément Léger]

On 14/05/2024 19:46, Jessica Clarke wrote:
> On 14 May 2024, at 18:25, Cheng Yang <yangcheng.work at foxmail.com> wrote:
>>
>> On 14 May 2024, at 17:50, Cheng Yang <yangcheng.work at="" foxmail.com=""> wrote:
>> >> 
>> >> >On Tue, May 14, 2024 at 9:58 PM Clément Léger <cleger at="" rivosinc.com=""> wrote:
>> >> >>
>> >> >>
>> >> >>
>> >> >> On 10/04/2024 07:18, Anup Patel wrote:
>> >> >> > On Mon, Apr 8, 2024 at 9:01 PM Xiang W <wxjstz at="" 126.com=""> wrote:
>> >> >> >>
>> >> >> >> Now opensbi can run at any address via dynamic relocation. We can
>> >> >> >> remove FW_TEXT_START.
>> >> >> >>
>> >> >> >> Signed-off-by: Xiang W <wxjstz at="" 126.com="">
>> >> >> >
>> >> >> > LGTM.
>> >> >> >
>> >> >> > Reviewed-by: Anup Patel <anup at="" brainfault.org="">
>> >> >> > Tested-by: Anup Patel <anup at="" brainfault.org="">
>> >> >> >
>> >> >> > Applied this patch to the riscv/opensbi repo.
>> >> >> >
>> >> >> > Thanks,
>> >> >> > Anup
>> >> >>
>> >> >> Hi Anup,
>> >> >>
>> >> >> This patch seems to break spike support. The newly created ELF is not
>> >> >> marked as EXEC anymore but DYNAMIC and it fails with the following error:
>> >> >>
>> >> >> spike: ../fesvr/elfloader.cc:45:
>> >> >> std::map<std::__cxx11::basic_string<char>, long unsigned int>
>> >> >> load_elf(const char*, memif_t*, reg_t*, unsigned int): Assertion
>> >> >> `IS_ELF_EXEC(*eh64)' failed.
>> >> >>
>> >> >> Should we fixed OpenSBI or allow spike to load a DYNAMIC elf ?
>> >> >
>> >> >I think it is better to use FW_DYNAMIC in Spike because various
>> >> >other projects (such as U-Boot, QEMU, etc) use it.
>> >> >
>> >> >Also, Spike should support loading M-mode firmware in BIN
>> >> >(flat binary) format as well.
>> >> >
>> >> >Regards,
>> >> >Anup
>> >> >
>> >> >>
>> >> >> Thanks,
>> >> >>
>> >> >> Clément
>> >> >>
>> >> 
>> >> Hi Anup,
>> >> 
>> >> I think this patch is not that necessary, and after applying 
>> >> this patch, we lost an elf file that can disassemble or directly 
>> >> provide symbol addresses to gdb, which caused some difficulties 
>> >> in debugging. In the past, I could pass it in Specify the address 
>> >> of FW_TEXT_START during compilation to generate an ELF file containing 
>> >> accurate symbol addresses, which is useful for debugging OpenSBI.
>> >
>> >That’s what symbol-file /path/to/elf -o offset is for.
>> >
>> >Jess
>> >
>>
>> Hi Jess,
>>
>> Thanks for your tip, it can indeed solve the problem of gdb debugging. 
>> But I use vscode to start gdb for opensbi debugging. It reads symbols 
>> from the elf file in the specified "program" option by default, so I 
>> still need some other settings. I still recommend keeping FW_TEXT_START 
>> instead of removing it.
> 
> “My editor of choice is deficient” isn’t suitable justification for
> imposing complexity like this on everyone IMO. But regardless, Visual
> Studio Code has the ability to run custom commands on GDB startup[1].

Yeah, clearly, using (add-)symbol-file is common practice. That's not a
good argument against reverting the current change. Moreover, OpenSBI
DYNAMIC version works fine with Qemu (and gdb debugging providing you
give it a correct loading address) so it should be supported under spike
as well.

Clément

> 
> Jess
> 
> [1] https://code.visualstudio.com/docs/cpp/launch-json-reference#_setupcommands
> 
>