[PATCH] MIPS: Stop using dla in 32 bit kernels

[PATCH] MIPS: Stop using dla in 32 bit kernels

[Paul Burton]

The instruction_hazard macro made use of the dla pseudo-instruction even
for 32 bit kernels. Although it surrounded that use with ".set mips64rX"
that isn't sufficient to avoid warnings when built with -mabi=32, as
CONFIG_32BIT=y kernels are:

      CC      arch/mips/mm/c-r4k.o
    {standard input}: Assembler messages:
    {standard input}:4105: Warning: dla used to load 32-bit register;
        recommend using la instead
    {standard input}:4129: Warning: dla used to load 32-bit register;
        recommend using la instead

Avoid this by instead making use of the PTR_LA macro which defines the
appropriate variant of the "la" instruction to use.

Tested with Codescape GNU Tools 2015.06-05 for MIPS IMG Linux, which
includes binutils 2.24.90 & gcc 4.9.2.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---

 arch/mips/include/asm/hazards.h | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/mips/include/asm/hazards.h b/arch/mips/include/asm/hazards.h
index 7b99efd..ffe2aaa 100644
--- a/arch/mips/include/asm/hazards.h
+++ b/arch/mips/include/asm/hazards.h
@@ -11,6 +11,7 @@
 #define _ASM_HAZARDS_H
 
 #include <linux/stringify.h>
+#include <asm/asm.h>
 #include <asm/compiler.h>
 
 #define ___ssnop							\
@@ -54,22 +55,16 @@
 
 /*
  * gcc has a tradition of misscompiling the previous construct using the
- * address of a label as argument to inline assembler.	Gas otoh has the
- * annoying difference between la and dla which are only usable for 32-bit
- * rsp. 64-bit code, so can't be used without conditional compilation.
- * The alterantive is switching the assembler to 64-bit code which happens
- * to work right even for 32-bit code ...
+ * address of a label as argument to inline assembler.
  */
 #define instruction_hazard()						\
 do {									\
 	unsigned long tmp;						\
 									\
 	__asm__ __volatile__(						\
-	"	.set "MIPS_ISA_LEVEL"				\n"	\
-	"	dla	%0, 1f					\n"	\
-	"	jr.hb	%0					\n"	\
-	"	.set	mips0					\n"	\
-	"1:							\n"	\
+	__stringify(PTR_LA) "	%0, 1f\n\t"				\
+	"jr.hb	%0\n\t"							\
+	"1:"								\
 	: "=r" (tmp));							\
 } while (0)
 
-- 
2.7.0

Re: [PATCH] MIPS: Stop using dla in 32 bit kernels

[Ralf Baechle]

On Thu, Feb 04, 2016 at 02:31:57PM +0000, Paul Burton wrote:

>       CC      arch/mips/mm/c-r4k.o
>     {standard input}: Assembler messages:
>     {standard input}:4105: Warning: dla used to load 32-bit register;
>         recommend using la instead
>     {standard input}:4129: Warning: dla used to load 32-bit register;
>         recommend using la instead

Sigh.  Another new binutils warning?

> Avoid this by instead making use of the PTR_LA macro which defines the
> appropriate variant of the "la" instruction to use.
> 
> Tested with Codescape GNU Tools 2015.06-05 for MIPS IMG Linux, which
> includes binutils 2.24.90 & gcc 4.9.2.

> @@ -54,22 +55,16 @@
>  
>  /*
>   * gcc has a tradition of misscompiling the previous construct using the
> - * address of a label as argument to inline assembler.	Gas otoh has the
> - * annoying difference between la and dla which are only usable for 32-bit
> - * rsp. 64-bit code, so can't be used without conditional compilation.
> - * The alterantive is switching the assembler to 64-bit code which happens
> - * to work right even for 32-bit code ...
> + * address of a label as argument to inline assembler.
>   */
>  #define instruction_hazard()						\
>  do {									\
>  	unsigned long tmp;						\
>  									\
>  	__asm__ __volatile__(						\
> -	"	.set "MIPS_ISA_LEVEL"				\n"	\
> -	"	dla	%0, 1f					\n"	\
> -	"	jr.hb	%0					\n"	\
> -	"	.set	mips0					\n"	\
> -	"1:							\n"	\
> +	__stringify(PTR_LA) "	%0, 1f\n\t"				\
> +	"jr.hb	%0\n\t"							\
> +	"1:"								\
>  	: "=r" (tmp));							\
>  } while (0)


The .set will need to stay or this will fail up on older processors
with

