Re: [PATCH v6 7/8] KVM: arm64: Unwind and dump nVHE HYP stacktrace

From: Mark Rutland <mark.rutland@arm.com>
To: Kalesh Singh <kaleshsingh@google.com>
Cc: Fuad Tabba <tabba@google.com>, Will Deacon <will@kernel.org>,
	Marc Zyngier <maz@kernel.org>,
	Quentin Perret <qperret@google.com>,
	Suren Baghdasaryan <surenb@google.com>,
	"Cc: Android Kernel" <kernel-team@android.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Mark Brown <broonie@kernel.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Peter Collingbourne <pcc@google.com>,
	"Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>,
	Stephen Boyd <swboyd@chromium.org>,
	Andrew Walbran <qwandor@google.com>,
	Andrew Scull <ascull@google.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	"moderated list:ARM64 PORT (AARCH64 ARCHITECTURE)" 
	<linux-arm-kernel@lists.infradead.org>,
	kvmarm <kvmarm@lists.cs.columbia.edu>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 7/8] KVM: arm64: Unwind and dump nVHE HYP stacktrace
Date: Wed, 13 Apr 2022 14:58:54 +0100	[thread overview]
Message-ID: <YlbXHg64HK2d3dgm@FVFF77S0Q05N> (raw)
In-Reply-To: <CAC_TJveNRaDFcQGo9-eqKa3=1DnuVDs4U+ye795VcJ1ozVkMyg@mail.gmail.com>

Hi Kalesh,

Sorry for the radiosilence.

I see that in v7 you've dropped the stacktrace bits for now; I'm just
commenting here fot future reference.

On Thu, Mar 31, 2022 at 12:22:05PM -0700, Kalesh Singh wrote:
> Hi everyone,
> 
> There has been expressed interest in having hypervisor stack unwinding
> in production Android builds.
> 
> The current design targets NVHE_EL2_DEBUG enabled builds and is not
> suitable for production environments, since this config disables host
> stage-2 protection on hyp_panic() which breaks security guarantees.
> The benefit of this approach is that the stack unwinding can happen at
> EL1 and allows us to reuse most of the unwinding logic from the host
> kernel unwinder.
> 
> Proposal for how this can be done without disabling host stage-2 protection:
>   - The host allocates a "panic_info" page and shares it with the hypervisor.
>   - On hyp_panic(), the hypervisor can unwind and dump its stack
> addresses to the shared page.
>   - The host can read out this information and symbolize these addresses.
> 
> This would allow for getting hyp stack traces in production while
> preserving the security model. The downside being that the core
> unwinding logic would be duplicated at EL2.
> 
> Are there any objections to making this change?

I'm fine with the concept of splitting the unwind and logging steps; this is
akin to doing:

	stack_trace_save_tsk(...);
	...
	stack_trace_print(...);

... and I'm fine with having a stack_trace_save_hyp(...) variant.

However, I would like to ensure that we're reusing logic rather than
duplicating it wholesale. There are some changes I would like to make to the
stacktrace code in the near future that might make that a bit easier, e.g.
reworking the stack transition checks to be table-driven, and factoring out the
way we handle return trampolines.

I'll Cc you on changes to the stacktrace code. There are some preparatory
cleanups I'd like to get out of the way first which I'll send shortly.

Thanks,
Mark.