From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8D7FFC43461 for ; Mon, 7 Sep 2020 13:52:20 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 167F12078E for ; Mon, 7 Sep 2020 13:52:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="k8hYzu/I"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="Iw+ERmQO" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 167F12078E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Subject:To:From: Message-ID:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=TJyPirUckKWOYvXxde753DO5DnWNG2g7lAKrPM/Qdgw=; b=k8hYzu/IJzicmpL2OlzMlTo1r mmC5T3Xa27+LIOijvClLy6DM4zwRivS5tLqdw1Y24mfHiKvoDLxYc7I7pwwfkIo4X73CH+wp+ibPB Ouyre8qG8LRSZy7YsWFgLNQOV0xm3eCOYY5Zik2UBSY0iV7hUZrAOs5mSNlpvlcVbKsEk275FrKPb BId1QYJ8nG27DMVfyGDAoRVrzBz1x+5al+7bUXhj6NT/QSMUSzbb+C6VOIlOikbk4d9l52XiscR2Q J1aTQg2QCchPLHrWyl4QuCakSN6C8oc++zdxgqZEXvh2DgjOQtzjtFwF+wtiWcHZd3oWY9H14RZHe RMZ+nYlmg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kFHXP-0007Lm-7K; Mon, 07 Sep 2020 13:50:27 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kFHV5-0006DV-DP for linux-arm-kernel@lists.infradead.org; Mon, 07 Sep 2020 13:48:08 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 5C30120757; Mon, 7 Sep 2020 13:48:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1599486482; bh=VTa8dsypGLCQ/kgeKikQVM5EjsVYlMzzdc2/AundBo0=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Iw+ERmQOtAJmfNSvQcjpnmyyWecGgwZAb3rCAqKbEMofhslaqlbVAPVo+jjCfTrne NxcDIaFPh8ZoqTtbVXXTO7HAA82OXDgfBzvd12NBvxIHwK8rrWaekEjZxvozqrUDAu fIVd2YaOAJqcWojRmUrjlROwC2djJfDgZWVycJN4= Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kFHV2-009mDK-Ei; Mon, 07 Sep 2020 14:48:00 +0100 Date: Mon, 07 Sep 2020 14:47:59 +0100 Message-ID: <87pn6xlmuo.wl-maz@kernel.org> From: Marc Zyngier To: Andrew Scull Subject: Re: [PATCH v3 16/18] KVM: arm64: nVHE: Migrate hyp interface to SMCCC In-Reply-To: <20200903135307.251331-17-ascull@google.com> References: <20200903135307.251331-1-ascull@google.com> <20200903135307.251331-17-ascull@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26.3 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: ascull@google.com, kvmarm@lists.cs.columbia.edu, james.morse@arm.com, suzuki.poulose@arm.com, julien.thierry.kdev@gmail.com, will@kernel.org, catalin.marinas@arm.com, kernel-team@android.com, sudeep.holla@arm.com, linux-arm-kernel@lists.infradead.org, dbrazdil@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200907_094803_636385_6725EBE2 X-CRM114-Status: GOOD ( 43.41 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kernel-team@android.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, james.morse@arm.com, linux-arm-kernel@lists.infradead.org, Sudeep Holla , David Brazdil , will@kernel.org, kvmarm@lists.cs.columbia.edu, julien.thierry.kdev@gmail.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 03 Sep 2020 14:53:05 +0100, Andrew Scull 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 > Signed-off-by: David Brazdil 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 > 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 > #include > #include > #include > @@ -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 > > /* > - * 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 > #include > > -typedef unsigned long (*hypcall_fn_t) > - (unsigned long, unsigned long, unsigned long); > +#include > + > +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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel