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;
MIME-Version: 1.0
References: <20220314200148.2695206-1-kaleshsingh@google.com>
 <20220314200148.2695206-8-kaleshsingh@google.com> <CA+EHjTwQRUCjvcMHNe1f0kPBdU=vS+f2+e_eefhnJDR4s73cQA@mail.gmail.com>
 <CAC_TJveNRaDFcQGo9-eqKa3=1DnuVDs4U+ye795VcJ1ozVkMyg@mail.gmail.com> <YlbXHg64HK2d3dgm@FVFF77S0Q05N>
In-Reply-To: <YlbXHg64HK2d3dgm@FVFF77S0Q05N>
From:   Kalesh Singh <kaleshsingh@google.com>
Date:   Tue, 19 Apr 2022 10:37:56 -0700
Message-ID: <CAC_TJveJYFkHPQLYdL8SCEAwMPgwpF_-ctMqKJ9w=eDa_M0u5w@mail.gmail.com>
Subject: Re: [PATCH v6 7/8] KVM: arm64: Unwind and dump nVHE HYP stacktrace
To:     Mark Rutland <mark.rutland@arm.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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Apr 13, 2022 at 6:59 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> 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.

Agreed. Although some reimplementation may be unavoidable, as we can't
safely link against kernel code from the protected KVM hypervisor.
Perhaps we can move some of the common logic to a shared header that
can be included in both places (host, hyp), WDYT?

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

Sounds good to me.

Thanks,
Kalesh
>
> 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.