Re: [PATCH 4/7] xen/arm: set the SMP bit in the ACTLR register

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2012-10-25 at 12:57 +0100, Stefano Stabellini wrote:
> On Thu, 25 Oct 2012, Ian Campbell wrote:
> > On Wed, 2012-10-24 at 16:03 +0100, Stefano Stabellini wrote:
> > > From the Cortex A15 manual:
> > > 
> > > "Enables the processor to receive instruction cache, BTB, and TLB maintenance
> > > operations from other processors"
> > > 
> > > ...
> > > 
> > > "You must set this bit before enabling the caches and MMU, or
> > > performing any cache and TLB maintenance operations. The only time
> > > you must clear this bit is during a processor power-down sequence"
> > 
> > Is it considered a bug that the firmware doesn't do this?
> 
> Why would it be? You can run fairly complicated pieces of software
> without caches or MMU.

True, I guess I just considered that not setting the SMP bit on the
second processor when the firmware starts it seemed a bit odd. If you
aren't caches/MMU/etc them then setting the bit is pretty much a NOP (or
maybe it isn't?).

> 
> 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > ---
> > >  xen/arch/arm/head.S             |    6 ++++++
> > >  xen/include/asm-arm/processor.h |   30 ++++++++++++++++++++++++++++++
> > >  2 files changed, 36 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/head.S b/xen/arch/arm/head.S
> > > index 3d01be0..c784f4d 100644
> > > --- a/xen/arch/arm/head.S
> > > +++ b/xen/arch/arm/head.S
> > > @@ -148,6 +148,12 @@ skip_bss:
> > >  
> > >  	PRINT("- Setting up control registers -\r\n")
> > >  	
> > > +	/* XXX: Cortex A15 specific */
> > 
> > We probably need some sort of early proc info keyed of the processor id
> > (like Linux) has so we can push this processor specific stuff into the
> > right places.
> 
> Indeed.
> I wrote something simple that should be OK until we get more than
> ~10 different processors.
> 
> 
> > > +	/* Set up the SMP bit in ACTLR */
> > > +	mrc   CP32(r0, ACTLR)
> > > +	orr   r0, r0, #(ACTLR_SMP) /* enable SMP bit*/
> > > +	mcr   CP32(r0, ACTLR)
> > 
> > Linux does this IFF it isn't already set for some reason:
> > #ifdef CONFIG_SMP
> >         ALT_SMP(mrc     p15, 0, r0, c1, c0, 1)
> >         ALT_UP(mov      r0, #(1 << 6))          @ fake it for UP
> >         tst     r0, #(1 << 6)                   @ SMP/nAMP mode enabled?
> >         orreq   r0, r0, #(1 << 6)               @ Enable SMP/nAMP mode
> >         orreq   r0, r0, r10                     @ Enable CPU-specific SMP bits
> >         mcreq   p15, 0, r0, c1, c0, 1
> > #endif
> > 
> > Might just relate to the "fake it up for UP" thing I suppose.
> 
> Considering that we don't have a UP config option for Xen (I am aware
> of), there is no need for us to do that, I think.
> 
> 
> > > +
> > >  	/* Set up memory attribute type tables */
> > >  	ldr   r0, =MAIR0VAL
> > >  	ldr   r1, =MAIR1VAL
> > > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> > > index 3849b23..91a5836 100644
> > > --- a/xen/include/asm-arm/processor.h
> > > +++ b/xen/include/asm-arm/processor.h
> > > @@ -34,6 +34,36 @@
> > >  #define SCTLR_A         (1<<1)
> > >  #define SCTLR_M         (1<<0)
> > >  
> > > +/* ACTLR Auxiliary Control Register */
> > > +#define ACTLR_SNOOP_DELAYED      (1<<31)
> > 
> > If these are CortexA15 specific then I think they ought to be
> > ACTLR_CA15_FOO or something. And possibly in a new processor-ca15.h
> > header instead.
> 
> OK