| Message ID | 330B3BAFC6FDB763+20250402074247.64483-1-wangyuli@uniontech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] riscv: KGDB: Do not inline arch_kgdb_breakpoint() | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | warning | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
Hi WangYuli, On 02/04/2025 09:42, WangYuli wrote: > The arch_kgdb_breakpoint() function defines the kgdb_compiled_break > symbol using inline assembly. > > There's a potential issue where the compiler might inline > arch_kgdb_breakpoint(), which would then define the kgdb_breakinst I guess you meant kgdb_compiled_break. > symbol multiple times, leading to fail to link vmlinux.o. > This isn't merely a potential compilation problem. The intent here > is to determine the global symbol address of kgdb_compiled_break, > and if this function is inlined multiple times, it would logically > be a grave error. > > Link: https://lore.kernel.org/all/4b4187c1-77e5-44b7-885f-d6826723dd9a@sifive.com/ > Fixes: fe89bd2be866 ("riscv: Add KGDB support") > Co-developed-by: Huacai Chen <chenhuacai@loongson.cn> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> > Signed-off-by: WangYuli <wangyuli@uniontech.com> > --- > Changelog: > *v1->v2: Add the missing __ASSEMBLY__ check and substitute > ".option rvc/norvc" with ".option push/pop". > --- > arch/riscv/include/asm/kgdb.h | 9 +-------- > arch/riscv/kernel/kgdb.c | 8 ++++++++ > 2 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/arch/riscv/include/asm/kgdb.h b/arch/riscv/include/asm/kgdb.h > index 46677daf708b..d9f6a8fc387f 100644 > --- a/arch/riscv/include/asm/kgdb.h > +++ b/arch/riscv/include/asm/kgdb.h > @@ -19,16 +19,9 @@ > > #ifndef __ASSEMBLY__ > > +extern void arch_kgdb_breakpoint(void); The "extern" is not needed here. > extern unsigned long kgdb_compiled_break; > > -static inline void arch_kgdb_breakpoint(void) > -{ > - asm(".global kgdb_compiled_break\n" > - ".option norvc\n" > - "kgdb_compiled_break: ebreak\n" > - ".option rvc\n"); > -} > - > #endif /* !__ASSEMBLY__ */ > > #define DBG_REG_ZERO "zero" > diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c > index 2e0266ae6bd7..5873d3970360 100644 > --- a/arch/riscv/kernel/kgdb.c > +++ b/arch/riscv/kernel/kgdb.c > @@ -254,6 +254,14 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc) > regs->epc = pc; > } > > +noinline void arch_kgdb_breakpoint(void) > +{ > + asm(".global kgdb_compiled_break\n" > + ".option push\n" Here you forgot .option norvc. But this fix as mentioned by Samuel should be in a separate patch. Thanks, Alex > + "kgdb_compiled_break: ebreak\n" > + ".option pop\n"); > +} > + > void kgdb_arch_handle_qxfer_pkt(char *remcom_in_buffer, > char *remcom_out_buffer) > {
On Thu, 03 Apr 2025 02:58:50 PDT (-0700), alex@ghiti.fr wrote: > Hi WangYuli, > > On 02/04/2025 09:42, WangYuli wrote: >> The arch_kgdb_breakpoint() function defines the kgdb_compiled_break >> symbol using inline assembly. >> >> There's a potential issue where the compiler might inline >> arch_kgdb_breakpoint(), which would then define the kgdb_breakinst > > > I guess you meant kgdb_compiled_break. > > >> symbol multiple times, leading to fail to link vmlinux.o. >> This isn't merely a potential compilation problem. The intent here >> is to determine the global symbol address of kgdb_compiled_break, >> and if this function is inlined multiple times, it would logically >> be a grave error. >> >> Link: https://lore.kernel.org/all/4b4187c1-77e5-44b7-885f-d6826723dd9a@sifive.com/ >> Fixes: fe89bd2be866 ("riscv: Add KGDB support") >> Co-developed-by: Huacai Chen <chenhuacai@loongson.cn> >> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> >> Signed-off-by: WangYuli <wangyuli@uniontech.com> >> --- >> Changelog: >> *v1->v2: Add the missing __ASSEMBLY__ check and substitute >> ".option rvc/norvc" with ".option push/pop". >> --- >> arch/riscv/include/asm/kgdb.h | 9 +-------- >> arch/riscv/kernel/kgdb.c | 8 ++++++++ >> 2 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/arch/riscv/include/asm/kgdb.h b/arch/riscv/include/asm/kgdb.h >> index 46677daf708b..d9f6a8fc387f 100644 >> --- a/arch/riscv/include/asm/kgdb.h >> +++ b/arch/riscv/include/asm/kgdb.h >> @@ -19,16 +19,9 @@ >> >> #ifndef __ASSEMBLY__ >> >> +extern void arch_kgdb_breakpoint(void); > > > The "extern" is not needed here. > > >> extern unsigned long kgdb_compiled_break; >> >> -static inline void arch_kgdb_breakpoint(void) >> -{ >> - asm(".global kgdb_compiled_break\n" >> - ".option norvc\n" >> - "kgdb_compiled_break: ebreak\n" >> - ".option rvc\n"); >> -} >> - >> #endif /* !__ASSEMBLY__ */ >> >> #define DBG_REG_ZERO "zero" >> diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c >> index 2e0266ae6bd7..5873d3970360 100644 >> --- a/arch/riscv/kernel/kgdb.c >> +++ b/arch/riscv/kernel/kgdb.c >> @@ -254,6 +254,14 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc) >> regs->epc = pc; >> } >> >> +noinline void arch_kgdb_breakpoint(void) >> +{ >> + asm(".global kgdb_compiled_break\n" >> + ".option push\n" > > > Here you forgot .option norvc. But this fix as mentioned by Samuel > should be in a separate patch. I don't think we actually need either here: we're just looking at the address of kgdb_compiled_break, so it's fine if it ends up as a c.ebreak. > Thanks, > > Alex > > >> + "kgdb_compiled_break: ebreak\n" >> + ".option pop\n"); >> +} >> + >> void kgdb_arch_handle_qxfer_pkt(char *remcom_in_buffer, >> char *remcom_out_buffer) >> {
diff --git a/arch/riscv/include/asm/kgdb.h b/arch/riscv/include/asm/kgdb.h index 46677daf708b..d9f6a8fc387f 100644 --- a/arch/riscv/include/asm/kgdb.h +++ b/arch/riscv/include/asm/kgdb.h @@ -19,16 +19,9 @@ #ifndef __ASSEMBLY__ +extern void arch_kgdb_breakpoint(void); extern unsigned long kgdb_compiled_break; -static inline void arch_kgdb_breakpoint(void) -{ - asm(".global kgdb_compiled_break\n" - ".option norvc\n" - "kgdb_compiled_break: ebreak\n" - ".option rvc\n"); -} - #endif /* !__ASSEMBLY__ */ #define DBG_REG_ZERO "zero" diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c index 2e0266ae6bd7..5873d3970360 100644 --- a/arch/riscv/kernel/kgdb.c +++ b/arch/riscv/kernel/kgdb.c @@ -254,6 +254,14 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc) regs->epc = pc; } +noinline void arch_kgdb_breakpoint(void) +{ + asm(".global kgdb_compiled_break\n" + ".option push\n" + "kgdb_compiled_break: ebreak\n" + ".option pop\n"); +} + void kgdb_arch_handle_qxfer_pkt(char *remcom_in_buffer, char *remcom_out_buffer) {
The arch_kgdb_breakpoint() function defines the kgdb_compiled_break symbol using inline assembly. There's a potential issue where the compiler might inline arch_kgdb_breakpoint(), which would then define the kgdb_breakinst symbol multiple times, leading to fail to link vmlinux.o. This isn't merely a potential compilation problem. The intent here is to determine the global symbol address of kgdb_compiled_break, and if this function is inlined multiple times, it would logically be a grave error. Link: https://lore.kernel.org/all/4b4187c1-77e5-44b7-885f-d6826723dd9a@sifive.com/ Fixes: fe89bd2be866 ("riscv: Add KGDB support") Co-developed-by: Huacai Chen <chenhuacai@loongson.cn> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> Signed-off-by: WangYuli <wangyuli@uniontech.com> --- Changelog: *v1->v2: Add the missing __ASSEMBLY__ check and substitute ".option rvc/norvc" with ".option push/pop". --- arch/riscv/include/asm/kgdb.h | 9 +-------- arch/riscv/kernel/kgdb.c | 8 ++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)