Re: [PATCH] KVM: arm64: Disable LTO in hyp
From: Sami Tolvanen <samitolvanen@google.com>
Date: 2021-03-05 16:56:32
Also in:
kvmarm, lkml
Subsystem:
arm64 port (aarch64 architecture), kernel virtual machine for arm64 (kvm/arm64), the rest · Maintainers:
Catalin Marinas, Will Deacon, Marc Zyngier, Oliver Upton, Linus Torvalds
On Fri, Mar 5, 2021 at 6:22 AM Ard Biesheuvel [off-list ref] wrote:
On Fri, 5 Mar 2021 at 12:38, Marc Zyngier [off-list ref] wrote:quoted
On Fri, 05 Mar 2021 02:38:17 +0000, Sami Tolvanen [off-list ref] wrote:quoted
On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen [off-list ref] wrote:quoted
On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier [off-list ref] wrote:quoted
On Thu, 04 Mar 2021 21:25:41 +0000, Sami Tolvanen [off-list ref] wrote:[...]quoted
quoted
quoted
quoted
I assume hyp_panic() ends up being placed too far from __guest_enter() when the kernel is large enough. Possibly something to do with LLVM always splitting functions into separate sections with LTO. I'm not sure why the linker cannot shuffle things around to make everyone happy in this case, but I confirmed that this patch also fixes the build issue for me:diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c index af8e940d0f03..128197b7c794 100644 --- a/arch/arm64/kvm/hyp/vhe/switch.c +++ b/arch/arm64/kvm/hyp/vhe/switch.c@@ -214,7 +214,7 @@ static void __hyp_call_panic(u64 spsr, u64 elr, u64 par) } NOKPROBE_SYMBOL(__hyp_call_panic); -void __noreturn hyp_panic(void) +void __noreturn hyp_panic(void) __section(".text") { u64 spsr = read_sysreg_el2(SYS_SPSR); u64 elr = read_sysreg_el2(SYS_ELR);We're getting into black-magic territory here. Why wouldn't hyp_panic be in the .text section already?It's not quite black magic. LLVM essentially flips on -ffunction-sections with LTO and therefore, hyp_panic() will be in .text.hyp_panic in vmlinux.o, while __guest_enter() will be in .text. Everything ends up in .text when we link vmlinux, of course. $ readelf --sections vmlinux.o | grep hyp_panic [3936] .text.hyp_panic PROGBITS 0000000000000000 004b56e4Note that disabling LTO here has essentially the same effect as using __section(".text"). It stops the compiler from splitting these functions into .text.* sections and makes it less likely that hyp_panic() ends up too far away from __guest_enter(). If neither of these workarounds sound appealing, I suppose we could alternatively change hyp/entry.S to use adr_l for hyp_panic. Thoughts?That would be an actual fix instead of a workaround, as it would remove existing assumptions about the relative locations of the two objects. I guess you need to fix both instances with something such as:diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S index b0afad7a99c6..a43e1f7ee354 100644 --- a/arch/arm64/kvm/hyp/entry.S +++ b/arch/arm64/kvm/hyp/entry.S@@ -85,8 +85,10 @@ SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL) // If the hyp context is loaded, go straight to hyp_panic get_loaded_vcpu x0, x1 - cbz x0, hyp_panic - + cbnz x0, 1f + adr_l x0, hyp_panic + br x0 +1:Agree with replacing the conditional branches that refer to external symbols: the compiler never emits those, for the reason we are seeing here, i.e., the range is simply insufficient. But let's just use 'b hyp_panic' instead, no?
Alright, this seems to work for me:
diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index b0afad7a99c6..c62265951467 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S@@ -85,8 +85,10 @@ SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL) // If the hyp context is loaded, go straight to hyp_panic get_loaded_vcpu x0, x1 - cbz x0, hyp_panic + cbnz x0, 1f + b hyp_panic +1: // The hyp context is saved so make sure it is restored to allow // hyp_panic to run at hyp and, subsequently, panic to run in the host. // This makes use of __guest_exit to avoid duplication but sets the
@@ -94,7 +96,7 @@ SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL) // current state is saved to the guest context but it will only be // accurate if the guest had been completely restored. adr_this_cpu x0, kvm_hyp_ctxt, x1 - adr x1, hyp_panic + adr_l x1, hyp_panic str x1, [x0, #CPU_XREG_OFFSET(30)] get_vcpu_ptr x1, x0
But when I say work, I mean this fixes the allmodconfig build with LTO, and my kernel boots at EL2. I don't actually have a way to properly test KVM on arm64. If nobody sees obvious issues here, I can send a proper patch a bit later.
quoted
// The hyp context is saved so make sure it is restored to allow // hyp_panic to run at hyp and, subsequently, panic to run in the host. // This makes use of __guest_exit to avoid duplication but sets the@@ -94,7 +96,7 @@ SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL) // current state is saved to the guest context but it will only be // accurate if the guest had been completely restored. adr_this_cpu x0, kvm_hyp_ctxt, x1 - adr x1, hyp_panic + adr_l x1, hyp_panic str x1, [x0, #CPU_XREG_OFFSET(30)] get_vcpu_ptr x1, x0which is completely untested. I wouldn't be surprised if there were more of these somewhere.A quick grep gives me $ objdump -r vmlinux.o |grep BR19 000000000005b6e0 R_AARCH64_CONDBR19 hyp_panic 0000000000418e08 R_AARCH64_CONDBR19 __memcpy 0000000000418e14 R_AARCH64_CONDBR19 __memcpy
These two are in __memmove. This shouldn't be an issue, at least with LTO.
0000000000003818 R_AARCH64_CONDBR19 __kvm_nvhe___guest_exit_panic 0000000000003898 R_AARCH64_CONDBR19 __kvm_nvhe___guest_exit_panic 0000000000003918 R_AARCH64_CONDBR19 __kvm_nvhe___guest_exit_panic 0000000000003998 R_AARCH64_CONDBR19 __kvm_nvhe___guest_exit_panic 0000000000003a18 R_AARCH64_CONDBR19 __kvm_nvhe___guest_exit_panic 0000000000003a98 R_AARCH64_CONDBR19 __kvm_nvhe___guest_exit_panic 0000000000003b18 R_AARCH64_CONDBR19 __kvm_nvhe___guest_exit_panic 0000000000003b98 R_AARCH64_CONDBR19 __kvm_nvhe___guest_exit_panic 0000000000003c10 R_AARCH64_CONDBR19 __kvm_nvhe___host_exit 0000000000003c1c R_AARCH64_CONDBR19 __kvm_nvhe___host_exit 00000000000064f0 R_AARCH64_CONDBR19 __kvm_nvhe_hyp_panic 000000000000078c R_AARCH64_CONDBR19 __kvm_nvhe___kvm_handle_stub_hvc
It looks like all the nvhe code is linked together into kvm_nvhe.o, so I don't think these will cause problems either. Sami _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel