[PATCH] MIPS: Fix protected_cache(e)_op() for microMIPS

[PATCH] MIPS: Fix protected_cache(e)_op() for microMIPS

[James Hogan]

From: Paul Burton <paul.burton@imgtec.com>

When building for microMIPS we need to ensure that the assembler always
knows that there is code at the target of a branch or jump. Commit
7170bdc77755 ("MIPS: Add return errors to protected cache ops")
introduced a fixup path to protected_cache(e)_op() which does not meet
this requirement. The fixup path jumps to the "2" label but we don't
know what will be placed at that label since it's at the end of the
inline asm. If the inline asm happens to be followed by an instruction
manually encoded with .word or similar then the toolchain will not know
that "2" labels code and linking will fail with:

  mips-img-linux-gnu-ld: arch/mips/mm/c-r4k.o: .fixup+0x0: Unsupported
  jump between ISA modes; consider recompiling with interlinking
  enabled.

Fix this by declaring that "2" labels code using the .insn directive.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Fixes: 7170bdc77755 ("MIPS: Add return errors to protected cache ops")
Cc: linux-mips@linux-mips.org
Cc: Ralf Baechle <ralf@linux-mips.org>
Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
Ralf: This fixes microMIPS build since a patch that is already merged
into kvm/next. I was going to send you a pull request for those patches
anyway, so it probably makes sense if I just append to that branch first
and let the fix get upstream via the MIPS tree.
---
 arch/mips/include/asm/r4kcache.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/r4kcache.h b/arch/mips/include/asm/r4kcache.h
index 7227c158cbf8..55fd94e6cd0b 100644
--- a/arch/mips/include/asm/r4kcache.h
+++ b/arch/mips/include/asm/r4kcache.h
@@ -154,7 +154,8 @@ static inline void flush_scache_line(unsigned long addr)
 	"	.set	noreorder		\n"		\
 	"	.set "MIPS_ISA_ARCH_LEVEL"	\n"		\
 	"1:	cache	%1, (%2)		\n"		\
-	"2:	.set	pop			\n"		\
+	"2:	.insn				\n"		\
+	"	.set	pop			\n"		\
 	"	.section .fixup,\"ax\"		\n"		\
 	"3:	li	%0, %3			\n"		\
 	"	j	2b			\n"		\
@@ -177,7 +178,8 @@ static inline void flush_scache_line(unsigned long addr)
 	"	.set	mips0			\n"		\
 	"	.set	eva			\n"		\
 	"1:	cachee	%1, (%2)		\n"		\
-	"2:	.set	pop			\n"		\
+	"2:	.insn				\n"		\
+	"	.set	pop			\n"		\
 	"	.section .fixup,\"ax\"		\n"		\
 	"3:	li	%0, %3			\n"		\
 	"	j	2b			\n"		\
-- 
2.11.0

[PATCH] MIPS: Fix protected_cache(e)_op() for microMIPS

[James Hogan]

From: Paul Burton <paul.burton@imgtec.com>

When building for microMIPS we need to ensure that the assembler always
knows that there is code at the target of a branch or jump. Commit
7170bdc77755 ("MIPS: Add return errors to protected cache ops")
introduced a fixup path to protected_cache(e)_op() which does not meet
this requirement. The fixup path jumps to the "2" label but we don't
know what will be placed at that label since it's at the end of the
inline asm. If the inline asm happens to be followed by an instruction
manually encoded with .word or similar then the toolchain will not know
that "2" labels code and linking will fail with:

  mips-img-linux-gnu-ld: arch/mips/mm/c-r4k.o: .fixup+0x0: Unsupported
  jump between ISA modes; consider recompiling with interlinking
  enabled.

Fix this by declaring that "2" labels code using the .insn directive.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Fixes: 7170bdc77755 ("MIPS: Add return errors to protected cache ops")
Cc: linux-mips@linux-mips.org
Cc: Ralf Baechle <ralf@linux-mips.org>
Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
Ralf: This fixes microMIPS build since a patch that is already merged
into kvm/next. I was going to send you a pull request for those patches
anyway, so it probably makes sense if I just append to that branch first
and let the fix get upstream via the MIPS tree.
---
 arch/mips/include/asm/r4kcache.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/r4kcache.h b/arch/mips/include/asm/r4kcache.h
index 7227c158cbf8..55fd94e6cd0b 100644
--- a/arch/mips/include/asm/r4kcache.h
+++ b/arch/mips/include/asm/r4kcache.h
@@ -154,7 +154,8 @@ static inline void flush_scache_line(unsigned long addr)
 	"	.set	noreorder		\n"		\
 	"	.set "MIPS_ISA_ARCH_LEVEL"	\n"		\
 	"1:	cache	%1, (%2)		\n"		\
-	"2:	.set	pop			\n"		\
+	"2:	.insn				\n"		\
+	"	.set	pop			\n"		\
 	"	.section .fixup,\"ax\"		\n"		\
 	"3:	li	%0, %3			\n"		\
 	"	j	2b			\n"		\
