Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 21 Jul 2022 21:35:26 +0100
From:   Oliver Upton <oliver.upton@linux.dev>
To:     Kalesh Singh <kaleshsingh@google.com>
Cc:     maz@kernel.org, mark.rutland@arm.com, broonie@kernel.org,
        madvenka@linux.microsoft.com, tabba@google.com, will@kernel.org,
        qperret@google.com, james.morse@arm.com, alexandru.elisei@arm.com,
        suzuki.poulose@arm.com, catalin.marinas@arm.com,
        andreyknvl@gmail.com, vincenzo.frascino@arm.com,
        mhiramat@kernel.org, ast@kernel.org, drjones@redhat.com,
        wangkefeng.wang@huawei.com, elver@google.com, keirf@google.com,
        yuzenghui@huawei.com, ardb@kernel.org,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu,
        linux-kernel@vger.kernel.org, android-mm@google.com,
        kernel-team@android.com
Subject: Re: [PATCH v5 17/17] KVM: arm64: Introduce hyp_dump_backtrace()
Message-ID: <Ytm4ji93wyjRI7Jw@google.com>
References: <20220721055728.718573-1-kaleshsingh@google.com>
 <20220721055728.718573-18-kaleshsingh@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220721055728.718573-18-kaleshsingh@google.com>
Precedence: bulk

Hi Kalesh,

Nifty series! Had the chance to take it for a spin :) Few comments
below:

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@google.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.

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:

<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.

<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.

</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)

--
Thanks,
Oliver