Date:   Thu, 21 Jul 2022 17:01:07 -0700
From:   Kalesh Singh <kaleshsingh@...gle.com>
To:     Oliver Upton <oliver.upton@...ux.dev>
Cc:     Marc Zyngier <maz@...nel.org>, Mark Rutland <mark.rutland@....com>,
        Mark Brown <broonie@...nel.org>,
        "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>,
        Fuad Tabba <tabba@...gle.com>, Will Deacon <will@...nel.org>,
        Quentin Perret <qperret@...gle.com>,
        James Morse <james.morse@....com>,
        Alexandru Elisei <alexandru.elisei@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        andreyknvl@...il.com, vincenzo.frascino@....com,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrew Jones <drjones@...hat.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        Marco Elver <elver@...gle.com>, Keir Fraser <keirf@...gle.com>,
        Zenghui Yu <yuzenghui@...wei.com>,
        Ard Biesheuvel <ardb@...nel.org>,
        "moderated list:ARM64 PORT (AARCH64 ARCHITECTURE)" 
        <linux-arm-kernel@...ts.infradead.org>,
        kvmarm <kvmarm@...ts.cs.columbia.edu>,
        LKML <linux-kernel@...r.kernel.org>, android-mm@...gle.com,
        "Cc: Android Kernel" <kernel-team@...roid.com>
Subject: Re: [PATCH v5 17/17] KVM: arm64: Introduce hyp_dump_backtrace()

On Thu, Jul 21, 2022 at 1:35 PM Oliver Upton <oliver.upton@...ux.dev> wrote:
>
> Hi Kalesh,
>
> Nifty series! Had the chance to take it for a spin :) Few comments
> below:

Hi Oliver. Thanks for giving it a try :)

>
> On Wed, Jul 20, 2022 at 10:57:28PM -0700, Kalesh Singh wrote:
> > In non-protected nVHE mode, unwinds and dumps the hypervisor backtrace
> > from EL1. This is possible beacuase the host can directly access the
> > hypervisor stack pages in non-proteced mode.
> >
> > Signed-off-by: Kalesh Singh <kaleshsingh@...gle.com>
> > ---
> >
> > Changes in v5:
> >   - Move code out from nvhe.h header to handle_exit.c, per Marc
> >   - Fix stacktrace symoblization when CONFIG_RAMDOMIZE_BASE is enabled,
> >     per Fuad
> >   - Use regular comments instead of doc comments, per Fuad
> >
> >  arch/arm64/kvm/handle_exit.c | 65 +++++++++++++++++++++++++++++++-----
> >  1 file changed, 56 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> > index ad568da5c7d7..432b6b26f4ad 100644
> > --- a/arch/arm64/kvm/handle_exit.c
> > +++ b/arch/arm64/kvm/handle_exit.c
>
> [...]
>
> > @@ -318,6 +319,56 @@ void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
> >               kvm_handle_guest_serror(vcpu, kvm_vcpu_get_esr(vcpu));
> >  }
> >
> > +/*
> > + * kvm_nvhe_print_backtrace_entry - Symbolizes and prints the HYP stack address
> > + */
> > +static void kvm_nvhe_print_backtrace_entry(unsigned long addr,
> > +                                               unsigned long hyp_offset)
> > +{
> > +     unsigned long va_mask = GENMASK_ULL(vabits_actual - 1, 0);
> > +
> > +     /* Mask tags and convert to kern addr */
> > +     addr = (addr & va_mask) + hyp_offset;
> > +     kvm_err(" [<%016lx>] %pB\n", addr, (void *)(addr + kaslr_offset()));
> > +}
>
> It is a bit odd to see this get churned from the last patch. Is it
> possible to introduce the helper earlier on? In fact, the non-protected
> patch should come first to layer the series better.

Agreed. pKVM is the one with the extra layer. I'll reorder this in the
next version.

>
> Also, it seems to me that there isn't much need for the indirection if
> the pKVM unwinder is made to work around the below function signature:

Ok, I'll fold it into the below function.

>
> <snip>
>
> > +/*
> > + * hyp_dump_backtrace_entry - Dump an entry of the non-protected nVHE HYP stacktrace
> > + *
> > + * @arg    : the hypervisor offset, used for address translation
> > + * @where  : the program counter corresponding to the stack frame
> > + */
> > +static bool hyp_dump_backtrace_entry(void *arg, unsigned long where)
> > +{
> > +     kvm_nvhe_print_backtrace_entry(where, (unsigned long)arg);
> > +
> > +     return true;
> > +}
>
> </snip>
>
> > +/*
> > + * hyp_dump_backtrace - Dump the non-proteced nVHE HYP backtrace.
> > + *
> > + * @hyp_offset: hypervisor offset, used for address translation.
> > + *
> > + * The host can directly access HYP stack pages in non-protected
> > + * mode, so the unwinding is done directly from EL1. This removes
> > + * the need for shared buffers between host and hypervisor for
> > + * the stacktrace.
> > + */
> > +static void hyp_dump_backtrace(unsigned long hyp_offset)
> > +{
> > +     struct kvm_nvhe_stacktrace_info *stacktrace_info;
> > +     struct unwind_state state;
> > +
> > +     stacktrace_info = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
> > +
> > +     kvm_nvhe_unwind_init(&state, stacktrace_info->fp, stacktrace_info->pc);
> > +
> > +     kvm_err("Non-protected nVHE HYP call trace:\n");
>
> I don't see the value in discerning non-protected vs. protected in the
> preamble, as panic() will dump the kernel commandline which presumably
> would have a `kvm-arm.mode=protected` in the case of pKVM.

Ok, I'll remove the distinction.

>
> <nit>
>
> Not entirely your problem, but we should really use a consistent name
> for that thing we have living at EL2. Hyp or nVHE (but not both) would
> be appropriate.
>

Right, I think just "nVHE" is probably sufficient. (Open to other suggestions)

> </nit>
>
> > +     unwind(&state, hyp_dump_backtrace_entry, (void *)hyp_offset);
> > +     kvm_err("---- End of Non-protected nVHE HYP call trace ----\n");
>
> Maybe:
>
> kvm_err("---[ end ${NAME_FOR_EL2} trace ]---");
>
> (more closely matches the pattern of the arm64 stacktrace)

Agreed.

Thanks,
Kalesh

>
> --
> Thanks,
> Oliver

Powered by blists - more mailing lists