Re: [PATCH v3 03/12] KVM: arm64: Pass pointer to __pkvm_init_switch_pgd

From: Will Deacon <will@kernel.org>
To: "Pierre-Clément Tosi" <ptosi@google.com>
Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Vincent Donnefort <vdonnefort@google.com>
Subject: Re: [PATCH v3 03/12] KVM: arm64: Pass pointer to __pkvm_init_switch_pgd
Date: Mon, 13 May 2024 15:17:22 +0100	[thread overview]
Message-ID: <20240513141721.GC28749@willie-the-truck> (raw)
In-Reply-To: <20240510112645.3625702-4-ptosi@google.com>

On Fri, May 10, 2024 at 12:26:32PM +0100, Pierre-Clément Tosi wrote:
> Make the function take a VA pointer, instead of a phys_addr_t, to fully
> take advantage of the high-level C language and its type checker.
> 
> Perform all accesses to the kvm_nvhe_init_params before disabling the
> MMU, removing the need to access it using physical addresses, which was
> the reason for taking a phys_addr_t.
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/include/asm/kvm_hyp.h   |  3 ++-
>  arch/arm64/kvm/hyp/nvhe/hyp-init.S | 12 +++++++++---
>  arch/arm64/kvm/hyp/nvhe/setup.c    |  4 +---
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index 96daf7cf6802..c195e71d0746 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -123,7 +123,8 @@ void __noreturn __hyp_do_panic(struct kvm_cpu_context *host_ctxt, u64 spsr,
>  #endif
>  
>  #ifdef __KVM_NVHE_HYPERVISOR__
> -void __pkvm_init_switch_pgd(phys_addr_t params, void (*finalize_fn)(void));
> +void __pkvm_init_switch_pgd(struct kvm_nvhe_init_params *params,
> +			    void (*finalize_fn)(void));
>  int __pkvm_init(phys_addr_t phys, unsigned long size, unsigned long nr_cpus,
>  		unsigned long *per_cpu_base, u32 hyp_va_bits);
>  void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt);
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> index 2994878d68ea..5a15737b4233 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> @@ -265,7 +265,15 @@ alternative_else_nop_endif
>  
>  SYM_CODE_END(__kvm_handle_stub_hvc)
>  
> +/*
> + * void __pkvm_init_switch_pgd(struct kvm_nvhe_init_params *params,
> + *                             void (*finalize_fn)(void));
> + */
>  SYM_FUNC_START(__pkvm_init_switch_pgd)
> +	/* Load the inputs from the VA pointer before turning the MMU off */
> +	ldr	x5, [x0, #NVHE_INIT_PGD_PA]
> +	ldr	x0, [x0, #NVHE_INIT_STACK_HYP_VA]
> +
>  	/* Turn the MMU off */
>  	pre_disable_mmu_workaround
>  	mrs	x2, sctlr_el2
> @@ -276,15 +284,13 @@ SYM_FUNC_START(__pkvm_init_switch_pgd)
>  	tlbi	alle2
>  
>  	/* Install the new pgtables */
> -	ldr	x3, [x0, #NVHE_INIT_PGD_PA]
> -	phys_to_ttbr x4, x3
> +	phys_to_ttbr x4, x5
>  alternative_if ARM64_HAS_CNP
>  	orr	x4, x4, #TTBR_CNP_BIT
>  alternative_else_nop_endif
>  	msr	ttbr0_el2, x4
>  
>  	/* Set the new stack pointer */
> -	ldr	x0, [x0, #NVHE_INIT_STACK_HYP_VA]
>  	mov	sp, x0
>  
>  	/* And turn the MMU back on! */

Hmm, if we can hoist the memory accesses all the way like this, then
couldn't we just move them into the caller's C code? Maybe that's what
we planned to do in the first place, which would explain why the
prototype of __pkvm_init_switch_pgd() is out-of-sync.

In other words, drop the previous patch and pass in the pgd and SP as
arguments to the asm.

Will

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