linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Debug info for nVHE hyp panics
@ 2021-02-23 15:57 Andrew Scull
  2021-02-23 15:57 ` [PATCH v2 1/4] bug: Remove redundant condition check in report_bug Andrew Scull
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Andrew Scull @ 2021-02-23 15:57 UTC (permalink / raw)
  To: kvmarm
  Cc: linux-kernel, maz, james.morse, suzuki.poulose,
	julien.thierry.kdev, will, catalin.marinas, kernel-team,
	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.

The lib/bug.c changes apply on 5.11.

This arm64 KVM changes apply on top of the previous panic fix at
https://lore.kernel.org/r/20210219122406.1337626-1-ascull@google.com/

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 (4):
  bug: Remove redundant condition check in report_bug
  bug: Factor out a getter for a bug's 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            | 31 ++++++++++++++
 arch/arm64/kvm/hyp/include/hyp/switch.h |  2 -
 arch/arm64/kvm/hyp/nvhe/host.S          | 17 ++++----
 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, 77 insertions(+), 50 deletions(-)

-- 
2.30.0.617.g56c4b15f3c-goog


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/4] bug: Remove redundant condition check in report_bug
  2021-02-23 15:57 [PATCH v2 0/4] Debug info for nVHE hyp panics Andrew Scull
@ 2021-02-23 15:57 ` Andrew Scull
  2021-02-23 16:25   ` Steven Rostedt
  2021-02-23 15:57 ` [PATCH v2 2/4] bug: Factor out a getter for a bug's file line Andrew Scull
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Andrew Scull @ 2021-02-23 15:57 UTC (permalink / raw)
  To: kvmarm
  Cc: linux-kernel, maz, james.morse, suzuki.poulose,
	julien.thierry.kdev, will, catalin.marinas, kernel-team,
	Andrew Scull, Peter Zijlstra, Steven Rostedt (VMware)

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.

Signed-off-by: Andrew Scull <ascull@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "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 7103440c0ee1..4ab398a2de93 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -158,30 +158,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.30.0.617.g56c4b15f3c-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/4] bug: Factor out a getter for a bug's file line
  2021-02-23 15:57 [PATCH v2 0/4] Debug info for nVHE hyp panics Andrew Scull
  2021-02-23 15:57 ` [PATCH v2 1/4] bug: Remove redundant condition check in report_bug Andrew Scull
@ 2021-02-23 15:57 ` Andrew Scull
  2021-02-23 16:35   ` Steven Rostedt
  2021-02-23 15:57 ` [PATCH v2 3/4] KVM: arm64: Use BUG and BUG_ON in nVHE hyp Andrew Scull
  2021-02-23 15:57 ` [PATCH v2 4/4] KVM: arm64: Log source when panicking from " Andrew Scull
  3 siblings, 1 reply; 7+ messages in thread
From: Andrew Scull @ 2021-02-23 15:57 UTC (permalink / raw)
  To: kvmarm
  Cc: linux-kernel, maz, james.morse, suzuki.poulose,
	julien.thierry.kdev, will, catalin.marinas, kernel-team,
	Andrew Scull, Peter Zijlstra, Steven Rostedt (VMware)

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>
---
 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 4ab398a2de93..f936615176b8 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -130,6 +130,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;
@@ -156,17 +172,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.30.0.617.g56c4b15f3c-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 3/4] KVM: arm64: Use BUG and BUG_ON in nVHE hyp
  2021-02-23 15:57 [PATCH v2 0/4] Debug info for nVHE hyp panics Andrew Scull
  2021-02-23 15:57 ` [PATCH v2 1/4] bug: Remove redundant condition check in report_bug Andrew Scull
  2021-02-23 15:57 ` [PATCH v2 2/4] bug: Factor out a getter for a bug's file line Andrew Scull
@ 2021-02-23 15:57 ` Andrew Scull
  2021-02-23 15:57 ` [PATCH v2 4/4] KVM: arm64: Log source when panicking from " Andrew Scull
  3 siblings, 0 replies; 7+ messages in thread
From: Andrew Scull @ 2021-02-23 15:57 UTC (permalink / raw)
  To: kvmarm
  Cc: linux-kernel, maz, james.morse, suzuki.poulose,
	julien.thierry.kdev, will, catalin.marinas, kernel-team,
	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 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 4/4] KVM: arm64: Log source when panicking from nVHE hyp
  2021-02-23 15:57 [PATCH v2 0/4] Debug info for nVHE hyp panics Andrew Scull
                   ` (2 preceding siblings ...)
  2021-02-23 15:57 ` [PATCH v2 3/4] KVM: arm64: Use BUG and BUG_ON in nVHE hyp Andrew Scull
@ 2021-02-23 15:57 ` Andrew Scull
  3 siblings, 0 replies; 7+ messages in thread
