Re: [PATCH] Handle mtsprg and mfsprg properly for BookE

[Jeff Baker <jbaker at qnx dot com>]

> Correct for what processor?  My BOOKE reference says of SPRG:
>
> 	SPR number	Access		Privileged
> SPRG0	272		Read/Write	Yes
> SPRG1	273		Read/Write	Yes
> SPRG2	274		Read/Write	Yes
> SPRG3	259		Read-only	No
> 	275		Read/Write	Yes
> SPRG4	260		Read-only	No
> 	276		Read/Write	Yes
> SPRG5	261		Read-only	No
> 	277		Read/Write	Yes
> SPRG6	262		Read-only	No
> 	278		Read/Write	Yes
> SPRG7	263		Read-only	No
> 	279		Read/Write	Yes
> USPRG0	256		Read/Write	No
>
> That says to me that there are two possible SPR numbers you use to
> access each of SPRG3 to SPRG7.  And the duplicate numbers start at
> SPRG3, not SPRG4.  Now you know why nobody reviewed your patch, and
> why I'm not approving it as is..

My BookE reference states that user-mode read access to sprg3 is 
implementation dependant.  Since the opcode table doesn't have an 
mfsprg3 with an SPR number of 259 I (apparently incorrectly) assumed 
that we didn't need that.

I also assumed that one reason for submitting a patch to the mailing 
list is so that people who know more than I do can review it and then 
point out where I'm making mistakes.  If that isn't the case then can 
you suggest somewhere I can go or someone I can ask for help if I'm 
working on a patch that I need but don't 100% understand?

> If I was a programmer with an ounce of sense (debatable), I'd want
> to use "mfspr gpr,sprgN" where the sprgN are symbols or macros equated
> to the right spr register for the context, rather than trusting the
> assembler to do the right thing with "mfsprg gpr,N" or even worse,
> "mfsprgN gpr".

That may be true but I was asked to get it working.  I don't believe 
that technically it is correct for m[t,f]sprg to complain that the 
register is greater than 3 if you have specified an architecture that 
should allow up to 7.  Even if so, 'mfsprg %r7,3' will be accepted as 
correct but will produce an SPRG number of 275, not 259 as you 
previously indicated it should.  Same with 'mfsprg3 %r7'.

> > +/* Some dialects (BookE, 405) have 8 SPRG registers instead of
> > + the normal 4.  In addition, mfsprg instructions must
> > + have sprn5 cleared when using registers 4 through 7. */
>
>
> Is this horrible formatting an artifact of your mailer?

No my editor does that.  Easily fixed.

> > +static unsigned long
> > +insert_sprg (unsigned long insn,
> > +	    long value,
> > +	    int dialect,
> > +	    const char **errmsg)
> > +{
> > +  /* This check uses PPC_OPCODE_403 because PPC405 is later defined
> > +   as a synonym.  If ever a 405 specific dialect is added this
> > +   check should use that instead. */
> > +  if ((dialect & PPC_OPCODE_BOOKE) || (dialect & PPC_OPCODE_403))
> > +  {
> > +    if (value > 7)
> > +      *errmsg = _("Invalid operand.  Must be between 0 and 7.");
>
>
> These error messages should be all lower case, and without a full-stop.
> I suggest simply "invalid sprg number".  Then you can restructure the
> function as
>
>   if (value > 7
>       || (value > 3
> 	  && (dialect & (PPC_OPCODE_BOOKE | PPC_OPCODE_403)) == 0))
>     *errmsg = _("invalid sprg number");
>
>   insn |= (value & 7) << 16;
>   if (value > 3 or maybe 2 && some other condition)
>     insn &= bit twiddle;
>   return insn;

Easily done once I'm positive what the proper conditions should be.

> > + 1cc:	7c f3 42 a6	mfsprg  r7,3
> > + 1d0:	7c e7 42 a6	mfsprg7 r7
> > + 1d4:	7c f7 43 a6	mtsprg  7,r7
>
>
> This isn't the nicest disassembly, a result of mfsprg[3-7] appearing in
> the opcode table before mfsprg.

I know.  I briefly thought about implementing an extract_sprg to handle 
that but there really isn't a good way to decide how to display it.

> > +	mfsprg %r7, 3
> > +	mfsprg %r7, 7
> > +	mtsprg 7, %r7