/tmp/ccKNXiPT.s:21: Error: opcode not supported on this processor: mips1 (mips1) `jr.hb '

The opcode of JR.HB will by older processors be treated as just a JR afair.

Or with less inline assembler obscurities something like:

void foo(void)
{
        void *jr = &&jr;

        __asm__ __volatile__(
	"       .set	"MIPS_ISA_LEVEL"				\n"
        "       jr.hb							\n"
	"	.set	mips0						\n"
        : /* no outputs */
        : "r" (jr));
jr:     ;
}

Now GCC can even schedule loading the address or do other clever things.

  Ralf

Re: [PATCH] MIPS: Stop using dla in 32 bit kernels

[Paul Burton]

On Thu, Feb 04, 2016 at 04:18:34PM +0100, Ralf Baechle wrote:
> On Thu, Feb 04, 2016 at 02:31:57PM +0000, Paul Burton wrote:
> 
> >       CC      arch/mips/mm/c-r4k.o
> >     {standard input}: Assembler messages:
> >     {standard input}:4105: Warning: dla used to load 32-bit register;
> >         recommend using la instead
> >     {standard input}:4129: Warning: dla used to load 32-bit register;
> >         recommend using la instead
> 
> Sigh.  Another new binutils warning?
> 
> > Avoid this by instead making use of the PTR_LA macro which defines the
> > appropriate variant of the "la" instruction to use.
> > 
> > Tested with Codescape GNU Tools 2015.06-05 for MIPS IMG Linux, which
> > includes binutils 2.24.90 & gcc 4.9.2.
> 
> > @@ -54,22 +55,16 @@
> >  
> >  /*
> >   * gcc has a tradition of misscompiling the previous construct using the
> > - * address of a label as argument to inline assembler.	Gas otoh has the
> > - * annoying difference between la and dla which are only usable for 32-bit
> > - * rsp. 64-bit code, so can't be used without conditional compilation.
> > - * The alterantive is switching the assembler to 64-bit code which happens
> > - * to work right even for 32-bit code ...
> > + * address of a label as argument to inline assembler.
> >   */
> >  #define instruction_hazard()						\
> >  do {									\
> >  	unsigned long tmp;						\
> >  									\
> >  	__asm__ __volatile__(						\
> > -	"	.set "MIPS_ISA_LEVEL"				\n"	\
> > -	"	dla	%0, 1f					\n"	\
> > -	"	jr.hb	%0					\n"	\
> > -	"	.set	mips0					\n"	\
> > -	"1:							\n"	\
> > +	__stringify(PTR_LA) "	%0, 1f\n\t"				\
> > +	"jr.hb	%0\n\t"							\
> > +	"1:"								\
> >  	: "=r" (tmp));							\
> >  } while (0)
> 
> 
> The .set will need to stay or this will fail up on older processors
> with
> 
> /tmp/ccKNXiPT.s:21: Error: opcode not supported on this processor: mips1 (mips1) `jr.hb '
> 
> The opcode of JR.HB will by older processors be treated as just a JR afair.

Ok, I'll put the .set back.

> Or with less inline assembler obscurities something like:
> 
> void foo(void)
> {
>         void *jr = &&jr;
> 
>         __asm__ __volatile__(
> 	"       .set	"MIPS_ISA_LEVEL"				\n"
>         "       jr.hb							\n"
> 	"	.set	mips0						\n"
>         : /* no outputs */
>         : "r" (jr));
> jr:     ;
> }
> 
> Now GCC can even schedule loading the address or do other clever things.

Yes, but judging from the comment preceeding instruction_hazard() some
versions of gcc can also do clever things like miscompile that. Maybe we
don't care any more - the comment doesn't say which versions were
affected...

Thanks,
    Paul

>   Ralf

Re: [PATCH] MIPS: Stop using dla in 32 bit kernels

[Maciej W. Rozycki]

On Thu, 4 Feb 2016, Ralf Baechle wrote:

> >       CC      arch/mips/mm/c-r4k.o
> >     {standard input}: Assembler messages:
> >     {standard input}:4105: Warning: dla used to load 32-bit register;
> >         recommend using la instead
> >     {standard input}:4129: Warning: dla used to load 32-bit register;
> >         recommend using la instead
> 
> Sigh.  Another new binutils warning?

 It's been there almost since forever or specifically:

commit 3bec30a8305f9f5d5649b5e1fc9ed78a1c3c109a
Author: Thiemo Seufer <ths@networkno.de>
Date:   Tue May 14 23:35:59 2002 +0000

	* config/tc-mips.c (macro): Warn about wrong la/dla use.

It might be interesting to know what changed in the kernel to trigger this 
warning -- at the very least see the CONFIG_CPU option active and the 
compiler's command line hidden in the original report; I'm assuming 
arch/mips/mm/c-r4k.o is built with `-Werror', so it would be a 
catastrophic failure if it triggered before and it's indeed a regression 
rather than a bug which has always been there.

> > @@ -54,22 +55,16 @@
> >  
> >  /*
> >   * gcc has a tradition of misscompiling the previous construct using the
> > - * address of a label as argument to inline assembler.	Gas otoh has the
> > - * annoying difference between la and dla which are only usable for 32-bit
> > - * rsp. 64-bit code, so can't be used without conditional compilation.
> > - * The alterantive is switching the assembler to 64-bit code which happens
> > - * to work right even for 32-bit code ...
> > + * address of a label as argument to inline assembler.
> >   */
[...]
> The .set will need to stay or this will fail up on older processors
> with
> 
> /tmp/ccKNXiPT.s:21: Error: opcode not supported on this processor: mips1 (mips1) `jr.hb '
> 
> The opcode of JR.HB will by older processors be treated as just a JR afair.

 Some will run and some may trap with RI I believe.  IIRC the original 
MIPS32r1 4Kc (not 4KEc) traps, though this might merely have been an 
erratum we could handle in `do_ri', as the ERET at exit would do as well.

> Or with less inline assembler obscurities something like:
> 
> void foo(void)
> {
>         void *jr = &&jr;
> 
>         __asm__ __volatile__(
> 	"       .set	"MIPS_ISA_LEVEL"				\n"
>         "       jr.hb							\n"
> 	"	.set	mips0						\n"
>         : /* no outputs */
>         : "r" (jr));
> jr:     ;
> }
> 
> Now GCC can even schedule loading the address or do other clever things.

 This may have a chance to work, but I don't think it's a supported 
construct.  This is asking for trouble IMHO, as GCC expects an asm to fall 
through and you might be missing instructions which the compiler has 
placed between the asm and `jr' (IIUC this is even noted in the comment I 
left quoted above).  Of course here we expect nothing to be there, but 
still.

 With newer GCC versions I think you could use `asm goto', however it's a 
pretty recent addition and I don't think we want to limit people in the 
choice of the compiler to use to build Linux for such an obscure corner 
case.

  Maciej