* [PATCH 0/2] Debug info for nVHE hyp panics
@ 2021-02-23 9:49 Andrew Scull
2021-02-23 9:49 ` [PATCH 1/2] KVM: arm64: Use BUG and BUG_ON in nVHE hyp Andrew Scull
2021-02-23 9:49 ` [PATCH 2/2] KVM: arm64: Log source when panicking from " Andrew Scull
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Scull @ 2021-02-23 9:49 UTC (permalink / raw)
To: kvmarm; +Cc: kernel-team, maz, catalin.marinas, will
After being on the receiving end of an nVHE hyp panic and trying to
figure out what is was trying to tell me, it seemed like a good excuse
to add some debug info.
nVHE hyp has its own address space. Hyp VAs aren't much use for
debugging but they can be converted into kimg addresses which are
useful for looking up in vmlinux.
There are also a couple of invariant tests that call hyp_panic() but
these would give the ELR_EL2 of the previous VM which isn't very
meaningful. Converting these to use BUG() lets the correct hyp address
be captured and the source file and line can even be logged!
This applied on top of the previous panic fix at
https://lore.kernel.org/r/20210219122406.1337626-1-ascull@google.com/
Andrew Scull (2):
KVM: arm64: Use BUG and BUG_ON in nVHE hyp
KVM: arm64: Log source when panicking from nVHE hyp
arch/arm64/include/asm/kvm_hyp.h | 1 -
arch/arm64/include/asm/kvm_mmu.h | 2 ++
arch/arm64/kernel/image-vars.h | 3 +-
arch/arm64/kvm/handle_exit.c | 38 +++++++++++++++++++++++++
arch/arm64/kvm/hyp/include/hyp/switch.h | 2 --
arch/arm64/kvm/hyp/nvhe/host.S | 18 ++++++------
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 2 +-
arch/arm64/kvm/hyp/nvhe/hyp-smp.c | 6 ++--
arch/arm64/kvm/hyp/nvhe/psci-relay.c | 2 --
9 files changed, 52 insertions(+), 22 deletions(-)
--
2.30.0.617.g56c4b15f3c-goog
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: arm64: Use BUG and BUG_ON in nVHE hyp
2021-02-23 9:49 [PATCH 0/2] Debug info for nVHE hyp panics Andrew Scull
@ 2021-02-23 9:49 ` Andrew Scull
2021-02-23 9:49 ` [PATCH 2/2] KVM: arm64: Log source when panicking from " Andrew Scull
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Scull @ 2021-02-23 9:49 UTC (permalink / raw)
To: kvmarm; +Cc: kernel-team, maz, catalin.marinas, will
hyp_panic() reports the address of the panic by using ELR_EL2, but this
isn't a useful address when hyp_panic() is called directly. Replace such
direct calls with BUG() and BUG_ON() which use BRK to trigger and
exception that then goes to hyp_panic() with the correct address. Also
remove the hyp_panic() declaration from the header file to avoid
accidental misuse.
Signed-off-by: Andrew Scull <ascull@google.com>
---
arch/arm64/include/asm/kvm_hyp.h | 1 -
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 2 +-
arch/arm64/kvm/hyp/nvhe/hyp-smp.c | 6 ++----
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index fb8404fefd1f..746eb9a2891b 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -95,7 +95,6 @@ u64 __guest_enter(struct kvm_vcpu *vcpu);
bool kvm_host_psci_handler(struct kvm_cpu_context *host_ctxt);
-void __noreturn hyp_panic(void);
#ifdef __KVM_NVHE_HYPERVISOR__
void __noreturn __hyp_do_panic(struct kvm_cpu_context *host_ctxt, u64 spsr,
u64 elr, u64 par);
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index a906f9e2ff34..9f37a4240562 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -181,6 +181,6 @@ void handle_trap(struct kvm_cpu_context *host_ctxt)
handle_host_smc(host_ctxt);
break;
default:
- hyp_panic();
+ BUG();
}
}
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-smp.c b/arch/arm64/kvm/hyp/nvhe/hyp-smp.c
index 2997aa156d8e..4495aed04240 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-smp.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-smp.c
@@ -18,8 +18,7 @@ u64 __ro_after_init hyp_cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID
u64 cpu_logical_map(unsigned int cpu)
{
- if (cpu >= ARRAY_SIZE(hyp_cpu_logical_map))
- hyp_panic();
+ BUG_ON(cpu >= ARRAY_SIZE(hyp_cpu_logical_map));
return hyp_cpu_logical_map[cpu];
}
@@ -30,8 +29,7 @@ unsigned long __hyp_per_cpu_offset(unsigned int cpu)
unsigned long this_cpu_base;
unsigned long elf_base;
- if (cpu >= ARRAY_SIZE(kvm_arm_hyp_percpu_base))
- hyp_panic();
+ BUG_ON(cpu >= ARRAY_SIZE(kvm_arm_hyp_percpu_base));
cpu_base_array = (unsigned long *)hyp_symbol_addr(kvm_arm_hyp_percpu_base);
this_cpu_base = kern_hyp_va(cpu_base_array[cpu]);
--
2.30.0.617.g56c4b15f3c-goog
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: arm64: Log source when panicking from nVHE hyp
2021-02-23 9:49 [PATCH 0/2] Debug info for nVHE hyp panics Andrew Scull
2021-02-23 9:49 ` [PATCH 1/2] KVM: arm64: Use BUG and BUG_ON in nVHE hyp Andrew Scull
@ 2021-02-23 9:49 ` Andrew Scull
2021-02-23 11:29 ` Marc Zyngier
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Scull @ 2021-02-23 9:49 UTC (permalink / raw)
To: kvmarm; +Cc: kernel-team, maz, catalin.marinas, will
To aid with debugging, add details of the source of a panic. This is
done by having nVHE hyp exit to nvhe_hyp_panic_handler() rather than
directly to panic(). The handler will then add the extra details for
debugging before panicking the kernel.
If the panic was due to a BUG(), look up the metadata to log the file
and line, if available, otherwise log the kimg address that can be
looked up in vmlinux.
Signed-off-by: Andrew Scull <ascull@google.com>
---
arch/arm64/include/asm/kvm_mmu.h | 2 ++
arch/arm64/kernel/image-vars.h | 3 +-
arch/arm64/kvm/handle_exit.c | 38 +++++++++++++++++++++++++
arch/arm64/kvm/hyp/include/hyp/switch.h | 2 --
arch/arm64/kvm/hyp/nvhe/host.S | 18 ++++++------
arch/arm64/kvm/hyp/nvhe/psci-relay.c | 2 --
6 files changed, 49 insertions(+), 16 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index e52d82aeadca..f07c55f9dd1e 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -130,6 +130,8 @@ void kvm_update_va_mask(struct alt_instr *alt,
__le32 *origptr, __le32 *updptr, int nr_inst);
void kvm_compute_layout(void);
+#define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset)
+
static __always_inline unsigned long __kern_hyp_va(unsigned long v)
{
asm volatile(ALTERNATIVE_CB("and %0, %0, #1\n"
diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
index f676243abac6..cf12b0d6441e 100644
--- a/arch/arm64/kernel/image-vars.h
+++ b/arch/arm64/kernel/image-vars.h
@@ -71,8 +71,7 @@ KVM_NVHE_ALIAS(kvm_get_kimage_voffset);
KVM_NVHE_ALIAS(kvm_vgic_global_state);
/* Kernel symbols used to call panic() from nVHE hyp code (via ERET). */
-KVM_NVHE_ALIAS(__hyp_panic_string);
-KVM_NVHE_ALIAS(panic);
+KVM_NVHE_ALIAS(nvhe_hyp_panic_handler);
/* Vectors installed by hyp-init on reset HVC. */
KVM_NVHE_ALIAS(__hyp_stub_vectors);
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index cebe39f3b1b6..5e0d9a5152e5 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -291,3 +291,41 @@ void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
if (exception_index == ARM_EXCEPTION_EL1_SERROR)
kvm_handle_guest_serror(vcpu, kvm_vcpu_get_esr(vcpu));
}
+
+void __noreturn __cold nvhe_hyp_panic_handler(bool hyp, u64 spsr, u64 elr,
+ u64 par, uintptr_t vcpu,
+ u64 far, u64 hpfar, u64 esr) {
+ extern const char __hyp_panic_string[];
+ u64 elr_in_kimg = __phys_to_kimg(__hyp_pa(elr));
+
+ if (!hyp) {
+ kvm_err("Invalid host exception to nVHE hyp!\n");
+ } else if (ESR_ELx_EC(esr) == ESR_ELx_EC_BRK64 &&
+ (esr & ESR_ELx_BRK64_ISS_COMMENT_MASK) == BUG_BRK_IMM) {
+ struct bug_entry *bug = find_bug(elr_in_kimg);
+ const char *file = NULL;
+ unsigned line = 0;
+
+ /* All hyp bugs, including warnings, are treated as fatal. */
+ if (bug) {
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+#ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
+ file = bug->file;
+#else
+ file = (const char *)bug + bug->file_disp;
+#endif
+ line = bug->line;
+#endif
+ }
+
+ if (file) {
+ kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
+ } else {
+ kvm_err("nVHE hyp BUG at: %016llx!\n", elr_in_kimg);
+ }
+ } else {
+ kvm_err("nVHE hyp panic at: %016llx!\n", elr_in_kimg);
+ }
+
+ panic(__hyp_panic_string, spsr, elr, esr, far, hpfar, par, vcpu);
+}
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index 84473574c2e7..f9e8bb97d199 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -30,8 +30,6 @@
#include <asm/processor.h>
#include <asm/thread_info.h>
-extern const char __hyp_panic_string[];
-
extern struct exception_table_entry __start___kvm_ex_table;
extern struct exception_table_entry __stop___kvm_ex_table;
diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
index 3dc5a9f3e575..d9a9dd66b1a2 100644
--- a/arch/arm64/kvm/hyp/nvhe/host.S
+++ b/arch/arm64/kvm/hyp/nvhe/host.S
@@ -77,21 +77,19 @@ SYM_FUNC_END(__host_enter)
SYM_FUNC_START(__hyp_do_panic)
mov x29, x0
- /* Load the format string into x0 and arguments into x1-7 */
- ldr x0, =__hyp_panic_string
-
- mov x6, x3
- get_vcpu_ptr x7, x3
-
- mrs x3, esr_el2
- mrs x4, far_el2
- mrs x5, hpfar_el2
+ /* Load the panic arguments into x0-7 */
+ cmp x0, xzr
+ cset x0, ne
+ get_vcpu_ptr x4, x5
+ mrs x5, far_el2
+ mrs x6, hpfar_el2
+ mrs x7, esr_el2
/* Prepare and exit to the host's panic funciton. */
mov lr, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\
PSR_MODE_EL1h)
msr spsr_el2, lr
- ldr lr, =panic
+ ldr lr, =nvhe_hyp_panic_handler
msr elr_el2, lr
/* Enter the host, restoring the host context if it was provided. */
diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
index 8e7128cb7667..54b70189229b 100644
--- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
+++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
@@ -22,8 +22,6 @@ void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt);
struct kvm_host_psci_config __ro_after_init kvm_host_psci_config;
s64 __ro_after_init hyp_physvirt_offset;
-#define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset)
-
#define INVALID_CPU_ID UINT_MAX
struct psci_boot_args {
--
2.30.0.617.g56c4b15f3c-goog
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: arm64: Log source when panicking from nVHE hyp
2021-02-23 9:49 ` [PATCH 2/2] KVM: arm64: Log source when panicking from " Andrew Scull
@ 2021-02-23 11:29 ` Marc Zyngier
2021-02-23 12:34 ` Andrew Scull
0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2021-02-23 11:29 UTC (permalink / raw)
To: Andrew Scull; +Cc: kernel-team, catalin.marinas, will, kvmarm
Hi Andrew,
On Tue, 23 Feb 2021 09:49:27 +0000,
Andrew Scull <ascull@google.com> wrote:
>
> To aid with debugging, add details of the source of a panic. This is
> done by having nVHE hyp exit to nvhe_hyp_panic_handler() rather than
> directly to panic(). The handler will then add the extra details for
> debugging before panicking the kernel.
>
> If the panic was due to a BUG(), look up the metadata to log the file
> and line, if available, otherwise log the kimg address that can be
> looked up in vmlinux.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
> arch/arm64/include/asm/kvm_mmu.h | 2 ++
> arch/arm64/kernel/image-vars.h | 3 +-
> arch/arm64/kvm/handle_exit.c | 38 +++++++++++++++++++++++++
> arch/arm64/kvm/hyp/include/hyp/switch.h | 2 --
> arch/arm64/kvm/hyp/nvhe/host.S | 18 ++++++------
> arch/arm64/kvm/hyp/nvhe/psci-relay.c | 2 --
> 6 files changed, 49 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index e52d82aeadca..f07c55f9dd1e 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -130,6 +130,8 @@ void kvm_update_va_mask(struct alt_instr *alt,
> __le32 *origptr, __le32 *updptr, int nr_inst);
> void kvm_compute_layout(void);
>
> +#define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset)
> +
> static __always_inline unsigned long __kern_hyp_va(unsigned long v)
> {
> asm volatile(ALTERNATIVE_CB("and %0, %0, #1\n"
> diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
> index f676243abac6..cf12b0d6441e 100644
> --- a/arch/arm64/kernel/image-vars.h
> +++ b/arch/arm64/kernel/image-vars.h
> @@ -71,8 +71,7 @@ KVM_NVHE_ALIAS(kvm_get_kimage_voffset);
> KVM_NVHE_ALIAS(kvm_vgic_global_state);
>
> /* Kernel symbols used to call panic() from nVHE hyp code (via ERET). */
> -KVM_NVHE_ALIAS(__hyp_panic_string);
> -KVM_NVHE_ALIAS(panic);
> +KVM_NVHE_ALIAS(nvhe_hyp_panic_handler);
>
> /* Vectors installed by hyp-init on reset HVC. */
> KVM_NVHE_ALIAS(__hyp_stub_vectors);
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index cebe39f3b1b6..5e0d9a5152e5 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -291,3 +291,41 @@ void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
> if (exception_index == ARM_EXCEPTION_EL1_SERROR)
> kvm_handle_guest_serror(vcpu, kvm_vcpu_get_esr(vcpu));
> }
> +
> +void __noreturn __cold nvhe_hyp_panic_handler(bool hyp, u64 spsr, u64 elr,
> + u64 par, uintptr_t vcpu,
> + u64 far, u64 hpfar, u64 esr) {
> + extern const char __hyp_panic_string[];
Is there any reason left to not to make this a symbol at all, but
instead to feed the string to the panic() call below?
> + u64 elr_in_kimg = __phys_to_kimg(__hyp_pa(elr));
> +
> + if (!hyp) {
> + kvm_err("Invalid host exception to nVHE hyp!\n");
Do we need to have hyp as a parameter? Can't we work that out from the
SPSR passed as a parameter?
> + } else if (ESR_ELx_EC(esr) == ESR_ELx_EC_BRK64 &&
> + (esr & ESR_ELx_BRK64_ISS_COMMENT_MASK) == BUG_BRK_IMM) {
> + struct bug_entry *bug = find_bug(elr_in_kimg);
> + const char *file = NULL;
> + unsigned line = 0;
> +
> + /* All hyp bugs, including warnings, are treated as fatal. */
> + if (bug) {
> +#ifdef CONFIG_DEBUG_BUGVERBOSE
> +#ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
> + file = bug->file;
> +#else
> + file = (const char *)bug + bug->file_disp;
> +#endif
> + line = bug->line;
> +#endif
It looks like you have lifted this from lib/bugs.c. Would it be worth
making it a helper that lives there? Something like
struct bug_entry *bug_get_entry(unsigned long pc, char **file,
unsigned int *line);
that hides the #ifdefery and the fund_bug() call away? The
disable_trace_on_warning() call isn't a problem, as we're about to die
anyway.
> + }
> +
> + if (file) {
> + kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
> + } else {
> + kvm_err("nVHE hyp BUG at: %016llx!\n", elr_in_kimg);
> + }
> + } else {
> + kvm_err("nVHE hyp panic at: %016llx!\n", elr_in_kimg);
> + }
> +
> + panic(__hyp_panic_string, spsr, elr, esr, far, hpfar, par, vcpu);
> +}
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 84473574c2e7..f9e8bb97d199 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -30,8 +30,6 @@
> #include <asm/processor.h>
> #include <asm/thread_info.h>
>
> -extern const char __hyp_panic_string[];
> -
> extern struct exception_table_entry __start___kvm_ex_table;
> extern struct exception_table_entry __stop___kvm_ex_table;
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
> index 3dc5a9f3e575..d9a9dd66b1a2 100644
> --- a/arch/arm64/kvm/hyp/nvhe/host.S
> +++ b/arch/arm64/kvm/hyp/nvhe/host.S
> @@ -77,21 +77,19 @@ SYM_FUNC_END(__host_enter)
> SYM_FUNC_START(__hyp_do_panic)
> mov x29, x0
>
> - /* Load the format string into x0 and arguments into x1-7 */
> - ldr x0, =__hyp_panic_string
> -
> - mov x6, x3
> - get_vcpu_ptr x7, x3
> -
> - mrs x3, esr_el2
> - mrs x4, far_el2
> - mrs x5, hpfar_el2
> + /* Load the panic arguments into x0-7 */
> + cmp x0, xzr
> + cset x0, ne
> + get_vcpu_ptr x4, x5
> + mrs x5, far_el2
> + mrs x6, hpfar_el2
> + mrs x7, esr_el2
>
> /* Prepare and exit to the host's panic funciton. */
> mov lr, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\
> PSR_MODE_EL1h)
> msr spsr_el2, lr
> - ldr lr, =panic
> + ldr lr, =nvhe_hyp_panic_handler
> msr elr_el2, lr
>
> /* Enter the host, restoring the host context if it was provided. */
> diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> index 8e7128cb7667..54b70189229b 100644
> --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> @@ -22,8 +22,6 @@ void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt);
> struct kvm_host_psci_config __ro_after_init kvm_host_psci_config;
> s64 __ro_after_init hyp_physvirt_offset;
>
> -#define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset)
> -
> #define INVALID_CPU_ID UINT_MAX
>
> struct psci_boot_args {
> --
> 2.30.0.617.g56c4b15f3c-goog
>
>
Otherwise, I like the idea!
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] 6+ messages in thread
* Re: [PATCH 2/2] KVM: arm64: Log source when panicking from nVHE hyp
2021-02-23 11:29 ` Marc Zyngier
@ 2021-02-23 12:34 ` Andrew Scull
2021-02-23 15:18 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Scull @ 2021-02-23 12:34 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kernel-team, catalin.marinas, will, kvmarm
On Tue, Feb 23, 2021 at 11:29:54AM +0000, Marc Zyngier wrote:
> Hi Andrew,
>
> On Tue, 23 Feb 2021 09:49:27 +0000,
> Andrew Scull <ascull@google.com> wrote:
> >
> > To aid with debugging, add details of the source of a panic. This is
> > done by having nVHE hyp exit to nvhe_hyp_panic_handler() rather than
> > directly to panic(). The handler will then add the extra details for
> > debugging before panicking the kernel.
> >
> > If the panic was due to a BUG(), look up the metadata to log the file
> > and line, if available, otherwise log the kimg address that can be
> > looked up in vmlinux.
> >
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> > arch/arm64/include/asm/kvm_mmu.h | 2 ++
> > arch/arm64/kernel/image-vars.h | 3 +-
> > arch/arm64/kvm/handle_exit.c | 38 +++++++++++++++++++++++++
> > arch/arm64/kvm/hyp/include/hyp/switch.h | 2 --
> > arch/arm64/kvm/hyp/nvhe/host.S | 18 ++++++------
> > arch/arm64/kvm/hyp/nvhe/psci-relay.c | 2 --
> > 6 files changed, 49 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > index e52d82aeadca..f07c55f9dd1e 100644
> > --- a/arch/arm64/include/asm/kvm_mmu.h
> > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > @@ -130,6 +130,8 @@ void kvm_update_va_mask(struct alt_instr *alt,
> > __le32 *origptr, __le32 *updptr, int nr_inst);
> > void kvm_compute_layout(void);
> >
> > +#define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset)
> > +
> > static __always_inline unsigned long __kern_hyp_va(unsigned long v)
> > {
> > asm volatile(ALTERNATIVE_CB("and %0, %0, #1\n"
> > diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
> > index f676243abac6..cf12b0d6441e 100644
> > --- a/arch/arm64/kernel/image-vars.h
> > +++ b/arch/arm64/kernel/image-vars.h
> > @@ -71,8 +71,7 @@ KVM_NVHE_ALIAS(kvm_get_kimage_voffset);
> > KVM_NVHE_ALIAS(kvm_vgic_global_state);
> >
> > /* Kernel symbols used to call panic() from nVHE hyp code (via ERET). */
> > -KVM_NVHE_ALIAS(__hyp_panic_string);
> > -KVM_NVHE_ALIAS(panic);
> > +KVM_NVHE_ALIAS(nvhe_hyp_panic_handler);
> >
> > /* Vectors installed by hyp-init on reset HVC. */
> > KVM_NVHE_ALIAS(__hyp_stub_vectors);
> > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> > index cebe39f3b1b6..5e0d9a5152e5 100644
> > --- a/arch/arm64/kvm/handle_exit.c
> > +++ b/arch/arm64/kvm/handle_exit.c
> > @@ -291,3 +291,41 @@ void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
> > if (exception_index == ARM_EXCEPTION_EL1_SERROR)
> > kvm_handle_guest_serror(vcpu, kvm_vcpu_get_esr(vcpu));
> > }
> > +
> > +void __noreturn __cold nvhe_hyp_panic_handler(bool hyp, u64 spsr, u64 elr,
> > + u64 par, uintptr_t vcpu,
> > + u64 far, u64 hpfar, u64 esr) {
> > + extern const char __hyp_panic_string[];
>
> Is there any reason left to not to make this a symbol at all, but
> instead to feed the string to the panic() call below?
No, none. I guess it was only a symbol so nVHE hyp could point at it and
doesn't matter any more. There doesn't seem to be a good reason for VHE
and nVHE to have the same panic string.
> > + u64 elr_in_kimg = __phys_to_kimg(__hyp_pa(elr));
> > +
> > + if (!hyp) {
> > + kvm_err("Invalid host exception to nVHE hyp!\n");
>
> Do we need to have hyp as a parameter? Can't we work that out from the
> SPSR passed as a parameter?
You're right, that should have some useful information already. Didn't
occur to me before, I'll play with it.
> > + } else if (ESR_ELx_EC(esr) == ESR_ELx_EC_BRK64 &&
> > + (esr & ESR_ELx_BRK64_ISS_COMMENT_MASK) == BUG_BRK_IMM) {
> > + struct bug_entry *bug = find_bug(elr_in_kimg);
> > + const char *file = NULL;
> > + unsigned line = 0;
> > +
> > + /* All hyp bugs, including warnings, are treated as fatal. */
> > + if (bug) {
> > +#ifdef CONFIG_DEBUG_BUGVERBOSE
> > +#ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
> > + file = bug->file;
> > +#else
> > + file = (const char *)bug + bug->file_disp;
> > +#endif
> > + line = bug->line;
> > +#endif
>
> It looks like you have lifted this from lib/bugs.c.
Bingo!
> Would it be worth
> making it a helper that lives there? Something like
>
> struct bug_entry *bug_get_entry(unsigned long pc, char **file,
> unsigned int *line);
>
> that hides the #ifdefery and the fund_bug() call away? The
> disable_trace_on_warning() call isn't a problem, as we're about to die
> anyway.
Yeah, that'd make a lot of sense. Please protect me from the wrath of
the community when I touch the common code..
> > + }
> > +
> > + if (file) {
> > + kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
> > + } else {
> > + kvm_err("nVHE hyp BUG at: %016llx!\n", elr_in_kimg);
> > + }
> > + } else {
> > + kvm_err("nVHE hyp panic at: %016llx!\n", elr_in_kimg);
> > + }
> > +
> > + panic(__hyp_panic_string, spsr, elr, esr, far, hpfar, par, vcpu);
> > +}
> > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > index 84473574c2e7..f9e8bb97d199 100644
> > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > @@ -30,8 +30,6 @@
> > #include <asm/processor.h>
> > #include <asm/thread_info.h>
> >
> > -extern const char __hyp_panic_string[];
> > -
> > extern struct exception_table_entry __start___kvm_ex_table;
> > extern struct exception_table_entry __stop___kvm_ex_table;
> >
> > diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
> > index 3dc5a9f3e575..d9a9dd66b1a2 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/host.S
> > +++ b/arch/arm64/kvm/hyp/nvhe/host.S
> > @@ -77,21 +77,19 @@ SYM_FUNC_END(__host_enter)
> > SYM_FUNC_START(__hyp_do_panic)
> > mov x29, x0
> >
> > - /* Load the format string into x0 and arguments into x1-7 */
> > - ldr x0, =__hyp_panic_string
> > -
> > - mov x6, x3
> > - get_vcpu_ptr x7, x3
> > -
> > - mrs x3, esr_el2
> > - mrs x4, far_el2
> > - mrs x5, hpfar_el2
> > + /* Load the panic arguments into x0-7 */
> > + cmp x0, xzr
> > + cset x0, ne
> > + get_vcpu_ptr x4, x5
> > + mrs x5, far_el2
> > + mrs x6, hpfar_el2
> > + mrs x7, esr_el2
> >
> > /* Prepare and exit to the host's panic funciton. */
> > mov lr, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\
> > PSR_MODE_EL1h)
> > msr spsr_el2, lr
> > - ldr lr, =panic
> > + ldr lr, =nvhe_hyp_panic_handler
> > msr elr_el2, lr
> >
> > /* Enter the host, restoring the host context if it was provided. */
> > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > index 8e7128cb7667..54b70189229b 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > @@ -22,8 +22,6 @@ void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt);
> > struct kvm_host_psci_config __ro_after_init kvm_host_psci_config;
> > s64 __ro_after_init hyp_physvirt_offset;
> >
> > -#define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset)
> > -
> > #define INVALID_CPU_ID UINT_MAX
> >
> > struct psci_boot_args {
> > --
> > 2.30.0.617.g56c4b15f3c-goog
> >
> >
>
> Otherwise, I like the idea!
>
> 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] 6+ messages in thread
* Re: [PATCH 2/2] KVM: arm64: Log source when panicking from nVHE hyp
2021-02-23 12:34 ` Andrew Scull
@ 2021-02-23 15:18 ` Marc Zyngier
0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2021-02-23 15:18 UTC (permalink / raw)
To: Andrew Scull; +Cc: kernel-team, catalin.marinas, will, kvmarm
On Tue, 23 Feb 2021 12:34:22 +0000,
Andrew Scull <ascull@google.com> wrote:
>
> On Tue, Feb 23, 2021 at 11:29:54AM +0000, Marc Zyngier wrote:
[...]
> > Would it be worth making it a helper that lives there? Something
> > like
> >
> > struct bug_entry *bug_get_entry(unsigned long pc, char **file,
> > unsigned int *line);
> >
> > that hides the #ifdefery and the fund_bug() call away? The
> > disable_trace_on_warning() call isn't a problem, as we're about to die
> > anyway.
>
> Yeah, that'd make a lot of sense. Please protect me from the wrath of
> the community when I touch the common code..
The community should be pleased if you gifted it with a piece of
reusable code instead of some less palatable copy-paste! :-)
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] 6+ messages in thread
end of thread, other threads:[~2021-02-23 15:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-23 9:49 [PATCH 0/2] Debug info for nVHE hyp panics Andrew Scull
2021-02-23 9:49 ` [PATCH 1/2] KVM: arm64: Use BUG and BUG_ON in nVHE hyp Andrew Scull
2021-02-23 9:49 ` [PATCH 2/2] KVM: arm64: Log source when panicking from " Andrew Scull
2021-02-23 11:29 ` Marc Zyngier
2021-02-23 12:34 ` Andrew Scull
2021-02-23 15:18 ` 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.