[PATCH 0/2] Fix fallouts from asm/opcodes.h removal

[PATCH 0/2] Fix fallouts from asm/opcodes.h removal

[Marc Zyngier]

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(-)

-- 
2.10.2

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

[Marc Zyngier]

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 <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/probes.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h
index e175a82..6a5b289 100644
--- a/arch/arm64/include/asm/probes.h
+++ b/arch/arm64/include/asm/probes.h
@@ -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 *);
 
-- 
2.10.2

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

[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 <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/alternative.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index 39feb85..39f8c98 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -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
 
-- 
2.10.2

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

[Will Deacon]

On Sat, Dec 03, 2016 at 02:05:38PM +0000, Marc Zyngier wrote:
> 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).

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.

> 
> Fixes: 792d47379f4d ("arm64: alternative: add auto-nop infrastructure")
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/alternative.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
> index 39feb85..39f8c98 100644
> --- a/arch/arm64/include/asm/alternative.h
> +++ b/arch/arm64/include/asm/alternative.h
> @@ -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.

> +
>  .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

> +	 * "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

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

[Marc Zyngier]

On 05/12/16 10:05, Will Deacon wrote:
> On Sat, Dec 03, 2016 at 02:05:38PM +0000, Marc Zyngier wrote:
>> 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).
> 
> 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.

> 
>>
>> Fixes: 792d47379f4d ("arm64: alternative: add auto-nop infrastructure")
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/include/asm/alternative.h | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
>> index 39feb85..39f8c98 100644
>> --- a/arch/arm64/include/asm/alternative.h
>> +++ b/arch/arm64/include/asm/alternative.h
>> @@ -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.

> 
>> +
>>  .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
> 
>> +	 * "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.
-- 
Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

On 05/12/16 10:58, Marc Zyngier wrote:
> On 05/12/16 10:05, Will Deacon wrote:
>> On Sat, Dec 03, 2016 at 02:05:38PM +0000, Marc Zyngier wrote:
>>> 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).
>>
>> 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.

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.
-- 
Jazz is not dead. It just smells funny...