head -r344018 powerpc64 variant on Powermac G5 (2 sockets, 2 cores each): [*buffer arena] shows up more . . .? [mpc85xx_smp_timebase_sync problem too]
Justin Hibbits
chmeeedalf at gmail.com
Tue Apr 16 21:42:16 UTC 2019
On Tue, 16 Apr 2019 14:26:39 -0700
Mark Millard <marklmi at yahoo.com> wrote:
> [Looks to me like the use and content of mpc85xx_smp_timebase_sync
> have the same type of problems I noted for the proposed powermac
> patch.]
>
...
> >
> > As mentioned, I had only compiled it. Your examination of the code
> > path demonstrates that the patch is insufficient, and would hang at
> > unleash anyway. The sleep/wake logic probably needs to be updated
> > anyway. It was written for a G4 powerbook primarily for the
> > PMU-based cpufreq driver, so some bits might need to be moved
> > around. Orthogonal to this issue, though.
>
> It appears to me that the overall sequence:
>
> platform_smp_timebase_sync(0, 1); // called from
> cpudep_ap_setup . . .
> // The following are from in machdep_ap_bootstrap . . .
> while (ap_letgo == 0)
> __asm __volatile("or 31,31,31");
> __asm __volatile("or 6,6,6");
> . . .
> platform_smp_timebase_sync(ap_timebase, 1);
>
> calls mpc85xx_smp_timebase_sync twice per ap and that the
> 2nd call has tb_ready && mp_ncpus<=cpu_done for each such
> ap, leading to a lack of coordination of the activity that
> 2nd time for each ap:
>
> static void
> mpc85xx_smp_timebase_sync(platform_t plat, u_long tb, int ap)
> {
> static volatile bool tb_ready;
> static volatile int cpu_done;
>
> if (ap) {
> /* APs. Hold off until we get a stable timebase. */
> while (!tb_ready)
> atomic_thread_fence_seq_cst();
> mttb(tb);
> atomic_add_int(&cpu_done, 1);
> while (cpu_done < mp_ncpus)
> atomic_thread_fence_seq_cst();
> } else {
> /* BSP */
> freeze_timebase(rcpm_dev, true);
> tb_ready = true;
> mttb(tb);
> atomic_add_int(&cpu_done, 1);
> while (cpu_done < mp_ncpus)
> atomic_thread_fence_seq_cst();
> freeze_timebase(rcpm_dev, false);
> }
> }
>
Not for mpc85xx. This is call is only in the AIM cpudep_ap_setup, and
really shouldn't be there anyway. The original code to just set the
timebase was there to set it to 0 just as a semi-sane value until the
core got to a stable state. The original code was literally "mttb(0)"
for G4, and a check for hypervisor with mttb(0). Had that been
sufficient, the platform_smp_timebase_sync() would not be a problem to
do the real sync as I had mentioned in my patch before.
The powermac patch that I had provided was derived from this
well-working mpc85xx patch. However, I had neglected to check that the
sync wasn't used elsewhere as well. If the cpudep_ap_setup() use is
removed, then this should work fine.
- Justin
More information about the freebsd-ppc
mailing list