Re: [PATCH v3 2/5] KVM: arm64: Compile stacktrace.nvhe.o

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
Date: Wed, 08 Jun 2022 08:33:46 +0100	[thread overview]
Message-ID: <87leu7zk11.wl-maz@kernel.org> (raw)
In-Reply-To: <20220607165105.639716-3-kaleshsingh@google.com>

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel