Message ID | 499d1fd01b0d1d9a8b46a55bb863afd0c76f1111.1646422845.git.isaku.yamahata@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM TDX basic feature support | expand |
On 3/4/22 20:49, isaku.yamahata@intel.com wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > Add a private pointer to kvm_mmu_page for private EPT. > > To resolve KVM page fault on private GPA, it will allocate additional page > for Secure EPT in addition to private EPT. Add memory allocator for it and > topup its memory allocator before resolving KVM page fault similar to > shared EPT page. Allocation of those memory will be done for TDP MMU by > alloc_tdp_mmu_page(). Freeing those memory will be done for TDP MMU on > behalf of kvm_tdp_mmu_zap_all() called by kvm_mmu_zap_all(). Private EPT > page needs to carry one more page used for Secure EPT in addition to the > private EPT page. Add private pointer to struct kvm_mmu_page for that > purpose and Add helper functions to allocate/free a page for Secure EPT. > Also add helper functions to check if a given kvm_mmu_page is private. > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/mmu/mmu.c | 9 ++++ > arch/x86/kvm/mmu/mmu_internal.h | 84 +++++++++++++++++++++++++++++++++ > arch/x86/kvm/mmu/tdp_mmu.c | 3 ++ > 4 files changed, 97 insertions(+) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index fcab2337819c..0c8cc7d73371 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -689,6 +689,7 @@ struct kvm_vcpu_arch { > struct kvm_mmu_memory_cache mmu_shadow_page_cache; > struct kvm_mmu_memory_cache mmu_gfn_array_cache; > struct kvm_mmu_memory_cache mmu_page_header_cache; > + struct kvm_mmu_memory_cache mmu_private_sp_cache; > > /* > * QEMU userspace and the guest each have their own FPU state. > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 6e9847b1124b..8def8b97978f 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -758,6 +758,13 @@ static int mmu_topup_shadow_page_cache(struct kvm_vcpu *vcpu) > struct kvm_mmu_memory_cache *mc = &vcpu->arch.mmu_shadow_page_cache; > int start, end, i, r; > > + if (kvm_gfn_stolen_mask(vcpu->kvm)) { > + r = kvm_mmu_topup_memory_cache(&vcpu->arch.mmu_private_sp_cache, > + PT64_ROOT_MAX_LEVEL); > + if (r) > + return r; > + } > + > if (shadow_init_value) > start = kvm_mmu_memory_cache_nr_free_objects(mc); > > @@ -799,6 +806,7 @@ static void mmu_free_memory_caches(struct kvm_vcpu *vcpu) > { > kvm_mmu_free_memory_cache(&vcpu->arch.mmu_pte_list_desc_cache); > kvm_mmu_free_memory_cache(&vcpu->arch.mmu_shadow_page_cache); > + kvm_mmu_free_memory_cache(&vcpu->arch.mmu_private_sp_cache); > kvm_mmu_free_memory_cache(&vcpu->arch.mmu_gfn_array_cache); > kvm_mmu_free_memory_cache(&vcpu->arch.mmu_page_header_cache); > } > @@ -1791,6 +1799,7 @@ static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, int direct > if (!direct) > sp->gfns = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_gfn_array_cache); > set_page_private(virt_to_page(sp->spt), (unsigned long)sp); > + kvm_mmu_init_private_sp(sp); > > /* > * active_mmu_pages must be a FIFO list, as kvm_zap_obsolete_pages() > diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h > index da6166b5c377..80f7a74a71dc 100644 > --- a/arch/x86/kvm/mmu/mmu_internal.h > +++ b/arch/x86/kvm/mmu/mmu_internal.h > @@ -53,6 +53,10 @@ struct kvm_mmu_page { > u64 *spt; > /* hold the gfn of each spte inside spt */ > gfn_t *gfns; > +#ifdef CONFIG_KVM_MMU_PRIVATE > + /* associated private shadow page, e.g. SEPT page */ > + void *private_sp; > +#endif > /* Currently serving as active root */ > union { > int root_count; > @@ -104,6 +108,86 @@ static inline int kvm_mmu_page_as_id(struct kvm_mmu_page *sp) > return kvm_mmu_role_as_id(sp->role); > } > > +/* > + * TDX vcpu allocates page for root Secure EPT page and assigns to CPU secure > + * EPT pointer. KVM doesn't need to allocate and link to the secure EPT. > + * Dummy value to make is_pivate_sp() return true. > + */ > +#define KVM_MMU_PRIVATE_SP_ROOT ((void *)1) > + > +#ifdef CONFIG_KVM_MMU_PRIVATE > +static inline bool is_private_sp(struct kvm_mmu_page *sp) > +{ > + return !!sp->private_sp; > +} > + > +static inline bool is_private_spte(u64 *sptep) > +{ > + return is_private_sp(sptep_to_sp(sptep)); > +} > + > +static inline void *kvm_mmu_private_sp(struct kvm_mmu_page *sp) > +{ > + return sp->private_sp; > +} > + > +static inline void kvm_mmu_init_private_sp(struct kvm_mmu_page *sp) > +{ > + sp->private_sp = NULL; > +} > + > +/* Valid sp->role.level is required. */ > +static inline void kvm_mmu_alloc_private_sp(struct kvm_vcpu *vcpu, > + struct kvm_mmu_page *sp) > +{ > + if (vcpu->arch.mmu->shadow_root_level == sp->role.level) > + sp->private_sp = KVM_MMU_PRIVATE_SP_ROOT; > + else > + sp->private_sp = > + kvm_mmu_memory_cache_alloc( > + &vcpu->arch.mmu_private_sp_cache); > + /* > + * Because mmu_private_sp_cache is topped up before staring kvm page > + * fault resolving, the allocation above shouldn't fail. > + */ > + WARN_ON_ONCE(!sp->private_sp); > +} > + > +static inline void kvm_mmu_free_private_sp(struct kvm_mmu_page *sp) > +{ > + if (sp->private_sp != KVM_MMU_PRIVATE_SP_ROOT) > + free_page((unsigned long)sp->private_sp); > +} > +#else > +static inline bool is_private_sp(struct kvm_mmu_page *sp) > +{ > + return false; > +} > + > +static inline bool is_private_spte(u64 *sptep) > +{ > + return false; > +} > + > +static inline void *kvm_mmu_private_sp(struct kvm_mmu_page *sp) > +{ > + return NULL; > +} > + > +static inline void kvm_mmu_init_private_sp(struct kvm_mmu_page *sp) > +{ > +} > + > +static inline void kvm_mmu_alloc_private_sp(struct kvm_vcpu *vcpu, > + struct kvm_mmu_page *sp) > +{ > +} > + > +static inline void kvm_mmu_free_private_sp(struct kvm_mmu_page *sp) > +{ > +} > +#endif > + > static inline bool kvm_mmu_page_ad_need_write_protect(struct kvm_mmu_page *sp) > { > /* > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 8db262440d5c..a68f3a22836b 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -59,6 +59,8 @@ static bool zap_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, > > static void tdp_mmu_free_sp(struct kvm_mmu_page *sp) > { > + if (is_private_sp(sp)) > + kvm_mmu_free_private_sp(sp); > free_page((unsigned long)sp->spt); > kmem_cache_free(mmu_page_header_cache, sp); > } > @@ -184,6 +186,7 @@ static struct kvm_mmu_page *alloc_tdp_mmu_page(struct kvm_vcpu *vcpu, gfn_t gfn, > sp->role.word = page_role_for_level(vcpu, level).word; > sp->gfn = gfn; > sp->tdp_mmu_page = true; > + kvm_mmu_init_private_sp(sp); > > trace_kvm_mmu_get_page(sp, true); > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
On Fri, 2022-03-04 at 11:49 -0800, isaku.yamahata@intel.com wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > Add a private pointer to kvm_mmu_page for private EPT. > > To resolve KVM page fault on private GPA, it will allocate additional page > for Secure EPT in addition to private EPT. Add memory allocator for it and > topup its memory allocator before resolving KVM page fault similar to > shared EPT page. Allocation of those memory will be done for TDP MMU by > alloc_tdp_mmu_page(). Freeing those memory will be done for TDP MMU on > behalf of kvm_tdp_mmu_zap_all() called by kvm_mmu_zap_all(). Private EPT > page needs to carry one more page used for Secure EPT in addition to the > private EPT page. Add private pointer to struct kvm_mmu_page for that > purpose and Add helper functions to allocate/free a page for Secure EPT. > Also add helper functions to check if a given kvm_mmu_page is private. > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/mmu/mmu.c | 9 ++++ > arch/x86/kvm/mmu/mmu_internal.h | 84 +++++++++++++++++++++++++++++++++ > arch/x86/kvm/mmu/tdp_mmu.c | 3 ++ > 4 files changed, 97 insertions(+) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index fcab2337819c..0c8cc7d73371 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -689,6 +689,7 @@ struct kvm_vcpu_arch { > struct kvm_mmu_memory_cache mmu_shadow_page_cache; > struct kvm_mmu_memory_cache mmu_gfn_array_cache; > struct kvm_mmu_memory_cache mmu_page_header_cache; > + struct kvm_mmu_memory_cache mmu_private_sp_cache; > > /* > * QEMU userspace and the guest each have their own FPU state. > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 6e9847b1124b..8def8b97978f 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -758,6 +758,13 @@ static int mmu_topup_shadow_page_cache(struct kvm_vcpu *vcpu) > struct kvm_mmu_memory_cache *mc = &vcpu->arch.mmu_shadow_page_cache; > int start, end, i, r; > > + if (kvm_gfn_stolen_mask(vcpu->kvm)) { Please get rid of kvm_gfn_stolen_mask(). > + r = kvm_mmu_topup_memory_cache(&vcpu->arch.mmu_private_sp_cache, > + PT64_ROOT_MAX_LEVEL); > + if (r) > + return r; > + } > + > if (shadow_init_value) > start = kvm_mmu_memory_cache_nr_free_objects(mc); >
On 4/7/22 01:43, Kai Huang wrote: >> + if (kvm_gfn_stolen_mask(vcpu->kvm)) { > Please get rid of kvm_gfn_stolen_mask(). > Kai, please follow the other reviews that I have posted in the last few days. Paolo
On Thu, 2022-04-07 at 15:52 +0200, Paolo Bonzini wrote: > On 4/7/22 01:43, Kai Huang wrote: > > > + if (kvm_gfn_stolen_mask(vcpu->kvm)) { > > Please get rid of kvm_gfn_stolen_mask(). > > > > Kai, please follow the other reviews that I have posted in the last few > days. > > Paolo > Do you mean below reply? "I think use of kvm_gfn_stolen_mask() should be minimized anyway. I would rename it to to kvm_{gfn,gpa}_private_mask and not return bool." I also mean we should not use kvm_gfn_stolen_mask(). I don't have opinion on the new name. Perhaps kvm_is_protected_vm() is my preference though.
On 4/8/22 00:53, Kai Huang wrote: >> > Do you mean below reply? > > "I think use of kvm_gfn_stolen_mask() should be minimized anyway. I > would rename it to to kvm_{gfn,gpa}_private_mask and not return bool." > > I also mean we should not use kvm_gfn_stolen_mask(). I don't have opinion on > the new name. Perhaps kvm_is_protected_vm() is my preference though. But this is one of the case where it would survive, even with the changed name. Paolo
On Fri, 2022-04-08 at 01:03 +0200, Paolo Bonzini wrote: > On 4/8/22 00:53, Kai Huang wrote: > > > > > Do you mean below reply? > > > > "I think use of kvm_gfn_stolen_mask() should be minimized anyway. I > > would rename it to to kvm_{gfn,gpa}_private_mask and not return bool." > > > > I also mean we should not use kvm_gfn_stolen_mask(). I don't have opinion on > > the new name. Perhaps kvm_is_protected_vm() is my preference though. > > But this is one of the case where it would survive, even with the > changed name. > > Paolo > Perhaps I confused you (sorry about that). Yes we do need the check here. I just dislike the function name.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index fcab2337819c..0c8cc7d73371 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -689,6 +689,7 @@ struct kvm_vcpu_arch { struct kvm_mmu_memory_cache mmu_shadow_page_cache; struct kvm_mmu_memory_cache mmu_gfn_array_cache; struct kvm_mmu_memory_cache mmu_page_header_cache; + struct kvm_mmu_memory_cache mmu_private_sp_cache; /* * QEMU userspace and the guest each have their own FPU state. diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 6e9847b1124b..8def8b97978f 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -758,6 +758,13 @@ static int mmu_topup_shadow_page_cache(struct kvm_vcpu *vcpu) struct kvm_mmu_memory_cache *mc = &vcpu->arch.mmu_shadow_page_cache; int start, end, i, r; + if (kvm_gfn_stolen_mask(vcpu->kvm)) { + r = kvm_mmu_topup_memory_cache(&vcpu->arch.mmu_private_sp_cache, + PT64_ROOT_MAX_LEVEL); + if (r) + return r; + } + if (shadow_init_value) start = kvm_mmu_memory_cache_nr_free_objects(mc); @@ -799,6 +806,7 @@ static void mmu_free_memory_caches(struct kvm_vcpu *vcpu) { kvm_mmu_free_memory_cache(&vcpu->arch.mmu_pte_list_desc_cache); kvm_mmu_free_memory_cache(&vcpu->arch.mmu_shadow_page_cache); + kvm_mmu_free_memory_cache(&vcpu->arch.mmu_private_sp_cache); kvm_mmu_free_memory_cache(&vcpu->arch.mmu_gfn_array_cache); kvm_mmu_free_memory_cache(&vcpu->arch.mmu_page_header_cache); } @@ -1791,6 +1799,7 @@ static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, int direct if (!direct) sp->gfns = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_gfn_array_cache); set_page_private(virt_to_page(sp->spt), (unsigned long)sp); + kvm_mmu_init_private_sp(sp); /* * active_mmu_pages must be a FIFO list, as kvm_zap_obsolete_pages() diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h index da6166b5c377..80f7a74a71dc 100644 --- a/arch/x86/kvm/mmu/mmu_internal.h +++ b/arch/x86/kvm/mmu/mmu_internal.h @@ -53,6 +53,10 @@ struct kvm_mmu_page { u64 *spt; /* hold the gfn of each spte inside spt */ gfn_t *gfns; +#ifdef CONFIG_KVM_MMU_PRIVATE + /* associated private shadow page, e.g. SEPT page */ + void *private_sp; +#endif /* Currently serving as active root */ union { int root_count; @@ -104,6 +108,86 @@ static inline int kvm_mmu_page_as_id(struct kvm_mmu_page *sp) return kvm_mmu_role_as_id(sp->role); } +/* + * TDX vcpu allocates page for root Secure EPT page and assigns to CPU secure + * EPT pointer. KVM doesn't need to allocate and link to the secure EPT. + * Dummy value to make is_pivate_sp() return true. + */ +#define KVM_MMU_PRIVATE_SP_ROOT ((void *)1) + +#ifdef CONFIG_KVM_MMU_PRIVATE +static inline bool is_private_sp(struct kvm_mmu_page *sp) +{ + return !!sp->private_sp; +} + +static inline bool is_private_spte(u64 *sptep) +{ + return is_private_sp(sptep_to_sp(sptep)); +} + +static inline void *kvm_mmu_private_sp(struct kvm_mmu_page *sp) +{ + return sp->private_sp; +} + +static inline void kvm_mmu_init_private_sp(struct kvm_mmu_page *sp) +{ + sp->private_sp = NULL; +} + +/* Valid sp->role.level is required. */ +static inline void kvm_mmu_alloc_private_sp(struct kvm_vcpu *vcpu, + struct kvm_mmu_page *sp) +{ + if (vcpu->arch.mmu->shadow_root_level == sp->role.level) + sp->private_sp = KVM_MMU_PRIVATE_SP_ROOT; + else + sp->private_sp = + kvm_mmu_memory_cache_alloc( + &vcpu->arch.mmu_private_sp_cache); + /* + * Because mmu_private_sp_cache is topped up before staring kvm page + * fault resolving, the allocation above shouldn't fail. + */ + WARN_ON_ONCE(!sp->private_sp); +} + +static inline void kvm_mmu_free_private_sp(struct kvm_mmu_page *sp) +{ + if (sp->private_sp != KVM_MMU_PRIVATE_SP_ROOT) + free_page((unsigned long)sp->private_sp); +} +#else +static inline bool is_private_sp(struct kvm_mmu_page *sp) +{ + return false; +} + +static inline bool is_private_spte(u64 *sptep) +{ + return false; +} + +static inline void *kvm_mmu_private_sp(struct kvm_mmu_page *sp) +{ + return NULL; +} + +static inline void kvm_mmu_init_private_sp(struct kvm_mmu_page *sp) +{ +} + +static inline void kvm_mmu_alloc_private_sp(struct kvm_vcpu *vcpu, + struct kvm_mmu_page *sp) +{ +} + +static inline void kvm_mmu_free_private_sp(struct kvm_mmu_page *sp) +{ +} +#endif + static inline bool kvm_mmu_page_ad_need_write_protect(struct kvm_mmu_page *sp) { /* diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 8db262440d5c..a68f3a22836b 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -59,6 +59,8 @@ static bool zap_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root, static void tdp_mmu_free_sp(struct kvm_mmu_page *sp) { + if (is_private_sp(sp)) + kvm_mmu_free_private_sp(sp); free_page((unsigned long)sp->spt); kmem_cache_free(mmu_page_header_cache, sp); } @@ -184,6 +186,7 @@ static struct kvm_mmu_page *alloc_tdp_mmu_page(struct kvm_vcpu *vcpu, gfn_t gfn, sp->role.word = page_role_for_level(vcpu, level).word; sp->gfn = gfn; sp->tdp_mmu_page = true; + kvm_mmu_init_private_sp(sp); trace_kvm_mmu_get_page(sp, true);