Re: head -r344018 powerpc64 variant on Powermac G5 (2 sockets, 2 cores each): [*buffer arena] shows up more . . .? [mpc85xx_smp_timebase_sync problem too]

On Tue, 16 Apr 2019 14:26:39 -0700
Mark Millard <[email protected]> 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
_______________________________________________
[email protected] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-ppc
To unsubscribe, send any mail to "[email protected]"