use adrp/add pairs for PLT entries
cover use adrp/add pairs for PLT entries
Currently, PLT entries use a non-idiomatic movn/movz/movz/br instruction sequence which is also longer than necessary. Also, the code emitting them does not use the instruction generation code but open codes the opcodes directly.
The extended KASLR range is now 4 GB, given that we switched to the small code model everywhere else (including for modules), so we can switch to adrp/add/br sequences which are easier in the I-cache.
So implement adrp/add pair generation in the instruction generation code and wire it up into the PLT code. Note that the Cortex-A53 errata handling requires some special care to ensure that generated veneers are not susceptible to the erratum.
Ard Biesheuvel (2):
arm64/insn: add support for emitting ADR/ADRP instructions arm64/module: switch to ADRP/ADD sequences for PLT entries
arch/arm64/include/asm/insn.h | 8 ++ arch/arm64/include/asm/module.h | 36 ++------ arch/arm64/kernel/ftrace.c | 2 +- arch/arm64/kernel/insn.c | 29 ++++++ arch/arm64/kernel/module-plts.c | 93 +++++++++++++++----- arch/arm64/kernel/module.c | 4 +- 6 files changed, 119 insertions(+), 53 deletions(-)
Signature · 1 line
2.17.1
PATCH 1/2 arm64/insn: add support for emitting ADR/ADRP instructions
Add support for emitting ADR and ADRP instructions so we can switch over our PLT generation code in a subsequent patch.
--- arch/arm64/include/asm/insn.h | 8 ++++++ arch/arm64/kernel/insn.c | 29 ++++++++++++++++++++ 2 files changed, 37 insertions(+)
arch/arm64/include/asm/insn.h +8 -0
@@ -261,6 +261,11 @@ enum aarch64_insn_prfm_policy { AARCH64_INSN_PRFM_POLICY_STRM, }; +enum aarch64_insn_adr_type { +AARCH64_INSN_ADR_TYPE_ADRP , +AARCH64_INSN_ADR_TYPE_ADR , +}; +#define __AARCH64_INSN_FUNCS (abbr, mask, val) \ static __always_inline bool aarch64_insn_is_##abbr(u32 code) \ { return (code & (mask)) == (val); } \
@@ -393,6 +398,9 @@ u32 aarch64_insn_gen_add_sub_imm(enum aarch64_insn_register dst,enum aarch64_insn_register src, int imm, enum aarch64_insn_variant variant, enum aarch64_insn_adsb_type type); +u32 aarch64_insn_gen_adr (unsigned long pc,unsigned long addr, +enum aarch64_insn_register reg, +enum aarch64_insn_adr_type type); u32 aarch64_insn_gen_bitfield(enum aarch64_insn_register dst, enum aarch64_insn_register src, int immr, int imms,
arch/arm64/kernel/insn.c +29 -0
@@ -1239,6 +1239,35 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,return aarch64_insn_encode_immediate (AARCH64_INSN_IMM_6 , insn, shift); } +u32 aarch64_insn_gen_adr (unsigned long pc,unsigned long addr, +enum aarch64_insn_register reg, +enum aarch64_insn_adr_type type) +{ +u32 insn; +s32 offset; + +switch (type) { +case AARCH64_INSN_ADR_TYPE_ADR : + insn =aarch64_insn_get_adr_value (); + offset = addr - pc; +break ; +case AARCH64_INSN_ADR_TYPE_ADRP : + insn =aarch64_insn_get_adrp_value (); + offset = (addr -ALIGN_DOWN (pc,SZ_4K )) >>12 ; +break ; +default : +pr_err ("%s: unknown adr encoding %d\n" , __func__, type); +return AARCH64_BREAK_FAULT ; + } + +if (offset < -SZ_1M || offset >=SZ_1M ) +return AARCH64_BREAK_FAULT ; + + insn =aarch64_insn_encode_register (AARCH64_INSN_REGTYPE_RD , insn, reg); + +return aarch64_insn_encode_immediate (AARCH64_INSN_IMM_ADR , insn, offset); +} +/* * Decode the imm field of a branch, and return the byte offset as a * signed value (so it can be used when computing a new branch
Signature · 1 line
2.17.1
PATCH 2/2 arm64/module: switch to ADRP/ADD sequences for PLT entries
In reply to Ard BiesheuvelNow that we have switched to the small code model entirely, and reduced the extended KASLR range to 4 GB, we can be sure that the targets of relative branches that are out of range are in range for a ADRP/ADD pair, which is one instruction shorter than our current MOVN/MOVK/MOVK sequence, and is more idiomatic and so it is more likely to be implemented efficiently by micro-architectures.
So switch over the ordinary PLT code and the special handling of the Cortex-A53 ADRP errata, as well as the ftrace trampline handling.
--- arch/arm64/include/asm/module.h | 36 ++------ arch/arm64/kernel/ftrace.c | 2 +- arch/arm64/kernel/module-plts.c | 93 +++++++++++++++----- arch/arm64/kernel/module.c | 4 +- 4 files changed, 82 insertions(+), 53 deletions(-)
arch/arm64/include/asm/module.h +8 -28
@@ -56,39 +56,19 @@ struct plt_entry {* is exactly what we are dealing with here, we are free to use x16 * as a scratch register in the PLT veneers. */ -__le32 mov0;/* movn x16, #0x.... */ -__le32 mov1;/* movk x16, #0x...., lsl #16 */ -__le32 mov2;/* movk x16, #0x...., lsl #32 */ +__le32 adrp;/* adrp x16, .... */ +__le32 add;/* add x16, x16, #0x.... */ __le32 br; /* br x16 */ }; -static inline struct plt_entry get_plt_entry (u64 val) +static inline bool is_forbidden_offset_for_adrp (void *place) { -/* -* MOVK/MOVN/MOVZ opcode: -* +--------+------------+--------+-----------+-------------+---------+ -* | sf[31] | opc[30:29] | 100101 | hw[22:21] | imm16[20:5] | Rd[4:0] | -* +--------+------------+--------+-----------+-------------+---------+ -* -* Rd := 0x10 (x16) -* hw := 0b00 (no shift), 0b01 (lsl #16), 0b10 (lsl #32) -* opc := 0b11 (MOVK), 0b00 (MOVN), 0b10 (MOVZ) -* sf := 1 (64-bit variant) -*/ -return (struct plt_entry ){ -cpu_to_le32 (0x92800010 | (((~val ) &0xffff )) <<5 ), -cpu_to_le32 (0xf2a00010 | ((( val >>16 ) &0xffff )) <<5 ), -cpu_to_le32 (0xf2c00010 | ((( val >>32 ) &0xffff )) <<5 ), -cpu_to_le32 (0xd61f0200 ) - }; +return IS_ENABLED (CONFIG_ARM64_ERRATUM_843419 ) && +cpus_have_const_cap (ARM64_WORKAROUND_843419 ) && + ((u64 )place &0xfff ) >=0xff8 ; } -static inline bool plt_entries_equal (const struct plt_entry *a, -const struct plt_entry *b) -{ -return a->mov0 == b->mov0 && - a->mov1 == b->mov1 && - a->mov2 == b->mov2; -} +struct plt_entry get_plt_entry (u64 dst,void *pc); +bool plt_entries_equal (const struct plt_entry *a,const struct plt_entry *b);#endif /* __ASM_MODULE_H */
arch/arm64/kernel/ftrace.c +1 -1
@@ -104,7 +104,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)* is added in the future, but for now, the pr_err() below * deals with a theoretical issue only. */ - trampoline =get_plt_entry (addr); + trampoline =get_plt_entry (addr, mod->arch.ftrace_trampoline); if (!plt_entries_equal(mod->arch.ftrace_trampoline, &trampoline)) { if (!plt_entries_equal(mod->arch.ftrace_trampoline,
arch/arm64/kernel/module-plts.c +72 -21
138 lines not on this page · raw message
arch/arm64/kernel/module.c +1 -3
@@ -202,9 +202,7 @@ static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val) {u32 insn; -if (!IS_ENABLED (CONFIG_ARM64_ERRATUM_843419 ) || - !cpus_have_const_cap (ARM64_WORKAROUND_843419 ) || - ((u64 )place &0xfff ) <0xff8 ) +if (!is_forbidden_offset_for_adrp (place))return reloc_insn_imm (RELOC_OP_PAGE , place, val,12 ,21 ,AARCH64_INSN_IMM_ADR );
Signature · 1 line
2.17.1
PATCH 2/2 arm64/module: switch to ADRP/ADD sequences for PLT entries
Quoted 7 lines · Ard Biesheuvel
Now that we have switched to the small code model entirely, and reduced the extended KASLR range to 4 GB, we can be sure that the targets of relative branches that are out of range are in range for a ADRP/ADD pair, which is one instruction shorter than our current MOVN/MOVK/MOVK sequence, and is more idiomatic and so it is more likely to be implemented efficiently by micro-architectures.
Quoted 5 lines
So switch over the ordinary PLT code and the special handling of the Cortex-A53 ADRP errata, as well as the ftrace trampline handling.
Signed-off-by Ard Biesheuvel <[email protected]>
Generally, an ACK by me, but...
Quoted 10 lines
arch/arm64/kernel/module-plts.c +2
-0@@ -11,6 +11,55 @@#include <linux/module.h> #include <linux/sort.h> +static struct plt_entry __get_adrp_add_pair (u64 dst,u64 pc, +enum aarch64_insn_register reg)
arch/arm64/kernel/module-plts.c line 25
Quoted 10 lines
arch/arm64/kernel/module-plts.c +10
-0+{ +u32 adrp, add; + + adrp =aarch64_insn_gen_adr (pc, dst, reg,AARCH64_INSN_ADR_TYPE_ADRP ); + add =aarch64_insn_gen_add_sub_imm (reg, reg, dst %SZ_4K , +AARCH64_INSN_VARIANT_64BIT , +AARCH64_INSN_ADSB_ADD ); + +return (struct plt_entry ){cpu_to_le32 (adrp),cpu_to_le32 (add) }; +}
Will __get_adrp_add_pair get reused? Otherwise it would just be inlined below, but then again why is it returning a partial struct plt_entry?
arch/arm64/kernel/module-plts.c line 30
Quoted 4 lines
arch/arm64/kernel/module-plts.c +4
-0+struct plt_entry get_plt_entry (u64 dst,void *pc) +{ +struct plt_entry plt; +static u32 br;
Well, _I_ would call this variable insn_br_x16...
arch/arm64/kernel/module-plts.c line 40
Quoted 9 lines
arch/arm64/kernel/module-plts.c +9
-0+if (!br) + br =aarch64_insn_gen_branch_reg (AARCH64_INSN_REG_16 , +AARCH64_INSN_BRANCH_NOLINK ); + + plt =__get_adrp_add_pair (dst, (u64 )pc,AARCH64_INSN_REG_16 ); + plt.br =cpu_to_le32 (br); + +return plt; +}
But I'm really lost with this one:
Quoted 10 lines
arch/arm64/kernel/module-plts.c +10
-0+bool plt_entries_equal (const struct plt_entry *a,const struct plt_entry *b) +{ +u64 p, q; + +/* +* Check whether both entries refer to the same target: +* do the cheapest checks first. +*/ +if (a->add != b->add || a->br != b->br) +return false;
arch/arm64/kernel/module-plts.c line 61
Quoted 10 lines
arch/arm64/kernel/module-plts.c +10
-0+ + p =ALIGN_DOWN ((u64 )a,SZ_4K ); + q =ALIGN_DOWN ((u64 )b,SZ_4K ); + +if (a->adrp == b->adrp && p == q) +return true; + +return (p +aarch64_insn_adrp_get_offset (le32_to_cpu (a->adrp))) == + (q +aarch64_insn_adrp_get_offset (le32_to_cpu (b->adrp))); +}
IIUC addr/addrp are PC-relative? So in order to tell whether they lead to the same destination, their location (a and b) must _fully_ been taken into account, not just some bits?
Also, plt entries residing at different locations might address the same target, but (a->add != b->add || a->br != b->br) would yield true despite that. Is this intended?
Torsten
PATCH 2/2 arm64/module: switch to ADRP/ADD sequences for PLT entries
Quoted 25 lines · Torsten Duwe
Quoted 7 lines · Ard Biesheuvel
Now that we have switched to the small code model entirely, and reduced the extended KASLR range to 4 GB, we can be sure that the targets of relative branches that are out of range are in range for a ADRP/ADD pair, which is one instruction shorter than our current MOVN/MOVK/MOVK sequence, and is more idiomatic and so it is more likely to be implemented efficiently by micro-architectures.
Quoted 5 lines
So switch over the ordinary PLT code and the special handling of the Cortex-A53 ADRP errata, as well as the ftrace trampline handling.
Signed-off-by Ard Biesheuvel <[email protected]>Generally, an ACK by me, but...
Quoted 8 lines
arch/arm64/kernel/module-plts.c +0
-0@@ -11,6 +11,55 @@#include <linux/module.h> #include <linux/sort.h>
Quoted 16 lines
arch/arm64/kernel/module-plts.c line 25
Quoted 12 lines
arch/arm64/kernel/module-plts.c +12
-0+static struct plt_entry __get_adrp_add_pair (u64 dst,u64 pc, +enum aarch64_insn_register reg) +{ +u32 adrp, add; + + adrp =aarch64_insn_gen_adr (pc, dst, reg,AARCH64_INSN_ADR_TYPE_ADRP ); + add =aarch64_insn_gen_add_sub_imm (reg, reg, dst %SZ_4K , +AARCH64_INSN_VARIANT_64BIT , +AARCH64_INSN_ADSB_ADD ); + +return (struct plt_entry ){cpu_to_le32 (adrp),cpu_to_le32 (add) }; +}Will
__get_adrp_add_pairget reused? Otherwise it would just be inlined below, but then again why is it returning a partial structplt_entry?
Because it is used in two places: get_plt_entry() and module_emit_veneer_for_adrp()
Quoted 35 lines
arch/arm64/kernel/module-plts.c line 30
Quoted 4 lines
arch/arm64/kernel/module-plts.c +4
-0+struct plt_entry get_plt_entry (u64 dst,void *pc) +{ +struct plt_entry plt; +static u32 br;Well, _I_ would call this variable
insn_br_x16...arch/arm64/kernel/module-plts.c line 40
Quoted 9 lines
arch/arm64/kernel/module-plts.c +9
-0+if (!br) + br =aarch64_insn_gen_branch_reg (AARCH64_INSN_REG_16 , +AARCH64_INSN_BRANCH_NOLINK ); + + plt =__get_adrp_add_pair (dst, (u64 )pc,AARCH64_INSN_REG_16 ); + plt.br =cpu_to_le32 (br); + +return plt; +}But I'm really lost with this one:
Quoted 16 lines
arch/arm64/kernel/module-plts.c +16
-0+bool plt_entries_equal (const struct plt_entry *a,const struct plt_entry *b) +{ +u64 p, q; + +/* +* Check whether both entries refer to the same target: +* do the cheapest checks first. +*/ +if (a->add != b->add || a->br != b->br) +return false; + + p =ALIGN_DOWN ((u64 )a,SZ_4K ); + q =ALIGN_DOWN ((u64 )b,SZ_4K ); + +if (a->adrp == b->adrp && p == q) +return true;
Quoted 9 lines
arch/arm64/kernel/module-plts.c line 61
Quoted 4 lines
arch/arm64/kernel/module-plts.c +4
-0+ +return (p +aarch64_insn_adrp_get_offset (le32_to_cpu (a->adrp))) == + (q +aarch64_insn_adrp_get_offset (le32_to_cpu (b->adrp))); +}IIUC addr/addrp are PC-relative? So in order to tell whether they lead to the same destination, their location (a and b) must _fully_ been taken into account, not just some bits?
The criterion is whether they point to the same target.
So the reasoning is as follows:
- if the 'add' or 'br' opcodes are different, they are definitely not equal
- if the 'add' and 'br' opcodes are the same, the 'adrp' opcodes are the same, and the adrp instructions reside in the same 4 KB sized/4 KB aligned window, they must point to the same symbol
- otherwise, decode the instructions to see if they point to the same symbol. Note that we already checked the 'add's so no need to check them again.
Quoted 4 lines
Also, plt entries residing at different locations might address the same target, but (
a->add!=b->add||a->br!=b->br) would yield true despite that. Is this intended?
If they address the same target, the add will be the same. The br also has to be the same because we cannot reuse an ordinary PLT as and ADRP veneer or vice versa.
PATCH 2/2 arm64/module: switch to ADRP/ADD sequences for PLT entries
Quoted 21 lines · Ard Biesheuvel
Quoted 20 lines · Torsten Duwe
On Thu, Nov 22, 2018 at 09:46:46AM +0100, Ard Biesheuvel wrote:
But I'm really lost with this one:
Quoted 16 lines
arch/arm64/kernel/module-plts.c +16
-0+bool plt_entries_equal (const struct plt_entry *a,const struct plt_entry *b) +{ +u64 p, q; + +/* +* Check whether both entries refer to the same target: +* do the cheapest checks first. +*/ +if (a->add != b->add || a->br != b->br) +return false; + + p =ALIGN_DOWN ((u64 )a,SZ_4K ); + q =ALIGN_DOWN ((u64 )b,SZ_4K ); + +if (a->adrp == b->adrp && p == q) +return true;
Quoted 8 lines
Quoted 8 lines
arch/arm64/kernel/module-plts.c line 61
Quoted 4 lines
arch/arm64/kernel/module-plts.c +4
-0+ +return (p +aarch64_insn_adrp_get_offset (le32_to_cpu (a->adrp))) == + (q +aarch64_insn_adrp_get_offset (le32_to_cpu (b->adrp))); +}IIUC addr/addrp are PC-relative? So in order to tell whether they lead to the same destination, their location (a and b) must _fully_ been taken into account, not just some bits?
Ok, only the % 4k part of the PC is taken into account for adrp, I learned; well then.
Quoted 12 lines
The criterion is whether they point to the same target.
So the reasoning is as follows:
- if the 'add' or 'br' opcodes are different, they are definitely not equal
- if the 'add' and 'br' opcodes are the same, the 'adrp' opcodes are the same, and the adrp instructions reside in the same 4 KB sized/4 KB aligned window, they must point to the same symbol
- otherwise, decode the instructions to see if they point to the same symbol. Note that we already checked the 'add's so no need to check them again.
Quoted 8 lines
Quoted 4 lines
Also, plt entries residing at different locations might address the same target, but (
a->add!=b->add||a->br!=b->br) would yield true despite that. Is this intended?If they address the same target, the add will be the same. The br also has to be the same because we cannot reuse an ordinary PLT as and ADRP veneer or vice versa.
Ah, _that's_ the purpose! Could you please clarify it like
/* make sure we're comparing equally typed veneers (or quote the above) */ if (a->br != b->br)
return false; /* different offsets into the page can never lead to equal dest */ if (a->add != b->add) return false;
/* it remains to compare the destination pages */ ...
Seems like this is per se difficult territory, see erratum#843419 ;-)
Torsten
cover use adrp/add pairs for PLT entries
In reply to Ard BiesheuvelHi Ard,
Quoted 9 lines · Ard Biesheuvel
Currently, PLT entries use a non-idiomatic movn/movz/movz/br instruction sequence which is also longer than necessary. Also, the code emitting them does not use the instruction generation code but open codes the opcodes directly.
The extended KASLR range is now 4 GB, given that we switched to the small code model everywhere else (including for modules), so we can switch to adrp/add/br sequences which are easier in the I-cache.
Quoted 7 lines
So implement adrp/add pair generation in the instruction generation code and wire it up into the PLT code. Note that the Cortex-A53 errata handling requires some special care to ensure that generated veneers are not susceptible to the erratum.
Cc Torsten Duwe <[email protected]>Cc Jessica Yu <[email protected]>
I've applied this, with a couple of extra comments in the plt comparison code and the Reviewed-by from Torsten. There were some trivial conflicts with Jessica's rework of the plt lookup, but I think I got it right. Please take a look at for-next/core when you get a chance.
Will
cover use adrp/add pairs for PLT entries
Quoted 18 lines · Will Deacon
Hi Ard,
Quoted 14 lines · Ard Biesheuvel
Currently, PLT entries use a non-idiomatic movn/movz/movz/br instruction sequence which is also longer than necessary. Also, the code emitting them does not use the instruction generation code but open codes the opcodes directly.
The extended KASLR range is now 4 GB, given that we switched to the small code model everywhere else (including for modules), so we can switch to adrp/add/br sequences which are easier in the I-cache.
So implement adrp/add pair generation in the instruction generation code and wire it up into the PLT code. Note that the Cortex-A53 errata handling requires some special care to ensure that generated veneers are not susceptible to the erratum.
Quoted 8 lines
Quoted 2 lines
Cc Torsten Duwe <[email protected]>Cc Jessica Yu <[email protected]>I've applied this, with a couple of extra comments in the plt comparison code and the Reviewed-by from Torsten. There were some trivial conflicts with Jessica's rework of the plt lookup, but I think I got it right. Please take a look at for-next/core when you get a chance.
Looks fine to me.