xen/arch/x86/boot/x86_64.S | 6 ++---- xen/arch/x86/include/asm/asm-defns.h | 9 +++++++++ xen/arch/x86/x86_64/entry.S | 12 ++++-------- 3 files changed, 15 insertions(+), 12 deletions(-)
It was previously noted that CALL/BUG is a weird combination, but there is
good reason to use this pattern.
Introduce an explicit tailcall macro make it clearer in context.
No functional change.
Reported-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
It would be nicer if tailcall was shorter, but that loses clarity. RISC-V has
'tail' as an alias for 'b', but that looses the call aspect, and tcall isn't
sufficiently recognisable as tailcall IMO.
---
xen/arch/x86/boot/x86_64.S | 6 ++----
xen/arch/x86/include/asm/asm-defns.h | 9 +++++++++
xen/arch/x86/x86_64/entry.S | 12 ++++--------
3 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 5d12937a0e40..04bb62ae8680 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -74,14 +74,12 @@ ENTRY(__high_start)
.L_ap_cet_done:
#endif /* CONFIG_XEN_SHSTK || CONFIG_XEN_IBT */
- call start_secondary
- BUG /* start_secondary() shouldn't return. */
+ tailcall start_secondary
.L_bsp:
/* Pass off the Multiboot info structure to C land (if applicable). */
mov multiboot_ptr(%rip),%edi
- call __start_xen
- BUG /* __start_xen() shouldn't return. */
+ tailcall __start_xen
.section .data.page_aligned, "aw", @progbits
.align PAGE_SIZE, 0
diff --git a/xen/arch/x86/include/asm/asm-defns.h b/xen/arch/x86/include/asm/asm-defns.h
index 8bd9007731d5..9a7073ced5be 100644
--- a/xen/arch/x86/include/asm/asm-defns.h
+++ b/xen/arch/x86/include/asm/asm-defns.h
@@ -20,6 +20,15 @@
.byte 0x0f, 0x01, 0xdd
.endm
+/*
+ * Call a noreturn function. This could be JMP, but CALL results in a more
+ * helpful backtrace. BUG is to catch functions which do decide to return...
+ */
+.macro tailcall fn:req
+ call \fn
+ BUG /* Shouldn't return */
+.endm
+
.macro INDIRECT_BRANCH insn:req arg:req
/*
* Create an indirect branch. insn is one of call/jmp, arg is a single
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 8b77d7113bbf..bca1500e2b45 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -824,8 +824,7 @@ handle_exception_saved:
DISPATCH(X86_EXC_CP, do_entry_CP)
#undef DISPATCH
- call do_unhandled_trap
- BUG /* do_unhandled_trap() shouldn't return. */
+ tailcall do_unhandled_trap
.L_exn_dispatch_done:
mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
@@ -880,8 +879,7 @@ exception_with_ints_disabled:
FATAL_exception_with_ints_disabled:
xorl %esi,%esi
movq %rsp,%rdi
- call fatal_trap
- BUG /* fatal_trap() shouldn't return. */
+ tailcall fatal_trap
ENTRY(divide_error)
ENDBR64
@@ -989,8 +987,7 @@ ENTRY(double_fault)
.Ldblf_cr3_okay:
movq %rsp,%rdi
- call do_double_fault
- BUG /* do_double_fault() shouldn't return. */
+ tailcall do_double_fault
ENTRY(nmi)
ENDBR64
@@ -1085,8 +1082,7 @@ handle_ist_exception:
DISPATCH(X86_EXC_MC, do_machine_check)
#undef DISPATCH
- call do_unhandled_trap
- BUG /* do_unhandled_trap() shouldn't return. */
+ tailcall do_unhandled_trap
.L_ist_dispatch_done:
mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
base-commit: f51e5d8eae8ece77a949571f39ee49904f3129aa
--
2.30.2
On 30.06.2023 17:20, Andrew Cooper wrote: > It was previously noted that CALL/BUG is a weird combination, but there is > good reason to use this pattern. > > Introduce an explicit tailcall macro make it clearer in context. > > No functional change. > > Reported-by: Jan Beulich <JBeulich@suse.com> Did I? Must have been a long time back, as I don't think I remember us talking about this. > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > > It would be nicer if tailcall was shorter, but that loses clarity. RISC-V has > 'tail' as an alias for 'b', but that looses the call aspect, and tcall isn't > sufficiently recognisable as tailcall IMO. I agree with all aspects you mention here. Jan
On 04/07/2023 3:29 pm, Jan Beulich wrote: > On 30.06.2023 17:20, Andrew Cooper wrote: >> It was previously noted that CALL/BUG is a weird combination, but there is >> good reason to use this pattern. >> >> Introduce an explicit tailcall macro make it clearer in context. >> >> No functional change. >> >> Reported-by: Jan Beulich <JBeulich@suse.com> > Did I? Must have been a long time back, as I don't think I remember us > talking about this. This was discussed on multiple patch reviews, where I was introducing the pattern and you were complaining about the BUG and comment. I can drop the tag if you'd prefer. >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks ~Andrew
On 04.07.2023 19:04, Andrew Cooper wrote: > On 04/07/2023 3:29 pm, Jan Beulich wrote: >> On 30.06.2023 17:20, Andrew Cooper wrote: >>> It was previously noted that CALL/BUG is a weird combination, but there is >>> good reason to use this pattern. >>> >>> Introduce an explicit tailcall macro make it clearer in context. >>> >>> No functional change. >>> >>> Reported-by: Jan Beulich <JBeulich@suse.com> >> Did I? Must have been a long time back, as I don't think I remember us >> talking about this. > > This was discussed on multiple patch reviews, where I was introducing > the pattern and you were complaining about the BUG and comment. I can > drop the tag if you'd prefer. I'll leave that up to you. As said, it must have been quite some time back. Jan
© 2016 - 2024 Red Hat, Inc.