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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
