Linux KVM/arm64 development list
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: Andrew Scull <ascull@google.com>
Cc: kernel-team@android.com, catalin.marinas@arm.com,
	will@kernel.org, kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH 2/2] KVM: arm64: Log source when panicking from nVHE hyp
Date: Tue, 23 Feb 2021 11:29:54 +0000	[thread overview]
Message-ID: <87czwr10p9.wl-maz@kernel.org> (raw)
In-Reply-To: <20210223094927.766572-3-ascull@google.com>

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

  reply	other threads:[~2021-02-23 11:30 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2021-02-23 12:34     ` Andrew Scull
2021-02-23 15:18       ` Marc Zyngier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87czwr10p9.wl-maz@kernel.org \
    --to=maz@kernel.org \
    --cc=ascull@google.com \
    --cc=catalin.marinas@arm.com \
    --cc=kernel-team@android.com \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox