SOLVED (patch): FYI: Pine64+ 2GB (so A64) booting and non-debug vs. debug kernel: "APs not started" for failure cases only: solved with patch
Mark Millard
markmi at dsl-only.net
Sun Sep 17 02:48:06 UTC 2017
On 2017-Sep-16, at 7:21 PM, Mark Millard <markmi at dsl-only.net> wrote:
> [As the following does not flow well from the previous
> message and stands somewhat on its own in some respects:
> I top post this.]
>
> ffff0000005fb14c <release_aps+0xb4> adrp x8, ffff000000aaa000 <tmpbuffer+0x8a0>
> ffff0000005fb150 <release_aps+0xb8> add x8, x8, #0x778
> ffff0000005fb154 <release_aps+0xbc> adrp x0, ffff0000006c9000 <digits+0x1e74e>
> ffff0000005fb158 <release_aps+0xc0> add x0, x0, #0xb8a
> ffff0000005fb15c <release_aps+0xc4> stlr w20, [x8]
> ffff0000005fb160 <release_aps+0xc8> sev
>
> I ran into the following mention of SEV and making sure
> it is appropriately delayed:
>
>> The mechanism that signals an event to other PEs is IMPLEMENTATION DEFINED. The PE is not required to guarantee the ordering of this event with respect to the completion of memory accesses by instructions before the SEV instruction. Therefore, ARM recommends that software includes a DSB instruction before any SEV instruction.
>>
>> The SEVL instruction appears to execute in program order relative to any subsequent WFE instruction executed on the same PE, without the need for any explicit insertion of barrier instructions.
The following patch has allowed me to boot
the pine64+ 2GB with even a non-debug kernel.
# svnlite diff /usr/src/sys/arm64/arm64/mp_machdep.c
Index: /usr/src/sys/arm64/arm64/mp_machdep.c
===================================================================
--- /usr/src/sys/arm64/arm64/mp_machdep.c (revision 323246)
+++ /usr/src/sys/arm64/arm64/mp_machdep.c (working copy)
@@ -236,7 +236,9 @@
atomic_store_rel_int(&aps_ready, 1);
/* Wake up the other CPUs */
- __asm __volatile("sev");
+ __asm __volatile(
+ "dsb ish \n"
+ "sev \n");
printf("Release APs\n");
I will add this patch to bugzilla 222234.
===
Mark Millard
markmi at dsl-only.net
On 2017-Sep-16, at 4:08 PM, Mark Millard <markmi at dsl-only.net> wrote:
> [Adding a couple of numbers that help
> interpret what I found as far as what
> specifically did not work as expected.]
>
> On 2017-Sep-16, at 3:17 PM, Mark Millard <markmi at dsl-only.net> wrote:
>
>> A new finding:
>>
>> When verbose boot messages are enabled
>> there is an earlier contrast between when
>> booting works overall vs. when it later
>> fails:
>>
>> When it works:
>>
>> subsystem f000000
>> release_aps(0)... Release APs
>> done.
>>
>> When it fails:
>>
>> subsystem f000000
>> release_aps(0)... Release APs
>> APs not started
>> done.
>>
>> And it well explains why ->pc_curthread
>> ends up NULL for secondaries (in particular
>> cpu == 1), init_secondary had never executed
>> the assignments show below:
>>
>> while (!aps_ready)
>> __asm __volatile("wfe");
>>
>> /* Initialize curthread */
>> KASSERT(PCPU_GET(idlethread) != NULL, ("no idle thread"));
>> pcpup->pc_curthread = pcpup->pc_idlethread;
>> pcpup->pc_curpcb = pcpup->pc_idlethread->td_pcb;
>>
>> The subsystem messages are from:
>>
>> static void
>> release_aps(void *dummy __unused)
>> {
>> int i;
>>
>> /* Only release CPUs if they exist */
>> if (mp_ncpus == 1)
>> return;
>>
>> intr_pic_ipi_setup(IPI_AST, "ast", ipi_ast, NULL);
>> intr_pic_ipi_setup(IPI_PREEMPT, "preempt", ipi_preempt, NULL);
>> intr_pic_ipi_setup(IPI_RENDEZVOUS, "rendezvous", ipi_rendezvous, NULL);
>> intr_pic_ipi_setup(IPI_STOP, "stop", ipi_stop, NULL);
>> intr_pic_ipi_setup(IPI_STOP_HARD, "stop hard", ipi_stop, NULL);
>> intr_pic_ipi_setup(IPI_HARDCLOCK, "hardclock", ipi_hardclock, NULL);
>>
>> atomic_store_rel_int(&aps_ready, 1);
>> /* Wake up the other CPUs */
>> __asm __volatile("sev");
>>
>> printf("Release APs\n");
>>
>> for (i = 0; i < 2000; i++) {
>> if (smp_started)
>> return;
>> DELAY(1000);
>> }
>>
>> printf("APs not started\n");
>> }
>> SYSINIT(start_aps, SI_SUB_SMP, SI_ORDER_FIRST, release_aps, NULL);
>>
>>
>> init_secondary has an example or two of not using
>> atomic_load_acq_int when atomic_store_rel_int is in
>> use. One is:
>>
>> while (!aps_ready)
>> __asm __volatile("wfe");
>>
>> /* Initialize curthread */
>> KASSERT(PCPU_GET(idlethread) != NULL, ("no idle thread"));
>> pcpup->pc_curthread = pcpup->pc_idlethread;
>> pcpup->pc_curpcb = pcpup->pc_idlethread->td_pcb;
>>
>> where aps_ready was declared via:
>>
>> /* Set to 1 once we're ready to let the APs out of the pen. */
>> volatile int aps_ready = 0;
>>
>> where release_aps has the use of atomic_store_rel_int:
>>
>> atomic_store_rel_int(&aps_ready, 1);
>> /* Wake up the other CPUs */
>> __asm __volatile("sev");
>>
>> There is also in init_secondary:
>>
>> atomic_add_rel_32(&smp_cpus, 1);
>>
>> if (smp_cpus == mp_ncpus) {
>> /* enable IPI's, tlb shootdown, freezes etc */
>> atomic_store_rel_int(&smp_started, 1);
>> }
>>
>> where smp_cpus is accessed without being explicitly
>> atomic. mp_ncpus seems to have no atomic use at all.
>>
>> Where:
>>
>> /usr/src/sys/sys/smp.h:extern int smp_cpus;
>> /usr/src/sys/kern/subr_smp.c:int smp_cpus = 1; /* how many cpu's running */
>>
>> So no "volatile", unlike the earlier example.
>>
>> /usr/src/sys/kern/kern_umtx.c: if (smp_cpus > 1) {
>> /usr/src/sys/kern/subr_smp.c:SYSCTL_INT(_kern_smp, OID_AUTO, cpus, CTLFLAG_RD|CTLFLAG_CAPRD, &smp_cpus, 0,
>>
>> /usr/src/sys/sys/smp.h:extern int mp_ncpus;
>> /usr/src/sys/kern/subr_smp.c:int mp_ncpus;
>>
>>
>> The smp_started is not explicitly accessed as atomic
>> in release_aps but in init_secondary has its update
>> to 1 via:
>>
>> mtx_lock_spin(&ap_boot_mtx);
>>
>> atomic_add_rel_32(&smp_cpus, 1);
>>
>> if (smp_cpus == mp_ncpus) {
>> /* enable IPI's, tlb shootdown, freezes etc */
>> atomic_store_rel_int(&smp_started, 1);
>> }
>>
>> mtx_unlock_spin(&ap_boot_mtx);
>>
>> where:
>>
>> /usr/src/sys/sys/smp.h:extern volatile int smp_started;
>> /usr/src/sys/kern/subr_smp.c:volatile int smp_started;
>>
>> ("volatile" again for this context.)
>>
>> I'll also note that for the sparc64 architecture
>> there is some code like:
>>
>> if (__predict_false(atomic_load_acq_int(&smp_started) == 0))
>>
>> that is explicitly matched to the atomic_store_rel_int
>> in its mp_machdep.c .
>>
>> I do not have enough background aarch64 knowledge to
>> know if it is provable that atomic_load_acq_int is not
>> needed in some of these cases.
>>
>> But getting "APs not started" at least sometimes
>> suggests an intermittent failure of the code as
>> it is.
>>
>>
>> Another difference is lack of explicit initialization
>> of smp_started but explicit initialization of aps_ready
>> and smp_cpus .
>>
>>
>>
>> I have no clue if the boot sequence is supposed to
>> handle "APs not started" by reverting to not being
>> a symmetric multiprocessing boot or some other
>> specific way instead of trying to avoiding use of
>> what was not initialized by:
>>
>> pcpup->pc_curthread = pcpup->pc_idlethread;
>> pcpup->pc_curpcb = pcpup->pc_idlethread->td_pcb;
>>
>> in init_secondary.
>
>
> Example mp_ncpus and smp_cpus figures from a
> failed Pine64+ 2GB boot:
>
> db> print/x *smp_cpus
> 1
> db> print/x *mp_ncpus
> 138800000004
>
> But that should be a 4 byte width. Showing
> some context for reference:
>
> db> x/bx mp_ncpus-4,4
> rebooting: 0 0 0 0
> db> x/bx mp_ncpus,4
> mp_ncpus: 4 0 0 0
> db> x/bx mp_ncpus+4,4
> scsi_delay: 88 13 0 0
>
> For completeness:
>
> db> x/bx smp_cpus-4,4
> sysctl___kern_smp_disabled+0x5c: 0 0 0 0
> db> x/bx smp_cpus,4
> smp_cpus: 1 0 0 0
> db> x/bx smp_cpus+4,4
> smp_cpus+0x4: 0 0 0 0
>
> So smp_cpus was not incremented in memory. This
> goes along with no occurances of:
>
> pcpup->pc_curthread = pcpup->pc_idlethread;
> pcpup->pc_curpcb = pcpup->pc_idlethread->td_pcb;
>
> updates happening in init_secondary:
>
> /* Spin until the BSP releases the APs */
> while (!aps_ready)
> __asm __volatile("wfe");
>
> /* Initialize curthread */
> KASSERT(PCPU_GET(idlethread) != NULL, ("no idle thread"));
> pcpup->pc_curthread = pcpup->pc_idlethread;
> pcpup->pc_curpcb = pcpup->pc_idlethread->td_pcb;
> . . .
> mtx_lock_spin(&ap_boot_mtx);
>
> atomic_add_rel_32(&smp_cpus, 1);
>
> if (smp_cpus == mp_ncpus) {
> /* enable IPI's, tlb shootdown, freezes etc */
> atomic_store_rel_int(&smp_started, 1);
> }
>
> mtx_unlock_spin(&ap_boot_mtx);
>
> Which seems to imply that the aps_ready
> update:
>
> atomic_store_rel_int(&aps_ready, 1);
> /* Wake up the other CPUs */
> __asm __volatile("sev");
>
> in release_aps was not seen in the:
>
> while (!aps_ready)
> __asm __volatile("wfe");
>
> in init_secondary.
>
> My guess is that "while (!aps_ready)" needs
> to be explicit about its atomic status.
> aps_ready is already volatile but apparently
> that is not enough for this context to be
> reliable.
>
> The other potential needs for explicit atomics
> are for later execution but may be required
> overall as well.
More information about the freebsd-arm
mailing list