| Message ID | 20240530210714.364118-10-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX MMU prep series part 1 | expand |
On Thu, 2024-05-30 at 14:07 -0700, Rick Edgecombe wrote: > > Update handle_changed_spte() to take a role. Use this for a KVM_BUG_ON(). I'm wondering if the KVM_BUG_ON() is not worth the cost to the diffstat.
On Fri, May 31, 2024 at 12:01 AM Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote: > > On Thu, 2024-05-30 at 14:07 -0700, Rick Edgecombe wrote: > > > > Update handle_changed_spte() to take a role. Use this for a KVM_BUG_ON(). > > I'm wondering if the KVM_BUG_ON() is not worth the cost to the diffstat. Agreed, it seems like pointless churn. Paolo
On Thu, May 30, 2024 at 11:07 PM Rick Edgecombe <rick.p.edgecombe@intel.com> wrote: > +static inline bool kvm_on_mirror(const struct kvm *kvm, enum kvm_process process) > +{ > + if (!kvm_has_mirrored_tdp(kvm)) > + return false; > + > + return process & KVM_PROCESS_PRIVATE; > +} > + > +static inline bool kvm_on_direct(const struct kvm *kvm, enum kvm_process process) > +{ > + if (!kvm_has_mirrored_tdp(kvm)) > + return true; > + > + return process & KVM_PROCESS_SHARED; > +} These helpers don't add much, it's easier to just write kvm_process_to_root_types() as if (process & KVM_PROCESS_SHARED) ret |= KVM_DIRECT_ROOTS; if (process & KVM_PROCESS_PRIVATE) ret |= kvm_has_mirror_tdp(kvm) ? KVM_MIRROR_ROOTS : KVM_DIRECT_ROOTS; WARN_ON_ONCE(!ret); return ret; (Here I chose to rename kvm_has_mirrored_tdp to kvm_has_mirror_tdp; but if you prefer to call it kvm_has_external_tdp, it's just as readable. Whatever floats your boat). > struct kvm *kvm = vcpu->kvm; > - struct kvm_mmu_page *root = root_to_sp(vcpu->arch.mmu->root.hpa); > + enum kvm_tdp_mmu_root_types root_type = tdp_mmu_get_fault_root_type(kvm, fault); > + struct kvm_mmu_page *root = tdp_mmu_get_root(vcpu, root_type); Please make this struct kvm_mmu_page *root = tdp_mmu_get_root_for_fault(vcpu, fault); Most other uses of tdp_mmu_get_root() don't really need a root_type, I'll get to them in the reviews for the rest of the series. > enum kvm_tdp_mmu_root_types { > KVM_VALID_ROOTS = BIT(0), > + KVM_DIRECT_ROOTS = BIT(1), > + KVM_MIRROR_ROOTS = BIT(2), > > - KVM_ANY_ROOTS = 0, > - KVM_ANY_VALID_ROOTS = KVM_VALID_ROOTS, > + KVM_ANY_ROOTS = KVM_DIRECT_ROOTS | KVM_MIRROR_ROOTS, > + KVM_ANY_VALID_ROOTS = KVM_DIRECT_ROOTS | KVM_MIRROR_ROOTS | KVM_VALID_ROOTS, See previous review. > +static inline enum kvm_tdp_mmu_root_types tdp_mmu_get_fault_root_type(struct kvm *kvm, > + struct kvm_page_fault *fault) > +{ > + if (fault->is_private) > + return kvm_process_to_root_types(kvm, KVM_PROCESS_PRIVATE); > + return KVM_DIRECT_ROOTS; > +} > + > +static inline struct kvm_mmu_page *tdp_mmu_get_root(struct kvm_vcpu *vcpu, enum kvm_tdp_mmu_root_types type) > +{ > + if (type == KVM_MIRROR_ROOTS) > + return root_to_sp(vcpu->arch.mmu->mirror_root_hpa); > + return root_to_sp(vcpu->arch.mmu->root.hpa); > +} static inline struct kvm_mmu_page * tdp_mmu_get_root_for_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) { hpa_t root_hpa; if (unlikely(fault->is_private && kvm_has_mirror_tdp(kvm))) root_hpa = vcpu->arch.mmu->mirror_root_hpa; else root_hpa = vcpu->arch.mmu->root.hpa; return root_to_sp(root_hpa); } Paolo
On Fri, 2024-06-07 at 10:46 +0200, Paolo Bonzini wrote: > On Thu, May 30, 2024 at 11:07 PM Rick Edgecombe > <rick.p.edgecombe@intel.com> wrote: > > +static inline bool kvm_on_mirror(const struct kvm *kvm, enum kvm_process > > process) > > +{ > > + if (!kvm_has_mirrored_tdp(kvm)) > > + return false; > > + > > + return process & KVM_PROCESS_PRIVATE; > > +} > > + > > +static inline bool kvm_on_direct(const struct kvm *kvm, enum kvm_process > > process) > > +{ > > + if (!kvm_has_mirrored_tdp(kvm)) > > + return true; > > + > > + return process & KVM_PROCESS_SHARED; > > +} > > These helpers don't add much, it's easier to just write > kvm_process_to_root_types() as > > if (process & KVM_PROCESS_SHARED) > ret |= KVM_DIRECT_ROOTS; > if (process & KVM_PROCESS_PRIVATE) > ret |= kvm_has_mirror_tdp(kvm) ? KVM_MIRROR_ROOTS : KVM_DIRECT_ROOTS; > > WARN_ON_ONCE(!ret); > return ret; The point of kvm_on_mirror() and kvm_on_direct() was to try to centralize TDX specific checks in mmu.h. Whether that is a good idea aside, looking at it now I'm not sure if it even does a good job. I'll try it the way you suggest. > > (Here I chose to rename kvm_has_mirrored_tdp to kvm_has_mirror_tdp; > but if you prefer to call it kvm_has_external_tdp, it's just as > readable. Whatever floats your boat). > > > struct kvm *kvm = vcpu->kvm; > > - struct kvm_mmu_page *root = root_to_sp(vcpu->arch.mmu->root.hpa); > > + enum kvm_tdp_mmu_root_types root_type = > > tdp_mmu_get_fault_root_type(kvm, fault); > > + struct kvm_mmu_page *root = tdp_mmu_get_root(vcpu, root_type); > > Please make this > > struct kvm_mmu_page *root = tdp_mmu_get_root_for_fault(vcpu, fault); Yes, that seems clearer. [snip] > > > +static inline enum kvm_tdp_mmu_root_types > > tdp_mmu_get_fault_root_type(struct kvm *kvm, > > + struct > > kvm_page_fault *fault) > > +{ > > + if (fault->is_private) > > + return kvm_process_to_root_types(kvm, KVM_PROCESS_PRIVATE); > > + return KVM_DIRECT_ROOTS; > > +} > > + > > +static inline struct kvm_mmu_page *tdp_mmu_get_root(struct kvm_vcpu *vcpu, > > enum kvm_tdp_mmu_root_types type) > > +{ > > + if (type == KVM_MIRROR_ROOTS) > > + return root_to_sp(vcpu->arch.mmu->mirror_root_hpa); > > + return root_to_sp(vcpu->arch.mmu->root.hpa); > > +} > > static inline struct kvm_mmu_page * > tdp_mmu_get_root_for_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault > *fault) > { > hpa_t root_hpa; > if (unlikely(fault->is_private && kvm_has_mirror_tdp(kvm))) > root_hpa = vcpu->arch.mmu->mirror_root_hpa; > else > root_hpa = vcpu->arch.mmu->root.hpa; > return root_to_sp(root_hpa); > } I was not loving the amount of indirection here in the patch, but thought it centralized the logic a bit better. This way seems good, given that the actual logic is not that complex.
On Fri, Jun 7, 2024 at 10:27 PM Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote: > > static inline struct kvm_mmu_page * > > tdp_mmu_get_root_for_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault > > *fault) > > { > > hpa_t root_hpa; > > if (unlikely(fault->is_private && kvm_has_mirror_tdp(kvm))) > > root_hpa = vcpu->arch.mmu->mirror_root_hpa; > > else > > root_hpa = vcpu->arch.mmu->root.hpa; > > return root_to_sp(root_hpa); > > } > > I was not loving the amount of indirection here in the patch, but thought it > centralized the logic a bit better. This way seems good, given that the actual > logic is not that complex. My proposed implementation is a bit TDX-specific though... Something like this is more agnostic, and it exploits nicely the difference between fault->addr and fault->gfn: if (!kvm_gfn_direct_mask(kvm) || (gpa_to_gfn(fault->addr) & kvm_gfn_direct_mask(kvm)) root_hpa = vcpu->arch.mmu->root.hpa; else root_hpa = vcpu->arch.mmu->mirror_root_hpa; return root_to_sp(root_hpa); Paolo
On Sat, 2024-06-08 at 11:13 +0200, Paolo Bonzini wrote: > > I was not loving the amount of indirection here in the patch, but thought it > > centralized the logic a bit better. This way seems good, given that the > > actual > > logic is not that complex. > > My proposed implementation is a bit TDX-specific though... Something > like this is more agnostic, and it exploits nicely the difference > between fault->addr and fault->gfn: > > if (!kvm_gfn_direct_mask(kvm) || > (gpa_to_gfn(fault->addr) & kvm_gfn_direct_mask(kvm)) > root_hpa = vcpu->arch.mmu->root.hpa; > else > root_hpa = vcpu->arch.mmu->mirror_root_hpa; > return root_to_sp(root_hpa); Agreed that this is less TDX specific and it means that this part of the generic MMU code doesn't need to know that the mirror/direct matches to private vs shared. I don't love that it has such a complicated conditional for the normal VM case, though. Just for readability. The previous versions checked kvm_gfn_shared_mask() more readily in various open coded spots. In this v2 we tried to reduce this and instead always rely on the "private" concept to switch between the roots in the generic code. I think it's arguably a little easier to understand if we stick to a single way of deciding which root to use. But I don't feel like any of these solutions discussed is perfectly clean. So I'm ok taking the benefits you prefer. I guess doing bitwise operations when possible is kind of the KVM way, haha. :)
On Mon, Jun 10, 2024 at 2:09 AM Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote: > Agreed that this is less TDX specific and it means that this part of the generic > MMU code doesn't need to know that the mirror/direct matches to private vs > shared. I don't love that it has such a complicated conditional for the normal > VM case, though. Just for readability. > > The previous versions checked kvm_gfn_shared_mask() more readily in various open > coded spots. In this v2 we tried to reduce this and instead always rely on > the "private" concept to switch between the roots in the generic code. I think > it's arguably a little easier to understand if we stick to a single way of > deciding which root to use. But there isn't any other place that relies on is_private, right? So... > But I don't feel like any of these solutions discussed is perfectly clean. So > I'm ok taking the benefits you prefer. I guess doing bitwise operations when > possible is kind of the KVM way, haha. :) ... while I'm definitely guilty of that, :) it does seem the cleanest option to use fault->addr to go from struct kvm_page_fault to the kind of root. If you prefer, you can introduce a bool kvm_is_addr_direct(struct kvm *kvm, gpa_t gpa), and use it here as kvm_is_addr_direct(kvm, fault->addr). Maybe that's the best of both worlds. Paolo
On Mon, 2024-06-10 at 11:23 +0200, Paolo Bonzini wrote: > > The previous versions checked kvm_gfn_shared_mask() more readily in various > > open > > coded spots. In this v2 we tried to reduce this and instead always rely on > > the "private" concept to switch between the roots in the generic code. I > > think > > it's arguably a little easier to understand if we stick to a single way of > > deciding which root to use. > > But there isn't any other place that relies on is_private, right? So... I meant the "private" concept in general, like triggering off of KVM_PROCESS_PRIVATE to use the mirror. > > > But I don't feel like any of these solutions discussed is perfectly clean. > > So > > I'm ok taking the benefits you prefer. I guess doing bitwise operations when > > possible is kind of the KVM way, haha. :) > > ... while I'm definitely guilty of that, :) it does seem the cleanest > option to use fault->addr to go from struct kvm_page_fault to the kind > of root. > > If you prefer, you can introduce a bool kvm_is_addr_direct(struct kvm > *kvm, gpa_t gpa), and use it here as kvm_is_addr_direct(kvm, > fault->addr). Maybe that's the best of both worlds. Yes, I think a helper will make it easier for non-TDX readers.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index c9af963ab897..d4446dde0ace 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -470,6 +470,7 @@ struct kvm_mmu { int (*sync_spte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, int i); struct kvm_mmu_root_info root; + hpa_t mirror_root_hpa; union kvm_cpu_role cpu_role; union kvm_mmu_page_role root_role; diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h index f0713b6e4ee5..006f06463a27 100644 --- a/arch/x86/kvm/mmu.h +++ b/arch/x86/kvm/mmu.h @@ -328,4 +328,20 @@ static inline gfn_t kvm_gfn_direct_mask(const struct kvm *kvm) { return kvm->arch.gfn_direct_mask; } + +static inline bool kvm_on_mirror(const struct kvm *kvm, enum kvm_process process) +{ + if (!kvm_has_mirrored_tdp(kvm)) + return false; + + return process & KVM_PROCESS_PRIVATE; +} + +static inline bool kvm_on_direct(const struct kvm *kvm, enum kvm_process process) +{ + if (!kvm_has_mirrored_tdp(kvm)) + return true; + + return process & KVM_PROCESS_SHARED; +} #endif diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 12178945922f..01fb918612ae 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3701,7 +3701,9 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu) int r; if (tdp_mmu_enabled) { - kvm_tdp_mmu_alloc_root(vcpu); + if (kvm_has_mirrored_tdp(vcpu->kvm)) + kvm_tdp_mmu_alloc_root(vcpu, true); + kvm_tdp_mmu_alloc_root(vcpu, false); return 0; } @@ -6245,6 +6247,7 @@ static int __kvm_mmu_create(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) mmu->root.hpa = INVALID_PAGE; mmu->root.pgd = 0; + mmu->mirror_root_hpa = INVALID_PAGE; for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++) mmu->prev_roots[i] = KVM_MMU_ROOT_INFO_INVALID; @@ -7220,6 +7223,12 @@ int kvm_mmu_vendor_module_init(void) void kvm_mmu_destroy(struct kvm_vcpu *vcpu) { kvm_mmu_unload(vcpu); + if (tdp_mmu_enabled) { + read_lock(&vcpu->kvm->mmu_lock); + mmu_free_root_page(vcpu->kvm, &vcpu->arch.mmu->mirror_root_hpa, + NULL); + read_unlock(&vcpu->kvm->mmu_lock); + } free_mmu_pages(&vcpu->arch.root_mmu); free_mmu_pages(&vcpu->arch.guest_mmu); mmu_free_memory_caches(vcpu); diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 5e8f652cd8b1..0fc168a3fff1 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -95,10 +95,18 @@ void kvm_tdp_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *root) static bool tdp_mmu_root_match(struct kvm_mmu_page *root, enum kvm_tdp_mmu_root_types types) { + if (WARN_ON_ONCE(!(types & (KVM_DIRECT_ROOTS | KVM_MIRROR_ROOTS)))) + return false; + if ((types & KVM_VALID_ROOTS) && root->role.invalid) return false; - return true; + if ((types & KVM_DIRECT_ROOTS) && !is_mirror_sp(root)) + return true; + if ((types & KVM_MIRROR_ROOTS) && is_mirror_sp(root)) + return true; + + return false; } /* @@ -233,7 +241,7 @@ static void tdp_mmu_init_child_sp(struct kvm_mmu_page *child_sp, tdp_mmu_init_sp(child_sp, iter->sptep, iter->gfn, role); } -void kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu) +void kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu, bool mirror) { struct kvm_mmu *mmu = vcpu->arch.mmu; union kvm_mmu_page_role role = mmu->root_role; @@ -241,6 +249,9 @@ void kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu) struct kvm *kvm = vcpu->kvm; struct kvm_mmu_page *root; + if (mirror) + kvm_mmu_page_role_set_mirrored(&role); + /* * Check for an existing root before acquiring the pages lock to avoid * unnecessary serialization if multiple vCPUs are loading a new root. @@ -292,13 +303,17 @@ void kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu) * and actually consuming the root if it's invalidated after dropping * mmu_lock, and the root can't be freed as this vCPU holds a reference. */ - mmu->root.hpa = __pa(root->spt); - mmu->root.pgd = 0; + if (mirror) { + mmu->mirror_root_hpa = __pa(root->spt); + } else { + mmu->root.hpa = __pa(root->spt); + mmu->root.pgd = 0; + } } static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn, - u64 old_spte, u64 new_spte, int level, - bool shared); + u64 old_spte, u64 new_spte, + union kvm_mmu_page_role role, bool shared); static void tdp_account_mmu_page(struct kvm *kvm, struct kvm_mmu_page *sp) { @@ -425,7 +440,7 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared) REMOVED_SPTE, level); } handle_changed_spte(kvm, kvm_mmu_page_as_id(sp), gfn, - old_spte, REMOVED_SPTE, level, shared); + old_spte, REMOVED_SPTE, sp->role, shared); } call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback); @@ -438,7 +453,7 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared) * @gfn: the base GFN that was mapped by the SPTE * @old_spte: The value of the SPTE before the change * @new_spte: The value of the SPTE after the change - * @level: the level of the PT the SPTE is part of in the paging structure + * @role: the role of the PT the SPTE is part of in the paging structure * @shared: This operation may not be running under the exclusive use of * the MMU lock and the operation must synchronize with other * threads that might be modifying SPTEs. @@ -448,9 +463,11 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared) * and fast_pf_fix_direct_spte()). */ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn, - u64 old_spte, u64 new_spte, int level, - bool shared) + u64 old_spte, u64 new_spte, + union kvm_mmu_page_role role, bool shared) { + bool is_mirror = kvm_mmu_page_role_is_mirror(role); + int level = role.level; bool was_present = is_shadow_present_pte(old_spte); bool is_present = is_shadow_present_pte(new_spte); bool was_leaf = was_present && is_last_spte(old_spte, level); @@ -531,8 +548,11 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn, * pages are kernel allocations and should never be migrated. */ if (was_present && !was_leaf && - (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed))) + (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed))) { + KVM_BUG_ON(is_mirror != + is_mirror_sptep(spte_to_child_pt(old_spte, level)), kvm); handle_removed_pt(kvm, spte_to_child_pt(old_spte, level), shared); + } if (was_leaf && is_accessed_spte(old_spte) && (!is_present || !is_accessed_spte(new_spte) || pfn_changed)) @@ -585,6 +605,7 @@ static inline int tdp_mmu_set_spte_atomic(struct kvm *kvm, struct tdp_iter *iter, u64 new_spte) { + u64 *sptep = rcu_dereference(iter->sptep); int ret; lockdep_assert_held_read(&kvm->mmu_lock); @@ -594,7 +615,7 @@ static inline int tdp_mmu_set_spte_atomic(struct kvm *kvm, return ret; handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte, - new_spte, iter->level, true); + new_spte, sptep_to_sp(sptep)->role, true); return 0; } @@ -602,6 +623,7 @@ static inline int tdp_mmu_set_spte_atomic(struct kvm *kvm, static inline int tdp_mmu_zap_spte_atomic(struct kvm *kvm, struct tdp_iter *iter) { + union kvm_mmu_page_role role; int ret; lockdep_assert_held_read(&kvm->mmu_lock); @@ -628,6 +650,7 @@ static inline int tdp_mmu_zap_spte_atomic(struct kvm *kvm, */ __kvm_tdp_mmu_write_spte(iter->sptep, SHADOW_NONPRESENT_VALUE); + role = sptep_to_sp(iter->sptep)->role; /* * Process the zapped SPTE after flushing TLBs, and after replacing * REMOVED_SPTE with 0. This minimizes the amount of time vCPUs are @@ -635,7 +658,7 @@ static inline int tdp_mmu_zap_spte_atomic(struct kvm *kvm, * SPTEs. */ handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte, - 0, iter->level, true); + SHADOW_NONPRESENT_VALUE, role, true); return 0; } @@ -657,6 +680,8 @@ static inline int tdp_mmu_zap_spte_atomic(struct kvm *kvm, static u64 tdp_mmu_set_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep, u64 old_spte, u64 new_spte, gfn_t gfn, int level) { + union kvm_mmu_page_role role; + lockdep_assert_held_write(&kvm->mmu_lock); /* @@ -670,7 +695,9 @@ static u64 tdp_mmu_set_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep, old_spte = kvm_tdp_mmu_write_spte(sptep, old_spte, new_spte, level); - handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level, false); + role = sptep_to_sp(sptep)->role; + role.level = level; + handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, role, false); return old_spte; } @@ -1114,7 +1141,8 @@ static int tdp_mmu_split_huge_page(struct kvm *kvm, struct tdp_iter *iter, int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) { struct kvm *kvm = vcpu->kvm; - struct kvm_mmu_page *root = root_to_sp(vcpu->arch.mmu->root.hpa); + enum kvm_tdp_mmu_root_types root_type = tdp_mmu_get_fault_root_type(kvm, fault); + struct kvm_mmu_page *root = tdp_mmu_get_root(vcpu, root_type); struct tdp_iter iter; struct kvm_mmu_page *sp; int ret = RET_PF_RETRY; @@ -1151,13 +1179,18 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) */ sp = tdp_mmu_alloc_sp(vcpu); tdp_mmu_init_child_sp(sp, &iter); + if (is_mirror_sp(sp)) + kvm_mmu_alloc_mirrored_spt(vcpu, sp); sp->nx_huge_page_disallowed = fault->huge_page_disallowed; - if (is_shadow_present_pte(iter.old_spte)) + if (is_shadow_present_pte(iter.old_spte)) { + /* Don't support large page for mirrored roots (TDX) */ + KVM_BUG_ON(is_mirror_sptep(iter.sptep), vcpu->kvm); r = tdp_mmu_split_huge_page(kvm, &iter, sp, true); - else + } else { r = tdp_mmu_link_sp(kvm, &iter, sp, true); + } /* * Force the guest to retry if installing an upper level SPTE @@ -1812,7 +1845,8 @@ int kvm_tdp_mmu_get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes, u64 *kvm_tdp_mmu_fast_pf_get_last_sptep(struct kvm_vcpu *vcpu, u64 addr, u64 *spte) { - struct kvm_mmu_page *root = root_to_sp(vcpu->arch.mmu->root.hpa); + /* Fast pf is not supported for mirrored roots */ + struct kvm_mmu_page *root = tdp_mmu_get_root(vcpu, KVM_DIRECT_ROOTS); struct tdp_iter iter; gfn_t gfn = addr >> PAGE_SHIFT; tdp_ptep_t sptep = NULL; diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h index e7055a5333a8..934269b82f20 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.h +++ b/arch/x86/kvm/mmu/tdp_mmu.h @@ -10,7 +10,7 @@ void kvm_mmu_init_tdp_mmu(struct kvm *kvm); void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm); -void kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu); +void kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu, bool private); __must_check static inline bool kvm_tdp_mmu_get_root(struct kvm_mmu_page *root) { @@ -21,11 +21,44 @@ void kvm_tdp_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *root); enum kvm_tdp_mmu_root_types { KVM_VALID_ROOTS = BIT(0), + KVM_DIRECT_ROOTS = BIT(1), + KVM_MIRROR_ROOTS = BIT(2), - KVM_ANY_ROOTS = 0, - KVM_ANY_VALID_ROOTS = KVM_VALID_ROOTS, + KVM_ANY_ROOTS = KVM_DIRECT_ROOTS | KVM_MIRROR_ROOTS, + KVM_ANY_VALID_ROOTS = KVM_DIRECT_ROOTS | KVM_MIRROR_ROOTS | KVM_VALID_ROOTS, }; +static inline enum kvm_tdp_mmu_root_types kvm_process_to_root_types(struct kvm *kvm, + enum kvm_process process) +{ + enum kvm_tdp_mmu_root_types ret = 0; + + WARN_ON_ONCE(process == BUGGY_KVM_INVALIDATION); + + if (kvm_on_mirror(kvm, process)) + ret |= KVM_MIRROR_ROOTS; + + if (kvm_on_direct(kvm, process)) + ret |= KVM_DIRECT_ROOTS; + + return ret; +} + +static inline enum kvm_tdp_mmu_root_types tdp_mmu_get_fault_root_type(struct kvm *kvm, + struct kvm_page_fault *fault) +{ + if (fault->is_private) + return kvm_process_to_root_types(kvm, KVM_PROCESS_PRIVATE); + return KVM_DIRECT_ROOTS; +} + +static inline struct kvm_mmu_page *tdp_mmu_get_root(struct kvm_vcpu *vcpu, enum kvm_tdp_mmu_root_types type) +{ + if (type == KVM_MIRROR_ROOTS) + return root_to_sp(vcpu->arch.mmu->mirror_root_hpa); + return root_to_sp(vcpu->arch.mmu->root.hpa); +} + bool kvm_tdp_mmu_zap_leafs(struct kvm *kvm, gfn_t start, gfn_t end, bool flush); bool kvm_tdp_mmu_zap_sp(struct kvm *kvm, struct kvm_mmu_page *sp); void kvm_tdp_mmu_zap_all(struct kvm *kvm);