Skip to content
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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
65 changes: 65 additions & 0 deletions elfloader-tool/include/arch-arm/64/mode/aarch64.h
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))
58 changes: 1 addition & 57 deletions elfloader-tool/include/arch-arm/64/mode/assembler.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,63 +9,7 @@
/* This file contains useful macros for assembly code. */

#ifdef __ASSEMBLER__

#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))
#include <mode/aarch64.h>

.macro enable_mmu sctlr tmp
mrs \tmp, \sctlr
Expand Down
6 changes: 3 additions & 3 deletions elfloader-tool/include/arch-arm/64/mode/structures.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
#define PMD_BITS 9
#define PMD_SIZE_BITS (PMD_BITS + PMDE_SIZE_BITS)

#define GET_PGD_INDEX(x) (((x) >> (ARM_2MB_BLOCK_BITS + PMD_BITS + PUD_BITS)) & MASK(PGD_BITS))
#define GET_PUD_INDEX(x) (((x) >> (ARM_2MB_BLOCK_BITS + PMD_BITS)) & MASK(PUD_BITS))
#define GET_PMD_INDEX(x) (((x) >> (ARM_2MB_BLOCK_BITS)) & MASK(PMD_BITS))
#define GET_PGD_INDEX(x) (((word_t)(x) >> (ARM_2MB_BLOCK_BITS + PMD_BITS + PUD_BITS)) & MASK(PGD_BITS))
Copy link
Member

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?

Copy link
Author

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 change word_t to uintptr_t here?

Copy link
Member

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.

Copy link
Author

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:

The following type designates an unsigned integer type with the property that any valid
pointer to void can be converted to this type, then converted back to pointer to void,
and the result will compare equal to the original pointer: uintptr_t

So the strictest form would be a cast to void * first before uintptr_t. However, I think all the reasonable implementations have sensible ways for any pointer casted to uintptr_t to behave such that we can skip the void * cast.

#define GET_PUD_INDEX(x) (((word_t)(x) >> (ARM_2MB_BLOCK_BITS + PMD_BITS)) & MASK(PUD_BITS))
#define GET_PMD_INDEX(x) (((word_t)(x) >> (ARM_2MB_BLOCK_BITS)) & MASK(PMD_BITS))

extern uint64_t _boot_pgd_up[BIT(PGD_BITS)];
extern uint64_t _boot_pud_up[BIT(PUD_BITS)];
Expand Down
2 changes: 1 addition & 1 deletion elfloader-tool/include/arch-arm/cpuid.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
uint32_t read_cpuid_id(void);

/* read MP ID register from CPUID */
uint32_t read_cpuid_mpidr(void);
andybui01 marked this conversation as resolved.
Show resolved Hide resolved
word_t read_cpuid_mpidr(void);

/* check if CPU is in HYP/EL2 mode */
word_t is_hyp_mode(void);
Expand Down
1 change: 1 addition & 0 deletions elfloader-tool/src/arch-arm/64/cpuid.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <armv/machine.h>
#include <types.h>
#include <cpuid.h>

/* we only care about the affinity bits */
#define MPIDR_MASK (0xff00ffffff)
Expand Down
2 changes: 1 addition & 1 deletion elfloader-tool/src/arch-arm/64/crt0.S
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ BEGIN_FUNC(_start)
bl fixup_image_base
mov x2, x0
/* restore original arguments for next step */
ldp x0, x1, [sp, #-16]!
ldp x0, x1, [sp], #16
/* fixup_image_base returns 1 if no need to move */
cmp x2, #1
beq 1f
Expand Down
10 changes: 6 additions & 4 deletions elfloader-tool/src/arch-arm/64/mmu.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include <mode/structures.h>
#include <printf.h>
#include <abort.h>
#include <mode/aarch64.h>
#include <armv/machine.h>
andybui01 marked this conversation as resolved.
Show resolved Hide resolved

/*
* Create a "boot" page table, which contains a 1:1 mapping below
Expand All @@ -30,7 +32,7 @@ void init_boot_vspace(struct image_info *kernel_info)
for (i = 0; i < BIT(PUD_BITS); i++) {
_boot_pud_down[i] = (i << ARM_1GB_BLOCK_BITS)
| BIT(10) /* access flag */
| (0 << 2) /* strongly ordered memory */
| (MT_DEVICE_nGnRnE << 2) /* strongly ordered memory */
| BIT(0); /* 1G block */
}

