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

From: Marc Zyngier <maz@kernel.org>
To: Kalesh Singh <kaleshsingh@google.com>
Cc: Kefeng Wang <wangkefeng.wang@huawei.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Alexei Starovoitov <ast@kernel.org>,
	will@kernel.org, surenb@google.com, kvmarm@lists.cs.columbia.edu,
	"Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>,
	linux-arm-kernel@lists.infradead.org, kernel-team@android.com,
	broonie@kernel.org, tjmercier@google.com,
	linux-kernel@vger.kernel.org,
	Masami Hiramatsu <mhiramat@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.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm