Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 08 Jun 2022 08:33:46 +0100
Message-ID: <87leu7zk11.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Kalesh Singh <kaleshsingh@google.com>
Cc:     mark.rutland@arm.com, broonie@kernel.org, will@kernel.org,
        qperret@google.com, tabba@google.com, surenb@google.com,
        tjmercier@google.com, 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>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>,
        Andrew Jones <drjones@redhat.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        Zenghui Yu <yuzenghui@huawei.com>,
        Keir Fraser <keirf@google.com>,
        Ard Biesheuvel <ardb@kernel.org>,
        Oliver Upton <oupton@google.com>,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/5] KVM: arm64: Compile stacktrace.nvhe.o
In-Reply-To: <20220607165105.639716-3-kaleshsingh@google.com>
References: <20220607165105.639716-1-kaleshsingh@google.com>
        <20220607165105.639716-3-kaleshsingh@google.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Tue, 07 Jun 2022 17:50:44 +0100,
Kalesh Singh <kaleshsingh@google.com> wrote:
> 
> Recompile stack unwinding code for use with the nVHE hypervisor. This is
> a preparatory patch that will allow reusing most of the kernel unwinding
> logic in the nVHE hypervisor.
> 
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> ---
> 
> Changes in v3:
>   - Add Mark's Reviewed-by tag
> 
> Changes in v2:
>   - Split out refactoring of common unwinding logic into a separate patch,
>     per Mark Brown
> 
>  arch/arm64/include/asm/stacktrace.h | 18 +++++++++-----
>  arch/arm64/kernel/stacktrace.c      | 37 ++++++++++++++++-------------
>  arch/arm64/kvm/hyp/nvhe/Makefile    |  3 ++-
>  3 files changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index aec9315bf156..f5af9a94c5a6 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -16,12 +16,14 @@
>  #include <asm/sdei.h>
>  
>  enum stack_type {
> -	STACK_TYPE_UNKNOWN,
> +#ifndef __KVM_NVHE_HYPERVISOR__
>  	STACK_TYPE_TASK,
>  	STACK_TYPE_IRQ,
>  	STACK_TYPE_OVERFLOW,
>  	STACK_TYPE_SDEI_NORMAL,
>  	STACK_TYPE_SDEI_CRITICAL,
> +#endif /* !__KVM_NVHE_HYPERVISOR__ */
> +	STACK_TYPE_UNKNOWN,

What is the reason for this reordering? I have the sinking feeling
that this could play badly with the logic that assumes that it is
legal to switch from a lesser stack type to a higher one, and could
allow switching to a duff stack.

I would at least like to see a justification of why this isn't less
safe than the current code.

[...]

> index f9fe4dc21b1f..c0ff0d6fc403 100644
> --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> @@ -14,7 +14,8 @@ lib-objs := $(addprefix ../../../lib/, $(lib-objs))
>  
>  obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o \
>  	 hyp-main.o hyp-smp.o psci-relay.o early_alloc.o page_alloc.o \
> -	 cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o
> +	 cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o \
> +	 ../../../kernel/stacktrace.o

This, I positively hate. It is only a marginally better than the
cross-arch references we used to have with arch/arm/kvm. I'd be much
more happy with an include file containing the shared code. It would
also allow the removal of some of the #ifdeferry. Note that this is
the approach that we ended up adopting for the VHE/nVHE split.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.