[PATCH v4] firmware: remove FW_TEXT_START

[Anup Patel]

On Wed, May 15, 2024 at 1:12 PM Clément Léger <cleger at rivosinc.com> wrote:
>
>
>
> 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.

I have sent a patch to bring back the FW_TEXT_START parameter
as optional (partially reverting the removal of FW_TEXT_START).

I am not a fan of this patch and I will drop it if more people
disagree with it.

Regards,
Anup

>
> Clément
>
> >
> > Jess
> >
> > [1] https://code.visualstudio.com/docs/cpp/launch-json-reference#_setupcommands
> >
> >
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi