From: Andrew Scull <ascull@google.com>
To: Marc Zyngier <maz@kernel.org>
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 12:34:22 +0000 [thread overview]
Message-ID: <YDT2TvFFxre5i3KN@google.com> (raw)
In-Reply-To: <87czwr10p9.wl-maz@kernel.org>
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
next prev parent reply other threads:[~2021-02-23 12:34 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
2021-02-23 12:34 ` Andrew Scull [this message]
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=YDT2TvFFxre5i3KN@google.com \
--to=ascull@google.com \
--cc=catalin.marinas@arm.com \
--cc=kernel-team@android.com \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=maz@kernel.org \
--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 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.