* [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 18:45 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-04 18:45 UTC (permalink / raw)
To: Marc Zyngier, James Morse
Cc: Nathan Chancellor, Kees Cook, Julien Thierry, Suzuki K Poulose,
Catalin Marinas, Will Deacon, linux-arm-kernel, kvmarm,
linux-kernel, Sami Tolvanen
allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
linker errors:
ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
[-1048576, 1048575]; references hyp_panic
>>> defined in vmlinux.o
ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
[-1048576, 1048575]; references hyp_panic
>>> defined in vmlinux.o
As LTO is not really necessary for the hypervisor code, disable it for
the hyp directory to fix the build.
Link: https://github.com/ClangBuiltLinux/linux/issues/1317
Reported-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
arch/arm64/kvm/hyp/Makefile | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
index 687598e41b21..e8116016e6a8 100644
--- a/arch/arm64/kvm/hyp/Makefile
+++ b/arch/arm64/kvm/hyp/Makefile
@@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
$(DISABLE_STACKLEAK_PLUGIN)
obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
+
+# Disable LTO for the files in this directory
+KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
--
2.30.1.766.gb4fecdf3b7-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 18:45 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-04 18:45 UTC (permalink / raw)
To: Marc Zyngier, James Morse
Cc: Kees Cook, Catalin Marinas, linux-kernel, Nathan Chancellor,
linux-arm-kernel, Sami Tolvanen, Will Deacon, kvmarm
allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
linker errors:
ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
[-1048576, 1048575]; references hyp_panic
>>> defined in vmlinux.o
ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
[-1048576, 1048575]; references hyp_panic
>>> defined in vmlinux.o
As LTO is not really necessary for the hypervisor code, disable it for
the hyp directory to fix the build.
Link: https://github.com/ClangBuiltLinux/linux/issues/1317
Reported-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
arch/arm64/kvm/hyp/Makefile | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
index 687598e41b21..e8116016e6a8 100644
--- a/arch/arm64/kvm/hyp/Makefile
+++ b/arch/arm64/kvm/hyp/Makefile
@@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
$(DISABLE_STACKLEAK_PLUGIN)
obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
+
+# Disable LTO for the files in this directory
+KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
--
2.30.1.766.gb4fecdf3b7-goog
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 18:45 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-04 18:45 UTC (permalink / raw)
To: Marc Zyngier, James Morse
Cc: Nathan Chancellor, Kees Cook, Julien Thierry, Suzuki K Poulose,
Catalin Marinas, Will Deacon, linux-arm-kernel, kvmarm,
linux-kernel, Sami Tolvanen
allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
linker errors:
ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
[-1048576, 1048575]; references hyp_panic
>>> defined in vmlinux.o
ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
[-1048576, 1048575]; references hyp_panic
>>> defined in vmlinux.o
As LTO is not really necessary for the hypervisor code, disable it for
the hyp directory to fix the build.
Link: https://github.com/ClangBuiltLinux/linux/issues/1317
Reported-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
arch/arm64/kvm/hyp/Makefile | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
index 687598e41b21..e8116016e6a8 100644
--- a/arch/arm64/kvm/hyp/Makefile
+++ b/arch/arm64/kvm/hyp/Makefile
@@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
$(DISABLE_STACKLEAK_PLUGIN)
obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
+
+# Disable LTO for the files in this directory
+KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
--
2.30.1.766.gb4fecdf3b7-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
2021-03-04 18:45 ` Sami Tolvanen
(?)
@ 2021-03-04 19:15 ` Marc Zyngier
-1 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-04 19:15 UTC (permalink / raw)
To: Sami Tolvanen
Cc: James Morse, Nathan Chancellor, Kees Cook, Julien Thierry,
Suzuki K Poulose, Catalin Marinas, Will Deacon, linux-arm-kernel,
kvmarm, linux-kernel
On Thu, 04 Mar 2021 18:45:44 +0000,
Sami Tolvanen <samitolvanen@google.com> wrote:
>
> allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> linker errors:
>
> ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
I assume this message is only an oddity, right? Because
__guest_enter() is as far as you can imagine from irqbypass.c...
> relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> [-1048576, 1048575]; references hyp_panic
> >>> defined in vmlinux.o
>
> ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> [-1048576, 1048575]; references hyp_panic
> >>> defined in vmlinux.o
>
> As LTO is not really necessary for the hypervisor code, disable it for
> the hyp directory to fix the build.
Can you shed some light on what the problem is exactly?
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1317
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Tested-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
> arch/arm64/kvm/hyp/Makefile | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 687598e41b21..e8116016e6a8 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
> $(DISABLE_STACKLEAK_PLUGIN)
>
> obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
> +
> +# Disable LTO for the files in this directory
> +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
>
> base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
Can this be reduced to the nvhe part of the tree? The rest of the
hypervisor should support being built with LTO, I'd expect. Or am I
missing something more significant?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 19:15 ` Marc Zyngier
0 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-04 19:15 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Kees Cook, Catalin Marinas, linux-kernel, Nathan Chancellor,
linux-arm-kernel, Will Deacon, kvmarm
On Thu, 04 Mar 2021 18:45:44 +0000,
Sami Tolvanen <samitolvanen@google.com> wrote:
>
> allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> linker errors:
>
> ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
I assume this message is only an oddity, right? Because
__guest_enter() is as far as you can imagine from irqbypass.c...
> relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> [-1048576, 1048575]; references hyp_panic
> >>> defined in vmlinux.o
>
> ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> [-1048576, 1048575]; references hyp_panic
> >>> defined in vmlinux.o
>
> As LTO is not really necessary for the hypervisor code, disable it for
> the hyp directory to fix the build.
Can you shed some light on what the problem is exactly?
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1317
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Tested-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
> arch/arm64/kvm/hyp/Makefile | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 687598e41b21..e8116016e6a8 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
> $(DISABLE_STACKLEAK_PLUGIN)
>
> obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
> +
> +# Disable LTO for the files in this directory
> +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
>
> base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
Can this be reduced to the nvhe part of the tree? The rest of the
hypervisor should support being built with LTO, I'd expect. Or am I
missing something more significant?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 19:15 ` Marc Zyngier
0 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-04 19:15 UTC (permalink / raw)
To: Sami Tolvanen
Cc: James Morse, Nathan Chancellor, Kees Cook, Julien Thierry,
Suzuki K Poulose, Catalin Marinas, Will Deacon, linux-arm-kernel,
kvmarm, linux-kernel
On Thu, 04 Mar 2021 18:45:44 +0000,
Sami Tolvanen <samitolvanen@google.com> wrote:
>
> allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> linker errors:
>
> ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
I assume this message is only an oddity, right? Because
__guest_enter() is as far as you can imagine from irqbypass.c...
> relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> [-1048576, 1048575]; references hyp_panic
> >>> defined in vmlinux.o
>
> ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> [-1048576, 1048575]; references hyp_panic
> >>> defined in vmlinux.o
>
> As LTO is not really necessary for the hypervisor code, disable it for
> the hyp directory to fix the build.
Can you shed some light on what the problem is exactly?
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1317
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Tested-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
> arch/arm64/kvm/hyp/Makefile | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 687598e41b21..e8116016e6a8 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
> $(DISABLE_STACKLEAK_PLUGIN)
>
> obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
> +
> +# Disable LTO for the files in this directory
> +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
>
> base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
Can this be reduced to the nvhe part of the tree? The rest of the
hypervisor should support being built with LTO, I'd expect. Or am I
missing something more significant?
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
2021-03-04 19:15 ` Marc Zyngier
(?)
@ 2021-03-04 21:25 ` Sami Tolvanen
-1 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-04 21:25 UTC (permalink / raw)
To: Marc Zyngier, Fangrui Song, Nick Desaulniers
Cc: James Morse, Nathan Chancellor, Kees Cook, Julien Thierry,
Suzuki K Poulose, Catalin Marinas, Will Deacon, linux-arm-kernel,
kvmarm, LKML
On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 04 Mar 2021 18:45:44 +0000,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > linker errors:
> >
> > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
>
> I assume this message is only an oddity, right? Because
> __guest_enter() is as far as you can imagine from irqbypass.c...
I'm not sure what's up with the filename in the error message. Fangrui
or Nick probably have a better idea.
> > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > [-1048576, 1048575]; references hyp_panic
> > >>> defined in vmlinux.o
> >
> > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > [-1048576, 1048575]; references hyp_panic
> > >>> defined in vmlinux.o
> >
> > As LTO is not really necessary for the hypervisor code, disable it for
> > the hyp directory to fix the build.
>
> Can you shed some light on what the problem is exactly?
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);
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1317
> > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > Tested-by: Nathan Chancellor <nathan@kernel.org>
> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > ---
> > arch/arm64/kvm/hyp/Makefile | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> > index 687598e41b21..e8116016e6a8 100644
> > --- a/arch/arm64/kvm/hyp/Makefile
> > +++ b/arch/arm64/kvm/hyp/Makefile
> > @@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
> > $(DISABLE_STACKLEAK_PLUGIN)
> >
> > obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
> > +
> > +# Disable LTO for the files in this directory
> > +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
> >
> > base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
>
> Can this be reduced to the nvhe part of the tree? The rest of the
> hypervisor should support being built with LTO, I'd expect. Or am I
> missing something more significant?
No, this error appears to be about hyp_panic() in the vhe code. While
I'm not sure how beneficial LTO is in hypervisor code, there shouldn't
be any other reason we can't use it there.
Sami
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 21:25 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-04 21:25 UTC (permalink / raw)
To: Marc Zyngier, Fangrui Song, Nick Desaulniers
Cc: Kees Cook, Catalin Marinas, LKML, Nathan Chancellor,
linux-arm-kernel, Will Deacon, kvmarm
On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 04 Mar 2021 18:45:44 +0000,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > linker errors:
> >
> > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
>
> I assume this message is only an oddity, right? Because
> __guest_enter() is as far as you can imagine from irqbypass.c...
I'm not sure what's up with the filename in the error message. Fangrui
or Nick probably have a better idea.
> > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > [-1048576, 1048575]; references hyp_panic
> > >>> defined in vmlinux.o
> >
> > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > [-1048576, 1048575]; references hyp_panic
> > >>> defined in vmlinux.o
> >
> > As LTO is not really necessary for the hypervisor code, disable it for
> > the hyp directory to fix the build.
>
> Can you shed some light on what the problem is exactly?
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);
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1317
> > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > Tested-by: Nathan Chancellor <nathan@kernel.org>
> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > ---
> > arch/arm64/kvm/hyp/Makefile | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> > index 687598e41b21..e8116016e6a8 100644
> > --- a/arch/arm64/kvm/hyp/Makefile
> > +++ b/arch/arm64/kvm/hyp/Makefile
> > @@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
> > $(DISABLE_STACKLEAK_PLUGIN)
> >
> > obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
> > +
> > +# Disable LTO for the files in this directory
> > +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
> >
> > base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
>
> Can this be reduced to the nvhe part of the tree? The rest of the
> hypervisor should support being built with LTO, I'd expect. Or am I
> missing something more significant?
No, this error appears to be about hyp_panic() in the vhe code. While
I'm not sure how beneficial LTO is in hypervisor code, there shouldn't
be any other reason we can't use it there.
Sami
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 21:25 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-04 21:25 UTC (permalink / raw)
To: Marc Zyngier, Fangrui Song, Nick Desaulniers
Cc: James Morse, Nathan Chancellor, Kees Cook, Julien Thierry,
Suzuki K Poulose, Catalin Marinas, Will Deacon, linux-arm-kernel,
kvmarm, LKML
On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 04 Mar 2021 18:45:44 +0000,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > linker errors:
> >
> > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
>
> I assume this message is only an oddity, right? Because
> __guest_enter() is as far as you can imagine from irqbypass.c...
I'm not sure what's up with the filename in the error message. Fangrui
or Nick probably have a better idea.
> > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > [-1048576, 1048575]; references hyp_panic
> > >>> defined in vmlinux.o
> >
> > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > [-1048576, 1048575]; references hyp_panic
> > >>> defined in vmlinux.o
> >
> > As LTO is not really necessary for the hypervisor code, disable it for
> > the hyp directory to fix the build.
>
> Can you shed some light on what the problem is exactly?
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);
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1317
> > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > Tested-by: Nathan Chancellor <nathan@kernel.org>
> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > ---
> > arch/arm64/kvm/hyp/Makefile | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> > index 687598e41b21..e8116016e6a8 100644
> > --- a/arch/arm64/kvm/hyp/Makefile
> > +++ b/arch/arm64/kvm/hyp/Makefile
> > @@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
> > $(DISABLE_STACKLEAK_PLUGIN)
> >
> > obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
> > +
> > +# Disable LTO for the files in this directory
> > +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
> >
> > base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
>
> Can this be reduced to the nvhe part of the tree? The rest of the
> hypervisor should support being built with LTO, I'd expect. Or am I
> missing something more significant?
No, this error appears to be about hyp_panic() in the vhe code. While
I'm not sure how beneficial LTO is in hypervisor code, there shouldn't
be any other reason we can't use it there.
Sami
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
2021-03-04 21:25 ` Sami Tolvanen
(?)
@ 2021-03-04 22:17 ` Marc Zyngier
-1 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-04 22:17 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Fangrui Song, Nick Desaulniers, James Morse, Nathan Chancellor,
Kees Cook, Julien Thierry, Suzuki K Poulose, Catalin Marinas,
Will Deacon, linux-arm-kernel, kvmarm, LKML
On Thu, 04 Mar 2021 21:25:41 +0000,
Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 04 Mar 2021 18:45:44 +0000,
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > > linker errors:
> > >
> > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
> >
> > I assume this message is only an oddity, right? Because
> > __guest_enter() is as far as you can imagine from irqbypass.c...
>
> I'm not sure what's up with the filename in the error message. Fangrui
> or Nick probably have a better idea.
>
> > > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > > [-1048576, 1048575]; references hyp_panic
> > > >>> defined in vmlinux.o
> > >
> > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > > [-1048576, 1048575]; references hyp_panic
> > > >>> defined in vmlinux.o
> > >
> > > As LTO is not really necessary for the hypervisor code, disable it for
> > > the hyp directory to fix the build.
> >
> > Can you shed some light on what the problem is exactly?
>
> 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?
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1317
> > > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > > Tested-by: Nathan Chancellor <nathan@kernel.org>
> > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > > ---
> > > arch/arm64/kvm/hyp/Makefile | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> > > index 687598e41b21..e8116016e6a8 100644
> > > --- a/arch/arm64/kvm/hyp/Makefile
> > > +++ b/arch/arm64/kvm/hyp/Makefile
> > > @@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
> > > $(DISABLE_STACKLEAK_PLUGIN)
> > >
> > > obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
> > > +
> > > +# Disable LTO for the files in this directory
> > > +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
> > >
> > > base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
> >
> > Can this be reduced to the nvhe part of the tree? The rest of the
> > hypervisor should support being built with LTO, I'd expect. Or am I
> > missing something more significant?
>
> No, this error appears to be about hyp_panic() in the vhe code. While
> I'm not sure how beneficial LTO is in hypervisor code, there shouldn't
> be any other reason we can't use it there.
The VHE part of the hypervisor is the kernel itself, and not any
different from the rest of the code. It should be able to benefit from
LTO. On the contrary, the nVHE part is what could do without LTO,
given that it is essentially a separate object that happens to be
bundled with the kernel.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 22:17 ` Marc Zyngier
0 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-04 22:17 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Kees Cook, Fangrui Song, Catalin Marinas, Nick Desaulniers, LKML,
Nathan Chancellor, linux-arm-kernel, Will Deacon, kvmarm
On Thu, 04 Mar 2021 21:25:41 +0000,
Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 04 Mar 2021 18:45:44 +0000,
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > > linker errors:
> > >
> > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
> >
> > I assume this message is only an oddity, right? Because
> > __guest_enter() is as far as you can imagine from irqbypass.c...
>
> I'm not sure what's up with the filename in the error message. Fangrui
> or Nick probably have a better idea.
>
> > > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > > [-1048576, 1048575]; references hyp_panic
> > > >>> defined in vmlinux.o
> > >
> > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > > [-1048576, 1048575]; references hyp_panic
> > > >>> defined in vmlinux.o
> > >
> > > As LTO is not really necessary for the hypervisor code, disable it for
> > > the hyp directory to fix the build.
> >
> > Can you shed some light on what the problem is exactly?
>
> 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?
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1317
> > > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > > Tested-by: Nathan Chancellor <nathan@kernel.org>
> > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > > ---
> > > arch/arm64/kvm/hyp/Makefile | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> > > index 687598e41b21..e8116016e6a8 100644
> > > --- a/arch/arm64/kvm/hyp/Makefile
> > > +++ b/arch/arm64/kvm/hyp/Makefile
> > > @@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
> > > $(DISABLE_STACKLEAK_PLUGIN)
> > >
> > > obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
> > > +
> > > +# Disable LTO for the files in this directory
> > > +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
> > >
> > > base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
> >
> > Can this be reduced to the nvhe part of the tree? The rest of the
> > hypervisor should support being built with LTO, I'd expect. Or am I
> > missing something more significant?
>
> No, this error appears to be about hyp_panic() in the vhe code. While
> I'm not sure how beneficial LTO is in hypervisor code, there shouldn't
> be any other reason we can't use it there.
The VHE part of the hypervisor is the kernel itself, and not any
different from the rest of the code. It should be able to benefit from
LTO. On the contrary, the nVHE part is what could do without LTO,
given that it is essentially a separate object that happens to be
bundled with the kernel.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 22:17 ` Marc Zyngier
0 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-04 22:17 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Fangrui Song, Nick Desaulniers, James Morse, Nathan Chancellor,
Kees Cook, Julien Thierry, Suzuki K Poulose, Catalin Marinas,
Will Deacon, linux-arm-kernel, kvmarm, LKML
On Thu, 04 Mar 2021 21:25:41 +0000,
Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 04 Mar 2021 18:45:44 +0000,
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > > linker errors:
> > >
> > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
> >
> > I assume this message is only an oddity, right? Because
> > __guest_enter() is as far as you can imagine from irqbypass.c...
>
> I'm not sure what's up with the filename in the error message. Fangrui
> or Nick probably have a better idea.
>
> > > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > > [-1048576, 1048575]; references hyp_panic
> > > >>> defined in vmlinux.o
> > >
> > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > > [-1048576, 1048575]; references hyp_panic
> > > >>> defined in vmlinux.o
> > >
> > > As LTO is not really necessary for the hypervisor code, disable it for
> > > the hyp directory to fix the build.
> >
> > Can you shed some light on what the problem is exactly?
>
> 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?
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1317
> > > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > > Tested-by: Nathan Chancellor <nathan@kernel.org>
> > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > > ---
> > > arch/arm64/kvm/hyp/Makefile | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> > > index 687598e41b21..e8116016e6a8 100644
> > > --- a/arch/arm64/kvm/hyp/Makefile
> > > +++ b/arch/arm64/kvm/hyp/Makefile
> > > @@ -11,3 +11,6 @@ subdir-ccflags-y := -I$(incdir) \
> > > $(DISABLE_STACKLEAK_PLUGIN)
> > >
> > > obj-$(CONFIG_KVM) += vhe/ nvhe/ pgtable.o
> > > +
> > > +# Disable LTO for the files in this directory
> > > +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS))
> > >
> > > base-commit: f69d02e37a85645aa90d18cacfff36dba370f797
> >
> > Can this be reduced to the nvhe part of the tree? The rest of the
> > hypervisor should support being built with LTO, I'd expect. Or am I
> > missing something more significant?
>
> No, this error appears to be about hyp_panic() in the vhe code. While
> I'm not sure how beneficial LTO is in hypervisor code, there shouldn't
> be any other reason we can't use it there.
The VHE part of the hypervisor is the kernel itself, and not any
different from the rest of the code. It should be able to benefit from
LTO. On the contrary, the nVHE part is what could do without LTO,
given that it is essentially a separate object that happens to be
bundled with the kernel.
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
2021-03-04 22:17 ` Marc Zyngier
(?)
@ 2021-03-04 22:34 ` Sami Tolvanen
-1 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-04 22:34 UTC (permalink / raw)
To: Marc Zyngier
Cc: Fangrui Song, Nick Desaulniers, James Morse, Nathan Chancellor,
Kees Cook, Julien Thierry, Suzuki K Poulose, Catalin Marinas,
Will Deacon, linux-arm-kernel, kvmarm, LKML
On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 04 Mar 2021 21:25:41 +0000,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 04 Mar 2021 18:45:44 +0000,
> > > Sami Tolvanen <samitolvanen@google.com> wrote:
> > > >
> > > > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > > > linker errors:
> > > >
> > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
> > >
> > > I assume this message is only an oddity, right? Because
> > > __guest_enter() is as far as you can imagine from irqbypass.c...
> >
> > I'm not sure what's up with the filename in the error message. Fangrui
> > or Nick probably have a better idea.
> >
> > > > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > > > [-1048576, 1048575]; references hyp_panic
> > > > >>> defined in vmlinux.o
> > > >
> > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > > > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > > > [-1048576, 1048575]; references hyp_panic
> > > > >>> defined in vmlinux.o
> > > >
> > > > As LTO is not really necessary for the hypervisor code, disable it for
> > > > the hyp directory to fix the build.
> > >
> > > Can you shed some light on what the problem is exactly?
> >
> > 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
Sami
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 22:34 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-04 22:34 UTC (permalink / raw)
To: Marc Zyngier
Cc: Kees Cook, Fangrui Song, Catalin Marinas, Nick Desaulniers, LKML,
Nathan Chancellor, linux-arm-kernel, Will Deacon, kvmarm
On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 04 Mar 2021 21:25:41 +0000,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 04 Mar 2021 18:45:44 +0000,
> > > Sami Tolvanen <samitolvanen@google.com> wrote:
> > > >
> > > > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > > > linker errors:
> > > >
> > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
> > >
> > > I assume this message is only an oddity, right? Because
> > > __guest_enter() is as far as you can imagine from irqbypass.c...
> >
> > I'm not sure what's up with the filename in the error message. Fangrui
> > or Nick probably have a better idea.
> >
> > > > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > > > [-1048576, 1048575]; references hyp_panic
> > > > >>> defined in vmlinux.o
> > > >
> > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > > > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > > > [-1048576, 1048575]; references hyp_panic
> > > > >>> defined in vmlinux.o
> > > >
> > > > As LTO is not really necessary for the hypervisor code, disable it for
> > > > the hyp directory to fix the build.
> > >
> > > Can you shed some light on what the problem is exactly?
> >
> > 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
Sami
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-04 22:34 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-04 22:34 UTC (permalink / raw)
To: Marc Zyngier
Cc: Fangrui Song, Nick Desaulniers, James Morse, Nathan Chancellor,
Kees Cook, Julien Thierry, Suzuki K Poulose, Catalin Marinas,
Will Deacon, linux-arm-kernel, kvmarm, LKML
On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 04 Mar 2021 21:25:41 +0000,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 04 Mar 2021 18:45:44 +0000,
> > > Sami Tolvanen <samitolvanen@google.com> wrote:
> > > >
> > > > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > > > linker errors:
> > > >
> > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
> > >
> > > I assume this message is only an oddity, right? Because
> > > __guest_enter() is as far as you can imagine from irqbypass.c...
> >
> > I'm not sure what's up with the filename in the error message. Fangrui
> > or Nick probably have a better idea.
> >
> > > > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > > > [-1048576, 1048575]; references hyp_panic
> > > > >>> defined in vmlinux.o
> > > >
> > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > > > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > > > [-1048576, 1048575]; references hyp_panic
> > > > >>> defined in vmlinux.o
> > > >
> > > > As LTO is not really necessary for the hypervisor code, disable it for
> > > > the hyp directory to fix the build.
> > >
> > > Can you shed some light on what the problem is exactly?
> >
> > 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
Sami
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
2021-03-04 22:34 ` Sami Tolvanen
(?)
@ 2021-03-05 2:38 ` Sami Tolvanen
-1 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-05 2:38 UTC (permalink / raw)
To: Marc Zyngier
Cc: Fangrui Song, Nick Desaulniers, James Morse, Nathan Chancellor,
Kees Cook, Julien Thierry, Suzuki K Poulose, Catalin Marinas,
Will Deacon, linux-arm-kernel, kvmarm, LKML
On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 04 Mar 2021 21:25:41 +0000,
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Thu, 04 Mar 2021 18:45:44 +0000,
> > > > Sami Tolvanen <samitolvanen@google.com> wrote:
> > > > >
> > > > > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > > > > linker errors:
> > > > >
> > > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
> > > >
> > > > I assume this message is only an oddity, right? Because
> > > > __guest_enter() is as far as you can imagine from irqbypass.c...
> > >
> > > I'm not sure what's up with the filename in the error message. Fangrui
> > > or Nick probably have a better idea.
> > >
> > > > > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > > > > [-1048576, 1048575]; references hyp_panic
> > > > > >>> defined in vmlinux.o
> > > > >
> > > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > > > > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > > > > [-1048576, 1048575]; references hyp_panic
> > > > > >>> defined in vmlinux.o
> > > > >
> > > > > As LTO is not really necessary for the hypervisor code, disable it for
> > > > > the hyp directory to fix the build.
> > > >
> > > > Can you shed some light on what the problem is exactly?
> > >
> > > 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?
Sami
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-05 2:38 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-05 2:38 UTC (permalink / raw)
To: Marc Zyngier
Cc: Kees Cook, Fangrui Song, Catalin Marinas, Nick Desaulniers, LKML,
Nathan Chancellor, linux-arm-kernel, Will Deacon, kvmarm
On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 04 Mar 2021 21:25:41 +0000,
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Thu, 04 Mar 2021 18:45:44 +0000,
> > > > Sami Tolvanen <samitolvanen@google.com> wrote:
> > > > >
> > > > > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > > > > linker errors:
> > > > >
> > > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
> > > >
> > > > I assume this message is only an oddity, right? Because
> > > > __guest_enter() is as far as you can imagine from irqbypass.c...
> > >
> > > I'm not sure what's up with the filename in the error message. Fangrui
> > > or Nick probably have a better idea.
> > >
> > > > > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > > > > [-1048576, 1048575]; references hyp_panic
> > > > > >>> defined in vmlinux.o
> > > > >
> > > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > > > > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > > > > [-1048576, 1048575]; references hyp_panic
> > > > > >>> defined in vmlinux.o
> > > > >
> > > > > As LTO is not really necessary for the hypervisor code, disable it for
> > > > > the hyp directory to fix the build.
> > > >
> > > > Can you shed some light on what the problem is exactly?
> > >
> > > 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?
Sami
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-05 2:38 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-05 2:38 UTC (permalink / raw)
To: Marc Zyngier
Cc: Fangrui Song, Nick Desaulniers, James Morse, Nathan Chancellor,
Kees Cook, Julien Thierry, Suzuki K Poulose, Catalin Marinas,
Will Deacon, linux-arm-kernel, kvmarm, LKML
On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 04 Mar 2021 21:25:41 +0000,
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > On Thu, Mar 4, 2021 at 11:15 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Thu, 04 Mar 2021 18:45:44 +0000,
> > > > Sami Tolvanen <samitolvanen@google.com> wrote:
> > > > >
> > > > > allmodconfig + CONFIG_LTO_CLANG_THIN=y fails to build due to following
> > > > > linker errors:
> > > > >
> > > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21CC):
> > > >
> > > > I assume this message is only an oddity, right? Because
> > > > __guest_enter() is as far as you can imagine from irqbypass.c...
> > >
> > > I'm not sure what's up with the filename in the error message. Fangrui
> > > or Nick probably have a better idea.
> > >
> > > > > relocation R_AARCH64_CONDBR19 out of range: 2031220 is not in
> > > > > [-1048576, 1048575]; references hyp_panic
> > > > > >>> defined in vmlinux.o
> > > > >
> > > > > ld.lld: error: irqbypass.c:(function __guest_enter: .text+0x21E0):
> > > > > relocation R_AARCH64_ADR_PREL_LO21 out of range: 2031200 is not in
> > > > > [-1048576, 1048575]; references hyp_panic
> > > > > >>> defined in vmlinux.o
> > > > >
> > > > > As LTO is not really necessary for the hypervisor code, disable it for
> > > > > the hyp directory to fix the build.
> > > >
> > > > Can you shed some light on what the problem is exactly?
> > >
> > > 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?
Sami
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
2021-03-05 2:38 ` Sami Tolvanen
(?)
@ 2021-03-05 11:36 ` Marc Zyngier
-1 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-05 11:36 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Fangrui Song, Nick Desaulniers, James Morse, Nathan Chancellor,
Kees Cook, Julien Thierry, Suzuki K Poulose, Catalin Marinas,
Will Deacon, linux-arm-kernel, kvmarm, LKML
On Fri, 05 Mar 2021 02:38:17 +0000,
Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 04 Mar 2021 21:25:41 +0000,
> > > Sami Tolvanen <samitolvanen@google.com> wrote:
[...]
> > > > 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.
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-05 11:36 ` Marc Zyngier
0 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-05 11:36 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Kees Cook, Fangrui Song, Catalin Marinas, Nick Desaulniers, LKML,
Nathan Chancellor, linux-arm-kernel, Will Deacon, kvmarm
On Fri, 05 Mar 2021 02:38:17 +0000,
Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 04 Mar 2021 21:25:41 +0000,
> > > Sami Tolvanen <samitolvanen@google.com> wrote:
[...]
> > > > 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.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-05 11:36 ` Marc Zyngier
0 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-05 11:36 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Fangrui Song, Nick Desaulniers, James Morse, Nathan Chancellor,
Kees Cook, Julien Thierry, Suzuki K Poulose, Catalin Marinas,
Will Deacon, linux-arm-kernel, kvmarm, LKML
On Fri, 05 Mar 2021 02:38:17 +0000,
Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 04 Mar 2021 21:25:41 +0000,
> > > Sami Tolvanen <samitolvanen@google.com> wrote:
[...]
> > > > 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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
2021-03-05 11:36 ` Marc Zyngier
(?)
@ 2021-03-05 14:22 ` Ard Biesheuvel
-1 siblings, 0 replies; 30+ messages in thread
From: Ard Biesheuvel @ 2021-03-05 14:22 UTC (permalink / raw)
To: Marc Zyngier
Cc: Sami Tolvanen, Fangrui Song, Nick Desaulniers, James Morse,
Nathan Chancellor, Kees Cook, Julien Thierry, Suzuki K Poulose,
Catalin Marinas, Will Deacon, linux-arm-kernel, kvmarm, LKML
On Fri, 5 Mar 2021 at 12:38, Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 05 Mar 2021 02:38:17 +0000,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Thu, 04 Mar 2021 21:25:41 +0000,
> > > > Sami Tolvanen <samitolvanen@google.com> wrote:
>
> [...]
>
> > > > > 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:
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?
> // 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.
>
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
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
So there are definitely a few other cases that require another look.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-05 14:22 ` Ard Biesheuvel
0 siblings, 0 replies; 30+ messages in thread
From: Ard Biesheuvel @ 2021-03-05 14:22 UTC (permalink / raw)
To: Marc Zyngier
Cc: Kees Cook, Fangrui Song, Catalin Marinas, Nick Desaulniers, LKML,
Nathan Chancellor, linux-arm-kernel, Sami Tolvanen, Will Deacon,
kvmarm
On Fri, 5 Mar 2021 at 12:38, Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 05 Mar 2021 02:38:17 +0000,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Thu, 04 Mar 2021 21:25:41 +0000,
> > > > Sami Tolvanen <samitolvanen@google.com> wrote:
>
> [...]
>
> > > > > 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:
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?
> // 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.
>
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
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
So there are definitely a few other cases that require another look.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-05 14:22 ` Ard Biesheuvel
0 siblings, 0 replies; 30+ messages in thread
From: Ard Biesheuvel @ 2021-03-05 14:22 UTC (permalink / raw)
To: Marc Zyngier
Cc: Sami Tolvanen, Fangrui Song, Nick Desaulniers, James Morse,
Nathan Chancellor, Kees Cook, Julien Thierry, Suzuki K Poulose,
Catalin Marinas, Will Deacon, linux-arm-kernel, kvmarm, LKML
On Fri, 5 Mar 2021 at 12:38, Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 05 Mar 2021 02:38:17 +0000,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Thu, 04 Mar 2021 21:25:41 +0000,
> > > > Sami Tolvanen <samitolvanen@google.com> wrote:
>
> [...]
>
> > > > > 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:
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?
> // 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.
>
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
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
So there are definitely a few other cases that require another look.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
2021-03-05 14:22 ` Ard Biesheuvel
(?)
@ 2021-03-05 16:55 ` Sami Tolvanen
-1 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-05 16:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Marc Zyngier, Fangrui Song, Nick Desaulniers, James Morse,
Nathan Chancellor, Kees Cook, Julien Thierry, Suzuki K Poulose,
Catalin Marinas, Will Deacon, linux-arm-kernel, kvmarm, LKML
On Fri, Mar 5, 2021 at 6:22 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 5 Mar 2021 at 12:38, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Fri, 05 Mar 2021 02:38:17 +0000,
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> > > >
> > > > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > >
> > > > > On Thu, 04 Mar 2021 21:25:41 +0000,
> > > > > Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > [...]
> >
> > > > > > 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:
>
> 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.
> > // 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.
> >
>
> 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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-05 16:55 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-05 16:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Kees Cook, Fangrui Song, Marc Zyngier, Nick Desaulniers, LKML,
Nathan Chancellor, linux-arm-kernel, Catalin Marinas,
Will Deacon, kvmarm
On Fri, Mar 5, 2021 at 6:22 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 5 Mar 2021 at 12:38, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Fri, 05 Mar 2021 02:38:17 +0000,
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> > > >
> > > > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > >
> > > > > On Thu, 04 Mar 2021 21:25:41 +0000,
> > > > > Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > [...]
> >
> > > > > > 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:
>
> 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.
> > // 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.
> >
>
> 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
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-05 16:55 ` Sami Tolvanen
0 siblings, 0 replies; 30+ messages in thread
From: Sami Tolvanen @ 2021-03-05 16:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Marc Zyngier, Fangrui Song, Nick Desaulniers, James Morse,
Nathan Chancellor, Kees Cook, Julien Thierry, Suzuki K Poulose,
Catalin Marinas, Will Deacon, linux-arm-kernel, kvmarm, LKML
On Fri, Mar 5, 2021 at 6:22 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 5 Mar 2021 at 12:38, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Fri, 05 Mar 2021 02:38:17 +0000,
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> > > >
> > > > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > >
> > > > > On Thu, 04 Mar 2021 21:25:41 +0000,
> > > > > Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > [...]
> >
> > > > > > 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:
>
> 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.
> > // 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.
> >
>
> 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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
2021-03-05 16:55 ` Sami Tolvanen
(?)
@ 2021-03-05 17:07 ` Marc Zyngier
-1 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-05 17:07 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Ard Biesheuvel, Fangrui Song, Nick Desaulniers, James Morse,
Nathan Chancellor, Kees Cook, Julien Thierry, Suzuki K Poulose,
Catalin Marinas, Will Deacon, linux-arm-kernel, kvmarm, LKML
On 2021-03-05 16:55, Sami Tolvanen wrote:
> On Fri, Mar 5, 2021 at 6:22 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Fri, 5 Mar 2021 at 12:38, Marc Zyngier <maz@kernel.org> wrote:
>> >
>> > On Fri, 05 Mar 2021 02:38:17 +0000,
>> > Sami Tolvanen <samitolvanen@google.com> wrote:
>> > >
>> > > On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>> > > >
>> > > > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
>> > > > >
>> > > > > On Thu, 04 Mar 2021 21:25:41 +0000,
>> > > > > Sami Tolvanen <samitolvanen@google.com> wrote:
>> >
>> > [...]
>> >
>> > > > > > 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:
>>
>> 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.
Please do, it is "obviously correct"! ;-)
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-05 17:07 ` Marc Zyngier
0 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-05 17:07 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Kees Cook, Fangrui Song, Catalin Marinas, Nick Desaulniers, LKML,
kvmarm, Nathan Chancellor, linux-arm-kernel, Will Deacon,
Ard Biesheuvel
On 2021-03-05 16:55, Sami Tolvanen wrote:
> On Fri, Mar 5, 2021 at 6:22 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Fri, 5 Mar 2021 at 12:38, Marc Zyngier <maz@kernel.org> wrote:
>> >
>> > On Fri, 05 Mar 2021 02:38:17 +0000,
>> > Sami Tolvanen <samitolvanen@google.com> wrote:
>> > >
>> > > On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>> > > >
>> > > > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
>> > > > >
>> > > > > On Thu, 04 Mar 2021 21:25:41 +0000,
>> > > > > Sami Tolvanen <samitolvanen@google.com> wrote:
>> >
>> > [...]
>> >
>> > > > > > 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:
>>
>> 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.
Please do, it is "obviously correct"! ;-)
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: arm64: Disable LTO in hyp
@ 2021-03-05 17:07 ` Marc Zyngier
0 siblings, 0 replies; 30+ messages in thread
From: Marc Zyngier @ 2021-03-05 17:07 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Ard Biesheuvel, Fangrui Song, Nick Desaulniers, James Morse,
Nathan Chancellor, Kees Cook, Julien Thierry, Suzuki K Poulose,
Catalin Marinas, Will Deacon, linux-arm-kernel, kvmarm, LKML
On 2021-03-05 16:55, Sami Tolvanen wrote:
> On Fri, Mar 5, 2021 at 6:22 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Fri, 5 Mar 2021 at 12:38, Marc Zyngier <maz@kernel.org> wrote:
>> >
>> > On Fri, 05 Mar 2021 02:38:17 +0000,
>> > Sami Tolvanen <samitolvanen@google.com> wrote:
>> > >
>> > > On Thu, Mar 4, 2021 at 2:34 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>> > > >
>> > > > On Thu, Mar 4, 2021 at 2:17 PM Marc Zyngier <maz@kernel.org> wrote:
>> > > > >
>> > > > > On Thu, 04 Mar 2021 21:25:41 +0000,
>> > > > > Sami Tolvanen <samitolvanen@google.com> wrote:
>> >
>> > [...]
>> >
>> > > > > > 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:
>>
>> 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.
Please do, it is "obviously correct"! ;-)
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2021-03-05 18:58 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-04 18:45 [PATCH] KVM: arm64: Disable LTO in hyp Sami Tolvanen
2021-03-04 18:45 ` Sami Tolvanen
2021-03-04 18:45 ` Sami Tolvanen
2021-03-04 19:15 ` Marc Zyngier
2021-03-04 19:15 ` Marc Zyngier
2021-03-04 19:15 ` Marc Zyngier
2021-03-04 21:25 ` Sami Tolvanen
2021-03-04 21:25 ` Sami Tolvanen
2021-03-04 21:25 ` Sami Tolvanen
2021-03-04 22:17 ` Marc Zyngier
2021-03-04 22:17 ` Marc Zyngier
2021-03-04 22:17 ` Marc Zyngier
2021-03-04 22:34 ` Sami Tolvanen
2021-03-04 22:34 ` Sami Tolvanen
2021-03-04 22:34 ` Sami Tolvanen
2021-03-05 2:38 ` Sami Tolvanen
2021-03-05 2:38 ` Sami Tolvanen
2021-03-05 2:38 ` Sami Tolvanen
2021-03-05 11:36 ` Marc Zyngier
2021-03-05 11:36 ` Marc Zyngier
2021-03-05 11:36 ` Marc Zyngier
2021-03-05 14:22 ` Ard Biesheuvel
2021-03-05 14:22 ` Ard Biesheuvel
2021-03-05 14:22 ` Ard Biesheuvel
2021-03-05 16:55 ` Sami Tolvanen
2021-03-05 16:55 ` Sami Tolvanen
2021-03-05 16:55 ` Sami Tolvanen
2021-03-05 17:07 ` Marc Zyngier
2021-03-05 17:07 ` Marc Zyngier
2021-03-05 17:07 ` Marc Zyngier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.