| Message ID | 20220913094252.3555240-6-andy.chiu@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable ftrace with kernel preemption for RISC-V | expand |
Is this patch related to this series? On Tue, Sep 13, 2022 at 5:44 PM Andy Chiu <andy.chiu@sifive.com> wrote: > > runtime code patching must be done at a naturally aligned address, or we > may execute on a partial instruction. If it's true, we can't use static branches at all. Have you encountered a problem? If you are right, arm64 ... csky all need the patch. > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > Reviewed-by: Greentime Hu <greentime.hu@sifive.com> > --- > arch/riscv/include/asm/jump_label.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h > index 38af2ec7b9bf..729991e8f782 100644 > --- a/arch/riscv/include/asm/jump_label.h > +++ b/arch/riscv/include/asm/jump_label.h > @@ -18,6 +18,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, > bool branch) > { > asm_volatile_goto( > + " .align 2 \n\t" > " .option push \n\t" > " .option norelax \n\t" > " .option norvc \n\t" > @@ -39,6 +40,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, > bool branch) > { > asm_volatile_goto( > + " .align 2 \n\t" > " .option push \n\t" > " .option norelax \n\t" > " .option norvc \n\t" > -- > 2.36.0 >
On 13 Sept 2022, at 10:42, Andy Chiu <andy.chiu@sifive.com> wrote: > > runtime code patching must be done at a naturally aligned address, or we > may execute on a partial instruction. > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > Reviewed-by: Greentime Hu <greentime.hu@sifive.com> > --- > arch/riscv/include/asm/jump_label.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h > index 38af2ec7b9bf..729991e8f782 100644 > --- a/arch/riscv/include/asm/jump_label.h > +++ b/arch/riscv/include/asm/jump_label.h > @@ -18,6 +18,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, > bool branch) > { > asm_volatile_goto( > + " .align 2 \n\t" .align is a horrible directive whose meaning changes between architectures and requires careful thought, especially when the argument is a power of 2. Better to use .balign 4, or .p2align 2 if you really want to for some reason. Jess > " .option push \n\t" > " .option norelax \n\t" > " .option norvc \n\t" > @@ -39,6 +40,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, > bool branch) > { > asm_volatile_goto( > + " .align 2 \n\t" > " .option push \n\t" > " .option norelax \n\t" > " .option norvc \n\t" > -- > 2.36.0 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Sep 14, 2022 at 10:24 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 13 Sept 2022, at 10:42, Andy Chiu <andy.chiu@sifive.com> wrote: > > > > runtime code patching must be done at a naturally aligned address, or we > > may execute on a partial instruction. > > > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > > Reviewed-by: Greentime Hu <greentime.hu@sifive.com> > > --- > > arch/riscv/include/asm/jump_label.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h > > index 38af2ec7b9bf..729991e8f782 100644 > > --- a/arch/riscv/include/asm/jump_label.h > > +++ b/arch/riscv/include/asm/jump_label.h > > @@ -18,6 +18,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, > > bool branch) > > { > > asm_volatile_goto( > > + " .align 2 \n\t" > > .align is a horrible directive whose meaning changes between > architectures and requires careful thought, especially when the > argument is a power of 2. Better to use .balign 4, or .p2align 2 if you > really want to for some reason. Do we really need to align here? Should it be naturally done by the compiler? > > Jess > > > " .option push \n\t" > > " .option norelax \n\t" > > " .option norvc \n\t" > > @@ -39,6 +40,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, > > bool branch) > > { > > asm_volatile_goto( > > + " .align 2 \n\t" > > " .option push \n\t" > > " .option norelax \n\t" > > " .option norvc \n\t" > > -- > > 2.36.0 > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv >
On 15 Sept 2022, at 02:47, Guo Ren <guoren@kernel.org> wrote: > > On Wed, Sep 14, 2022 at 10:24 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: >> >> On 13 Sept 2022, at 10:42, Andy Chiu <andy.chiu@sifive.com> wrote: >>> >>> runtime code patching must be done at a naturally aligned address, or we >>> may execute on a partial instruction. >>> >>> Signed-off-by: Andy Chiu <andy.chiu@sifive.com> >>> Reviewed-by: Greentime Hu <greentime.hu@sifive.com> >>> --- >>> arch/riscv/include/asm/jump_label.h | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h >>> index 38af2ec7b9bf..729991e8f782 100644 >>> --- a/arch/riscv/include/asm/jump_label.h >>> +++ b/arch/riscv/include/asm/jump_label.h >>> @@ -18,6 +18,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, >>> bool branch) >>> { >>> asm_volatile_goto( >>> + " .align 2 \n\t" >> >> .align is a horrible directive whose meaning changes between >> architectures and requires careful thought, especially when the >> argument is a power of 2. Better to use .balign 4, or .p2align 2 if you >> really want to for some reason. > Do we really need to align here? Should it be naturally done by the compiler? Compilers don’t add alignment do inline assembly beyond what’s needed. And assemblers won’t align beyond what the architecture requires here either as far as I know for .option rvc, since normally you still have an RVC-capable processor you just don’t want compressed instructions at that specific point. Jess >> >> Jess >> >>> " .option push \n\t" >>> " .option norelax \n\t" >>> " .option norvc \n\t" >>> @@ -39,6 +40,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, >>> bool branch) >>> { >>> asm_volatile_goto( >>> + " .align 2 \n\t" >>> " .option push \n\t" >>> " .option norelax \n\t" >>> " .option norvc \n\t" >>> -- >>> 2.36.0 >>> >>> >>> _______________________________________________ >>> linux-riscv mailing list >>> linux-riscv@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-riscv >> > > > -- > Best Regards > Guo Ren
Hi Guo, Sorry for sending it again, I forgot to send in plain-text on the last mail. On Wed, Sep 14, 2022 at 3:06 PM Guo Ren <guoren@kernel.org> wrote: > > Is this patch related to this series? > This is related to dynamic code patching but not the mechanism of "function tracer" itself. You are right, I should submit another patch for that. > On Tue, Sep 13, 2022 at 5:44 PM Andy Chiu <andy.chiu@sifive.com> wrote: > > > > runtime code patching must be done at a naturally aligned address, or we > > may execute on a partial instruction. > If it's true, we can't use static branches at all. Have you > encountered a problem? > > If you are right, arm64 ... csky all need the patch. > In fact we have run into problems that traced back to static jump functions during the test. We switched tracer randomly for every 1~5 seconds on a dual-core QEMU setup and found the kernel stucking at a static branch where it jumps to itself. The reason is that the static branch was 2-byte but not 4-byte aligned. Then, the kernel would patch the instruction, either J or NOP, with 2 half-word stores, if the machine does not have efficient unaligned accesses. Thus, there exists moments where a half of the NOP mixes with the other half of the J when transitioning the branch. In our particular case, on a little-endian machine, the upper half of the NOP was mixed with the lower part of the J when enabling the branch, resulting in a jump that jumped to itself. On the other way, it would result in a HINT instruction when disabling the branch, but it might not be observable. ARM64 does not have this problem since all instructions must be 4-byte aligned. Regards, Andy
On Sat, Sep 17, 2022 at 7:54 AM Andy Chiu <andy.chiu@sifive.com> wrote: > > Hi Guo, > > Sorry for sending it again, I forgot to send in plain-text on the last mail. > > On Wed, Sep 14, 2022 at 3:06 PM Guo Ren <guoren@kernel.org> wrote: > > > > Is this patch related to this series? > > > > This is related to dynamic code patching but not the mechanism of > "function tracer" itself. You are right, I should submit another patch > for that. > > > On Tue, Sep 13, 2022 at 5:44 PM Andy Chiu <andy.chiu@sifive.com> wrote: > > > > > > runtime code patching must be done at a naturally aligned address, or we > > > may execute on a partial instruction. > > If it's true, we can't use static branches at all. Have you > > encountered a problem? > > > > If you are right, arm64 ... csky all need the patch. > > > In fact we have run into problems that traced back to static jump > functions during the test. We switched tracer randomly for every 1~5 > seconds on a dual-core QEMU setup and found the kernel stucking at a > static branch where it jumps to itself. The reason is that the static > branch was 2-byte but not 4-byte aligned. Then, the kernel would patch > the instruction, either J or NOP, with 2 half-word stores, if the > machine does not have efficient unaligned accesses. Thus, there exists > moments where a half of the NOP mixes with the other half of the J > when transitioning the branch. In our particular case, on a > little-endian machine, the upper half of the NOP was mixed with the > lower part of the J when enabling the branch, resulting in a jump that > jumped to itself. On the other way, it would result in a HINT > instruction when disabling the branch, but it might not be observable. How about limiting the static branch to 16-bit instruction? (nop16 - br16) > > ARM64 does not have this problem since all instructions must be 4-byte aligned. But, csky does (16+32). Thx. > > Regards, > Andy
From: Guo Ren <guoren@linux.alibaba.com> Reduce size of static branch. Signed-off-by: Guo Ren <guoren@linux.alibaba.com> Signed-off-by: Guo Ren <guoren@kernel.org> --- arch/riscv/include/asm/jump_label.h | 17 ++++++++++----- arch/riscv/kernel/jump_label.c | 32 +++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h index 38af2ec7b9bf..78f747dfa8a2 100644 --- a/arch/riscv/include/asm/jump_label.h +++ b/arch/riscv/include/asm/jump_label.h @@ -12,17 +12,21 @@ #include <linux/types.h> #include <asm/asm.h> +#ifdef CONFIG_RISCV_ISA_C +#define JUMP_LABEL_NOP_SIZE 2 +#else #define JUMP_LABEL_NOP_SIZE 4 +#endif static __always_inline bool arch_static_branch(struct static_key *key, bool branch) { asm_volatile_goto( - " .option push \n\t" - " .option norelax \n\t" - " .option norvc \n\t" +#ifdef CONFIG_RISCV_ISA_C + "1: c.nop \n\t" +#else "1: nop \n\t" - " .option pop \n\t" +#endif " .pushsection __jump_table, \"aw\" \n\t" " .align " RISCV_LGPTR " \n\t" " .long 1b - ., %l[label] - . \n\t" @@ -39,11 +43,14 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch) { asm_volatile_goto( +#ifdef CONFIG_RISCV_ISA_C + "1: c.j %l[label] \n\t" +#else " .option push \n\t" " .option norelax \n\t" - " .option norvc \n\t" "1: jal zero, %l[label] \n\t" " .option pop \n\t" +#endif " .pushsection __jump_table, \"aw\" \n\t" " .align " RISCV_LGPTR " \n\t" " .long 1b - ., %l[label] - . \n\t" diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c index e6694759dbd0..64a4e5df093d 100644 --- a/arch/riscv/kernel/jump_label.c +++ b/arch/riscv/kernel/jump_label.c @@ -11,21 +11,52 @@ #include <asm/bug.h> #include <asm/patch.h> +#ifdef CONFIG_RISCV_ISA_C +#define RISCV_INSN_C_NOP 0x0001U +#define RISCV_INSN_C_JAL 0xa001U +#else #define RISCV_INSN_NOP 0x00000013U #define RISCV_INSN_JAL 0x0000006fU +#endif void arch_jump_label_transform(struct jump_entry *entry, enum jump_label_type type) { void *addr = (void *)jump_entry_code(entry); +#ifdef CONFIG_RISCV_ISA_C + u16 insn; +#else u32 insn; +#endif if (type == JUMP_LABEL_JMP) { long offset = jump_entry_target(entry) - jump_entry_code(entry); +#ifdef CONFIG_RISCV_ISA_C + if (WARN_ON(offset & 1 || offset < -2048 || offset >= 2048)) + return; + /* + * 001 | imm[11|4|9:8|10|6|7|3:1|5] 01 - C.JAL + */ + insn = RISCV_INSN_C_JAL | + (((u16)offset & GENMASK(5, 5)) >> (5 - 2)) | + (((u16)offset & GENMASK(3, 1)) << (3 - 1)) | + (((u16)offset & GENMASK(7, 7)) >> (7 - 6)) | + (((u16)offset & GENMASK(6, 6)) << (7 - 6)) | + (((u16)offset & GENMASK(10, 10)) >> (10 - 8)) | + (((u16)offset & GENMASK(9, 8)) << (9 - 8)) | + (((u16)offset & GENMASK(4, 4)) << (11 - 4)) | + (((u16)offset & GENMASK(11, 11)) << (12 - 11)); + } else { + insn = RISCV_INSN_C_NOP; + } +#else if (WARN_ON(offset & 1 || offset < -524288 || offset >= 524288)) return; + /* + * imm[20|10:1|11|19:12] | rd | 1101111 - JAL + */ insn = RISCV_INSN_JAL | (((u32)offset & GENMASK(19, 12)) << (12 - 12)) | (((u32)offset & GENMASK(11, 11)) << (20 - 11)) | @@ -34,6 +65,7 @@ void arch_jump_label_transform(struct jump_entry *entry, } else { insn = RISCV_INSN_NOP; } +#endif mutex_lock(&text_mutex); patch_text_nosync(addr, &insn, sizeof(insn));
On Sun, Sep 18, 2022 at 2:39 AM <guoren@kernel.org> wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > Reduce size of static branch. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Signed-off-by: Guo Ren <guoren@kernel.org> > --- > arch/riscv/include/asm/jump_label.h | 17 ++++++++++----- > arch/riscv/kernel/jump_label.c | 32 +++++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+), 5 deletions(-) > > diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h > index 38af2ec7b9bf..78f747dfa8a2 100644 > --- a/arch/riscv/include/asm/jump_label.h > +++ b/arch/riscv/include/asm/jump_label.h > @@ -12,17 +12,21 @@ > #include <linux/types.h> > #include <asm/asm.h> > > +#ifdef CONFIG_RISCV_ISA_C > +#define JUMP_LABEL_NOP_SIZE 2 > +#else > #define JUMP_LABEL_NOP_SIZE 4 > +#endif > > static __always_inline bool arch_static_branch(struct static_key *key, > bool branch) > { > asm_volatile_goto( > - " .option push \n\t" > - " .option norelax \n\t" > - " .option norvc \n\t" > +#ifdef CONFIG_RISCV_ISA_C > + "1: c.nop \n\t" > +#else > "1: nop \n\t" > - " .option pop \n\t" > +#endif > " .pushsection __jump_table, \"aw\" \n\t" > " .align " RISCV_LGPTR " \n\t" > " .long 1b - ., %l[label] - . \n\t" > @@ -39,11 +43,14 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, > bool branch) > { > asm_volatile_goto( > +#ifdef CONFIG_RISCV_ISA_C > + "1: c.j %l[label] \n\t" > +#else > " .option push \n\t" > " .option norelax \n\t" > - " .option norvc \n\t" > "1: jal zero, %l[label] \n\t" > " .option pop \n\t" > +#endif > " .pushsection __jump_table, \"aw\" \n\t" > " .align " RISCV_LGPTR " \n\t" > " .long 1b - ., %l[label] - . \n\t" > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c > index e6694759dbd0..64a4e5df093d 100644 > --- a/arch/riscv/kernel/jump_label.c > +++ b/arch/riscv/kernel/jump_label.c > @@ -11,21 +11,52 @@ > #include <asm/bug.h> > #include <asm/patch.h> > > +#ifdef CONFIG_RISCV_ISA_C > +#define RISCV_INSN_C_NOP 0x0001U > +#define RISCV_INSN_C_JAL 0xa001U > +#else > #define RISCV_INSN_NOP 0x00000013U > #define RISCV_INSN_JAL 0x0000006fU > +#endif > > void arch_jump_label_transform(struct jump_entry *entry, > enum jump_label_type type) > { > void *addr = (void *)jump_entry_code(entry); > +#ifdef CONFIG_RISCV_ISA_C > + u16 insn; > +#else > u32 insn; > +#endif > > if (type == JUMP_LABEL_JMP) { > long offset = jump_entry_target(entry) - jump_entry_code(entry); > +#ifdef CONFIG_RISCV_ISA_C > + if (WARN_ON(offset & 1 || offset < -2048 || offset >= 2048)) > + return; > > + /* > + * 001 | imm[11|4|9:8|10|6|7|3:1|5] 01 - C.JAL > + */ > + insn = RISCV_INSN_C_JAL | > + (((u16)offset & GENMASK(5, 5)) >> (5 - 2)) | > + (((u16)offset & GENMASK(3, 1)) << (3 - 1)) | > + (((u16)offset & GENMASK(7, 7)) >> (7 - 6)) | > + (((u16)offset & GENMASK(6, 6)) << (7 - 6)) | > + (((u16)offset & GENMASK(10, 10)) >> (10 - 8)) | > + (((u16)offset & GENMASK(9, 8)) << (9 - 8)) | > + (((u16)offset & GENMASK(4, 4)) << (11 - 4)) | > + (((u16)offset & GENMASK(11, 11)) << (12 - 11)); > + } else { > + insn = RISCV_INSN_C_NOP; > + } > +#else > if (WARN_ON(offset & 1 || offset < -524288 || offset >= 524288)) For the unify, it also should be: if (WARN_ON(offset & 1 || offset < -2048 || offset >= 2048)) > return; > > + /* > + * imm[20|10:1|11|19:12] | rd | 1101111 - JAL > + */ > insn = RISCV_INSN_JAL | > (((u32)offset & GENMASK(19, 12)) << (12 - 12)) | > (((u32)offset & GENMASK(11, 11)) << (20 - 11)) | > @@ -34,6 +65,7 @@ void arch_jump_label_transform(struct jump_entry *entry, > } else { > insn = RISCV_INSN_NOP; > } > +#endif > > mutex_lock(&text_mutex); > patch_text_nosync(addr, &insn, sizeof(insn)); > -- > 2.36.1 >
On Sun, Sep 18, 2022 at 2:39 AM <guoren@kernel.org> wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > Reduce size of static branch. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Signed-off-by: Guo Ren <guoren@kernel.org> > --- > arch/riscv/include/asm/jump_label.h | 17 ++++++++++----- > arch/riscv/kernel/jump_label.c | 32 +++++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+), 5 deletions(-) > > diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h > index 38af2ec7b9bf..78f747dfa8a2 100644 > --- a/arch/riscv/include/asm/jump_label.h > +++ b/arch/riscv/include/asm/jump_label.h > @@ -12,17 +12,21 @@ > #include <linux/types.h> > #include <asm/asm.h> > > +#ifdef CONFIG_RISCV_ISA_C > +#define JUMP_LABEL_NOP_SIZE 2 > +#else > #define JUMP_LABEL_NOP_SIZE 4 > +#endif > > static __always_inline bool arch_static_branch(struct static_key *key, > bool branch) > { > asm_volatile_goto( > - " .option push \n\t" > - " .option norelax \n\t" > - " .option norvc \n\t" > +#ifdef CONFIG_RISCV_ISA_C > + "1: c.nop \n\t" > +#else > "1: nop \n\t" > - " .option pop \n\t" > +#endif > " .pushsection __jump_table, \"aw\" \n\t" > " .align " RISCV_LGPTR " \n\t" > " .long 1b - ., %l[label] - . \n\t" We should check the range during compile time, here. Run-time checking is the last and most painful method to prevent some errors. > @@ -39,11 +43,14 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, > bool branch) > { > asm_volatile_goto( > +#ifdef CONFIG_RISCV_ISA_C > + "1: c.j %l[label] \n\t" > +#else > " .option push \n\t" > " .option norelax \n\t" > - " .option norvc \n\t" > "1: jal zero, %l[label] \n\t" > " .option pop \n\t" > +#endif > " .pushsection __jump_table, \"aw\" \n\t" > " .align " RISCV_LGPTR " \n\t" > " .long 1b - ., %l[label] - . \n\t" > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c > index e6694759dbd0..64a4e5df093d 100644 > --- a/arch/riscv/kernel/jump_label.c > +++ b/arch/riscv/kernel/jump_label.c > @@ -11,21 +11,52 @@ > #include <asm/bug.h> > #include <asm/patch.h> > > +#ifdef CONFIG_RISCV_ISA_C > +#define RISCV_INSN_C_NOP 0x0001U > +#define RISCV_INSN_C_JAL 0xa001U > +#else > #define RISCV_INSN_NOP 0x00000013U > #define RISCV_INSN_JAL 0x0000006fU > +#endif > > void arch_jump_label_transform(struct jump_entry *entry, > enum jump_label_type type) > { > void *addr = (void *)jump_entry_code(entry); > +#ifdef CONFIG_RISCV_ISA_C > + u16 insn; > +#else > u32 insn; > +#endif > > if (type == JUMP_LABEL_JMP) { > long offset = jump_entry_target(entry) - jump_entry_code(entry); > +#ifdef CONFIG_RISCV_ISA_C > + if (WARN_ON(offset & 1 || offset < -2048 || offset >= 2048)) > + return; > > + /* > + * 001 | imm[11|4|9:8|10|6|7|3:1|5] 01 - C.JAL > + */ > + insn = RISCV_INSN_C_JAL | > + (((u16)offset & GENMASK(5, 5)) >> (5 - 2)) | > + (((u16)offset & GENMASK(3, 1)) << (3 - 1)) | > + (((u16)offset & GENMASK(7, 7)) >> (7 - 6)) | > + (((u16)offset & GENMASK(6, 6)) << (7 - 6)) | > + (((u16)offset & GENMASK(10, 10)) >> (10 - 8)) | > + (((u16)offset & GENMASK(9, 8)) << (9 - 8)) | > + (((u16)offset & GENMASK(4, 4)) << (11 - 4)) | > + (((u16)offset & GENMASK(11, 11)) << (12 - 11)); > + } else { > + insn = RISCV_INSN_C_NOP; > + } > +#else > if (WARN_ON(offset & 1 || offset < -524288 || offset >= 524288)) For the unify, it also should be: if (WARN_ON(offset & 1 || offset < -2048 || offset >= 2048)) After some tests, 2048 is enough for the current riscv Linux. > return; > > + /* > + * imm[20|10:1|11|19:12] | rd | 1101111 - JAL > + */ > insn = RISCV_INSN_JAL | > (((u32)offset & GENMASK(19, 12)) << (12 - 12)) | > (((u32)offset & GENMASK(11, 11)) << (20 - 11)) | > @@ -34,6 +65,7 @@ void arch_jump_label_transform(struct jump_entry *entry, > } else { > insn = RISCV_INSN_NOP; > } > +#endif > > mutex_lock(&text_mutex); > patch_text_nosync(addr, &insn, sizeof(insn)); > -- > 2.36.1 >
On 17 Sept 2022, at 19:38, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > Reduce size of static branch. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Signed-off-by: Guo Ren <guoren@kernel.org> > --- > arch/riscv/include/asm/jump_label.h | 17 ++++++++++----- > arch/riscv/kernel/jump_label.c | 32 +++++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+), 5 deletions(-) > > diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h > index 38af2ec7b9bf..78f747dfa8a2 100644 > --- a/arch/riscv/include/asm/jump_label.h > +++ b/arch/riscv/include/asm/jump_label.h > @@ -12,17 +12,21 @@ > #include <linux/types.h> > #include <asm/asm.h> > > +#ifdef CONFIG_RISCV_ISA_C > +#define JUMP_LABEL_NOP_SIZE 2 > +#else > #define JUMP_LABEL_NOP_SIZE 4 > +#endif > > static __always_inline bool arch_static_branch(struct static_key *key, > bool branch) > { > asm_volatile_goto( > - " .option push \n\t" > - " .option norelax \n\t" > - " .option norvc \n\t" > +#ifdef CONFIG_RISCV_ISA_C > + "1: c.nop \n\t" > +#else > "1: nop \n\t" > - " .option pop \n\t" > +#endif > " .pushsection __jump_table, \"aw\" \n\t" > " .align " RISCV_LGPTR " \n\t" > " .long 1b - ., %l[label] - . \n\t" > @@ -39,11 +43,14 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, > bool branch) > { > asm_volatile_goto( > +#ifdef CONFIG_RISCV_ISA_C > + "1: c.j %l[label] \n\t" > +#else > " .option push \n\t" > " .option norelax \n\t" > - " .option norvc \n\t" > "1: jal zero, %l[label] \n\t" > " .option pop \n\t" > +#endif > " .pushsection __jump_table, \"aw\" \n\t" > " .align " RISCV_LGPTR " \n\t" > " .long 1b - ., %l[label] - . \n\t" > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c > index e6694759dbd0..64a4e5df093d 100644 > --- a/arch/riscv/kernel/jump_label.c > +++ b/arch/riscv/kernel/jump_label.c > @@ -11,21 +11,52 @@ > #include <asm/bug.h> > #include <asm/patch.h> > > +#ifdef CONFIG_RISCV_ISA_C > +#define RISCV_INSN_C_NOP 0x0001U > +#define RISCV_INSN_C_JAL 0xa001U This is C.J (i.e. JAL X0) not C.JAL (i.e. JAL RA, which is RV32-only and not what you want since it clobbers RA). Jess > +#else > #define RISCV_INSN_NOP 0x00000013U > #define RISCV_INSN_JAL 0x0000006fU > +#endif > > void arch_jump_label_transform(struct jump_entry *entry, > enum jump_label_type type) > { > void *addr = (void *)jump_entry_code(entry); > +#ifdef CONFIG_RISCV_ISA_C > + u16 insn; > +#else > u32 insn; > +#endif > > if (type == JUMP_LABEL_JMP) { > long offset = jump_entry_target(entry) - jump_entry_code(entry); > +#ifdef CONFIG_RISCV_ISA_C > + if (WARN_ON(offset & 1 || offset < -2048 || offset >= 2048)) > + return; > > + /* > + * 001 | imm[11|4|9:8|10|6|7|3:1|5] 01 - C.JAL > + */ > + insn = RISCV_INSN_C_JAL | > + (((u16)offset & GENMASK(5, 5)) >> (5 - 2)) | > + (((u16)offset & GENMASK(3, 1)) << (3 - 1)) | > + (((u16)offset & GENMASK(7, 7)) >> (7 - 6)) | > + (((u16)offset & GENMASK(6, 6)) << (7 - 6)) | > + (((u16)offset & GENMASK(10, 10)) >> (10 - 8)) | > + (((u16)offset & GENMASK(9, 8)) << (9 - 8)) | > + (((u16)offset & GENMASK(4, 4)) << (11 - 4)) | > + (((u16)offset & GENMASK(11, 11)) << (12 - 11)); > + } else { > + insn = RISCV_INSN_C_NOP; > + } > +#else > if (WARN_ON(offset & 1 || offset < -524288 || offset >= 524288)) > return; > > + /* > + * imm[20|10:1|11|19:12] | rd | 1101111 - JAL > + */ > insn = RISCV_INSN_JAL | > (((u32)offset & GENMASK(19, 12)) << (12 - 12)) | > (((u32)offset & GENMASK(11, 11)) << (20 - 11)) | > @@ -34,6 +65,7 @@ void arch_jump_label_transform(struct jump_entry *entry, > } else { > insn = RISCV_INSN_NOP; > } > +#endif > > mutex_lock(&text_mutex); > patch_text_nosync(addr, &insn, sizeof(insn)); > -- > 2.36.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Sun, Sep 18, 2022 at 8:12 AM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 17 Sept 2022, at 19:38, guoren@kernel.org wrote: > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > Reduce size of static branch. > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Signed-off-by: Guo Ren <guoren@kernel.org> > > --- > > arch/riscv/include/asm/jump_label.h | 17 ++++++++++----- > > arch/riscv/kernel/jump_label.c | 32 +++++++++++++++++++++++++++++ > > 2 files changed, 44 insertions(+), 5 deletions(-) > > > > diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h > > index 38af2ec7b9bf..78f747dfa8a2 100644 > > --- a/arch/riscv/include/asm/jump_label.h > > +++ b/arch/riscv/include/asm/jump_label.h > > @@ -12,17 +12,21 @@ > > #include <linux/types.h> > > #include <asm/asm.h> > > > > +#ifdef CONFIG_RISCV_ISA_C > > +#define JUMP_LABEL_NOP_SIZE 2 > > +#else > > #define JUMP_LABEL_NOP_SIZE 4 > > +#endif > > > > static __always_inline bool arch_static_branch(struct static_key *key, > > bool branch) > > { > > asm_volatile_goto( > > - " .option push \n\t" > > - " .option norelax \n\t" > > - " .option norvc \n\t" > > +#ifdef CONFIG_RISCV_ISA_C > > + "1: c.nop \n\t" > > +#else > > "1: nop \n\t" > > - " .option pop \n\t" > > +#endif > > " .pushsection __jump_table, \"aw\" \n\t" > > " .align " RISCV_LGPTR " \n\t" > > " .long 1b - ., %l[label] - . \n\t" > > @@ -39,11 +43,14 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, > > bool branch) > > { > > asm_volatile_goto( > > +#ifdef CONFIG_RISCV_ISA_C > > + "1: c.j %l[label] \n\t" > > +#else > > " .option push \n\t" > > " .option norelax \n\t" > > - " .option norvc \n\t" > > "1: jal zero, %l[label] \n\t" > > " .option pop \n\t" > > +#endif > > " .pushsection __jump_table, \"aw\" \n\t" > > " .align " RISCV_LGPTR " \n\t" > > " .long 1b - ., %l[label] - . \n\t" > > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c > > index e6694759dbd0..64a4e5df093d 100644 > > --- a/arch/riscv/kernel/jump_label.c > > +++ b/arch/riscv/kernel/jump_label.c > > @@ -11,21 +11,52 @@ > > #include <asm/bug.h> > > #include <asm/patch.h> > > > > +#ifdef CONFIG_RISCV_ISA_C > > +#define RISCV_INSN_C_NOP 0x0001U > > +#define RISCV_INSN_C_JAL 0xa001U > > This is C.J (i.e. JAL X0) not C.JAL (i.e. JAL RA, which is RV32-only > and not what you want since it clobbers RA). Sorry for the macro naming bug. I know I'm using c.j. 101 imm[11|4|9:8|10|6|7|3:1|5] 01 C.J 001 imm[11|4|9:8|10|6|7|3:1|5] 01 C.JAL (RV32) Here is the correction: -#define RISCV_INSN_C_JAL 0xa001U +#define RISCV_INSN_C_J 0xa001U > > Jess > > > +#else > > #define RISCV_INSN_NOP 0x00000013U > > #define RISCV_INSN_JAL 0x0000006fU > > +#endif > > > > void arch_jump_label_transform(struct jump_entry *entry, > > enum jump_label_type type) > > { > > void *addr = (void *)jump_entry_code(entry); > > +#ifdef CONFIG_RISCV_ISA_C > > + u16 insn; > > +#else > > u32 insn; > > +#endif > > > > if (type == JUMP_LABEL_JMP) { > > long offset = jump_entry_target(entry) - jump_entry_code(entry); > > +#ifdef CONFIG_RISCV_ISA_C > > + if (WARN_ON(offset & 1 || offset < -2048 || offset >= 2048)) > > + return; > > > > + /* > > + * 001 | imm[11|4|9:8|10|6|7|3:1|5] 01 - C.JAL > > + */ > > + insn = RISCV_INSN_C_JAL | > > + (((u16)offset & GENMASK(5, 5)) >> (5 - 2)) | > > + (((u16)offset & GENMASK(3, 1)) << (3 - 1)) | > > + (((u16)offset & GENMASK(7, 7)) >> (7 - 6)) | > > + (((u16)offset & GENMASK(6, 6)) << (7 - 6)) | > > + (((u16)offset & GENMASK(10, 10)) >> (10 - 8)) | > > + (((u16)offset & GENMASK(9, 8)) << (9 - 8)) | > > + (((u16)offset & GENMASK(4, 4)) << (11 - 4)) | > > + (((u16)offset & GENMASK(11, 11)) << (12 - 11)); > > + } else { > > + insn = RISCV_INSN_C_NOP; > > + } > > +#else > > if (WARN_ON(offset & 1 || offset < -524288 || offset >= 524288)) > > return; > > > > + /* > > + * imm[20|10:1|11|19:12] | rd | 1101111 - JAL > > + */ > > insn = RISCV_INSN_JAL | > > (((u32)offset & GENMASK(19, 12)) << (12 - 12)) | > > (((u32)offset & GENMASK(11, 11)) << (20 - 11)) | > > @@ -34,6 +65,7 @@ void arch_jump_label_transform(struct jump_entry *entry, > > } else { > > insn = RISCV_INSN_NOP; > > } > > +#endif > > > > mutex_lock(&text_mutex); > > patch_text_nosync(addr, &insn, sizeof(insn)); > > -- > > 2.36.1 > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv >
diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h index 38af2ec7b9bf..729991e8f782 100644 --- a/arch/riscv/include/asm/jump_label.h +++ b/arch/riscv/include/asm/jump_label.h @@ -18,6 +18,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool branch) { asm_volatile_goto( + " .align 2 \n\t" " .option push \n\t" " .option norelax \n\t" " .option norvc \n\t" @@ -39,6 +40,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch) { asm_volatile_goto( + " .align 2 \n\t" " .option push \n\t" " .option norelax \n\t" " .option norvc \n\t"