-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
elfloader: improve stability #191
base: master
Are you sure you want to change the base?
Changes from all commits
7e410f0
78644d1
864c7c7
7417e3c
1b4cddf
326dc87
6e44ac6
33a9819
f317679
970da42
657d6ca
c555152
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/* | ||
* Copyright 2023, NIO GmbH | ||
* | ||
* SPDX-License-Identifier: GPL-2.0-only | ||
*/ | ||
#pragma once | ||
|
||
/* This file contains useful defines for assembly and C code. */ | ||
|
||
#define PSR_F_BIT 0x00000040 | ||
#define PSR_I_BIT 0x00000080 | ||
#define PSR_A_BIT 0x00000100 | ||
#define PSR_D_BIT 0x00000200 | ||
|
||
#define PSR_MODE_EL0t 0x00000000 | ||
#define PSR_MODE_EL1t 0x00000004 | ||
#define PSR_MODE_EL1h 0x00000005 | ||
#define PSR_MODE_EL2t 0x00000008 | ||
#define PSR_MODE_EL2h 0x00000009 | ||
#define PSR_MODE_SVC_32 0x00000013 | ||
|
||
#define TCR_T0SZ(x) ((64 - (x))) | ||
#define TCR_T1SZ(x) ((64 - (x)) << 16) | ||
#define TCR_TxSZ(x) (TCR_T0SZ(x) | TCR_T1SZ(x)) | ||
|
||
#define TCR_IRGN0_WBWC (1 << 8) | ||
#define TCR_IRGN_NC ((0 << 8) | (0 << 24)) | ||
#define TCR_IRGN_WBWA ((1 << 8) | (1 << 24)) | ||
#define TCR_IRGN_WT ((2 << 8) | (2 << 24)) | ||
#define TCR_IRGN_WBnWA ((3 << 8) | (3 << 24)) | ||
#define TCR_IRGN_MASK ((3 << 8) | (3 << 24)) | ||
|
||
#define TCR_ORGN0_WBWC (1 << 10) | ||
#define TCR_ORGN_NC ((0 << 10) | (0 << 26)) | ||
#define TCR_ORGN_WBWA ((1 << 10) | (1 << 26)) | ||
#define TCR_ORGN_WT ((2 << 10) | (2 << 26)) | ||
#define TCR_ORGN_WBnWA ((3 << 10) | (3 << 26)) | ||
#define TCR_ORGN_MASK ((3 << 10) | (3 << 26)) | ||
|
||
#define TCR_SH0_ISH (3 << 12) | ||
#define TCR_SHARED ((3 << 12) | (3 << 28)) | ||
|
||
#define TCR_TG0_4K (0 << 14) | ||
#define TCR_TG0_64K (1 << 14) | ||
#define TCR_TG1_4K (2 << 30) | ||
#define TCR_TG1_64K (3 << 30) | ||
|
||
#define TCR_PS_4G (0 << 16) | ||
#define TCR_PS_64G (1 << 16) | ||
#define TCR_PS_1T (2 << 16) | ||
#define TCR_PS_4T (3 << 16) | ||
#define TCR_PS_16T (4 << 16) | ||
#define TCR_PS_256T (5 << 16) | ||
|
||
/* bits are reserved as 1 */ | ||
#define TCR_EL2_RES1 ((1 << 23) | (1 << 31)) | ||
#define TCR_ASID16 (1 << 36) | ||
|
||
#define MT_DEVICE_nGnRnE 0 | ||
#define MT_DEVICE_nGnRE 1 | ||
#define MT_DEVICE_GRE 2 | ||
#define MT_NORMAL_NC 3 | ||
#define MT_NORMAL 4 | ||
#define MT_NORMAL_WT 5 | ||
#define MAIR(_attr, _mt) ((_attr) << ((_mt) * 8)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,7 @@ static volatile int non_boot_lock = 0; | |
void arm_disable_dcaches(void); | ||
|
||
extern void const *dtb; | ||
extern uint32_t dtb_size; | ||
extern size_t dtb_size; | ||
|
||
/* Entry point for all CPUs other than the initial. */ | ||
void non_boot_main(void) | ||
|
@@ -34,7 +34,11 @@ void non_boot_main(void) | |
#endif | ||
/* Spin until the first CPU has finished initialisation. */ | ||
while (!non_boot_lock) { | ||
#ifndef CONFIG_ARCH_AARCH64 | ||
#ifdef CONFIG_ARCH_AARCH64 | ||
/* The compiler may optimize this loop away, add a dsb() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment seems incorrect, the variable is declared There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes you're correct it's declared There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please fix the comment. The compiler can't optimize the loop away, because the variable is volatile. However, we may need the explicit CPU barrier instruction between the read instruction stream created by the loop to ensure the CPU does not try to optimize out multiple reads to the same location. I wonder, have you seen that without the barrier things really fail or just take much longer until some synchronization kick in, as the change is not visible immediately? It might be worth adding the specific details in the commit comment then. Especially, if the cache setting between reader and writer could still be different here, then we may need explicit cache flush/invalidation here also to guarantee visibility. I wonder, is we should even remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cpu_idle() is just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current code as is
And since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, I see. But It's a bit odd that ARMv7 is doing a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this can be simplified to just a barrier and we can find a way to do a cache invalidation (not just aarch64) on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since on aarch64 the D-Cache is disabled before the loop anyway, just a |
||
* to force a reload. */ | ||
dsb(); | ||
#else | ||
cpu_idle(); | ||
#endif | ||
} | ||
|
@@ -58,10 +62,10 @@ void non_boot_main(void) | |
arm_enable_mmu(); | ||
} | ||
|
||
/* Jump to the kernel. */ | ||
/* Jump to the kernel. Note: Our DTB is smaller than 4 GiB. */ | ||
((init_arm_kernel_t)kernel_info.virt_entry)(user_info.phys_region_start, | ||
user_info.phys_region_end, user_info.phys_virt_offset, | ||
user_info.virt_entry, (paddr_t)dtb, dtb_size); | ||
user_info.virt_entry, (paddr_t)dtb, (uint32_t)dtb_size); | ||
|
||
printf("AP Kernel returned back to the elf-loader.\n"); | ||
abort(); | ||
|
@@ -117,7 +121,13 @@ WEAK void init_cpus(void) | |
abort(); | ||
} | ||
|
||
while (!is_core_up(num_cpus)); | ||
while (!is_core_up(num_cpus)) { | ||
#if defined(CONFIG_ARCH_AARCH64) | ||
/* The compiler may optimize this loop away, add a dsb() | ||
andybui01 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* to force a reload. */ | ||
dsb(); | ||
#endif | ||
} | ||
printf("Core %d is up with logic id %d\n", elfloader_cpus[i].cpu_id, num_cpus); | ||
num_cpus++; | ||
} | ||
|
@@ -134,6 +144,17 @@ void smp_boot(void) | |
arm_disable_dcaches(); | ||
#endif | ||
init_cpus(); | ||
|
||
#if defined(CONFIG_ARCH_AARCH64) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could avoid the code duplication:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why you do the
|
||
dsb(); | ||
#endif | ||
|
||
non_boot_lock = 1; | ||
|
||
#if defined(CONFIG_ARCH_AARCH64) | ||
/* Secondary CPUs may still run with MMU & caches off. Force the update to be visible. */ | ||
andybui01 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
asm volatile("dc civac, %0\n\t" :: "r"(&non_boot_lock) : "memory");; | ||
#endif | ||
|
||
} | ||
#endif /* CONFIG_MAX_NUM_NODES */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the commit comment it says that this works for pointer now also, but pointer must be cast to
uintptr_t
first to make then an integer. Maybe add this here also then?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the reason behind the comment is that in
tools/seL4/elfloader-tool/include/types.h
"word_t is practically an alias for size_t/uintptr_t on the platforms we support so far". Do you mean to just changeword_t
touintptr_t
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think strict C rules say, that pointers must be cast to
uintptr_t
to make them an integer, from there you can cast the integer to any other integer type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is reasonable, I did find this excerpt in the C99 standard though, and it's the only part I could find on
uintptr_t
:So the strictest form would be a cast to
void *
first beforeuintptr_t
. However, I think all the reasonable implementations have sensible ways for any pointer casted touintptr_t
to behave such that we can skip thevoid *
cast.