Re: [PATCH] KVM: arm64: Disable LTO in hyp

On Fri, 05 Mar 2021 02:38:17 +0000,
Sami Tolvanen [off-list ref] wrote:

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

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  004b56e4

Note 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:
 	// 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

which is completely untested. I wouldn't be surprised if there were
more of these somewhere.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel