Re: [PATCH 10/15] MIPS: Add code for new system 'paravirt'.

[Andreas Herrmann <andreas.herrmann@caviumnetworks.com>]

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