Date: Tue, 16 Apr 2019 16:42:10 -0500 From: Justin Hibbits <chmeeedalf@gmail.com> To: Mark Millard <marklmi@yahoo.com> Cc: freebsd-ppc <freebsd-ppc@freebsd.org> Subject: Re: head -r344018 powerpc64 variant on Powermac G5 (2 sockets, 2 cores each): [*buffer arena] shows up more . . .? [mpc85xx_smp_timebase_sync problem too] Message-ID: <20190416164210.26827a6d@titan.knownspace> In-Reply-To: <984188AE-93A9-4321-99EC-522C2E0CE9A9@yahoo.com> References: <D9B56EE2-35C7-44A2-9229-D9E4AECAD3E1@yahoo.com> <20190306151914.44ea831c@titan.knownspace> <8668AAF7-9E6A-4278-9D1B-2ECDBD3804AA@yahoo.com> <99AD89F8-0F90-48BE-A060-DA12FD7129E6@yahoo.com> <20190306223611.75c8a87e@titan.knownspace> <984188AE-93A9-4321-99EC-522C2E0CE9A9@yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 16 Apr 2019 14:26:39 -0700
Mark Millard <marklmi@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190416164210.26827a6d>