Re: [PATCH v3 4/5] KVM: arm64: Allocate shared stacktrace pages

[Marc Zyngier <maz@kernel.org>]

On Tue, 07 Jun 2022 17:50:46 +0100,
Kalesh Singh <kaleshsingh@google.com> wrote:
> 
> The nVHE hypervisor can use this shared area to dump its stacktrace
> addresses on hyp_panic(). Symbolization and printing the stacktrace can
> then be handled by the host in EL1 (done in a later patch in this series).
> 
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> ---
>  arch/arm64/include/asm/kvm_asm.h |  1 +
>  arch/arm64/kvm/arm.c             | 34 ++++++++++++++++++++++++++++++++
>  arch/arm64/kvm/hyp/nvhe/setup.c  | 11 +++++++++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 2e277f2ed671..ad31ac68264f 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -174,6 +174,7 @@ struct kvm_nvhe_init_params {
>  	unsigned long hcr_el2;
>  	unsigned long vttbr;
>  	unsigned long vtcr;
> +	unsigned long stacktrace_hyp_va;
>  };
>  
>  /* Translate a kernel address @ptr into its equivalent linear mapping */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 400bb0fe2745..c0a936a7623d 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -50,6 +50,7 @@ DEFINE_STATIC_KEY_FALSE(kvm_protected_mode_initialized);
>  DECLARE_KVM_HYP_PER_CPU(unsigned long, kvm_hyp_vector);
>  
>  static DEFINE_PER_CPU(unsigned long, kvm_arm_hyp_stack_page);
> +DEFINE_PER_CPU(unsigned long, kvm_arm_hyp_stacktrace_page);

Why isn't this static, since the address is passed via the
kvm_nvhe_init_params block?

>  unsigned long kvm_arm_hyp_percpu_base[NR_CPUS];
>  DECLARE_KVM_NVHE_PER_CPU(struct kvm_nvhe_init_params, kvm_init_params);
>  
> @@ -1554,6 +1555,7 @@ static void cpu_prepare_hyp_mode(int cpu)
>  	tcr |= (idmap_t0sz & GENMASK(TCR_TxSZ_WIDTH - 1, 0)) << TCR_T0SZ_OFFSET;
>  	params->tcr_el2 = tcr;
>  
> +	params->stacktrace_hyp_va = kern_hyp_va(per_cpu(kvm_arm_hyp_stacktrace_page, cpu));
>  	params->pgd_pa = kvm_mmu_get_httbr();
>  	if (is_protected_kvm_enabled())
>  		params->hcr_el2 = HCR_HOST_NVHE_PROTECTED_FLAGS;
> @@ -1845,6 +1847,7 @@ static void teardown_hyp_mode(void)
>  	free_hyp_pgds();
>  	for_each_possible_cpu(cpu) {
>  		free_page(per_cpu(kvm_arm_hyp_stack_page, cpu));
> +		free_page(per_cpu(kvm_arm_hyp_stacktrace_page, cpu));
>  		free_pages(kvm_arm_hyp_percpu_base[cpu], nvhe_percpu_order());
>  	}
>  }
> @@ -1936,6 +1939,23 @@ static int init_hyp_mode(void)
>  		per_cpu(kvm_arm_hyp_stack_page, cpu) = stack_page;
>  	}
>  
> +	/*
> +	 * Allocate stacktrace pages for Hypervisor-mode.
> +	 * This is used by the hypervisor to share its stacktrace
> +	 * with the host on a hyp_panic().
> +	 */
> +	for_each_possible_cpu(cpu) {
> +		unsigned long stacktrace_page;
> +
> +		stacktrace_page = __get_free_page(GFP_KERNEL);
> +		if (!stacktrace_page) {
> +			err = -ENOMEM;
> +			goto out_err;
> +		}
> +
> +		per_cpu(kvm_arm_hyp_stacktrace_page, cpu) = stacktrace_page;

I have the same feeling as with the overflow stack. This is
potentially a huge amount of memory: on my test box, with 64k pages,
this is a whole 10MB that I give away for something that is only a
debug facility.

Can this somehow be limited? I don't see it being less than a page as
a problem, as the memory is always shared back with EL1 in the case of
pKVM (and normal KVM doesn't have that problem anyway).

Alternatively, this should be restricted to pKVM. With normal nVHE,
the host should be able to parse the EL2 stack directly with some
offsetting. Actually, this is probably the best option.

Thanks,

	M.

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