Fix fallouts from asm/opcodes.h removal

PATCH New Marc Zyngier 2 participants 6 messages Dec 3, 2016 – Dec 5, 2016 arm64

lore

Share & tools

lore

Marc Zyngier

author

2016-12-03 14:05 UTC #0 lore raw

cover Fix fallouts from asm/opcodes.h removal

Since the asm/opcodes.h file was removed, two bugs have cropped up:

- probes.h contains a reference to asm/opcodes.h that was missed

- An ugly (and alas familiar) bug has reappeared, leading to obscure compilation errors when a .inst is part of an alternative and that the "auto nop" feature is used to pad the alternative sequence (and the whole thing is assembled with an old version of gas). This is triggered again now that we generate SET_PSTATE_PAN/UAO using .inst.

The first bug is easy to fix, while the second requires some really ugly (if limited) surgery using the .fill directive to replace the "nops" macro.

Marc Zyngier (2):
arm64: Remove reference to asm/opcodes.h
arm64: alternatives: Work around NOP generation with broken assembler

 arch/arm64/include/asm/alternative.h | 12 +++++++++++-
 arch/arm64/include/asm/probes.h      |  2 --
 2 files changed, 11 insertions(+), 3 deletions(-)

Signature · 1 line

2.10.2

Marc Zyngier

author

2016-12-03 14:05 UTC #1 lore raw

PATCH 1/2 arm64: Remove reference to asm/opcodes.h

The asm/opcodes.h file is now gone, but probes.h still references it for not obvious reason. Removing the #include directive fixes the compilation.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/probes.h | 2 --
 1 file changed, 2 deletions(-)

arch/arm64/include/asm/probes.h +0 -2

index e175a82..6a5b289 100644
@@ -15,8 +15,6 @@
 #ifndef _ARM_PROBES_H
 #define _ARM_PROBES_H
 
-#include <asm/opcodes.h>
-
 typedef u32 probe_opcode_t;
 typedef void (probes_handler_t) (u32 opcode, long addr, struct pt_regs *);

Signature · 1 line

2.10.2

Marc Zyngier

author

2016-12-03 14:05 UTC #2 lore raw

PATCH 2/2 arm64: alternatives: Work around NOP generation with broken assembler

In reply to Marc Zyngier

When compiling a .inst directive in an alternative clause with a rather old version of gas (circa 2.24), and when used with the alternative_else_nop_endif feature, the compilation fails with a rather cryptic message such as:

arch/arm64/lib/clear_user.S:33: Error: bad or irreducible absolute expression

