On Wed, May 21, 2014 at 02:39:00PM +0100, James Hogan wrote:
> On 20/05/14 15:47, Andreas Herrmann wrote:
> > diff --git a/arch/mips/include/asm/mach-paravirt/kernel-entry-init.h b/arch/mips/include/asm/mach-paravirt/kernel-entry-init.h
> > new file mode 100644
> > index 0000000..c812efa
> > --- /dev/null
> > +++ b/arch/mips/include/asm/mach-paravirt/kernel-entry-init.h
> > @@ -0,0 +1,49 @@
>
> > +/*
> > + * Do SMP slave processor setup necessary before we can safely execute
> > + * C code.
> > + */
> > + .macro smp_slave_setup
> > + mfc0 t0, CP0_EBASE
> > + andi t0, t0, 0x3ff # CPUNum
> > + slti t1, t0, NR_CPUS
> > + bnez t1, 1f
> > +2:
> > + di
> > + wait
> > + b 2b # Unknown CPU, loop forever.
> > +1:
> > + PTR_LA t1, paravirt_smp_sp
> > + PTR_SLL t0, PTR_SCALESHIFT
> > + PTR_ADDU t1, t1, t0
> > +3:
> > + PTR_L sp, 0(t1)
> > + beqz sp, 3b # Spin until told to proceed.
> > +
> > + PTR_LA t1, paravirt_smp_gp
> > + PTR_ADDU t1, t1, t0
>
> Usually smp_wmb() at the writer needs to be paired with smp_rmb() at the
> reader (i.e. here) to guarantee that the two memory locations become
> visible to this CPU in the correct order, so I think you need a sync of
> some kind between here to be portable beyond Octeon.
Yes, I think, I should add a sync here ...
> > + PTR_L gp, 0(t1)
> > + .endm
-- 8< --
> > +static void paravirt_boot_secondary(int cpu, struct task_struct *idle)
> > +{
> > + paravirt_smp_gp[cpu] = (unsigned long)(task_thread_info(idle));
>
> spurious brackets around task_thread_info(idle)
>
> > + wmb();
>
> Wouldn't smp_wmb() be more accurate?
... use smp_wmb there ...
> > + paravirt_smp_sp[cpu] = __KSTK_TOS(idle);
> > + mb();
>
> is this barrier necessary?
... and omit this one.
Andreas
