[PATCH v2 2/3] ARM: iwmmxt: Port problematic iwmmxt support code to v7/Thumb-2

[Nicolas Pitre]

On Mon, 12 Sep 2011, Dave Martin wrote:

> On Fri, Sep 09, 2011 at 01:51:30PM -0400, Nicolas Pitre wrote:
> > On Fri, 9 Sep 2011, Dave Martin wrote:
> > 
> > > On Fri, Sep 09, 2011 at 09:21:28AM -0400, Nicolas Pitre wrote:
> > > > On Fri, 9 Sep 2011, Jon Medhurst (Tixy) wrote:
> > > > 
> > > > > On Thu, 2011-09-08 at 11:49 -0700, Eric Miao wrote:
> > > > > > So the problem is really when compiling this file with existing toolchain,
> > > > > > it's downgrading to v5 compatible mode, and the instruction below
> > > > > > 
> > > > > > sub pc, lr, r1, lsr #32
> > > > > > 
> > > > > > wouldn't be encoded when building a THUMB2 kernel. Considering the
> > > > > > r1, lsr #32 is actually to create an explicit data dependency of the previous
> > > > > > co-processor instruction, would it be one option to rewrite this as something
> > > > > > like:
> > > > > > 
> > > > > > mov r1, r1
> > > > > > mov pc, lr
> > > > > 
> > > > > That doesn't include a data dependency of PC on R1, so it's possible for
> > > > > MOV PC, LR and subsequent instructions to be executed before MOV R1, R1
> > > > > has completed. We would want...
> > > > > 
> > > > > add lr, lr, r1, lsr #32
> > > > > mov pc, lr
> > > > 
> > > > But isn't the first insn unavailable with Thumb2?
> > > > 
> > > > Maybe something like:
> > > > 
> > > > 	sub	r1, r1, r1
> > > > 	add	pc, lr, r1
> > > 
> > > Thumb-2 implies v7, the v7 implies that we have (and need) a real ISB.  So for the
> > > Thumb-2 case, this should always become
> > > 
> > > 	isb
> > 
> > Don't forget that here we are talking about the Marvell implementation.  
> > Since I've no doubt that the real isb certainly works, so far the 
> > advertized isb has always been implemented with a CP readback with a 
> > data dependency.  Don't forget that this CPU core can also pretend to be 
> > an ARMv6 and they probably didn't duplicate the whole instruction 
> > decoder for each modes.
> 
> If the isb instruction doesn't work for this in v7 mode, I think that
> would be an architecture violation, but you're right that this may
> not be available, or may not behave identically, when the CPU is
> behaving like a v6 part.

I'm sure isb is available in ARMv7 mode.

What I'm saying is that the advertised trick for ARMv6 mode certainly 
must work just as well here in ARMv7 mode.

> > So I really doubt the Marvell implementation would behave any 
> > differently if this special isb is expressed in terms of Thumb2 rather 
> > than ARM instructions as the backend is certainly common to all the 
> > modes. If we can use the same implementation for all modes then we'll 
> > just simplify maintenance of this code.
> 
> Agreed, but the trouble is that what constitutes a suitable data
> dependency is completely microarchitecture dependent -- we're relying
> on side-effects outside the architecture here.

Sure.  But what is there now is what is documented by Marvell docs i.e. 
create a data dependency on the register used to read back the CP value.  
I've seen a few implementation variations on that theme too.

> We can't code _exactly_ the same thing in Thumb-2 because of restrictions
> in the instruction set, so we have to settle for something that "looks
> similar" -- but there's no guarantee it will work.
> 
> Do you feel your judgement on this is authoritative?  If so, then
> we can go ahead with your suggestion; otherwise we might need input
> from someone else.

My suggestion would be what Tixy also proposed:

	mrc	p15, 0, r1, c2, c0, 0
	add	lr, lr, r1, lsr #32
	mov	pc, lr

Running this past people still at Marvell (I'm not anymore) wouldn't 
hurt of course.


Nicolas