From: Isaku Yamahata <[email protected]>
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 <[email protected]>
---
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);
--
2.25.1
On 3/4/22 20:49, [email protected] wrote:
> From: Isaku Yamahata <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
On Fri, 2022-03-04 at 11:49 -0800, [email protected] wrote:
> From: Isaku Yamahata <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
--
Thanks,
-Kai