| Message ID | 20240408153013.653350-1-wxjstz@126.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v4] firmware: remove FW_TEXT_START | expand |
On Mon, Apr 8, 2024 at 9:01 PM Xiang W <wxjstz@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@126.com> LGTM. Reviewed-by: Anup Patel <anup@brainfault.org> Tested-by: Anup Patel <anup@brainfault.org> Applied this patch to the riscv/opensbi repo. Thanks, Anup > --- > > v4 changes: > - only remove FW_TEXT_START by anup's suggestion > > docs/firmware/fw.md | 2 -- > docs/firmware/fw_jump.md | 4 ++-- > docs/firmware/fw_payload.md | 6 +++--- > docs/platform/generic.md | 11 +++-------- > firmware/fw_base.S | 4 +--- > firmware/fw_base.ldS | 3 +-- > firmware/fw_payload.elf.ldS | 2 +- > firmware/objects.mk | 4 ---- > firmware/payloads/test.elf.ldS | 2 +- > platform/fpga/ariane/objects.mk | 1 - > platform/fpga/openpiton/objects.mk | 1 - > platform/generic/objects.mk | 3 +-- > platform/kendryte/k210/objects.mk | 1 - > platform/nuclei/ux600/objects.mk | 1 - > platform/template/objects.mk | 9 ++------- > 15 files changed, 15 insertions(+), 39 deletions(-) > > diff --git a/docs/firmware/fw.md b/docs/firmware/fw.md > index 2f4deb5..3cc0262 100644 > --- a/docs/firmware/fw.md > +++ b/docs/firmware/fw.md > @@ -61,8 +61,6 @@ Firmware Configuration and Compilation > All firmware types support the following common compile time configuration > parameters: > > -* **FW_TEXT_START** - Defines the execution address of the OpenSBI firmware. > - This configuration parameter is mandatory. > * **FW_FDT_PATH** - Path to an external flattened device tree binary file to > be embedded in the *.rodata* section of the final firmware. If this option > is not provided then the firmware will expect the FDT to be passed as an > diff --git a/docs/firmware/fw_jump.md b/docs/firmware/fw_jump.md > index 2ee6b29..66be016 100644 > --- a/docs/firmware/fw_jump.md > +++ b/docs/firmware/fw_jump.md > @@ -35,7 +35,7 @@ follows: > At least one of *FW_JUMP_ADDR* and *FW_JUMP_OFFSET* (see below) should be > defined. Compilation errors will result from not defining one of them. > > -* **FW_JUMP_OFFSET** - Address offset from the *FW_TEXT_START* where the > +* **FW_JUMP_OFFSET** - Address offset from the opensbi load address where the > entry point of the next booting stage is located. This offset is used as > relocatable address of the next booting stage entry point. If *FW_JUMP_ADDR* > is also defined, the firmware will prefer *FW_JUMP_ADDR*. > @@ -62,7 +62,7 @@ follows: > echo fdt overlaps kernel, increase FW_JUMP_FDT_ADDR > ``` > > -* **FW_JUMP_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where > +* **FW_JUMP_FDT_OFFSET** - Address offset from the opensbi load address where > the FDT will be passed to the next booting stage. This offset is used > as relocatable address of the FDT passed to the next booting stage. If > *FW_JUMP_FDT_ADDR* is also defined, the firmware will prefer > diff --git a/docs/firmware/fw_payload.md b/docs/firmware/fw_payload.md > index a67fc50..d25be60 100644 > --- a/docs/firmware/fw_payload.md > +++ b/docs/firmware/fw_payload.md > @@ -36,8 +36,8 @@ options. These configuration parameters can be defined using either the top > level `make` command line or the target platform *objects.mk* configuration > file. The parameters currently defined are as follows: > > -* **FW_PAYLOAD_OFFSET** - Offset from *FW_TEXT_START* where the payload binary > - will be linked in the final *FW_PAYLOAD* firmware binary image. This > +* **FW_PAYLOAD_OFFSET** - Offset from the opensbi load address where the payload > + binary will be linked in the final *FW_PAYLOAD* firmware binary image. This > configuration parameter is mandatory if *FW_PAYLOAD_ALIGN* is not defined. > Compilation errors will result from an incorrect definition of > *FW_PAYLOAD_OFFSET* or of *FW_PAYLOAD_ALIGN*, or if neither of these > @@ -62,7 +62,7 @@ file. The parameters currently defined are as follows: > firmware will pass the FDT address passed by the previous booting stage > to the next booting stage. > > -* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where > +* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the opensbi load address where > the FDT will be passed to the next booting stage. This offset is used as > relocatable address of the FDT passed to the next booting stage. If > *FW_PAYLOAD_FDT_ADDR* is also defined, the firmware will prefer *FW_PAYLOAD_FDT_ADDR*. > diff --git a/docs/platform/generic.md b/docs/platform/generic.md > index c29eb04..709b436 100644 > --- a/docs/platform/generic.md > +++ b/docs/platform/generic.md > @@ -9,10 +9,9 @@ boards. > > By default, the generic FDT platform makes following assumptions: > > -1. platform FW_TEXT_START is 0x80000000 > -2. platform features are default > -3. platform stack size is default > -4. platform has no quirks or work-arounds > +1. platform features are default > +2. platform stack size is default > +3. platform has no quirks or work-arounds > > The above assumptions (except 1) can be overridden by adding special platform > callbacks which will be called based on FDT root node compatible string. > @@ -33,10 +32,6 @@ Users of the generic FDT platform will have to ensure that: > To build the platform-specific library and firmware images, provide the > *PLATFORM=generic* parameter to the top level `make` command. > > -For custom FW_TEXT_START, we can build the platform-specific library and > -firmware images by passing *PLATFORM=generic FW_TEXT_START=<custom_text_start>* > -parameter to the top level `make` command. > - > Platform Options > ---------------- > > diff --git a/firmware/fw_base.S b/firmware/fw_base.S > index b950c0b..9f995a2 100644 > --- a/firmware/fw_base.S > +++ b/firmware/fw_base.S > @@ -53,9 +53,7 @@ _try_lottery: > bnez a6, _wait_for_boot_hart > > /* relocate the global table content */ > - li t0, FW_TEXT_START /* link start */ > - lla t1, _fw_start /* load start */ > - sub t2, t1, t0 /* load offset */ > + lla t2, _fw_start > lla t0, __rel_dyn_start > lla t1, __rel_dyn_end > beq t0, t1, _relocate_done > diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS > index fb47984..9d11db5 100644 > --- a/firmware/fw_base.ldS > +++ b/firmware/fw_base.ldS > @@ -7,8 +7,7 @@ > * Anup Patel <anup.patel@wdc.com> > */ > > - . = FW_TEXT_START; > - /* Don't add any section between FW_TEXT_START and _fw_start */ > + /* Don't add any section before _fw_start */ > PROVIDE(_fw_start = .); > > . = ALIGN(0x1000); /* Need this to create proper sections */ > diff --git a/firmware/fw_payload.elf.ldS b/firmware/fw_payload.elf.ldS > index f1a544b..94e1ac6 100644 > --- a/firmware/fw_payload.elf.ldS > +++ b/firmware/fw_payload.elf.ldS > @@ -15,7 +15,7 @@ SECTIONS > #include "fw_base.ldS" > > #ifdef FW_PAYLOAD_OFFSET > - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; > + . = FW_PAYLOAD_OFFSET; > #else > . = ALIGN(FW_PAYLOAD_ALIGN); > #endif > diff --git a/firmware/objects.mk b/firmware/objects.mk > index e6b364b..a51ff65 100644 > --- a/firmware/objects.mk > +++ b/firmware/objects.mk > @@ -13,10 +13,6 @@ firmware-cflags-y += > firmware-asflags-y += > firmware-ldflags-y += > > -ifdef FW_TEXT_START > -firmware-genflags-y += -DFW_TEXT_START=$(FW_TEXT_START) > -endif > - > ifdef FW_FDT_PATH > firmware-genflags-y += -DFW_FDT_PATH=\"$(FW_FDT_PATH)\" > ifdef FW_FDT_PADDING > diff --git a/firmware/payloads/test.elf.ldS b/firmware/payloads/test.elf.ldS > index 2328a1b..08e008f 100644 > --- a/firmware/payloads/test.elf.ldS > +++ b/firmware/payloads/test.elf.ldS > @@ -13,7 +13,7 @@ ENTRY(_start) > SECTIONS > { > #ifdef FW_PAYLOAD_OFFSET > - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; > + . = FW_PAYLOAD_OFFSET; > #else > . = ALIGN(FW_PAYLOAD_ALIGN); > #endif > diff --git a/platform/fpga/ariane/objects.mk b/platform/fpga/ariane/objects.mk > index 83581ac..d1177f4 100644 > --- a/platform/fpga/ariane/objects.mk > +++ b/platform/fpga/ariane/objects.mk > @@ -17,7 +17,6 @@ platform-objs-y += platform.o > PLATFORM_RISCV_XLEN = 64 > > # Blobs to build > -FW_TEXT_START=0x80000000 > FW_JUMP=n > > ifeq ($(PLATFORM_RISCV_XLEN), 32) > diff --git a/platform/fpga/openpiton/objects.mk b/platform/fpga/openpiton/objects.mk > index c8c345a..1a0ce0c 100644 > --- a/platform/fpga/openpiton/objects.mk > +++ b/platform/fpga/openpiton/objects.mk > @@ -16,7 +16,6 @@ platform-objs-y += platform.o > PLATFORM_RISCV_XLEN = 64 > > # Blobs to build > -FW_TEXT_START=0x80000000 > FW_JUMP=n > > ifeq ($(PLATFORM_RISCV_XLEN), 32) > diff --git a/platform/generic/objects.mk b/platform/generic/objects.mk > index 85aa723..c215935 100644 > --- a/platform/generic/objects.mk > +++ b/platform/generic/objects.mk > @@ -15,14 +15,13 @@ platform-ldflags-y = > > # Command for platform specific "make run" > platform-runcmd = qemu-system-riscv$(PLATFORM_RISCV_XLEN) -M virt -m 256M \ > - -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.elf > + -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.bin > > # Objects to build > platform-objs-y += platform.o > platform-objs-y += platform_override_modules.o > > # Blobs to build > -FW_TEXT_START=0x80000000 > FW_DYNAMIC=y > FW_JUMP=y > ifeq ($(PLATFORM_RISCV_XLEN), 32) > diff --git a/platform/kendryte/k210/objects.mk b/platform/kendryte/k210/objects.mk > index 1bfb898..efac3d2 100644 > --- a/platform/kendryte/k210/objects.mk > +++ b/platform/kendryte/k210/objects.mk > @@ -21,6 +21,5 @@ platform-varprefix-k210.o = dt_k210 > platform-padding-k210.o = 2048 > > # Blobs to build > -FW_TEXT_START=0x80000000 > FW_PAYLOAD=y > FW_PAYLOAD_ALIGN=0x1000 > diff --git a/platform/nuclei/ux600/objects.mk b/platform/nuclei/ux600/objects.mk > index 7c429e0..2753a7f 100644 > --- a/platform/nuclei/ux600/objects.mk > +++ b/platform/nuclei/ux600/objects.mk > @@ -22,7 +22,6 @@ platform-runcmd = xl_spike \ > platform-objs-y += platform.o > > # Blobs to build > -FW_TEXT_START=0xA0000000 > FW_DYNAMIC=y > FW_JUMP=y > > diff --git a/platform/template/objects.mk b/platform/template/objects.mk > index b143cbc..f240a55 100644 > --- a/platform/template/objects.mk > +++ b/platform/template/objects.mk > @@ -41,9 +41,6 @@ platform-objs-y += platform.o > # > # platform-objs-y += <dt file name>.o > > -# Firmware load address configuration. This is mandatory. > -FW_TEXT_START=0x80000000 > - > # Optional parameter for path to external FDT > # FW_FDT_PATH="path to platform flattened device tree file" > > @@ -69,8 +66,7 @@ FW_JUMP=<y|n> > # endif > # FW_JUMP_FDT_OFFSET=0x2200000 > # > -# You can use fixed address for jump firmware as an alternative option, > -# but this may fail when setting wrong FW_TEXT_START. Use with caution. > +# You can use fixed address for jump firmware as an alternative option. > # SBI will prefer "<X>_ADDR" if both "<X>_ADDR" and "<X>_OFFSET" are > # defined > # ifeq ($(PLATFORM_RISCV_XLEN), 32) > @@ -97,8 +93,7 @@ endif > # FW_PAYLOAD_PATH="path to next boot stage binary image file" > # FW_PAYLOAD_FDT_OFFSET=0x2200000 > # > -# You can use fixed address for payload firmware as an alternative option, > -# but this may fail when setting wrong FW_TEXT_START. Use with caution. > +# You can use fixed address for payload firmware as an alternative option. > # SBI will prefer "FW_PAYLOAD_FDT_ADDR" if both "FW_PAYLOAD_FDT_OFFSET" > # and "FW_PAYLOAD_FDT_ADDR" are defined. > # FW_PAYLOAD_FDT_ADDR=0x82200000 > -- > 2.43.0 >
On 10/04/2024 07:18, Anup Patel wrote: > On Mon, Apr 8, 2024 at 9:01 PM Xiang W <wxjstz@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@126.com> > > LGTM. > > Reviewed-by: Anup Patel <anup@brainfault.org> > Tested-by: Anup Patel <anup@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 ? Thanks, Clément > >> --- >> >> v4 changes: >> - only remove FW_TEXT_START by anup's suggestion >> >> docs/firmware/fw.md | 2 -- >> docs/firmware/fw_jump.md | 4 ++-- >> docs/firmware/fw_payload.md | 6 +++--- >> docs/platform/generic.md | 11 +++-------- >> firmware/fw_base.S | 4 +--- >> firmware/fw_base.ldS | 3 +-- >> firmware/fw_payload.elf.ldS | 2 +- >> firmware/objects.mk | 4 ---- >> firmware/payloads/test.elf.ldS | 2 +- >> platform/fpga/ariane/objects.mk | 1 - >> platform/fpga/openpiton/objects.mk | 1 - >> platform/generic/objects.mk | 3 +-- >> platform/kendryte/k210/objects.mk | 1 - >> platform/nuclei/ux600/objects.mk | 1 - >> platform/template/objects.mk | 9 ++------- >> 15 files changed, 15 insertions(+), 39 deletions(-) >> >> diff --git a/docs/firmware/fw.md b/docs/firmware/fw.md >> index 2f4deb5..3cc0262 100644 >> --- a/docs/firmware/fw.md >> +++ b/docs/firmware/fw.md >> @@ -61,8 +61,6 @@ Firmware Configuration and Compilation >> All firmware types support the following common compile time configuration >> parameters: >> >> -* **FW_TEXT_START** - Defines the execution address of the OpenSBI firmware. >> - This configuration parameter is mandatory. >> * **FW_FDT_PATH** - Path to an external flattened device tree binary file to >> be embedded in the *.rodata* section of the final firmware. If this option >> is not provided then the firmware will expect the FDT to be passed as an >> diff --git a/docs/firmware/fw_jump.md b/docs/firmware/fw_jump.md >> index 2ee6b29..66be016 100644 >> --- a/docs/firmware/fw_jump.md >> +++ b/docs/firmware/fw_jump.md >> @@ -35,7 +35,7 @@ follows: >> At least one of *FW_JUMP_ADDR* and *FW_JUMP_OFFSET* (see below) should be >> defined. Compilation errors will result from not defining one of them. >> >> -* **FW_JUMP_OFFSET** - Address offset from the *FW_TEXT_START* where the >> +* **FW_JUMP_OFFSET** - Address offset from the opensbi load address where the >> entry point of the next booting stage is located. This offset is used as >> relocatable address of the next booting stage entry point. If *FW_JUMP_ADDR* >> is also defined, the firmware will prefer *FW_JUMP_ADDR*. >> @@ -62,7 +62,7 @@ follows: >> echo fdt overlaps kernel, increase FW_JUMP_FDT_ADDR >> ``` >> >> -* **FW_JUMP_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where >> +* **FW_JUMP_FDT_OFFSET** - Address offset from the opensbi load address where >> the FDT will be passed to the next booting stage. This offset is used >> as relocatable address of the FDT passed to the next booting stage. If >> *FW_JUMP_FDT_ADDR* is also defined, the firmware will prefer >> diff --git a/docs/firmware/fw_payload.md b/docs/firmware/fw_payload.md >> index a67fc50..d25be60 100644 >> --- a/docs/firmware/fw_payload.md >> +++ b/docs/firmware/fw_payload.md >> @@ -36,8 +36,8 @@ options. These configuration parameters can be defined using either the top >> level `make` command line or the target platform *objects.mk* configuration >> file. The parameters currently defined are as follows: >> >> -* **FW_PAYLOAD_OFFSET** - Offset from *FW_TEXT_START* where the payload binary >> - will be linked in the final *FW_PAYLOAD* firmware binary image. This >> +* **FW_PAYLOAD_OFFSET** - Offset from the opensbi load address where the payload >> + binary will be linked in the final *FW_PAYLOAD* firmware binary image. This >> configuration parameter is mandatory if *FW_PAYLOAD_ALIGN* is not defined. >> Compilation errors will result from an incorrect definition of >> *FW_PAYLOAD_OFFSET* or of *FW_PAYLOAD_ALIGN*, or if neither of these >> @@ -62,7 +62,7 @@ file. The parameters currently defined are as follows: >> firmware will pass the FDT address passed by the previous booting stage >> to the next booting stage. >> >> -* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where >> +* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the opensbi load address where >> the FDT will be passed to the next booting stage. This offset is used as >> relocatable address of the FDT passed to the next booting stage. If >> *FW_PAYLOAD_FDT_ADDR* is also defined, the firmware will prefer *FW_PAYLOAD_FDT_ADDR*. >> diff --git a/docs/platform/generic.md b/docs/platform/generic.md >> index c29eb04..709b436 100644 >> --- a/docs/platform/generic.md >> +++ b/docs/platform/generic.md >> @@ -9,10 +9,9 @@ boards. >> >> By default, the generic FDT platform makes following assumptions: >> >> -1. platform FW_TEXT_START is 0x80000000 >> -2. platform features are default >> -3. platform stack size is default >> -4. platform has no quirks or work-arounds >> +1. platform features are default >> +2. platform stack size is default >> +3. platform has no quirks or work-arounds >> >> The above assumptions (except 1) can be overridden by adding special platform >> callbacks which will be called based on FDT root node compatible string. >> @@ -33,10 +32,6 @@ Users of the generic FDT platform will have to ensure that: >> To build the platform-specific library and firmware images, provide the >> *PLATFORM=generic* parameter to the top level `make` command. >> >> -For custom FW_TEXT_START, we can build the platform-specific library and >> -firmware images by passing *PLATFORM=generic FW_TEXT_START=<custom_text_start>* >> -parameter to the top level `make` command. >> - >> Platform Options >> ---------------- >> >> diff --git a/firmware/fw_base.S b/firmware/fw_base.S >> index b950c0b..9f995a2 100644 >> --- a/firmware/fw_base.S >> +++ b/firmware/fw_base.S >> @@ -53,9 +53,7 @@ _try_lottery: >> bnez a6, _wait_for_boot_hart >> >> /* relocate the global table content */ >> - li t0, FW_TEXT_START /* link start */ >> - lla t1, _fw_start /* load start */ >> - sub t2, t1, t0 /* load offset */ >> + lla t2, _fw_start >> lla t0, __rel_dyn_start >> lla t1, __rel_dyn_end >> beq t0, t1, _relocate_done >> diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS >> index fb47984..9d11db5 100644 >> --- a/firmware/fw_base.ldS >> +++ b/firmware/fw_base.ldS >> @@ -7,8 +7,7 @@ >> * Anup Patel <anup.patel@wdc.com> >> */ >> >> - . = FW_TEXT_START; >> - /* Don't add any section between FW_TEXT_START and _fw_start */ >> + /* Don't add any section before _fw_start */ >> PROVIDE(_fw_start = .); >> >> . = ALIGN(0x1000); /* Need this to create proper sections */ >> diff --git a/firmware/fw_payload.elf.ldS b/firmware/fw_payload.elf.ldS >> index f1a544b..94e1ac6 100644 >> --- a/firmware/fw_payload.elf.ldS >> +++ b/firmware/fw_payload.elf.ldS >> @@ -15,7 +15,7 @@ SECTIONS >> #include "fw_base.ldS" >> >> #ifdef FW_PAYLOAD_OFFSET >> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; >> + . = FW_PAYLOAD_OFFSET; >> #else >> . = ALIGN(FW_PAYLOAD_ALIGN); >> #endif >> diff --git a/firmware/objects.mk b/firmware/objects.mk >> index e6b364b..a51ff65 100644 >> --- a/firmware/objects.mk >> +++ b/firmware/objects.mk >> @@ -13,10 +13,6 @@ firmware-cflags-y += >> firmware-asflags-y += >> firmware-ldflags-y += >> >> -ifdef FW_TEXT_START >> -firmware-genflags-y += -DFW_TEXT_START=$(FW_TEXT_START) >> -endif >> - >> ifdef FW_FDT_PATH >> firmware-genflags-y += -DFW_FDT_PATH=\"$(FW_FDT_PATH)\" >> ifdef FW_FDT_PADDING >> diff --git a/firmware/payloads/test.elf.ldS b/firmware/payloads/test.elf.ldS >> index 2328a1b..08e008f 100644 >> --- a/firmware/payloads/test.elf.ldS >> +++ b/firmware/payloads/test.elf.ldS >> @@ -13,7 +13,7 @@ ENTRY(_start) >> SECTIONS >> { >> #ifdef FW_PAYLOAD_OFFSET >> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; >> + . = FW_PAYLOAD_OFFSET; >> #else >> . = ALIGN(FW_PAYLOAD_ALIGN); >> #endif >> diff --git a/platform/fpga/ariane/objects.mk b/platform/fpga/ariane/objects.mk >> index 83581ac..d1177f4 100644 >> --- a/platform/fpga/ariane/objects.mk >> +++ b/platform/fpga/ariane/objects.mk >> @@ -17,7 +17,6 @@ platform-objs-y += platform.o >> PLATFORM_RISCV_XLEN = 64 >> >> # Blobs to build >> -FW_TEXT_START=0x80000000 >> FW_JUMP=n >> >> ifeq ($(PLATFORM_RISCV_XLEN), 32) >> diff --git a/platform/fpga/openpiton/objects.mk b/platform/fpga/openpiton/objects.mk >> index c8c345a..1a0ce0c 100644 >> --- a/platform/fpga/openpiton/objects.mk >> +++ b/platform/fpga/openpiton/objects.mk >> @@ -16,7 +16,6 @@ platform-objs-y += platform.o >> PLATFORM_RISCV_XLEN = 64 >> >> # Blobs to build >> -FW_TEXT_START=0x80000000 >> FW_JUMP=n >> >> ifeq ($(PLATFORM_RISCV_XLEN), 32) >> diff --git a/platform/generic/objects.mk b/platform/generic/objects.mk >> index 85aa723..c215935 100644 >> --- a/platform/generic/objects.mk >> +++ b/platform/generic/objects.mk >> @@ -15,14 +15,13 @@ platform-ldflags-y = >> >> # Command for platform specific "make run" >> platform-runcmd = qemu-system-riscv$(PLATFORM_RISCV_XLEN) -M virt -m 256M \ >> - -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.elf >> + -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.bin >> >> # Objects to build >> platform-objs-y += platform.o >> platform-objs-y += platform_override_modules.o >> >> # Blobs to build >> -FW_TEXT_START=0x80000000 >> FW_DYNAMIC=y >> FW_JUMP=y >> ifeq ($(PLATFORM_RISCV_XLEN), 32) >> diff --git a/platform/kendryte/k210/objects.mk b/platform/kendryte/k210/objects.mk >> index 1bfb898..efac3d2 100644 >> --- a/platform/kendryte/k210/objects.mk >> +++ b/platform/kendryte/k210/objects.mk >> @@ -21,6 +21,5 @@ platform-varprefix-k210.o = dt_k210 >> platform-padding-k210.o = 2048 >> >> # Blobs to build >> -FW_TEXT_START=0x80000000 >> FW_PAYLOAD=y >> FW_PAYLOAD_ALIGN=0x1000 >> diff --git a/platform/nuclei/ux600/objects.mk b/platform/nuclei/ux600/objects.mk >> index 7c429e0..2753a7f 100644 >> --- a/platform/nuclei/ux600/objects.mk >> +++ b/platform/nuclei/ux600/objects.mk >> @@ -22,7 +22,6 @@ platform-runcmd = xl_spike \ >> platform-objs-y += platform.o >> >> # Blobs to build >> -FW_TEXT_START=0xA0000000 >> FW_DYNAMIC=y >> FW_JUMP=y >> >> diff --git a/platform/template/objects.mk b/platform/template/objects.mk >> index b143cbc..f240a55 100644 >> --- a/platform/template/objects.mk >> +++ b/platform/template/objects.mk >> @@ -41,9 +41,6 @@ platform-objs-y += platform.o >> # >> # platform-objs-y += <dt file name>.o >> >> -# Firmware load address configuration. This is mandatory. >> -FW_TEXT_START=0x80000000 >> - >> # Optional parameter for path to external FDT >> # FW_FDT_PATH="path to platform flattened device tree file" >> >> @@ -69,8 +66,7 @@ FW_JUMP=<y|n> >> # endif >> # FW_JUMP_FDT_OFFSET=0x2200000 >> # >> -# You can use fixed address for jump firmware as an alternative option, >> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. >> +# You can use fixed address for jump firmware as an alternative option. >> # SBI will prefer "<X>_ADDR" if both "<X>_ADDR" and "<X>_OFFSET" are >> # defined >> # ifeq ($(PLATFORM_RISCV_XLEN), 32) >> @@ -97,8 +93,7 @@ endif >> # FW_PAYLOAD_PATH="path to next boot stage binary image file" >> # FW_PAYLOAD_FDT_OFFSET=0x2200000 >> # >> -# You can use fixed address for payload firmware as an alternative option, >> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. >> +# You can use fixed address for payload firmware as an alternative option. >> # SBI will prefer "FW_PAYLOAD_FDT_ADDR" if both "FW_PAYLOAD_FDT_OFFSET" >> # and "FW_PAYLOAD_FDT_ADDR" are defined. >> # FW_PAYLOAD_FDT_ADDR=0x82200000 >> -- >> 2.43.0 >> >
On Tue, May 14, 2024 at 9:58 PM Clément Léger <cleger@rivosinc.com> wrote: > > > > On 10/04/2024 07:18, Anup Patel wrote: > > On Mon, Apr 8, 2024 at 9:01 PM Xiang W <wxjstz@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@126.com> > > > > LGTM. > > > > Reviewed-by: Anup Patel <anup@brainfault.org> > > Tested-by: Anup Patel <anup@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 > > > > >> --- > >> > >> v4 changes: > >> - only remove FW_TEXT_START by anup's suggestion > >> > >> docs/firmware/fw.md | 2 -- > >> docs/firmware/fw_jump.md | 4 ++-- > >> docs/firmware/fw_payload.md | 6 +++--- > >> docs/platform/generic.md | 11 +++-------- > >> firmware/fw_base.S | 4 +--- > >> firmware/fw_base.ldS | 3 +-- > >> firmware/fw_payload.elf.ldS | 2 +- > >> firmware/objects.mk | 4 ---- > >> firmware/payloads/test.elf.ldS | 2 +- > >> platform/fpga/ariane/objects.mk | 1 - > >> platform/fpga/openpiton/objects.mk | 1 - > >> platform/generic/objects.mk | 3 +-- > >> platform/kendryte/k210/objects.mk | 1 - > >> platform/nuclei/ux600/objects.mk | 1 - > >> platform/template/objects.mk | 9 ++------- > >> 15 files changed, 15 insertions(+), 39 deletions(-) > >> > >> diff --git a/docs/firmware/fw.md b/docs/firmware/fw.md > >> index 2f4deb5..3cc0262 100644 > >> --- a/docs/firmware/fw.md > >> +++ b/docs/firmware/fw.md > >> @@ -61,8 +61,6 @@ Firmware Configuration and Compilation > >> All firmware types support the following common compile time configuration > >> parameters: > >> > >> -* **FW_TEXT_START** - Defines the execution address of the OpenSBI firmware. > >> - This configuration parameter is mandatory. > >> * **FW_FDT_PATH** - Path to an external flattened device tree binary file to > >> be embedded in the *.rodata* section of the final firmware. If this option > >> is not provided then the firmware will expect the FDT to be passed as an > >> diff --git a/docs/firmware/fw_jump.md b/docs/firmware/fw_jump.md > >> index 2ee6b29..66be016 100644 > >> --- a/docs/firmware/fw_jump.md > >> +++ b/docs/firmware/fw_jump.md > >> @@ -35,7 +35,7 @@ follows: > >> At least one of *FW_JUMP_ADDR* and *FW_JUMP_OFFSET* (see below) should be > >> defined. Compilation errors will result from not defining one of them. > >> > >> -* **FW_JUMP_OFFSET** - Address offset from the *FW_TEXT_START* where the > >> +* **FW_JUMP_OFFSET** - Address offset from the opensbi load address where the > >> entry point of the next booting stage is located. This offset is used as > >> relocatable address of the next booting stage entry point. If *FW_JUMP_ADDR* > >> is also defined, the firmware will prefer *FW_JUMP_ADDR*. > >> @@ -62,7 +62,7 @@ follows: > >> echo fdt overlaps kernel, increase FW_JUMP_FDT_ADDR > >> ``` > >> > >> -* **FW_JUMP_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where > >> +* **FW_JUMP_FDT_OFFSET** - Address offset from the opensbi load address where > >> the FDT will be passed to the next booting stage. This offset is used > >> as relocatable address of the FDT passed to the next booting stage. If > >> *FW_JUMP_FDT_ADDR* is also defined, the firmware will prefer > >> diff --git a/docs/firmware/fw_payload.md b/docs/firmware/fw_payload.md > >> index a67fc50..d25be60 100644 > >> --- a/docs/firmware/fw_payload.md > >> +++ b/docs/firmware/fw_payload.md > >> @@ -36,8 +36,8 @@ options. These configuration parameters can be defined using either the top > >> level `make` command line or the target platform *objects.mk* configuration > >> file. The parameters currently defined are as follows: > >> > >> -* **FW_PAYLOAD_OFFSET** - Offset from *FW_TEXT_START* where the payload binary > >> - will be linked in the final *FW_PAYLOAD* firmware binary image. This > >> +* **FW_PAYLOAD_OFFSET** - Offset from the opensbi load address where the payload > >> + binary will be linked in the final *FW_PAYLOAD* firmware binary image. This > >> configuration parameter is mandatory if *FW_PAYLOAD_ALIGN* is not defined. > >> Compilation errors will result from an incorrect definition of > >> *FW_PAYLOAD_OFFSET* or of *FW_PAYLOAD_ALIGN*, or if neither of these > >> @@ -62,7 +62,7 @@ file. The parameters currently defined are as follows: > >> firmware will pass the FDT address passed by the previous booting stage > >> to the next booting stage. > >> > >> -* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where > >> +* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the opensbi load address where > >> the FDT will be passed to the next booting stage. This offset is used as > >> relocatable address of the FDT passed to the next booting stage. If > >> *FW_PAYLOAD_FDT_ADDR* is also defined, the firmware will prefer *FW_PAYLOAD_FDT_ADDR*. > >> diff --git a/docs/platform/generic.md b/docs/platform/generic.md > >> index c29eb04..709b436 100644 > >> --- a/docs/platform/generic.md > >> +++ b/docs/platform/generic.md > >> @@ -9,10 +9,9 @@ boards. > >> > >> By default, the generic FDT platform makes following assumptions: > >> > >> -1. platform FW_TEXT_START is 0x80000000 > >> -2. platform features are default > >> -3. platform stack size is default > >> -4. platform has no quirks or work-arounds > >> +1. platform features are default > >> +2. platform stack size is default > >> +3. platform has no quirks or work-arounds > >> > >> The above assumptions (except 1) can be overridden by adding special platform > >> callbacks which will be called based on FDT root node compatible string. > >> @@ -33,10 +32,6 @@ Users of the generic FDT platform will have to ensure that: > >> To build the platform-specific library and firmware images, provide the > >> *PLATFORM=generic* parameter to the top level `make` command. > >> > >> -For custom FW_TEXT_START, we can build the platform-specific library and > >> -firmware images by passing *PLATFORM=generic FW_TEXT_START=<custom_text_start>* > >> -parameter to the top level `make` command. > >> - > >> Platform Options > >> ---------------- > >> > >> diff --git a/firmware/fw_base.S b/firmware/fw_base.S > >> index b950c0b..9f995a2 100644 > >> --- a/firmware/fw_base.S > >> +++ b/firmware/fw_base.S > >> @@ -53,9 +53,7 @@ _try_lottery: > >> bnez a6, _wait_for_boot_hart > >> > >> /* relocate the global table content */ > >> - li t0, FW_TEXT_START /* link start */ > >> - lla t1, _fw_start /* load start */ > >> - sub t2, t1, t0 /* load offset */ > >> + lla t2, _fw_start > >> lla t0, __rel_dyn_start > >> lla t1, __rel_dyn_end > >> beq t0, t1, _relocate_done > >> diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS > >> index fb47984..9d11db5 100644 > >> --- a/firmware/fw_base.ldS > >> +++ b/firmware/fw_base.ldS > >> @@ -7,8 +7,7 @@ > >> * Anup Patel <anup.patel@wdc.com> > >> */ > >> > >> - . = FW_TEXT_START; > >> - /* Don't add any section between FW_TEXT_START and _fw_start */ > >> + /* Don't add any section before _fw_start */ > >> PROVIDE(_fw_start = .); > >> > >> . = ALIGN(0x1000); /* Need this to create proper sections */ > >> diff --git a/firmware/fw_payload.elf.ldS b/firmware/fw_payload.elf.ldS > >> index f1a544b..94e1ac6 100644 > >> --- a/firmware/fw_payload.elf.ldS > >> +++ b/firmware/fw_payload.elf.ldS > >> @@ -15,7 +15,7 @@ SECTIONS > >> #include "fw_base.ldS" > >> > >> #ifdef FW_PAYLOAD_OFFSET > >> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; > >> + . = FW_PAYLOAD_OFFSET; > >> #else > >> . = ALIGN(FW_PAYLOAD_ALIGN); > >> #endif > >> diff --git a/firmware/objects.mk b/firmware/objects.mk > >> index e6b364b..a51ff65 100644 > >> --- a/firmware/objects.mk > >> +++ b/firmware/objects.mk > >> @@ -13,10 +13,6 @@ firmware-cflags-y += > >> firmware-asflags-y += > >> firmware-ldflags-y += > >> > >> -ifdef FW_TEXT_START > >> -firmware-genflags-y += -DFW_TEXT_START=$(FW_TEXT_START) > >> -endif > >> - > >> ifdef FW_FDT_PATH > >> firmware-genflags-y += -DFW_FDT_PATH=\"$(FW_FDT_PATH)\" > >> ifdef FW_FDT_PADDING > >> diff --git a/firmware/payloads/test.elf.ldS b/firmware/payloads/test.elf.ldS > >> index 2328a1b..08e008f 100644 > >> --- a/firmware/payloads/test.elf.ldS > >> +++ b/firmware/payloads/test.elf.ldS > >> @@ -13,7 +13,7 @@ ENTRY(_start) > >> SECTIONS > >> { > >> #ifdef FW_PAYLOAD_OFFSET > >> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; > >> + . = FW_PAYLOAD_OFFSET; > >> #else > >> . = ALIGN(FW_PAYLOAD_ALIGN); > >> #endif > >> diff --git a/platform/fpga/ariane/objects.mk b/platform/fpga/ariane/objects.mk > >> index 83581ac..d1177f4 100644 > >> --- a/platform/fpga/ariane/objects.mk > >> +++ b/platform/fpga/ariane/objects.mk > >> @@ -17,7 +17,6 @@ platform-objs-y += platform.o > >> PLATFORM_RISCV_XLEN = 64 > >> > >> # Blobs to build > >> -FW_TEXT_START=0x80000000 > >> FW_JUMP=n > >> > >> ifeq ($(PLATFORM_RISCV_XLEN), 32) > >> diff --git a/platform/fpga/openpiton/objects.mk b/platform/fpga/openpiton/objects.mk > >> index c8c345a..1a0ce0c 100644 > >> --- a/platform/fpga/openpiton/objects.mk > >> +++ b/platform/fpga/openpiton/objects.mk > >> @@ -16,7 +16,6 @@ platform-objs-y += platform.o > >> PLATFORM_RISCV_XLEN = 64 > >> > >> # Blobs to build > >> -FW_TEXT_START=0x80000000 > >> FW_JUMP=n > >> > >> ifeq ($(PLATFORM_RISCV_XLEN), 32) > >> diff --git a/platform/generic/objects.mk b/platform/generic/objects.mk > >> index 85aa723..c215935 100644 > >> --- a/platform/generic/objects.mk > >> +++ b/platform/generic/objects.mk > >> @@ -15,14 +15,13 @@ platform-ldflags-y = > >> > >> # Command for platform specific "make run" > >> platform-runcmd = qemu-system-riscv$(PLATFORM_RISCV_XLEN) -M virt -m 256M \ > >> - -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.elf > >> + -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.bin > >> > >> # Objects to build > >> platform-objs-y += platform.o > >> platform-objs-y += platform_override_modules.o > >> > >> # Blobs to build > >> -FW_TEXT_START=0x80000000 > >> FW_DYNAMIC=y > >> FW_JUMP=y > >> ifeq ($(PLATFORM_RISCV_XLEN), 32) > >> diff --git a/platform/kendryte/k210/objects.mk b/platform/kendryte/k210/objects.mk > >> index 1bfb898..efac3d2 100644 > >> --- a/platform/kendryte/k210/objects.mk > >> +++ b/platform/kendryte/k210/objects.mk > >> @@ -21,6 +21,5 @@ platform-varprefix-k210.o = dt_k210 > >> platform-padding-k210.o = 2048 > >> > >> # Blobs to build > >> -FW_TEXT_START=0x80000000 > >> FW_PAYLOAD=y > >> FW_PAYLOAD_ALIGN=0x1000 > >> diff --git a/platform/nuclei/ux600/objects.mk b/platform/nuclei/ux600/objects.mk > >> index 7c429e0..2753a7f 100644 > >> --- a/platform/nuclei/ux600/objects.mk > >> +++ b/platform/nuclei/ux600/objects.mk > >> @@ -22,7 +22,6 @@ platform-runcmd = xl_spike \ > >> platform-objs-y += platform.o > >> > >> # Blobs to build > >> -FW_TEXT_START=0xA0000000 > >> FW_DYNAMIC=y > >> FW_JUMP=y > >> > >> diff --git a/platform/template/objects.mk b/platform/template/objects.mk > >> index b143cbc..f240a55 100644 > >> --- a/platform/template/objects.mk > >> +++ b/platform/template/objects.mk > >> @@ -41,9 +41,6 @@ platform-objs-y += platform.o > >> # > >> # platform-objs-y += <dt file name>.o > >> > >> -# Firmware load address configuration. This is mandatory. > >> -FW_TEXT_START=0x80000000 > >> - > >> # Optional parameter for path to external FDT > >> # FW_FDT_PATH="path to platform flattened device tree file" > >> > >> @@ -69,8 +66,7 @@ FW_JUMP=<y|n> > >> # endif > >> # FW_JUMP_FDT_OFFSET=0x2200000 > >> # > >> -# You can use fixed address for jump firmware as an alternative option, > >> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. > >> +# You can use fixed address for jump firmware as an alternative option. > >> # SBI will prefer "<X>_ADDR" if both "<X>_ADDR" and "<X>_OFFSET" are > >> # defined > >> # ifeq ($(PLATFORM_RISCV_XLEN), 32) > >> @@ -97,8 +93,7 @@ endif > >> # FW_PAYLOAD_PATH="path to next boot stage binary image file" > >> # FW_PAYLOAD_FDT_OFFSET=0x2200000 > >> # > >> -# You can use fixed address for payload firmware as an alternative option, > >> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. > >> +# You can use fixed address for payload firmware as an alternative option. > >> # SBI will prefer "FW_PAYLOAD_FDT_ADDR" if both "FW_PAYLOAD_FDT_OFFSET" > >> # and "FW_PAYLOAD_FDT_ADDR" are defined. > >> # FW_PAYLOAD_FDT_ADDR=0x82200000 > >> -- > >> 2.43.0 > >> > >
>On Tue, May 14, 2024 at 9:58 PM Clément Léger <cleger@rivosinc.com> wrote:
>>
>>
>>
>> On 10/04/2024 07:18, Anup Patel wrote:
>> > On Mon, Apr 8, 2024 at 9:01 PM Xiang W <wxjstz@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@126.com>
>> >
>> > LGTM.
>> >
>> > Reviewed-by: Anup Patel <anup@brainfault.org>
>> > Tested-by: Anup Patel <anup@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.
Best regards,
Cheng Yang
>> >
>> >> ---
>> >>
>> >> v4 changes:
>> >> - only remove FW_TEXT_START by anup's suggestion
>> >>
>> >> docs/firmware/fw.md | 2 --
>> >> docs/firmware/fw_jump.md | 4 ++--
>> >> docs/firmware/fw_payload.md | 6 +++---
>> >> docs/platform/generic.md | 11 +++--------
>> >> firmware/fw_base.S | 4 +---
>> >> firmware/fw_base.ldS | 3 +--
>> >> firmware/fw_payload.elf.ldS | 2 +-
>> >> firmware/objects.mk | 4 ----
>> >> firmware/payloads/test.elf.ldS | 2 +-
>> >> platform/fpga/ariane/objects.mk | 1 -
>> >> platform/fpga/openpiton/objects.mk | 1 -
>> >> platform/generic/objects.mk | 3 +--
>> >> platform/kendryte/k210/objects.mk | 1 -
>> >> platform/nuclei/ux600/objects.mk | 1 -
>> >> platform/template/objects.mk | 9 ++-------
>> >> 15 files changed, 15 insertions(+), 39 deletions(-)
>> >>
>> >> diff --git a/docs/firmware/fw.md b/docs/firmware/fw.md
>> >> index 2f4deb5..3cc0262 100644
>> >> --- a/docs/firmware/fw.md
>> >> +++ b/docs/firmware/fw.md
>> >> @@ -61,8 +61,6 @@ Firmware Configuration and Compilation
>> >> All firmware types support the following common compile time configuration
>> >> parameters:
>> >>
>> >> -* **FW_TEXT_START** - Defines the execution address of the OpenSBI firmware.
>> >> - This configuration parameter is mandatory.
>> >> * **FW_FDT_PATH** - Path to an external flattened device tree binary file to
>> >> be embedded in the *.rodata* section of the final firmware. If this option
>> >> is not provided then the firmware will expect the FDT to be passed as an
>> >> diff --git a/docs/firmware/fw_jump.md b/docs/firmware/fw_jump.md
>> >> index 2ee6b29..66be016 100644
>> >> --- a/docs/firmware/fw_jump.md
>> >> +++ b/docs/firmware/fw_jump.md
>> >> @@ -35,7 +35,7 @@ follows:
>> >> At least one of *FW_JUMP_ADDR* and *FW_JUMP_OFFSET* (see below) should be
>> >> defined. Compilation errors will result from not defining one of them.
>> >>
>> >> -* **FW_JUMP_OFFSET** - Address offset from the *FW_TEXT_START* where the
>> >> +* **FW_JUMP_OFFSET** - Address offset from the opensbi load address where the
>> >> entry point of the next booting stage is located. This offset is used as
>> >> relocatable address of the next booting stage entry point. If *FW_JUMP_ADDR*
>> >> is also defined, the firmware will prefer *FW_JUMP_ADDR*.
>> >> @@ -62,7 +62,7 @@ follows:
>> >> echo fdt overlaps kernel, increase FW_JUMP_FDT_ADDR
>> >> ```
>> >>
>> >> -* **FW_JUMP_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where
>> >> +* **FW_JUMP_FDT_OFFSET** - Address offset from the opensbi load address where
>> >> the FDT will be passed to the next booting stage. This offset is used
>> >> as relocatable address of the FDT passed to the next booting stage. If
>> >> *FW_JUMP_FDT_ADDR* is also defined, the firmware will prefer
>> >> diff --git a/docs/firmware/fw_payload.md b/docs/firmware/fw_payload.md
>> >> index a67fc50..d25be60 100644
>> >> --- a/docs/firmware/fw_payload.md
>> >> +++ b/docs/firmware/fw_payload.md
>> >> @@ -36,8 +36,8 @@ options. These configuration parameters can be defined using either the top
>> >> level `make` command line or the target platform *objects.mk* configuration
>> >> file. The parameters currently defined are as follows:
>> >>
>> >> -* **FW_PAYLOAD_OFFSET** - Offset from *FW_TEXT_START* where the payload binary
>> >> - will be linked in the final *FW_PAYLOAD* firmware binary image. This
>> >> +* **FW_PAYLOAD_OFFSET** - Offset from the opensbi load address where the payload
>> >> + binary will be linked in the final *FW_PAYLOAD* firmware binary image. This
>> >> configuration parameter is mandatory if *FW_PAYLOAD_ALIGN* is not defined.
>> >> Compilation errors will result from an incorrect definition of
>> >> *FW_PAYLOAD_OFFSET* or of *FW_PAYLOAD_ALIGN*, or if neither of these
>> >> @@ -62,7 +62,7 @@ file. The parameters currently defined are as follows:
>> >> firmware will pass the FDT address passed by the previous booting stage
>> >> to the next booting stage.
>> >>
>> >> -* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where
>> >> +* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the opensbi load address where
>> >> the FDT will be passed to the next booting stage. This offset is used as
>> >> relocatable address of the FDT passed to the next booting stage. If
>> >> *FW_PAYLOAD_FDT_ADDR* is also defined, the firmware will prefer *FW_PAYLOAD_FDT_ADDR*.
>> >> diff --git a/docs/platform/generic.md b/docs/platform/generic.md
>> >> index c29eb04..709b436 100644
>> >> --- a/docs/platform/generic.md
>> >> +++ b/docs/platform/generic.md
>> >> @@ -9,10 +9,9 @@ boards.
>> >>
>> >> By default, the generic FDT platform makes following assumptions:
>> >>
>> >> -1. platform FW_TEXT_START is 0x80000000
>> >> -2. platform features are default
>> >> -3. platform stack size is default
>> >> -4. platform has no quirks or work-arounds
>> >> +1. platform features are default
>> >> +2. platform stack size is default
>> >> +3. platform has no quirks or work-arounds
>> >>
>> >> The above assumptions (except 1) can be overridden by adding special platform
>> >> callbacks which will be called based on FDT root node compatible string.
>> >> @@ -33,10 +32,6 @@ Users of the generic FDT platform will have to ensure that:
>> >> To build the platform-specific library and firmware images, provide the
>> >> *PLATFORM=generic* parameter to the top level `make` command.
>> >>
>> >> -For custom FW_TEXT_START, we can build the platform-specific library and
>> >> -firmware images by passing *PLATFORM=generic FW_TEXT_START=<custom_text_start>*
>> >> -parameter to the top level `make` command.
>> >> -
>> >> Platform Options
>> >> ----------------
>> >>
>> >> diff --git a/firmware/fw_base.S b/firmware/fw_base.S
>> >> index b950c0b..9f995a2 100644
>> >> --- a/firmware/fw_base.S
>> >> +++ b/firmware/fw_base.S
>> >> @@ -53,9 +53,7 @@ _try_lottery:
>> >> bnez a6, _wait_for_boot_hart
>> >>
>> >> /* relocate the global table content */
>> >> - li t0, FW_TEXT_START /* link start */
>> >> - lla t1, _fw_start /* load start */
>> >> - sub t2, t1, t0 /* load offset */
>> >> + lla t2, _fw_start
>> >> lla t0, __rel_dyn_start
>> >> lla t1, __rel_dyn_end
>> >> beq t0, t1, _relocate_done
>> >> diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS
>> >> index fb47984..9d11db5 100644
>> >> --- a/firmware/fw_base.ldS
>> >> +++ b/firmware/fw_base.ldS
>> >> @@ -7,8 +7,7 @@
>> >> * Anup Patel <anup.patel@wdc.com>
>> >> */
>> >>
>> >> - . = FW_TEXT_START;
>> >> - /* Don't add any section between FW_TEXT_START and _fw_start */
>> >> + /* Don't add any section before _fw_start */
>> >> PROVIDE(_fw_start = .);
>> >>
>> >> . = ALIGN(0x1000); /* Need this to create proper sections */
>> >> diff --git a/firmware/fw_payload.elf.ldS b/firmware/fw_payload.elf.ldS
>> >> index f1a544b..94e1ac6 100644
>> >> --- a/firmware/fw_payload.elf.ldS
>> >> +++ b/firmware/fw_payload.elf.ldS
>> >> @@ -15,7 +15,7 @@ SECTIONS
>> >> #include "fw_base.ldS"
>> >>
>> >> #ifdef FW_PAYLOAD_OFFSET
>> >> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET;
>> >> + . = FW_PAYLOAD_OFFSET;
>> >> #else
>> >> . = ALIGN(FW_PAYLOAD_ALIGN);
>> >> #endif
>> >> diff --git a/firmware/objects.mk b/firmware/objects.mk
>> >> index e6b364b..a51ff65 100644
>> >> --- a/firmware/objects.mk
>> >> +++ b/firmware/objects.mk
>> >> @@ -13,10 +13,6 @@ firmware-cflags-y +=
>> >> firmware-asflags-y +=
>> >> firmware-ldflags-y +=
>> >>
>> >> -ifdef FW_TEXT_START
>> >> -firmware-genflags-y += -DFW_TEXT_START=$(FW_TEXT_START)
>> >> -endif
>> >> -
>> >> ifdef FW_FDT_PATH
>> >> firmware-genflags-y += -DFW_FDT_PATH=\"$(FW_FDT_PATH)\"
>> >> ifdef FW_FDT_PADDING
>> >> diff --git a/firmware/payloads/test.elf.ldS b/firmware/payloads/test.elf.ldS
>> >> index 2328a1b..08e008f 100644
>> >> --- a/firmware/payloads/test.elf.ldS
>> >> +++ b/firmware/payloads/test.elf.ldS
>> >> @@ -13,7 +13,7 @@ ENTRY(_start)
>> >> SECTIONS
>> >> {
>> >> #ifdef FW_PAYLOAD_OFFSET
>> >> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET;
>> >> + . = FW_PAYLOAD_OFFSET;
>> >> #else
>> >> . = ALIGN(FW_PAYLOAD_ALIGN);
>> >> #endif
>> >> diff --git a/platform/fpga/ariane/objects.mk b/platform/fpga/ariane/objects.mk
>> >> index 83581ac..d1177f4 100644
>> >> --- a/platform/fpga/ariane/objects.mk
>> >> +++ b/platform/fpga/ariane/objects.mk
>> >> @@ -17,7 +17,6 @@ platform-objs-y += platform.o
>> >> PLATFORM_RISCV_XLEN = 64
>> >>
>> >> # Blobs to build
>> >> -FW_TEXT_START=0x80000000
>> >> FW_JUMP=n
>> >>
>> >> ifeq ($(PLATFORM_RISCV_XLEN), 32)
>> >> diff --git a/platform/fpga/openpiton/objects.mk b/platform/fpga/openpiton/objects.mk
>> >> index c8c345a..1a0ce0c 100644
>> >> --- a/platform/fpga/openpiton/objects.mk
>> >> +++ b/platform/fpga/openpiton/objects.mk
>> >> @@ -16,7 +16,6 @@ platform-objs-y += platform.o
>> >> PLATFORM_RISCV_XLEN = 64
>> >>
>> >> # Blobs to build
>> >> -FW_TEXT_START=0x80000000
>> >> FW_JUMP=n
>> >>
>> >> ifeq ($(PLATFORM_RISCV_XLEN), 32)
>> >> diff --git a/platform/generic/objects.mk b/platform/generic/objects.mk
>> >> index 85aa723..c215935 100644
>> >> --- a/platform/generic/objects.mk
>> >> +++ b/platform/generic/objects.mk
>> >> @@ -15,14 +15,13 @@ platform-ldflags-y =
>> >>
>> >> # Command for platform specific "make run"
>> >> platform-runcmd = qemu-system-riscv$(PLATFORM_RISCV_XLEN) -M virt -m 256M \
>> >> - -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.elf
>> >> + -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.bin
>> >>
>> >> # Objects to build
>> >> platform-objs-y += platform.o
>> >> platform-objs-y += platform_override_modules.o
>> >>
>> >> # Blobs to build
>> >> -FW_TEXT_START=0x80000000
>> >> FW_DYNAMIC=y
>> >> FW_JUMP=y
>> >> ifeq ($(PLATFORM_RISCV_XLEN), 32)
>> >> diff --git a/platform/kendryte/k210/objects.mk b/platform/kendryte/k210/objects.mk
>> >> index 1bfb898..efac3d2 100644
>> >> --- a/platform/kendryte/k210/objects.mk
>> >> +++ b/platform/kendryte/k210/objects.mk
>> >> @@ -21,6 +21,5 @@ platform-varprefix-k210.o = dt_k210
>> >> platform-padding-k210.o = 2048
>> >>
>> >> # Blobs to build
>> >> -FW_TEXT_START=0x80000000
>> >> FW_PAYLOAD=y
>> >> FW_PAYLOAD_ALIGN=0x1000
>> >> diff --git a/platform/nuclei/ux600/objects.mk b/platform/nuclei/ux600/objects.mk
>> >> index 7c429e0..2753a7f 100644
>> >> --- a/platform/nuclei/ux600/objects.mk
>> >> +++ b/platform/nuclei/ux600/objects.mk
>> >> @@ -22,7 +22,6 @@ platform-runcmd = xl_spike \
>> >> platform-objs-y += platform.o
>> >>
>> >> # Blobs to build
>> >> -FW_TEXT_START=0xA0000000
>> >> FW_DYNAMIC=y
>> >> FW_JUMP=y
>> >>
>> >> diff --git a/platform/template/objects.mk b/platform/template/objects.mk
>> >> index b143cbc..f240a55 100644
>> >> --- a/platform/template/objects.mk
>> >> +++ b/platform/template/objects.mk
>> >> @@ -41,9 +41,6 @@ platform-objs-y += platform.o
>> >> #
>> >> # platform-objs-y += <dt file="" name="">.o
>> >>
>> >> -# Firmware load address configuration. This is mandatory.
>> >> -FW_TEXT_START=0x80000000
>> >> -
>> >> # Optional parameter for path to external FDT
>> >> # FW_FDT_PATH="path to platform flattened device tree file"
>> >>
>> >> @@ -69,8 +66,7 @@ FW_JUMP=<y|n>
>> >> # endif
>> >> # FW_JUMP_FDT_OFFSET=0x2200000
>> >> #
>> >> -# You can use fixed address for jump firmware as an alternative option,
>> >> -# but this may fail when setting wrong FW_TEXT_START. Use with caution.
>> >> +# You can use fixed address for jump firmware as an alternative option.
>> >> # SBI will prefer "<x>_ADDR" if both "<x>_ADDR" and "<x>_OFFSET" are
>> >> # defined
>> >> # ifeq ($(PLATFORM_RISCV_XLEN), 32)
>> >> @@ -97,8 +93,7 @@ endif
>> >> # FW_PAYLOAD_PATH="path to next boot stage binary image file"
>> >> # FW_PAYLOAD_FDT_OFFSET=0x2200000
>> >> #
>> >> -# You can use fixed address for payload firmware as an alternative option,
>> >> -# but this may fail when setting wrong FW_TEXT_START. Use with caution.
>> >> +# You can use fixed address for payload firmware as an alternative option.
>> >> # SBI will prefer "FW_PAYLOAD_FDT_ADDR" if both "FW_PAYLOAD_FDT_OFFSET"
>> >> # and "FW_PAYLOAD_FDT_ADDR" are defined.
>> >> # FW_PAYLOAD_FDT_ADDR=0x82200000
>> >> --
>> >> 2.43.0
>> >>
>> >
>
>--
>opensbi mailing list
>opensbi@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/opensbi</x></x></x></y|n></dt></anup.patel@wdc.com></custom_text_start></std::__cxx11::basic_string<char></anup@brainfault.org></anup@brainfault.org></wxjstz@126.com></wxjstz@126.com></cleger@rivosinc.com>
On 14 May 2024, at 17:33, Anup Patel <anup@brainfault.org> wrote: > > On Tue, May 14, 2024 at 9:58 PM Clément Léger <cleger@rivosinc.com> wrote: >> >> >> >> On 10/04/2024 07:18, Anup Patel wrote: >>> On Mon, Apr 8, 2024 at 9:01 PM Xiang W <wxjstz@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@126.com> >>> >>> LGTM. >>> >>> Reviewed-by: Anup Patel <anup@brainfault.org> >>> Tested-by: Anup Patel <anup@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. That’s an orthogonal concern. This is about the ELF file type, not the boot method used, but supporting more boot methods is good too. Spike should support ET_DYN files, but will need to adjust all the load addresses as they’ll be zero, so it’s not just a case of adding an option to the assert. > Also, Spike should support loading M-mode firmware in BIN > (flat binary) format as well. There’s not really a good reason to prefer a flat binary over an ELF for an emulator*. For a board it makes sense because you want to have the flat binary ready to go in memory, but for an emulator it’s more useful to have the richer structure that an ELF provides, especially if that includes debug info. Historically Spike needed it to be an ELF so it could extract the HTIF symbol information as otherwise you’d have no console I/O, but I believe these days Spike can emulate an 8250 so that may no longer be a requirement? Jess * Space savings are minimal to non-existent; Debian’s U-Boot qemu-riscv64_smode saves 0.8%, and the generic OpenSBI flat binaries are bigger than the ELF files > Regards, > Anup > >> >> Thanks, >> >> Clément >> >>> >>>> --- >>>> >>>> v4 changes: >>>> - only remove FW_TEXT_START by anup's suggestion >>>> >>>> docs/firmware/fw.md | 2 -- >>>> docs/firmware/fw_jump.md | 4 ++-- >>>> docs/firmware/fw_payload.md | 6 +++--- >>>> docs/platform/generic.md | 11 +++-------- >>>> firmware/fw_base.S | 4 +--- >>>> firmware/fw_base.ldS | 3 +-- >>>> firmware/fw_payload.elf.ldS | 2 +- >>>> firmware/objects.mk | 4 ---- >>>> firmware/payloads/test.elf.ldS | 2 +- >>>> platform/fpga/ariane/objects.mk | 1 - >>>> platform/fpga/openpiton/objects.mk | 1 - >>>> platform/generic/objects.mk | 3 +-- >>>> platform/kendryte/k210/objects.mk | 1 - >>>> platform/nuclei/ux600/objects.mk | 1 - >>>> platform/template/objects.mk | 9 ++------- >>>> 15 files changed, 15 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/docs/firmware/fw.md b/docs/firmware/fw.md >>>> index 2f4deb5..3cc0262 100644 >>>> --- a/docs/firmware/fw.md >>>> +++ b/docs/firmware/fw.md >>>> @@ -61,8 +61,6 @@ Firmware Configuration and Compilation >>>> All firmware types support the following common compile time configuration >>>> parameters: >>>> >>>> -* **FW_TEXT_START** - Defines the execution address of the OpenSBI firmware. >>>> - This configuration parameter is mandatory. >>>> * **FW_FDT_PATH** - Path to an external flattened device tree binary file to >>>> be embedded in the *.rodata* section of the final firmware. If this option >>>> is not provided then the firmware will expect the FDT to be passed as an >>>> diff --git a/docs/firmware/fw_jump.md b/docs/firmware/fw_jump.md >>>> index 2ee6b29..66be016 100644 >>>> --- a/docs/firmware/fw_jump.md >>>> +++ b/docs/firmware/fw_jump.md >>>> @@ -35,7 +35,7 @@ follows: >>>> At least one of *FW_JUMP_ADDR* and *FW_JUMP_OFFSET* (see below) should be >>>> defined. Compilation errors will result from not defining one of them. >>>> >>>> -* **FW_JUMP_OFFSET** - Address offset from the *FW_TEXT_START* where the >>>> +* **FW_JUMP_OFFSET** - Address offset from the opensbi load address where the >>>> entry point of the next booting stage is located. This offset is used as >>>> relocatable address of the next booting stage entry point. If *FW_JUMP_ADDR* >>>> is also defined, the firmware will prefer *FW_JUMP_ADDR*. >>>> @@ -62,7 +62,7 @@ follows: >>>> echo fdt overlaps kernel, increase FW_JUMP_FDT_ADDR >>>> ``` >>>> >>>> -* **FW_JUMP_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where >>>> +* **FW_JUMP_FDT_OFFSET** - Address offset from the opensbi load address where >>>> the FDT will be passed to the next booting stage. This offset is used >>>> as relocatable address of the FDT passed to the next booting stage. If >>>> *FW_JUMP_FDT_ADDR* is also defined, the firmware will prefer >>>> diff --git a/docs/firmware/fw_payload.md b/docs/firmware/fw_payload.md >>>> index a67fc50..d25be60 100644 >>>> --- a/docs/firmware/fw_payload.md >>>> +++ b/docs/firmware/fw_payload.md >>>> @@ -36,8 +36,8 @@ options. These configuration parameters can be defined using either the top >>>> level `make` command line or the target platform *objects.mk* configuration >>>> file. The parameters currently defined are as follows: >>>> >>>> -* **FW_PAYLOAD_OFFSET** - Offset from *FW_TEXT_START* where the payload binary >>>> - will be linked in the final *FW_PAYLOAD* firmware binary image. This >>>> +* **FW_PAYLOAD_OFFSET** - Offset from the opensbi load address where the payload >>>> + binary will be linked in the final *FW_PAYLOAD* firmware binary image. This >>>> configuration parameter is mandatory if *FW_PAYLOAD_ALIGN* is not defined. >>>> Compilation errors will result from an incorrect definition of >>>> *FW_PAYLOAD_OFFSET* or of *FW_PAYLOAD_ALIGN*, or if neither of these >>>> @@ -62,7 +62,7 @@ file. The parameters currently defined are as follows: >>>> firmware will pass the FDT address passed by the previous booting stage >>>> to the next booting stage. >>>> >>>> -* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where >>>> +* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the opensbi load address where >>>> the FDT will be passed to the next booting stage. This offset is used as >>>> relocatable address of the FDT passed to the next booting stage. If >>>> *FW_PAYLOAD_FDT_ADDR* is also defined, the firmware will prefer *FW_PAYLOAD_FDT_ADDR*. >>>> diff --git a/docs/platform/generic.md b/docs/platform/generic.md >>>> index c29eb04..709b436 100644 >>>> --- a/docs/platform/generic.md >>>> +++ b/docs/platform/generic.md >>>> @@ -9,10 +9,9 @@ boards. >>>> >>>> By default, the generic FDT platform makes following assumptions: >>>> >>>> -1. platform FW_TEXT_START is 0x80000000 >>>> -2. platform features are default >>>> -3. platform stack size is default >>>> -4. platform has no quirks or work-arounds >>>> +1. platform features are default >>>> +2. platform stack size is default >>>> +3. platform has no quirks or work-arounds >>>> >>>> The above assumptions (except 1) can be overridden by adding special platform >>>> callbacks which will be called based on FDT root node compatible string. >>>> @@ -33,10 +32,6 @@ Users of the generic FDT platform will have to ensure that: >>>> To build the platform-specific library and firmware images, provide the >>>> *PLATFORM=generic* parameter to the top level `make` command. >>>> >>>> -For custom FW_TEXT_START, we can build the platform-specific library and >>>> -firmware images by passing *PLATFORM=generic FW_TEXT_START=<custom_text_start>* >>>> -parameter to the top level `make` command. >>>> - >>>> Platform Options >>>> ---------------- >>>> >>>> diff --git a/firmware/fw_base.S b/firmware/fw_base.S >>>> index b950c0b..9f995a2 100644 >>>> --- a/firmware/fw_base.S >>>> +++ b/firmware/fw_base.S >>>> @@ -53,9 +53,7 @@ _try_lottery: >>>> bnez a6, _wait_for_boot_hart >>>> >>>> /* relocate the global table content */ >>>> - li t0, FW_TEXT_START /* link start */ >>>> - lla t1, _fw_start /* load start */ >>>> - sub t2, t1, t0 /* load offset */ >>>> + lla t2, _fw_start >>>> lla t0, __rel_dyn_start >>>> lla t1, __rel_dyn_end >>>> beq t0, t1, _relocate_done >>>> diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS >>>> index fb47984..9d11db5 100644 >>>> --- a/firmware/fw_base.ldS >>>> +++ b/firmware/fw_base.ldS >>>> @@ -7,8 +7,7 @@ >>>> * Anup Patel <anup.patel@wdc.com> >>>> */ >>>> >>>> - . = FW_TEXT_START; >>>> - /* Don't add any section between FW_TEXT_START and _fw_start */ >>>> + /* Don't add any section before _fw_start */ >>>> PROVIDE(_fw_start = .); >>>> >>>> . = ALIGN(0x1000); /* Need this to create proper sections */ >>>> diff --git a/firmware/fw_payload.elf.ldS b/firmware/fw_payload.elf.ldS >>>> index f1a544b..94e1ac6 100644 >>>> --- a/firmware/fw_payload.elf.ldS >>>> +++ b/firmware/fw_payload.elf.ldS >>>> @@ -15,7 +15,7 @@ SECTIONS >>>> #include "fw_base.ldS" >>>> >>>> #ifdef FW_PAYLOAD_OFFSET >>>> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; >>>> + . = FW_PAYLOAD_OFFSET; >>>> #else >>>> . = ALIGN(FW_PAYLOAD_ALIGN); >>>> #endif >>>> diff --git a/firmware/objects.mk b/firmware/objects.mk >>>> index e6b364b..a51ff65 100644 >>>> --- a/firmware/objects.mk >>>> +++ b/firmware/objects.mk >>>> @@ -13,10 +13,6 @@ firmware-cflags-y += >>>> firmware-asflags-y += >>>> firmware-ldflags-y += >>>> >>>> -ifdef FW_TEXT_START >>>> -firmware-genflags-y += -DFW_TEXT_START=$(FW_TEXT_START) >>>> -endif >>>> - >>>> ifdef FW_FDT_PATH >>>> firmware-genflags-y += -DFW_FDT_PATH=\"$(FW_FDT_PATH)\" >>>> ifdef FW_FDT_PADDING >>>> diff --git a/firmware/payloads/test.elf.ldS b/firmware/payloads/test.elf.ldS >>>> index 2328a1b..08e008f 100644 >>>> --- a/firmware/payloads/test.elf.ldS >>>> +++ b/firmware/payloads/test.elf.ldS >>>> @@ -13,7 +13,7 @@ ENTRY(_start) >>>> SECTIONS >>>> { >>>> #ifdef FW_PAYLOAD_OFFSET >>>> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; >>>> + . = FW_PAYLOAD_OFFSET; >>>> #else >>>> . = ALIGN(FW_PAYLOAD_ALIGN); >>>> #endif >>>> diff --git a/platform/fpga/ariane/objects.mk b/platform/fpga/ariane/objects.mk >>>> index 83581ac..d1177f4 100644 >>>> --- a/platform/fpga/ariane/objects.mk >>>> +++ b/platform/fpga/ariane/objects.mk >>>> @@ -17,7 +17,6 @@ platform-objs-y += platform.o >>>> PLATFORM_RISCV_XLEN = 64 >>>> >>>> # Blobs to build >>>> -FW_TEXT_START=0x80000000 >>>> FW_JUMP=n >>>> >>>> ifeq ($(PLATFORM_RISCV_XLEN), 32) >>>> diff --git a/platform/fpga/openpiton/objects.mk b/platform/fpga/openpiton/objects.mk >>>> index c8c345a..1a0ce0c 100644 >>>> --- a/platform/fpga/openpiton/objects.mk >>>> +++ b/platform/fpga/openpiton/objects.mk >>>> @@ -16,7 +16,6 @@ platform-objs-y += platform.o >>>> PLATFORM_RISCV_XLEN = 64 >>>> >>>> # Blobs to build >>>> -FW_TEXT_START=0x80000000 >>>> FW_JUMP=n >>>> >>>> ifeq ($(PLATFORM_RISCV_XLEN), 32) >>>> diff --git a/platform/generic/objects.mk b/platform/generic/objects.mk >>>> index 85aa723..c215935 100644 >>>> --- a/platform/generic/objects.mk >>>> +++ b/platform/generic/objects.mk >>>> @@ -15,14 +15,13 @@ platform-ldflags-y = >>>> >>>> # Command for platform specific "make run" >>>> platform-runcmd = qemu-system-riscv$(PLATFORM_RISCV_XLEN) -M virt -m 256M \ >>>> - -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.elf >>>> + -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.bin >>>> >>>> # Objects to build >>>> platform-objs-y += platform.o >>>> platform-objs-y += platform_override_modules.o >>>> >>>> # Blobs to build >>>> -FW_TEXT_START=0x80000000 >>>> FW_DYNAMIC=y >>>> FW_JUMP=y >>>> ifeq ($(PLATFORM_RISCV_XLEN), 32) >>>> diff --git a/platform/kendryte/k210/objects.mk b/platform/kendryte/k210/objects.mk >>>> index 1bfb898..efac3d2 100644 >>>> --- a/platform/kendryte/k210/objects.mk >>>> +++ b/platform/kendryte/k210/objects.mk >>>> @@ -21,6 +21,5 @@ platform-varprefix-k210.o = dt_k210 >>>> platform-padding-k210.o = 2048 >>>> >>>> # Blobs to build >>>> -FW_TEXT_START=0x80000000 >>>> FW_PAYLOAD=y >>>> FW_PAYLOAD_ALIGN=0x1000 >>>> diff --git a/platform/nuclei/ux600/objects.mk b/platform/nuclei/ux600/objects.mk >>>> index 7c429e0..2753a7f 100644 >>>> --- a/platform/nuclei/ux600/objects.mk >>>> +++ b/platform/nuclei/ux600/objects.mk >>>> @@ -22,7 +22,6 @@ platform-runcmd = xl_spike \ >>>> platform-objs-y += platform.o >>>> >>>> # Blobs to build >>>> -FW_TEXT_START=0xA0000000 >>>> FW_DYNAMIC=y >>>> FW_JUMP=y >>>> >>>> diff --git a/platform/template/objects.mk b/platform/template/objects.mk >>>> index b143cbc..f240a55 100644 >>>> --- a/platform/template/objects.mk >>>> +++ b/platform/template/objects.mk >>>> @@ -41,9 +41,6 @@ platform-objs-y += platform.o >>>> # >>>> # platform-objs-y += <dt file name>.o >>>> >>>> -# Firmware load address configuration. This is mandatory. >>>> -FW_TEXT_START=0x80000000 >>>> - >>>> # Optional parameter for path to external FDT >>>> # FW_FDT_PATH="path to platform flattened device tree file" >>>> >>>> @@ -69,8 +66,7 @@ FW_JUMP=<y|n> >>>> # endif >>>> # FW_JUMP_FDT_OFFSET=0x2200000 >>>> # >>>> -# You can use fixed address for jump firmware as an alternative option, >>>> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. >>>> +# You can use fixed address for jump firmware as an alternative option. >>>> # SBI will prefer "<X>_ADDR" if both "<X>_ADDR" and "<X>_OFFSET" are >>>> # defined >>>> # ifeq ($(PLATFORM_RISCV_XLEN), 32) >>>> @@ -97,8 +93,7 @@ endif >>>> # FW_PAYLOAD_PATH="path to next boot stage binary image file" >>>> # FW_PAYLOAD_FDT_OFFSET=0x2200000 >>>> # >>>> -# You can use fixed address for payload firmware as an alternative option, >>>> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. >>>> +# You can use fixed address for payload firmware as an alternative option. >>>> # SBI will prefer "FW_PAYLOAD_FDT_ADDR" if both "FW_PAYLOAD_FDT_OFFSET" >>>> # and "FW_PAYLOAD_FDT_ADDR" are defined. >>>> # FW_PAYLOAD_FDT_ADDR=0x82200000 >>>> -- >>>> 2.43.0
On 14 May 2024, at 17:50, Cheng Yang <yangcheng.work@foxmail.com> wrote: > > >On Tue, May 14, 2024 at 9:58 PM Clément Léger <cleger@rivosinc.com> wrote: > >> > >> > >> > >> On 10/04/2024 07:18, Anup Patel wrote: > >> > On Mon, Apr 8, 2024 at 9:01 PM Xiang W <wxjstz@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@126.com> > >> > > >> > LGTM. > >> > > >> > Reviewed-by: Anup Patel <anup@brainfault.org> > >> > Tested-by: Anup Patel <anup@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
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. Best regards, Cheng Yang</std::__cxx11::basic_string<char></anup></anup></wxjstz></wxjstz></cleger></yangcheng.work>
On 14/05/2024 18:56, Jessica Clarke wrote: > On 14 May 2024, at 17:33, Anup Patel <anup@brainfault.org> wrote: >> >> On Tue, May 14, 2024 at 9:58 PM Clément Léger <cleger@rivosinc.com> wrote: >>> >>> >>> >>> On 10/04/2024 07:18, Anup Patel wrote: >>>> On Mon, Apr 8, 2024 at 9:01 PM Xiang W <wxjstz@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@126.com> >>>> >>>> LGTM. >>>> >>>> Reviewed-by: Anup Patel <anup@brainfault.org> >>>> Tested-by: Anup Patel <anup@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. > > That’s an orthogonal concern. This is about the ELF file type, not the > boot method used, but supporting more boot methods is good too. > > Spike should support ET_DYN files, but will need to adjust all the load > addresses as they’ll be zero, so it’s not just a case of adding an > option to the assert. Yes sure. If nobody is looking at that, I'll see if I can take care of it. Clément > >> Also, Spike should support loading M-mode firmware in BIN >> (flat binary) format as well. > > There’s not really a good reason to prefer a flat binary over an ELF > for an emulator*. For a board it makes sense because you want to have > the flat binary ready to go in memory, but for an emulator it’s more > useful to have the richer structure that an ELF provides, especially if > that includes debug info. Historically Spike needed it to be an ELF so > it could extract the HTIF symbol information as otherwise you’d have no > console I/O, but I believe these days Spike can emulate an 8250 so that > may no longer be a requirement? > > Jess > > * Space savings are minimal to non-existent; Debian’s U-Boot > qemu-riscv64_smode saves 0.8%, and the generic OpenSBI flat > binaries are bigger than the ELF files > >> Regards, >> Anup >> >>> >>> Thanks, >>> >>> Clément >>> >>>> >>>>> --- >>>>> >>>>> v4 changes: >>>>> - only remove FW_TEXT_START by anup's suggestion >>>>> >>>>> docs/firmware/fw.md | 2 -- >>>>> docs/firmware/fw_jump.md | 4 ++-- >>>>> docs/firmware/fw_payload.md | 6 +++--- >>>>> docs/platform/generic.md | 11 +++-------- >>>>> firmware/fw_base.S | 4 +--- >>>>> firmware/fw_base.ldS | 3 +-- >>>>> firmware/fw_payload.elf.ldS | 2 +- >>>>> firmware/objects.mk | 4 ---- >>>>> firmware/payloads/test.elf.ldS | 2 +- >>>>> platform/fpga/ariane/objects.mk | 1 - >>>>> platform/fpga/openpiton/objects.mk | 1 - >>>>> platform/generic/objects.mk | 3 +-- >>>>> platform/kendryte/k210/objects.mk | 1 - >>>>> platform/nuclei/ux600/objects.mk | 1 - >>>>> platform/template/objects.mk | 9 ++------- >>>>> 15 files changed, 15 insertions(+), 39 deletions(-) >>>>> >>>>> diff --git a/docs/firmware/fw.md b/docs/firmware/fw.md >>>>> index 2f4deb5..3cc0262 100644 >>>>> --- a/docs/firmware/fw.md >>>>> +++ b/docs/firmware/fw.md >>>>> @@ -61,8 +61,6 @@ Firmware Configuration and Compilation >>>>> All firmware types support the following common compile time configuration >>>>> parameters: >>>>> >>>>> -* **FW_TEXT_START** - Defines the execution address of the OpenSBI firmware. >>>>> - This configuration parameter is mandatory. >>>>> * **FW_FDT_PATH** - Path to an external flattened device tree binary file to >>>>> be embedded in the *.rodata* section of the final firmware. If this option >>>>> is not provided then the firmware will expect the FDT to be passed as an >>>>> diff --git a/docs/firmware/fw_jump.md b/docs/firmware/fw_jump.md >>>>> index 2ee6b29..66be016 100644 >>>>> --- a/docs/firmware/fw_jump.md >>>>> +++ b/docs/firmware/fw_jump.md >>>>> @@ -35,7 +35,7 @@ follows: >>>>> At least one of *FW_JUMP_ADDR* and *FW_JUMP_OFFSET* (see below) should be >>>>> defined. Compilation errors will result from not defining one of them. >>>>> >>>>> -* **FW_JUMP_OFFSET** - Address offset from the *FW_TEXT_START* where the >>>>> +* **FW_JUMP_OFFSET** - Address offset from the opensbi load address where the >>>>> entry point of the next booting stage is located. This offset is used as >>>>> relocatable address of the next booting stage entry point. If *FW_JUMP_ADDR* >>>>> is also defined, the firmware will prefer *FW_JUMP_ADDR*. >>>>> @@ -62,7 +62,7 @@ follows: >>>>> echo fdt overlaps kernel, increase FW_JUMP_FDT_ADDR >>>>> ``` >>>>> >>>>> -* **FW_JUMP_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where >>>>> +* **FW_JUMP_FDT_OFFSET** - Address offset from the opensbi load address where >>>>> the FDT will be passed to the next booting stage. This offset is used >>>>> as relocatable address of the FDT passed to the next booting stage. If >>>>> *FW_JUMP_FDT_ADDR* is also defined, the firmware will prefer >>>>> diff --git a/docs/firmware/fw_payload.md b/docs/firmware/fw_payload.md >>>>> index a67fc50..d25be60 100644 >>>>> --- a/docs/firmware/fw_payload.md >>>>> +++ b/docs/firmware/fw_payload.md >>>>> @@ -36,8 +36,8 @@ options. These configuration parameters can be defined using either the top >>>>> level `make` command line or the target platform *objects.mk* configuration >>>>> file. The parameters currently defined are as follows: >>>>> >>>>> -* **FW_PAYLOAD_OFFSET** - Offset from *FW_TEXT_START* where the payload binary >>>>> - will be linked in the final *FW_PAYLOAD* firmware binary image. This >>>>> +* **FW_PAYLOAD_OFFSET** - Offset from the opensbi load address where the payload >>>>> + binary will be linked in the final *FW_PAYLOAD* firmware binary image. This >>>>> configuration parameter is mandatory if *FW_PAYLOAD_ALIGN* is not defined. >>>>> Compilation errors will result from an incorrect definition of >>>>> *FW_PAYLOAD_OFFSET* or of *FW_PAYLOAD_ALIGN*, or if neither of these >>>>> @@ -62,7 +62,7 @@ file. The parameters currently defined are as follows: >>>>> firmware will pass the FDT address passed by the previous booting stage >>>>> to the next booting stage. >>>>> >>>>> -* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where >>>>> +* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the opensbi load address where >>>>> the FDT will be passed to the next booting stage. This offset is used as >>>>> relocatable address of the FDT passed to the next booting stage. If >>>>> *FW_PAYLOAD_FDT_ADDR* is also defined, the firmware will prefer *FW_PAYLOAD_FDT_ADDR*. >>>>> diff --git a/docs/platform/generic.md b/docs/platform/generic.md >>>>> index c29eb04..709b436 100644 >>>>> --- a/docs/platform/generic.md >>>>> +++ b/docs/platform/generic.md >>>>> @@ -9,10 +9,9 @@ boards. >>>>> >>>>> By default, the generic FDT platform makes following assumptions: >>>>> >>>>> -1. platform FW_TEXT_START is 0x80000000 >>>>> -2. platform features are default >>>>> -3. platform stack size is default >>>>> -4. platform has no quirks or work-arounds >>>>> +1. platform features are default >>>>> +2. platform stack size is default >>>>> +3. platform has no quirks or work-arounds >>>>> >>>>> The above assumptions (except 1) can be overridden by adding special platform >>>>> callbacks which will be called based on FDT root node compatible string. >>>>> @@ -33,10 +32,6 @@ Users of the generic FDT platform will have to ensure that: >>>>> To build the platform-specific library and firmware images, provide the >>>>> *PLATFORM=generic* parameter to the top level `make` command. >>>>> >>>>> -For custom FW_TEXT_START, we can build the platform-specific library and >>>>> -firmware images by passing *PLATFORM=generic FW_TEXT_START=<custom_text_start>* >>>>> -parameter to the top level `make` command. >>>>> - >>>>> Platform Options >>>>> ---------------- >>>>> >>>>> diff --git a/firmware/fw_base.S b/firmware/fw_base.S >>>>> index b950c0b..9f995a2 100644 >>>>> --- a/firmware/fw_base.S >>>>> +++ b/firmware/fw_base.S >>>>> @@ -53,9 +53,7 @@ _try_lottery: >>>>> bnez a6, _wait_for_boot_hart >>>>> >>>>> /* relocate the global table content */ >>>>> - li t0, FW_TEXT_START /* link start */ >>>>> - lla t1, _fw_start /* load start */ >>>>> - sub t2, t1, t0 /* load offset */ >>>>> + lla t2, _fw_start >>>>> lla t0, __rel_dyn_start >>>>> lla t1, __rel_dyn_end >>>>> beq t0, t1, _relocate_done >>>>> diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS >>>>> index fb47984..9d11db5 100644 >>>>> --- a/firmware/fw_base.ldS >>>>> +++ b/firmware/fw_base.ldS >>>>> @@ -7,8 +7,7 @@ >>>>> * Anup Patel <anup.patel@wdc.com> >>>>> */ >>>>> >>>>> - . = FW_TEXT_START; >>>>> - /* Don't add any section between FW_TEXT_START and _fw_start */ >>>>> + /* Don't add any section before _fw_start */ >>>>> PROVIDE(_fw_start = .); >>>>> >>>>> . = ALIGN(0x1000); /* Need this to create proper sections */ >>>>> diff --git a/firmware/fw_payload.elf.ldS b/firmware/fw_payload.elf.ldS >>>>> index f1a544b..94e1ac6 100644 >>>>> --- a/firmware/fw_payload.elf.ldS >>>>> +++ b/firmware/fw_payload.elf.ldS >>>>> @@ -15,7 +15,7 @@ SECTIONS >>>>> #include "fw_base.ldS" >>>>> >>>>> #ifdef FW_PAYLOAD_OFFSET >>>>> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; >>>>> + . = FW_PAYLOAD_OFFSET; >>>>> #else >>>>> . = ALIGN(FW_PAYLOAD_ALIGN); >>>>> #endif >>>>> diff --git a/firmware/objects.mk b/firmware/objects.mk >>>>> index e6b364b..a51ff65 100644 >>>>> --- a/firmware/objects.mk >>>>> +++ b/firmware/objects.mk >>>>> @@ -13,10 +13,6 @@ firmware-cflags-y += >>>>> firmware-asflags-y += >>>>> firmware-ldflags-y += >>>>> >>>>> -ifdef FW_TEXT_START >>>>> -firmware-genflags-y += -DFW_TEXT_START=$(FW_TEXT_START) >>>>> -endif >>>>> - >>>>> ifdef FW_FDT_PATH >>>>> firmware-genflags-y += -DFW_FDT_PATH=\"$(FW_FDT_PATH)\" >>>>> ifdef FW_FDT_PADDING >>>>> diff --git a/firmware/payloads/test.elf.ldS b/firmware/payloads/test.elf.ldS >>>>> index 2328a1b..08e008f 100644 >>>>> --- a/firmware/payloads/test.elf.ldS >>>>> +++ b/firmware/payloads/test.elf.ldS >>>>> @@ -13,7 +13,7 @@ ENTRY(_start) >>>>> SECTIONS >>>>> { >>>>> #ifdef FW_PAYLOAD_OFFSET >>>>> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; >>>>> + . = FW_PAYLOAD_OFFSET; >>>>> #else >>>>> . = ALIGN(FW_PAYLOAD_ALIGN); >>>>> #endif >>>>> diff --git a/platform/fpga/ariane/objects.mk b/platform/fpga/ariane/objects.mk >>>>> index 83581ac..d1177f4 100644 >>>>> --- a/platform/fpga/ariane/objects.mk >>>>> +++ b/platform/fpga/ariane/objects.mk >>>>> @@ -17,7 +17,6 @@ platform-objs-y += platform.o >>>>> PLATFORM_RISCV_XLEN = 64 >>>>> >>>>> # Blobs to build >>>>> -FW_TEXT_START=0x80000000 >>>>> FW_JUMP=n >>>>> >>>>> ifeq ($(PLATFORM_RISCV_XLEN), 32) >>>>> diff --git a/platform/fpga/openpiton/objects.mk b/platform/fpga/openpiton/objects.mk >>>>> index c8c345a..1a0ce0c 100644 >>>>> --- a/platform/fpga/openpiton/objects.mk >>>>> +++ b/platform/fpga/openpiton/objects.mk >>>>> @@ -16,7 +16,6 @@ platform-objs-y += platform.o >>>>> PLATFORM_RISCV_XLEN = 64 >>>>> >>>>> # Blobs to build >>>>> -FW_TEXT_START=0x80000000 >>>>> FW_JUMP=n >>>>> >>>>> ifeq ($(PLATFORM_RISCV_XLEN), 32) >>>>> diff --git a/platform/generic/objects.mk b/platform/generic/objects.mk >>>>> index 85aa723..c215935 100644 >>>>> --- a/platform/generic/objects.mk >>>>> +++ b/platform/generic/objects.mk >>>>> @@ -15,14 +15,13 @@ platform-ldflags-y = >>>>> >>>>> # Command for platform specific "make run" >>>>> platform-runcmd = qemu-system-riscv$(PLATFORM_RISCV_XLEN) -M virt -m 256M \ >>>>> - -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.elf >>>>> + -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.bin >>>>> >>>>> # Objects to build >>>>> platform-objs-y += platform.o >>>>> platform-objs-y += platform_override_modules.o >>>>> >>>>> # Blobs to build >>>>> -FW_TEXT_START=0x80000000 >>>>> FW_DYNAMIC=y >>>>> FW_JUMP=y >>>>> ifeq ($(PLATFORM_RISCV_XLEN), 32) >>>>> diff --git a/platform/kendryte/k210/objects.mk b/platform/kendryte/k210/objects.mk >>>>> index 1bfb898..efac3d2 100644 >>>>> --- a/platform/kendryte/k210/objects.mk >>>>> +++ b/platform/kendryte/k210/objects.mk >>>>> @@ -21,6 +21,5 @@ platform-varprefix-k210.o = dt_k210 >>>>> platform-padding-k210.o = 2048 >>>>> >>>>> # Blobs to build >>>>> -FW_TEXT_START=0x80000000 >>>>> FW_PAYLOAD=y >>>>> FW_PAYLOAD_ALIGN=0x1000 >>>>> diff --git a/platform/nuclei/ux600/objects.mk b/platform/nuclei/ux600/objects.mk >>>>> index 7c429e0..2753a7f 100644 >>>>> --- a/platform/nuclei/ux600/objects.mk >>>>> +++ b/platform/nuclei/ux600/objects.mk >>>>> @@ -22,7 +22,6 @@ platform-runcmd = xl_spike \ >>>>> platform-objs-y += platform.o >>>>> >>>>> # Blobs to build >>>>> -FW_TEXT_START=0xA0000000 >>>>> FW_DYNAMIC=y >>>>> FW_JUMP=y >>>>> >>>>> diff --git a/platform/template/objects.mk b/platform/template/objects.mk >>>>> index b143cbc..f240a55 100644 >>>>> --- a/platform/template/objects.mk >>>>> +++ b/platform/template/objects.mk >>>>> @@ -41,9 +41,6 @@ platform-objs-y += platform.o >>>>> # >>>>> # platform-objs-y += <dt file name>.o >>>>> >>>>> -# Firmware load address configuration. This is mandatory. >>>>> -FW_TEXT_START=0x80000000 >>>>> - >>>>> # Optional parameter for path to external FDT >>>>> # FW_FDT_PATH="path to platform flattened device tree file" >>>>> >>>>> @@ -69,8 +66,7 @@ FW_JUMP=<y|n> >>>>> # endif >>>>> # FW_JUMP_FDT_OFFSET=0x2200000 >>>>> # >>>>> -# You can use fixed address for jump firmware as an alternative option, >>>>> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. >>>>> +# You can use fixed address for jump firmware as an alternative option. >>>>> # SBI will prefer "<X>_ADDR" if both "<X>_ADDR" and "<X>_OFFSET" are >>>>> # defined >>>>> # ifeq ($(PLATFORM_RISCV_XLEN), 32) >>>>> @@ -97,8 +93,7 @@ endif >>>>> # FW_PAYLOAD_PATH="path to next boot stage binary image file" >>>>> # FW_PAYLOAD_FDT_OFFSET=0x2200000 >>>>> # >>>>> -# You can use fixed address for payload firmware as an alternative option, >>>>> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. >>>>> +# You can use fixed address for payload firmware as an alternative option. >>>>> # SBI will prefer "FW_PAYLOAD_FDT_ADDR" if both "FW_PAYLOAD_FDT_OFFSET" >>>>> # and "FW_PAYLOAD_FDT_ADDR" are defined. >>>>> # FW_PAYLOAD_FDT_ADDR=0x82200000 >>>>> -- >>>>> 2.43.0 > >
On 14 May 2024, at 18:25, Cheng Yang <yangcheng.work@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]. Jess [1] https://code.visualstudio.com/docs/cpp/launch-json-reference#_setupcommands
On Tue, May 14, 2024 at 10:26 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 14 May 2024, at 17:33, Anup Patel <anup@brainfault.org> wrote: > > > > On Tue, May 14, 2024 at 9:58 PM Clément Léger <cleger@rivosinc.com> wrote: > >> > >> > >> > >> On 10/04/2024 07:18, Anup Patel wrote: > >>> On Mon, Apr 8, 2024 at 9:01 PM Xiang W <wxjstz@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@126.com> > >>> > >>> LGTM. > >>> > >>> Reviewed-by: Anup Patel <anup@brainfault.org> > >>> Tested-by: Anup Patel <anup@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. > > That’s an orthogonal concern. This is about the ELF file type, not the > boot method used, but supporting more boot methods is good too. > > Spike should support ET_DYN files, but will need to adjust all the load > addresses as they’ll be zero, so it’s not just a case of adding an > option to the assert. Ahh, I misinterpreted "DYNAMIC" as FW_DYNAMIC. Thanks for clarifying. > > > Also, Spike should support loading M-mode firmware in BIN > > (flat binary) format as well. > > There’s not really a good reason to prefer a flat binary over an ELF > for an emulator*. For a board it makes sense because you want to have > the flat binary ready to go in memory, but for an emulator it’s more > useful to have the richer structure that an ELF provides, especially if > that includes debug info. Historically Spike needed it to be an ELF so > it could extract the HTIF symbol information as otherwise you’d have no > console I/O, but I believe these days Spike can emulate an 8250 so that > may no longer be a requirement? Nothing against using ELF on emulators but it is generally better to use the same firmware binary on both real board and emulators. In fact, the same fw_dynamic.bin works on QEMU RISC-V and a variety of RISC-V boards. Regarding HTIF location, both OpenSBI and U-Boot support detecting HTIF location through DT so even flat firmware binary can use HTIF. Regards, Anup > > Jess > > * Space savings are minimal to non-existent; Debian’s U-Boot > qemu-riscv64_smode saves 0.8%, and the generic OpenSBI flat > binaries are bigger than the ELF files > > > Regards, > > Anup > > > >> > >> Thanks, > >> > >> Clément > >> > >>> > >>>> --- > >>>> > >>>> v4 changes: > >>>> - only remove FW_TEXT_START by anup's suggestion > >>>> > >>>> docs/firmware/fw.md | 2 -- > >>>> docs/firmware/fw_jump.md | 4 ++-- > >>>> docs/firmware/fw_payload.md | 6 +++--- > >>>> docs/platform/generic.md | 11 +++-------- > >>>> firmware/fw_base.S | 4 +--- > >>>> firmware/fw_base.ldS | 3 +-- > >>>> firmware/fw_payload.elf.ldS | 2 +- > >>>> firmware/objects.mk | 4 ---- > >>>> firmware/payloads/test.elf.ldS | 2 +- > >>>> platform/fpga/ariane/objects.mk | 1 - > >>>> platform/fpga/openpiton/objects.mk | 1 - > >>>> platform/generic/objects.mk | 3 +-- > >>>> platform/kendryte/k210/objects.mk | 1 - > >>>> platform/nuclei/ux600/objects.mk | 1 - > >>>> platform/template/objects.mk | 9 ++------- > >>>> 15 files changed, 15 insertions(+), 39 deletions(-) > >>>> > >>>> diff --git a/docs/firmware/fw.md b/docs/firmware/fw.md > >>>> index 2f4deb5..3cc0262 100644 > >>>> --- a/docs/firmware/fw.md > >>>> +++ b/docs/firmware/fw.md > >>>> @@ -61,8 +61,6 @@ Firmware Configuration and Compilation > >>>> All firmware types support the following common compile time configuration > >>>> parameters: > >>>> > >>>> -* **FW_TEXT_START** - Defines the execution address of the OpenSBI firmware. > >>>> - This configuration parameter is mandatory. > >>>> * **FW_FDT_PATH** - Path to an external flattened device tree binary file to > >>>> be embedded in the *.rodata* section of the final firmware. If this option > >>>> is not provided then the firmware will expect the FDT to be passed as an > >>>> diff --git a/docs/firmware/fw_jump.md b/docs/firmware/fw_jump.md > >>>> index 2ee6b29..66be016 100644 > >>>> --- a/docs/firmware/fw_jump.md > >>>> +++ b/docs/firmware/fw_jump.md > >>>> @@ -35,7 +35,7 @@ follows: > >>>> At least one of *FW_JUMP_ADDR* and *FW_JUMP_OFFSET* (see below) should be > >>>> defined. Compilation errors will result from not defining one of them. > >>>> > >>>> -* **FW_JUMP_OFFSET** - Address offset from the *FW_TEXT_START* where the > >>>> +* **FW_JUMP_OFFSET** - Address offset from the opensbi load address where the > >>>> entry point of the next booting stage is located. This offset is used as > >>>> relocatable address of the next booting stage entry point. If *FW_JUMP_ADDR* > >>>> is also defined, the firmware will prefer *FW_JUMP_ADDR*. > >>>> @@ -62,7 +62,7 @@ follows: > >>>> echo fdt overlaps kernel, increase FW_JUMP_FDT_ADDR > >>>> ``` > >>>> > >>>> -* **FW_JUMP_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where > >>>> +* **FW_JUMP_FDT_OFFSET** - Address offset from the opensbi load address where > >>>> the FDT will be passed to the next booting stage. This offset is used > >>>> as relocatable address of the FDT passed to the next booting stage. If > >>>> *FW_JUMP_FDT_ADDR* is also defined, the firmware will prefer > >>>> diff --git a/docs/firmware/fw_payload.md b/docs/firmware/fw_payload.md > >>>> index a67fc50..d25be60 100644 > >>>> --- a/docs/firmware/fw_payload.md > >>>> +++ b/docs/firmware/fw_payload.md > >>>> @@ -36,8 +36,8 @@ options. These configuration parameters can be defined using either the top > >>>> level `make` command line or the target platform *objects.mk* configuration > >>>> file. The parameters currently defined are as follows: > >>>> > >>>> -* **FW_PAYLOAD_OFFSET** - Offset from *FW_TEXT_START* where the payload binary > >>>> - will be linked in the final *FW_PAYLOAD* firmware binary image. This > >>>> +* **FW_PAYLOAD_OFFSET** - Offset from the opensbi load address where the payload > >>>> + binary will be linked in the final *FW_PAYLOAD* firmware binary image. This > >>>> configuration parameter is mandatory if *FW_PAYLOAD_ALIGN* is not defined. > >>>> Compilation errors will result from an incorrect definition of > >>>> *FW_PAYLOAD_OFFSET* or of *FW_PAYLOAD_ALIGN*, or if neither of these > >>>> @@ -62,7 +62,7 @@ file. The parameters currently defined are as follows: > >>>> firmware will pass the FDT address passed by the previous booting stage > >>>> to the next booting stage. > >>>> > >>>> -* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where > >>>> +* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the opensbi load address where > >>>> the FDT will be passed to the next booting stage. This offset is used as > >>>> relocatable address of the FDT passed to the next booting stage. If > >>>> *FW_PAYLOAD_FDT_ADDR* is also defined, the firmware will prefer *FW_PAYLOAD_FDT_ADDR*. > >>>> diff --git a/docs/platform/generic.md b/docs/platform/generic.md > >>>> index c29eb04..709b436 100644 > >>>> --- a/docs/platform/generic.md > >>>> +++ b/docs/platform/generic.md > >>>> @@ -9,10 +9,9 @@ boards. > >>>> > >>>> By default, the generic FDT platform makes following assumptions: > >>>> > >>>> -1. platform FW_TEXT_START is 0x80000000 > >>>> -2. platform features are default > >>>> -3. platform stack size is default > >>>> -4. platform has no quirks or work-arounds > >>>> +1. platform features are default > >>>> +2. platform stack size is default > >>>> +3. platform has no quirks or work-arounds > >>>> > >>>> The above assumptions (except 1) can be overridden by adding special platform > >>>> callbacks which will be called based on FDT root node compatible string. > >>>> @@ -33,10 +32,6 @@ Users of the generic FDT platform will have to ensure that: > >>>> To build the platform-specific library and firmware images, provide the > >>>> *PLATFORM=generic* parameter to the top level `make` command. > >>>> > >>>> -For custom FW_TEXT_START, we can build the platform-specific library and > >>>> -firmware images by passing *PLATFORM=generic FW_TEXT_START=<custom_text_start>* > >>>> -parameter to the top level `make` command. > >>>> - > >>>> Platform Options > >>>> ---------------- > >>>> > >>>> diff --git a/firmware/fw_base.S b/firmware/fw_base.S > >>>> index b950c0b..9f995a2 100644 > >>>> --- a/firmware/fw_base.S > >>>> +++ b/firmware/fw_base.S > >>>> @@ -53,9 +53,7 @@ _try_lottery: > >>>> bnez a6, _wait_for_boot_hart > >>>> > >>>> /* relocate the global table content */ > >>>> - li t0, FW_TEXT_START /* link start */ > >>>> - lla t1, _fw_start /* load start */ > >>>> - sub t2, t1, t0 /* load offset */ > >>>> + lla t2, _fw_start > >>>> lla t0, __rel_dyn_start > >>>> lla t1, __rel_dyn_end > >>>> beq t0, t1, _relocate_done > >>>> diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS > >>>> index fb47984..9d11db5 100644 > >>>> --- a/firmware/fw_base.ldS > >>>> +++ b/firmware/fw_base.ldS > >>>> @@ -7,8 +7,7 @@ > >>>> * Anup Patel <anup.patel@wdc.com> > >>>> */ > >>>> > >>>> - . = FW_TEXT_START; > >>>> - /* Don't add any section between FW_TEXT_START and _fw_start */ > >>>> + /* Don't add any section before _fw_start */ > >>>> PROVIDE(_fw_start = .); > >>>> > >>>> . = ALIGN(0x1000); /* Need this to create proper sections */ > >>>> diff --git a/firmware/fw_payload.elf.ldS b/firmware/fw_payload.elf.ldS > >>>> index f1a544b..94e1ac6 100644 > >>>> --- a/firmware/fw_payload.elf.ldS > >>>> +++ b/firmware/fw_payload.elf.ldS > >>>> @@ -15,7 +15,7 @@ SECTIONS > >>>> #include "fw_base.ldS" > >>>> > >>>> #ifdef FW_PAYLOAD_OFFSET > >>>> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; > >>>> + . = FW_PAYLOAD_OFFSET; > >>>> #else > >>>> . = ALIGN(FW_PAYLOAD_ALIGN); > >>>> #endif > >>>> diff --git a/firmware/objects.mk b/firmware/objects.mk > >>>> index e6b364b..a51ff65 100644 > >>>> --- a/firmware/objects.mk > >>>> +++ b/firmware/objects.mk > >>>> @@ -13,10 +13,6 @@ firmware-cflags-y += > >>>> firmware-asflags-y += > >>>> firmware-ldflags-y += > >>>> > >>>> -ifdef FW_TEXT_START > >>>> -firmware-genflags-y += -DFW_TEXT_START=$(FW_TEXT_START) > >>>> -endif > >>>> - > >>>> ifdef FW_FDT_PATH > >>>> firmware-genflags-y += -DFW_FDT_PATH=\"$(FW_FDT_PATH)\" > >>>> ifdef FW_FDT_PADDING > >>>> diff --git a/firmware/payloads/test.elf.ldS b/firmware/payloads/test.elf.ldS > >>>> index 2328a1b..08e008f 100644 > >>>> --- a/firmware/payloads/test.elf.ldS > >>>> +++ b/firmware/payloads/test.elf.ldS > >>>> @@ -13,7 +13,7 @@ ENTRY(_start) > >>>> SECTIONS > >>>> { > >>>> #ifdef FW_PAYLOAD_OFFSET > >>>> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; > >>>> + . = FW_PAYLOAD_OFFSET; > >>>> #else > >>>> . = ALIGN(FW_PAYLOAD_ALIGN); > >>>> #endif > >>>> diff --git a/platform/fpga/ariane/objects.mk b/platform/fpga/ariane/objects.mk > >>>> index 83581ac..d1177f4 100644 > >>>> --- a/platform/fpga/ariane/objects.mk > >>>> +++ b/platform/fpga/ariane/objects.mk > >>>> @@ -17,7 +17,6 @@ platform-objs-y += platform.o > >>>> PLATFORM_RISCV_XLEN = 64 > >>>> > >>>> # Blobs to build > >>>> -FW_TEXT_START=0x80000000 > >>>> FW_JUMP=n > >>>> > >>>> ifeq ($(PLATFORM_RISCV_XLEN), 32) > >>>> diff --git a/platform/fpga/openpiton/objects.mk b/platform/fpga/openpiton/objects.mk > >>>> index c8c345a..1a0ce0c 100644 > >>>> --- a/platform/fpga/openpiton/objects.mk > >>>> +++ b/platform/fpga/openpiton/objects.mk > >>>> @@ -16,7 +16,6 @@ platform-objs-y += platform.o > >>>> PLATFORM_RISCV_XLEN = 64 > >>>> > >>>> # Blobs to build > >>>> -FW_TEXT_START=0x80000000 > >>>> FW_JUMP=n > >>>> > >>>> ifeq ($(PLATFORM_RISCV_XLEN), 32) > >>>> diff --git a/platform/generic/objects.mk b/platform/generic/objects.mk > >>>> index 85aa723..c215935 100644 > >>>> --- a/platform/generic/objects.mk > >>>> +++ b/platform/generic/objects.mk > >>>> @@ -15,14 +15,13 @@ platform-ldflags-y = > >>>> > >>>> # Command for platform specific "make run" > >>>> platform-runcmd = qemu-system-riscv$(PLATFORM_RISCV_XLEN) -M virt -m 256M \ > >>>> - -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.elf > >>>> + -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.bin > >>>> > >>>> # Objects to build > >>>> platform-objs-y += platform.o > >>>> platform-objs-y += platform_override_modules.o > >>>> > >>>> # Blobs to build > >>>> -FW_TEXT_START=0x80000000 > >>>> FW_DYNAMIC=y > >>>> FW_JUMP=y > >>>> ifeq ($(PLATFORM_RISCV_XLEN), 32) > >>>> diff --git a/platform/kendryte/k210/objects.mk b/platform/kendryte/k210/objects.mk > >>>> index 1bfb898..efac3d2 100644 > >>>> --- a/platform/kendryte/k210/objects.mk > >>>> +++ b/platform/kendryte/k210/objects.mk > >>>> @@ -21,6 +21,5 @@ platform-varprefix-k210.o = dt_k210 > >>>> platform-padding-k210.o = 2048 > >>>> > >>>> # Blobs to build > >>>> -FW_TEXT_START=0x80000000 > >>>> FW_PAYLOAD=y > >>>> FW_PAYLOAD_ALIGN=0x1000 > >>>> diff --git a/platform/nuclei/ux600/objects.mk b/platform/nuclei/ux600/objects.mk > >>>> index 7c429e0..2753a7f 100644 > >>>> --- a/platform/nuclei/ux600/objects.mk > >>>> +++ b/platform/nuclei/ux600/objects.mk > >>>> @@ -22,7 +22,6 @@ platform-runcmd = xl_spike \ > >>>> platform-objs-y += platform.o > >>>> > >>>> # Blobs to build > >>>> -FW_TEXT_START=0xA0000000 > >>>> FW_DYNAMIC=y > >>>> FW_JUMP=y > >>>> > >>>> diff --git a/platform/template/objects.mk b/platform/template/objects.mk > >>>> index b143cbc..f240a55 100644 > >>>> --- a/platform/template/objects.mk > >>>> +++ b/platform/template/objects.mk > >>>> @@ -41,9 +41,6 @@ platform-objs-y += platform.o > >>>> # > >>>> # platform-objs-y += <dt file name>.o > >>>> > >>>> -# Firmware load address configuration. This is mandatory. > >>>> -FW_TEXT_START=0x80000000 > >>>> - > >>>> # Optional parameter for path to external FDT > >>>> # FW_FDT_PATH="path to platform flattened device tree file" > >>>> > >>>> @@ -69,8 +66,7 @@ FW_JUMP=<y|n> > >>>> # endif > >>>> # FW_JUMP_FDT_OFFSET=0x2200000 > >>>> # > >>>> -# You can use fixed address for jump firmware as an alternative option, > >>>> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. > >>>> +# You can use fixed address for jump firmware as an alternative option. > >>>> # SBI will prefer "<X>_ADDR" if both "<X>_ADDR" and "<X>_OFFSET" are > >>>> # defined > >>>> # ifeq ($(PLATFORM_RISCV_XLEN), 32) > >>>> @@ -97,8 +93,7 @@ endif > >>>> # FW_PAYLOAD_PATH="path to next boot stage binary image file" > >>>> # FW_PAYLOAD_FDT_OFFSET=0x2200000 > >>>> # > >>>> -# You can use fixed address for payload firmware as an alternative option, > >>>> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. > >>>> +# You can use fixed address for payload firmware as an alternative option. > >>>> # SBI will prefer "FW_PAYLOAD_FDT_ADDR" if both "FW_PAYLOAD_FDT_OFFSET" > >>>> # and "FW_PAYLOAD_FDT_ADDR" are defined. > >>>> # FW_PAYLOAD_FDT_ADDR=0x82200000 > >>>> -- > >>>> 2.43.0 > >
On Tue, May 14, 2024 at 10:20 PM Cheng Yang <yangcheng.work@foxmail.com> wrote: > > >On Tue, May 14, 2024 at 9:58 PM Clément Léger <cleger@rivosinc.com> wrote: > >> > >> > >> > >> On 10/04/2024 07:18, Anup Patel wrote: > >> > On Mon, Apr 8, 2024 at 9:01 PM Xiang W <wxjstz@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@126.com> > >> > > >> > LGTM. > >> > > >> > Reviewed-by: Anup Patel <anup@brainfault.org> > >> > Tested-by: Anup Patel <anup@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. If FW_TEXT_START is needed only for convenience in debugging then we can bring it back as an optional parameter which is by default set to 0x0. Regards, Anup > > Best regards, > Cheng Yang > > >> > > >> >> --- > >> >> > >> >> v4 changes: > >> >> - only remove FW_TEXT_START by anup's suggestion > >> >> > >> >> docs/firmware/fw.md | 2 -- > >> >> docs/firmware/fw_jump.md | 4 ++-- > >> >> docs/firmware/fw_payload.md | 6 +++--- > >> >> docs/platform/generic.md | 11 +++-------- > >> >> firmware/fw_base.S | 4 +--- > >> >> firmware/fw_base.ldS | 3 +-- > >> >> firmware/fw_payload.elf.ldS | 2 +- > >> >> firmware/objects.mk | 4 ---- > >> >> firmware/payloads/test.elf.ldS | 2 +- > >> >> platform/fpga/ariane/objects.mk | 1 - > >> >> platform/fpga/openpiton/objects.mk | 1 - > >> >> platform/generic/objects.mk | 3 +-- > >> >> platform/kendryte/k210/objects.mk | 1 - > >> >> platform/nuclei/ux600/objects.mk | 1 - > >> >> platform/template/objects.mk | 9 ++------- > >> >> 15 files changed, 15 insertions(+), 39 deletions(-) > >> >> > >> >> diff --git a/docs/firmware/fw.md b/docs/firmware/fw.md > >> >> index 2f4deb5..3cc0262 100644 > >> >> --- a/docs/firmware/fw.md > >> >> +++ b/docs/firmware/fw.md > >> >> @@ -61,8 +61,6 @@ Firmware Configuration and Compilation > >> >> All firmware types support the following common compile time configuration > >> >> parameters: > >> >> > >> >> -* **FW_TEXT_START** - Defines the execution address of the OpenSBI firmware. > >> >> - This configuration parameter is mandatory. > >> >> * **FW_FDT_PATH** - Path to an external flattened device tree binary file to > >> >> be embedded in the *.rodata* section of the final firmware. If this option > >> >> is not provided then the firmware will expect the FDT to be passed as an > >> >> diff --git a/docs/firmware/fw_jump.md b/docs/firmware/fw_jump.md > >> >> index 2ee6b29..66be016 100644 > >> >> --- a/docs/firmware/fw_jump.md > >> >> +++ b/docs/firmware/fw_jump.md > >> >> @@ -35,7 +35,7 @@ follows: > >> >> At least one of *FW_JUMP_ADDR* and *FW_JUMP_OFFSET* (see below) should be > >> >> defined. Compilation errors will result from not defining one of them. > >> >> > >> >> -* **FW_JUMP_OFFSET** - Address offset from the *FW_TEXT_START* where the > >> >> +* **FW_JUMP_OFFSET** - Address offset from the opensbi load address where the > >> >> entry point of the next booting stage is located. This offset is used as > >> >> relocatable address of the next booting stage entry point. If *FW_JUMP_ADDR* > >> >> is also defined, the firmware will prefer *FW_JUMP_ADDR*. > >> >> @@ -62,7 +62,7 @@ follows: > >> >> echo fdt overlaps kernel, increase FW_JUMP_FDT_ADDR > >> >> ``` > >> >> > >> >> -* **FW_JUMP_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where > >> >> +* **FW_JUMP_FDT_OFFSET** - Address offset from the opensbi load address where > >> >> the FDT will be passed to the next booting stage. This offset is used > >> >> as relocatable address of the FDT passed to the next booting stage. If > >> >> *FW_JUMP_FDT_ADDR* is also defined, the firmware will prefer > >> >> diff --git a/docs/firmware/fw_payload.md b/docs/firmware/fw_payload.md > >> >> index a67fc50..d25be60 100644 > >> >> --- a/docs/firmware/fw_payload.md > >> >> +++ b/docs/firmware/fw_payload.md > >> >> @@ -36,8 +36,8 @@ options. These configuration parameters can be defined using either the top > >> >> level `make` command line or the target platform *objects.mk* configuration > >> >> file. The parameters currently defined are as follows: > >> >> > >> >> -* **FW_PAYLOAD_OFFSET** - Offset from *FW_TEXT_START* where the payload binary > >> >> - will be linked in the final *FW_PAYLOAD* firmware binary image. This > >> >> +* **FW_PAYLOAD_OFFSET** - Offset from the opensbi load address where the payload > >> >> + binary will be linked in the final *FW_PAYLOAD* firmware binary image. This > >> >> configuration parameter is mandatory if *FW_PAYLOAD_ALIGN* is not defined. > >> >> Compilation errors will result from an incorrect definition of > >> >> *FW_PAYLOAD_OFFSET* or of *FW_PAYLOAD_ALIGN*, or if neither of these > >> >> @@ -62,7 +62,7 @@ file. The parameters currently defined are as follows: > >> >> firmware will pass the FDT address passed by the previous booting stage > >> >> to the next booting stage. > >> >> > >> >> -* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where > >> >> +* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the opensbi load address where > >> >> the FDT will be passed to the next booting stage. This offset is used as > >> >> relocatable address of the FDT passed to the next booting stage. If > >> >> *FW_PAYLOAD_FDT_ADDR* is also defined, the firmware will prefer *FW_PAYLOAD_FDT_ADDR*. > >> >> diff --git a/docs/platform/generic.md b/docs/platform/generic.md > >> >> index c29eb04..709b436 100644 > >> >> --- a/docs/platform/generic.md > >> >> +++ b/docs/platform/generic.md > >> >> @@ -9,10 +9,9 @@ boards. > >> >> > >> >> By default, the generic FDT platform makes following assumptions: > >> >> > >> >> -1. platform FW_TEXT_START is 0x80000000 > >> >> -2. platform features are default > >> >> -3. platform stack size is default > >> >> -4. platform has no quirks or work-arounds > >> >> +1. platform features are default > >> >> +2. platform stack size is default > >> >> +3. platform has no quirks or work-arounds > >> >> > >> >> The above assumptions (except 1) can be overridden by adding special platform > >> >> callbacks which will be called based on FDT root node compatible string. > >> >> @@ -33,10 +32,6 @@ Users of the generic FDT platform will have to ensure that: > >> >> To build the platform-specific library and firmware images, provide the > >> >> *PLATFORM=generic* parameter to the top level `make` command. > >> >> > >> >> -For custom FW_TEXT_START, we can build the platform-specific library and > >> >> -firmware images by passing *PLATFORM=generic FW_TEXT_START=<custom_text_start>* > >> >> -parameter to the top level `make` command. > >> >> - > >> >> Platform Options > >> >> ---------------- > >> >> > >> >> diff --git a/firmware/fw_base.S b/firmware/fw_base.S > >> >> index b950c0b..9f995a2 100644 > >> >> --- a/firmware/fw_base.S > >> >> +++ b/firmware/fw_base.S > >> >> @@ -53,9 +53,7 @@ _try_lottery: > >> >> bnez a6, _wait_for_boot_hart > >> >> > >> >> /* relocate the global table content */ > >> >> - li t0, FW_TEXT_START /* link start */ > >> >> - lla t1, _fw_start /* load start */ > >> >> - sub t2, t1, t0 /* load offset */ > >> >> + lla t2, _fw_start > >> >> lla t0, __rel_dyn_start > >> >> lla t1, __rel_dyn_end > >> >> beq t0, t1, _relocate_done > >> >> diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS > >> >> index fb47984..9d11db5 100644 > >> >> --- a/firmware/fw_base.ldS > >> >> +++ b/firmware/fw_base.ldS > >> >> @@ -7,8 +7,7 @@ > >> >> * Anup Patel <anup.patel@wdc.com> > >> >> */ > >> >> > >> >> - . = FW_TEXT_START; > >> >> - /* Don't add any section between FW_TEXT_START and _fw_start */ > >> >> + /* Don't add any section before _fw_start */ > >> >> PROVIDE(_fw_start = .); > >> >> > >> >> . = ALIGN(0x1000); /* Need this to create proper sections */ > >> >> diff --git a/firmware/fw_payload.elf.ldS b/firmware/fw_payload.elf.ldS > >> >> index f1a544b..94e1ac6 100644 > >> >> --- a/firmware/fw_payload.elf.ldS > >> >> +++ b/firmware/fw_payload.elf.ldS > >> >> @@ -15,7 +15,7 @@ SECTIONS > >> >> #include "fw_base.ldS" > >> >> > >> >> #ifdef FW_PAYLOAD_OFFSET > >> >> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; > >> >> + . = FW_PAYLOAD_OFFSET; > >> >> #else > >> >> . = ALIGN(FW_PAYLOAD_ALIGN); > >> >> #endif > >> >> diff --git a/firmware/objects.mk b/firmware/objects.mk > >> >> index e6b364b..a51ff65 100644 > >> >> --- a/firmware/objects.mk > >> >> +++ b/firmware/objects.mk > >> >> @@ -13,10 +13,6 @@ firmware-cflags-y += > >> >> firmware-asflags-y += > >> >> firmware-ldflags-y += > >> >> > >> >> -ifdef FW_TEXT_START > >> >> -firmware-genflags-y += -DFW_TEXT_START=$(FW_TEXT_START) > >> >> -endif > >> >> - > >> >> ifdef FW_FDT_PATH > >> >> firmware-genflags-y += -DFW_FDT_PATH=\"$(FW_FDT_PATH)\" > >> >> ifdef FW_FDT_PADDING > >> >> diff --git a/firmware/payloads/test.elf.ldS b/firmware/payloads/test.elf.ldS > >> >> index 2328a1b..08e008f 100644 > >> >> --- a/firmware/payloads/test.elf.ldS > >> >> +++ b/firmware/payloads/test.elf.ldS > >> >> @@ -13,7 +13,7 @@ ENTRY(_start) > >> >> SECTIONS > >> >> { > >> >> #ifdef FW_PAYLOAD_OFFSET > >> >> - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; > >> >> + . = FW_PAYLOAD_OFFSET; > >> >> #else > >> >> . = ALIGN(FW_PAYLOAD_ALIGN); > >> >> #endif > >> >> diff --git a/platform/fpga/ariane/objects.mk b/platform/fpga/ariane/objects.mk > >> >> index 83581ac..d1177f4 100644 > >> >> --- a/platform/fpga/ariane/objects.mk > >> >> +++ b/platform/fpga/ariane/objects.mk > >> >> @@ -17,7 +17,6 @@ platform-objs-y += platform.o > >> >> PLATFORM_RISCV_XLEN = 64 > >> >> > >> >> # Blobs to build > >> >> -FW_TEXT_START=0x80000000 > >> >> FW_JUMP=n > >> >> > >> >> ifeq ($(PLATFORM_RISCV_XLEN), 32) > >> >> diff --git a/platform/fpga/openpiton/objects.mk b/platform/fpga/openpiton/objects.mk > >> >> index c8c345a..1a0ce0c 100644 > >> >> --- a/platform/fpga/openpiton/objects.mk > >> >> +++ b/platform/fpga/openpiton/objects.mk > >> >> @@ -16,7 +16,6 @@ platform-objs-y += platform.o > >> >> PLATFORM_RISCV_XLEN = 64 > >> >> > >> >> # Blobs to build > >> >> -FW_TEXT_START=0x80000000 > >> >> FW_JUMP=n > >> >> > >> >> ifeq ($(PLATFORM_RISCV_XLEN), 32) > >> >> diff --git a/platform/generic/objects.mk b/platform/generic/objects.mk > >> >> index 85aa723..c215935 100644 > >> >> --- a/platform/generic/objects.mk > >> >> +++ b/platform/generic/objects.mk > >> >> @@ -15,14 +15,13 @@ platform-ldflags-y = > >> >> > >> >> # Command for platform specific "make run" > >> >> platform-runcmd = qemu-system-riscv$(PLATFORM_RISCV_XLEN) -M virt -m 256M \ > >> >> - -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.elf > >> >> + -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.bin > >> >> > >> >> # Objects to build > >> >> platform-objs-y += platform.o > >> >> platform-objs-y += platform_override_modules.o > >> >> > >> >> # Blobs to build > >> >> -FW_TEXT_START=0x80000000 > >> >> FW_DYNAMIC=y > >> >> FW_JUMP=y > >> >> ifeq ($(PLATFORM_RISCV_XLEN), 32) > >> >> diff --git a/platform/kendryte/k210/objects.mk b/platform/kendryte/k210/objects.mk > >> >> index 1bfb898..efac3d2 100644 > >> >> --- a/platform/kendryte/k210/objects.mk > >> >> +++ b/platform/kendryte/k210/objects.mk > >> >> @@ -21,6 +21,5 @@ platform-varprefix-k210.o = dt_k210 > >> >> platform-padding-k210.o = 2048 > >> >> > >> >> # Blobs to build > >> >> -FW_TEXT_START=0x80000000 > >> >> FW_PAYLOAD=y > >> >> FW_PAYLOAD_ALIGN=0x1000 > >> >> diff --git a/platform/nuclei/ux600/objects.mk b/platform/nuclei/ux600/objects.mk > >> >> index 7c429e0..2753a7f 100644 > >> >> --- a/platform/nuclei/ux600/objects.mk > >> >> +++ b/platform/nuclei/ux600/objects.mk > >> >> @@ -22,7 +22,6 @@ platform-runcmd = xl_spike \ > >> >> platform-objs-y += platform.o > >> >> > >> >> # Blobs to build > >> >> -FW_TEXT_START=0xA0000000 > >> >> FW_DYNAMIC=y > >> >> FW_JUMP=y > >> >> > >> >> diff --git a/platform/template/objects.mk b/platform/template/objects.mk > >> >> index b143cbc..f240a55 100644 > >> >> --- a/platform/template/objects.mk > >> >> +++ b/platform/template/objects.mk > >> >> @@ -41,9 +41,6 @@ platform-objs-y += platform.o > >> >> # > >> >> # platform-objs-y += <dt file="" name="">.o > >> >> > >> >> -# Firmware load address configuration. This is mandatory. > >> >> -FW_TEXT_START=0x80000000 > >> >> - > >> >> # Optional parameter for path to external FDT > >> >> # FW_FDT_PATH="path to platform flattened device tree file" > >> >> > >> >> @@ -69,8 +66,7 @@ FW_JUMP=<y|n> > >> >> # endif > >> >> # FW_JUMP_FDT_OFFSET=0x2200000 > >> >> # > >> >> -# You can use fixed address for jump firmware as an alternative option, > >> >> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. > >> >> +# You can use fixed address for jump firmware as an alternative option. > >> >> # SBI will prefer "<x>_ADDR" if both "<x>_ADDR" and "<x>_OFFSET" are > >> >> # defined > >> >> # ifeq ($(PLATFORM_RISCV_XLEN), 32) > >> >> @@ -97,8 +93,7 @@ endif > >> >> # FW_PAYLOAD_PATH="path to next boot stage binary image file" > >> >> # FW_PAYLOAD_FDT_OFFSET=0x2200000 > >> >> # > >> >> -# You can use fixed address for payload firmware as an alternative option, > >> >> -# but this may fail when setting wrong FW_TEXT_START. Use with caution. > >> >> +# You can use fixed address for payload firmware as an alternative option. > >> >> # SBI will prefer "FW_PAYLOAD_FDT_ADDR" if both "FW_PAYLOAD_FDT_OFFSET" > >> >> # and "FW_PAYLOAD_FDT_ADDR" are defined. > >> >> # FW_PAYLOAD_FDT_ADDR=0x82200000 > >> >> -- > >> >> 2.43.0 > >> >> > >> > > > > >-- > >opensbi mailing list > >opensbi@lists.infradead.org > >http://lists.infradead.org/mailman/listinfo/opensbi</x></x></x></y|n></dt></anup.patel@wdc.com></custom_text_start></std::__cxx11::basic_string<char></anup@brainfault.org></anup@brainfault.org></wxjstz@126.com></wxjstz@126.com></cleger@rivosinc.com>
On 14/05/2024 19:46, Jessica Clarke wrote: > On 14 May 2024, at 18:25, Cheng Yang <yangcheng.work@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 > >
On Wed, May 15, 2024 at 1:12 PM Clément Léger <cleger@rivosinc.com> wrote: > > > > On 14/05/2024 19:46, Jessica Clarke wrote: > > On 14 May 2024, at 18:25, Cheng Yang <yangcheng.work@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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
diff --git a/docs/firmware/fw.md b/docs/firmware/fw.md index 2f4deb5..3cc0262 100644 --- a/docs/firmware/fw.md +++ b/docs/firmware/fw.md @@ -61,8 +61,6 @@ Firmware Configuration and Compilation All firmware types support the following common compile time configuration parameters: -* **FW_TEXT_START** - Defines the execution address of the OpenSBI firmware. - This configuration parameter is mandatory. * **FW_FDT_PATH** - Path to an external flattened device tree binary file to be embedded in the *.rodata* section of the final firmware. If this option is not provided then the firmware will expect the FDT to be passed as an diff --git a/docs/firmware/fw_jump.md b/docs/firmware/fw_jump.md index 2ee6b29..66be016 100644 --- a/docs/firmware/fw_jump.md +++ b/docs/firmware/fw_jump.md @@ -35,7 +35,7 @@ follows: At least one of *FW_JUMP_ADDR* and *FW_JUMP_OFFSET* (see below) should be defined. Compilation errors will result from not defining one of them. -* **FW_JUMP_OFFSET** - Address offset from the *FW_TEXT_START* where the +* **FW_JUMP_OFFSET** - Address offset from the opensbi load address where the entry point of the next booting stage is located. This offset is used as relocatable address of the next booting stage entry point. If *FW_JUMP_ADDR* is also defined, the firmware will prefer *FW_JUMP_ADDR*. @@ -62,7 +62,7 @@ follows: echo fdt overlaps kernel, increase FW_JUMP_FDT_ADDR ``` -* **FW_JUMP_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where +* **FW_JUMP_FDT_OFFSET** - Address offset from the opensbi load address where the FDT will be passed to the next booting stage. This offset is used as relocatable address of the FDT passed to the next booting stage. If *FW_JUMP_FDT_ADDR* is also defined, the firmware will prefer diff --git a/docs/firmware/fw_payload.md b/docs/firmware/fw_payload.md index a67fc50..d25be60 100644 --- a/docs/firmware/fw_payload.md +++ b/docs/firmware/fw_payload.md @@ -36,8 +36,8 @@ options. These configuration parameters can be defined using either the top level `make` command line or the target platform *objects.mk* configuration file. The parameters currently defined are as follows: -* **FW_PAYLOAD_OFFSET** - Offset from *FW_TEXT_START* where the payload binary - will be linked in the final *FW_PAYLOAD* firmware binary image. This +* **FW_PAYLOAD_OFFSET** - Offset from the opensbi load address where the payload + binary will be linked in the final *FW_PAYLOAD* firmware binary image. This configuration parameter is mandatory if *FW_PAYLOAD_ALIGN* is not defined. Compilation errors will result from an incorrect definition of *FW_PAYLOAD_OFFSET* or of *FW_PAYLOAD_ALIGN*, or if neither of these @@ -62,7 +62,7 @@ file. The parameters currently defined are as follows: firmware will pass the FDT address passed by the previous booting stage to the next booting stage. -* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the *FW_TEXT_START* where +* **FW_PAYLOAD_FDT_OFFSET** - Address offset from the opensbi load address where the FDT will be passed to the next booting stage. This offset is used as relocatable address of the FDT passed to the next booting stage. If *FW_PAYLOAD_FDT_ADDR* is also defined, the firmware will prefer *FW_PAYLOAD_FDT_ADDR*. diff --git a/docs/platform/generic.md b/docs/platform/generic.md index c29eb04..709b436 100644 --- a/docs/platform/generic.md +++ b/docs/platform/generic.md @@ -9,10 +9,9 @@ boards. By default, the generic FDT platform makes following assumptions: -1. platform FW_TEXT_START is 0x80000000 -2. platform features are default -3. platform stack size is default -4. platform has no quirks or work-arounds +1. platform features are default +2. platform stack size is default +3. platform has no quirks or work-arounds The above assumptions (except 1) can be overridden by adding special platform callbacks which will be called based on FDT root node compatible string. @@ -33,10 +32,6 @@ Users of the generic FDT platform will have to ensure that: To build the platform-specific library and firmware images, provide the *PLATFORM=generic* parameter to the top level `make` command. -For custom FW_TEXT_START, we can build the platform-specific library and -firmware images by passing *PLATFORM=generic FW_TEXT_START=<custom_text_start>* -parameter to the top level `make` command. - Platform Options ---------------- diff --git a/firmware/fw_base.S b/firmware/fw_base.S index b950c0b..9f995a2 100644 --- a/firmware/fw_base.S +++ b/firmware/fw_base.S @@ -53,9 +53,7 @@ _try_lottery: bnez a6, _wait_for_boot_hart /* relocate the global table content */ - li t0, FW_TEXT_START /* link start */ - lla t1, _fw_start /* load start */ - sub t2, t1, t0 /* load offset */ + lla t2, _fw_start lla t0, __rel_dyn_start lla t1, __rel_dyn_end beq t0, t1, _relocate_done diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS index fb47984..9d11db5 100644 --- a/firmware/fw_base.ldS +++ b/firmware/fw_base.ldS @@ -7,8 +7,7 @@ * Anup Patel <anup.patel@wdc.com> */ - . = FW_TEXT_START; - /* Don't add any section between FW_TEXT_START and _fw_start */ + /* Don't add any section before _fw_start */ PROVIDE(_fw_start = .); . = ALIGN(0x1000); /* Need this to create proper sections */ diff --git a/firmware/fw_payload.elf.ldS b/firmware/fw_payload.elf.ldS index f1a544b..94e1ac6 100644 --- a/firmware/fw_payload.elf.ldS +++ b/firmware/fw_payload.elf.ldS @@ -15,7 +15,7 @@ SECTIONS #include "fw_base.ldS" #ifdef FW_PAYLOAD_OFFSET - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; + . = FW_PAYLOAD_OFFSET; #else . = ALIGN(FW_PAYLOAD_ALIGN); #endif diff --git a/firmware/objects.mk b/firmware/objects.mk index e6b364b..a51ff65 100644 --- a/firmware/objects.mk +++ b/firmware/objects.mk @@ -13,10 +13,6 @@ firmware-cflags-y += firmware-asflags-y += firmware-ldflags-y += -ifdef FW_TEXT_START -firmware-genflags-y += -DFW_TEXT_START=$(FW_TEXT_START) -endif - ifdef FW_FDT_PATH firmware-genflags-y += -DFW_FDT_PATH=\"$(FW_FDT_PATH)\" ifdef FW_FDT_PADDING diff --git a/firmware/payloads/test.elf.ldS b/firmware/payloads/test.elf.ldS index 2328a1b..08e008f 100644 --- a/firmware/payloads/test.elf.ldS +++ b/firmware/payloads/test.elf.ldS @@ -13,7 +13,7 @@ ENTRY(_start) SECTIONS { #ifdef FW_PAYLOAD_OFFSET - . = FW_TEXT_START + FW_PAYLOAD_OFFSET; + . = FW_PAYLOAD_OFFSET; #else . = ALIGN(FW_PAYLOAD_ALIGN); #endif diff --git a/platform/fpga/ariane/objects.mk b/platform/fpga/ariane/objects.mk index 83581ac..d1177f4 100644 --- a/platform/fpga/ariane/objects.mk +++ b/platform/fpga/ariane/objects.mk @@ -17,7 +17,6 @@ platform-objs-y += platform.o PLATFORM_RISCV_XLEN = 64 # Blobs to build -FW_TEXT_START=0x80000000 FW_JUMP=n ifeq ($(PLATFORM_RISCV_XLEN), 32) diff --git a/platform/fpga/openpiton/objects.mk b/platform/fpga/openpiton/objects.mk index c8c345a..1a0ce0c 100644 --- a/platform/fpga/openpiton/objects.mk +++ b/platform/fpga/openpiton/objects.mk @@ -16,7 +16,6 @@ platform-objs-y += platform.o PLATFORM_RISCV_XLEN = 64 # Blobs to build -FW_TEXT_START=0x80000000 FW_JUMP=n ifeq ($(PLATFORM_RISCV_XLEN), 32) diff --git a/platform/generic/objects.mk b/platform/generic/objects.mk index 85aa723..c215935 100644 --- a/platform/generic/objects.mk +++ b/platform/generic/objects.mk @@ -15,14 +15,13 @@ platform-ldflags-y = # Command for platform specific "make run" platform-runcmd = qemu-system-riscv$(PLATFORM_RISCV_XLEN) -M virt -m 256M \ - -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.elf + -nographic -bios $(build_dir)/platform/generic/firmware/fw_payload.bin # Objects to build platform-objs-y += platform.o platform-objs-y += platform_override_modules.o # Blobs to build -FW_TEXT_START=0x80000000 FW_DYNAMIC=y FW_JUMP=y ifeq ($(PLATFORM_RISCV_XLEN), 32) diff --git a/platform/kendryte/k210/objects.mk b/platform/kendryte/k210/objects.mk index 1bfb898..efac3d2 100644 --- a/platform/kendryte/k210/objects.mk +++ b/platform/kendryte/k210/objects.mk @@ -21,6 +21,5 @@ platform-varprefix-k210.o = dt_k210 platform-padding-k210.o = 2048 # Blobs to build -FW_TEXT_START=0x80000000 FW_PAYLOAD=y FW_PAYLOAD_ALIGN=0x1000 diff --git a/platform/nuclei/ux600/objects.mk b/platform/nuclei/ux600/objects.mk index 7c429e0..2753a7f 100644 --- a/platform/nuclei/ux600/objects.mk +++ b/platform/nuclei/ux600/objects.mk @@ -22,7 +22,6 @@ platform-runcmd = xl_spike \ platform-objs-y += platform.o # Blobs to build -FW_TEXT_START=0xA0000000 FW_DYNAMIC=y FW_JUMP=y diff --git a/platform/template/objects.mk b/platform/template/objects.mk index b143cbc..f240a55 100644 --- a/platform/template/objects.mk +++ b/platform/template/objects.mk @@ -41,9 +41,6 @@ platform-objs-y += platform.o # # platform-objs-y += <dt file name>.o -# Firmware load address configuration. This is mandatory. -FW_TEXT_START=0x80000000 - # Optional parameter for path to external FDT # FW_FDT_PATH="path to platform flattened device tree file" @@ -69,8 +66,7 @@ FW_JUMP=<y|n> # endif # FW_JUMP_FDT_OFFSET=0x2200000 # -# You can use fixed address for jump firmware as an alternative option, -# but this may fail when setting wrong FW_TEXT_START. Use with caution. +# You can use fixed address for jump firmware as an alternative option. # SBI will prefer "<X>_ADDR" if both "<X>_ADDR" and "<X>_OFFSET" are # defined # ifeq ($(PLATFORM_RISCV_XLEN), 32) @@ -97,8 +93,7 @@ endif # FW_PAYLOAD_PATH="path to next boot stage binary image file" # FW_PAYLOAD_FDT_OFFSET=0x2200000 # -# You can use fixed address for payload firmware as an alternative option, -# but this may fail when setting wrong FW_TEXT_START. Use with caution. +# You can use fixed address for payload firmware as an alternative option. # SBI will prefer "FW_PAYLOAD_FDT_ADDR" if both "FW_PAYLOAD_FDT_OFFSET" # and "FW_PAYLOAD_FDT_ADDR" are defined. # FW_PAYLOAD_FDT_ADDR=0x82200000
Now opensbi can run at any address via dynamic relocation. We can remove FW_TEXT_START. Signed-off-by: Xiang W <wxjstz@126.com> --- v4 changes: - only remove FW_TEXT_START by anup's suggestion docs/firmware/fw.md | 2 -- docs/firmware/fw_jump.md | 4 ++-- docs/firmware/fw_payload.md | 6 +++--- docs/platform/generic.md | 11 +++-------- firmware/fw_base.S | 4 +--- firmware/fw_base.ldS | 3 +-- firmware/fw_payload.elf.ldS | 2 +- firmware/objects.mk | 4 ---- firmware/payloads/test.elf.ldS | 2 +- platform/fpga/ariane/objects.mk | 1 - platform/fpga/openpiton/objects.mk | 1 - platform/generic/objects.mk | 3 +-- platform/kendryte/k210/objects.mk | 1 - platform/nuclei/ux600/objects.mk | 1 - platform/template/objects.mk | 9 ++------- 15 files changed, 15 insertions(+), 39 deletions(-)