which is caused by the bug described in eb7c11ee3c5c ("arm64:
alternative: Work around .inst assembler bugs").

This effectively prevents the use of the "nops" macro, which requires the number of instruction as a parameter (the assembler is confused and unable to compute the difference between two labels).

As an alternative(!), use the .fill directive to output the number of required NOPs (.fill has the good idea to output the fill pattern in the endianness of the instruction stream).

Fixes 792d47379f4d ("arm64: alternative: add auto-nop infrastructure")

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/alternative.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

arch/arm64/include/asm/alternative.h +11 -1

index 39feb85..39f8c98 100644
@@ -159,9 +159,19 @@ void apply_alternatives(void *start, size_t length);
  * of NOPs. The number of NOPs is chosen automatically to match the
  * previous case.
  */
+
+#define NOP_INST	0xd503201f
+
 .macro alternative_else_nop_endif
 alternative_else
-	nops	(662b-661b) / AARCH64_INSN_SIZE
+	/*
+	 * The same assembler bug strikes again here if the first half
+	 * of the alternative sequence contains a .inst, leading to a
+	 * bizarre error message. Fortulately, .fill replaces the
+	 * "nops" macro by inserting padding with the target machine
+	 * endianness.
+	 */
+	.fill	(662b-661b) / AARCH64_INSN_SIZE, AARCH64_INSN_SIZE, NOP_INST
 alternative_endif
 .endm

Signature · 1 line

2.10.2

Will Deacon

2016-12-05 10:05 UTC #3 lore raw

PATCH 2/2 arm64: alternatives: Work around NOP generation with broken assembler

Quoted 10 lines · Marc Zyngier

When compiling a .inst directive in an alternative clause with a rather old version of gas (circa 2.24), and when used with the alternative_else_nop_endif feature, the compilation fails with a rather cryptic message such as:
arch/arm64/lib/clear_user.S:33: Error: bad or irreducible absolute expression
which is caused by the bug described in eb7c11ee3c5c ("arm64:
alternative: Work around .inst assembler bugs").

Quoted 7 lines

This effectively prevents the use of the "nops" macro, which requires the number of instruction as a parameter (the assembler is confused and unable to compute the difference between two labels).
As an alternative(!), use the .fill directive to output the number of required NOPs (.fill has the good idea to output the fill pattern in the endianness of the instruction stream).

Are you sure about that? The gas docs say:

`The contents of each repeat bytes is taken from an 8-byte number. The
highest order 4 bytes are zero. The lowest order 4 bytes are value rendered in the byte-order of an integer on the computer as is assembling for.'

and I'd expect "integer" to refer to data values, rather than instructions.

Quoted 12 lines

Fixes 792d47379f4d ("arm64: alternative: add auto-nop infrastructure")
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/include/asm/alternative.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
arch/arm64/include/asm/alternative.h +0 -0
index 39feb85..39f8c98 100644

arch/arm64/include/asm/alternative.h void apply_alternatives(void *start, size_t length); · line 163

Quoted 5 lines

arch/arm64/include/asm/alternative.h +2 -0

@@ -159,9 +159,19 @@ void apply_alternatives(void *start, size_t length);
  * of NOPs. The number of NOPs is chosen automatically to match the
  * previous case.
  */
+
+#define NOP_INST	0xd503201f

If we define this, let's put it in insn.h or something. It could even be used in the vdso linker script, which open codes this constant.

arch/arm64/include/asm/alternative.h void apply_alternatives(void *start, size_t length); · line 170

Quoted 8 lines

arch/arm64/include/asm/alternative.h +5 -1

+
 .macro alternative_else_nop_endif
 alternative_else
-	nops	(662b-661b) / AARCH64_INSN_SIZE
+	/*
+	 * The same assembler bug strikes again here if the first half
+	 * of the alternative sequence contains a .inst, leading to a
+	 * bizarre error message. Fortulately, .fill replaces the

Fortunately

arch/arm64/include/asm/alternative.h void apply_alternatives(void *start, size_t length); · line 174

Quoted 4 lines

arch/arm64/include/asm/alternative.h +4 -0

+	 * "nops" macro by inserting padding with the target machine
+	 * endianness.
+	 */
+	.fill	(662b-661b) / AARCH64_INSN_SIZE, AARCH64_INSN_SIZE, NOP_INST

Assuming we can get .fill to do the right thing, we should probably use it in __nops rather than open-coding it here. Or is the issue that we're unable to pass (662b-661b) / AARCH64_INSN_SIZE to any macro?

Will

Marc Zyngier

author

2016-12-05 10:58 UTC #4 lore raw

PATCH 2/2 arm64: alternatives: Work around NOP generation with broken assembler

Quoted 19 lines · Will Deacon

Quoted 10 lines · Marc Zyngier
When compiling a .inst directive in an alternative clause with a rather old version of gas (circa 2.24), and when used with the alternative_else_nop_endif feature, the compilation fails with a rather cryptic message such as:
arch/arm64/lib/clear_user.S:33: Error: bad or irreducible absolute expression
which is caused by the bug described in eb7c11ee3c5c ("arm64:
alternative: Work around .inst assembler bugs").
Quoted 7 lines
This effectively prevents the use of the "nops" macro, which requires the number of instruction as a parameter (the assembler is confused and unable to compute the difference between two labels).
As an alternative(!), use the .fill directive to output the number of required NOPs (.fill has the good idea to output the fill pattern in the endianness of the instruction stream).

Quoted 7 lines

Are you sure about that? The gas docs say:
`The contents of each repeat bytes is taken from an 8-byte number. The
highest order 4 bytes are zero. The lowest order 4 bytes are value rendered in the byte-order of an integer on the computer as is assembling for.'
and I'd expect "integer" to refer to data values, rather than instructions.

My tests on 2.24 and 2.25 seem to show that the output is always LE, which could be another GAS issue. I've asked the binutils people for information.

Quoted 8 lines

Quoted 8 lines
Fixes 792d47379f4d ("arm64: alternative: add auto-nop infrastructure")
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/include/asm/alternative.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Quoted 13 lines

arch/arm64/include/asm/alternative.h void apply_alternatives(void *start, size_t length); · line 163
Quoted 10 lines
arch/arm64/include/asm/alternative.h +2 -0
index 39feb85..39f8c98 100644
@@ -159,9 +159,19 @@ void apply_alternatives(void *start, size_t length);
  * of NOPs. The number of NOPs is chosen automatically to match the
  * previous case.
  */
+
+#define NOP_INST	0xd503201f
If we define this, let's put it in insn.h or something. It could even be used in the vdso linker script, which open codes this constant.

Sure.

Quoted 12 lines

arch/arm64/include/asm/alternative.h void apply_alternatives(void *start, size_t length); · line 170
Quoted 9 lines
arch/arm64/include/asm/alternative.h +5 -1
+
 .macro alternative_else_nop_endif
 alternative_else
-	nops	(662b-661b) / AARCH64_INSN_SIZE
+	/*
+	 * The same assembler bug strikes again here if the first half
+	 * of the alternative sequence contains a .inst, leading to a
+	 * bizarre error message. Fortulately, .fill replaces the
Fortunately

Quoted 8 lines

arch/arm64/include/asm/alternative.h void apply_alternatives(void *start, size_t length); · line 174
Quoted 4 lines
arch/arm64/include/asm/alternative.h +4 -0
+	 * "nops" macro by inserting padding with the target machine
+	 * endianness.
+	 */
+	.fill	(662b-661b) / AARCH64_INSN_SIZE, AARCH64_INSN_SIZE, NOP_INST
Assuming we can get .fill to do the right thing, we should probably use it in __nops rather than open-coding it here. Or is the issue that we're unable to pass (662b-661b) / AARCH64_INSN_SIZE to any macro?

Having an intermediate macro seems to work (probably because this is not actually evaluated until it reaches .fill). I'll update the patch if I get confirmation of the validity of the workaround from the binutils people.

Otherwise, we'll have to find another alternative, and maybe go back to not using the auto-nop for SET_PSTATE_PAN/UAO.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Marc Zyngier

author

2016-12-05 11:54 UTC #5 lore raw

PATCH 2/2 arm64: alternatives: Work around NOP generation with broken assembler

Quoted 26 lines · Marc Zyngier

Quoted 25 lines · Will Deacon
Quoted 10 lines · Marc Zyngier
When compiling a .inst directive in an alternative clause with a rather old version of gas (circa 2.24), and when used with the alternative_else_nop_endif feature, the compilation fails with a rather cryptic message such as:
arch/arm64/lib/clear_user.S:33: Error: bad or irreducible absolute expression
which is caused by the bug described in eb7c11ee3c5c ("arm64:
alternative: Work around .inst assembler bugs").
Quoted 7 lines
This effectively prevents the use of the "nops" macro, which requires the number of instruction as a parameter (the assembler is confused and unable to compute the difference between two labels).
As an alternative(!), use the .fill directive to output the number of required NOPs (.fill has the good idea to output the fill pattern in the endianness of the instruction stream).
Are you sure about that? The gas docs say:
`The contents of each repeat bytes is taken from an 8-byte number. The
highest order 4 bytes are zero. The lowest order 4 bytes are value rendered in the byte-order of an integer on the computer as is assembling for.'

Quoted 5 lines

Quoted 1 line
and I'd expect "integer" to refer to data values, rather than instructions.
My tests on 2.24 and 2.25 seem to show that the output is always LE, which could be another GAS issue. I've asked the binutils people for information.

Well, my testing was wrong, as objdump -d was lying to me by displaying something in little-endian form. Time for some more head scratching.

M.

Signature · 1 line

Jazz is not dead. It just smells funny...