[PATCH v3 0/5] Debug info for nVHE hyp panics

[PATCH v3 0/5] Debug info for nVHE hyp panics

[Andrew Scull]

Panics from arm64's nVHE hyp mode are hard to interpret. This series
adds some more debug info to help with diagnosis.

Using BUG() in nVHE hyp gives a meaningful address to locate invariants
that fail to hold. The host can also look up the bug to provide the file
and line, if the debug configs are enabled. Otherwise a kimg address is
much more useful than a hyp VA since it can be looked up in vmlinux.

Applies on 5.12-rc3.

From v2 (https://lore.kernel.org/r/20210223155759.3495252-1-ascull@google.com/):
 - Rebased
 - An extra little refactor in bug.c for Steve
 - Handling for the randomized offset

From v1 (https://lore.kernel.org/r/20210223094927.766572-1-ascull@google.com/):
 - keeping struct bug details in bug.c
 - using SPSR to distinguish hyp from host
 - inlined __hyp_panic_string

Andrew Scull (5):
  bug: Remove redundant condition check in report_bug
  bug: Factor out a getter for a bug's file line
  bug: Assign values once in bug_get_file_line()
  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            | 45 +++++++++++++++++++++
 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 -
 arch/arm64/kvm/hyp/vhe/switch.c         |  4 +-
 include/linux/bug.h                     |  3 ++
 lib/bug.c                               | 54 +++++++++++++------------
 12 files changed, 91 insertions(+), 51 deletions(-)

-- 
2.31.0.rc2.261.g7f71774620-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 1/5] bug: Remove redundant condition check in report_bug

[Andrew Scull]

report_bug() will return early if it cannot find a bug corresponding to
the provided address. The subsequent test for the bug will always be
true so remove it.

Fixes: 1b4cfe3c0a30d ("lib/bug.c: exclude non-BUG/WARN exceptions from report_bug()")
Signed-off-by: Andrew Scull <ascull@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/bug.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/lib/bug.c b/lib/bug.c
index 8f9d537bfb2a..b92da1f6e21b 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -155,30 +155,27 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 
 	file = NULL;
 	line = 0;
-	warning = 0;
 
-	if (bug) {
 #ifdef CONFIG_DEBUG_BUGVERBOSE
 #ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
-		file = bug->file;
+	file = bug->file;
 #else
-		file = (const char *)bug + bug->file_disp;
+	file = (const char *)bug + bug->file_disp;
 #endif
-		line = bug->line;
+	line = bug->line;
 #endif
-		warning = (bug->flags & BUGFLAG_WARNING) != 0;
-		once = (bug->flags & BUGFLAG_ONCE) != 0;
-		done = (bug->flags & BUGFLAG_DONE) != 0;
-
-		if (warning && once) {
-			if (done)
-				return BUG_TRAP_TYPE_WARN;
-
-			/*
-			 * Since this is the only store, concurrency is not an issue.
-			 */
-			bug->flags |= BUGFLAG_DONE;
-		}
+	warning = (bug->flags & BUGFLAG_WARNING) != 0;
+	once = (bug->flags & BUGFLAG_ONCE) != 0;
+	done = (bug->flags & BUGFLAG_DONE) != 0;
+
+	if (warning && once) {
+		if (done)
+			return BUG_TRAP_TYPE_WARN;
+
+		/*
+		 * Since this is the only store, concurrency is not an issue.
+		 */
+		bug->flags |= BUGFLAG_DONE;
 	}
 
 	/*
-- 
2.31.0.rc2.261.g7f71774620-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 1/5] bug: Remove redundant condition check in report_bug

[Will Deacon]

Hi Andrew,

Thanks for the patch.

On Thu, Mar 18, 2021 at 02:33:07PM +0000, Andrew Scull wrote:
> report_bug() will return early if it cannot find a bug corresponding to
> the provided address. The subsequent test for the bug will always be
> true so remove it.
> 
> Fixes: 1b4cfe3c0a30d ("lib/bug.c: exclude non-BUG/WARN exceptions from report_bug()")

This is purely cosmetic, so I'm not sure this warrants a fixes tag (i.e. I
wouldn't expect this change to be backported).

> Signed-off-by: Andrew Scull <ascull@google.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  lib/bug.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)

But the change looks good:

Acked-by: Will Deacon <will@kernel.org>

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 2/5] bug: Factor out a getter for a bug's file line

[Andrew Scull]

There is some non-trivial config-based logic to get the file name and
line number associated with a bug. Factor this out to a getter that can
be resused.

Signed-off-by: Andrew Scull <ascull@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/bug.h |  3 +++
 lib/bug.c           | 27 +++++++++++++++++----------
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index f639bd0122f3..e3841bee4c8d 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -36,6 +36,9 @@ static inline int is_warning_bug(const struct bug_entry *bug)
 	return bug->flags & BUGFLAG_WARNING;
 }
 
+void bug_get_file_line(struct bug_entry *bug, const char **file,
+		       unsigned int *line);
+
 struct bug_entry *find_bug(unsigned long bugaddr);
 
 enum bug_trap_type report_bug(unsigned long bug_addr, struct pt_regs *regs);
diff --git a/lib/bug.c b/lib/bug.c
index b92da1f6e21b..e65398082cbc 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -127,6 +127,22 @@ static inline struct bug_entry *module_find_bug(unsigned long bugaddr)
 }
 #endif
 
+void bug_get_file_line(struct bug_entry *bug, const char **file,
+		       unsigned int *line)
+{
+	*file = NULL;
+	*line = 0;
+
+#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
+}
+
 struct bug_entry *find_bug(unsigned long bugaddr)
 {
 	struct bug_entry *bug;
@@ -153,17 +169,8 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 
 	disable_trace_on_warning();
 
-	file = NULL;
-	line = 0;
+	bug_get_file_line(bug, &file, &line);
 
-#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
 	warning = (bug->flags & BUGFLAG_WARNING) != 0;
 	once = (bug->flags & BUGFLAG_ONCE) != 0;
 	done = (bug->flags & BUGFLAG_DONE) != 0;
-- 
2.31.0.rc2.261.g7f71774620-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 2/5] bug: Factor out a getter for a bug's file line

[Will Deacon]

On Thu, Mar 18, 2021 at 02:33:08PM +0000, Andrew Scull wrote:
> There is some non-trivial config-based logic to get the file name and
> line number associated with a bug. Factor this out to a getter that can
> be resused.

typo: "resused"

> 
> Signed-off-by: Andrew Scull <ascull@google.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  include/linux/bug.h |  3 +++
>  lib/bug.c           | 27 +++++++++++++++++----------
>  2 files changed, 20 insertions(+), 10 deletions(-)

Patch looks fine:

Acked-by: Will Deacon <will@kernel.org>

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 3/5] bug: Assign values once in bug_get_file_line()

[Andrew Scull]

Set bug_get_file_line()'s output parameter values directly rather than
first nullifying them and then conditionally setting new values.

Signed-off-by: Andrew Scull <ascull@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
---
 lib/bug.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/bug.c b/lib/bug.c
index e65398082cbc..45a0584f6541 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -130,9 +130,6 @@ static inline struct bug_entry *module_find_bug(unsigned long bugaddr)
 void bug_get_file_line(struct bug_entry *bug, const char **file,
 		       unsigned int *line)
 {
-	*file = NULL;
-	*line = 0;
-
 #ifdef CONFIG_DEBUG_BUGVERBOSE
 #ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
 	*file = bug->file;
@@ -140,6 +137,9 @@ void bug_get_file_line(struct bug_entry *bug, const char **file,
 	*file = (const char *)bug + bug->file_disp;
 #endif
 	*line = bug->line;
+#else
+	*file = NULL;
+	*line = 0;
 #endif
 }
 
-- 
2.31.0.rc2.261.g7f71774620-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 3/5] bug: Assign values once in bug_get_file_line()

[Will Deacon]

On Thu, Mar 18, 2021 at 02:33:09PM +0000, Andrew Scull wrote:
> Set bug_get_file_line()'s output parameter values directly rather than
> first nullifying them and then conditionally setting new values.
> 
> Signed-off-by: Andrew Scull <ascull@google.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> ---
>  lib/bug.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/bug.c b/lib/bug.c
> index e65398082cbc..45a0584f6541 100644
> --- a/lib/bug.c
> +++ b/lib/bug.c
> @@ -130,9 +130,6 @@ static inline struct bug_entry *module_find_bug(unsigned long bugaddr)
>  void bug_get_file_line(struct bug_entry *bug, const char **file,
>  		       unsigned int *line)
>  {
> -	*file = NULL;
> -	*line = 0;
> -
>  #ifdef CONFIG_DEBUG_BUGVERBOSE
>  #ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
>  	*file = bug->file;
> @@ -140,6 +137,9 @@ void bug_get_file_line(struct bug_entry *bug, const char **file,
>  	*file = (const char *)bug + bug->file_disp;
>  #endif
>  	*line = bug->line;
> +#else
> +	*file = NULL;
> +	*line = 0;

Acked-by: Will Deacon <will@kernel.org>

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 4/5] KVM: arm64: Use BUG and BUG_ON in nVHE hyp

[Andrew Scull]

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 an
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 32ae676236b6..fe5fc814f228 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -100,7 +100,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 936328207bde..821a69601dd9 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -178,6 +178,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 879559057dee..9f54833af400 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 *)&kvm_arm_hyp_percpu_base;
 	this_cpu_base = kern_hyp_va(cpu_base_array[cpu]);
-- 
2.31.0.rc2.261.g7f71774620-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 4/5] KVM: arm64: Use BUG and BUG_ON in nVHE hyp

[Will Deacon]

On Thu, Mar 18, 2021 at 02:33:10PM +0000, Andrew Scull wrote:
> 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 an
> 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(-)

Makes sense to me:

Acked-by: Will Deacon <will@kernel.org>

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 5/5] KVM: arm64: Log source when panicking from nVHE hyp

[Andrew Scull]

To aid with debugging, add details of the source of a panic from nVHE
hyp. 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 an address that can be looked up
in vmlinux. The hyp offset is also logged to allow other hyp VAs to be
converted, similar to how the kernel offset is logged during a panic.

__hyp_panic_string is now inlined since it no longer needs to be
referenced as a symbol and the message is free to diverge between VHE
and nVHE.

The following is an example of the logs generated by a BUG in nVHE hyp.

[   46.754840] kvm [307]: nVHE hyp BUG at: arch/arm64/kvm/hyp/nvhe/switch.c:242!
[   46.755357] kvm [307]: Hyp Offset: 0xfffea6c58e1e0000
[   46.755824] Kernel panic - not syncing: HYP panic:
[   46.755824] PS:400003c9 PC:0000d93a82c705ac ESR:f2000800
[   46.755824] FAR:0000000080080000 HPFAR:0000000000800800 PAR:0000000000000000
[   46.755824] VCPU:0000d93a880d0000
[   46.756960] CPU: 3 PID: 307 Comm: kvm-vcpu-0 Not tainted 5.12.0-rc3-00005-gc572b99cf65b-dirty #133
[   46.757459] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
[   46.758366] Call trace:
[   46.758601]  dump_backtrace+0x0/0x1b0
[   46.758856]  show_stack+0x18/0x70
[   46.759057]  dump_stack+0xd0/0x12c
[   46.759236]  panic+0x16c/0x334
[   46.759426]  arm64_kernel_unmapped_at_el0+0x0/0x30
[   46.759661]  kvm_arch_vcpu_ioctl_run+0x134/0x750
[   46.759936]  kvm_vcpu_ioctl+0x2f0/0x970
[   46.760156]  __arm64_sys_ioctl+0xa8/0xec
[   46.760379]  el0_svc_common.constprop.0+0x60/0x120
[   46.760627]  do_el0_svc+0x24/0x90
[   46.760766]  el0_svc+0x2c/0x54
[   46.760915]  el0_sync_handler+0x1a4/0x1b0
[   46.761146]  el0_sync+0x170/0x180
[   46.761889] SMP: stopping secondary CPUs
[   46.762786] Kernel Offset: 0x3e1cd2820000 from 0xffff800010000000
[   46.763142] PHYS_OFFSET: 0xffffa9f680000000
[   46.763359] CPU features: 0x00240022,61806008
[   46.763651] Memory Limit: none
[   46.813867] ---[ end Kernel panic - not syncing: HYP panic:
[   46.813867] PS:400003c9 PC:0000d93a82c705ac ESR:f2000800
[   46.813867] FAR:0000000080080000 HPFAR:0000000000800800 PAR:0000000000000000
[   46.813867] VCPU:0000d93a880d0000 ]---

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            | 45 +++++++++++++++++++++++++
 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 --
 arch/arm64/kvm/hyp/vhe/switch.c         |  4 +--
 7 files changed, 56 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 90873851f677..7c17a67d2291 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -121,6 +121,8 @@ void kvm_update_va_mask(struct alt_instr *alt,
 void kvm_compute_layout(void);
 void kvm_apply_hyp_relocations(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 5aa9ed1e9ec6..5ff2b6909387 100644
--- a/arch/arm64/kernel/image-vars.h
+++ b/arch/arm64/kernel/image-vars.h
@@ -70,8 +70,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..6f48336b1d86 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -291,3 +291,48 @@ 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(u64 esr, u64 spsr, u64 elr,
+					      u64 par, uintptr_t vcpu,
+					      u64 far, u64 hpfar) {
+	u64 elr_in_kimg = __phys_to_kimg(__hyp_pa(elr));
+	u64 hyp_offset = elr_in_kimg - kaslr_offset() - elr;
+	u64 mode = spsr & PSR_MODE_MASK;
+
+	/*
+	 * The nVHE hyp symbols are not included by kallsyms to avoid issues
+	 * with aliasing. That means that the symbols cannot be printed with the
+	 * "%pS" format specifier, so fall back to the vmlinux address if
+	 * there's no better option.
+	 */
+	if (mode != PSR_MODE_EL2t && mode != PSR_MODE_EL2h) {
+		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 int line = 0;
+
+		/* All hyp bugs, including warnings, are treated as fatal. */
+		if (bug)
+			bug_get_file_line(bug, &file, &line);
+
+		if (file)
+			kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
+		else
+			kvm_err("nVHE hyp BUG at: %016llx!\n", elr + hyp_offset);
+	} else {
+		kvm_err("nVHE hyp panic at: %016llx!\n", elr + hyp_offset);
+	}
+
+	/*
+	 * Hyp has panicked and we're going to handle that by panicking the
+	 * kernel. The kernel offset will be revealed in the panic so we're
+	 * also safe to reveal the hyp offset as a debugging aid for translating
+	 * hyp VAs to vmlinux addresses.
+	 */
+	kvm_err("Hyp Offset: 0x%llx\n", hyp_offset);
+
+	panic("HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%016lx\n",
+	      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 6c1f51f25eb3..32d0c036c050 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 5d94584840cc..2b23400e0fb3 100644
--- a/arch/arm64/kvm/hyp/nvhe/host.S
+++ b/arch/arm64/kvm/hyp/nvhe/host.S
@@ -79,22 +79,18 @@ SYM_FUNC_START(__hyp_do_panic)
 	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
 	hyp_kimg_va lr, x6
 	msr	elr_el2, lr
 
 	mov	x29, x0
 
-	/* Load the format string into x0 and arguments into x1-7 */
-	ldr	x0, =__hyp_panic_string
-	hyp_kimg_va x0, x6
-
-	/* Load the format arguments into x1-7. */
-	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 */
+	mrs	x0, esr_el2
+	get_vcpu_ptr x4, x5
+	mrs	x5, far_el2
+	mrs	x6, hpfar_el2
+	mov	x7, xzr			// Unused argument
 
 	/* Enter the host, conditionally restoring the host context. */
 	cbz	x29, __host_enter_without_restoring
diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
index 63de71c0481e..ca630896476d 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 {
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
index af8e940d0f03..7b8f7db5c1ed 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -27,8 +27,6 @@
 #include <asm/processor.h>
 #include <asm/thread_info.h>
 
-const char __hyp_panic_string[] = "HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n";
-
 /* VHE specific context */
 DEFINE_PER_CPU(struct kvm_host_data, kvm_host_data);
 DEFINE_PER_CPU(struct kvm_cpu_context, kvm_hyp_ctxt);
@@ -207,7 +205,7 @@ static void __hyp_call_panic(u64 spsr, u64 elr, u64 par)
 	__deactivate_traps(vcpu);
 	sysreg_restore_host_state_vhe(host_ctxt);
 
-	panic(__hyp_panic_string,
+	panic("HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n",
 	      spsr, elr,
 	      read_sysreg_el2(SYS_ESR), read_sysreg_el2(SYS_FAR),
 	      read_sysreg(hpfar_el2), par, vcpu);
-- 
2.31.0.rc2.261.g7f71774620-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 5/5] KVM: arm64: Log source when panicking from nVHE hyp

[Will Deacon]

On Thu, Mar 18, 2021 at 02:33:11PM +0000, Andrew Scull wrote:
> To aid with debugging, add details of the source of a panic from nVHE
> hyp. 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 an address that can be looked up
> in vmlinux. The hyp offset is also logged to allow other hyp VAs to be
> converted, similar to how the kernel offset is logged during a panic.
> 
> __hyp_panic_string is now inlined since it no longer needs to be
> referenced as a symbol and the message is free to diverge between VHE
> and nVHE.
> 
> The following is an example of the logs generated by a BUG in nVHE hyp.
> 
> [   46.754840] kvm [307]: nVHE hyp BUG at: arch/arm64/kvm/hyp/nvhe/switch.c:242!
> [   46.755357] kvm [307]: Hyp Offset: 0xfffea6c58e1e0000
> [   46.755824] Kernel panic - not syncing: HYP panic:
> [   46.755824] PS:400003c9 PC:0000d93a82c705ac ESR:f2000800
> [   46.755824] FAR:0000000080080000 HPFAR:0000000000800800 PAR:0000000000000000
> [   46.755824] VCPU:0000d93a880d0000
> [   46.756960] CPU: 3 PID: 307 Comm: kvm-vcpu-0 Not tainted 5.12.0-rc3-00005-gc572b99cf65b-dirty #133
> [   46.757459] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
> [   46.758366] Call trace:
> [   46.758601]  dump_backtrace+0x0/0x1b0
> [   46.758856]  show_stack+0x18/0x70
> [   46.759057]  dump_stack+0xd0/0x12c
> [   46.759236]  panic+0x16c/0x334
> [   46.759426]  arm64_kernel_unmapped_at_el0+0x0/0x30
> [   46.759661]  kvm_arch_vcpu_ioctl_run+0x134/0x750
> [   46.759936]  kvm_vcpu_ioctl+0x2f0/0x970
> [   46.760156]  __arm64_sys_ioctl+0xa8/0xec
> [   46.760379]  el0_svc_common.constprop.0+0x60/0x120
> [   46.760627]  do_el0_svc+0x24/0x90
> [   46.760766]  el0_svc+0x2c/0x54
> [   46.760915]  el0_sync_handler+0x1a4/0x1b0
> [   46.761146]  el0_sync+0x170/0x180
> [   46.761889] SMP: stopping secondary CPUs
> [   46.762786] Kernel Offset: 0x3e1cd2820000 from 0xffff800010000000
> [   46.763142] PHYS_OFFSET: 0xffffa9f680000000
> [   46.763359] CPU features: 0x00240022,61806008
> [   46.763651] Memory Limit: none

Nice!

> [   46.813867] ---[ end Kernel panic - not syncing: HYP panic:
> [   46.813867] PS:400003c9 PC:0000d93a82c705ac ESR:f2000800
> [   46.813867] FAR:0000000080080000 HPFAR:0000000000800800 PAR:0000000000000000
> [   46.813867] VCPU:0000d93a880d0000 ]---

Why did these last three lines get printed twice?

> 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            | 45 +++++++++++++++++++++++++
>  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 --
>  arch/arm64/kvm/hyp/vhe/switch.c         |  4 +--
>  7 files changed, 56 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 90873851f677..7c17a67d2291 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -121,6 +121,8 @@ void kvm_update_va_mask(struct alt_instr *alt,
>  void kvm_compute_layout(void);
>  void kvm_apply_hyp_relocations(void);
>  
> +#define __hyp_pa(x) (((phys_addr_t)(x)) + hyp_physvirt_offset)

Just a heads up: but Quentin's series moves this same macro, but to a
different header (arch/arm64/kvm/hyp/include/nvhe/memory.h)

>  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 5aa9ed1e9ec6..5ff2b6909387 100644
> --- a/arch/arm64/kernel/image-vars.h
> +++ b/arch/arm64/kernel/image-vars.h
> @@ -70,8 +70,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..6f48336b1d86 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -291,3 +291,48 @@ 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(u64 esr, u64 spsr, u64 elr,
> +					      u64 par, uintptr_t vcpu,
> +					      u64 far, u64 hpfar) {

Interesting, I don't think I've seen __cold used in arch/arm64 before. Does
it make any difference to the generated code?

> +	u64 elr_in_kimg = __phys_to_kimg(__hyp_pa(elr));
> +	u64 hyp_offset = elr_in_kimg - kaslr_offset() - elr;
> +	u64 mode = spsr & PSR_MODE_MASK;
> +
> +	/*
> +	 * The nVHE hyp symbols are not included by kallsyms to avoid issues
> +	 * with aliasing. That means that the symbols cannot be printed with the
> +	 * "%pS" format specifier, so fall back to the vmlinux address if
> +	 * there's no better option.
> +	 */
> +	if (mode != PSR_MODE_EL2t && mode != PSR_MODE_EL2h) {
> +		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 int line = 0;
> +
> +		/* All hyp bugs, including warnings, are treated as fatal. */
> +		if (bug)
> +			bug_get_file_line(bug, &file, &line);
> +
> +		if (file)
> +			kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
> +		else
> +			kvm_err("nVHE hyp BUG at: %016llx!\n", elr + hyp_offset);
> +	} else {
> +		kvm_err("nVHE hyp panic at: %016llx!\n", elr + hyp_offset);
> +	}
> +
> +	/*
> +	 * Hyp has panicked and we're going to handle that by panicking the
> +	 * kernel. The kernel offset will be revealed in the panic so we're
> +	 * also safe to reveal the hyp offset as a debugging aid for translating
> +	 * hyp VAs to vmlinux addresses.
> +	 */
> +	kvm_err("Hyp Offset: 0x%llx\n", hyp_offset);
> +
> +	panic("HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%016lx\n",
> +	      spsr, elr, esr, far, hpfar, par, vcpu);

Is %016lx the right conversion specifier for uintptr_t?

> +}
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 6c1f51f25eb3..32d0c036c050 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 5d94584840cc..2b23400e0fb3 100644
> --- a/arch/arm64/kvm/hyp/nvhe/host.S
> +++ b/arch/arm64/kvm/hyp/nvhe/host.S
> @@ -79,22 +79,18 @@ SYM_FUNC_START(__hyp_do_panic)
>  	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
>  	hyp_kimg_va lr, x6
>  	msr	elr_el2, lr
>  
>  	mov	x29, x0
>  
> -	/* Load the format string into x0 and arguments into x1-7 */
> -	ldr	x0, =__hyp_panic_string
> -	hyp_kimg_va x0, x6
> -
> -	/* Load the format arguments into x1-7. */
> -	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 */
> +	mrs	x0, esr_el2
> +	get_vcpu_ptr x4, x5
> +	mrs	x5, far_el2
> +	mrs	x6, hpfar_el2
> +	mov	x7, xzr			// Unused argument

Why do you need to clear x7 if it's unused?

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 5/5] KVM: arm64: Log source when panicking from nVHE hyp

[Marc Zyngier]

On Thu, 18 Mar 2021 16:59:47 +0000,
Will Deacon <will@kernel.org> wrote:
> 
> On Thu, Mar 18, 2021 at 02:33:11PM +0000, Andrew Scull wrote:
> > To aid with debugging, add details of the source of a panic from nVHE
> > hyp. 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 an address that can be looked up
> > in vmlinux. The hyp offset is also logged to allow other hyp VAs to be
> > converted, similar to how the kernel offset is logged during a panic.
> > 
> > __hyp_panic_string is now inlined since it no longer needs to be
> > referenced as a symbol and the message is free to diverge between VHE
> > and nVHE.
> > 
> > The following is an example of the logs generated by a BUG in nVHE hyp.
> > 
> > [   46.754840] kvm [307]: nVHE hyp BUG at: arch/arm64/kvm/hyp/nvhe/switch.c:242!
> > [   46.755357] kvm [307]: Hyp Offset: 0xfffea6c58e1e0000
> > [   46.755824] Kernel panic - not syncing: HYP panic:
> > [   46.755824] PS:400003c9 PC:0000d93a82c705ac ESR:f2000800
> > [   46.755824] FAR:0000000080080000 HPFAR:0000000000800800 PAR:0000000000000000
> > [   46.755824] VCPU:0000d93a880d0000
> > [   46.756960] CPU: 3 PID: 307 Comm: kvm-vcpu-0 Not tainted 5.12.0-rc3-00005-gc572b99cf65b-dirty #133
> > [   46.757459] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
> > [   46.758366] Call trace:
> > [   46.758601]  dump_backtrace+0x0/0x1b0
> > [   46.758856]  show_stack+0x18/0x70
> > [   46.759057]  dump_stack+0xd0/0x12c
> > [   46.759236]  panic+0x16c/0x334
> > [   46.759426]  arm64_kernel_unmapped_at_el0+0x0/0x30
> > [   46.759661]  kvm_arch_vcpu_ioctl_run+0x134/0x750
> > [   46.759936]  kvm_vcpu_ioctl+0x2f0/0x970
> > [   46.760156]  __arm64_sys_ioctl+0xa8/0xec
> > [   46.760379]  el0_svc_common.constprop.0+0x60/0x120
> > [   46.760627]  do_el0_svc+0x24/0x90
> > [   46.760766]  el0_svc+0x2c/0x54
> > [   46.760915]  el0_sync_handler+0x1a4/0x1b0
> > [   46.761146]  el0_sync+0x170/0x180
> > [   46.761889] SMP: stopping secondary CPUs
> > [   46.762786] Kernel Offset: 0x3e1cd2820000 from 0xffff800010000000
> > [   46.763142] PHYS_OFFSET: 0xffffa9f680000000
> > [   46.763359] CPU features: 0x00240022,61806008
> > [   46.763651] Memory Limit: none
> 
> Nice!
> 
> > [   46.813867] ---[ end Kernel panic - not syncing: HYP panic:
> > [   46.813867] PS:400003c9 PC:0000d93a82c705ac ESR:f2000800
> > [   46.813867] FAR:0000000080080000 HPFAR:0000000000800800 PAR:0000000000000000
> > [   46.813867] VCPU:0000d93a880d0000 ]---
> 
> Why did these last three lines get printed twice?

That's the panic string that gets repeated, a standard "feature" of
the panic code:

root@tiger-roach:~# echo -n 'c' > /proc/sysrq-trigger 
[250622.941867] sysrq: Trigger a crash
[250622.941903] Kernel panic - not syncing: sysrq triggered crash
[250622.945515] CPU: 0 PID: 4890 Comm: bash Tainted: G            E     5.11.0 #3124
[250622.952930] Hardware name:  , BIOS 2021.01-rc2-00012-gde865f7ee1 11/16/2020
[250622.959917] Call trace:
[250622.962417]  dump_backtrace+0x0/0x1e4
[250622.966126]  show_stack+0x24/0x80
[250622.969490]  dump_stack+0xc8/0x120
[250622.972940]  panic+0x178/0x38c
[250622.976045]  sysrq_handle_crash+0x28/0x30
[250622.980098]  __handle_sysrq+0x98/0x1a4
[250622.983893]  write_sysrq_trigger+0x94/0x12c
[250622.988120]  proc_reg_write+0xb4/0xf0
[250622.991828]  vfs_write+0xfc/0x29c
[250622.995192]  ksys_write+0x74/0x100
[250622.998642]  __arm64_sys_write+0x28/0x3c
[250623.002610]  el0_svc_common.constprop.0+0x80/0x1c0
[250623.007440]  do_el0_svc+0x30/0xa0
[250623.010803]  el0_svc+0x28/0x70
[250623.013908]  el0_sync_handler+0x1a8/0x1ac
[250623.017962]  el0_sync+0x174/0x180
[250623.021331] SMP: stopping secondary CPUs
[250623.025301] Kernel Offset: 0x435917bf0000 from 0xffff800010000000
[250623.031417] PHYS_OFFSET: 0xffff9bc5c0000000
[250623.035643] CPU features: 0x08240022,2aa0a830
[250623.040042] Memory Limit: none
[250623.043153] ---[ end Kernel panic - not syncing: sysrq triggered crash ]---

KVM just happens to have a fairly large, multi line panic string.

	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

Re: [PATCH v3 5/5] KVM: arm64: Log source when panicking from nVHE hyp

[Andrew Scull]

On Thu, 18 Mar 2021 at 16:59, Will Deacon <will@kernel.org> wrote:
>
> On Thu, Mar 18, 2021 at 02:33:11PM +0000, Andrew Scull wrote:
> > To aid with debugging, add details of the source of a panic from nVHE
> > hyp. 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 an address that can be looked up
> > in vmlinux. The hyp offset is also logged to allow other hyp VAs to be
> > converted, similar to how the kernel offset is logged during a panic.
> >
> > __hyp_panic_string is now inlined since it no longer needs to be
> > referenced as a symbol and the message is free to diverge between VHE
> > and nVHE.
> >
> > The following is an example of the logs generated by a BUG in nVHE hyp.
> >
> > [   46.754840] kvm [307]: nVHE hyp BUG at: arch/arm64/kvm/hyp/nvhe/switch.c:242!
> > [   46.755357] kvm [307]: Hyp Offset: 0xfffea6c58e1e0000
> > [   46.755824] Kernel panic - not syncing: HYP panic:
> > [   46.755824] PS:400003c9 PC:0000d93a82c705ac ESR:f2000800
> > [   46.755824] FAR:0000000080080000 HPFAR:0000000000800800 PAR:0000000000000000
> > [   46.755824] VCPU:0000d93a880d0000
> > [   46.756960] CPU: 3 PID: 307 Comm: kvm-vcpu-0 Not tainted 5.12.0-rc3-00005-gc572b99cf65b-dirty #133
> > [   46.757459] Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
> > [   46.758366] Call trace:
> > [   46.758601]  dump_backtrace+0x0/0x1b0
> > [   46.758856]  show_stack+0x18/0x70
> > [   46.759057]  dump_stack+0xd0/0x12c
> > [   46.759236]  panic+0x16c/0x334
> > [   46.759426]  arm64_kernel_unmapped_at_el0+0x0/0x30
> > [   46.759661]  kvm_arch_vcpu_ioctl_run+0x134/0x750
> > [   46.759936]  kvm_vcpu_ioctl+0x2f0/0x970
> > [   46.760156]  __arm64_sys_ioctl+0xa8/0xec
> > [   46.760379]  el0_svc_common.constprop.0+0x60/0x120
> > [   46.760627]  do_el0_svc+0x24/0x90
> > [   46.760766]  el0_svc+0x2c/0x54
> > [   46.760915]  el0_sync_handler+0x1a4/0x1b0
> > [   46.761146]  el0_sync+0x170/0x180
> > [   46.761889] SMP: stopping secondary CPUs
> > [   46.762786] Kernel Offset: 0x3e1cd2820000 from 0xffff800010000000
> > [   46.763142] PHYS_OFFSET: 0xffffa9f680000000
> > [   46.763359] CPU features: 0x00240022,61806008
> > [   46.763651] Memory Limit: none
>
> Nice!
>
> > [   46.813867] ---[ end Kernel panic - not syncing: HYP panic:
> > [   46.813867] PS:400003c9 PC:0000d93a82c705ac ESR:f2000800
> > [   46.813867] FAR:0000000080080000 HPFAR:0000000000800800 PAR:0000000000000000
> > [   46.813867] VCPU:0000d93a880d0000 ]---
>
> Why did these last three lines get printed twice?
>
> > 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            | 45 +++++++++++++++++++++++++
> >  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 --
> >  arch/arm64/kvm/hyp/vhe/switch.c         |  4 +--
> >  7 files changed, 56 insertions(+), 20 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > index 90873851f677..7c17a67d2291 100644
> > --- a/arch/arm64/include/asm/kvm_mmu.h
> > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > @@ -121,6 +121,8 @@ void kvm_update_va_mask(struct alt_instr *alt,
> >  void kvm_compute_layout(void);
> >  void kvm_apply_hyp_relocations(void);
> >
> > +#define __hyp_pa(x) (((phys_addr_t)(x)) + hyp_physvirt_offset)
>
> Just a heads up: but Quentin's series moves this same macro, but to a
> different header (arch/arm64/kvm/hyp/include/nvhe/memory.h)

I can make sure we're putting it in the same header to ease the merge.

> >  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 5aa9ed1e9ec6..5ff2b6909387 100644
> > --- a/arch/arm64/kernel/image-vars.h
> > +++ b/arch/arm64/kernel/image-vars.h
> > @@ -70,8 +70,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..6f48336b1d86 100644
> > --- a/arch/arm64/kvm/handle_exit.c
> > +++ b/arch/arm64/kvm/handle_exit.c
> > @@ -291,3 +291,48 @@ 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(u64 esr, u64 spsr, u64 elr,
> > +                                           u64 par, uintptr_t vcpu,
> > +                                           u64 far, u64 hpfar) {
>
> Interesting, I don't think I've seen __cold used in arch/arm64 before. Does
> it make any difference to the generated code?

Honestly, I just copied the signature of panic. There was no good
reason for it so I'll drop it if it stands out as odd.

> > +     u64 elr_in_kimg = __phys_to_kimg(__hyp_pa(elr));
> > +     u64 hyp_offset = elr_in_kimg - kaslr_offset() - elr;
> > +     u64 mode = spsr & PSR_MODE_MASK;
> > +
> > +     /*
> > +      * The nVHE hyp symbols are not included by kallsyms to avoid issues
> > +      * with aliasing. That means that the symbols cannot be printed with the
> > +      * "%pS" format specifier, so fall back to the vmlinux address if
> > +      * there's no better option.
> > +      */
> > +     if (mode != PSR_MODE_EL2t && mode != PSR_MODE_EL2h) {
> > +             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 int line = 0;
> > +
> > +             /* All hyp bugs, including warnings, are treated as fatal. */
> > +             if (bug)
> > +                     bug_get_file_line(bug, &file, &line);
> > +
> > +             if (file)
> > +                     kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
> > +             else
> > +                     kvm_err("nVHE hyp BUG at: %016llx!\n", elr + hyp_offset);
> > +     } else {
> > +             kvm_err("nVHE hyp panic at: %016llx!\n", elr + hyp_offset);
> > +     }
> > +
> > +     /*
> > +      * Hyp has panicked and we're going to handle that by panicking the
> > +      * kernel. The kernel offset will be revealed in the panic so we're
> > +      * also safe to reveal the hyp offset as a debugging aid for translating
> > +      * hyp VAs to vmlinux addresses.
> > +      */
> > +     kvm_err("Hyp Offset: 0x%llx\n", hyp_offset);
> > +
> > +     panic("HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%016lx\n",
> > +           spsr, elr, esr, far, hpfar, par, vcpu);
>
> Is %016lx the right conversion specifier for uintptr_t?

PRIxPTR looks to be the right specifier in the standards, but that's
not in the kernel. `uintptr_t` looks to be defined as `unsigned long`
in types.h which would make %lx the right specifier, IIUC.

> > +}
> > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > index 6c1f51f25eb3..32d0c036c050 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 5d94584840cc..2b23400e0fb3 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/host.S
> > +++ b/arch/arm64/kvm/hyp/nvhe/host.S
> > @@ -79,22 +79,18 @@ SYM_FUNC_START(__hyp_do_panic)
> >       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
> >       hyp_kimg_va lr, x6
> >       msr     elr_el2, lr
> >
> >       mov     x29, x0
> >
> > -     /* Load the format string into x0 and arguments into x1-7 */
> > -     ldr     x0, =__hyp_panic_string
> > -     hyp_kimg_va x0, x6
> > -
> > -     /* Load the format arguments into x1-7. */
> > -     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 */
> > +     mrs     x0, esr_el2
> > +     get_vcpu_ptr x4, x5
> > +     mrs     x5, far_el2
> > +     mrs     x6, hpfar_el2
> > +     mov     x7, xzr                 // Unused argument
>
> Why do you need to clear x7 if it's unused?

x7 is one of the registers that's preserved by __host_enter_for_panic
so I was probably thinking that __host_enter_for_panic takes arguments
x0-7, so let's initialize x7.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 5/5] KVM: arm64: Log source when panicking from nVHE hyp

[Andrew Scull]

> > > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > > index 90873851f677..7c17a67d2291 100644
> > > --- a/arch/arm64/include/asm/kvm_mmu.h
> > > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > > @@ -121,6 +121,8 @@ void kvm_update_va_mask(struct alt_instr *alt,
> > >  void kvm_compute_layout(void);
> > >  void kvm_apply_hyp_relocations(void);
> > >
> > > +#define __hyp_pa(x) (((phys_addr_t)(x)) + hyp_physvirt_offset)
> >
> > Just a heads up: but Quentin's series moves this same macro, but to a
> > different header (arch/arm64/kvm/hyp/include/nvhe/memory.h)
>
> I can make sure we're putting it in the same header to ease the merge.

Seems I was too optimistic since nvhe/memory.h can only be used from
nVHE hyp. I could rebase no top of Quentin to move the macro somewhere
that can be used from here, but that would add a series dependency
that you usually seem to try and avoid so I'm not going to change
this, unless you'd prefer that I do.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 5/5] KVM: arm64: Log source when panicking from nVHE hyp

[Marc Zyngier]

On 2021-03-19 10:38, Andrew Scull wrote:
>> > > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> > > index 90873851f677..7c17a67d2291 100644
>> > > --- a/arch/arm64/include/asm/kvm_mmu.h
>> > > +++ b/arch/arm64/include/asm/kvm_mmu.h
>> > > @@ -121,6 +121,8 @@ void kvm_update_va_mask(struct alt_instr *alt,
>> > >  void kvm_compute_layout(void);
>> > >  void kvm_apply_hyp_relocations(void);
>> > >
>> > > +#define __hyp_pa(x) (((phys_addr_t)(x)) + hyp_physvirt_offset)
>> >
>> > Just a heads up: but Quentin's series moves this same macro, but to a
>> > different header (arch/arm64/kvm/hyp/include/nvhe/memory.h)
>> 
>> I can make sure we're putting it in the same header to ease the merge.
> 
> Seems I was too optimistic since nvhe/memory.h can only be used from
> nVHE hyp. I could rebase no top of Quentin to move the macro somewhere
> that can be used from here, but that would add a series dependency
> that you usually seem to try and avoid so I'm not going to change
> this, unless you'd prefer that I do.

I can deal with this at merge time.

         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

Re: [PATCH v3 0/5] Debug info for nVHE hyp panics

[Marc Zyngier]

On Thu, 18 Mar 2021 14:33:06 +0000, Andrew Scull wrote:
> Panics from arm64's nVHE hyp mode are hard to interpret. This series
> adds some more debug info to help with diagnosis.
> 
> Using BUG() in nVHE hyp gives a meaningful address to locate invariants
> that fail to hold. The host can also look up the bug to provide the file
> and line, if the debug configs are enabled. Otherwise a kimg address is
> much more useful than a hyp VA since it can be looked up in vmlinux.
> 
> [...]

Applied to kvm-arm64/nvhe-panic-info, thanks!

[1/5] bug: Remove redundant condition check in report_bug
      commit: 3ad1a6cb0abc63d036fc866bd7c2c5983516dec5
[2/5] bug: Factor out a getter for a bug's file line
      commit: 26dbc7e299c7ebbb6a95e2c620b21b5280b37c57
[3/5] bug: Assign values once in bug_get_file_line()
      commit: 5b8be5d875a996776708ba174fcd08c8bcd721a5
[4/5] KVM: arm64: Use BUG and BUG_ON in nVHE hyp
      commit: f79e616f27ab6cd74deb0995a8eead3d1c9d65af
[5/5] KVM: arm64: Log source when panicking from nVHE hyp
      commit: aec0fae62e47050019474936248a311a0ab08705

Cheers,

	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