| Message ID | 20221207180821.2479987-13-heiko@sntech.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Palmer Dabbelt |
| Headers | show |
| Series | Allow calls in alternatives | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/patch_count | success | Link |
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be fixes |
| conchuod/fixes_present | success | Fixes tag present in non-next series |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/alphanumeric_selects | success | Out of order selects before the patch: 45 and now 45 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/build_warn_rv64 | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | warning | CHECK: Alignment should match open parenthesis |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Wed, Dec 07, 2022 at 07:08:21PM +0100, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > Alternatives live in a different section, so addresses used by call > functions will point to wrong locations after the patch got applied. > > Similar to arm64, adjust the location to consider that offset. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > --- > arch/riscv/include/asm/alternative.h | 3 ++ > arch/riscv/kernel/alternative.c | 56 ++++++++++++++++++++++++++++ > arch/riscv/kernel/cpufeature.c | 5 ++- > 3 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h > index 6511dd73e812..1bd4027d34ca 100644 > --- a/arch/riscv/include/asm/alternative.h > +++ b/arch/riscv/include/asm/alternative.h > @@ -27,6 +27,9 @@ void __init apply_boot_alternatives(void); > void __init apply_early_boot_alternatives(void); > void apply_module_alternatives(void *start, size_t length); > > +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > + int patch_offset); > + > struct alt_entry { > void *old_ptr; /* address of original instruciton or data */ > void *alt_ptr; /* address of replacement instruction or data */ > diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c > index a7d26a00beea..c29e198ed9df 100644 > --- a/arch/riscv/kernel/alternative.c > +++ b/arch/riscv/kernel/alternative.c > @@ -15,6 +15,8 @@ > #include <asm/vendorid_list.h> > #include <asm/sbi.h> > #include <asm/csr.h> > +#include <asm/insn.h> > +#include <asm/patch.h> > > struct cpu_manufacturer_info_t { > unsigned long vendor_id; > @@ -53,6 +55,60 @@ static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_inf > } > } > > +static u32 riscv_instruction_at(void *p) > +{ > + u16 *parcel = p; > + > + return (unsigned int)parcel[0] | (unsigned int)parcel[1] << 16; I feel bad for not mentioning this before - can we replace this magic 16 with something self documenting? > +} > + > +static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 insn1, u32 insn2, int patch_offset) > +{ > + /* pick the original auipc + jalr */ > + u32 call[2] = { insn1, insn2 }; > + s32 imm; > + > + /* get and adjust new target address */ > + imm = riscv_insn_extract_utype_itype_imm(insn1, insn2); > + imm -= patch_offset; > + > + /* update instructions */ > + riscv_insn_insert_utype_itype_imm(call, imm); > + > + /* patch the call place again */ > + patch_text_nosync(ptr, call, sizeof(u32) * 2); > +} Obv. I have not left R-b tags on stuff without trying to understand what's going on, but from a lay-observer point of view, I think these function names & flow does a good job of explaining some of the black magic in this neck of the woods. > + > +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > + int patch_offset) > +{ > + int num_instr = len / sizeof(u32); instr... > + int i; > + > + /* > + * stop one instruction before the end, as we're checking > + * for auipc + jalr > + */ > + for (i = 0; i < num_instr; i++) { > + u32 inst = riscv_instruction_at(alt_ptr + i * sizeof(u32)); ...inst... > + > + /* may be the start of an auipc + jalr pair */ > + if (riscv_insn_is_auipc(inst) && i < num_instr - 1) { ...insn. Is there a reason for that? Also, I've gotten myself slightly confused about the loop. You "stop one instruction before the end" but the main loop goes from 0 -> num_instr. The inner loop then checks for i < num_instr - 1. What am I missing that prevents the outer loop from stopping at num_instr - 1 instead? > + u32 inst2 = riscv_instruction_at(alt_ptr + (i + 1) * sizeof(u32)); > + > + if (!riscv_insn_is_jalr(inst2)) > + continue; > + > + /* call will use ra register */ Super minor, but "call will use ra register" is a little unclear. As written, it makes perfect sense when you've been staring at this code, but not so much if you're passing through.. How about: /* if this instruction pair is a call, it will use the ra register */ > + if (RV_EXTRACT_RD_REG(inst) != 1) > + continue; > All minor stuff though, so you can re-add my R-b either way: Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Conor. > + riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32), > + inst, inst2, patch_offset); > + } > + } > +} > + > /* > * This is called very early in the boot process (directly after we run > * a feature detect on the boot CPU). No need to worry about other CPUs > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 694267d1fe81..e91ec3d8e240 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -316,8 +316,11 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, > } > > tmp = (1U << alt->errata_id); > - if (cpu_req_feature & tmp) > + if (cpu_req_feature & tmp) { > patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len); > + riscv_alternative_fix_offsets(alt->old_ptr, alt->alt_len, > + alt->old_ptr - alt->alt_ptr); > + } > } > } > #endif > -- > 2.35.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On 7 Dec 2022, at 20:48, Conor Dooley <conor@kernel.org> wrote: > > On Wed, Dec 07, 2022 at 07:08:21PM +0100, Heiko Stuebner wrote: >> From: Heiko Stuebner <heiko.stuebner@vrull.eu> >> >> Alternatives live in a different section, so addresses used by call >> functions will point to wrong locations after the patch got applied. >> >> Similar to arm64, adjust the location to consider that offset. >> >> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> >> --- >> arch/riscv/include/asm/alternative.h | 3 ++ >> arch/riscv/kernel/alternative.c | 56 ++++++++++++++++++++++++++++ >> arch/riscv/kernel/cpufeature.c | 5 ++- >> 3 files changed, 63 insertions(+), 1 deletion(-) >> >> diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h >> index 6511dd73e812..1bd4027d34ca 100644 >> --- a/arch/riscv/include/asm/alternative.h >> +++ b/arch/riscv/include/asm/alternative.h >> @@ -27,6 +27,9 @@ void __init apply_boot_alternatives(void); >> void __init apply_early_boot_alternatives(void); >> void apply_module_alternatives(void *start, size_t length); >> >> +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, >> + int patch_offset); >> + >> struct alt_entry { >> void *old_ptr; /* address of original instruciton or data */ >> void *alt_ptr; /* address of replacement instruction or data */ >> diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c >> index a7d26a00beea..c29e198ed9df 100644 >> --- a/arch/riscv/kernel/alternative.c >> +++ b/arch/riscv/kernel/alternative.c >> @@ -15,6 +15,8 @@ >> #include <asm/vendorid_list.h> >> #include <asm/sbi.h> >> #include <asm/csr.h> >> +#include <asm/insn.h> >> +#include <asm/patch.h> >> >> struct cpu_manufacturer_info_t { >> unsigned long vendor_id; >> @@ -53,6 +55,60 @@ static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_inf >> } >> } >> >> +static u32 riscv_instruction_at(void *p) >> +{ >> + u16 *parcel = p; >> + >> + return (unsigned int)parcel[0] | (unsigned int)parcel[1] << 16; > > I feel bad for not mentioning this before - can we replace this magic 16 > with something self documenting? I mean, it’s a u16 * two lines above, that seems a bit unnecessary... I'd argue giving it a name would be worse than not. Though these unsigned ints should be u32 to match the return type. Jess >> +} >> + >> +static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 insn1, u32 insn2, int patch_offset) >> +{ >> + /* pick the original auipc + jalr */ >> + u32 call[2] = { insn1, insn2 }; >> + s32 imm; >> + >> + /* get and adjust new target address */ >> + imm = riscv_insn_extract_utype_itype_imm(insn1, insn2); >> + imm -= patch_offset; >> + >> + /* update instructions */ >> + riscv_insn_insert_utype_itype_imm(call, imm); >> + >> + /* patch the call place again */ >> + patch_text_nosync(ptr, call, sizeof(u32) * 2); >> +} > > Obv. I have not left R-b tags on stuff without trying to understand > what's going on, but from a lay-observer point of view, I think these > function names & flow does a good job of explaining some of the black > magic in this neck of the woods. > >> + >> +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, >> + int patch_offset) >> +{ >> + int num_instr = len / sizeof(u32); > > instr... > >> + int i; >> + >> + /* >> + * stop one instruction before the end, as we're checking >> + * for auipc + jalr >> + */ >> + for (i = 0; i < num_instr; i++) { >> + u32 inst = riscv_instruction_at(alt_ptr + i * sizeof(u32)); > > ...inst... > >> + >> + /* may be the start of an auipc + jalr pair */ >> + if (riscv_insn_is_auipc(inst) && i < num_instr - 1) { > > ...insn. > > Is there a reason for that? > > Also, I've gotten myself slightly confused about the loop. You "stop one > instruction before the end" but the main loop goes from 0 -> num_instr. > The inner loop then checks for i < num_instr - 1. What am I missing that > prevents the outer loop from stopping at num_instr - 1 instead? >> + u32 inst2 = riscv_instruction_at(alt_ptr + (i + 1) * sizeof(u32)); >> + >> + if (!riscv_insn_is_jalr(inst2)) >> + continue; >> + >> + /* call will use ra register */ > > Super minor, but "call will use ra register" is a little unclear. As > written, it makes perfect sense when you've been staring at this code, > but not so much if you're passing through.. How about: > /* if this instruction pair is a call, it will use the ra register */ > >> + if (RV_EXTRACT_RD_REG(inst) != 1) >> + continue; >> > > All minor stuff though, so you can re-add my R-b either way: > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > Thanks, > Conor. > >> + riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32), >> + inst, inst2, patch_offset); >> + } >> + } >> +} >> + >> /* >> * This is called very early in the boot process (directly after we run >> * a feature detect on the boot CPU). No need to worry about other CPUs >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> index 694267d1fe81..e91ec3d8e240 100644 >> --- a/arch/riscv/kernel/cpufeature.c >> +++ b/arch/riscv/kernel/cpufeature.c >> @@ -316,8 +316,11 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, >> } >> >> tmp = (1U << alt->errata_id); >> - if (cpu_req_feature & tmp) >> + if (cpu_req_feature & tmp) { >> patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len); >> + riscv_alternative_fix_offsets(alt->old_ptr, alt->alt_len, >> + alt->old_ptr - alt->alt_ptr); >> + } >> } >> } >> #endif >> -- >> 2.35.1 >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Dec 07, 2022 at 10:00:13PM +0000, Jessica Clarke wrote: > >> +static u32 riscv_instruction_at(void *p) > >> +{ > >> + u16 *parcel = p; > >> + > >> + return (unsigned int)parcel[0] | (unsigned int)parcel[1] << 16; > > > > I feel bad for not mentioning this before - can we replace this magic 16 > > with something self documenting? > > I mean, it’s a u16 * two lines above, that seems a bit unnecessary... I guess that's why I didn't mention it before... Can safely disregard that request so Heiko!
Am Mittwoch, 7. Dezember 2022, 21:48:08 CET schrieb Conor Dooley: > On Wed, Dec 07, 2022 at 07:08:21PM +0100, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > Alternatives live in a different section, so addresses used by call > > functions will point to wrong locations after the patch got applied. > > > > Similar to arm64, adjust the location to consider that offset. > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > > --- > > arch/riscv/include/asm/alternative.h | 3 ++ > > arch/riscv/kernel/alternative.c | 56 ++++++++++++++++++++++++++++ > > arch/riscv/kernel/cpufeature.c | 5 ++- > > 3 files changed, 63 insertions(+), 1 deletion(-) > > > > diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h > > index 6511dd73e812..1bd4027d34ca 100644 > > --- a/arch/riscv/include/asm/alternative.h > > +++ b/arch/riscv/include/asm/alternative.h > > @@ -27,6 +27,9 @@ void __init apply_boot_alternatives(void); > > void __init apply_early_boot_alternatives(void); > > void apply_module_alternatives(void *start, size_t length); > > > > +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > > + int patch_offset); > > + > > struct alt_entry { > > void *old_ptr; /* address of original instruciton or data */ > > void *alt_ptr; /* address of replacement instruction or data */ > > diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c > > index a7d26a00beea..c29e198ed9df 100644 > > --- a/arch/riscv/kernel/alternative.c > > +++ b/arch/riscv/kernel/alternative.c > > @@ -15,6 +15,8 @@ > > #include <asm/vendorid_list.h> > > #include <asm/sbi.h> > > #include <asm/csr.h> > > +#include <asm/insn.h> > > +#include <asm/patch.h> > > > > struct cpu_manufacturer_info_t { > > unsigned long vendor_id; > > @@ -53,6 +55,60 @@ static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_inf > > } > > } > > > > +static u32 riscv_instruction_at(void *p) > > +{ > > + u16 *parcel = p; > > + > > + return (unsigned int)parcel[0] | (unsigned int)parcel[1] << 16; > > I feel bad for not mentioning this before - can we replace this magic 16 > with something self documenting? > > > +} > > + > > +static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 insn1, u32 insn2, int patch_offset) > > +{ > > + /* pick the original auipc + jalr */ > > + u32 call[2] = { insn1, insn2 }; > > + s32 imm; > > + > > + /* get and adjust new target address */ > > + imm = riscv_insn_extract_utype_itype_imm(insn1, insn2); > > + imm -= patch_offset; > > + > > + /* update instructions */ > > + riscv_insn_insert_utype_itype_imm(call, imm); > > + > > + /* patch the call place again */ > > + patch_text_nosync(ptr, call, sizeof(u32) * 2); > > +} > > Obv. I have not left R-b tags on stuff without trying to understand > what's going on, but from a lay-observer point of view, I think these > function names & flow does a good job of explaining some of the black > magic in this neck of the woods. > > > + > > +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > > + int patch_offset) > > +{ > > + int num_instr = len / sizeof(u32); > > instr... > > > + int i; > > + > > + /* > > + * stop one instruction before the end, as we're checking > > + * for auipc + jalr > > + */ > > + for (i = 0; i < num_instr; i++) { > > + u32 inst = riscv_instruction_at(alt_ptr + i * sizeof(u32)); > > ...inst... > > > + > > + /* may be the start of an auipc + jalr pair */ > > + if (riscv_insn_is_auipc(inst) && i < num_instr - 1) { > > ...insn. > > Is there a reason for that? I guess more of a generational issue - with the code spanning too much time :-) So poll question, what would be preferred? I think I remember seeing all of them somewhere, so I'm unsure what to standardize on. > > Also, I've gotten myself slightly confused about the loop. You "stop one > instruction before the end" but the main loop goes from 0 -> num_instr. > The inner loop then checks for i < num_instr - 1. What am I missing that > prevents the outer loop from stopping at num_instr - 1 instead? The idea with this is to allow a if(riscv_insn_is_jal(inst)) riscv_alternative_fix_jal(...) etc, and everything else is a single instruction so needs one more loop iteration, only for auipc+jalr do we want to stop one earlier. So to get this alternatives_fix_offsets() main entry point I made the loop do the one iteration more again. > > + u32 inst2 = riscv_instruction_at(alt_ptr + (i + 1) * sizeof(u32)); > > + > > + if (!riscv_insn_is_jalr(inst2)) > > + continue; > > + > > + /* call will use ra register */ > > Super minor, but "call will use ra register" is a little unclear. As > written, it makes perfect sense when you've been staring at this code, > but not so much if you're passing through.. How about: > /* if this instruction pair is a call, it will use the ra register */ sure :-) > > > + if (RV_EXTRACT_RD_REG(inst) != 1) > > + continue; > > > > All minor stuff though, so you can re-add my R-b either way: > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> thanks :-) Heiko
On 07/12/2022 22:37, Heiko Stuebner wrote: > Am Mittwoch, 7. Dezember 2022, 21:48:08 CET schrieb Conor Dooley: >> On Wed, Dec 07, 2022 at 07:08:21PM +0100, Heiko Stuebner wrote: >>> From: Heiko Stuebner <heiko.stuebner@vrull.eu> >>> +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, >>> + int patch_offset) >>> +{ >>> + int num_instr = len / sizeof(u32); >> >> instr... >> >>> + int i; >>> + >>> + /* >>> + * stop one instruction before the end, as we're checking >>> + * for auipc + jalr >>> + */ >>> + for (i = 0; i < num_instr; i++) { >>> + u32 inst = riscv_instruction_at(alt_ptr + i * sizeof(u32)); >> >> ...inst... >> >>> + >>> + /* may be the start of an auipc + jalr pair */ >>> + if (riscv_insn_is_auipc(inst) && i < num_instr - 1) { >> >> ...insn. >> >> Is there a reason for that? > > I guess more of a generational issue - with the code spanning > too much time :-) > > So poll question, what would be preferred? > I think I remember seeing all of them somewhere, so I'm unsure what > to standardize on. I'd vote for insn, since it's used in the functions you're calling. >> Also, I've gotten myself slightly confused about the loop. You "stop one >> instruction before the end" but the main loop goes from 0 -> num_instr. >> The inner loop then checks for i < num_instr - 1. What am I missing that >> prevents the outer loop from stopping at num_instr - 1 instead? > > The idea with this is to allow a > if(riscv_insn_is_jal(inst)) > riscv_alternative_fix_jal(...) > > etc, and everything else is a single instruction so needs one more > loop iteration, only for auipc+jalr do we want to stop one earlier. > > So to get this alternatives_fix_offsets() main entry point I > made the loop do the one iteration more again. Right, fair enough in my book :) Thanks, Conor.
On Wed, Dec 07, 2022 at 07:08:21PM +0100, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > Alternatives live in a different section, so addresses used by call > functions will point to wrong locations after the patch got applied. > > Similar to arm64, adjust the location to consider that offset. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > --- > arch/riscv/include/asm/alternative.h | 3 ++ > arch/riscv/kernel/alternative.c | 56 ++++++++++++++++++++++++++++ > arch/riscv/kernel/cpufeature.c | 5 ++- > 3 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h > index 6511dd73e812..1bd4027d34ca 100644 > --- a/arch/riscv/include/asm/alternative.h > +++ b/arch/riscv/include/asm/alternative.h > @@ -27,6 +27,9 @@ void __init apply_boot_alternatives(void); > void __init apply_early_boot_alternatives(void); > void apply_module_alternatives(void *start, size_t length); > > +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > + int patch_offset); > + > struct alt_entry { > void *old_ptr; /* address of original instruciton or data */ > void *alt_ptr; /* address of replacement instruction or data */ > diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c > index a7d26a00beea..c29e198ed9df 100644 > --- a/arch/riscv/kernel/alternative.c > +++ b/arch/riscv/kernel/alternative.c > @@ -15,6 +15,8 @@ > #include <asm/vendorid_list.h> > #include <asm/sbi.h> > #include <asm/csr.h> > +#include <asm/insn.h> > +#include <asm/patch.h> > > struct cpu_manufacturer_info_t { > unsigned long vendor_id; > @@ -53,6 +55,60 @@ static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_inf > } > } > > +static u32 riscv_instruction_at(void *p) > +{ > + u16 *parcel = p; > + > + return (unsigned int)parcel[0] | (unsigned int)parcel[1] << 16; > +} > + > +static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 insn1, u32 insn2, int patch_offset) Maybe instead of 'insn1' and 'insn2' names we should use the names 'auipc' and 'jalr' to self-document which is which. > +{ > + /* pick the original auipc + jalr */ If we change the names, then the comment above could be removed. > + u32 call[2] = { insn1, insn2 }; > + s32 imm; > + > + /* get and adjust new target address */ > + imm = riscv_insn_extract_utype_itype_imm(insn1, insn2); > + imm -= patch_offset; > + > + /* update instructions */ > + riscv_insn_insert_utype_itype_imm(call, imm); > + > + /* patch the call place again */ > + patch_text_nosync(ptr, call, sizeof(u32) * 2); > +} > + > +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > + int patch_offset) > +{ > + int num_instr = len / sizeof(u32); > + int i; > + > + /* > + * stop one instruction before the end, as we're checking > + * for auipc + jalr > + */ > + for (i = 0; i < num_instr; i++) { > + u32 inst = riscv_instruction_at(alt_ptr + i * sizeof(u32)); > + > + /* may be the start of an auipc + jalr pair */ > + if (riscv_insn_is_auipc(inst) && i < num_instr - 1) { > + u32 inst2 = riscv_instruction_at(alt_ptr + (i + 1) * sizeof(u32)); > + > + if (!riscv_insn_is_jalr(inst2)) > + continue; > + > + /* call will use ra register */ > + if (RV_EXTRACT_RD_REG(inst) != 1) > + continue; > + > + riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32), > + inst, inst2, patch_offset); > + } > + } > +} > + > /* > * This is called very early in the boot process (directly after we run > * a feature detect on the boot CPU). No need to worry about other CPUs > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 694267d1fe81..e91ec3d8e240 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -316,8 +316,11 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, > } > > tmp = (1U << alt->errata_id); > - if (cpu_req_feature & tmp) > + if (cpu_req_feature & tmp) { > patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len); > + riscv_alternative_fix_offsets(alt->old_ptr, alt->alt_len, > + alt->old_ptr - alt->alt_ptr); > + } > } > } > #endif > -- > 2.35.1 > Besides the naming nit which probably isn't worth respinning for Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
On Wed, Dec 7, 2022 at 6:08 PM Heiko Stuebner <heiko@sntech.de> wrote: > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > Alternatives live in a different section, so addresses used by call > functions will point to wrong locations after the patch got applied. > > Similar to arm64, adjust the location to consider that offset. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > --- > arch/riscv/include/asm/alternative.h | 3 ++ > arch/riscv/kernel/alternative.c | 56 ++++++++++++++++++++++++++++ > arch/riscv/kernel/cpufeature.c | 5 ++- > 3 files changed, 63 insertions(+), 1 deletion(-) > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Cheers, Prabhakar
diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h index 6511dd73e812..1bd4027d34ca 100644 --- a/arch/riscv/include/asm/alternative.h +++ b/arch/riscv/include/asm/alternative.h @@ -27,6 +27,9 @@ void __init apply_boot_alternatives(void); void __init apply_early_boot_alternatives(void); void apply_module_alternatives(void *start, size_t length); +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, + int patch_offset); + struct alt_entry { void *old_ptr; /* address of original instruciton or data */ void *alt_ptr; /* address of replacement instruction or data */ diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c index a7d26a00beea..c29e198ed9df 100644 --- a/arch/riscv/kernel/alternative.c +++ b/arch/riscv/kernel/alternative.c @@ -15,6 +15,8 @@ #include <asm/vendorid_list.h> #include <asm/sbi.h> #include <asm/csr.h> +#include <asm/insn.h> +#include <asm/patch.h> struct cpu_manufacturer_info_t { unsigned long vendor_id; @@ -53,6 +55,60 @@ static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_inf } } +static u32 riscv_instruction_at(void *p) +{ + u16 *parcel = p; + + return (unsigned int)parcel[0] | (unsigned int)parcel[1] << 16; +} + +static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 insn1, u32 insn2, int patch_offset) +{ + /* pick the original auipc + jalr */ + u32 call[2] = { insn1, insn2 }; + s32 imm; + + /* get and adjust new target address */ + imm = riscv_insn_extract_utype_itype_imm(insn1, insn2); + imm -= patch_offset; + + /* update instructions */ + riscv_insn_insert_utype_itype_imm(call, imm); + + /* patch the call place again */ + patch_text_nosync(ptr, call, sizeof(u32) * 2); +} + +void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, + int patch_offset) +{ + int num_instr = len / sizeof(u32); + int i; + + /* + * stop one instruction before the end, as we're checking + * for auipc + jalr + */ + for (i = 0; i < num_instr; i++) { + u32 inst = riscv_instruction_at(alt_ptr + i * sizeof(u32)); + + /* may be the start of an auipc + jalr pair */ + if (riscv_insn_is_auipc(inst) && i < num_instr - 1) { + u32 inst2 = riscv_instruction_at(alt_ptr + (i + 1) * sizeof(u32)); + + if (!riscv_insn_is_jalr(inst2)) + continue; + + /* call will use ra register */ + if (RV_EXTRACT_RD_REG(inst) != 1) + continue; + + riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32), + inst, inst2, patch_offset); + } + } +} + /* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 694267d1fe81..e91ec3d8e240 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -316,8 +316,11 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, } tmp = (1U << alt->errata_id); - if (cpu_req_feature & tmp) + if (cpu_req_feature & tmp) { patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len); + riscv_alternative_fix_offsets(alt->old_ptr, alt->alt_len, + alt->old_ptr - alt->alt_ptr); + } } } #endif