elfloader: improve stability

[andybui01]

As mentioned in #190, this is the series of commits that do general touch-ups in the elfloader.

This should reduce the Jetson Orin PR to 1 commit, and the aarch64 booting sequence PR (these commits rely on each other) to 6 commits, which should be more manageable, although I'll prepare those PRs are this passes (hopefully).

[axel-h]

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

[axel-h]

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

[axel-h]

Looking at the commit comment, isn't the driver list even "ro-data" actually and should be put there?

The commit comment also contains the question "The driver list entry is present in the aarch32 linker script for
EFI, why was it missing for 64 bit?" It should be removed provide the proper answer instead, that this was missing and is added now.

[andybui01]

This is within the riscv linker script:

    . = ALIGN(16);
    .rodata :
    {
        *(.srodata*)
        . = ALIGN(16);
        *(.rodata)
        *(.rodata.*)
        /*
         * ld crashes when we add this here: *(_driver_list)
         */
        . = ALIGN(16);
        _archive_start = .;
        *(._archive_cpio)
        _archive_end = .;
    }

Moving the _driver_list into rodata may be breaking.

[axel-h]

Could the commit history give something why 'ld' is crashing here? Maybe this is fixed by now?

[andybui01]

Commit history didn't give any hints unfortunately. I don't have the risc-v toolchain currently so I'll try and build when I get the chance.

[axel-h]

I will also have a look on RISC-V.

[axel-h]

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?

[andybui01]

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?

[axel-h]

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.

[andybui01]

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.

[axel-h]

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.

[andybui01]

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?

[axel-h]

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.

[andybui01]

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

[axel-h]

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

[andybui01]

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.

[axel-h]

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?

[andybui01]

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?

[axel-h]

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.

[axel-h]

The simulation on ARMv8 fails now, could you have a look at this? Maybe you should split this PR even further up, so we can merge the trivial things faster.

[andybui01]

The simulation on ARMv8 fails now, could you have a look at this?

The failure is due to the following reason: as it stands, the elfloader creates the boot page table by mapping 2 regions:

1. The kernel region (kernel_info->virt_region_start) as MT_NORMAL memory.
2. A bulk 512 GiB 1:1 mapping (used by the elfloader) that is strongly-ordered.

The final commit (3166422) erroneously assumes that this is region is for device memory, that is not correct. Instead, this commit will be part of the page table PR to follow, which splits the boot page table into distinct regions (elfloader - normal, kernel - normal, uart mmio - strongly ordered).

[kent-mcleod]

As mentioned in #190, this is the series of commits that do general touch-ups in the elfloader.

This PR seems to not contain one of the touch up commits: 0108b77

And contains an additional commit that is not a touch up commit and changes behavior: 7e410f0

Is this intentional?

[andybui01]

As mentioned in #190, this is the series of commits that do general touch-ups in the elfloader.

This PR seems to not contain one of the touch up commits: 0108b77

And contains an additional commit that is not a touch up commit and changes behavior: 7e410f0

Is this intentional?

Yeah, the first one makes less sense to me without the other commits that all attempt to solve a problem (stabilize aarch64 booting for the Orin).

As for the second, I think its impact is low enough to be included here? It doesn't seem to be that big of a behavioral change.

[andybui01]

@axel-h A re-review would be appreciated if you can spare some time. Also the failing HW tests are passing when I check the log, as they reach the end but the message "All is well in the universe" becomes truncated.