| Message ID | 1552657145-10036-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: drop the nop() macro | expand |
On Fri, Mar 15, 2019 at 01:39:05PM +0000, Andrew Cooper wrote: > There isn't a plausible reason to insert nops like this into code. > > The sole use is in do_debug_key(), and exists to prevent the compiler > optimising the tail of the function with 'jmp debugger_trap_fatal' > > In practice, a compiler barrier suffices just as well to prevent the tailcall, > and doesn't involve inserting unnecessary instructions. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>>> On 15.03.19 at 14:41, <wei.liu2@citrix.com> wrote: > On Fri, Mar 15, 2019 at 01:39:05PM +0000, Andrew Cooper wrote: >> There isn't a plausible reason to insert nops like this into code. >> >> The sole use is in do_debug_key(), and exists to prevent the compiler >> optimising the tail of the function with 'jmp debugger_trap_fatal' >> >> In practice, a compiler barrier suffices just as well to prevent the tailcall, >> and doesn't involve inserting unnecessary instructions. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Reviewed-by: Wei Liu <wei.liu2@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com>
Hi Andrew, On 15/03/2019 13:39, Andrew Cooper wrote: > There isn't a plausible reason to insert nops like this into code. > > The sole use is in do_debug_key(), and exists to prevent the compiler > optimising the tail of the function with 'jmp debugger_trap_fatal' > > In practice, a compiler barrier suffices just as well to prevent the tailcall, > and doesn't involve inserting unnecessary instructions. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Julien Grall <julien.grall@arm.com> Cheers,
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c index c25a30e..4f4a660 100644 --- a/xen/common/keyhandler.c +++ b/xen/common/keyhandler.c @@ -480,9 +480,9 @@ static void do_debug_key(unsigned char key, struct cpu_user_regs *regs) { printk("'%c' pressed -> trapping into debugger\n", key); (void)debugger_trap_fatal(0xf001, regs); - nop(); /* Prevent the compiler doing tail call - optimisation, as that confuses xendbg a - bit. */ + + /* Prevent tail call optimisation, which confuses xendbg. */ + barrier(); } static void do_toggle_alt_key(unsigned char key, struct cpu_user_regs *regs) diff --git a/xen/include/asm-arm/system.h b/xen/include/asm-arm/system.h index b94e56f..e5d0626 100644 --- a/xen/include/asm-arm/system.h +++ b/xen/include/asm-arm/system.h @@ -5,9 +5,6 @@ #include <xen/lib.h> #include <public/arch-arm.h> -#define nop() \ - asm volatile ( "nop" ) - #define sev() asm volatile("sev" : : : "memory") #define wfe() asm volatile("wfe" : : : "memory") #define wfi() asm volatile("wfi" : : : "memory") diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h index 483cd20..c665499 100644 --- a/xen/include/asm-x86/system.h +++ b/xen/include/asm-x86/system.h @@ -17,9 +17,6 @@ #define clflush(a) \ asm volatile ( "clflush (%0)" : : "r"(a) ) -#define nop() \ - asm volatile ( "nop" ) - #define xchg(ptr,v) \ ((__typeof__(*(ptr)))__xchg((unsigned long)(v),(ptr),sizeof(*(ptr))))
There isn't a plausible reason to insert nops like this into code. The sole use is in do_debug_key(), and exists to prevent the compiler optimising the tail of the function with 'jmp debugger_trap_fatal' In practice, a compiler barrier suffices just as well to prevent the tailcall, and doesn't involve inserting unnecessary instructions. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> To preempt review questions, the (void) cast can't be dropped without breaking the ARM build. I tried cleaning up the debugger infrastructure a little (in particular to put this debugkey behind CONFIG_CRASH_DEBUG) but it is more than a 5 minute job to disentangle, and I don't have time. --- xen/common/keyhandler.c | 6 +++--- xen/include/asm-arm/system.h | 3 --- xen/include/asm-x86/system.h | 3 --- 3 files changed, 3 insertions(+), 9 deletions(-)