Expand All @@ -51,7 +53,7 @@ void init_boot_vspace(struct image_info *kernel_info)
#if CONFIG_MAX_NUM_NODES > 1
| (3 << 8) /* make sure the shareability is the same as the kernel's */
#endif
| (4 << 2) /* MT_NORMAL memory */
| (MT_NORMAL << 2) /* MT_NORMAL memory */
| BIT(0); /* 2M block */
first_paddr += BIT(ARM_2MB_BLOCK_BITS);
}
Expand All @@ -68,7 +70,7 @@ void init_hyp_boot_vspace(struct image_info *kernel_info)
for (i = 0; i < BIT(PUD_BITS); i++) {
_boot_pud_down[i] = (i << ARM_1GB_BLOCK_BITS)
| BIT(10) /* access flag */
| (0 << 2) /* strongly ordered memory */
| (MT_DEVICE_nGnRnE << 2) /* strongly ordered memory */
| BIT(0); /* 1G block */
}

Expand All @@ -85,7 +87,7 @@ void init_hyp_boot_vspace(struct image_info *kernel_info)
#if CONFIG_MAX_NUM_NODES > 1
| (3 << 8)
#endif
| (4 << 2) /* MT_NORMAL memory */
| (MT_NORMAL << 2) /* MT_NORMAL memory */
| BIT(0); /* 2M block */
}
}
6 changes: 6 additions & 0 deletions elfloader-tool/src/arch-arm/armv/armv8-a/64/smp.c
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ void core_entry(uint64_t sp)

int is_core_up(int i)
{
/* Secondary core may be booted with caches disabled,
* this value might be written in memory, invalidate our
* copy and get a new one. */
asm volatile("dc ivac, %0\n\t"
"dmb nsh\n\t"
:: "r"(&core_up[i]));
return core_up[i] == i;
}

Expand Down
9 changes: 8 additions & 1 deletion elfloader-tool/src/arch-arm/drivers/smp-psci.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*
* SPDX-License-Identifier: GPL-2.0-only
*/
#include <autoconf.h>
andybui01 marked this conversation as resolved.
Show resolved Hide resolved
#include <elfloader_common.h>
#include <devices_gen.h>
#include <drivers/common.h>
Expand All @@ -24,7 +25,13 @@ static int smp_psci_cpu_on(UNUSED struct elfloader_device *dev,
}
secondary_data.entry = entry;
secondary_data.stack = stack;
dmb();
#if defined(CONFIG_ARCH_AARCH64)
/* If the secondary core caches are off, need to make sure that the info
* is clean to the physical memory so that the sedcondary cores can read it.
*/
asm volatile("dc cvac, %0" :: "r"(&secondary_data));
dsb();
#endif
int ret = psci_cpu_on(cpu->cpu_id, (unsigned long)&secondary_startup, 0);
if (ret != PSCI_SUCCESS) {
printf("Failed to bring up core 0x%x with error %d\n", cpu->cpu_id, ret);
Expand Down
31 changes: 26 additions & 5 deletions elfloader-tool/src/arch-arm/smp_boot.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment seems incorrect, the variable is declared volatile, so the compiler is not allowed to optimize this out. However, the barrier might sill be needed to force a OO-pipeline not to try smart things. Instead of the barrier, we might also consider using https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html to let the compiler pick the appropriate meachnism.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're correct it's declared volatile so this is not needed. I can add a asm volatile("": : :"memory") to the loop body to let the compiler handle this?

Copy link
Member

Choose a reason for hiding this comment

The 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 #ifdef CONFIG_ARCH_AARCH64 here, as the same could also happen on ARMv7, so we dsb could be helpful here also. It does not harm at at least. Also, this code is not time critical, it's the general core synchronization that happen on boot. So we can be extra cautions here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cpu_idle() is just dsb() and wfi() paired, so we can make this common.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. cpu_idle() is doing something completely different, so this should not be used here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code as is

    while (!non_boot_lock) {
#ifdef CONFIG_ARCH_AARCH64
        dsb();
#else
        cpu_idle();
#endif
    }

And since cpu_idle() is itself: dsb() + wfi(), the dsb() would already be common.

Copy link
Member

@axel-h axel-h Apr 11, 2024

Choose a reason for hiding this comment

The 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 wfi() here also - I can't see who triggers the event on the other core that will bring us out of the wfi then. Did this ever work or was this stuck forever then - or did something else trigger an event by chance? I can't find anything in the commit history that explains this. Except that commit #4879dfe9 disbled this for CONFIG_ARCH_AARCH64 anyway as it might not have worked?

Copy link
Author

Choose a reason for hiding this comment

The 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 non_boot_lock when the boot CPU finishes initializing?

Copy link
Member

Choose a reason for hiding this comment

The 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 dsb() should be sufficient here. And it seems this worked well for your colleagues when the made the code change.
Since I'm not really sure how this is supposed to work on ARMv7/AARCH32 and we can't test this now either, it might be best to just leave it and add a comment that calling cpu_idle() this looks like a hack that should either be explained or reworked.

* to force a reload. */
dsb();
#else
cpu_idle();
#endif
}
Expand All @@ -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();
Expand Down Expand Up @@ -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++;
}
Expand All @@ -134,6 +144,17 @@ void smp_boot(void)
arm_disable_dcaches();
#endif
init_cpus();

