From: Will Deacon <will@kernel.org> Date: 2022-06-30 13:59:36
Hi everyone,
This series has been extracted from the pKVM base support series (aka
"pKVM mega-patch") previously posted here:
https://lore.kernel.org/kvmarm/20220519134204.5379-1-will@kernel.org/
Unlike that more comprehensive series, this one is fairly fundamental
and does not introduce any new ABI commitments, leaving questions
involving the management of guest private memory and the creation of
protected VMs for future work. Instead, this series extends the pKVM EL2
code so that it can dynamically instantiate and manage VM shadow
structures without the host being able to access them directly. These
shadow structures consist of a shadow VM, a set of shadow vCPUs and the
stage-2 page-table and the pages used to hold them are returned to the
host when the VM is destroyed.
The last patch is marked as RFC because, although it plumbs in the
shadow state, it is woefully inefficient and copies to/from the host
state on every vCPU run. Without the last patch, the new structures are
unused but we move considerably closer to isolating guests from the
host.
The series is based on Marc's rework of the flags
(kvm-arm64/burn-the-flags).
Feedback welcome.
Cheers,
Will, Quentin, Fuad and Marc
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Will Deacon <will@kernel.org>
Cc: Alexandru Elisei <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Chao Peng <redacted>
Cc: Quentin Perret <redacted>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Michael Roth <redacted>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Fuad Tabba <redacted>
Cc: Oliver Upton <redacted>
Cc: Marc Zyngier <maz@kernel.org>
Cc: kernel-team@android.com
Cc: kvm@vger.kernel.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-arm-kernel@lists.infradead.org
--->8
Fuad Tabba (3):
KVM: arm64: Add hyp_spinlock_t static initializer
KVM: arm64: Introduce shadow VM state at EL2
KVM: arm64: Instantiate VM shadow data from EL1
Quentin Perret (15):
KVM: arm64: Move hyp refcount manipulation helpers
KVM: arm64: Allow non-coalescable pages in a hyp_pool
KVM: arm64: Add flags to struct hyp_page
KVM: arm64: Back hyp_vmemmap for all of memory
KVM: arm64: Make hyp stage-1 refcnt correct on the whole range
KVM: arm64: Implement do_donate() helper for donating memory
KVM: arm64: Prevent the donation of no-map pages
KVM: arm64: Add helpers to pin memory shared with hyp
KVM: arm64: Add pcpu fixmap infrastructure at EL2
KVM: arm64: Add generic hyp_memcache helpers
KVM: arm64: Instantiate guest stage-2 page-tables at EL2
KVM: arm64: Return guest memory from EL2 via dedicated teardown
memcache
KVM: arm64: Unmap kvm_arm_hyp_percpu_base from the host
KVM: arm64: Explicitly map kvm_vgic_global_state at EL2
KVM: arm64: Don't map host sections in pkvm
Will Deacon (6):
KVM: arm64: Unify identifiers used to distinguish host and hypervisor
KVM: arm64: Include asm/kvm_mmu.h in nvhe/mem_protect.h
KVM: arm64: Initialise hyp symbols regardless of pKVM
KVM: arm64: Provide I-cache invalidation by VA at EL2
KVM: arm64: Maintain a copy of 'kvm_arm_vmid_bits' at EL2
KVM: arm64: Use the shadow vCPU structure in handle___kvm_vcpu_run()
arch/arm64/include/asm/kvm_asm.h | 6 +-
arch/arm64/include/asm/kvm_host.h | 65 +++
arch/arm64/include/asm/kvm_hyp.h | 3 +
arch/arm64/include/asm/kvm_pgtable.h | 8 +
arch/arm64/include/asm/kvm_pkvm.h | 38 ++
arch/arm64/kernel/image-vars.h | 15 -
arch/arm64/kvm/arm.c | 40 +-
arch/arm64/kvm/hyp/hyp-constants.c | 3 +
arch/arm64/kvm/hyp/include/nvhe/gfp.h | 6 +-
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 19 +-
arch/arm64/kvm/hyp/include/nvhe/memory.h | 26 +-
arch/arm64/kvm/hyp/include/nvhe/mm.h | 18 +-
arch/arm64/kvm/hyp/include/nvhe/pkvm.h | 70 +++
arch/arm64/kvm/hyp/include/nvhe/spinlock.h | 10 +-
arch/arm64/kvm/hyp/nvhe/cache.S | 11 +
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 105 +++-
arch/arm64/kvm/hyp/nvhe/hyp-smp.c | 2 +
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 456 +++++++++++++++++-
arch/arm64/kvm/hyp/nvhe/mm.c | 136 +++++-
arch/arm64/kvm/hyp/nvhe/page_alloc.c | 42 +-
arch/arm64/kvm/hyp/nvhe/pkvm.c | 438 +++++++++++++++++
arch/arm64/kvm/hyp/nvhe/setup.c | 96 ++--
arch/arm64/kvm/hyp/pgtable.c | 9 +
arch/arm64/kvm/mmu.c | 26 +
arch/arm64/kvm/pkvm.c | 121 ++++-
25 files changed, 1625 insertions(+), 144 deletions(-)
create mode 100644 arch/arm64/kvm/hyp/include/nvhe/pkvm.h
--
2.37.0.rc0.161.g10f37bed90-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2022-06-30 13:59:26
From: Quentin Perret <redacted>
We will soon need to manipulate struct hyp_page refcounts from outside
page_alloc.c, so move the helpers to a header file.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/memory.h | 18 ++++++++++++++++++
arch/arm64/kvm/hyp/nvhe/page_alloc.c | 19 -------------------
2 files changed, 18 insertions(+), 19 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 13:59:43
From: Quentin Perret <redacted>
Add a 'flags' field to struct hyp_page, and reduce the size of the order
field to u8 to avoid growing the struct size.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/gfp.h | 6 +++---
arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 ++-
arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 +++++++-------
3 files changed, 12 insertions(+), 11 deletions(-)
@@ -51,7 +51,7 @@ static struct hyp_page *__find_buddy_nocheck(struct hyp_pool *pool,/* Find a buddy page currently available for allocation */staticstructhyp_page*__find_buddy_avail(structhyp_pool*pool,structhyp_page*p,-unsignedshortorder)+u8order){structhyp_page*buddy=__find_buddy_nocheck(pool,p,order);
From: Will Deacon <will@kernel.org> Date: 2022-06-30 13:59:44
From: Quentin Perret <redacted>
All the contiguous pages used to initialize a hyp_pool are considered
coalescable, which means that the hyp page allocator will actively
try to merge them with their buddies on the hyp_put_page() path.
However, using hyp_put_page() on a page that is not part of the inital
memory range given to a hyp_pool() is currently unsupported.
In order to allow dynamically extending hyp pools at run-time, add a
check to __hyp_attach_page() to allow inserting 'external' pages into
the free-list of order 0. This will be necessary to allow lazy
donation of pages from the host to the hypervisor when allocating guest
stage-2 page-table pages at EL2.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/nvhe/page_alloc.c | 5 +++++
1 file changed, 5 insertions(+)
@@ -116,6 +120,7 @@ static void __hyp_attach_page(struct hyp_pool *pool,p=min(p,buddy);}+insert:/* Mark the new head, and insert it */p->order=order;page_add_to_list(p,&pool->free_area[order]);
--
2.37.0.rc0.161.g10f37bed90-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:00:25
From: Quentin Perret <redacted>
The EL2 vmemmap in nVHE Protected mode is currently very sparse: only
memory pages owned by the hypervisor itself have a matching struct
hyp_page. But since the size of these structs has been reduced
significantly, it appears that we can afford backing the vmemmap for all
of memory.
This will simplify a lot memory tracking as the hypervisor will have a
place to store metadata (e.g. refcounts) that wouldn't otherwise fit in
the 4 SW bits we have in the host stage-2 page-table for instance.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/kvm_pkvm.h | 26 +++++++++++++++++++++++
arch/arm64/kvm/hyp/include/nvhe/mm.h | 14 +------------
arch/arm64/kvm/hyp/nvhe/mm.c | 31 ++++++++++++++++++++++++----
arch/arm64/kvm/hyp/nvhe/page_alloc.c | 4 +---
arch/arm64/kvm/hyp/nvhe/setup.c | 7 +++----
arch/arm64/kvm/pkvm.c | 18 ++--------------
6 files changed, 60 insertions(+), 40 deletions(-)
@@ -235,10 +235,8 @@ int hyp_pool_init(struct hyp_pool *pool, u64 pfn, unsigned int nr_pages,/* Init the vmemmap portion */p=hyp_phys_to_page(phys);-for(i=0;i<nr_pages;i++){-p[i].order=0;+for(i=0;i<nr_pages;i++)hyp_set_page_refcounted(&p[i]);-}/* Attach the unused pages to the buddy tree */for(i=reserved_pages;i<nr_pages;i++)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:01:13
From: Quentin Perret <redacted>
We currently fixup the hypervisor stage-1 refcount only for specific
portions of the hyp stage-1 VA space. In order to allow unmapping pages
outside of these ranges, let's fixup the refcount for the entire hyp VA
space.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/nvhe/setup.c | 62 +++++++++++++++++++++++----------
1 file changed, 43 insertions(+), 19 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:02:22
The 'pkvm_component_id' enum type provides constants to refer to the
host and the hypervisor, yet this information is duplicated by the
'pkvm_hyp_id' constant.
Remove the definition of 'pkvm_hyp_id' and move the 'pkvm_component_id'
type definition to 'mem_protect.h' so that it can be used outside of
the memory protection code.
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 6 +++++-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 8 --------
arch/arm64/kvm/hyp/nvhe/setup.c | 2 +-
3 files changed, 6 insertions(+), 10 deletions(-)
@@ -51,7 +51,11 @@ struct host_kvm {};externstructhost_kvmhost_kvm;-externconstu8pkvm_hyp_id;+/* This corresponds to page-table locking order */+enumpkvm_component_id{+PKVM_ID_HOST,+PKVM_ID_HYP,+};int__pkvm_prot_finalize(void);int__pkvm_host_share_hyp(u64pfn);
@@ -384,12 +382,6 @@ void handle_host_mem_abort(struct kvm_cpu_context *host_ctxt)BUG_ON(ret&&ret!=-EAGAIN);}-/* This corresponds to locking order */-enumpkvm_component_id{-PKVM_ID_HOST,-PKVM_ID_HYP,-};-structpkvm_mem_transition{u64nr_pages;
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:03:30
From: Quentin Perret <redacted>
Transferring ownership information of a memory region from one component
to another can be achieved using a "donate" operation, which results
in the previous owner losing access to the underlying pages entirely.
Implement a do_donate() helper, along the same lines as do_{un,}share,
and provide this functionality for the host-{to,from}-hyp cases as this
will later be used to donate/reclaim memory pages to store VM metadata
at EL2.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 2 +
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 239 ++++++++++++++++++
2 files changed, 241 insertions(+)
@@ -395,6 +395,9 @@ struct pkvm_mem_transition {/* Address in the completer's address space */u64completer_addr;}host;+struct{+u64completer_addr;+}hyp;};}initiator;
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:04:20
From: Quentin Perret <redacted>
Memory regions marked as no-map in DT routinely include TrustZone
carevouts and such. Although donating such pages to the hypervisor may
not breach confidentiality, it may be used to corrupt its state in
uncontrollable ways. To prevent this, let's block host-initiated memory
transitions targeting no-map pages altogether in nVHE protected mode as
there should be no valid reason to do this currently.
Thankfully, the pKVM EL2 hypervisor has a full copy of the host's list
of memblock regions, hence allowing to check for the presence of the
MEMBLOCK_NOMAP flag on any given region at EL2 easily.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:05:14
From: Quentin Perret <redacted>
Add helpers allowing the hypervisor to check whether a range of pages
are currently shared by the host, and 'pin' them if so by blocking host
unshare operations until the memory has been unpinned. This will allow
the hypervisor to take references on host-provided data-structures
(struct kvm and such) and be guaranteed these pages will remain in a
stable state until it decides to release them, e.g. during guest
teardown.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 3 ++
arch/arm64/kvm/hyp/include/nvhe/memory.h | 7 ++-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 48 +++++++++++++++++++
3 files changed, 57 insertions(+), 1 deletion(-)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:05:29
nvhe/mem_protect.h refers to __load_stage2() in the definition of
__load_host_stage2() but doesn't include the relevant header.
Include asm/kvm_mmu.h in nvhe/mem_protect.h so that users of the latter
don't have to do this themselves.
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 1 +
1 file changed, 1 insertion(+)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:05:35
From: Fuad Tabba <redacted>
Having a static initializer for hyp_spinlock_t simplifies its
use when there isn't an initializing function.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/spinlock.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -9,6 +9,9 @@#include<linux/memblock.h>#include<asm/kvm_pgtable.h>+/* Maximum number of protected VMs that can be created. */+#define KVM_MAX_PVMS 255+#define HYP_MEMBLOCK_REGIONS 128externstructmemblock_regionkvm_nvhe_sym(hyp_memory)[];
@@ -40,6 +43,11 @@ static inline unsigned long hyp_vmemmap_pages(size_t vmemmap_entry_size)returnres>>PAGE_SHIFT;}+staticinlineunsignedlonghyp_shadow_table_pages(void)+{+returnPAGE_ALIGN(KVM_MAX_PVMS*sizeof(void*))>>PAGE_SHIFT;+}+staticinlineunsignedlong__hyp_pgtable_max_pages(unsignedlongnr_pages){unsignedlongtotal=0,i;
@@ -0,0 +1,60 @@+/* SPDX-License-Identifier: GPL-2.0-only */+/*+*Copyright(C)2021GoogleLLC+*Author:FuadTabba<tabba@google.com>+*/++#ifndef __ARM64_KVM_NVHE_PKVM_H__+#define __ARM64_KVM_NVHE_PKVM_H__++#include<asm/kvm_pkvm.h>++/*+*Holdstherelevantdataformaintainingthevcpustatecompletelyathyp.+*/+structkvm_shadow_vcpu_state{+/* The data for the shadow vcpu. */+structkvm_vcpushadow_vcpu;++/* A pointer to the host's vcpu. */+structkvm_vcpu*host_vcpu;++/* A pointer to the shadow vm. */+structkvm_shadow_vm*shadow_vm;+};++/*+*Holdstherelevantdataforrunningaprotectedvm.+*/+structkvm_shadow_vm{+/* The data for the shadow kvm. */+structkvmkvm;++/* The host's kvm structure. */+structkvm*host_kvm;++/* The total size of the donated shadow area. */+size_tshadow_area_size;++structkvm_pgtablepgt;++/* Array of the shadow state per vcpu. */+structkvm_shadow_vcpu_stateshadow_vcpu_states[0];+};++staticinlinestructkvm_shadow_vcpu_state*get_shadow_state(structkvm_vcpu*shadow_vcpu)+{+returncontainer_of(shadow_vcpu,structkvm_shadow_vcpu_state,shadow_vcpu);+}++staticinlinestructkvm_shadow_vm*get_shadow_vm(structkvm_vcpu*shadow_vcpu)+{+returnget_shadow_state(shadow_vcpu)->shadow_vm;+}++voidhyp_shadow_table_init(void*tbl);+int__pkvm_init_shadow(structkvm*kvm,unsignedlongshadow_hva,+size_tshadow_size,unsignedlongpgd_hva);+int__pkvm_teardown_shadow(unsignedintshadow_handle);++#endif /* __ARM64_KVM_NVHE_PKVM_H__ */
@@ -183,3 +186,398 @@ void __pkvm_vcpu_init_traps(struct kvm_vcpu *vcpu)pvm_init_traps_aa64mmfr0(vcpu);pvm_init_traps_aa64mmfr1(vcpu);}++/*+*Starttheshadowtablehandleattheoffsetdefinedinsteadofat0.+*Mainlyforsanitycheckinganddebugging.+*/+#define HANDLE_OFFSET 0x1000++staticunsignedintshadow_handle_to_idx(unsignedintshadow_handle)+{+returnshadow_handle-HANDLE_OFFSET;+}++staticunsignedintidx_to_shadow_handle(unsignedintidx)+{+returnidx+HANDLE_OFFSET;+}++/*+*Spinlockforprotectingtheshadowtablerelatedstate.+*Protectswritestoshadow_table,nr_shadow_entries,andnext_shadow_alloc,+*aswellasreadsandwritestolast_shadow_vcpu_lookup.+*/+staticDEFINE_HYP_SPINLOCK(shadow_lock);++/*+*ThetableofshadowentriesforprotectedVMsinhyp.+*Allocatedathypinitializationandsetup.+*/+staticstructkvm_shadow_vm**shadow_table;++/* Current number of vms in the shadow table. */+staticunsignedintnr_shadow_entries;++/* The next entry index to try to allocate from. */+staticunsignedintnext_shadow_alloc;++voidhyp_shadow_table_init(void*tbl)+{+WARN_ON(shadow_table);+shadow_table=tbl;+}++/*+*Returntheshadowvmcorrespondingtothehandle.+*/+staticstructkvm_shadow_vm*find_shadow_by_handle(unsignedintshadow_handle)+{+unsignedintshadow_idx=shadow_handle_to_idx(shadow_handle);++if(unlikely(shadow_idx>=KVM_MAX_PVMS))+returnNULL;++returnshadow_table[shadow_idx];+}++staticvoidunpin_host_vcpus(structkvm_shadow_vcpu_state*shadow_vcpu_states,+unsignedintnr_vcpus)+{+inti;++for(i=0;i<nr_vcpus;i++){+structkvm_vcpu*host_vcpu=shadow_vcpu_states[i].host_vcpu;+hyp_unpin_shared_mem(host_vcpu,host_vcpu+1);+}+}++staticintset_host_vcpus(structkvm_shadow_vcpu_state*shadow_vcpu_states,+unsignedintnr_vcpus,+structkvm_vcpu**vcpu_array,+size_tvcpu_array_size)+{+inti;++if(vcpu_array_size<sizeof(*vcpu_array)*nr_vcpus)+return-EINVAL;++for(i=0;i<nr_vcpus;i++){+structkvm_vcpu*host_vcpu=kern_hyp_va(vcpu_array[i]);++if(hyp_pin_shared_mem(host_vcpu,host_vcpu+1)){+unpin_host_vcpus(shadow_vcpu_states,i);+return-EBUSY;+}++shadow_vcpu_states[i].host_vcpu=host_vcpu;+}++return0;+}++staticintinit_shadow_structs(structkvm*kvm,structkvm_shadow_vm*vm,+structkvm_vcpu**vcpu_array,+unsignedintnr_vcpus)+{+inti;++vm->host_kvm=kvm;+vm->kvm.created_vcpus=nr_vcpus;+vm->kvm.arch.vtcr=host_kvm.arch.vtcr;++for(i=0;i<nr_vcpus;i++){+structkvm_shadow_vcpu_state*shadow_vcpu_state=&vm->shadow_vcpu_states[i];+structkvm_vcpu*shadow_vcpu=&shadow_vcpu_state->shadow_vcpu;+structkvm_vcpu*host_vcpu=shadow_vcpu_state->host_vcpu;++shadow_vcpu_state->shadow_vm=vm;++shadow_vcpu->kvm=&vm->kvm;+shadow_vcpu->vcpu_id=READ_ONCE(host_vcpu->vcpu_id);+shadow_vcpu->vcpu_idx=i;++shadow_vcpu->arch.hw_mmu=&vm->kvm.arch.mmu;+}++return0;+}++staticbool__exists_shadow(structkvm*host_kvm)+{+inti;+unsignedintnr_checked=0;++for(i=0;i<KVM_MAX_PVMS&&nr_checked<nr_shadow_entries;i++){+if(!shadow_table[i])+continue;++if(unlikely(shadow_table[i]->host_kvm==host_kvm))+returntrue;++nr_checked++;+}++returnfalse;+}++/*+*Allocateashadowtableentryandinsertapointertotheshadowvm.+*+*ReturnauniquehandletotheprotectedVMonsuccess,+*negativeerrorcodeonfailure.+*/+staticunsignedintinsert_shadow_table(structkvm*kvm,+structkvm_shadow_vm*vm,+size_tshadow_size)+{+structkvm_s2_mmu*mmu=&vm->kvm.arch.mmu;+unsignedintshadow_handle;+unsignedintvmid;++hyp_assert_lock_held(&shadow_lock);++if(unlikely(nr_shadow_entries>=KVM_MAX_PVMS))+return-ENOMEM;++/*+*Initializingprotectedstatemighthavefailed,yetamalicioushost+*couldtriggerthisfunction.Thus,ensurethatshadow_tableexists.+*/+if(unlikely(!shadow_table))+return-EINVAL;++/* Check that a shadow hasn't been created before for this host KVM. */+if(unlikely(__exists_shadow(kvm)))+return-EEXIST;++/* Find the next free entry in the shadow table. */+while(shadow_table[next_shadow_alloc])+next_shadow_alloc=(next_shadow_alloc+1)%KVM_MAX_PVMS;+shadow_handle=idx_to_shadow_handle(next_shadow_alloc);++vm->kvm.arch.pkvm.shadow_handle=shadow_handle;+vm->shadow_area_size=shadow_size;++/* VMID 0 is reserved for the host */+vmid=next_shadow_alloc+1;+if(vmid>0xff)+return-ENOMEM;++atomic64_set(&mmu->vmid.id,vmid);+mmu->arch=&vm->kvm.arch;+mmu->pgt=&vm->pgt;++shadow_table[next_shadow_alloc]=vm;+next_shadow_alloc=(next_shadow_alloc+1)%KVM_MAX_PVMS;+nr_shadow_entries++;++returnshadow_handle;+}++/*+*Deallocateandremovetheshadowtableentrycorrespondingtothehandle.+*/+staticvoidremove_shadow_table(unsignedintshadow_handle)+{+hyp_assert_lock_held(&shadow_lock);+shadow_table[shadow_handle_to_idx(shadow_handle)]=NULL;+nr_shadow_entries--;+}++staticsize_tpkvm_get_shadow_size(unsignedintnr_vcpus)+{+/* Shadow space for the vm struct and all of its vcpu states. */+returnsizeof(structkvm_shadow_vm)++sizeof(structkvm_shadow_vcpu_state)*nr_vcpus;+}++/*+*Checkwhetherthesizeoftheareadonatedbythehostissufficientfor+*theshadowstructuresrequiredfornr_vcpusaswellastheshadowvm.+*/+staticintcheck_shadow_size(unsignedintnr_vcpus,size_tshadow_size)+{+if(nr_vcpus<1||nr_vcpus>KVM_MAX_VCPUS)+return-EINVAL;++/*+*Shadowsizeisroundedupwhenallocatedanddonatedbythehost,+*soit'slikelytobelargerthanthesumofthestructsizes.+*/+if(shadow_size<pkvm_get_shadow_size(nr_vcpus))+return-ENOMEM;++return0;+}++staticvoid*map_donated_memory_noclear(unsignedlonghost_va,size_tsize)+{+void*va=(void*)kern_hyp_va(host_va);++if(!PAGE_ALIGNED(va)||!PAGE_ALIGNED(size))+returnNULL;++if(__pkvm_host_donate_hyp(hyp_virt_to_pfn(va),size>>PAGE_SHIFT))+returnNULL;++returnva;+}++staticvoid*map_donated_memory(unsignedlonghost_va,size_tsize)+{+void*va=map_donated_memory_noclear(host_va,size);++if(va)+memset(va,0,size);++returnva;+}++staticvoid__unmap_donated_memory(void*va,size_tsize)+{+WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(va),size>>PAGE_SHIFT));+}++staticvoidunmap_donated_memory(void*va,size_tsize)+{+if(!va)+return;++memset(va,0,size);+__unmap_donated_memory(va,size);+}++staticvoidunmap_donated_memory_noclear(void*va,size_tsize)+{+if(!va)+return;++__unmap_donated_memory(va,size);+}++/*+*InitializetheshadowcopyoftheprotectedVMstateusingthememory+*donatedbythehost.+*+*Unmapsthedonatedmemoryfromthehostatstage2.+*+*kvm:Apointertothehost'sstructkvm(hostva).+*shadow_hva:Thehostvaoftheareabeingdonatedfortheshadowstate.+*Mustbepagealigned.+*shadow_size:Thesizeoftheareabeingdonatedfortheshadowstate.+*Mustbeamultipleofthepagesize.+*pgd_hva:Thehostvaoftheareabeingdonatedforthestage-2PGDfor+*theVM.Mustbepagealigned.ItssizeisimpliedbytheVM's+*VTCR.+*Note:AnarraytothehostKVMVCPUs(hostVA)ispassedviathepgd,asto+*nottobedependentonhowtheVCPU'sarelayedoutinstructkvm.+*+*ReturnauniquehandletotheprotectedVMonsuccess,+*negativeerrorcodeonfailure.+*/+int__pkvm_init_shadow(structkvm*kvm,unsignedlongshadow_hva,+size_tshadow_size,unsignedlongpgd_hva)+{+structkvm_shadow_vm*vm=NULL;+unsignedintnr_vcpus;+size_tpgd_size=0;+void*pgd=NULL;+intret;++kvm=kern_hyp_va(kvm);+ret=hyp_pin_shared_mem(kvm,kvm+1);+if(ret)+returnret;++nr_vcpus=READ_ONCE(kvm->created_vcpus);+ret=check_shadow_size(nr_vcpus,shadow_size);+if(ret)+gotoerr_unpin_kvm;++ret=-ENOMEM;++vm=map_donated_memory(shadow_hva,shadow_size);+if(!vm)+gotoerr_remove_mappings;++pgd_size=kvm_pgtable_stage2_pgd_size(host_kvm.arch.vtcr);+pgd=map_donated_memory_noclear(pgd_hva,pgd_size);+if(!pgd)+gotoerr_remove_mappings;++ret=set_host_vcpus(vm->shadow_vcpu_states,nr_vcpus,pgd,pgd_size);+if(ret)+gotoerr_remove_mappings;++ret=init_shadow_structs(kvm,vm,pgd,nr_vcpus);+if(ret<0)+gotoerr_unpin_host_vcpus;++/* Add the entry to the shadow table. */+hyp_spin_lock(&shadow_lock);+ret=insert_shadow_table(kvm,vm,shadow_size);+if(ret<0)+gotoerr_unlock_unpin_host_vcpus;++ret=kvm_guest_prepare_stage2(vm,pgd);+if(ret)+gotoerr_remove_shadow_table;+hyp_spin_unlock(&shadow_lock);++returnvm->kvm.arch.pkvm.shadow_handle;++err_remove_shadow_table:+remove_shadow_table(vm->kvm.arch.pkvm.shadow_handle);+err_unlock_unpin_host_vcpus:+hyp_spin_unlock(&shadow_lock);+err_unpin_host_vcpus:+unpin_host_vcpus(vm->shadow_vcpu_states,nr_vcpus);+err_remove_mappings:+unmap_donated_memory(vm,shadow_size);+unmap_donated_memory_noclear(pgd,pgd_size);+err_unpin_kvm:+hyp_unpin_shared_mem(kvm,kvm+1);+returnret;+}++int__pkvm_teardown_shadow(unsignedintshadow_handle)+{+structkvm_shadow_vm*vm;+size_tshadow_size;+interr;++/* Lookup then remove entry from the shadow table. */+hyp_spin_lock(&shadow_lock);+vm=find_shadow_by_handle(shadow_handle);+if(!vm){+err=-ENOENT;+gotoerr_unlock;+}++if(WARN_ON(hyp_page_count(vm))){+err=-EBUSY;+gotoerr_unlock;+}++/* Ensure the VMID is clean before it can be reallocated */+__kvm_tlb_flush_vmid(&vm->kvm.arch.mmu);+remove_shadow_table(shadow_handle);+hyp_spin_unlock(&shadow_lock);++/* Reclaim guest pages (including page-table pages) */+reclaim_guest_pages(vm);+unpin_host_vcpus(vm->shadow_vcpu_states,vm->kvm.created_vcpus);++/* Push the metadata pages to the teardown memcache */+shadow_size=vm->shadow_area_size;+hyp_unpin_shared_mem(vm->host_kvm,vm->host_kvm+1);++memset(vm,0,shadow_size);+unmap_donated_memory_noclear(vm,shadow_size);+return0;++err_unlock:+hyp_spin_unlock(&shadow_lock);+returnerr;+}
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:06:45
From: Fuad Tabba <redacted>
Now that EL2 provides calls to create and destroy shadow VM structures,
plumb these into the KVM code at EL1 so that a shadow VM is created on
first vCPU run and destroyed later along with the 'struct kvm' at
teardown time.
Signed-off-by: Fuad Tabba <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/kvm_host.h | 6 ++
arch/arm64/include/asm/kvm_pkvm.h | 4 ++
arch/arm64/kvm/arm.c | 14 ++++
arch/arm64/kvm/hyp/hyp-constants.c | 3 +
arch/arm64/kvm/pkvm.c | 112 +++++++++++++++++++++++++++++
5 files changed, 139 insertions(+)
@@ -94,3 +95,114 @@ void __init kvm_hyp_reserve(void)kvm_info("Reserved %lld MiB at 0x%llx\n",hyp_mem_size>>20,hyp_mem_base);}++/*+*AllocatesanddonatesmemoryforEL2shadowstructs.+*+*Allocatesspacefortheshadowstate,whichincludestheshadowvmaswellas+*theshadowvcpustates.+*+*Storesanopaquehandlerinthekvmstructforfuturereference.+*+*Return0onsuccess,negativeerrorcodeonfailure.+*/+staticint__kvm_shadow_create(structkvm*kvm)+{+structkvm_vcpu*vcpu,**vcpu_array;+unsignedintshadow_handle;+size_tpgd_sz,shadow_sz;+void*pgd,*shadow_addr;+unsignedlongidx;+intret;++if(kvm->created_vcpus<1)+return-EINVAL;++pgd_sz=kvm_pgtable_stage2_pgd_size(kvm->arch.vtcr);+/*+*ThePGDpageswillbereclaimedusingahyp_memcachewhichimplies+*pagegranularity.So,usealloc_pages_exact()togetindividual+*refcounts.+*/+pgd=alloc_pages_exact(pgd_sz,GFP_KERNEL_ACCOUNT);+if(!pgd)+return-ENOMEM;++/* Allocate memory to donate to hyp for the kvm and vcpu state. */+shadow_sz=PAGE_ALIGN(KVM_SHADOW_VM_SIZE++KVM_SHADOW_VCPU_STATE_SIZE*kvm->created_vcpus);+shadow_addr=alloc_pages_exact(shadow_sz,GFP_KERNEL_ACCOUNT);+if(!shadow_addr){+ret=-ENOMEM;+gotofree_pgd;+}++/* Stash the vcpu pointers into the PGD */+BUILD_BUG_ON(KVM_MAX_VCPUS>(PAGE_SIZE/sizeof(u64)));+vcpu_array=pgd;+kvm_for_each_vcpu(idx,vcpu,kvm){+/* Indexing of the vcpus to be sequential starting at 0. */+if(WARN_ON(vcpu->vcpu_idx!=idx)){+ret=-EINVAL;+gotofree_shadow;+}++vcpu_array[idx]=vcpu;+}++/* Donate the shadow memory to hyp and let hyp initialize it. */+ret=kvm_call_hyp_nvhe(__pkvm_init_shadow,kvm,shadow_addr,shadow_sz,+pgd);+if(ret<0)+gotofree_shadow;++shadow_handle=ret;++/* Store the shadow handle given by hyp for future call reference. */+kvm->arch.pkvm.shadow_handle=shadow_handle;+kvm->arch.pkvm.hyp_donations.pgd=pgd;+kvm->arch.pkvm.hyp_donations.shadow=shadow_addr;+return0;++free_shadow:+free_pages_exact(shadow_addr,shadow_sz);+free_pgd:+free_pages_exact(pgd,pgd_sz);+returnret;+}++intkvm_shadow_create(structkvm*kvm)+{+intret=0;++mutex_lock(&kvm->arch.pkvm.shadow_lock);+if(!kvm->arch.pkvm.shadow_handle)+ret=__kvm_shadow_create(kvm);+mutex_unlock(&kvm->arch.pkvm.shadow_lock);++returnret;+}++voidkvm_shadow_destroy(structkvm*kvm)+{+size_tpgd_sz,shadow_sz;++if(kvm->arch.pkvm.shadow_handle)+WARN_ON(kvm_call_hyp_nvhe(__pkvm_teardown_shadow,+kvm->arch.pkvm.shadow_handle));++kvm->arch.pkvm.shadow_handle=0;++shadow_sz=PAGE_ALIGN(KVM_SHADOW_VM_SIZE++KVM_SHADOW_VCPU_STATE_SIZE*kvm->created_vcpus);+pgd_sz=kvm_pgtable_stage2_pgd_size(kvm->arch.vtcr);++free_pages_exact(kvm->arch.pkvm.hyp_donations.shadow,shadow_sz);+free_pages_exact(kvm->arch.pkvm.hyp_donations.pgd,pgd_sz);+}++intkvm_init_pvm(structkvm*kvm)+{+mutex_init(&kvm->arch.pkvm.shadow_lock);+return0;+}
--
2.37.0.rc0.161.g10f37bed90-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:07:15
From: Quentin Perret <redacted>
We will soon need to temporarily map pages into the hypervisor stage-1
in nVHE protected mode. To do this efficiently, let's introduce a
per-cpu fixmap allowing to map a single page without needing to take any
lock or to allocate memory.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 2 +
arch/arm64/kvm/hyp/include/nvhe/mm.h | 4 ++
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 1 -
arch/arm64/kvm/hyp/nvhe/mm.c | 72 +++++++++++++++++++
arch/arm64/kvm/hyp/nvhe/setup.c | 4 ++
5 files changed, 82 insertions(+), 1 deletion(-)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:08:00
The nVHE object at EL2 maintains its own copies of some host variables
so that, when pKVM is enabled, the host cannot directly modify the
hypervisor state. When running in normal nVHE mode, however, these
variables are still mirrored at EL2 but are not initialised.
Initialise the hypervisor symbols from the host copies regardless of
pKVM, ensuring that any reference to this data at EL2 with normal nVHE
will return an sensibly initialised value.
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/arm.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:08:36
In preparation for handling cache maintenance of guest pages at EL2,
introduce an EL2 copy of icache_inval_pou() which will later be plumbed
into the stage-2 page-table cache maintenance callbacks.
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/kvm_hyp.h | 1 +
arch/arm64/kernel/image-vars.h | 3 ---
arch/arm64/kvm/arm.c | 1 +
arch/arm64/kvm/hyp/nvhe/cache.S | 11 +++++++++++
arch/arm64/kvm/hyp/nvhe/pkvm.c | 3 +++
5 files changed, 16 insertions(+), 3 deletions(-)
@@ -80,9 +80,6 @@ KVM_NVHE_ALIAS(nvhe_hyp_panic_handler);/* Vectors installed by hyp-init on reset HVC. */KVM_NVHE_ALIAS(__hyp_stub_vectors);-/* Kernel symbol used by icache_is_vpipt(). */-KVM_NVHE_ALIAS(__icache_flags);-/* VMID bits set by the KVM VMID allocator */KVM_NVHE_ALIAS(kvm_arm_vmid_bits);
@@ -12,6 +12,9 @@#include<nvhe/pkvm.h>#include<nvhe/trap_handler.h>+/* Used by icache_is_vpipt(). */+unsignedlong__icache_flags;+/**SettrapregistervaluesbasedonfeaturesinID_AA64PFR0.*/
--
2.37.0.rc0.161.g10f37bed90-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:09:28
From: Quentin Perret <redacted>
The host and hypervisor will need to dynamically exchange memory pages
soon. Indeed, the hypervisor will rely on the host to donate memory
pages it can use to create guest stage-2 page-table and to store
metadata. In order to ease this process, introduce a struct hyp_memcache
which is essentially a linked list of available pages, indexed by
physical addresses.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/kvm_host.h | 57 +++++++++++++++++++
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 2 +
arch/arm64/kvm/hyp/nvhe/mm.c | 33 +++++++++++
arch/arm64/kvm/mmu.c | 26 +++++++++
4 files changed, 118 insertions(+)
@@ -308,3 +308,36 @@ int hyp_create_idmap(u32 hyp_va_bits)return__pkvm_create_mappings(start,end-start,start,PAGE_HYP_EXEC);}++staticvoid*admit_host_page(void*arg)+{+structkvm_hyp_memcache*host_mc=arg;++if(!host_mc->nr_pages)+returnNULL;++/*+*Thehoststillownsthepagesinitsmemcache,soweneedtogo+*throughafullhost-to-hypdonationcycletochangeit.Fortunately,+*__pkvm_host_donate_hyp()takescareofracesforus,soifit+*succeedswe'regoodtogo.+*/+if(__pkvm_host_donate_hyp(hyp_phys_to_pfn(host_mc->head),1))+returnNULL;++returnpop_hyp_memcache(host_mc,hyp_phys_to_virt);+}++/* Refill our local memcache by poping pages from the one provided by the host. */+intrefill_memcache(structkvm_hyp_memcache*mc,unsignedlongmin_pages,+structkvm_hyp_memcache*host_mc)+{+structkvm_hyp_memcachetmp=*host_mc;+intret;++ret=__topup_hyp_memcache(mc,min_pages,admit_host_page,+hyp_virt_to_phys,&tmp);+*host_mc=tmp;++returnret;+}
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:09:52
From: Quentin Perret <redacted>
Extend the shadow initialisation at EL2 so that we instantiate a memory
pool and a full 'struct kvm_s2_mmu' structure for each VM, with a
stage-2 page-table entirely independent from the one managed by the host
at EL1.
For now, the new page-table is unused as there is no way for the host
to map anything into it. Yet.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/pkvm.h | 6 ++
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 127 ++++++++++++++++++++++++-
2 files changed, 130 insertions(+), 3 deletions(-)
@@ -37,6 +40,9 @@ struct kvm_shadow_vm {size_tshadow_area_size;structkvm_pgtablepgt;+structkvm_pgtable_mm_opsmm_ops;+structhyp_poolpool;+hyp_spinlock_tlock;/* Array of the shadow state per vcpu. */structkvm_shadow_vcpu_stateshadow_vcpu_states[0];
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:10:56
From: Quentin Perret <redacted>
Rather than relying on the host to free the shadow VM pages explicitly
on teardown, introduce a dedicated teardown memcache which allows the
host to reclaim guest memory resources without having to keep track of
all of the allocations made by EL2.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/kvm_host.h | 6 +-----
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 2 +-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 17 +++++++++++------
arch/arm64/kvm/hyp/nvhe/pkvm.c | 8 +++++++-
arch/arm64/kvm/pkvm.c | 12 +-----------
5 files changed, 21 insertions(+), 24 deletions(-)
@@ -260,19 +260,24 @@ int kvm_guest_prepare_stage2(struct kvm_shadow_vm *vm, void *pgd)return0;}-voidreclaim_guest_pages(structkvm_shadow_vm*vm)+voidreclaim_guest_pages(structkvm_shadow_vm*vm,structkvm_hyp_memcache*mc){-unsignedlongnr_pages,pfn;--nr_pages=kvm_pgtable_stage2_pgd_size(vm->kvm.arch.vtcr)>>PAGE_SHIFT;-pfn=hyp_virt_to_pfn(vm->pgt.pgd);+void*addr;+/* Dump all pgtable pages in the hyp_pool */guest_lock_component(vm);kvm_pgtable_stage2_destroy(&vm->pgt);vm->kvm.arch.mmu.pgd_phys=0ULL;guest_unlock_component(vm);-WARN_ON(__pkvm_hyp_donate_host(pfn,nr_pages));+/* Drain the hyp_pool into the memcache */+addr=hyp_alloc_pages(&vm->pool,0);+while(addr){+memset(hyp_virt_to_page(addr),0,sizeof(structhyp_page));+push_hyp_memcache(mc,addr,hyp_virt_to_phys);+WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(addr),1));+addr=hyp_alloc_pages(&vm->pool,0);+}}int__pkvm_prot_finalize(void)
@@ -546,8 +546,10 @@ int __pkvm_init_shadow(struct kvm *kvm, unsigned long shadow_hva,int__pkvm_teardown_shadow(unsignedintshadow_handle){+structkvm_hyp_memcache*mc;structkvm_shadow_vm*vm;size_tshadow_size;+void*addr;interr;/* Lookup then remove entry from the shadow table. */
@@ -569,7 +571,8 @@ int __pkvm_teardown_shadow(unsigned int shadow_handle)hyp_spin_unlock(&shadow_lock);/* Reclaim guest pages (including page-table pages) */-reclaim_guest_pages(vm);+mc=&vm->host_kvm->arch.pkvm.teardown_mc;+reclaim_guest_pages(vm,mc);unpin_host_vcpus(vm->shadow_vcpu_states,vm->kvm.created_vcpus);/* Push the metadata pages to the teardown memcache */
@@ -577,6 +580,9 @@ int __pkvm_teardown_shadow(unsigned int shadow_handle)hyp_unpin_shared_mem(vm->host_kvm,vm->host_kvm+1);memset(vm,0,shadow_size);+for(addr=vm;addr<(void*)vm+shadow_size;addr+=PAGE_SIZE)+push_hyp_memcache(mc,addr,hyp_virt_to_phys);+unmap_donated_memory_noclear(vm,shadow_size);return0;
@@ -160,8 +160,6 @@ static int __kvm_shadow_create(struct kvm *kvm)/* Store the shadow handle given by hyp for future call reference. */kvm->arch.pkvm.shadow_handle=shadow_handle;-kvm->arch.pkvm.hyp_donations.pgd=pgd;-kvm->arch.pkvm.hyp_donations.shadow=shadow_addr;return0;free_shadow:
@@ -185,20 +183,12 @@ int kvm_shadow_create(struct kvm *kvm)voidkvm_shadow_destroy(structkvm*kvm){-size_tpgd_sz,shadow_sz;-if(kvm->arch.pkvm.shadow_handle)WARN_ON(kvm_call_hyp_nvhe(__pkvm_teardown_shadow,kvm->arch.pkvm.shadow_handle));kvm->arch.pkvm.shadow_handle=0;--shadow_sz=PAGE_ALIGN(KVM_SHADOW_VM_SIZE+-KVM_SHADOW_VCPU_STATE_SIZE*kvm->created_vcpus);-pgd_sz=kvm_pgtable_stage2_pgd_size(kvm->arch.vtcr);--free_pages_exact(kvm->arch.pkvm.hyp_donations.shadow,shadow_sz);-free_pages_exact(kvm->arch.pkvm.hyp_donations.pgd,pgd_sz);+free_hyp_memcache(&kvm->arch.pkvm.teardown_mc);}intkvm_init_pvm(structkvm*kvm)
--
2.37.0.rc0.161.g10f37bed90-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:11:43
Sharing 'kvm_arm_vmid_bits' between EL1 and EL2 allows the host to
modify the variable arbitrarily, potentially leading to all sorts of
shenanians as this is used to configure the VTTBR register for the
guest stage-2.
In preparation for unmapping host sections entirely from EL2, maintain
a copy of 'kvm_arm_vmid_bits' and initialise it from the host value
while it is still trusted.
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/kvm_hyp.h | 2 ++
arch/arm64/kernel/image-vars.h | 3 ---
arch/arm64/kvm/arm.c | 1 +
arch/arm64/kvm/hyp/nvhe/pkvm.c | 3 +++
4 files changed, 6 insertions(+), 3 deletions(-)
@@ -80,9 +80,6 @@ KVM_NVHE_ALIAS(nvhe_hyp_panic_handler);/* Vectors installed by hyp-init on reset HVC. */KVM_NVHE_ALIAS(__hyp_stub_vectors);-/* VMID bits set by the KVM VMID allocator */-KVM_NVHE_ALIAS(kvm_arm_vmid_bits);-/* Kernel symbols needed for cpus_have_final/const_caps checks. */KVM_NVHE_ALIAS(arm64_const_caps_ready);KVM_NVHE_ALIAS(cpu_hwcap_keys);
@@ -15,6 +15,9 @@/* Used by icache_is_vpipt(). */unsignedlong__icache_flags;+/* Used by kvm_get_vttbr(). */+unsignedintkvm_arm_vmid_bits;+/**SettrapregistervaluesbasedonfeaturesinID_AA64PFR0.*/
--
2.37.0.rc0.161.g10f37bed90-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:12:49
From: Quentin Perret <redacted>
In pKVM mode, we can't trust the host not to mess with the hypervisor
per-cpu offsets, so let's move the array containing them to the nVHE
code.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/kvm_asm.h | 4 ++--
arch/arm64/kernel/image-vars.h | 3 ---
arch/arm64/kvm/arm.c | 9 ++++-----
arch/arm64/kvm/hyp/nvhe/hyp-smp.c | 2 ++
4 files changed, 8 insertions(+), 10 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:13:55
From: Quentin Perret <redacted>
The pkvm hypervisor may need to read the kvm_vgic_global_state variable
at EL2. Make sure to explicitly map it in its stage-1 page-table rather
than relying on mapping all of the host .rodata section.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/nvhe/setup.c | 5 +++++
1 file changed, 5 insertions(+)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:14:59
From: Quentin Perret <redacted>
We no longer need to map the host's .rodata and .bss sections in the
pkvm hypervisor, so let's remove those mappings. This will avoid
creating dependencies at EL2 on host-controlled data-structures.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kernel/image-vars.h | 6 ------
arch/arm64/kvm/hyp/nvhe/setup.c | 14 +++-----------
2 files changed, 3 insertions(+), 17 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2022-06-30 14:15:48
As a stepping stone towards deprivileging the host's access to the
guest's vCPU structures, introduce some naive flush/sync routines to
copy most of the host vCPU into the shadow vCPU on vCPU run and back
again on return to EL1.
This allows us to run using the shadow structure when KVM is initialised
in protected mode.
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/pkvm.h | 4 ++
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 84 +++++++++++++++++++++++++-
arch/arm64/kvm/hyp/nvhe/pkvm.c | 28 +++++++++
3 files changed, 114 insertions(+), 2 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2022-07-06 19:18:41
On Thu, Jun 30, 2022, Will Deacon wrote:
Hi everyone,
This series has been extracted from the pKVM base support series (aka
"pKVM mega-patch") previously posted here:
https://lore.kernel.org/kvmarm/20220519134204.5379-1-will@kernel.org/
Unlike that more comprehensive series, this one is fairly fundamental
and does not introduce any new ABI commitments, leaving questions
involving the management of guest private memory and the creation of
protected VMs for future work. Instead, this series extends the pKVM EL2
code so that it can dynamically instantiate and manage VM shadow
structures without the host being able to access them directly. These
shadow structures consist of a shadow VM, a set of shadow vCPUs and the
stage-2 page-table and the pages used to hold them are returned to the
host when the VM is destroyed.
The last patch is marked as RFC because, although it plumbs in the
shadow state, it is woefully inefficient and copies to/from the host
state on every vCPU run. Without the last patch, the new structures are
unused but we move considerably closer to isolating guests from the
host.
The lack of documentation and the rather terse changelogs make this really hard
to review for folks that aren't intimately familiar with pKVM. I have a decent
idea of the end goal of "shadowing", but that's mostly because of my involvement in
similar x86 projects. Nothing in the changelogs ever explains _why_ pKVM uses
shadows.
I put "shadowing" in quotes because if the unstrusted host is aware that the VM
and vCPU it is manipulating aren't the "real" VMs/vCPUs, and there is an explicit API
between the untrusted host and pKVM for creating/destroying VMs/vCPUs, then I would
argue that it's not truly shadowing, especially if pKVM uses data/values verbatim
and only verifies correctness/safety. It's definitely a nit, but for future readers
I think overloading "shadowing" could be confusing.
And beyond the basics, IMO pKVM needs a more formal definition of exactly what
guest state is protected/hidden from the untrusted host. Peeking at the mega series,
there are a huge pile of patches that result in "gradual reduction of EL2 trust in
host data", but I couldn't any documentation that defines what that end result is.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2022-07-08 16:32:12
Hi Sean,
Thanks for having a look.
On Wed, Jul 06, 2022 at 07:17:29PM +0000, Sean Christopherson wrote:
On Thu, Jun 30, 2022, Will Deacon wrote:
quoted
This series has been extracted from the pKVM base support series (aka
"pKVM mega-patch") previously posted here:
https://lore.kernel.org/kvmarm/20220519134204.5379-1-will@kernel.org/
Unlike that more comprehensive series, this one is fairly fundamental
and does not introduce any new ABI commitments, leaving questions
involving the management of guest private memory and the creation of
protected VMs for future work. Instead, this series extends the pKVM EL2
code so that it can dynamically instantiate and manage VM shadow
structures without the host being able to access them directly. These
shadow structures consist of a shadow VM, a set of shadow vCPUs and the
stage-2 page-table and the pages used to hold them are returned to the
host when the VM is destroyed.
The last patch is marked as RFC because, although it plumbs in the
shadow state, it is woefully inefficient and copies to/from the host
state on every vCPU run. Without the last patch, the new structures are
unused but we move considerably closer to isolating guests from the
host.
The lack of documentation and the rather terse changelogs make this really hard
to review for folks that aren't intimately familiar with pKVM. I have a decent
idea of the end goal of "shadowing", but that's mostly because of my involvement in
similar x86 projects. Nothing in the changelogs ever explains _why_ pKVM uses
shadows.
That's understandable, but thanks for persevering; this series is pretty
down in the murky depths of the arm64 architecture and EL2 code so it
doesn't really map to the KVM code most folks are familiar with. It's fair
to say we're assuming a lot of niche prior knowledge (which is quite common
for arch code in my experience), 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.
On the more positive side, we'll be speaking at KVM forum about what we've
done here, so that will be a great place to discuss it further and then we
can also link back to the recordings in later postings of the mega-series.
I put "shadowing" in quotes because if the unstrusted host is aware that the VM
and vCPU it is manipulating aren't the "real" VMs/vCPUs, and there is an explicit API
between the untrusted host and pKVM for creating/destroying VMs/vCPUs, then I would
argue that it's not truly shadowing, especially if pKVM uses data/values verbatim
and only verifies correctness/safety. It's definitely a nit, but for future readers
I think overloading "shadowing" could be confusing.
Ah, this is really interesting and nicely puts the ball back in my court as
I'm not well versed with x86's use of "shadowing". We should probably use
another name (ideas?), but our "shadow" is very much explicit -- rather than
the host using its 'struct kvm's and 'struct kvm_vcpu's directly, it instead
passes those data structures to the hypervisor which allocates its own
structures (in some cases reusing the host definitions directly, e.g.
'struct kvm_vcpu') and returning a handle to the host as a reference. The
host can then issue hypercalls with this handle to load/put vCPUs of that
VM, run them once they are loaded and synchronise aspects of the vCPU state
between the host and the hypervisor copies for things like emulation traps
or interrupt injection. The main thing is that the pages containing the
hypervisor structures are not accessible by the host until the corresponding
VM is destroyed.
The advantage of doing it this way is that we don't need to change very
much of the host KVM code at all, and we can even reuse some of it directly
in the hypervisor (e.g. inline functions and macros).
Perhaps we should s/shadow/hyp/ to make this a little clearer?
And beyond the basics, IMO pKVM needs a more formal definition of exactly what
guest state is protected/hidden from the untrusted host. Peeking at the mega series,
there are a huge pile of patches that result in "gradual reduction of EL2 trust in
host data", but I couldn't any documentation that defines what that end result is.
That's fair; I'll work to extend the documentation in the next iteration of
the mega series to cover this in more detail. Roughly speaking, the end
result is that the vCPU register and memory state is inaccessible to the
host except in cases where the guest has done something to expose it such as
MMIO or a memory sharing hypercall. Given the complexity of the register
state (GPRs, floating point, SIMD, debug, etc) the mega-series elavates
portions of the state from the host to the hypervisor as separate patches
to structure things a bit better (that's where the gradual reduction comes
in).
Does that help at all?
Cheers,
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Vincent Donnefort <hidden> Date: 2022-07-18 10:55:59
On Thu, Jun 30, 2022 at 02:57:26PM +0100, Will Deacon wrote:
quoted hunk
From: Quentin Perret <redacted>
Add a 'flags' field to struct hyp_page, and reduce the size of the order
field to u8 to avoid growing the struct size.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/gfp.h | 6 +++---
arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 ++-
arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 +++++++-------
3 files changed, 12 insertions(+), 11 deletions(-)
BUG_ON in hyp_page_ref_inc() might now need to test for 0xff/HYP_NO_ORDER
instead of USHRT_MAX.
[...]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Vincent Donnefort <hidden> Date: 2022-07-18 10:58:24
On Mon, Jul 18, 2022 at 11:54:24AM +0100, Vincent Donnefort wrote:
On Thu, Jun 30, 2022 at 02:57:26PM +0100, Will Deacon wrote:
quoted
From: Quentin Perret <redacted>
Add a 'flags' field to struct hyp_page, and reduce the size of the order
field to u8 to avoid growing the struct size.
Signed-off-by: Quentin Perret <redacted>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/gfp.h | 6 +++---
arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 ++-
arch/arm64/kvm/hyp/nvhe/page_alloc.c | 14 +++++++-------
3 files changed, 12 insertions(+), 11 deletions(-)
+ * kvm_pgtable_stage2_pgd_size() - Helper to compute size of a stage-2 PGD+ * @vtcr: Content of the VTCR register.+ *+ * Return: the size (in bytes) of the stage-2 PGD+ */+size_t kvm_pgtable_stage2_pgd_size(u64 vtcr);+ /** * __kvm_pgtable_stage2_init() - Initialise a guest stage-2 page-table. * @pgt: Uninitialised page-table structure to initialise.
@@ -9,6 +9,9 @@#include<linux/memblock.h>#include<asm/kvm_pgtable.h>+/* Maximum number of protected VMs that can be created. */+#define KVM_MAX_PVMS 255+#define HYP_MEMBLOCK_REGIONS 128externstructmemblock_regionkvm_nvhe_sym(hyp_memory)[];
@@ -40,6 +43,11 @@ static inline unsigned long hyp_vmemmap_pages(size_t vmemmap_entry_size)returnres>>PAGE_SHIFT;}+staticinlineunsignedlonghyp_shadow_table_pages(void)+{+returnPAGE_ALIGN(KVM_MAX_PVMS*sizeof(void*))>>PAGE_SHIFT;+}+staticinlineunsignedlong__hyp_pgtable_max_pages(unsignedlongnr_pages){unsignedlongtotal=0,i;
@@ -0,0 +1,60 @@+/* SPDX-License-Identifier: GPL-2.0-only */+/*+*Copyright(C)2021GoogleLLC+*Author:FuadTabba<tabba@google.com>+*/++#ifndef __ARM64_KVM_NVHE_PKVM_H__+#define __ARM64_KVM_NVHE_PKVM_H__++#include<asm/kvm_pkvm.h>++/*+*Holdstherelevantdataformaintainingthevcpustatecompletelyathyp.+*/+structkvm_shadow_vcpu_state{+/* The data for the shadow vcpu. */+structkvm_vcpushadow_vcpu;++/* A pointer to the host's vcpu. */+structkvm_vcpu*host_vcpu;++/* A pointer to the shadow vm. */+structkvm_shadow_vm*shadow_vm;
IMHO, those declarations are already self-explanatory. The comments above don't
bring much.
quoted hunk
+};++/*+ * Holds the relevant data for running a protected vm.+ */+struct kvm_shadow_vm {+ /* The data for the shadow kvm. */+ struct kvm kvm;++ /* The host's kvm structure. */+ struct kvm *host_kvm;++ /* The total size of the donated shadow area. */+ size_t shadow_area_size;++ struct kvm_pgtable pgt;++ /* Array of the shadow state per vcpu. */+ struct kvm_shadow_vcpu_state shadow_vcpu_states[0];+};++static inline struct kvm_shadow_vcpu_state *get_shadow_state(struct kvm_vcpu *shadow_vcpu)+{+ return container_of(shadow_vcpu, struct kvm_shadow_vcpu_state, shadow_vcpu);+}++static inline struct kvm_shadow_vm *get_shadow_vm(struct kvm_vcpu *shadow_vcpu)+{+ return get_shadow_state(shadow_vcpu)->shadow_vm;+}++void hyp_shadow_table_init(void *tbl);+int __pkvm_init_shadow(struct kvm *kvm, unsigned long shadow_hva,+ size_t shadow_size, unsigned long pgd_hva);+int __pkvm_teardown_shadow(unsigned int shadow_handle);++#endif /* __ARM64_KVM_NVHE_PKVM_H__ */
@@ -183,3 +186,398 @@ void __pkvm_vcpu_init_traps(struct kvm_vcpu *vcpu) pvm_init_traps_aa64mmfr0(vcpu); pvm_init_traps_aa64mmfr1(vcpu); }++/*+ * Start the shadow table handle at the offset defined instead of at 0.+ * Mainly for sanity checking and debugging.+ */+#define HANDLE_OFFSET 0x1000++static unsigned int shadow_handle_to_idx(unsigned int shadow_handle)+{+ return shadow_handle - HANDLE_OFFSET;+}++static unsigned int idx_to_shadow_handle(unsigned int idx)+{+ return idx + HANDLE_OFFSET;+}++/*+ * Spinlock for protecting the shadow table related state.+ * Protects writes to shadow_table, nr_shadow_entries, and next_shadow_alloc,+ * as well as reads and writes to last_shadow_vcpu_lookup.+ */+static DEFINE_HYP_SPINLOCK(shadow_lock);++/*+ * The table of shadow entries for protected VMs in hyp.+ * Allocated at hyp initialization and setup.+ */+static struct kvm_shadow_vm **shadow_table;++/* Current number of vms in the shadow table. */+static unsigned int nr_shadow_entries;++/* The next entry index to try to allocate from. */+static unsigned int next_shadow_alloc;++void hyp_shadow_table_init(void *tbl)+{+ WARN_ON(shadow_table);+ shadow_table = tbl;+}++/*+ * Return the shadow vm corresponding to the handle.+ */+static struct kvm_shadow_vm *find_shadow_by_handle(unsigned int shadow_handle)+{+ unsigned int shadow_idx = shadow_handle_to_idx(shadow_handle);++ if (unlikely(shadow_idx >= KVM_MAX_PVMS))+ return NULL;++ return shadow_table[shadow_idx];+}++static void unpin_host_vcpus(struct kvm_shadow_vcpu_state *shadow_vcpu_states,+ unsigned int nr_vcpus)+{+ int i;++ for (i = 0; i < nr_vcpus; i++) {+ struct kvm_vcpu *host_vcpu = shadow_vcpu_states[i].host_vcpu;
IIRC, checkpatch likes an empty line after declarations.
quoted hunk
+ hyp_unpin_shared_mem(host_vcpu, host_vcpu + 1);+ }+}++static int set_host_vcpus(struct kvm_shadow_vcpu_state *shadow_vcpu_states,+ unsigned int nr_vcpus,+ struct kvm_vcpu **vcpu_array,+ size_t vcpu_array_size)+{+ int i;++ if (vcpu_array_size < sizeof(*vcpu_array) * nr_vcpus)+ return -EINVAL;++ for (i = 0; i < nr_vcpus; i++) {+ struct kvm_vcpu *host_vcpu = kern_hyp_va(vcpu_array[i]);++ if (hyp_pin_shared_mem(host_vcpu, host_vcpu + 1)) {+ unpin_host_vcpus(shadow_vcpu_states, i);+ return -EBUSY;+ }++ shadow_vcpu_states[i].host_vcpu = host_vcpu;+ }++ return 0;+}++static int init_shadow_structs(struct kvm *kvm, struct kvm_shadow_vm *vm,+ struct kvm_vcpu **vcpu_array,+ unsigned int nr_vcpus)+{+ int i;++ vm->host_kvm = kvm;+ vm->kvm.created_vcpus = nr_vcpus;+ vm->kvm.arch.vtcr = host_kvm.arch.vtcr;++ for (i = 0; i < nr_vcpus; i++) {+ struct kvm_shadow_vcpu_state *shadow_vcpu_state = &vm->shadow_vcpu_states[i];+ struct kvm_vcpu *shadow_vcpu = &shadow_vcpu_state->shadow_vcpu;+ struct kvm_vcpu *host_vcpu = shadow_vcpu_state->host_vcpu;++ shadow_vcpu_state->shadow_vm = vm;++ shadow_vcpu->kvm = &vm->kvm;+ shadow_vcpu->vcpu_id = READ_ONCE(host_vcpu->vcpu_id);+ shadow_vcpu->vcpu_idx = i;++ shadow_vcpu->arch.hw_mmu = &vm->kvm.arch.mmu;
In the end, we don't seem to use much from the struct kvm_cpu. Is it for
convinience that a smaller struct kvm_shadow_cpu hasn't been created, or we do
anticipate a later wider usage?
quoted hunk
+ }++ return 0;+}++static bool __exists_shadow(struct kvm *host_kvm)+{+ int i;+ unsigned int nr_checked = 0;++ for (i = 0; i < KVM_MAX_PVMS && nr_checked < nr_shadow_entries; i++) {+ if (!shadow_table[i])+ continue;++ if (unlikely(shadow_table[i]->host_kvm == host_kvm))+ return true;++ nr_checked++;+ }++ return false;+}++/*+ * Allocate a shadow table entry and insert a pointer to the shadow vm.+ *+ * Return a unique handle to the protected VM on success,+ * negative error code on failure.+ */+static unsigned int insert_shadow_table(struct kvm *kvm,+ struct kvm_shadow_vm *vm,+ size_t shadow_size)+{+ struct kvm_s2_mmu *mmu = &vm->kvm.arch.mmu;+ unsigned int shadow_handle;+ unsigned int vmid;++ hyp_assert_lock_held(&shadow_lock);++ if (unlikely(nr_shadow_entries >= KVM_MAX_PVMS))+ return -ENOMEM;++ /*+ * Initializing protected state might have failed, yet a malicious host+ * could trigger this function. Thus, ensure that shadow_table exists.+ */+ if (unlikely(!shadow_table))+ return -EINVAL;++ /* Check that a shadow hasn't been created before for this host KVM. */+ if (unlikely(__exists_shadow(kvm)))+ return -EEXIST;++ /* Find the next free entry in the shadow table. */+ while (shadow_table[next_shadow_alloc])+ next_shadow_alloc = (next_shadow_alloc + 1) % KVM_MAX_PVMS;
Couldn't it be merged with __exists_shadow which already knows the first free
shadow_table idx?
quoted hunk
+ shadow_handle = idx_to_shadow_handle(next_shadow_alloc);++ vm->kvm.arch.pkvm.shadow_handle = shadow_handle;+ vm->shadow_area_size = shadow_size;++ /* VMID 0 is reserved for the host */+ vmid = next_shadow_alloc + 1;+ if (vmid > 0xff)
Couldn't the 0xff be found with get_vmid_bits() or even from host_kvm.arch.vtcr?
Or does that depends on something completely different?
Also, appologies if this has been discussed already and I missed it, maybe
KVM_MAX_PVMS could be changed for that value - 1. Unless we think that archs
supporting 16 bits would waste way too much memory for that?
From: Marc Zyngier <maz@kernel.org> Date: 2022-07-19 09:43:16
On Mon, 18 Jul 2022 19:40:05 +0100,
Vincent Donnefort [off-list ref] wrote:
[...]
In the end, we don't seem to use much from the struct kvm_cpu. Is it for
convinience that a smaller struct kvm_shadow_cpu hasn't been created, or we do
anticipate a later wider usage?
The alternative would be to repaint the whole of the core KVM/arm64
code to be able to take the new structure in parallel with the
standard one. There is very little to gain from such a move.
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
@@ -212,6 +214,76 @@ int hyp_map_vectors(void)return0;}+void*hyp_fixmap_map(phys_addr_tphys)+{+void*addr=*this_cpu_ptr(&hyp_fixmap_base);+intret=kvm_pgtable_hyp_map(&pkvm_pgtable,(u64)addr,PAGE_SIZE,+phys,PAGE_HYP);+returnret?NULL:addr;+}++inthyp_fixmap_unmap(void)+{+void*addr=*this_cpu_ptr(&hyp_fixmap_base);+intret=kvm_pgtable_hyp_unmap(&pkvm_pgtable,(u64)addr,PAGE_SIZE);++return(ret!=PAGE_SIZE)?-EINVAL:0;+}+
I probably missed something but as the pagetable pages for this mapping are
pined, it seems impossible (currently) for this call to fail. Maybe a WARN_ON
would be more appropriate, especially the callers in the subsequent patches do
not seem to check for this function return value?
[...]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The pfn introduction being removed in a subsequent patch, this is probably
unecessary noise.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -212,6 +214,76 @@ int hyp_map_vectors(void)return0;}+void*hyp_fixmap_map(phys_addr_tphys)+{+void*addr=*this_cpu_ptr(&hyp_fixmap_base);+intret=kvm_pgtable_hyp_map(&pkvm_pgtable,(u64)addr,PAGE_SIZE,+phys,PAGE_HYP);+returnret?NULL:addr;+}++inthyp_fixmap_unmap(void)+{+void*addr=*this_cpu_ptr(&hyp_fixmap_base);+intret=kvm_pgtable_hyp_unmap(&pkvm_pgtable,(u64)addr,PAGE_SIZE);++return(ret!=PAGE_SIZE)?-EINVAL:0;+}+
I probably missed something but as the pagetable pages for this mapping are
pined, it seems impossible (currently) for this call to fail. Maybe a WARN_ON
would be more appropriate, especially the callers in the subsequent patches do
not seem to check for this function return value?
Right, I think that wouldn't hurt. And while looking at this, I
actually think we could get rid of these calls to the map/unmap
functions entirely by keeping the pointers to the reserved PTEs
directly in addition to the VA to which they correspond in the percpu
table. That way we could manipulate the PTEs directly and avoid
unnecessary pgtable walks. Bits [63:1] can probably remain untouched,
and {un}mapping is then only a matter of flipping bit 0 in the PTE
(and TLBI on the unmap path). I'll have a go at it.
Cheers,
Quentin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -212,6 +214,76 @@ int hyp_map_vectors(void)return0;}+void*hyp_fixmap_map(phys_addr_tphys)+{+void*addr=*this_cpu_ptr(&hyp_fixmap_base);+intret=kvm_pgtable_hyp_map(&pkvm_pgtable,(u64)addr,PAGE_SIZE,+phys,PAGE_HYP);+returnret?NULL:addr;+}++inthyp_fixmap_unmap(void)+{+void*addr=*this_cpu_ptr(&hyp_fixmap_base);+intret=kvm_pgtable_hyp_unmap(&pkvm_pgtable,(u64)addr,PAGE_SIZE);++return(ret!=PAGE_SIZE)?-EINVAL:0;+}+
I probably missed something but as the pagetable pages for this mapping are
pined, it seems impossible (currently) for this call to fail. Maybe a WARN_ON
would be more appropriate, especially the callers in the subsequent patches do
not seem to check for this function return value?
Right, I think that wouldn't hurt. And while looking at this, I
actually think we could get rid of these calls to the map/unmap
functions entirely by keeping the pointers to the reserved PTEs
directly in addition to the VA to which they correspond in the percpu
table. That way we could manipulate the PTEs directly and avoid
unnecessary pgtable walks. Bits [63:1] can probably remain untouched,
Well, the address bits need to change too obviously, but rest can stay.
and {un}mapping is then only a matter of flipping bit 0 in the PTE
(and TLBI on the unmap path). I'll have a go at it.
Cheers,
Quentin
From: Vincent Donnefort <hidden> Date: 2022-07-19 14:25:27
On Thu, Jun 30, 2022 at 02:57:23PM +0100, Will Deacon wrote:
Hi everyone,
This series has been extracted from the pKVM base support series (aka
"pKVM mega-patch") previously posted here:
https://lore.kernel.org/kvmarm/20220519134204.5379-1-will@kernel.org/
Unlike that more comprehensive series, this one is fairly fundamental
and does not introduce any new ABI commitments, leaving questions
involving the management of guest private memory and the creation of
protected VMs for future work. Instead, this series extends the pKVM EL2
code so that it can dynamically instantiate and manage VM shadow
structures without the host being able to access them directly. These
shadow structures consist of a shadow VM, a set of shadow vCPUs and the
stage-2 page-table and the pages used to hold them are returned to the
host when the VM is destroyed.
The last patch is marked as RFC because, although it plumbs in the
shadow state, it is woefully inefficient and copies to/from the host
state on every vCPU run. Without the last patch, the new structures are
unused but we move considerably closer to isolating guests from the
host.
The series is based on Marc's rework of the flags
(kvm-arm64/burn-the-flags).
Feedback welcome.
Cheers,
Only had few nitpicks
Reviewed-by: Vincent Donnefort <redacted>
Also, I've been using this patchset for quite a while now.
Tested-by: Vincent Donnefort <redacted>
[...]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Christopherson <seanjc@google.com> Date: 2022-07-19 16:12:57
Apologies for the slow reply.
On Fri, Jul 08, 2022, Will Deacon wrote:
On Wed, Jul 06, 2022 at 07:17:29PM +0000, Sean Christopherson wrote:
quoted
On Thu, Jun 30, 2022, Will Deacon wrote:
The lack of documentation and the rather terse changelogs make this really hard
to review for folks that aren't intimately familiar with pKVM. I have a decent
idea of the end goal of "shadowing", but that's mostly because of my involvement in
similar x86 projects. Nothing in the changelogs ever explains _why_ pKVM uses
shadows.
That's understandable, but thanks for persevering; this series is pretty
down in the murky depths of the arm64 architecture and EL2 code so it
doesn't really map to the KVM code most folks are familiar with. It's fair
to say we're assuming a lot of niche prior knowledge (which is quite common
for arch code in my experience),
Assuming prior knowledge is fine so long as that prior knowledge is something
that can be gained by future readers through public documentation. E.g. arch code
is always littered with acronyms, but someone can almost always decipher the
acronyms by reading specs or just searching the interwebs.
My objection to the changelogs is that they talk about "shadow" VMs, vCPUs, state,
structures, etc., without ever explaining what a shadow is, how it will be used,
or what its end purpose is.
A big part of the problem is that "shadow" is not unique terminology _and_ isn't
inherently tied to "protected kvm", e.g. a reader can't intuit that a "shadow vm"
is the the trusted hypervisors instance of the VM. And I can search for pKVM and
get relevant, helpful search results. But if I search for "shadow vm", I get
unrelated results, and "pkvm shadow vm" just leads me back to this series.
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.
On the more positive side, we'll be speaking at KVM forum about what we've
done here, so that will be a great place to discuss it further and then we
can also link back to the recordings in later postings of the mega-series.
quoted
I put "shadowing" in quotes because if the unstrusted host is aware that the VM
and vCPU it is manipulating aren't the "real" VMs/vCPUs, and there is an explicit API
between the untrusted host and pKVM for creating/destroying VMs/vCPUs, then I would
argue that it's not truly shadowing, especially if pKVM uses data/values verbatim
and only verifies correctness/safety. It's definitely a nit, but for future readers
I think overloading "shadowing" could be confusing.
Ah, this is really interesting and nicely puts the ball back in my court as
I'm not well versed with x86's use of "shadowing".
It's not just an x86, e.g. see https://en.wikipedia.org/wiki/Shadow_table. The
use in pKVM is _really_ close in that what pKVM calls the shadow is the "real"
data that's used, but pKVM inverts the typical virtualization usage, which is
why I find it confusing. I.e. instead of shadowing state being written by the
guest, pKVM is "shadowing" state written by the host. If there ever comes a need
to actually shadow guest state, e.g. for nested virtualization, then using shadow
to refer to the protected state is going to create a conundrum.
Honestly, I think pKVM is simply being too cute in picking names. And not just
for "shadow", e.g. IMO the flush/sync terminology in patch 24 is also unnecessarily
cute. Instead of coming up with clever names, just be explicit in what the code
is doing. E.g. something like:
flush_shadow_state() => sync_host_to_pkvm_vcpu()
sync_shadow_state() => sync_pkvm_to_host_vcpu()
Then readers know the two functions are pairs, and will have a decent idea of
what the functions do even if they don't fully understand pKVM vs. host.
"shadow_area_size" is another case where it's unnecessarily cryptic, e.g. just
call it "donated_memory_size".
Perhaps we should s/shadow/hyp/ to make this a little clearer?
Or maybe just "pkvm"? 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);
Even better is to not have to do that in the first place. AFAICT, there's no need
to do such a lookup in handle___kvm_vcpu_run() since pKVM already has pointers to
both the host vCPU and the pKVM vCPU.
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);
hyp_page_ref_inc(hyp_virt_to_page(vm));
unlock:
hyp_spin_unlock(&pkvm_global_lock);
return pkvm_vcpu;
}
struct kvm_vcpu *pkvm_vcpu_put(struct kvm_vcpu *pkvm_vcpu)
{
hyp_spin_lock(&pkvm_global_lock);
hyp_page_ref_dec(hyp_virt_to_page(pkvm_vcpu->kvm));
hyp_spin_unlock(&pkvm_global_lock);
}
static void sync_host_to_pkvm_vcpu(struct kvm_vcpu *pkvm_vcpu, struct kvm_vcpu *host_vcpu)
{
pkvm_vcpu->arch.ctxt = host_vcpu->arch.ctxt;
pkvm_vcpu->arch.sve_state = kern_hyp_va(host_vcpu->arch.sve_state);
pkvm_vcpu->arch.sve_max_vl = host_vcpu->arch.sve_max_vl;
pkvm_vcpu->arch.hw_mmu = host_vcpu->arch.hw_mmu;
pkvm_vcpu->arch.hcr_el2 = host_vcpu->arch.hcr_el2;
pkvm_vcpu->arch.mdcr_el2 = host_vcpu->arch.mdcr_el2;
pkvm_vcpu->arch.cptr_el2 = host_vcpu->arch.cptr_el2;
pkvm_vcpu->arch.iflags = host_vcpu->arch.iflags;
pkvm_vcpu->arch.fp_state = host_vcpu->arch.fp_state;
pkvm_vcpu->arch.debug_ptr = kern_hyp_va(host_vcpu->arch.debug_ptr);
pkvm_vcpu->arch.host_fpsimd_state = host_vcpu->arch.host_fpsimd_state;
pkvm_vcpu->arch.vsesr_el2 = host_vcpu->arch.vsesr_el2;
pkvm_vcpu->arch.vgic_cpu.vgic_v3 = host_vcpu->arch.vgic_cpu.vgic_v3;
}
static void sync_pkvm_to_host_vcpu(struct kvm_vcpu *host_vcpu, struct kvm_vcpu *pkvm_vcpu)
{
struct vgic_v3_cpu_if *pkvm_cpu_if = &pkvm_vcpu->arch.vgic_cpu.vgic_v3;
struct vgic_v3_cpu_if *host_cpu_if = &host_vcpu->arch.vgic_cpu.vgic_v3;
unsigned int i;
host_vcpu->arch.ctxt = pkvm_vcpu->arch.ctxt;
host_vcpu->arch.hcr_el2 = pkvm_vcpu->arch.hcr_el2;
host_vcpu->arch.cptr_el2 = pkvm_vcpu->arch.cptr_el2;
host_vcpu->arch.fault = pkvm_vcpu->arch.fault;
host_vcpu->arch.iflags = pkvm_vcpu->arch.iflags;
host_vcpu->arch.fp_state = pkvm_vcpu->arch.fp_state;
host_cpu_if->vgic_hcr = pkvm_cpu_if->vgic_hcr;
for (i = 0; i < pkvm_cpu_if->used_lrs; ++i)
host_cpu_if->vgic_lr[i] = pkvm_cpu_if->vgic_lr[i];
}
static void handle___kvm_vcpu_run(struct kvm_cpu_context *host_ctxt)
{
DECLARE_REG(struct kvm_vcpu *, host_vcpu, host_ctxt, 1);
int ret;
host_vcpu = kern_hyp_va(host_vcpu);
if (unlikely(is_protected_kvm_enabled())) {
struct kvm *host_kvm = kern_hyp_va(host_vcpu->kvm);
struct kvm_vcpu *pkvm_vcpu;
pkvm_vcpu = pkvm_vcpu_load(host_kvm, host_vcpu);
if (!pkvm_vcpu) {
ret = -EINVAL;
goto out;
}
sync_host_to_pkvm_vcpu(pkvm_vcpu, host_vcpu);
ret = __kvm_vcpu_run(pkvm_vcpu);
sync_pkvm_to_host_vcpu(host_vcpu, pkvm_vcpu);
pkvm_vcpu_put(pkvm_vcpu);
} else {
ret = __kvm_vcpu_run(host_vcpu);
}
out:
cpu_reg(host_ctxt, 1) = ret;
}
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2022-07-20 09:26:52
On Tue, 19 Jul 2022 17:11:32 +0100,
Sean Christopherson [off-list ref] wrote:
Honestly, I think pKVM is simply being too cute in picking names.
I don't know what you mean by "cute" here, but I assume this is not
exactly a flattering qualifier.
And not just for "shadow", e.g. IMO the flush/sync terminology in
patch 24 is also unnecessarily cute. Instead of coming up with
clever names, just be explicit in what the code is doing.
E.g. something like:
flush_shadow_state() => sync_host_to_pkvm_vcpu()
sync_shadow_state() => sync_pkvm_to_host_vcpu()
As much as I like bikesheding, this isn't going to happen. We have had
the sync/flush duality since day one, we have a lot of code based
around this naming, and departing from it seems counter productive.
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
From: Oliver Upton <hidden> Date: 2022-07-20 15:12:17
Hi Will,
On Thu, Jun 30, 2022 at 02:57:29PM +0100, Will Deacon wrote:
quoted hunk
The 'pkvm_component_id' enum type provides constants to refer to the
host and the hypervisor, yet this information is duplicated by the
'pkvm_hyp_id' constant.
Remove the definition of 'pkvm_hyp_id' and move the 'pkvm_component_id'
type definition to 'mem_protect.h' so that it can be used outside of
the memory protection code.
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 6 +++++-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 8 --------
arch/arm64/kvm/hyp/nvhe/setup.c | 2 +-
3 files changed, 6 insertions(+), 10 deletions(-)
@@ -51,7 +51,11 @@ struct host_kvm {};externstructhost_kvmhost_kvm;-externconstu8pkvm_hyp_id;+/* This corresponds to page-table locking order */+enumpkvm_component_id{+PKVM_ID_HOST,+PKVM_ID_HYP,+};
Since we have the concept of PTE ownership in pgtable.c, WDYT about
moving the owner ID enumeration there? KVM_MAX_OWNER_ID should be
incorporated in the enum too.
--
Thanks,
Oliver
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2022-07-20 18:16:35
Hi Oliver,
Thanks for having a look.
On Wed, Jul 20, 2022 at 03:11:04PM +0000, Oliver Upton wrote:
On Thu, Jun 30, 2022 at 02:57:29PM +0100, Will Deacon wrote:
quoted
The 'pkvm_component_id' enum type provides constants to refer to the
host and the hypervisor, yet this information is duplicated by the
'pkvm_hyp_id' constant.
Remove the definition of 'pkvm_hyp_id' and move the 'pkvm_component_id'
type definition to 'mem_protect.h' so that it can be used outside of
the memory protection code.
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 6 +++++-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 8 --------
arch/arm64/kvm/hyp/nvhe/setup.c | 2 +-
3 files changed, 6 insertions(+), 10 deletions(-)
@@ -51,7 +51,11 @@ struct host_kvm {};externstructhost_kvmhost_kvm;-externconstu8pkvm_hyp_id;+/* This corresponds to page-table locking order */+enumpkvm_component_id{+PKVM_ID_HOST,+PKVM_ID_HYP,+};
Since we have the concept of PTE ownership in pgtable.c, WDYT about
moving the owner ID enumeration there? KVM_MAX_OWNER_ID should be
incorporated in the enum too.
Interesting idea... I think we need the definition in a header file so that
it can be used by mem_protect.c, so I'm not entirely sure where you'd like
to see it moved.
The main worry I have is that if we ever need to distinguish e.g. one guest
instance from another, which is likely needed for sharing of memory
between more than just two components, then the pgtable code really cares
about the number of instances ("which guest is it?") whilst the mem_protect
cares about the component type ("is it a guest?").
Finally, the pgtable code is also used outside of pKVM so, although the
concept of ownership doesn't yet apply elsewhere, keeping the concept
available without dictacting the different types of owners makes sense to
me.
Does that make sense?
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
+ * kvm_pgtable_stage2_pgd_size() - Helper to compute size of a stage-2 PGD+ * @vtcr: Content of the VTCR register.+ *+ * Return: the size (in bytes) of the stage-2 PGD+ */
I'll also check this is valid kernel-doc before adding the new comment
syntax!
quoted
+/*+ * Holds the relevant data for maintaining the vcpu state completely at hyp.+ */+struct kvm_shadow_vcpu_state {+ /* The data for the shadow vcpu. */+ struct kvm_vcpu shadow_vcpu;++ /* A pointer to the host's vcpu. */+ struct kvm_vcpu *host_vcpu;++ /* A pointer to the shadow vm. */+ struct kvm_shadow_vm *shadow_vm;
IMHO, those declarations are already self-explanatory. The comments above don't
bring much.
Agreed, and Sean has ideas to rework bits of this as well. I'll drop the
comments.
I don't think this one is necessary, it is already included in mm.h.
I thought it was generally bad form to rely on transitive includes, as it
makes header rework even more painful than it already is.
quoted
+static void unpin_host_vcpus(struct kvm_shadow_vcpu_state *shadow_vcpu_states,+ unsigned int nr_vcpus)+{+ int i;++ for (i = 0; i < nr_vcpus; i++) {+ struct kvm_vcpu *host_vcpu = shadow_vcpu_states[i].host_vcpu;
IIRC, checkpatch likes an empty line after declarations.
We can fix that!
quoted
+static unsigned int insert_shadow_table(struct kvm *kvm,+ struct kvm_shadow_vm *vm,+ size_t shadow_size)+{+ struct kvm_s2_mmu *mmu = &vm->kvm.arch.mmu;+ unsigned int shadow_handle;+ unsigned int vmid;++ hyp_assert_lock_held(&shadow_lock);++ if (unlikely(nr_shadow_entries >= KVM_MAX_PVMS))+ return -ENOMEM;++ /*+ * Initializing protected state might have failed, yet a malicious host+ * could trigger this function. Thus, ensure that shadow_table exists.+ */+ if (unlikely(!shadow_table))+ return -EINVAL;++ /* Check that a shadow hasn't been created before for this host KVM. */+ if (unlikely(__exists_shadow(kvm)))+ return -EEXIST;++ /* Find the next free entry in the shadow table. */+ while (shadow_table[next_shadow_alloc])+ next_shadow_alloc = (next_shadow_alloc + 1) % KVM_MAX_PVMS;
Couldn't it be merged with __exists_shadow which already knows the first free
shadow_table idx?
Good idea, that would save us going through it twice.
quoted
+ shadow_handle = idx_to_shadow_handle(next_shadow_alloc);++ vm->kvm.arch.pkvm.shadow_handle = shadow_handle;+ vm->shadow_area_size = shadow_size;++ /* VMID 0 is reserved for the host */+ vmid = next_shadow_alloc + 1;+ if (vmid > 0xff)
Couldn't the 0xff be found with get_vmid_bits() or even from host_kvm.arch.vtcr?
Or does that depends on something completely different?
Also, appologies if this has been discussed already and I missed it, maybe
KVM_MAX_PVMS could be changed for that value - 1. Unless we think that archs
supporting 16 bits would waste way too much memory for that?
We should probably clamp the VMID based on KVM_MAX_PVMS here, as although
some CPUs support 16-bit VMIDs, we don't currently support that with pKVM.
I'll make that change to avoid hard-coding the constant here.
Thanks!
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2022-07-20 18:50:11
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:
quoted
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...
quoted
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
From: Sean Christopherson <seanjc@google.com> Date: 2022-07-20 21:18:08
On Wed, Jul 20, 2022, Will Deacon wrote:
Hi Sean,
On Tue, Jul 19, 2022 at 04:11:32PM +0000, Sean Christopherson wrote:
quoted
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.
Right, my suggestion was to rename that to pkvm_handle to avoid a direct conflict,
and then that naturally yields the "pkvm_handle => pkvm_vm" association. Or are
you expecting to shove more stuff into the that "pkvm" struct?
So I'd say either "pkvm_hyp" or "hyp" instead of "shadow". The latter is
nice and short...
I 100% agree that differentating between EL1 and EL2 is important for functions,
structs and global variables, but I would argue it's not so important for fields
and local variables where the "owning" struct/function provides that context. But
that's actually a partial argument for just using "hyp".
My concern with just using e.g. "kvm_hyp" is that, because non-pKVM nVHE also has
the host vs. hyp split, it could lead people to believe that "kvm_hyp" is also
used for the non-pKVM case.
So, what about a blend? E.g. "struct pkvm_hyp_vcpu *hyp_vcpu". That provides
the context that the struct is specific to the EL2 side of pKVM, most usage is
nice and short, and the "hyp" prefix avoids the ambiguity that a bare "pkvm" would
suffer for EL1 vs. EL2.
Doesn't look awful?
static void handle___kvm_vcpu_run(struct kvm_cpu_context *host_ctxt)
{
DECLARE_REG(struct kvm_vcpu *, host_vcpu, host_ctxt, 1);
int ret;
host_vcpu = kern_hyp_va(host_vcpu);
if (unlikely(is_protected_kvm_enabled())) {
struct pkvm_hyp_vcpu *hyp_vcpu;
struct kvm *host_kvm;
host_kvm = kern_hyp_va(host_vcpu->kvm);
hyp_vcpu = pkvm_load_hyp_vcpu(host_kvm->arch.pkvm.handle,
host_vcpu->vcpu_idx);
if (!hyp_vcpu) {
ret = -EINVAL;
goto out;
}
flush_pkvm_guest_state(hyp_vcpu);
ret = __kvm_vcpu_run(shadow_vcpu);
sync_pkvm_guest_state(hyp_vcpu);
pkvm_put_hyp_vcpu(shadow_state);
} else {
/* The host is fully trusted, run its vCPU directly. */
ret = __kvm_vcpu_run(host_vcpu);
}
out:
cpu_reg(host_ctxt, 1) = ret;
}
quoted
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:
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.
Hmm, are there guardrails in place to prevent using "unsafe" fields from "struct kvm"
and "struct kvm_vcpu" at EL2? If not, it seems like embedding the common structs
in the hyp/pkvm-specific structs is going bite us in the rear at some point.
Mostly out of curiosity, I assume the EL2 restriction only applies to nVHE mode?
And waaaay off topic, has anyone explored adding macro magic to generate wrappers
to (un)marshall registers to parameters/returns for the hyp functions? E.g. it'd
be neat if you could make the code look like this without having to add a wrapper
for every function:
static int handle___kvm_vcpu_run(unsigned long __host_vcpu)
{
struct kvm_vcpu *host_vcpu = kern_hyp_va(__host_vcpu);
int ret;
if (unlikely(is_protected_kvm_enabled())) {
struct pkvm_hyp_vcpu *hyp_vcpu;
struct kvm *host_kvm;
host_kvm = kern_hyp_va(host_vcpu->kvm);
hyp_vcpu = pkvm_load_hyp_vcpu(host_kvm->arch.pkvm.handle,
host_vcpu->vcpu_idx);
if (!hyp_vcpu)
return -EINVAL;
flush_hypervisor_state(hyp_vcpu);
ret = __kvm_vcpu_run(shadow_vcpu);
sync_hypervisor_state(hyp_vcpu);
pkvm_put_hyp_vcpu(shadow_state);
} else {
/* The host is fully trusted, run its vCPU directly. */
ret = __kvm_vcpu_run(host_vcpu);
}
return ret;
}
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Oliver Upton <hidden> Date: 2022-07-29 19:29:30
Hi Will,
Sorry, I didn't see your reply til now.
On Wed, Jul 20, 2022 at 07:14:07PM +0100, Will Deacon wrote:
Hi Oliver,
Thanks for having a look.
On Wed, Jul 20, 2022 at 03:11:04PM +0000, Oliver Upton wrote:
quoted
On Thu, Jun 30, 2022 at 02:57:29PM +0100, Will Deacon wrote:
quoted
The 'pkvm_component_id' enum type provides constants to refer to the
host and the hypervisor, yet this information is duplicated by the
'pkvm_hyp_id' constant.
Remove the definition of 'pkvm_hyp_id' and move the 'pkvm_component_id'
type definition to 'mem_protect.h' so that it can be used outside of
the memory protection code.
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 6 +++++-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 8 --------
arch/arm64/kvm/hyp/nvhe/setup.c | 2 +-
3 files changed, 6 insertions(+), 10 deletions(-)
@@ -51,7 +51,11 @@ struct host_kvm {};externstructhost_kvmhost_kvm;-externconstu8pkvm_hyp_id;+/* This corresponds to page-table locking order */+enumpkvm_component_id{+PKVM_ID_HOST,+PKVM_ID_HYP,+};
Since we have the concept of PTE ownership in pgtable.c, WDYT about
moving the owner ID enumeration there? KVM_MAX_OWNER_ID should be
incorporated in the enum too.
Interesting idea... I think we need the definition in a header file so that
it can be used by mem_protect.c, so I'm not entirely sure where you'd like
to see it moved.
The main worry I have is that if we ever need to distinguish e.g. one guest
instance from another, which is likely needed for sharing of memory
between more than just two components, then the pgtable code really cares
about the number of instances ("which guest is it?") whilst the mem_protect
cares about the component type ("is it a guest?").
Finally, the pgtable code is also used outside of pKVM so, although the
concept of ownership doesn't yet apply elsewhere, keeping the concept
available without dictacting the different types of owners makes sense to
me.
Sorry, it was a silly suggestion to wedge the enum there. I don't think
it matters too much where it winds up, but something like:
enum kvm_pgtable_owner_id {
OWNER_ID_PKVM_HOST,
OWNER_ID_PKVM_HYP,
NR_PGTABLE_OWNER_IDS,
}
And put it somewhere that both pgtable.c and mem_protect.c can get at
it. That way bound checks (like in kvm_pgtable_stage2_set_owner())
organically work as new IDs are added.
--
Thanks,
Oliver
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel