| Message ID | C5DCACA951B6211D+20250408095454.67390-1-wangyuli@uniontech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] 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 | success | 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 08/04/2025 11:54, 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 You forgot to replace kgdb_breakinst into 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: > 1. Add the missing __ASSEMBLY__ check and substitute > ".option rvc/norvc" with ".option push/pop". > v2->v3: > 1. Remove "extern". > 2. Restore the inadvertently deleted .option norvc to prevent > a change in semantics. > --- > arch/riscv/include/asm/kgdb.h | 9 +-------- > arch/riscv/kernel/kgdb.c | 9 +++++++++ > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/arch/riscv/include/asm/kgdb.h b/arch/riscv/include/asm/kgdb.h > index 46677daf708b..cc11c4544cff 100644 > --- a/arch/riscv/include/asm/kgdb.h > +++ b/arch/riscv/include/asm/kgdb.h > @@ -19,16 +19,9 @@ > > #ifndef __ASSEMBLY__ > > +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..7f2d3d956167 100644 > --- a/arch/riscv/kernel/kgdb.c > +++ b/arch/riscv/kernel/kgdb.c > @@ -254,6 +254,15 @@ 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" > + ".option norvc\n" > + "kgdb_compiled_break: ebreak\n" > + ".option pop\n"); > +} You are fixing 2 things here, you need to split this patch into 2. And as noted by Palmer, we actually don't need norvc here, so you can remove it instead. Thanks, Alex > + > void kgdb_arch_handle_qxfer_pkt(char *remcom_in_buffer, > char *remcom_out_buffer) > {
Hi Alex On 2025/4/10 21:42, Alexandre Ghiti wrote: > Hi WangYuli, > > > You forgot to replace kgdb_breakinst into kgdb_compiled_break. OK, I'll fix it. Thanks, > > You are fixing 2 things here, you need to split this patch into 2. OK, > And as noted by Palmer, we actually don't need norvc here, so you can > remove it instead. I have some questions regarding this. If .option norvc is unnecessary, what is the significance of .option push/.option pop? Should they also be removed as well? However, will this still function properly when the RISC-V C extension is active? Thanks,
Hi WangYuli, On 10/04/2025 16:28, WangYuli wrote: > Hi Alex > > On 2025/4/10 21:42, Alexandre Ghiti wrote: >> Hi WangYuli, >> >> >> You forgot to replace kgdb_breakinst into kgdb_compiled_break. > > OK, I'll fix it. > > Thanks, > >> >> You are fixing 2 things here, you need to split this patch into 2. > OK, >> And as noted by Palmer, we actually don't need norvc here, so you can >> remove it instead. > > I have some questions regarding this. > > If .option norvc is unnecessary, what is the significance of .option > push/.option pop? > > Should they also be removed as well? Yes, you can remove them. > > However, will this still function properly when the RISC-V C extension > is active? .option norvc is used to prevent the assembler from using compressed instructions, but it's generally used when we need to ensure the size of the instructions that are used, which is not the case here as noted by Palmer since we only care about the address. So yes it will work fine with C enabled :) > > > Thanks, > Thanks for the changes! Alex
diff --git a/arch/riscv/include/asm/kgdb.h b/arch/riscv/include/asm/kgdb.h index 46677daf708b..cc11c4544cff 100644 --- a/arch/riscv/include/asm/kgdb.h +++ b/arch/riscv/include/asm/kgdb.h @@ -19,16 +19,9 @@ #ifndef __ASSEMBLY__ +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..7f2d3d956167 100644 --- a/arch/riscv/kernel/kgdb.c +++ b/arch/riscv/kernel/kgdb.c @@ -254,6 +254,15 @@ 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" + ".option norvc\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: 1. Add the missing __ASSEMBLY__ check and substitute ".option rvc/norvc" with ".option push/pop". v2->v3: 1. Remove "extern". 2. Restore the inadvertently deleted .option norvc to prevent a change in semantics. --- arch/riscv/include/asm/kgdb.h | 9 +-------- arch/riscv/kernel/kgdb.c | 9 +++++++++ 2 files changed, 10 insertions(+), 8 deletions(-)