Allow elfloader to put kernel images directly after firmware on RISCV #135

kent-mcleod · 2022-02-01T11:36:32Z

opensbi puts the elfloader in a fixed location right after it in memory which is where we normally wish to place the kernel with the elfloader higher in memory. This allows the kernel to immediately reclaim the elfloader memory and slightly reduces memory fragmentation by allowing the kernel reserved memory to be located next to the opensbi reserved memory and all memory after free for application use.

Test with: seL4/seL4#759

lsf37 · 2022-02-01T21:15:51Z

I'm not deep enough in the RISCV details to review this one, but I am in favour of the general idea.

yyshen · 2022-02-01T22:18:54Z

elfloader-tool/src/common.c

+        return (char *)0;
+    }
+
+    /* Check that the current image location doesn't overlap with the


we could copy backwards if they overlap, just not sure if that is necessary.

Actually the memmove implementation in the elfloader can handle this. I originally thought that it was just the same implementation of memcpy. I can fix this up to handle overlap apart from the code segment apart so that it's like the original assembly (which I originally thought was not properly handling overlap).

Isn't the issue when there is overlapping, there is chance that the code that is currently executing is not overwritten and thus the result might be undefined? So this must be detected and avoided. Same for overwriting the stack/global in bss

I've fixed this up to only check for collision up until _archive_start.

yyshen · 2022-02-01T22:20:28Z

elfloader-tool/src/arch-riscv/boot.c

@@ -89,11 +91,6 @@ static int map_kernel_window(struct image_info *kernel_info)

    /* Map the elfloader into the new address space */

