On Thu, Jul 21, 2022 at 1:35 PM Oliver Upton <oliver.upton@xxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >
> > 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
