| Message ID | 20230109181755.2383085-4-heiko@sntech.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Palmer Dabbelt |
| Headers | show |
| Series | Zbb string optimizations and call support in alternatives | expand |
On Mon, Jan 09, 2023 at 07:17:53PM +0100, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > Alternatives live in a different section, so addresses used by jal > instructions 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/kernel/alternative.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c > index 6212ea0eed72..985c284fe9f4 100644 > --- a/arch/riscv/kernel/alternative.c > +++ b/arch/riscv/kernel/alternative.c > @@ -79,6 +79,21 @@ static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 auipc_insn, > patch_text_nosync(ptr, call, sizeof(u32) * 2); > } > > +static void riscv_alternative_fix_jal(void *ptr, u32 jal_insn, int patch_offset) > +{ > + s32 imm; > + > + /* get and adjust new target address */ > + imm = riscv_insn_extract_jtype_imm(jal_insn); > + imm -= patch_offset; > + > + /* update instructions */ instruction > + riscv_insn_insert_jtype_imm(&jal_insn, imm); > + > + /* patch the call place again */ > + patch_text_nosync(ptr, &jal_insn, sizeof(u32)); > +} > + > void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > int patch_offset) > { > @@ -106,6 +121,18 @@ void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32), > insn, insn2, patch_offset); I think we should add an i++ here. There's not a problem now, since we're only adding a fixup for jal, not jalr, but we should future-proof this and there's no reason to revisit an already fixed-up instruction anyway. > } > + > + if (riscv_insn_is_jal(insn)) { > + s32 imm = riscv_insn_extract_jtype_imm(insn); > + > + /* don't modify jumps inside the alternative block */ Don't > + if ((alt_ptr + i * sizeof(u32) + imm) >= alt_ptr && > + (alt_ptr + i * sizeof(u32) + imm) < (alt_ptr + len)) > + continue; > + > + riscv_alternative_fix_jal(alt_ptr + i * sizeof(u32), > + insn, patch_offset); > + } > } > } > > -- > 2.35.1 > Otherwise Reviewed-by: Andrew Jones <ajones@ventanamicro.com> Thanks, drew
On Mon, Jan 09, 2023 at 07:17:53PM +0100, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > Alternatives live in a different section, so addresses used by jal > instructions will point to wrong locations after the patch got applied. > > Similar to arm64, adjust the location to consider that offset. I already submitted this in a month ago. https://lore.kernel.org/linux-riscv/20221204174632.3677-2-jszhang@kernel.org/ > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > --- > arch/riscv/kernel/alternative.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c > index 6212ea0eed72..985c284fe9f4 100644 > --- a/arch/riscv/kernel/alternative.c > +++ b/arch/riscv/kernel/alternative.c > @@ -79,6 +79,21 @@ static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 auipc_insn, > patch_text_nosync(ptr, call, sizeof(u32) * 2); > } > > +static void riscv_alternative_fix_jal(void *ptr, u32 jal_insn, int patch_offset) > +{ > + s32 imm; > + > + /* get and adjust new target address */ > + imm = riscv_insn_extract_jtype_imm(jal_insn); > + imm -= patch_offset; > + > + /* update instructions */ > + riscv_insn_insert_jtype_imm(&jal_insn, imm); > + > + /* patch the call place again */ > + patch_text_nosync(ptr, &jal_insn, sizeof(u32)); > +} > + > void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > int patch_offset) > { > @@ -106,6 +121,18 @@ void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32), > insn, insn2, patch_offset); > } > + > + if (riscv_insn_is_jal(insn)) { > + s32 imm = riscv_insn_extract_jtype_imm(insn); > + > + /* don't modify jumps inside the alternative block */ > + if ((alt_ptr + i * sizeof(u32) + imm) >= alt_ptr && > + (alt_ptr + i * sizeof(u32) + imm) < (alt_ptr + len)) > + continue; > + > + riscv_alternative_fix_jal(alt_ptr + i * sizeof(u32), > + insn, patch_offset); > + } > } > } > > -- > 2.35.1 >
Hi, Am Mittwoch, 11. Januar 2023, 14:18:14 CET schrieb Jisheng Zhang: > On Mon, Jan 09, 2023 at 07:17:53PM +0100, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > Alternatives live in a different section, so addresses used by jal > > instructions will point to wrong locations after the patch got applied. > > > > Similar to arm64, adjust the location to consider that offset. > > I already submitted this in a month ago. > https://lore.kernel.org/linux-riscv/20221204174632.3677-2-jszhang@kernel.org/ Yep, though the now merged base series for the call fixups changed a lot since that. With changes needed as we discussed in your series. As the change for the jal fixer itself was this small and I needed it for my work, I did just go ahead and implemented it so I could move forward. I'm not that attached to the authorship, so if you want we can set you as author or Co-Developed-by or so that is just fine. Heiko > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > > --- > > arch/riscv/kernel/alternative.c | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c > > index 6212ea0eed72..985c284fe9f4 100644 > > --- a/arch/riscv/kernel/alternative.c > > +++ b/arch/riscv/kernel/alternative.c > > @@ -79,6 +79,21 @@ static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 auipc_insn, > > patch_text_nosync(ptr, call, sizeof(u32) * 2); > > } > > > > +static void riscv_alternative_fix_jal(void *ptr, u32 jal_insn, int patch_offset) > > +{ > > + s32 imm; > > + > > + /* get and adjust new target address */ > > + imm = riscv_insn_extract_jtype_imm(jal_insn); > > + imm -= patch_offset; > > + > > + /* update instructions */ > > + riscv_insn_insert_jtype_imm(&jal_insn, imm); > > + > > + /* patch the call place again */ > > + patch_text_nosync(ptr, &jal_insn, sizeof(u32)); > > +} > > + > > void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > > int patch_offset) > > { > > @@ -106,6 +121,18 @@ void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > > riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32), > > insn, insn2, patch_offset); > > } > > + > > + if (riscv_insn_is_jal(insn)) { > > + s32 imm = riscv_insn_extract_jtype_imm(insn); > > + > > + /* don't modify jumps inside the alternative block */ > > + if ((alt_ptr + i * sizeof(u32) + imm) >= alt_ptr && > > + (alt_ptr + i * sizeof(u32) + imm) < (alt_ptr + len)) > > + continue; > > + > > + riscv_alternative_fix_jal(alt_ptr + i * sizeof(u32), > > + insn, patch_offset); > > + } > > } > > } > > >
On Wed, Jan 11, 2023 at 09:18:14PM +0800, Jisheng Zhang wrote: > On Mon, Jan 09, 2023 at 07:17:53PM +0100, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > Alternatives live in a different section, so addresses used by jal > > instructions will point to wrong locations after the patch got applied. > > > > Similar to arm64, adjust the location to consider that offset. > > I already submitted this in a month ago. > https://lore.kernel.org/linux-riscv/20221204174632.3677-2-jszhang@kernel.org/ > Hi Jisheng, It'd be great to see a v3 of your series on the list, since I'd like to base zicboz work on it. Thanks, drew
On Wed, Jan 11, 2023 at 03:15:19PM +0100, Andrew Jones wrote: > On Wed, Jan 11, 2023 at 09:18:14PM +0800, Jisheng Zhang wrote: > > On Mon, Jan 09, 2023 at 07:17:53PM +0100, Heiko Stuebner wrote: > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > > > Alternatives live in a different section, so addresses used by jal > > > instructions will point to wrong locations after the patch got applied. > > > > > > Similar to arm64, adjust the location to consider that offset. > > > > I already submitted this in a month ago. > > https://lore.kernel.org/linux-riscv/20221204174632.3677-2-jszhang@kernel.org/ > > > > Hi Jisheng, > > It'd be great to see a v3 of your series on the list, since I'd like to > base zicboz work on it. Hi Andrew, Sorry for being late. I'm cooking the patches, will send out today. Best Regards
On Tue, Jan 10, 2023 at 10:28:09AM +0100, Andrew Jones wrote: > On Mon, Jan 09, 2023 at 07:17:53PM +0100, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > Alternatives live in a different section, so addresses used by jal > > instructions 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/kernel/alternative.c | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c > > index 6212ea0eed72..985c284fe9f4 100644 > > --- a/arch/riscv/kernel/alternative.c > > +++ b/arch/riscv/kernel/alternative.c > > @@ -79,6 +79,21 @@ static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 auipc_insn, > > patch_text_nosync(ptr, call, sizeof(u32) * 2); > > } > > > > +static void riscv_alternative_fix_jal(void *ptr, u32 jal_insn, int patch_offset) > > +{ > > + s32 imm; > > + > > + /* get and adjust new target address */ > > + imm = riscv_insn_extract_jtype_imm(jal_insn); > > + imm -= patch_offset; > > + > > + /* update instructions */ > > instruction > > > + riscv_insn_insert_jtype_imm(&jal_insn, imm); > > + > > + /* patch the call place again */ > > + patch_text_nosync(ptr, &jal_insn, sizeof(u32)); > > +} > > + > > void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > > int patch_offset) > > { > > @@ -106,6 +121,18 @@ void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, > > riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32), > > insn, insn2, patch_offset); > > I think we should add an i++ here. There's not a problem now, since we're > only adding a fixup for jal, not jalr, but we should future-proof this and > there's no reason to revisit an already fixed-up instruction anyway. Hi Andrew, IMHO, adding the i++ here belongs to the aupic jalr imm patching commit, since the patch has been merged to riscv-next, I think it's better to add a seperate patch to do this improvement, thus I didn't update as commented here in my v3 series. Thanks > > > } > > + > > + if (riscv_insn_is_jal(insn)) { > > + s32 imm = riscv_insn_extract_jtype_imm(insn); > > + > > + /* don't modify jumps inside the alternative block */ > > Don't > > > + if ((alt_ptr + i * sizeof(u32) + imm) >= alt_ptr && > > + (alt_ptr + i * sizeof(u32) + imm) < (alt_ptr + len)) > > + continue; > > + > > + riscv_alternative_fix_jal(alt_ptr + i * sizeof(u32), > > + insn, patch_offset); > > + } > > } > > } > > > > -- > > 2.35.1 > > > > Otherwise > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > Thanks, > drew
diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c index 6212ea0eed72..985c284fe9f4 100644 --- a/arch/riscv/kernel/alternative.c +++ b/arch/riscv/kernel/alternative.c @@ -79,6 +79,21 @@ static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 auipc_insn, patch_text_nosync(ptr, call, sizeof(u32) * 2); } +static void riscv_alternative_fix_jal(void *ptr, u32 jal_insn, int patch_offset) +{ + s32 imm; + + /* get and adjust new target address */ + imm = riscv_insn_extract_jtype_imm(jal_insn); + imm -= patch_offset; + + /* update instructions */ + riscv_insn_insert_jtype_imm(&jal_insn, imm); + + /* patch the call place again */ + patch_text_nosync(ptr, &jal_insn, sizeof(u32)); +} + void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, int patch_offset) { @@ -106,6 +121,18 @@ void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len, riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32), insn, insn2, patch_offset); } + + if (riscv_insn_is_jal(insn)) { + s32 imm = riscv_insn_extract_jtype_imm(insn); + + /* don't modify jumps inside the alternative block */ + if ((alt_ptr + i * sizeof(u32) + imm) >= alt_ptr && + (alt_ptr + i * sizeof(u32) + imm) < (alt_ptr + len)) + continue; + + riscv_alternative_fix_jal(alt_ptr + i * sizeof(u32), + insn, patch_offset); + } } }