-    if (!IS_ALIGNED((uintptr_t)_text, PT_LEVEL_2_BITS)) {


could you explain why the check existed previously and why it can be removed a bit?

Because the kernel also could only handle setting up the kernel mappings if the image was on a page boundary and because the elfloader was always placed on a page boundary by the bbl/opensbi implementation. The rest of map_kernel_window was specialized to these assumptions and so had the checks to catch when there was divergence. Now that divergence can be handled by the code, the checks should be able to be removed.

yyshen · 2022-02-02T02:13:31Z

cmake-tool/helpers/application_settings.cmake

@@ -49,9 +49,6 @@ function(ApplyData61ElfLoaderSettings kernel_platform kernel_sel4_arch)
    if(KernelPlatformZynqmp AND KernelSel4ArchAarch32)
        set(IMAGE_START_ADDR 0x8000000 CACHE INTERNAL "" FORCE)
    endif()
-    if(KernelPlatformSpike AND KernelSel4ArchRiscV32)
-        set(IMAGE_START_ADDR 0x80400000 CACHE INTERNAL "" FORCE)


this can be removed because of the DTS overlay change in another PR?

Yea, it was being ignored by opensbi before and wasn't having any effect.

elfloader-tool/src/arch-riscv/crt0.S

elfloader-tool/src/arch-arm/64/crt0.S

elfloader-tool/src/arch-riscv/boot.c

kent-mcleod · 2022-02-04T11:21:58Z

These changes also allow the issue underlying #76 to be resolved.

elfloader-tool/src/arch-riscv/boot.c

kent-mcleod · 2022-02-09T08:07:56Z

This relocation needs to consider the DTB passed through from opensbi when doing relocations otherwise it could get overwritten. This is an outstanding issue on Arm (https://sel4.atlassian.net/browse/SELFOUR-2138)

Porting from assembly to a higher level language makes this function more portable. Signed-off-by: Kent McLeod <[email protected]>

It's not necessary to require the kernel and ELFloader images are loaded at the start of page boundaries. Signed-off-by: Kent McLeod <[email protected]>

When booting via opensbi, the image format is a binary image and so if the .bss section is at the end it must be initialized unconditionally. Signed-off-by: Kent McLeod <[email protected]>

This reverts commit 8de8c9f. Signed-off-by: Kent McLeod <[email protected]>

The SBI implementation usually loads the elfloader where the kernel would prefer to also be loaded. Allow the elfloader to move itself to a higher address before loading the kernel and user images. This requires ensuring that the stacks are not overwritten during the merge, and so must be placed before the _archive_cpio section in the linker script. Signed-off-by: Kent McLeod <[email protected]>

kent-mcleod · 2022-02-22T06:03:36Z

This relocation needs to consider the DTB passed through from opensbi when doing relocations otherwise it could get overwritten. This is an outstanding issue on Arm (https://sel4.atlassian.net/browse/SELFOUR-2138)

This is now handled by the most recent commit.

If a DTB has been passed in from an earlier stage boot loader during an elfloader image relocation, make sure that it doesn't get overwritten. On Arm this isn't relevant as the protocol when relocation happens doesn't also pass in a DTB. Signed-off-by: Kent McLeod <[email protected]>

axel-h · 2022-02-22T13:21:19Z

elfloader-tool/CMakeLists.txt

@@ -344,6 +344,8 @@ if(DEFINED KernelDTBPath)
    set_property(SOURCE src/drivers/driver.c PROPERTY OBJECT_DEPENDS ${DEVICES_GEN_H})
 endif()

+set_property(SOURCE src/common.c PROPERTY OBJECT_DEPENDS ${IMAGE_START_ADDR_H})


Is this really necessary? With this PR #include <image_start_addr.h> is added to common.c, so it should be part of the normal dependency generation.

axel-h · 2022-02-22T14:34:35Z

elfloader-tool/src/common.c

+
+
+    /* Perform the move and clean/invalidate caches if necessary */
+    char *ret = memmove(target_base, load_base, image_size);


Drop ret and use target_base instead, so it's easier to understand what this does. We could assert ro abort if memmove() does not return target_base, because that indicated something is terribly broken.

axel-h · 2022-02-22T14:42:12Z

elfloader-tool/src/arch-arm/64/crt0.S

@@ -26,12 +23,13 @@ BEGIN_FUNC(_start)
     * Binary images may not be loaded in the correct location.
     * Try and move ourselves so we're in the right place.
     */
+    mov     x0, #0 /* Arg 0 of fixup_image_base is NULL */


Isn't there a possibility on ARM also that we get passed a DTB here, and thus we have to check that it's not overwritten?

axel-h · 2022-02-22T14:49:24Z

elfloader-tool/src/common.c

+char *fixup_image_base(char const *fdt)
+{
+    char *load_base = _start;
+    char *target_base = (char *) IMAGE_START_ADDR;


Suggested change

char *target_base = (char *) IMAGE_START_ADDR;

char *target_base = (char *)IMAGE_START_ADDR;

axel-h · 2022-02-22T14:56:57Z

elfloader-tool/src/arch-riscv/boot.c

@@ -57,7 +59,10 @@ unsigned long l2pt[PTES_PER_PT] __attribute__((aligned(4096)));
 unsigned long l2pt_elf[PTES_PER_PT] __attribute__((aligned(4096)));
 #endif

-char elfloader_stack_alloc[BIT(CONFIG_KERNEL_STACK_BITS)];
+/* This stack cannot go in the .bss section because it's already in use when the
+ * .bss section is zeroed.


I wonder, don't we have the same problem on ARM also then? Seem implementing clear_bss in assembly, so it does not use a stack is the cleanest way to avoid running into issues here.

kent-mcleod mentioned this pull request Feb 1, 2022

RISC-V: reserve memory for SBI in device tree seL4/seL4#759

Draft

yyshen reviewed Feb 1, 2022

View reviewed changes

yyshen approved these changes Feb 2, 2022

View reviewed changes

yyshen reviewed Feb 2, 2022

View reviewed changes

kent-mcleod force-pushed the kent/patch33 branch from 52f3510 to 9381ff3 Compare February 2, 2022 04:43

kent-mcleod mentioned this pull request Feb 2, 2022

hifive on MSC+SMP too often fails IPC0001 seL4/sel4test#64

Closed

axel-h reviewed Feb 2, 2022

View reviewed changes

elfloader-tool/src/arch-riscv/crt0.S Show resolved Hide resolved

axel-h mentioned this pull request Feb 2, 2022

asm code calls clear_bss without a valid stack pointer #84

Closed

axel-h reviewed Feb 2, 2022

View reviewed changes

elfloader-tool/src/arch-arm/64/crt0.S Show resolved Hide resolved

axel-h reviewed Feb 2, 2022

View reviewed changes

elfloader-tool/src/arch-riscv/boot.c Show resolved Hide resolved

kent-mcleod force-pushed the kent/patch33 branch 3 times, most recently from fedf5ca to c422134 Compare February 4, 2022 11:19

kent-mcleod force-pushed the kent/patch33 branch from c422134 to 6bb649a Compare February 4, 2022 11:22

axel-h reviewed Feb 4, 2022

View reviewed changes

elfloader-tool/src/arch-riscv/boot.c Show resolved Hide resolved

kent-mcleod force-pushed the kent/patch33 branch from 6bb649a to 8ed4a93 Compare February 22, 2022 05:21

kent-mcleod added 5 commits February 22, 2022 17:02

elfloader: port fixup_image_base to C

4afb9e5

Porting from assembly to a higher level language makes this function more portable. Signed-off-by: Kent McLeod <[email protected]>

elfloader,RISC-V: Relax kernel alignment checks

a265630

It's not necessary to require the kernel and ELFloader images are loaded at the start of page boundaries. Signed-off-by: Kent McLeod <[email protected]>

elfloader,RISC-V: clear_bss unconditionally

6f20da5

When booting via opensbi, the image format is a binary image and so if the .bss section is at the end it must be initialized unconditionally. Signed-off-by: Kent McLeod <[email protected]>

Revert "riscv: Reposition .rodata to after .bss"

83064a6

This reverts commit 8de8c9f. Signed-off-by: Kent McLeod <[email protected]>

kent-mcleod force-pushed the kent/patch33 branch from 8ed4a93 to 0d14941 Compare February 22, 2022 06:02

kent-mcleod force-pushed the kent/patch33 branch from 0d14941 to 0675897 Compare February 22, 2022 06:06

axel-h reviewed Feb 22, 2022

View reviewed changes

axel-h mentioned this pull request Mar 16, 2022

[patch-axel-18] testing github CI axel-h/seL4#65

Closed

This was referenced Mar 29, 2022

[patch-axel-87] support qemu-riscv-virt platform Hensoldt-Cyber/seL4#11

Closed

[patch-axel-87a] support qemu-riscv-virt platform test Hensoldt-Cyber/seL4#12

Closed

axel-h mentioned this pull request Jul 1, 2022

IMAGE_START_ADDR vs actual start address on RISC-V #148

Open

kent-mcleod marked this pull request as draft April 23, 2023 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow elfloader to put kernel images directly after firmware on RISCV #135

Allow elfloader to put kernel images directly after firmware on RISCV #135

kent-mcleod commented Feb 1, 2022 •

edited by axel-h

Loading

lsf37 commented Feb 1, 2022

yyshen Feb 1, 2022

kent-mcleod Feb 1, 2022

axel-h Feb 2, 2022

kent-mcleod Feb 4, 2022

yyshen Feb 1, 2022

kent-mcleod Feb 1, 2022

yyshen Feb 2, 2022

kent-mcleod Feb 22, 2022

kent-mcleod commented Feb 4, 2022

kent-mcleod commented Feb 9, 2022

kent-mcleod commented Feb 22, 2022

axel-h Feb 22, 2022

axel-h Feb 22, 2022

axel-h Feb 22, 2022

axel-h Feb 22, 2022

axel-h Feb 22, 2022

		@@ -89,11 +91,6 @@ static int map_kernel_window(struct image_info *kernel_info)

		/* Map the elfloader into the new address space */

		if (!IS_ALIGNED((uintptr_t)_text, PT_LEVEL_2_BITS)) {



		/* Perform the move and clean/invalidate caches if necessary */
		char *ret = memmove(target_base, load_base, image_size);

	char target_base = (char ) IMAGE_START_ADDR;
	char target_base = (char )IMAGE_START_ADDR;

Allow elfloader to put kernel images directly after firmware on RISCV #135

Are you sure you want to change the base?

Allow elfloader to put kernel images directly after firmware on RISCV #135

Conversation

kent-mcleod commented Feb 1, 2022 • edited by axel-h Loading

lsf37 commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kent-mcleod commented Feb 4, 2022

kent-mcleod commented Feb 9, 2022

kent-mcleod commented Feb 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kent-mcleod commented Feb 1, 2022 •

edited by axel-h

Loading