#if defined(CONFIG_ARCH_AARCH64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could avoid the code duplication:

#if defined(CONFIG_ARCH_AARCH64)
    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. */
    asm volatile("dc civac, %0\n\t" :: "r"(&non_boot_lock) : "memory");;
#endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why you do the dsb() before updating non_boot_lock actually. This should get a comment that explains this. Seems we want to ensure all changes done far get visible, before we release the other cores. We should do the dsb() unconditionally then, not just on aarch64.

    /* ensure all operation complete before we release the other cores */
    dsb(); 
    /* release secondary cores */
    non_boot_lock = 1;
    /* Secondary cores may still run with MMU & caches off. Force the update to be visible. */ 
#if defined(CONFIG_ARCH_AARCH64) 
    asm volatile("dc civac, %0\n\t" :: "r"(&non_boot_lock) : "memory"); 
#else
    // ToDo: add flush operation here to be safe.
#endif

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 */
19 changes: 15 additions & 4 deletions elfloader-tool/src/arch-arm/sys_boot.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,13 @@
/* 0xd00dfeed in big endian */
#define DTB_MAGIC (0xedfe0dd0)

/* Maximum alignment we need to preserve when relocating (64K) */
#define MAX_ALIGN_BITS (14)
/* Maximum alignment we need to preserve when relocating (64K)
*
* The 64 kiB alignment is a maximum requirement for a stage2
* concatenated pagetable. See Table G5-4 in ARM DDI 0487I.a, page
* G5-9186.
*/
#define MAX_ALIGN_BITS (16)
andybui01 marked this conversation as resolved.
Show resolved Hide resolved

#ifdef CONFIG_IMAGE_EFI
ALIGN(BIT(PAGE_BITS)) VISIBLE
Expand Down Expand Up @@ -216,17 +221,23 @@ void continue_boot(int was_relocated)
arm_enable_mmu();
}

/* Enter kernel. The UART may no longer be accessible here. */
/* The UART may no longer be accessible here. */
if ((uintptr_t)uart_get_mmio() < kernel_info.virt_region_start) {
printf("Jumping to kernel-image entry point...\n\n");
}

#if defined(CONFIG_ARCH_AARCH64)
/* Clear D&A in DAIF */
asm volatile("msr daifclr, #0xC\n\t");
andybui01 marked this conversation as resolved.
Show resolved Hide resolved
#endif

/* 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,
(word_t)dtb,
dtb_size);
(uint32_t)dtb_size);

/* We should never get here. */
printf("ERROR: Kernel returned back to the ELF Loader\n");
Expand Down
Loading