| Message ID | 20180306171535.25681-6-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 06, 2018 at 05:15:35PM +0000, Ard Biesheuvel wrote: > Omit patching of ADRP instruction at module load time if the current > CPUs are not susceptible to the erratum. [...] > diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c > index 534bf1d47119..1a583ccace00 100644 > --- a/arch/arm64/kernel/module-plts.c > +++ b/arch/arm64/kernel/module-plts.c > @@ -158,7 +158,8 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num, > break; > case R_AARCH64_ADR_PREL_PG_HI21_NC: > case R_AARCH64_ADR_PREL_PG_HI21: > - if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419)) > + if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || > + !cpus_have_const_cap(ARM64_WORKAROUND_843419)) > break; > > /* > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > index 89217704944e..47b40aaa1a5d 100644 > --- a/arch/arm64/kernel/module.c > +++ b/arch/arm64/kernel/module.c > @@ -201,6 +201,7 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val, > static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val) > { > if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || > + !cpus_have_const_cap(ARM64_WORKAROUND_843419) || Mind if I drop the IS_ENABLED check here and in the hunk above? The const_cap check along should be sufficient, no? Will
On 8 March 2018 at 13:45, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Mar 06, 2018 at 05:15:35PM +0000, Ard Biesheuvel wrote: >> Omit patching of ADRP instruction at module load time if the current >> CPUs are not susceptible to the erratum. > > [...] > >> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c >> index 534bf1d47119..1a583ccace00 100644 >> --- a/arch/arm64/kernel/module-plts.c >> +++ b/arch/arm64/kernel/module-plts.c >> @@ -158,7 +158,8 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num, >> break; >> case R_AARCH64_ADR_PREL_PG_HI21_NC: >> case R_AARCH64_ADR_PREL_PG_HI21: >> - if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419)) >> + if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || >> + !cpus_have_const_cap(ARM64_WORKAROUND_843419)) >> break; >> >> /* >> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c >> index 89217704944e..47b40aaa1a5d 100644 >> --- a/arch/arm64/kernel/module.c >> +++ b/arch/arm64/kernel/module.c >> @@ -201,6 +201,7 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val, >> static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val) >> { >> if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || >> + !cpus_have_const_cap(ARM64_WORKAROUND_843419) || > > Mind if I drop the IS_ENABLED check here and in the hunk above? The > const_cap check along should be sufficient, no? > Without the IS_ENABLED() check, the code will always be present in the object file. I have no strong preference either way, though.
On 8 March 2018 at 13:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 8 March 2018 at 13:45, Will Deacon <will.deacon@arm.com> wrote: >> On Tue, Mar 06, 2018 at 05:15:35PM +0000, Ard Biesheuvel wrote: >>> Omit patching of ADRP instruction at module load time if the current >>> CPUs are not susceptible to the erratum. >> >> [...] >> >>> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c >>> index 534bf1d47119..1a583ccace00 100644 >>> --- a/arch/arm64/kernel/module-plts.c >>> +++ b/arch/arm64/kernel/module-plts.c >>> @@ -158,7 +158,8 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num, >>> break; >>> case R_AARCH64_ADR_PREL_PG_HI21_NC: >>> case R_AARCH64_ADR_PREL_PG_HI21: >>> - if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419)) >>> + if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || >>> + !cpus_have_const_cap(ARM64_WORKAROUND_843419)) >>> break; >>> >>> /* >>> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c >>> index 89217704944e..47b40aaa1a5d 100644 >>> --- a/arch/arm64/kernel/module.c >>> +++ b/arch/arm64/kernel/module.c >>> @@ -201,6 +201,7 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val, >>> static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val) >>> { >>> if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || >>> + !cpus_have_const_cap(ARM64_WORKAROUND_843419) || >> >> Mind if I drop the IS_ENABLED check here and in the hunk above? The >> const_cap check along should be sufficient, no? >> > > Without the IS_ENABLED() check, the code will always be present in the > object file. > > I have no strong preference either way, though. Ehm, strike that. You will probably hit a linker error if you drop it, because some dependent functions are #ifdef'ed out if the erratum workaround is disabled.
On Thu, Mar 08, 2018 at 01:47:26PM +0000, Ard Biesheuvel wrote: > On 8 March 2018 at 13:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 8 March 2018 at 13:45, Will Deacon <will.deacon@arm.com> wrote: > >> On Tue, Mar 06, 2018 at 05:15:35PM +0000, Ard Biesheuvel wrote: > >>> Omit patching of ADRP instruction at module load time if the current > >>> CPUs are not susceptible to the erratum. > >> > >> [...] > >> > >>> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c > >>> index 534bf1d47119..1a583ccace00 100644 > >>> --- a/arch/arm64/kernel/module-plts.c > >>> +++ b/arch/arm64/kernel/module-plts.c > >>> @@ -158,7 +158,8 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num, > >>> break; > >>> case R_AARCH64_ADR_PREL_PG_HI21_NC: > >>> case R_AARCH64_ADR_PREL_PG_HI21: > >>> - if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419)) > >>> + if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || > >>> + !cpus_have_const_cap(ARM64_WORKAROUND_843419)) > >>> break; > >>> > >>> /* > >>> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > >>> index 89217704944e..47b40aaa1a5d 100644 > >>> --- a/arch/arm64/kernel/module.c > >>> +++ b/arch/arm64/kernel/module.c > >>> @@ -201,6 +201,7 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val, > >>> static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val) > >>> { > >>> if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || > >>> + !cpus_have_const_cap(ARM64_WORKAROUND_843419) || > >> > >> Mind if I drop the IS_ENABLED check here and in the hunk above? The > >> const_cap check along should be sufficient, no? > >> > > > > Without the IS_ENABLED() check, the code will always be present in the > > object file. > > > > I have no strong preference either way, though. > > Ehm, strike that. You will probably hit a linker error if you drop it, > because some dependent functions are #ifdef'ed out if the erratum > workaround is disabled. Ah, ok then. Thanks for the quick response. Will
On Thu, Mar 08, 2018 at 01:46:34PM +0000, Ard Biesheuvel wrote: > On 8 March 2018 at 13:45, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, Mar 06, 2018 at 05:15:35PM +0000, Ard Biesheuvel wrote: > >> Omit patching of ADRP instruction at module load time if the current > >> CPUs are not susceptible to the erratum. > > > > [...] > > > >> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c > >> index 534bf1d47119..1a583ccace00 100644 > >> --- a/arch/arm64/kernel/module-plts.c > >> +++ b/arch/arm64/kernel/module-plts.c > >> @@ -158,7 +158,8 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num, > >> break; > >> case R_AARCH64_ADR_PREL_PG_HI21_NC: > >> case R_AARCH64_ADR_PREL_PG_HI21: > >> - if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419)) > >> + if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || > >> + !cpus_have_const_cap(ARM64_WORKAROUND_843419)) > >> break; > >> > >> /* > >> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > >> index 89217704944e..47b40aaa1a5d 100644 > >> --- a/arch/arm64/kernel/module.c > >> +++ b/arch/arm64/kernel/module.c > >> @@ -201,6 +201,7 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val, > >> static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val) > >> { > >> if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || > >> + !cpus_have_const_cap(ARM64_WORKAROUND_843419) || > > > > Mind if I drop the IS_ENABLED check here and in the hunk above? The > > const_cap check along should be sufficient, no? > > > > Without the IS_ENABLED() check, the code will always be present in the > object file. > > I have no strong preference either way, though. As with other case, perhaps fold this into a helper in <asm/cpufeature.h> ? static inline bool system_needs_arm64_workaround_843419() { return (IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) && cpus_have_const_cap(ARM64_WORKAROUND_843419)) } ... then use the inverse in the cases above. Thanks, Mark.
On 8 March 2018 at 13:49, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Mar 08, 2018 at 01:46:34PM +0000, Ard Biesheuvel wrote: >> On 8 March 2018 at 13:45, Will Deacon <will.deacon@arm.com> wrote: >> > On Tue, Mar 06, 2018 at 05:15:35PM +0000, Ard Biesheuvel wrote: >> >> Omit patching of ADRP instruction at module load time if the current >> >> CPUs are not susceptible to the erratum. >> > >> > [...] >> > >> >> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c >> >> index 534bf1d47119..1a583ccace00 100644 >> >> --- a/arch/arm64/kernel/module-plts.c >> >> +++ b/arch/arm64/kernel/module-plts.c >> >> @@ -158,7 +158,8 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num, >> >> break; >> >> case R_AARCH64_ADR_PREL_PG_HI21_NC: >> >> case R_AARCH64_ADR_PREL_PG_HI21: >> >> - if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419)) >> >> + if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || >> >> + !cpus_have_const_cap(ARM64_WORKAROUND_843419)) >> >> break; >> >> >> >> /* >> >> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c >> >> index 89217704944e..47b40aaa1a5d 100644 >> >> --- a/arch/arm64/kernel/module.c >> >> +++ b/arch/arm64/kernel/module.c >> >> @@ -201,6 +201,7 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val, >> >> static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val) >> >> { >> >> if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || >> >> + !cpus_have_const_cap(ARM64_WORKAROUND_843419) || >> > >> > Mind if I drop the IS_ENABLED check here and in the hunk above? The >> > const_cap check along should be sufficient, no? >> > >> >> Without the IS_ENABLED() check, the code will always be present in the >> object file. >> >> I have no strong preference either way, though. > > As with other case, perhaps fold this into a helper in > <asm/cpufeature.h> ? > > static inline bool system_needs_arm64_workaround_843419() > { > return (IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) && > cpus_have_const_cap(ARM64_WORKAROUND_843419)) > } > > ... then use the inverse in the cases above. > I'm fine with adding a helper, but 'system_needs_arm64_workaround_843419' is a bit misleading, given that it returns false if the system needs it but support is compiled out.
On Thu, Mar 08, 2018 at 01:54:26PM +0000, Ard Biesheuvel wrote: > On 8 March 2018 at 13:49, Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, Mar 08, 2018 at 01:46:34PM +0000, Ard Biesheuvel wrote: > >> On 8 March 2018 at 13:45, Will Deacon <will.deacon@arm.com> wrote: > >> > On Tue, Mar 06, 2018 at 05:15:35PM +0000, Ard Biesheuvel wrote: > >> >> Omit patching of ADRP instruction at module load time if the current > >> >> CPUs are not susceptible to the erratum. > >> > > >> > [...] > >> > > >> >> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c > >> >> index 534bf1d47119..1a583ccace00 100644 > >> >> --- a/arch/arm64/kernel/module-plts.c > >> >> +++ b/arch/arm64/kernel/module-plts.c > >> >> @@ -158,7 +158,8 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num, > >> >> break; > >> >> case R_AARCH64_ADR_PREL_PG_HI21_NC: > >> >> case R_AARCH64_ADR_PREL_PG_HI21: > >> >> - if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419)) > >> >> + if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || > >> >> + !cpus_have_const_cap(ARM64_WORKAROUND_843419)) > >> >> break; > >> >> > >> >> /* > >> >> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > >> >> index 89217704944e..47b40aaa1a5d 100644 > >> >> --- a/arch/arm64/kernel/module.c > >> >> +++ b/arch/arm64/kernel/module.c > >> >> @@ -201,6 +201,7 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val, > >> >> static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val) > >> >> { > >> >> if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || > >> >> + !cpus_have_const_cap(ARM64_WORKAROUND_843419) || > >> > > >> > Mind if I drop the IS_ENABLED check here and in the hunk above? The > >> > const_cap check along should be sufficient, no? > >> > > >> > >> Without the IS_ENABLED() check, the code will always be present in the > >> object file. > >> > >> I have no strong preference either way, though. > > > > As with other case, perhaps fold this into a helper in > > <asm/cpufeature.h> ? > > > > static inline bool system_needs_arm64_workaround_843419() > > { > > return (IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) && > > cpus_have_const_cap(ARM64_WORKAROUND_843419)) > > } > > > > ... then use the inverse in the cases above. > > > > I'm fine with adding a helper, but > 'system_needs_arm64_workaround_843419' is a bit misleading, given that > it returns false if the system needs it but support is compiled out. FWIW, I'm fine with the code as it is. Will
diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h index bb263820de13..39134c46bb13 100644 --- a/arch/arm64/include/asm/cpucaps.h +++ b/arch/arm64/include/asm/cpucaps.h @@ -45,7 +45,8 @@ #define ARM64_HARDEN_BRANCH_PREDICTOR 24 #define ARM64_HARDEN_BP_POST_GUEST_EXIT 25 #define ARM64_HAS_RAS_EXTN 26 +#define ARM64_WORKAROUND_843419 27 -#define ARM64_NCAPS 27 +#define ARM64_NCAPS 28 #endif /* __ASM_CPUCAPS_H */ diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c index b161abdd6e27..e18023e5deb9 100644 --- a/arch/arm64/kernel/cpu_errata.c +++ b/arch/arm64/kernel/cpu_errata.c @@ -298,6 +298,16 @@ const struct arm64_cpu_capabilities arm64_errata[] = { MIDR_CPU_VAR_REV(1, 2)), }, #endif +#ifdef CONFIG_ARM64_ERRATUM_843419 + { + /* Cortex-A53 r0p[01234] */ + .desc = "ARM erratum 843419", + .capability = ARM64_WORKAROUND_843419, + .def_scope = SCOPE_LOCAL_CPU, + MIDR_RANGE(MIDR_CORTEX_A53, 0x00, 0x04), + MIDR_FIXED(0x4, BIT(8)), + }, +#endif #ifdef CONFIG_ARM64_ERRATUM_845719 { /* Cortex-A53 r0p[01234] */ diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c index 534bf1d47119..1a583ccace00 100644 --- a/arch/arm64/kernel/module-plts.c +++ b/arch/arm64/kernel/module-plts.c @@ -158,7 +158,8 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num, break; case R_AARCH64_ADR_PREL_PG_HI21_NC: case R_AARCH64_ADR_PREL_PG_HI21: - if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419)) + if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || + !cpus_have_const_cap(ARM64_WORKAROUND_843419)) break; /* diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c index 89217704944e..47b40aaa1a5d 100644 --- a/arch/arm64/kernel/module.c +++ b/arch/arm64/kernel/module.c @@ -201,6 +201,7 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val, static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val) { if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_843419) || + !cpus_have_const_cap(ARM64_WORKAROUND_843419) || ((u64)place & 0xfff) < 0xff8) return reloc_insn_imm(RELOC_OP_PAGE, place, val, 12, 21, AARCH64_INSN_IMM_ADR);
Omit patching of ADRP instruction at module load time if the current CPUs are not susceptible to the erratum. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/include/asm/cpucaps.h | 3 ++- arch/arm64/kernel/cpu_errata.c | 10 ++++++++++ arch/arm64/kernel/module-plts.c | 3 ++- arch/arm64/kernel/module.c | 1 + 4 files changed, 15 insertions(+), 2 deletions(-)