From: Andrew Scull @ 2021-02-23 15:57 UTC (permalink / raw)
  To: kvmarm
  Cc: linux-kernel, maz, james.morse, suzuki.poulose,
	julien.thierry.kdev, will, catalin.marinas, kernel-team,
	Andrew Scull

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.

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

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            | 31 +++++++++++++++++++++++++
 arch/arm64/kvm/hyp/include/hyp/switch.h |  2 --
 arch/arm64/kvm/hyp/nvhe/host.S          | 17 ++++++--------
 arch/arm64/kvm/hyp/nvhe/psci-relay.c    |  2 --
 arch/arm64/kvm/hyp/vhe/switch.c         |  4 +---
 7 files changed, 42 insertions(+), 19 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..b25b88d8c150 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -291,3 +291,34 @@ 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 mode = spsr & PSR_MODE_MASK;
+
+	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_in_kimg);
+		}
+	} else {
+		kvm_err("nVHE hyp panic at: %016llx!\n", elr_in_kimg);
+	}
+
+	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 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..04d661614b0f 100644
--- a/arch/arm64/kvm/hyp/nvhe/host.S
+++ b/arch/arm64/kvm/hyp/nvhe/host.S
@@ -77,21 +77,18 @@ 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 */
+	mrs	x0, esr_el2
+	get_vcpu_ptr x4, x5
+	mrs	x5, far_el2
+	mrs	x6, hpfar_el2
+	mov	x7, x0			// Unused argument
 
 	/* 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 {
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.30.0.617.g56c4b15f3c-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/4] bug: Remove redundant condition check in report_bug
  2021-02-23 15:57 ` [PATCH v2 1/4] bug: Remove redundant condition check in report_bug Andrew Scull
@ 2021-02-23 16:25   ` Steven Rostedt
  0 siblings, 0 replies; 7+ messages in thread
From: Steven Rostedt @ 2021-02-23 16:25 UTC (permalink / raw)
  To: Andrew Scull
  Cc: kvmarm, linux-kernel, maz, james.morse, suzuki.poulose,
	julien.thierry.kdev, will, catalin.marinas, kernel-team,
	Peter Zijlstra, Kees Cook

On Tue, 23 Feb 2021 15:57:56 +0000
Andrew Scull <ascull@google.com> 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()")

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> 
> Signed-off-by: Andrew Scull <ascull@google.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "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 7103440c0ee1..4ab398a2de93 100644
> --- a/lib/bug.c
> +++ b/lib/bug.c
> @@ -158,30 +158,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;
>  	}
>  
>  	/*


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/4] bug: Factor out a getter for a bug's file line
  2021-02-23 15:57 ` [PATCH v2 2/4] bug: Factor out a getter for a bug's file line Andrew Scull
@ 2021-02-23 16:35   ` Steven Rostedt
  0 siblings, 0 replies; 7+ messages in thread
From: Steven Rostedt @ 2021-02-23 16:35 UTC (permalink / raw)
  To: Andrew Scull
  Cc: kvmarm, linux-kernel, maz, james.morse, suzuki.poulose,
	julien.thierry.kdev, will, catalin.marinas, kernel-team,
	Peter Zijlstra

On Tue, 23 Feb 2021 15:57:57 +0000
Andrew Scull <ascull@google.com> 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.
> 
> Signed-off-by: Andrew Scull <ascull@google.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "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 4ab398a2de93..f936615176b8 100644
> --- a/lib/bug.c
> +++ b/lib/bug.c
> @@ -130,6 +130,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;

Not that it should be part of this patch, as this patch is moving the code
and shouldn't modify it, but as a micro optimization, we could remove the
initialization from the beginning and place it here:

#else 
	*file = NULL;
	*line = 0;

But again, this patch is fine.

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve


> +#endif
> +}
> +
>  struct bug_entry *find_bug(unsigned long bugaddr)
>  {
>  	struct bug_entry *bug;
> @@ -156,17 +172,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;


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-02-23 16:37 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-23 15:57 [PATCH v2 0/4] Debug info for nVHE hyp panics Andrew Scull
2021-02-23 15:57 ` [PATCH v2 1/4] bug: Remove redundant condition check in report_bug Andrew Scull
2021-02-23 16:25   ` Steven Rostedt
2021-02-23 15:57 ` [PATCH v2 2/4] bug: Factor out a getter for a bug's file line Andrew Scull
2021-02-23 16:35   ` Steven Rostedt
2021-02-23 15:57 ` [PATCH v2 3/4] KVM: arm64: Use BUG and BUG_ON in nVHE hyp Andrew Scull
2021-02-23 15:57 ` [PATCH v2 4/4] KVM: arm64: Log source when panicking from " Andrew Scull

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).