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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 69505C43334 for ; Wed, 20 Jul 2022 18:49:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229469AbiGTStN (ORCPT ); Wed, 20 Jul 2022 14:49:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35142 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230253AbiGTStL (ORCPT ); Wed, 20 Jul 2022 14:49:11 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0E26E73586 for ; Wed, 20 Jul 2022 11:49:08 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 4E6FE61983 for ; Wed, 20 Jul 2022 18:49:08 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DAF93C341C7; Wed, 20 Jul 2022 18:49:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1658342947; bh=08+Q+ucM7zcCNE6VbGiO3oIo7eApyt+93Izjlu+1QBs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=I5gDqg3ywx4qWRFZdqk2of8EjGxUwRP32WqTD3A6zedGZCkRpKUr07J2YyY2VOvtH odIeq+MaBWBPl29VbjTYBkjxxWSkaMAb8dcNRLwRQ+dTnpJQmDHyLkGLAQkjsiOGM7 mtZWTAJSwC67EdecVJP1U7BILEn1WqlO9TWMl9kk52Qfn3QIkbe8qXyYTR+/qXdgkJ u0x857jsmj1+Eu+tlUUO3oyqTRWRYrXMKq+l5rVsrUDZCp2biBvFkzXWm/MiUFidl/ drl3U8SY0zufJAPUvVbgQluKsTWrDmSFbmNhGSZTP4W4QV0QcJLz8LNCsve0S5mmOM 2vgsJ5MzLryYw== Date: Wed, 20 Jul 2022 19:48:59 +0100 From: Will Deacon To: Sean Christopherson Cc: kvmarm@lists.cs.columbia.edu, Ard Biesheuvel , Alexandru Elisei , Andy Lutomirski , Catalin Marinas , James Morse , Chao Peng , Quentin Perret , Suzuki K Poulose , Michael Roth , Mark Rutland , Fuad Tabba , Oliver Upton , Marc Zyngier , kernel-team@android.com, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 00/24] KVM: arm64: Introduce pKVM shadow state at EL2 Message-ID: <20220720184859.GD16603@willie-the-truck> References: <20220630135747.26983-1-will@kernel.org> <20220708162359.GA6286@willie-the-truck> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Hi Sean, On Tue, Jul 19, 2022 at 04:11:32PM +0000, Sean Christopherson wrote: > Apologies for the slow reply. No problem; you've provided a tonne of insightful feedback here, so it was worth the wait. Thanks! > On Fri, Jul 08, 2022, Will Deacon wrote: > > but I wanted to inherit the broader cc list so you were aware of this > > break-away series. Sadly, I don't think beefing up the commit messages would > > get us to a point where somebody unfamiliar with the EL2 code already could > > give a constructive review, but we can try to expand them a bit if you > > genuinely think it would help. > > I'm not looking at it just from a review point, but also from a future readers > perspective. E.g. someone that looks at this changelog in isolation is going to > have no idea what a "shadow VM" is: > > KVM: arm64: Introduce pKVM shadow VM state at EL2 > > Introduce a table of shadow VM structures at EL2 and provide hypercalls > to the host for creating and destroying shadow VMs. > > Obviously there will be some context available in surrounding patches, but if you > avoid the "shadow" terminology and provide a bit more context, then it yields > something like: > > KVM: arm64: Add infrastructure to create and track pKVM instances at EL2 > > Introduce a global table (and lock) to track pKVM instances at EL2, and > provide hypercalls that can be used by the untrusted host to create and > destroy pKVM VMs. pKVM VM/vCPU state is directly accessible only by the > trusted hypervisor (EL2). > > Each pKVM VM is directly associated with an untrusted host KVM instance, > and is referenced by the host using an opaque handle. Future patches will > provide hypercalls to allow the host to initialize/set/get pKVM VM/vCPU > state using the opaque handle. Thanks, that's much better. I'll have to summon up the energy to go through the others as well... > > Perhaps we should s/shadow/hyp/ to make this a little clearer? > > Or maybe just "pkvm"? I think the "hyp" part is useful to distinguish the pkvm code running at EL2 from the pkvm code running at EL1. For example, we have a 'pkvm' member in 'struct kvm_arch' which is used by the _host_ at EL1. So I'd say either "pkvm_hyp" or "hyp" instead of "shadow". The latter is nice and short... > I think that's especially viable if you do away with > kvm_shadow_vcpu_state. As of this series at least, kvm_shadow_vcpu_state is > completely unnecessary. kvm_vcpu.kvm can be used to get at the VM, and thus pKVM > state via container_of(). Then the host_vcpu can be retrieved by using the > vcpu_idx, e.g. > > struct pkvm_vm *pkvm_vm = to_pkvm_vm(pkvm_vcpu->vm); > struct kvm_vcpu *host_vcpu; > > host_vcpu = kvm_get_vcpu(pkvm_vm->host_vm, pkvm_vcpu->vcpu_idx); Using container_of() here is neat; we can definitely go ahead with that change. However, looking at this in more detail with Fuad, removing 'struct kvm_shadow_vcpu_state' entirely isn't going to work: > E.g. I believe you can make the code look like this: > > struct kvm_arch { > ... > > /* > * For an unstructed host VM, pkvm_handle is used to lookup the > * associated pKVM instance. > */ > pvk_handle_t pkvm_handle; > }; > > struct pkvm_vm { > struct kvm kvm; > > /* Backpointer to the host's (untrusted) KVM instance. */ > struct kvm *host_kvm; > > size_t donated_memory_size; > > struct kvm_pgtable pgt; > }; > > static struct kvm *pkvm_get_vm(pkvm_handle_t handle) > { > unsigned int idx = pkvm_handle_to_idx(handle); > > if (unlikely(idx >= KVM_MAX_PVMS)) > return NULL; > > return pkvm_vm_table[idx]; > } > > struct kvm_vcpu *pkvm_vcpu_load(pkvm_handle_t handle, unsigned int vcpu_idx) > { > struct kvm_vpcu *pkvm_vcpu = NULL; > struct kvm *vm; > > hyp_spin_lock(&pkvm_global_lock); > vm = pkvm_get_vm(handle); > if (!vm || atomic_read(&vm->online_vcpus) <= vcpu_idx) > goto unlock; > > pkvm_vcpu = kvm_get_vcpu(vm, vcpu_idx); kvm_get_vcpu() makes use of an xarray to hold the vCPUs pointers and this is really something which we cannot support at EL2 where, amongst other things, we do not have support for RCU. Consequently, we do need to keep our own mapping from the shad^H^H^H^Hhyp vCPU to the host vCPU. We also end up expanding the 'struct kvm_shadow_vcpu_state' structure later to track additional vCPU state in the hypervisor, for example in the mega-series: https://lore.kernel.org/kvmarm/20220519134204.5379-78-will@kernel.org/#Z31arch:arm64:kvm:hyp:include:nvhe:pkvm.h Cheers, Will 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 Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2508FCCA485 for ; Wed, 20 Jul 2022 18:49:13 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 9C6954D557; Wed, 20 Jul 2022 14:49:13 -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 Lsu1LGVDWl5g; Wed, 20 Jul 2022 14:49:12 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 406424D0BA; Wed, 20 Jul 2022 14:49:12 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 0446C4D04E for ; Wed, 20 Jul 2022 14:49:11 -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 EVfpPXINlkZ1 for ; Wed, 20 Jul 2022 14:49:08 -0400 (EDT) Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id AC9804D04D for ; Wed, 20 Jul 2022 14:49:08 -0400 (EDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E8D66619C4; Wed, 20 Jul 2022 18:49:07 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DAF93C341C7; Wed, 20 Jul 2022 18:49:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1658342947; bh=08+Q+ucM7zcCNE6VbGiO3oIo7eApyt+93Izjlu+1QBs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=I5gDqg3ywx4qWRFZdqk2of8EjGxUwRP32WqTD3A6zedGZCkRpKUr07J2YyY2VOvtH odIeq+MaBWBPl29VbjTYBkjxxWSkaMAb8dcNRLwRQ+dTnpJQmDHyLkGLAQkjsiOGM7 mtZWTAJSwC67EdecVJP1U7BILEn1WqlO9TWMl9kk52Qfn3QIkbe8qXyYTR+/qXdgkJ u0x857jsmj1+Eu+tlUUO3oyqTRWRYrXMKq+l5rVsrUDZCp2biBvFkzXWm/MiUFidl/ drl3U8SY0zufJAPUvVbgQluKsTWrDmSFbmNhGSZTP4W4QV0QcJLz8LNCsve0S5mmOM 2vgsJ5MzLryYw== Date: Wed, 20 Jul 2022 19:48:59 +0100 From: Will Deacon To: Sean Christopherson Subject: Re: [PATCH v2 00/24] KVM: arm64: Introduce pKVM shadow state at EL2 Message-ID: <20220720184859.GD16603@willie-the-truck> References: <20220630135747.26983-1-will@kernel.org> <20220708162359.GA6286@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Cc: Marc Zyngier , kernel-team@android.com, kvm@vger.kernel.org, Catalin Marinas , Oliver Upton , Andy Lutomirski , linux-arm-kernel@lists.infradead.org, Michael Roth , Chao Peng , 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 Sean, On Tue, Jul 19, 2022 at 04:11:32PM +0000, Sean Christopherson wrote: > Apologies for the slow reply. No problem; you've provided a tonne of insightful feedback here, so it was worth the wait. Thanks! > On Fri, Jul 08, 2022, Will Deacon wrote: > > but I wanted to inherit the broader cc list so you were aware of this > > break-away series. Sadly, I don't think beefing up the commit messages would > > get us to a point where somebody unfamiliar with the EL2 code already could > > give a constructive review, but we can try to expand them a bit if you > > genuinely think it would help. > > I'm not looking at it just from a review point, but also from a future readers > perspective. E.g. someone that looks at this changelog in isolation is going to > have no idea what a "shadow VM" is: > > KVM: arm64: Introduce pKVM shadow VM state at EL2 > > Introduce a table of shadow VM structures at EL2 and provide hypercalls > to the host for creating and destroying shadow VMs. > > Obviously there will be some context available in surrounding patches, but if you > avoid the "shadow" terminology and provide a bit more context, then it yields > something like: > > KVM: arm64: Add infrastructure to create and track pKVM instances at EL2 > > Introduce a global table (and lock) to track pKVM instances at EL2, and > provide hypercalls that can be used by the untrusted host to create and > destroy pKVM VMs. pKVM VM/vCPU state is directly accessible only by the > trusted hypervisor (EL2). > > Each pKVM VM is directly associated with an untrusted host KVM instance, > and is referenced by the host using an opaque handle. Future patches will > provide hypercalls to allow the host to initialize/set/get pKVM VM/vCPU > state using the opaque handle. Thanks, that's much better. I'll have to summon up the energy to go through the others as well... > > Perhaps we should s/shadow/hyp/ to make this a little clearer? > > Or maybe just "pkvm"? I think the "hyp" part is useful to distinguish the pkvm code running at EL2 from the pkvm code running at EL1. For example, we have a 'pkvm' member in 'struct kvm_arch' which is used by the _host_ at EL1. So I'd say either "pkvm_hyp" or "hyp" instead of "shadow". The latter is nice and short... > I think that's especially viable if you do away with > kvm_shadow_vcpu_state. As of this series at least, kvm_shadow_vcpu_state is > completely unnecessary. kvm_vcpu.kvm can be used to get at the VM, and thus pKVM > state via container_of(). Then the host_vcpu can be retrieved by using the > vcpu_idx, e.g. > > struct pkvm_vm *pkvm_vm = to_pkvm_vm(pkvm_vcpu->vm); > struct kvm_vcpu *host_vcpu; > > host_vcpu = kvm_get_vcpu(pkvm_vm->host_vm, pkvm_vcpu->vcpu_idx); Using container_of() here is neat; we can definitely go ahead with that change. However, looking at this in more detail with Fuad, removing 'struct kvm_shadow_vcpu_state' entirely isn't going to work: > E.g. I believe you can make the code look like this: > > struct kvm_arch { > ... > > /* > * For an unstructed host VM, pkvm_handle is used to lookup the > * associated pKVM instance. > */ > pvk_handle_t pkvm_handle; > }; > > struct pkvm_vm { > struct kvm kvm; > > /* Backpointer to the host's (untrusted) KVM instance. */ > struct kvm *host_kvm; > > size_t donated_memory_size; > > struct kvm_pgtable pgt; > }; > > static struct kvm *pkvm_get_vm(pkvm_handle_t handle) > { > unsigned int idx = pkvm_handle_to_idx(handle); > > if (unlikely(idx >= KVM_MAX_PVMS)) > return NULL; > > return pkvm_vm_table[idx]; > } > > struct kvm_vcpu *pkvm_vcpu_load(pkvm_handle_t handle, unsigned int vcpu_idx) > { > struct kvm_vpcu *pkvm_vcpu = NULL; > struct kvm *vm; > > hyp_spin_lock(&pkvm_global_lock); > vm = pkvm_get_vm(handle); > if (!vm || atomic_read(&vm->online_vcpus) <= vcpu_idx) > goto unlock; > > pkvm_vcpu = kvm_get_vcpu(vm, vcpu_idx); kvm_get_vcpu() makes use of an xarray to hold the vCPUs pointers and this is really something which we cannot support at EL2 where, amongst other things, we do not have support for RCU. Consequently, we do need to keep our own mapping from the shad^H^H^H^Hhyp vCPU to the host vCPU. We also end up expanding the 'struct kvm_shadow_vcpu_state' structure later to track additional vCPU state in the hypervisor, for example in the mega-series: https://lore.kernel.org/kvmarm/20220519134204.5379-78-will@kernel.org/#Z31arch:arm64:kvm:hyp:include:nvhe:pkvm.h Cheers, Will _______________________________________________ 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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id B3405C43334 for ; Wed, 20 Jul 2022 18:50:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=pHyz9DtCYpmEWxaTNSfvNjzYYptY2pv9ZOxKIqYaKCk=; b=0RztcG/n/TkpVK 8XUAv8LaHYmnhU6wmnKEJYX4YcMbPp4xYgrlEhqVcRl6lRnip+wnLUc/vxO9vUdCgweNxLUZnonna I+RFL7jqN50X6YAK8UhQYtY9C5bIPq8ra11SUAIF2JeOBp+9KGFXryocDdmsZ43KpWKCR+0KfDL/X HJOJhV5S2wYJ+C+PZTKqUalOdnAeXNyCm2O9uMacXX6/IQ5djjgbFq58+s9SR+kz2LSRXNcpKh31t tT3RB+cVfHBypyNL4X425l0GpdaSC7S/Qb9rFhMa6OXnXISTu8s7sptG/I4eX+yxZwonxz/e3wHJU 9QxNdg8UDXRfYN1c/wCw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oEEkz-009Uqg-Vk; Wed, 20 Jul 2022 18:49:14 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oEEkw-009UnX-UN for linux-arm-kernel@lists.infradead.org; Wed, 20 Jul 2022 18:49:12 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E8D66619C4; Wed, 20 Jul 2022 18:49:07 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DAF93C341C7; Wed, 20 Jul 2022 18:49:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1658342947; bh=08+Q+ucM7zcCNE6VbGiO3oIo7eApyt+93Izjlu+1QBs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=I5gDqg3ywx4qWRFZdqk2of8EjGxUwRP32WqTD3A6zedGZCkRpKUr07J2YyY2VOvtH odIeq+MaBWBPl29VbjTYBkjxxWSkaMAb8dcNRLwRQ+dTnpJQmDHyLkGLAQkjsiOGM7 mtZWTAJSwC67EdecVJP1U7BILEn1WqlO9TWMl9kk52Qfn3QIkbe8qXyYTR+/qXdgkJ u0x857jsmj1+Eu+tlUUO3oyqTRWRYrXMKq+l5rVsrUDZCp2biBvFkzXWm/MiUFidl/ drl3U8SY0zufJAPUvVbgQluKsTWrDmSFbmNhGSZTP4W4QV0QcJLz8LNCsve0S5mmOM 2vgsJ5MzLryYw== Date: Wed, 20 Jul 2022 19:48:59 +0100 From: Will Deacon To: Sean Christopherson Cc: kvmarm@lists.cs.columbia.edu, Ard Biesheuvel , Alexandru Elisei , Andy Lutomirski , Catalin Marinas , James Morse , Chao Peng , Quentin Perret , Suzuki K Poulose , Michael Roth , Mark Rutland , Fuad Tabba , Oliver Upton , Marc Zyngier , kernel-team@android.com, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 00/24] KVM: arm64: Introduce pKVM shadow state at EL2 Message-ID: <20220720184859.GD16603@willie-the-truck> References: <20220630135747.26983-1-will@kernel.org> <20220708162359.GA6286@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220720_114911_076099_CC27F6E3 X-CRM114-Status: GOOD ( 37.90 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 Hi Sean, On Tue, Jul 19, 2022 at 04:11:32PM +0000, Sean Christopherson wrote: > Apologies for the slow reply. No problem; you've provided a tonne of insightful feedback here, so it was worth the wait. Thanks! > On Fri, Jul 08, 2022, Will Deacon wrote: > > but I wanted to inherit the broader cc list so you were aware of this > > break-away series. Sadly, I don't think beefing up the commit messages would > > get us to a point where somebody unfamiliar with the EL2 code already could > > give a constructive review, but we can try to expand them a bit if you > > genuinely think it would help. > > I'm not looking at it just from a review point, but also from a future readers > perspective. E.g. someone that looks at this changelog in isolation is going to > have no idea what a "shadow VM" is: > > KVM: arm64: Introduce pKVM shadow VM state at EL2 > > Introduce a table of shadow VM structures at EL2 and provide hypercalls > to the host for creating and destroying shadow VMs. > > Obviously there will be some context available in surrounding patches, but if you > avoid the "shadow" terminology and provide a bit more context, then it yields > something like: > > KVM: arm64: Add infrastructure to create and track pKVM instances at EL2 > > Introduce a global table (and lock) to track pKVM instances at EL2, and > provide hypercalls that can be used by the untrusted host to create and > destroy pKVM VMs. pKVM VM/vCPU state is directly accessible only by the > trusted hypervisor (EL2). > > Each pKVM VM is directly associated with an untrusted host KVM instance, > and is referenced by the host using an opaque handle. Future patches will > provide hypercalls to allow the host to initialize/set/get pKVM VM/vCPU > state using the opaque handle. Thanks, that's much better. I'll have to summon up the energy to go through the others as well... > > Perhaps we should s/shadow/hyp/ to make this a little clearer? > > Or maybe just "pkvm"? I think the "hyp" part is useful to distinguish the pkvm code running at EL2 from the pkvm code running at EL1. For example, we have a 'pkvm' member in 'struct kvm_arch' which is used by the _host_ at EL1. So I'd say either "pkvm_hyp" or "hyp" instead of "shadow". The latter is nice and short... > I think that's especially viable if you do away with > kvm_shadow_vcpu_state. As of this series at least, kvm_shadow_vcpu_state is > completely unnecessary. kvm_vcpu.kvm can be used to get at the VM, and thus pKVM > state via container_of(). Then the host_vcpu can be retrieved by using the > vcpu_idx, e.g. > > struct pkvm_vm *pkvm_vm = to_pkvm_vm(pkvm_vcpu->vm); > struct kvm_vcpu *host_vcpu; > > host_vcpu = kvm_get_vcpu(pkvm_vm->host_vm, pkvm_vcpu->vcpu_idx); Using container_of() here is neat; we can definitely go ahead with that change. However, looking at this in more detail with Fuad, removing 'struct kvm_shadow_vcpu_state' entirely isn't going to work: > E.g. I believe you can make the code look like this: > > struct kvm_arch { > ... > > /* > * For an unstructed host VM, pkvm_handle is used to lookup the > * associated pKVM instance. > */ > pvk_handle_t pkvm_handle; > }; > > struct pkvm_vm { > struct kvm kvm; > > /* Backpointer to the host's (untrusted) KVM instance. */ > struct kvm *host_kvm; > > size_t donated_memory_size; > > struct kvm_pgtable pgt; > }; > > static struct kvm *pkvm_get_vm(pkvm_handle_t handle) > { > unsigned int idx = pkvm_handle_to_idx(handle); > > if (unlikely(idx >= KVM_MAX_PVMS)) > return NULL; > > return pkvm_vm_table[idx]; > } > > struct kvm_vcpu *pkvm_vcpu_load(pkvm_handle_t handle, unsigned int vcpu_idx) > { > struct kvm_vpcu *pkvm_vcpu = NULL; > struct kvm *vm; > > hyp_spin_lock(&pkvm_global_lock); > vm = pkvm_get_vm(handle); > if (!vm || atomic_read(&vm->online_vcpus) <= vcpu_idx) > goto unlock; > > pkvm_vcpu = kvm_get_vcpu(vm, vcpu_idx); kvm_get_vcpu() makes use of an xarray to hold the vCPUs pointers and this is really something which we cannot support at EL2 where, amongst other things, we do not have support for RCU. Consequently, we do need to keep our own mapping from the shad^H^H^H^Hhyp vCPU to the host vCPU. We also end up expanding the 'struct kvm_shadow_vcpu_state' structure later to track additional vCPU state in the hypervisor, for example in the mega-series: https://lore.kernel.org/kvmarm/20220519134204.5379-78-will@kernel.org/#Z31arch:arm64:kvm:hyp:include:nvhe:pkvm.h Cheers, Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel