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=-7.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 0FD38C38A2A for ; Thu, 7 May 2020 13:10:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D3E1020870 for ; Thu, 7 May 2020 13:10:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1588857042; bh=ieqy+3LY8jE1+xFmcUK+l0Z6piJfFAFR7BpBJF3y+rM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=UfWimZi2qqJr4taC411JLeYvRgiUL4d0sG2Q32sA4kUXFCjUl2T+Q4liqkUK+42cl a9BTWBvjNvEmZPa7As3WS8BWCy6OJAiMJ9R8wpLT5GLfa09P/qAFCQJ+cbuoZyYPgJ NMnr0oN0Qs4d9zwBtsH1X3Ej4QNpYH2JPA6Bp7zg= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726308AbgEGNKm (ORCPT ); Thu, 7 May 2020 09:10:42 -0400 Received: from mail.kernel.org ([198.145.29.99]:45434 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725857AbgEGNKl (ORCPT ); Thu, 7 May 2020 09:10:41 -0400 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 4A16520735; Thu, 7 May 2020 13:10:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1588857040; bh=ieqy+3LY8jE1+xFmcUK+l0Z6piJfFAFR7BpBJF3y+rM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=NVRqGctajLq1T7nrzCEcqgRIwhpJvHQgGM205pfomc8MNeocCdmit1yibM9dVlPWP tGXPiRVWEnckU3WHmkZbRk6BfJ9qqol4jqoLUYjfzD8tB3uxBMDwyQuQnjw5pMphoG faii14U9EKzB78Ydmqv4HYY2m2lRthKbdOYRmOxs= 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 1jWgIQ-00AGiC-Lz; Thu, 07 May 2020 14:10:38 +0100 Date: Thu, 07 May 2020 14:10:30 +0100 Message-ID: <87d07fj3g9.wl-maz@kernel.org> From: Marc Zyngier To: David Brazdil Cc: Catalin Marinas , James Morse , Julien Thierry , Suzuki K Poulose , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Quentin Perret Subject: Re: [PATCH 02/15] arm64: kvm: Formalize host-hyp hypcall ABI In-Reply-To: <20200430144831.59194-3-dbrazdil@google.com> References: <20200430144831.59194-1-dbrazdil@google.com> <20200430144831.59194-3-dbrazdil@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 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: dbrazdil@google.com, catalin.marinas@arm.com, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, will@kernel.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, qperret@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi David, Quentin, On Thu, 30 Apr 2020 15:48:18 +0100, David Brazdil wrote: > > From: Quentin Perret > > In preparation for removing the hyp code from the TCB, convert the current nit: Unless we have a different interpretation of the TCB acronym, I think you want to remove anything *but* the EL2 code from the TCB. > EL1 - EL2 KVM ABI to use hypercall numbers in lieu of direct function pointers. > While this in itself does not provide any isolation, it is a first step towards > having a properly defined EL2 ABI. > > The implementation is based on a kvm_hcall_table which holds function pointers > to the hyp_text functions corresponding to each hypercall. This is highly > inspired from the arm64 sys_call_table, the main difference being the lack of > hcall wrappers at this stage. > > Signed-off-by: Quentin Perret > Signed-off-by: David Brazdil > --- > arch/arm64/include/asm/kvm_host.h | 20 ++++++- > arch/arm64/include/asm/kvm_host_hypercalls.h | 62 ++++++++++++++++++++ > arch/arm64/kvm/hyp/Makefile | 2 +- > arch/arm64/kvm/hyp/host_hypercall.c | 22 +++++++ > arch/arm64/kvm/hyp/hyp-entry.S | 36 +++++++++++- > 5 files changed, 137 insertions(+), 5 deletions(-) > create mode 100644 arch/arm64/include/asm/kvm_host_hypercalls.h > create mode 100644 arch/arm64/kvm/hyp/host_hypercall.c > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index e61143d6602d..5dec3b06f2b7 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -447,10 +448,25 @@ 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); > > -#define kvm_call_hyp_nvhe(hypfn, ...) \ > - __kvm_call_hyp((unsigned long)kvm_ksym_ref(hypfn), ##__VA_ARGS__) > +/* > + * Call the hypervisor via HVC. The hcall parameter must be the name of > + * a hypercall as defined in . > + * > + * Hypercalls take at most 6 parameters. > + */ > +#define kvm_call_hyp_nvhe(hcall, ...) \ > + __kvm_call_hyp(KVM_HOST_HCALL_NR(hcall), ##__VA_ARGS__) > > /* > + * u64 kvm_call_hyp(hcall, ...); > + * > + * Call the hypervisor. The hcall parameter must be the name of a hypercall > + * defined in . In the VHE case, this will be > + * translated into a direct function call. > + * > + * A hcall value of 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. > + * > * The couple of isb() below are there to guarantee the same behaviour > * on VHE as on !VHE, where the eret to EL1 acts as a context > * synchronization event. > diff --git a/arch/arm64/include/asm/kvm_host_hypercalls.h b/arch/arm64/include/asm/kvm_host_hypercalls.h > new file mode 100644 > index 000000000000..af8ad505d816 > --- /dev/null > +++ b/arch/arm64/include/asm/kvm_host_hypercalls.h > @@ -0,0 +1,62 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2020 Google, inc > + */ > + > +#ifndef __KVM_HOST_HCALL > +#define __KVM_HOST_HCALL(x) > +#endif > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_enable_ssbs 0 > +__KVM_HOST_HCALL(__kvm_enable_ssbs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_get_mdcr_el2 1 > +__KVM_HOST_HCALL(__kvm_get_mdcr_el2) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_timer_set_cntvoff 2 > +__KVM_HOST_HCALL(__kvm_timer_set_cntvoff) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_local_vmid 3 > +__KVM_HOST_HCALL(__kvm_tlb_flush_local_vmid) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_flush_vm_context 4 > +__KVM_HOST_HCALL(__kvm_flush_vm_context) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_vcpu_run_nvhe 5 > +__KVM_HOST_HCALL(__kvm_vcpu_run_nvhe) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_vmid 6 > +__KVM_HOST_HCALL(__kvm_tlb_flush_vmid) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_vmid_ipa 7 > +__KVM_HOST_HCALL(__kvm_tlb_flush_vmid_ipa) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_init_lrs 8 > +__KVM_HOST_HCALL(__vgic_v3_init_lrs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_get_ich_vtr_el2 9 > +__KVM_HOST_HCALL(__vgic_v3_get_ich_vtr_el2) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_write_vmcr 10 > +__KVM_HOST_HCALL(__vgic_v3_write_vmcr) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_restore_aprs 11 > +__KVM_HOST_HCALL(__vgic_v3_restore_aprs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_read_vmcr 12 > +__KVM_HOST_HCALL(__vgic_v3_read_vmcr) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_save_aprs 13 > +__KVM_HOST_HCALL(__vgic_v3_save_aprs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX_SIZE 14 This whole thing screams "enum" into my ears. Having to maintain these as #defines feels like a pain, specially if the numbers are never used in assembly code. (and for that, we have asm-offset.h). > + > +/* XXX - Arbitrary offset for KVM-related hypercalls */ > +#define __KVM_HOST_HCALL_BASE_SHIFT 8 > +#define __KVM_HOST_HCALL_BASE (1ULL << __KVM_HOST_HCALL_BASE_SHIFT) > +#define __KVM_HOST_HCALL_END (__KVM_HOST_HCALL_BASE + \ > + __KVM_HOST_HCALL_TABLE_IDX_SIZE) I don't really get what is this offset for. It is too small to be used to skip the stub hypercalls (you'd need to start at 0x1000). Given that you store the whole thing in an array, you're wasting some memory. I'm sure you have a good story for it though! ;-) > + > +/* Hypercall number = kvm offset + table idx */ > +#define KVM_HOST_HCALL_NR(name) (__KVM_HOST_HCALL_TABLE_IDX_##name + \ > + __KVM_HOST_HCALL_BASE) > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > index 8c9880783839..29e2b2cd2fbc 100644 > --- a/arch/arm64/kvm/hyp/Makefile > +++ b/arch/arm64/kvm/hyp/Makefile > @@ -9,7 +9,7 @@ ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING \ > obj-$(CONFIG_KVM) += hyp.o > > hyp-y := vgic-v3-sr.o timer-sr.o aarch32.o vgic-v2-cpuif-proxy.o sysreg-sr.o \ > - debug-sr.o entry.o switch.o fpsimd.o tlb.o hyp-entry.o > + debug-sr.o entry.o switch.o fpsimd.o tlb.o host_hypercall.o hyp-entry.o > > # KVM code is run at a different exception code with a different map, so > # compiler instrumentation that inserts callbacks or checks into the code may > diff --git a/arch/arm64/kvm/hyp/host_hypercall.c b/arch/arm64/kvm/hyp/host_hypercall.c > new file mode 100644 > index 000000000000..6b31310f36a8 > --- /dev/null > +++ b/arch/arm64/kvm/hyp/host_hypercall.c > @@ -0,0 +1,22 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2020 Google, inc > + */ > + > +#include > + > +typedef long (*kvm_hcall_fn_t)(void); > + > +static long __hyp_text kvm_hc_ni(void) > +{ > + return -ENOSYS; > +} > + > +#undef __KVM_HOST_HCALL > +#define __KVM_HOST_HCALL(name) \ > + [__KVM_HOST_HCALL_TABLE_IDX_##name] = (long (*)(void))name, > + > +const kvm_hcall_fn_t kvm_hcall_table[__KVM_HOST_HCALL_TABLE_IDX_SIZE] = { > + [0 ... __KVM_HOST_HCALL_TABLE_IDX_SIZE-1] = kvm_hc_ni, > +#include > +}; Cunning. At the same time, if you can do this once, you can do it twice, and generating the __KVM_HOST_HCALL_TABLE_IDX_* as an enum should be pretty easy, and could live in its own include file. > diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S > index c2a13ab3c471..1a51a0958504 100644 > --- a/arch/arm64/kvm/hyp/hyp-entry.S > +++ b/arch/arm64/kvm/hyp/hyp-entry.S > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > > .text > @@ -21,17 +22,26 @@ > .macro do_el2_call > /* > * Shuffle the parameters before calling the function > - * pointed to in x0. Assumes parameters in x[1,2,3]. > + * pointed to in x0. Assumes parameters in x[1,2,3,4,5,6]. > */ > str lr, [sp, #-16]! > mov lr, x0 > mov x0, x1 > mov x1, x2 > mov x2, x3 > + mov x3, x4 > + mov x4, x5 > + mov x5, x6 Has any function changed prototype as part of this patch that they'd require this change? It doesn't look like it. I guess this should be part of another patch. > blr lr > ldr lr, [sp], #16 > .endm > > +/* kern_hyp_va(lm_alias(ksym)) */ > +.macro ksym_hyp_va ksym, lm_offset > + sub \ksym, \ksym, \lm_offset > + kern_hyp_va \ksym > +.endm > + > el1_sync: // Guest trapped into EL2 > > mrs x0, esr_el2 > @@ -66,10 +76,32 @@ el1_sync: // Guest trapped into EL2 > br x5 > > 1: > + /* Check if the hcall number is valid */ > + cmp x0, #__KVM_HOST_HCALL_BASE > + b.lt 2f > + cmp x0, #__KVM_HOST_HCALL_END > + b.lt 3f > +2: > + mov x0, -ENOSYS > + eret > + > +3: > + /* Compute lm_alias() offset in x9 */ > + ldr_l x9, kimage_voffset > + ldr_l x10, physvirt_offset > + add x9, x9, x10 > + > + /* Find hcall function pointer in the table */ > + ldr x10, =kvm_hcall_table > + ksym_hyp_va x10, x9 Can't kvm_hcall_table be referenced with adr or adr_l? It'd certainly be nice to avoid the extra load for something that is essentially a build-time constant. Another thing that could be improved is to keep the lm_alias offset somewhere, saving one load per entry. Not a huge deal. > + sub x0, x0, #__KVM_HOST_HCALL_BASE > + add x0, x10, x0, lsl 3 The usual convention for immediate is to prefix them with #. > + ldr x0, [x0] > + ksym_hyp_va x0, x9 > + > /* > * Perform the EL2 call > */ > - kern_hyp_va x0 > do_el2_call > > eret > -- > 2.26.1 > > Thanks, M. -- Without deviation from the norm, progress is not possible. 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=-6.8 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, 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 ACFF4C54E49 for ; Thu, 7 May 2020 13:10:46 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 3A9ED20870 for ; Thu, 7 May 2020 13:10:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="NVRqGcta" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3A9ED20870 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id DA2304B31A; Thu, 7 May 2020 09:10:45 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@kernel.org Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id lWG5fSkp33uK; Thu, 7 May 2020 09:10:44 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 5493C4B2D2; Thu, 7 May 2020 09:10:44 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id F39994B30E for ; Thu, 7 May 2020 09:10:42 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id w8aR5YK5GVlZ for ; Thu, 7 May 2020 09:10:41 -0400 (EDT) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 65F624B307 for ; Thu, 7 May 2020 09:10:41 -0400 (EDT) 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 4A16520735; Thu, 7 May 2020 13:10:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1588857040; bh=ieqy+3LY8jE1+xFmcUK+l0Z6piJfFAFR7BpBJF3y+rM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=NVRqGctajLq1T7nrzCEcqgRIwhpJvHQgGM205pfomc8MNeocCdmit1yibM9dVlPWP tGXPiRVWEnckU3WHmkZbRk6BfJ9qqol4jqoLUYjfzD8tB3uxBMDwyQuQnjw5pMphoG faii14U9EKzB78Ydmqv4HYY2m2lRthKbdOYRmOxs= 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 1jWgIQ-00AGiC-Lz; Thu, 07 May 2020 14:10:38 +0100 Date: Thu, 07 May 2020 14:10:30 +0100 Message-ID: <87d07fj3g9.wl-maz@kernel.org> From: Marc Zyngier To: David Brazdil Subject: Re: [PATCH 02/15] arm64: kvm: Formalize host-hyp hypcall ABI In-Reply-To: <20200430144831.59194-3-dbrazdil@google.com> References: <20200430144831.59194-1-dbrazdil@google.com> <20200430144831.59194-3-dbrazdil@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 (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: dbrazdil@google.com, catalin.marinas@arm.com, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, will@kernel.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, qperret@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Cc: Catalin Marinas , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Will Deacon , kvmarm@lists.cs.columbia.edu X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu Hi David, Quentin, On Thu, 30 Apr 2020 15:48:18 +0100, David Brazdil wrote: > > From: Quentin Perret > > In preparation for removing the hyp code from the TCB, convert the current nit: Unless we have a different interpretation of the TCB acronym, I think you want to remove anything *but* the EL2 code from the TCB. > EL1 - EL2 KVM ABI to use hypercall numbers in lieu of direct function pointers. > While this in itself does not provide any isolation, it is a first step towards > having a properly defined EL2 ABI. > > The implementation is based on a kvm_hcall_table which holds function pointers > to the hyp_text functions corresponding to each hypercall. This is highly > inspired from the arm64 sys_call_table, the main difference being the lack of > hcall wrappers at this stage. > > Signed-off-by: Quentin Perret > Signed-off-by: David Brazdil > --- > arch/arm64/include/asm/kvm_host.h | 20 ++++++- > arch/arm64/include/asm/kvm_host_hypercalls.h | 62 ++++++++++++++++++++ > arch/arm64/kvm/hyp/Makefile | 2 +- > arch/arm64/kvm/hyp/host_hypercall.c | 22 +++++++ > arch/arm64/kvm/hyp/hyp-entry.S | 36 +++++++++++- > 5 files changed, 137 insertions(+), 5 deletions(-) > create mode 100644 arch/arm64/include/asm/kvm_host_hypercalls.h > create mode 100644 arch/arm64/kvm/hyp/host_hypercall.c > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index e61143d6602d..5dec3b06f2b7 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -447,10 +448,25 @@ 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); > > -#define kvm_call_hyp_nvhe(hypfn, ...) \ > - __kvm_call_hyp((unsigned long)kvm_ksym_ref(hypfn), ##__VA_ARGS__) > +/* > + * Call the hypervisor via HVC. The hcall parameter must be the name of > + * a hypercall as defined in . > + * > + * Hypercalls take at most 6 parameters. > + */ > +#define kvm_call_hyp_nvhe(hcall, ...) \ > + __kvm_call_hyp(KVM_HOST_HCALL_NR(hcall), ##__VA_ARGS__) > > /* > + * u64 kvm_call_hyp(hcall, ...); > + * > + * Call the hypervisor. The hcall parameter must be the name of a hypercall > + * defined in . In the VHE case, this will be > + * translated into a direct function call. > + * > + * A hcall value of 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. > + * > * The couple of isb() below are there to guarantee the same behaviour > * on VHE as on !VHE, where the eret to EL1 acts as a context > * synchronization event. > diff --git a/arch/arm64/include/asm/kvm_host_hypercalls.h b/arch/arm64/include/asm/kvm_host_hypercalls.h > new file mode 100644 > index 000000000000..af8ad505d816 > --- /dev/null > +++ b/arch/arm64/include/asm/kvm_host_hypercalls.h > @@ -0,0 +1,62 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2020 Google, inc > + */ > + > +#ifndef __KVM_HOST_HCALL > +#define __KVM_HOST_HCALL(x) > +#endif > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_enable_ssbs 0 > +__KVM_HOST_HCALL(__kvm_enable_ssbs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_get_mdcr_el2 1 > +__KVM_HOST_HCALL(__kvm_get_mdcr_el2) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_timer_set_cntvoff 2 > +__KVM_HOST_HCALL(__kvm_timer_set_cntvoff) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_local_vmid 3 > +__KVM_HOST_HCALL(__kvm_tlb_flush_local_vmid) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_flush_vm_context 4 > +__KVM_HOST_HCALL(__kvm_flush_vm_context) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_vcpu_run_nvhe 5 > +__KVM_HOST_HCALL(__kvm_vcpu_run_nvhe) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_vmid 6 > +__KVM_HOST_HCALL(__kvm_tlb_flush_vmid) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_vmid_ipa 7 > +__KVM_HOST_HCALL(__kvm_tlb_flush_vmid_ipa) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_init_lrs 8 > +__KVM_HOST_HCALL(__vgic_v3_init_lrs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_get_ich_vtr_el2 9 > +__KVM_HOST_HCALL(__vgic_v3_get_ich_vtr_el2) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_write_vmcr 10 > +__KVM_HOST_HCALL(__vgic_v3_write_vmcr) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_restore_aprs 11 > +__KVM_HOST_HCALL(__vgic_v3_restore_aprs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_read_vmcr 12 > +__KVM_HOST_HCALL(__vgic_v3_read_vmcr) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_save_aprs 13 > +__KVM_HOST_HCALL(__vgic_v3_save_aprs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX_SIZE 14 This whole thing screams "enum" into my ears. Having to maintain these as #defines feels like a pain, specially if the numbers are never used in assembly code. (and for that, we have asm-offset.h). > + > +/* XXX - Arbitrary offset for KVM-related hypercalls */ > +#define __KVM_HOST_HCALL_BASE_SHIFT 8 > +#define __KVM_HOST_HCALL_BASE (1ULL << __KVM_HOST_HCALL_BASE_SHIFT) > +#define __KVM_HOST_HCALL_END (__KVM_HOST_HCALL_BASE + \ > + __KVM_HOST_HCALL_TABLE_IDX_SIZE) I don't really get what is this offset for. It is too small to be used to skip the stub hypercalls (you'd need to start at 0x1000). Given that you store the whole thing in an array, you're wasting some memory. I'm sure you have a good story for it though! ;-) > + > +/* Hypercall number = kvm offset + table idx */ > +#define KVM_HOST_HCALL_NR(name) (__KVM_HOST_HCALL_TABLE_IDX_##name + \ > + __KVM_HOST_HCALL_BASE) > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > index 8c9880783839..29e2b2cd2fbc 100644 > --- a/arch/arm64/kvm/hyp/Makefile > +++ b/arch/arm64/kvm/hyp/Makefile > @@ -9,7 +9,7 @@ ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING \ > obj-$(CONFIG_KVM) += hyp.o > > hyp-y := vgic-v3-sr.o timer-sr.o aarch32.o vgic-v2-cpuif-proxy.o sysreg-sr.o \ > - debug-sr.o entry.o switch.o fpsimd.o tlb.o hyp-entry.o > + debug-sr.o entry.o switch.o fpsimd.o tlb.o host_hypercall.o hyp-entry.o > > # KVM code is run at a different exception code with a different map, so > # compiler instrumentation that inserts callbacks or checks into the code may > diff --git a/arch/arm64/kvm/hyp/host_hypercall.c b/arch/arm64/kvm/hyp/host_hypercall.c > new file mode 100644 > index 000000000000..6b31310f36a8 > --- /dev/null > +++ b/arch/arm64/kvm/hyp/host_hypercall.c > @@ -0,0 +1,22 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2020 Google, inc > + */ > + > +#include > + > +typedef long (*kvm_hcall_fn_t)(void); > + > +static long __hyp_text kvm_hc_ni(void) > +{ > + return -ENOSYS; > +} > + > +#undef __KVM_HOST_HCALL > +#define __KVM_HOST_HCALL(name) \ > + [__KVM_HOST_HCALL_TABLE_IDX_##name] = (long (*)(void))name, > + > +const kvm_hcall_fn_t kvm_hcall_table[__KVM_HOST_HCALL_TABLE_IDX_SIZE] = { > + [0 ... __KVM_HOST_HCALL_TABLE_IDX_SIZE-1] = kvm_hc_ni, > +#include > +}; Cunning. At the same time, if you can do this once, you can do it twice, and generating the __KVM_HOST_HCALL_TABLE_IDX_* as an enum should be pretty easy, and could live in its own include file. > diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S > index c2a13ab3c471..1a51a0958504 100644 > --- a/arch/arm64/kvm/hyp/hyp-entry.S > +++ b/arch/arm64/kvm/hyp/hyp-entry.S > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > > .text > @@ -21,17 +22,26 @@ > .macro do_el2_call > /* > * Shuffle the parameters before calling the function > - * pointed to in x0. Assumes parameters in x[1,2,3]. > + * pointed to in x0. Assumes parameters in x[1,2,3,4,5,6]. > */ > str lr, [sp, #-16]! > mov lr, x0 > mov x0, x1 > mov x1, x2 > mov x2, x3 > + mov x3, x4 > + mov x4, x5 > + mov x5, x6 Has any function changed prototype as part of this patch that they'd require this change? It doesn't look like it. I guess this should be part of another patch. > blr lr > ldr lr, [sp], #16 > .endm > > +/* kern_hyp_va(lm_alias(ksym)) */ > +.macro ksym_hyp_va ksym, lm_offset > + sub \ksym, \ksym, \lm_offset > + kern_hyp_va \ksym > +.endm > + > el1_sync: // Guest trapped into EL2 > > mrs x0, esr_el2 > @@ -66,10 +76,32 @@ el1_sync: // Guest trapped into EL2 > br x5 > > 1: > + /* Check if the hcall number is valid */ > + cmp x0, #__KVM_HOST_HCALL_BASE > + b.lt 2f > + cmp x0, #__KVM_HOST_HCALL_END > + b.lt 3f > +2: > + mov x0, -ENOSYS > + eret > + > +3: > + /* Compute lm_alias() offset in x9 */ > + ldr_l x9, kimage_voffset > + ldr_l x10, physvirt_offset > + add x9, x9, x10 > + > + /* Find hcall function pointer in the table */ > + ldr x10, =kvm_hcall_table > + ksym_hyp_va x10, x9 Can't kvm_hcall_table be referenced with adr or adr_l? It'd certainly be nice to avoid the extra load for something that is essentially a build-time constant. Another thing that could be improved is to keep the lm_alias offset somewhere, saving one load per entry. Not a huge deal. > + sub x0, x0, #__KVM_HOST_HCALL_BASE > + add x0, x10, x0, lsl 3 The usual convention for immediate is to prefix them with #. > + ldr x0, [x0] > + ksym_hyp_va x0, x9 > + > /* > * Perform the EL2 call > */ > - kern_hyp_va x0 > do_el2_call > > eret > -- > 2.26.1 > > 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 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=-7.0 required=3.0 tests=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 57195C38A24 for ; Thu, 7 May 2020 13:10:45 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 2B57A20735 for ; Thu, 7 May 2020 13:10:45 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="PkClx8en"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="NVRqGcta" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2B57A20735 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+infradead-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=bombadil.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=9s95P8nzz3JluhVnGXiiGtGnqocGiwyxgLoxBRNBsGI=; b=PkClx8enqNad+L Yd4ICGCfQwcZuDnfoIiknc7clkMOAmIivJx2PhVM4KsLvIgnGlzwEkLAr3TQk7essw9onEFOwWcPE ynKBDo0vlo/VVbSdVeFHZatZ3RVduiC6EM9+k3Z2M2b7w9IO1UeM+yqy8wGUfcT9k0dCrDBmoOg+l X4RKZ+Au30uotLwdWnt5Ak/wm6Lvhspj38qk9YKto3DuMn1LoKEzfuUEBWlQVAfCITqEzhPQALb0I i2dONYMRdKu00E/jPDts4JuVVnv5VJbHrnWp4LQW475yF1vau6zJ3CL1zjm3CwpAx2WJTrs1IyI4b FJehs6Lu7qGDvZ7biM3g==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jWgIW-0007cu-3F; Thu, 07 May 2020 13:10:44 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jWgIS-0007cZ-NN for linux-arm-kernel@lists.infradead.org; Thu, 07 May 2020 13:10:42 +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 4A16520735; Thu, 7 May 2020 13:10:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1588857040; bh=ieqy+3LY8jE1+xFmcUK+l0Z6piJfFAFR7BpBJF3y+rM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=NVRqGctajLq1T7nrzCEcqgRIwhpJvHQgGM205pfomc8MNeocCdmit1yibM9dVlPWP tGXPiRVWEnckU3WHmkZbRk6BfJ9qqol4jqoLUYjfzD8tB3uxBMDwyQuQnjw5pMphoG faii14U9EKzB78Ydmqv4HYY2m2lRthKbdOYRmOxs= 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 1jWgIQ-00AGiC-Lz; Thu, 07 May 2020 14:10:38 +0100 Date: Thu, 07 May 2020 14:10:30 +0100 Message-ID: <87d07fj3g9.wl-maz@kernel.org> From: Marc Zyngier To: David Brazdil Subject: Re: [PATCH 02/15] arm64: kvm: Formalize host-hyp hypcall ABI In-Reply-To: <20200430144831.59194-3-dbrazdil@google.com> References: <20200430144831.59194-1-dbrazdil@google.com> <20200430144831.59194-3-dbrazdil@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 (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: dbrazdil@google.com, catalin.marinas@arm.com, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, will@kernel.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, qperret@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-20200507_061040_801818_D2F517B4 X-CRM114-Status: GOOD ( 35.08 ) 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: Suzuki K Poulose , Catalin Marinas , Quentin Perret , linux-kernel@vger.kernel.org, James Morse , linux-arm-kernel@lists.infradead.org, Will Deacon , kvmarm@lists.cs.columbia.edu, Julien Thierry Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi David, Quentin, On Thu, 30 Apr 2020 15:48:18 +0100, David Brazdil wrote: > > From: Quentin Perret > > In preparation for removing the hyp code from the TCB, convert the current nit: Unless we have a different interpretation of the TCB acronym, I think you want to remove anything *but* the EL2 code from the TCB. > EL1 - EL2 KVM ABI to use hypercall numbers in lieu of direct function pointers. > While this in itself does not provide any isolation, it is a first step towards > having a properly defined EL2 ABI. > > The implementation is based on a kvm_hcall_table which holds function pointers > to the hyp_text functions corresponding to each hypercall. This is highly > inspired from the arm64 sys_call_table, the main difference being the lack of > hcall wrappers at this stage. > > Signed-off-by: Quentin Perret > Signed-off-by: David Brazdil > --- > arch/arm64/include/asm/kvm_host.h | 20 ++++++- > arch/arm64/include/asm/kvm_host_hypercalls.h | 62 ++++++++++++++++++++ > arch/arm64/kvm/hyp/Makefile | 2 +- > arch/arm64/kvm/hyp/host_hypercall.c | 22 +++++++ > arch/arm64/kvm/hyp/hyp-entry.S | 36 +++++++++++- > 5 files changed, 137 insertions(+), 5 deletions(-) > create mode 100644 arch/arm64/include/asm/kvm_host_hypercalls.h > create mode 100644 arch/arm64/kvm/hyp/host_hypercall.c > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index e61143d6602d..5dec3b06f2b7 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -447,10 +448,25 @@ 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); > > -#define kvm_call_hyp_nvhe(hypfn, ...) \ > - __kvm_call_hyp((unsigned long)kvm_ksym_ref(hypfn), ##__VA_ARGS__) > +/* > + * Call the hypervisor via HVC. The hcall parameter must be the name of > + * a hypercall as defined in . > + * > + * Hypercalls take at most 6 parameters. > + */ > +#define kvm_call_hyp_nvhe(hcall, ...) \ > + __kvm_call_hyp(KVM_HOST_HCALL_NR(hcall), ##__VA_ARGS__) > > /* > + * u64 kvm_call_hyp(hcall, ...); > + * > + * Call the hypervisor. The hcall parameter must be the name of a hypercall > + * defined in . In the VHE case, this will be > + * translated into a direct function call. > + * > + * A hcall value of 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. > + * > * The couple of isb() below are there to guarantee the same behaviour > * on VHE as on !VHE, where the eret to EL1 acts as a context > * synchronization event. > diff --git a/arch/arm64/include/asm/kvm_host_hypercalls.h b/arch/arm64/include/asm/kvm_host_hypercalls.h > new file mode 100644 > index 000000000000..af8ad505d816 > --- /dev/null > +++ b/arch/arm64/include/asm/kvm_host_hypercalls.h > @@ -0,0 +1,62 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2020 Google, inc > + */ > + > +#ifndef __KVM_HOST_HCALL > +#define __KVM_HOST_HCALL(x) > +#endif > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_enable_ssbs 0 > +__KVM_HOST_HCALL(__kvm_enable_ssbs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_get_mdcr_el2 1 > +__KVM_HOST_HCALL(__kvm_get_mdcr_el2) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_timer_set_cntvoff 2 > +__KVM_HOST_HCALL(__kvm_timer_set_cntvoff) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_local_vmid 3 > +__KVM_HOST_HCALL(__kvm_tlb_flush_local_vmid) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_flush_vm_context 4 > +__KVM_HOST_HCALL(__kvm_flush_vm_context) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_vcpu_run_nvhe 5 > +__KVM_HOST_HCALL(__kvm_vcpu_run_nvhe) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_vmid 6 > +__KVM_HOST_HCALL(__kvm_tlb_flush_vmid) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_vmid_ipa 7 > +__KVM_HOST_HCALL(__kvm_tlb_flush_vmid_ipa) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_init_lrs 8 > +__KVM_HOST_HCALL(__vgic_v3_init_lrs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_get_ich_vtr_el2 9 > +__KVM_HOST_HCALL(__vgic_v3_get_ich_vtr_el2) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_write_vmcr 10 > +__KVM_HOST_HCALL(__vgic_v3_write_vmcr) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_restore_aprs 11 > +__KVM_HOST_HCALL(__vgic_v3_restore_aprs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_read_vmcr 12 > +__KVM_HOST_HCALL(__vgic_v3_read_vmcr) > + > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_save_aprs 13 > +__KVM_HOST_HCALL(__vgic_v3_save_aprs) > + > +#define __KVM_HOST_HCALL_TABLE_IDX_SIZE 14 This whole thing screams "enum" into my ears. Having to maintain these as #defines feels like a pain, specially if the numbers are never used in assembly code. (and for that, we have asm-offset.h). > + > +/* XXX - Arbitrary offset for KVM-related hypercalls */ > +#define __KVM_HOST_HCALL_BASE_SHIFT 8 > +#define __KVM_HOST_HCALL_BASE (1ULL << __KVM_HOST_HCALL_BASE_SHIFT) > +#define __KVM_HOST_HCALL_END (__KVM_HOST_HCALL_BASE + \ > + __KVM_HOST_HCALL_TABLE_IDX_SIZE) I don't really get what is this offset for. It is too small to be used to skip the stub hypercalls (you'd need to start at 0x1000). Given that you store the whole thing in an array, you're wasting some memory. I'm sure you have a good story for it though! ;-) > + > +/* Hypercall number = kvm offset + table idx */ > +#define KVM_HOST_HCALL_NR(name) (__KVM_HOST_HCALL_TABLE_IDX_##name + \ > + __KVM_HOST_HCALL_BASE) > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > index 8c9880783839..29e2b2cd2fbc 100644 > --- a/arch/arm64/kvm/hyp/Makefile > +++ b/arch/arm64/kvm/hyp/Makefile > @@ -9,7 +9,7 @@ ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING \ > obj-$(CONFIG_KVM) += hyp.o > > hyp-y := vgic-v3-sr.o timer-sr.o aarch32.o vgic-v2-cpuif-proxy.o sysreg-sr.o \ > - debug-sr.o entry.o switch.o fpsimd.o tlb.o hyp-entry.o > + debug-sr.o entry.o switch.o fpsimd.o tlb.o host_hypercall.o hyp-entry.o > > # KVM code is run at a different exception code with a different map, so > # compiler instrumentation that inserts callbacks or checks into the code may > diff --git a/arch/arm64/kvm/hyp/host_hypercall.c b/arch/arm64/kvm/hyp/host_hypercall.c > new file mode 100644 > index 000000000000..6b31310f36a8 > --- /dev/null > +++ b/arch/arm64/kvm/hyp/host_hypercall.c > @@ -0,0 +1,22 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2020 Google, inc > + */ > + > +#include > + > +typedef long (*kvm_hcall_fn_t)(void); > + > +static long __hyp_text kvm_hc_ni(void) > +{ > + return -ENOSYS; > +} > + > +#undef __KVM_HOST_HCALL > +#define __KVM_HOST_HCALL(name) \ > + [__KVM_HOST_HCALL_TABLE_IDX_##name] = (long (*)(void))name, > + > +const kvm_hcall_fn_t kvm_hcall_table[__KVM_HOST_HCALL_TABLE_IDX_SIZE] = { > + [0 ... __KVM_HOST_HCALL_TABLE_IDX_SIZE-1] = kvm_hc_ni, > +#include > +}; Cunning. At the same time, if you can do this once, you can do it twice, and generating the __KVM_HOST_HCALL_TABLE_IDX_* as an enum should be pretty easy, and could live in its own include file. > diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S > index c2a13ab3c471..1a51a0958504 100644 > --- a/arch/arm64/kvm/hyp/hyp-entry.S > +++ b/arch/arm64/kvm/hyp/hyp-entry.S > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > > .text > @@ -21,17 +22,26 @@ > .macro do_el2_call > /* > * Shuffle the parameters before calling the function > - * pointed to in x0. Assumes parameters in x[1,2,3]. > + * pointed to in x0. Assumes parameters in x[1,2,3,4,5,6]. > */ > str lr, [sp, #-16]! > mov lr, x0 > mov x0, x1 > mov x1, x2 > mov x2, x3 > + mov x3, x4 > + mov x4, x5 > + mov x5, x6 Has any function changed prototype as part of this patch that they'd require this change? It doesn't look like it. I guess this should be part of another patch. > blr lr > ldr lr, [sp], #16 > .endm > > +/* kern_hyp_va(lm_alias(ksym)) */ > +.macro ksym_hyp_va ksym, lm_offset > + sub \ksym, \ksym, \lm_offset > + kern_hyp_va \ksym > +.endm > + > el1_sync: // Guest trapped into EL2 > > mrs x0, esr_el2 > @@ -66,10 +76,32 @@ el1_sync: // Guest trapped into EL2 > br x5 > > 1: > + /* Check if the hcall number is valid */ > + cmp x0, #__KVM_HOST_HCALL_BASE > + b.lt 2f > + cmp x0, #__KVM_HOST_HCALL_END > + b.lt 3f > +2: > + mov x0, -ENOSYS > + eret > + > +3: > + /* Compute lm_alias() offset in x9 */ > + ldr_l x9, kimage_voffset > + ldr_l x10, physvirt_offset > + add x9, x9, x10 > + > + /* Find hcall function pointer in the table */ > + ldr x10, =kvm_hcall_table > + ksym_hyp_va x10, x9 Can't kvm_hcall_table be referenced with adr or adr_l? It'd certainly be nice to avoid the extra load for something that is essentially a build-time constant. Another thing that could be improved is to keep the lm_alias offset somewhere, saving one load per entry. Not a huge deal. > + sub x0, x0, #__KVM_HOST_HCALL_BASE > + add x0, x10, x0, lsl 3 The usual convention for immediate is to prefix them with #. > + ldr x0, [x0] > + ksym_hyp_va x0, x9 > + > /* > * Perform the EL2 call > */ > - kern_hyp_va x0 > do_el2_call > > eret > -- > 2.26.1 > > 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