| Message ID | 4621EB3971C7BB9D+20250322045034.297491-1-wangyuli@uniontech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] MIPS: KGDB: Remove the useless "nop" | expand |
On Sat, 22 Mar 2025, WangYuli wrote: > The nop instruction surrounding "breakinst:\tbreak\n\t" appears to > serve no real purpose. "If it ain't broke, don't fix it" -- what problem are you trying to solve here? Saving 8 bytes of kernel text space? > Its introduction can be traced back to commit 51c6022fdb ("[PATCH] > MIPS update") within the Linux history tree [1]. This commit was > substantial, comprising 41010 lines, and provides no justification > for the insertion of this nop instruction. The actual origin is: commit e7c2a72e2680827d6a733931273a93461c0d8d1b Author: Ralf Baechle <ralf@linux-mips.org> Date: Tue Nov 14 08:00:00 1995 +0000 Import of Linux/MIPS 1.3.0 in the LMO tree and this is less than a year from the inception of the port while Ralf still bulk-imported MIPS changes on top of Linus's tree rather than maintaining them locally. This was still a couple of years before my days and we won't ever know more I'm afraid. > Based on the MIPS architecture specification, delay slots are only > present after jump instructions or MIPS1 load instructions. > Consequently, the nop here is not intended to satisfy a delay slot > requirement. There are more cases of delay slots in various earlier revisions of the MIPS architecture. But inline assembly uses the default reorder mode of assembly, which means no delay slot will ever span inside from preceding compiled code, and back in 1995 GCC did not itself schedule delay slots anyway, all compiled code used the reorder mode. > Thus, this instruction is suspicious and should probably be removed. Based on my experience these padding NOPs are likely there to ensure a clean state in the debug stub which may assign special meaning to other instructions present around the breakpoint instruction and they've been there for 30 years now, so I think they're best left alone unless you can name an actual problem they cause you. FAOD it's a NAK then. Let's better fix actual bugs instead. Maciej
diff --git a/arch/mips/kernel/kgdb.c b/arch/mips/kernel/kgdb.c index 09a2d7bb9eef..d6ccc7d2d34f 100644 --- a/arch/mips/kernel/kgdb.c +++ b/arch/mips/kernel/kgdb.c @@ -201,9 +201,7 @@ void arch_kgdb_breakpoint(void) __asm__ __volatile__( ".globl breakinst\n\t" ".set\tnoreorder\n\t" - "nop\n" "breakinst:\tbreak\n\t" - "nop\n\t" ".set\treorder"); }
The nop instruction surrounding "breakinst:\tbreak\n\t" appears to serve no real purpose. Its introduction can be traced back to commit 51c6022fdb ("[PATCH] MIPS update") within the Linux history tree [1]. This commit was substantial, comprising 41010 lines, and provides no justification for the insertion of this nop instruction. Based on the MIPS architecture specification, delay slots are only present after jump instructions or MIPS1 load instructions. Consequently, the nop here is not intended to satisfy a delay slot requirement. Thus, this instruction is suspicious and should probably be removed. [1]. https://web.git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git Cc: Maciej W. Rozycki <macro@orcam.me.uk> Cc: Huacai Chen <chenhuacai@kernel.org> Cc: WANG Xuerui <kernel@xen0n.name> Cc: Felix Yan <felixonmars@archlinux.org> Cc: Mingcong Bai <jeffbai@aosc.io> Signed-off-by: WangYuli <wangyuli@uniontech.com> --- NOTE: This patch is submitted as an RFC due to my incomplete knowledge of the extensive history of MIPS. The nop instruction in question may indeed have a valid reason for its existence, but its origins are likely too far in the past to easily ascertain. I would be grateful if anyone with relevant historical information could take the time to elaborate on the background, such as specific models or microarchitectures that might be impacted by this change. Thank you all very much for your assistance. --- arch/mips/kernel/kgdb.c | 2 -- 1 file changed, 2 deletions(-)