@@ -177,7 +178,8 @@ static inline void flush_scache_line(unsigned long addr)
 	"	.set	mips0			\n"		\
 	"	.set	eva			\n"		\
 	"1:	cachee	%1, (%2)		\n"		\
-	"2:	.set	pop			\n"		\
+	"2:	.insn				\n"		\
+	"	.set	pop			\n"		\
 	"	.section .fixup,\"ax\"		\n"		\
 	"3:	li	%0, %3			\n"		\
 	"	j	2b			\n"		\
-- 
2.11.0

Re: [PATCH] MIPS: Fix protected_cache(e)_op() for microMIPS

[Maciej W. Rozycki]

On Wed, 8 Feb 2017, James Hogan wrote:

> When building for microMIPS we need to ensure that the assembler always
> knows that there is code at the target of a branch or jump. Commit
> 7170bdc77755 ("MIPS: Add return errors to protected cache ops")
> introduced a fixup path to protected_cache(e)_op() which does not meet
> this requirement. The fixup path jumps to the "2" label but we don't
> know what will be placed at that label since it's at the end of the
> inline asm. If the inline asm happens to be followed by an instruction
> manually encoded with .word or similar then the toolchain will not know
> that "2" labels code and linking will fail with:
> 
>   mips-img-linux-gnu-ld: arch/mips/mm/c-r4k.o: .fixup+0x0: Unsupported
>   jump between ISA modes; consider recompiling with interlinking
>   enabled.

 That is not really the cause.  The cause is the `.section' pseudo-op that 
immediately physically follows, and which causes GAS to fix its current 
state.  Since by this point it hasn't seen an instruction any outstanding 
labels that haven't been finalised will be marked as `object' (i.e. data).  
So it doesn't really matter what follows the inline asm, an ISA mode 
mismatch will always happen here.

 The amount of infrastructure that would be required to have state saved 
and then restored where applicable to "see through section switches" is 
substantial enough for no one to have tried implementing it so far. 

 Consequently all places where a code label is immediately followed by one 
of the section switch directives (technically, by the syntax rules of the 
assembly language, such a label is attached to the directive), such as 
`.section', `.previous', `.popsection', etc., need to have `.insn' added 
to be treated correctly (arguably we could make them mark labels as 
`function' instead, however I fear it may break something out there in a 
subtler way as you won't get a similar error for data accesses and the 
semantics has been like this since the introduction of MIPS16 support back 
in 1996).

 I suggest that you send an update with the description amended so as not 
to confuse people about the actual cause.  Overall, unless you have done 
that already, I think it would be the best if our sources were reviewed 
for any remaining places across the MIPS port where a code label is 
followed by a section switch and `.insn' added there right away.  It will 
prevent people sneaking in new bad code by copying and pasting from the 
wrong place.

 I'll be happy to ack a patch with an updated description.

  Maciej

Re: [PATCH] MIPS: Fix protected_cache(e)_op() for microMIPS

[Maciej W. Rozycki]

On Wed, 8 Feb 2017, James Hogan wrote:

> When building for microMIPS we need to ensure that the assembler always
> knows that there is code at the target of a branch or jump. Commit
> 7170bdc77755 ("MIPS: Add return errors to protected cache ops")
> introduced a fixup path to protected_cache(e)_op() which does not meet
> this requirement. The fixup path jumps to the "2" label but we don't
> know what will be placed at that label since it's at the end of the
> inline asm. If the inline asm happens to be followed by an instruction
> manually encoded with .word or similar then the toolchain will not know
> that "2" labels code and linking will fail with:
> 
>   mips-img-linux-gnu-ld: arch/mips/mm/c-r4k.o: .fixup+0x0: Unsupported
>   jump between ISA modes; consider recompiling with interlinking
>   enabled.

 That is not really the cause.  The cause is the `.section' pseudo-op that 
immediately physically follows, and which causes GAS to fix its current 
state.  Since by this point it hasn't seen an instruction any outstanding 
labels that haven't been finalised will be marked as `object' (i.e. data).  
So it doesn't really matter what follows the inline asm, an ISA mode 
mismatch will always happen here.

 The amount of infrastructure that would be required to have state saved 
and then restored where applicable to "see through section switches" is 
substantial enough for no one to have tried implementing it so far. 

 Consequently all places where a code label is immediately followed by one 
of the section switch directives (technically, by the syntax rules of the 
assembly language, such a label is attached to the directive), such as 
`.section', `.previous', `.popsection', etc., need to have `.insn' added 
to be treated correctly (arguably we could make them mark labels as 
`function' instead, however I fear it may break something out there in a 
subtler way as you won't get a similar error for data accesses and the 
semantics has been like this since the introduction of MIPS16 support back 
in 1996).

 I suggest that you send an update with the description amended so as not 
to confuse people about the actual cause.  Overall, unless you have done 
that already, I think it would be the best if our sources were reviewed 
for any remaining places across the MIPS port where a code label is 
followed by a section switch and `.insn' added there right away.  It will 
prevent people sneaking in new bad code by copying and pasting from the 
wrong place.

 I'll be happy to ack a patch with an updated description.

  Maciej