Re: [PATCH v3 16/18] KVM: arm64: nVHE: Migrate hyp interface to SMCCC

[Marc Zyngier <maz@kernel.org>]

On Thu, 03 Sep 2020 14:53:05 +0100,
Andrew Scull <ascull@google.com> wrote:
> 
> Rather than passing arbitrary function pointers to run at hyp, define
> and equivalent set of SMCCC functions.
> 
> Since the SMCCC functions are strongly tied to the original function
> prototypes, it is not expected for the host to ever call an invalid ID
> but a warning is raised if this does ever occur.
> 
> As __kvm_vcpu_run is used for every switch between the host and a guest,
> it is explicitly singled out to be identified before the other function
> IDs to improve the performance of the hot path.
> 
> Signed-off-by: Andrew Scull <ascull@google.com>
> Signed-off-by: David Brazdil <dbrazdil@google.com>

Who is the author? If it is a co-development, use the ad-hoc tag.

> ---
>  arch/arm64/include/asm/kvm_asm.h   |  24 ++++++
>  arch/arm64/include/asm/kvm_host.h  |  25 ++++---
>  arch/arm64/kvm/arm.c               |   2 +-
>  arch/arm64/kvm/hyp.S               |  24 ++----
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 113 +++++++++++++++++++++++++----
>  5 files changed, 145 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 4bbde3d3989c..4a73f1349151 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -38,6 +38,30 @@
>  
>  #define __SMCCC_WORKAROUND_1_SMC_SZ 36
>  
> +#define KVM_HOST_SMCCC_ID(id)						\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
> +			   ARM_SMCCC_SMC_64,				\
> +			   ARM_SMCCC_OWNER_STANDARD_HYP,		\
> +			   (id))
> +
> +#define KVM_HOST_SMCCC_FUNC(name) KVM_HOST_SMCCC_ID(__KVM_HOST_SMCCC_FUNC_##name)
> +
> +#define __KVM_HOST_SMCCC_FUNC___kvm_hyp_init			0
> +#define __KVM_HOST_SMCCC_FUNC___kvm_flush_vm_context		1
> +#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid_ipa		2
> +#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid		3
> +#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_local_vmid	4
> +#define __KVM_HOST_SMCCC_FUNC___kvm_timer_set_cntvoff		5
> +#define __KVM_HOST_SMCCC_FUNC___kvm_vcpu_run			6
> +#define __KVM_HOST_SMCCC_FUNC___kvm_enable_ssbs			7
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_get_ich_vtr_el2		8
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_read_vmcr		9
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_write_vmcr		10
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_init_lrs		11
> +#define __KVM_HOST_SMCCC_FUNC___kvm_get_mdcr_el2		12
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_save_aprs		13
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_restore_aprs		14
> +
>  #ifndef __ASSEMBLY__
>  
>  #include <linux/mm.h>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 16adbefde1cc..82c941cf8890 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -11,6 +11,7 @@
>  #ifndef __ARM64_KVM_HOST_H__
>  #define __ARM64_KVM_HOST_H__
>  
> +#include <linux/arm-smccc.h>
>  #include <linux/bitmap.h>
>  #include <linux/types.h>
>  #include <linux/jump_label.h>
> @@ -479,18 +480,20 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);
>  void kvm_arm_halt_guest(struct kvm *kvm);
>  void kvm_arm_resume_guest(struct kvm *kvm);
>  
> -u64 __kvm_call_hyp(void *hypfn, ...);
> +u64 __kvm_call_hyp_init(phys_addr_t pgd_ptr,
> +			unsigned long hyp_stack_ptr,
> +			unsigned long vector_ptr,
> +			unsigned long tpidr_el2);
>  
> -#define kvm_call_hyp_nvhe(f, ...)					\
> -	do {								\
> -		DECLARE_KVM_NVHE_SYM(f);				\
> -		__kvm_call_hyp(kvm_ksym_ref_nvhe(f), ##__VA_ARGS__);	\
> -	} while(0)
> -
> -#define kvm_call_hyp_nvhe_ret(f, ...)					\
> +#define kvm_call_hyp_nvhe(f, ...)						\
>  	({								\
> -		DECLARE_KVM_NVHE_SYM(f);				\
> -		__kvm_call_hyp(kvm_ksym_ref_nvhe(f), ##__VA_ARGS__);	\
> +		struct arm_smccc_res res;				\
> +									\
> +		arm_smccc_1_1_hvc(KVM_HOST_SMCCC_FUNC(f),		\
> +				  ##__VA_ARGS__, &res);			\
> +		WARN_ON(res.a0 != SMCCC_RET_SUCCESS);			\
> +									\
> +		res.a1;							\
>  	})
>  
>  /*
> @@ -516,7 +519,7 @@ u64 __kvm_call_hyp(void *hypfn, ...);
>  			ret = f(__VA_ARGS__);				\
>  			isb();						\
>  		} else {						\
> -			ret = kvm_call_hyp_nvhe_ret(f, ##__VA_ARGS__);	\
> +			ret = kvm_call_hyp_nvhe(f, ##__VA_ARGS__);	\

nit: Just inline the whole macro here.

>  		}							\
>  									\
>  		ret;							\
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 6b7180072c8d..49aa08bd26de 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1288,7 +1288,7 @@ static void cpu_init_hyp_mode(void)
>  	 * cpus_have_const_cap() wrapper.
>  	 */
>  	BUG_ON(!system_capabilities_finalized());
> -	__kvm_call_hyp((void *)pgd_ptr, hyp_stack_ptr, vector_ptr, tpidr_el2);
> +	__kvm_call_hyp_init(pgd_ptr, hyp_stack_ptr, vector_ptr, tpidr_el2);
>  
>  	/*
>  	 * Disabling SSBD on a non-VHE system requires us to enable SSBS
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 3c79a1124af2..12aa426f7559 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -11,24 +11,12 @@
>  #include <asm/cpufeature.h>
>  
>  /*
> - * u64 __kvm_call_hyp(void *hypfn, ...);
> - *
> - * This is not really a variadic function in the classic C-way and care must
> - * be taken when calling this to ensure parameters are passed in registers
> - * only, since the stack will change between the caller and the callee.
> - *
> - * Call the function with the first argument containing a pointer to the
> - * function you wish to call in Hyp mode, and subsequent arguments will be
> - * passed as x0, x1, and x2 (a maximum of 3 arguments in addition to the
> - * function pointer can be passed).  The function being called must be mapped
> - * in Hyp mode (see init_hyp_mode in arch/arm/kvm/arm.c).  Return values are
> - * passed in x0.
> - *
> - * A function pointer with a value less than 0xfff has a special meaning,
> - * and is used to implement hyp stubs in the same way as in
> - * arch/arm64/kernel/hyp_stub.S.
> + * u64 __kvm_call_hyp_init(phys_addr_t pgd_ptr,
> + * 			   unsigned long hyp_stack_ptr,
> + * 			   unsigned long vector_ptr,
> + * 			   unsigned long tpidr_el2);
>   */
> -SYM_FUNC_START(__kvm_call_hyp)
> +SYM_FUNC_START(__kvm_call_hyp_init)
>  	hvc	#0
>  	ret
> -SYM_FUNC_END(__kvm_call_hyp)
> +SYM_FUNC_END(__kvm_call_hyp_init)
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index c8938e09f585..13093df70c87 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -12,24 +12,111 @@
>  #include <asm/kvm_hyp.h>
>  #include <asm/kvm_mmu.h>
>  
> -typedef unsigned long (*hypcall_fn_t)
> -	(unsigned long, unsigned long, unsigned long);
> +#include <kvm/arm_hypercalls.h>
> +
> +static void handle_host_hcall(unsigned long func_id,
> +			      struct kvm_cpu_context *host_ctxt)
> +{
> +	unsigned long ret = 0;
> +
> +	/*
> +	 * __kvm_vcpu_run is a hot path of the context switch so identify it
> +	 * quickly before searching through the other functions IDs.
> +	 */
> +	if (func_id == KVM_HOST_SMCCC_FUNC(__kvm_vcpu_run)) {
> +		struct kvm_vcpu *vcpu =
> +			(struct kvm_vcpu *)host_ctxt->regs.regs[1];
> +
> +		ret = __kvm_vcpu_run(vcpu);
> +		goto out;
> +	}

This is terribly ugly. How does it behave if you keep it in the
switch(), and make it function 0, for example?

> +
> +	switch (func_id) {
> +	case KVM_HOST_SMCCC_FUNC(__kvm_flush_vm_context):
> +		__kvm_flush_vm_context();
> +		break;
> +	case KVM_HOST_SMCCC_FUNC(__kvm_tlb_flush_vmid_ipa): {
> +			struct kvm_s2_mmu *mmu =
> +				(struct kvm_s2_mmu *)host_ctxt->regs.regs[1];
> +			phys_addr_t ipa = host_ctxt->regs.regs[2];
> +			int level = host_ctxt->regs.regs[3];
> +
> +			__kvm_tlb_flush_vmid_ipa(mmu, ipa, level);
> +			break;
> +		}

nit: The formatting hurts. If you have to use braces, don't introduce
extra indentation. And given how many times you extract a s2_mmu from
the first second argument, consider using a helper.

> +	case KVM_HOST_SMCCC_FUNC(__kvm_tlb_flush_vmid): {
> +			struct kvm_s2_mmu *mmu =
> +				(struct kvm_s2_mmu *)host_ctxt->regs.regs[1];
> +
> +			__kvm_tlb_flush_vmid(mmu);
> +			break;
> +		}
> +	case KVM_HOST_SMCCC_FUNC(__kvm_tlb_flush_local_vmid): {
> +			struct kvm_s2_mmu *mmu =
> +				(struct kvm_s2_mmu *)host_ctxt->regs.regs[1];
> +
> +			__kvm_tlb_flush_local_vmid(mmu);
> +			break;
> +		}
> +	case KVM_HOST_SMCCC_FUNC(__kvm_timer_set_cntvoff): {
> +			u64 cntvoff = host_ctxt->regs.regs[1];
> +
> +			__kvm_timer_set_cntvoff(cntvoff);
> +			break;
> +		}
> +	case KVM_HOST_SMCCC_FUNC(__kvm_enable_ssbs):
> +		__kvm_enable_ssbs();
> +		break;
> +	case KVM_HOST_SMCCC_FUNC(__vgic_v3_get_ich_vtr_el2):
> +		ret = __vgic_v3_get_ich_vtr_el2();
> +		break;
> +	case KVM_HOST_SMCCC_FUNC(__vgic_v3_read_vmcr):
> +		ret = __vgic_v3_read_vmcr();
> +		break;
> +	case KVM_HOST_SMCCC_FUNC(__vgic_v3_write_vmcr): {
> +			u32 vmcr = host_ctxt->regs.regs[1];
> +
> +			__vgic_v3_write_vmcr(vmcr);
> +			break;
> +		}
> +	case KVM_HOST_SMCCC_FUNC(__vgic_v3_init_lrs):
> +		__vgic_v3_init_lrs();
> +		break;
> +	case KVM_HOST_SMCCC_FUNC(__kvm_get_mdcr_el2):
> +		ret = __kvm_get_mdcr_el2();
> +		break;
> +	case KVM_HOST_SMCCC_FUNC(__vgic_v3_save_aprs): {
> +			struct vgic_v3_cpu_if *cpu_if =
> +				(struct vgic_v3_cpu_if *)host_ctxt->regs.regs[1];
> +
> +			__vgic_v3_save_aprs(cpu_if);
> +			break;
> +		}
> +	case KVM_HOST_SMCCC_FUNC(__vgic_v3_restore_aprs): {
> +			struct vgic_v3_cpu_if *cpu_if =
> +				(struct vgic_v3_cpu_if *)host_ctxt->regs.regs[1];
> +
> +			__vgic_v3_restore_aprs(cpu_if);
> +			break;
> +		}
> +	default:
> +		/* Invalid host HVC. */
> +		host_ctxt->regs.regs[0] = SMCCC_RET_NOT_SUPPORTED;
> +		return;
> +	}
> +
> +out:
> +	host_ctxt->regs.regs[0] = SMCCC_RET_SUCCESS;
> +	host_ctxt->regs.regs[1] = ret;
> +}
>  
>  void handle_trap(struct kvm_cpu_context *host_ctxt) {
>  	u64 esr = read_sysreg_el2(SYS_ESR);
> -	hypcall_fn_t func;
> -	unsigned long ret;
> +	unsigned long func_id;
>  
>  	if (ESR_ELx_EC(esr) != ESR_ELx_EC_HVC64)
>  		hyp_panic();
>  
> -	/*
> -	 * __kvm_call_hyp takes a pointer in the host address space and
> -	 * up to three arguments.
> -	 */
> -	func = (hypcall_fn_t)kern_hyp_va(host_ctxt->regs.regs[0]);
> -	ret = func(host_ctxt->regs.regs[1],
> -		   host_ctxt->regs.regs[2],
> -		   host_ctxt->regs.regs[3]);
> -	host_ctxt->regs.regs[0] = ret;
> +	func_id = host_ctxt->regs.regs[0];
> +	handle_host_hcall(func_id, host_ctxt);
>  }
> -- 
> 2.28.0.402.g5ffc5be6b7-goog
> 
> 

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