| Message ID | 20220805230513.148869-4-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Apply NX mitigation more precisely | expand |
On Fri, Aug 05, 2022, Sean Christopherson wrote: > Rename most of the variables/functions involved in the NX huge page > mitigation to provide consistency, e.g. lpage vs huge page, and NX huge > vs huge NX, and also to provide clarity, e.g. to make it obvious the flag > applies only to the NX huge page mitigation, not to any condition that > prevents creating a huge page. > > Leave the nx_lpage_splits stat alone as the name is ABI and thus set in > stone. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_host.h | 8 ++-- > arch/x86/kvm/mmu/mmu.c | 70 +++++++++++++++++---------------- > arch/x86/kvm/mmu/mmu_internal.h | 22 +++++++---- > arch/x86/kvm/mmu/paging_tmpl.h | 2 +- > arch/x86/kvm/mmu/tdp_mmu.c | 8 ++-- > 5 files changed, 59 insertions(+), 51 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index e8281d64a431..5634347e5d05 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1143,7 +1143,7 @@ struct kvm_arch { > struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES]; > struct list_head active_mmu_pages; > struct list_head zapped_obsolete_pages; > - struct list_head lpage_disallowed_mmu_pages; > + struct list_head possible_nx_huge_pages; Honestly, I am struggling to understand this one. 'possible_*' indicates that there are other possibilities. But what are those possibilities? I feel this name is more confusing than the original one. Maybe just keep the original name? > struct kvm_page_track_notifier_node mmu_sp_tracker; > struct kvm_page_track_notifier_head track_notifier_head; > /* > @@ -1259,7 +1259,7 @@ struct kvm_arch { > bool sgx_provisioning_allowed; > > struct kvm_pmu_event_filter __rcu *pmu_event_filter; > - struct task_struct *nx_lpage_recovery_thread; > + struct task_struct *nx_huge_page_recovery_thread; > > #ifdef CONFIG_X86_64 > /* > @@ -1304,8 +1304,8 @@ struct kvm_arch { > * - tdp_mmu_roots (above) > * - tdp_mmu_pages (above) > * - the link field of struct kvm_mmu_pages used by the TDP MMU > - * - lpage_disallowed_mmu_pages > - * - the lpage_disallowed_link field of struct kvm_mmu_pages used > + * - possible_nx_huge_pages; > + * - the possible_nx_huge_page_link field of struct kvm_mmu_pages used > * by the TDP MMU > * It is acceptable, but not necessary, to acquire this lock when > * the thread holds the MMU lock in write mode. > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 55dac44f3397..53d0dafa68ff 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -802,20 +802,20 @@ static void account_shadowed(struct kvm *kvm, struct kvm_mmu_page *sp) > kvm_flush_remote_tlbs_with_address(kvm, gfn, 1); > } > > -void account_huge_nx_page(struct kvm *kvm, struct kvm_mmu_page *sp, > +void account_nx_huge_page(struct kvm *kvm, struct kvm_mmu_page *sp, > bool nx_huge_page_possible) > { > - if (KVM_BUG_ON(!list_empty(&sp->lpage_disallowed_link), kvm)) > + if (KVM_BUG_ON(!list_empty(&sp->possible_nx_huge_page_link), kvm)) > return; > > - sp->lpage_disallowed = true; > + sp->nx_huge_page_disallowed = true; > > if (!nx_huge_page_possible) > return; > > ++kvm->stat.nx_lpage_splits; > - list_add_tail(&sp->lpage_disallowed_link, > - &kvm->arch.lpage_disallowed_mmu_pages); > + list_add_tail(&sp->possible_nx_huge_page_link, > + &kvm->arch.possible_nx_huge_pages); > } > > static void unaccount_shadowed(struct kvm *kvm, struct kvm_mmu_page *sp) > @@ -835,15 +835,15 @@ static void unaccount_shadowed(struct kvm *kvm, struct kvm_mmu_page *sp) > kvm_mmu_gfn_allow_lpage(slot, gfn); > } > > -void unaccount_huge_nx_page(struct kvm *kvm, struct kvm_mmu_page *sp) > +void unaccount_nx_huge_page(struct kvm *kvm, struct kvm_mmu_page *sp) > { > - sp->lpage_disallowed = false; > + sp->nx_huge_page_disallowed = false; > > - if (list_empty(&sp->lpage_disallowed_link)) > + if (list_empty(&sp->possible_nx_huge_page_link)) > return; > > --kvm->stat.nx_lpage_splits; > - list_del_init(&sp->lpage_disallowed_link); > + list_del_init(&sp->possible_nx_huge_page_link); > } > > static struct kvm_memory_slot * > @@ -2124,7 +2124,7 @@ static struct kvm_mmu_page *kvm_mmu_alloc_shadow_page(struct kvm *kvm, > > set_page_private(virt_to_page(sp->spt), (unsigned long)sp); > > - INIT_LIST_HEAD(&sp->lpage_disallowed_link); > + INIT_LIST_HEAD(&sp->possible_nx_huge_page_link); > > /* > * active_mmu_pages must be a FIFO list, as kvm_zap_obsolete_pages() > @@ -2483,8 +2483,8 @@ static bool __kvm_mmu_prepare_zap_page(struct kvm *kvm, > zapped_root = !is_obsolete_sp(kvm, sp); > } > > - if (sp->lpage_disallowed) > - unaccount_huge_nx_page(kvm, sp); > + if (sp->nx_huge_page_disallowed) > + unaccount_nx_huge_page(kvm, sp); > > sp->role.invalid = 1; > > @@ -3124,7 +3124,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > > link_shadow_page(vcpu, it.sptep, sp); > if (fault->is_tdp && fault->huge_page_disallowed) > - account_huge_nx_page(vcpu->kvm, sp, > + account_nx_huge_page(vcpu->kvm, sp, > fault->req_level >= it.level); > } > > @@ -5981,7 +5981,7 @@ int kvm_mmu_init_vm(struct kvm *kvm) > > INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); > INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); > - INIT_LIST_HEAD(&kvm->arch.lpage_disallowed_mmu_pages); > + INIT_LIST_HEAD(&kvm->arch.possible_nx_huge_pages); > spin_lock_init(&kvm->arch.mmu_unsync_pages_lock); > > r = kvm_mmu_init_tdp_mmu(kvm); > @@ -6699,7 +6699,7 @@ static int set_nx_huge_pages(const char *val, const struct kernel_param *kp) > kvm_mmu_zap_all_fast(kvm); > mutex_unlock(&kvm->slots_lock); > > - wake_up_process(kvm->arch.nx_lpage_recovery_thread); > + wake_up_process(kvm->arch.nx_huge_page_recovery_thread); > } > mutex_unlock(&kvm_lock); > } > @@ -6825,7 +6825,7 @@ static int set_nx_huge_pages_recovery_param(const char *val, const struct kernel > mutex_lock(&kvm_lock); > > list_for_each_entry(kvm, &vm_list, vm_list) > - wake_up_process(kvm->arch.nx_lpage_recovery_thread); > + wake_up_process(kvm->arch.nx_huge_page_recovery_thread); > > mutex_unlock(&kvm_lock); > } > @@ -6833,7 +6833,7 @@ static int set_nx_huge_pages_recovery_param(const char *val, const struct kernel > return err; > } > > -static void kvm_recover_nx_lpages(struct kvm *kvm) > +static void kvm_recover_nx_huge_pages(struct kvm *kvm) > { > unsigned long nx_lpage_splits = kvm->stat.nx_lpage_splits; > int rcu_idx; > @@ -6856,23 +6856,25 @@ static void kvm_recover_nx_lpages(struct kvm *kvm) > ratio = READ_ONCE(nx_huge_pages_recovery_ratio); > to_zap = ratio ? DIV_ROUND_UP(nx_lpage_splits, ratio) : 0; > for ( ; to_zap; --to_zap) { > - if (list_empty(&kvm->arch.lpage_disallowed_mmu_pages)) > + if (list_empty(&kvm->arch.possible_nx_huge_pages)) > break; > > /* > * We use a separate list instead of just using active_mmu_pages > - * because the number of lpage_disallowed pages is expected to > - * be relatively small compared to the total. > + * because the number of shadow pages that be replaced with an > + * NX huge page is expected to be relatively small compared to > + * the total number of shadow pages. And because the TDP MMU > + * doesn't use active_mmu_pages. > */ > - sp = list_first_entry(&kvm->arch.lpage_disallowed_mmu_pages, > + sp = list_first_entry(&kvm->arch.possible_nx_huge_pages, > struct kvm_mmu_page, > - lpage_disallowed_link); > - WARN_ON_ONCE(!sp->lpage_disallowed); > + possible_nx_huge_page_link); > + WARN_ON_ONCE(!sp->nx_huge_page_disallowed); > if (is_tdp_mmu_page(sp)) { > flush |= kvm_tdp_mmu_zap_sp(kvm, sp); > } else { > kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list); > - WARN_ON_ONCE(sp->lpage_disallowed); > + WARN_ON_ONCE(sp->nx_huge_page_disallowed); > } > > if (need_resched() || rwlock_needbreak(&kvm->mmu_lock)) { > @@ -6893,7 +6895,7 @@ static void kvm_recover_nx_lpages(struct kvm *kvm) > srcu_read_unlock(&kvm->srcu, rcu_idx); > } > > -static long get_nx_lpage_recovery_timeout(u64 start_time) > +static long get_nx_huge_page_recovery_timeout(u64 start_time) > { > bool enabled; > uint period; > @@ -6904,19 +6906,19 @@ static long get_nx_lpage_recovery_timeout(u64 start_time) > : MAX_SCHEDULE_TIMEOUT; > } > > -static int kvm_nx_lpage_recovery_worker(struct kvm *kvm, uintptr_t data) > +static int kvm_nx_huge_page_recovery_worker(struct kvm *kvm, uintptr_t data) > { > u64 start_time; > long remaining_time; > > while (true) { > start_time = get_jiffies_64(); > - remaining_time = get_nx_lpage_recovery_timeout(start_time); > + remaining_time = get_nx_huge_page_recovery_timeout(start_time); > > set_current_state(TASK_INTERRUPTIBLE); > while (!kthread_should_stop() && remaining_time > 0) { > schedule_timeout(remaining_time); > - remaining_time = get_nx_lpage_recovery_timeout(start_time); > + remaining_time = get_nx_huge_page_recovery_timeout(start_time); > set_current_state(TASK_INTERRUPTIBLE); > } > > @@ -6925,7 +6927,7 @@ static int kvm_nx_lpage_recovery_worker(struct kvm *kvm, uintptr_t data) > if (kthread_should_stop()) > return 0; > > - kvm_recover_nx_lpages(kvm); > + kvm_recover_nx_huge_pages(kvm); > } > } > > @@ -6933,17 +6935,17 @@ int kvm_mmu_post_init_vm(struct kvm *kvm) > { > int err; > > - err = kvm_vm_create_worker_thread(kvm, kvm_nx_lpage_recovery_worker, 0, > + err = kvm_vm_create_worker_thread(kvm, kvm_nx_huge_page_recovery_worker, 0, > "kvm-nx-lpage-recovery", > - &kvm->arch.nx_lpage_recovery_thread); > + &kvm->arch.nx_huge_page_recovery_thread); > if (!err) > - kthread_unpark(kvm->arch.nx_lpage_recovery_thread); > + kthread_unpark(kvm->arch.nx_huge_page_recovery_thread); > > return err; > } > > void kvm_mmu_pre_destroy_vm(struct kvm *kvm) > { > - if (kvm->arch.nx_lpage_recovery_thread) > - kthread_stop(kvm->arch.nx_lpage_recovery_thread); > + if (kvm->arch.nx_huge_page_recovery_thread) > + kthread_stop(kvm->arch.nx_huge_page_recovery_thread); > } > diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h > index cca1ad75d096..67879459a25c 100644 > --- a/arch/x86/kvm/mmu/mmu_internal.h > +++ b/arch/x86/kvm/mmu/mmu_internal.h > @@ -57,7 +57,13 @@ struct kvm_mmu_page { > bool tdp_mmu_page; > bool unsync; > u8 mmu_valid_gen; > - bool lpage_disallowed; /* Can't be replaced by an equiv large page */ > + > + /* > + * The shadow page can't be replaced by an equivalent huge page > + * because it is being used to map an executable page in the guest > + * and the NX huge page mitigation is enabled. > + */ > + bool nx_huge_page_disallowed; > > /* > * The following two entries are used to key the shadow page in the > @@ -102,12 +108,12 @@ struct kvm_mmu_page { > > /* > * Tracks shadow pages that, if zapped, would allow KVM to create an NX > - * huge page. A shadow page will have lpage_disallowed set but not be > - * on the list if a huge page is disallowed for other reasons, e.g. > - * because KVM is shadowing a PTE at the same gfn, the memslot isn't > - * properly aligned, etc... > + * huge page. A shadow page will have nx_huge_page_disallowed set but > + * not be on the list if a huge page is disallowed for other reasons, > + * e.g. because KVM is shadowing a PTE at the same gfn, the memslot > + * isn't properly aligned, etc... > */ > - struct list_head lpage_disallowed_link; > + struct list_head possible_nx_huge_page_link; > #ifdef CONFIG_X86_32 > /* > * Used out of the mmu-lock to avoid reading spte values while an > @@ -322,8 +328,8 @@ void disallowed_hugepage_adjust(struct kvm_page_fault *fault, u64 spte, int cur_ > > void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc); > > -void account_huge_nx_page(struct kvm *kvm, struct kvm_mmu_page *sp, > +void account_nx_huge_page(struct kvm *kvm, struct kvm_mmu_page *sp, > bool nx_huge_page_possible); > -void unaccount_huge_nx_page(struct kvm *kvm, struct kvm_mmu_page *sp); > +void unaccount_nx_huge_page(struct kvm *kvm, struct kvm_mmu_page *sp); > > #endif /* __KVM_X86_MMU_INTERNAL_H */ > diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h > index e450f49f2225..259c0f019f09 100644 > --- a/arch/x86/kvm/mmu/paging_tmpl.h > +++ b/arch/x86/kvm/mmu/paging_tmpl.h > @@ -714,7 +714,7 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault, > > link_shadow_page(vcpu, it.sptep, sp); > if (fault->huge_page_disallowed) > - account_huge_nx_page(vcpu->kvm, sp, > + account_nx_huge_page(vcpu->kvm, sp, > fault->req_level >= it.level); > } > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 903d0d3497b6..0e94182c87be 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -284,7 +284,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp(struct kvm_vcpu *vcpu) > static void tdp_mmu_init_sp(struct kvm_mmu_page *sp, tdp_ptep_t sptep, > gfn_t gfn, union kvm_mmu_page_role role) > { > - INIT_LIST_HEAD(&sp->lpage_disallowed_link); > + INIT_LIST_HEAD(&sp->possible_nx_huge_page_link); > > set_page_private(virt_to_page(sp->spt), (unsigned long)sp); > > @@ -392,8 +392,8 @@ static void tdp_mmu_unlink_sp(struct kvm *kvm, struct kvm_mmu_page *sp, > lockdep_assert_held_write(&kvm->mmu_lock); > > list_del(&sp->link); > - if (sp->lpage_disallowed) > - unaccount_huge_nx_page(kvm, sp); > + if (sp->nx_huge_page_disallowed) > + unaccount_nx_huge_page(kvm, sp); > > if (shared) > spin_unlock(&kvm->arch.tdp_mmu_pages_lock); > @@ -1132,7 +1132,7 @@ static int tdp_mmu_link_sp(struct kvm *kvm, struct tdp_iter *iter, > spin_lock(&kvm->arch.tdp_mmu_pages_lock); > list_add(&sp->link, &kvm->arch.tdp_mmu_pages); > if (account_nx) > - account_huge_nx_page(kvm, sp, true); > + account_nx_huge_page(kvm, sp, true); > spin_unlock(&kvm->arch.tdp_mmu_pages_lock); > > return 0; > -- > 2.37.1.559.g78731f0fdb-goog >
On Sun, Aug 14, 2022, Mingwei Zhang wrote: > On Fri, Aug 05, 2022, Sean Christopherson wrote: > > Rename most of the variables/functions involved in the NX huge page > > mitigation to provide consistency, e.g. lpage vs huge page, and NX huge > > vs huge NX, and also to provide clarity, e.g. to make it obvious the flag > > applies only to the NX huge page mitigation, not to any condition that > > prevents creating a huge page. > > > > Leave the nx_lpage_splits stat alone as the name is ABI and thus set in > > stone. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/include/asm/kvm_host.h | 8 ++-- > > arch/x86/kvm/mmu/mmu.c | 70 +++++++++++++++++---------------- > > arch/x86/kvm/mmu/mmu_internal.h | 22 +++++++---- > > arch/x86/kvm/mmu/paging_tmpl.h | 2 +- > > arch/x86/kvm/mmu/tdp_mmu.c | 8 ++-- > > 5 files changed, 59 insertions(+), 51 deletions(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index e8281d64a431..5634347e5d05 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -1143,7 +1143,7 @@ struct kvm_arch { > > struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES]; > > struct list_head active_mmu_pages; > > struct list_head zapped_obsolete_pages; > > - struct list_head lpage_disallowed_mmu_pages; > > + struct list_head possible_nx_huge_pages; > > Honestly, I am struggling to understand this one. 'possible_*' indicates > that there are other possibilities. But what are those possibilities? No, possible is being used as an adjective in this case. possible_nx_huge_pages is the list of shadow pages for which it's possible to replace the shadow page with an NX huge page. The noun version would yield a name like nx_huge_page_possiblities. > I feel this name is more confusing than the original one. Maybe just keep Ignoring lpage => huge_page, the current name is terribly inaccurate. The list doesn't contain all disallowed huge pages, nor does it even contain all disallowed NX huge pages, it specifically tracks shadow pages that might be able to be replaced with an NX huge page. I'm open to other names, but whatever we choose should be paired with account_nx_huge_page()'s param that is currently named "nx_huge_page_possible".
On Mon, Aug 15, 2022, Sean Christopherson wrote: > On Sun, Aug 14, 2022, Mingwei Zhang wrote: > > On Fri, Aug 05, 2022, Sean Christopherson wrote: > > > Rename most of the variables/functions involved in the NX huge page > > > mitigation to provide consistency, e.g. lpage vs huge page, and NX huge > > > vs huge NX, and also to provide clarity, e.g. to make it obvious the flag > > > applies only to the NX huge page mitigation, not to any condition that > > > prevents creating a huge page. > > > > > > Leave the nx_lpage_splits stat alone as the name is ABI and thus set in > > > stone. > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > arch/x86/include/asm/kvm_host.h | 8 ++-- > > > arch/x86/kvm/mmu/mmu.c | 70 +++++++++++++++++---------------- > > > arch/x86/kvm/mmu/mmu_internal.h | 22 +++++++---- > > > arch/x86/kvm/mmu/paging_tmpl.h | 2 +- > > > arch/x86/kvm/mmu/tdp_mmu.c | 8 ++-- > > > 5 files changed, 59 insertions(+), 51 deletions(-) > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > index e8281d64a431..5634347e5d05 100644 > > > --- a/arch/x86/include/asm/kvm_host.h > > > +++ b/arch/x86/include/asm/kvm_host.h > > > @@ -1143,7 +1143,7 @@ struct kvm_arch { > > > struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES]; > > > struct list_head active_mmu_pages; > > > struct list_head zapped_obsolete_pages; > > > - struct list_head lpage_disallowed_mmu_pages; > > > + struct list_head possible_nx_huge_pages; > > > > Honestly, I am struggling to understand this one. 'possible_*' indicates > > that there are other possibilities. But what are those possibilities? > > No, possible is being used as an adjective in this case. possible_nx_huge_pages > is the list of shadow pages for which it's possible to replace the shadow page > with an NX huge page. > > The noun version would yield a name like nx_huge_page_possiblities. Right, but these shadow pages are not NX huge pages, right? IIUC, they are pages to be zapped due to NX huge pages, aren't they? `nx_huge_page_disallowed` is easy to understand because it literally say 'nx_huge_page is not allowed', which is correct. But this one, it says 'possible nx_huge_pages', but they are not nx huge pages at all. Instead, they are 'shadow pages that are replaced with nx_huge_pages'. So that's why updates to this list is done together with stats nx_plage_splits. Please correct me if I am wrong. I am still struggling to understand the meaning of these variables. > > > I feel this name is more confusing than the original one. Maybe just keep > > Ignoring lpage => huge_page, the current name is terribly inaccurate. The list > doesn't contain all disallowed huge pages, nor does it even contain all disallowed > NX huge pages, it specifically tracks shadow pages that might be able to be > replaced with an NX huge page. > > I'm open to other names, but whatever we choose should be paired with > account_nx_huge_page()'s param that is currently named "nx_huge_page_possible". How about mmu_pages_replaced_by_nx_huge, mmu_pages_replaced_by_possible_nx_huge or something starting with possible_pages_, pages_ instead of possible_nx_huge_pages?
On Tue, Aug 16, 2022, Mingwei Zhang wrote: > On Mon, Aug 15, 2022, Sean Christopherson wrote: > > On Sun, Aug 14, 2022, Mingwei Zhang wrote: > > > On Fri, Aug 05, 2022, Sean Christopherson wrote: > > > > Rename most of the variables/functions involved in the NX huge page > > > > mitigation to provide consistency, e.g. lpage vs huge page, and NX huge > > > > vs huge NX, and also to provide clarity, e.g. to make it obvious the flag > > > > applies only to the NX huge page mitigation, not to any condition that > > > > prevents creating a huge page. > > > > > > > > Leave the nx_lpage_splits stat alone as the name is ABI and thus set in > > > > stone. > > > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > > --- > > > > arch/x86/include/asm/kvm_host.h | 8 ++-- > > > > arch/x86/kvm/mmu/mmu.c | 70 +++++++++++++++++---------------- > > > > arch/x86/kvm/mmu/mmu_internal.h | 22 +++++++---- > > > > arch/x86/kvm/mmu/paging_tmpl.h | 2 +- > > > > arch/x86/kvm/mmu/tdp_mmu.c | 8 ++-- > > > > 5 files changed, 59 insertions(+), 51 deletions(-) > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > > index e8281d64a431..5634347e5d05 100644 > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > @@ -1143,7 +1143,7 @@ struct kvm_arch { > > > > struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES]; > > > > struct list_head active_mmu_pages; > > > > struct list_head zapped_obsolete_pages; > > > > - struct list_head lpage_disallowed_mmu_pages; > > > > + struct list_head possible_nx_huge_pages; > > > > > > Honestly, I am struggling to understand this one. 'possible_*' indicates > > > that there are other possibilities. But what are those possibilities? > > > > No, possible is being used as an adjective in this case. possible_nx_huge_pages > > is the list of shadow pages for which it's possible to replace the shadow page > > with an NX huge page. > > > > The noun version would yield a name like nx_huge_page_possiblities. > > Right, but these shadow pages are not NX huge pages, right? IIUC, they > are pages to be zapped due to NX huge pages, aren't they? Yes, they are shadow pages that the NX recovery thread should zap, but the reason they should be zapped is because (a) the shadow page has at least one execute child SPTE, (b) zapping the shadow page will also zap its child SPTEs, and (c) eliminating all executable child SPTEs means KVM _might_ be able to instantiate an NX huge page. > `nx_huge_page_disallowed` is easy to understand because it literally say > 'nx_huge_page is not allowed', which is correct. No, it's not correct. The list isn't simply the set of shadow pages that disallow NX huge pages, it's the set of shadow pages that disallow NX huge pages _and_ that can possibly be replaced by an NX huge page if the shadow page and all its (executable) children go away. > But this one, it says 'possible nx_huge_pages', but they are not > nx huge pages at all. Yes, but they _can be_ NX huge pages, hence the "possible". A super verbose name would be something like mmu_pages_that_can_possibly_be_replaced_by_nx_huge_pages. > Instead, they are 'shadow pages that are replaced with nx_huge_pages'. So > that's why updates to this list is done together with stats nx_plage_splits. > > Please correct me if I am wrong. I am still struggling to understand the > meaning of these variables. > > > > > > I feel this name is more confusing than the original one. Maybe just keep > > > > Ignoring lpage => huge_page, the current name is terribly inaccurate. The list > > doesn't contain all disallowed huge pages, nor does it even contain all disallowed > > NX huge pages, it specifically tracks shadow pages that might be able to be > > replaced with an NX huge page. > > > > I'm open to other names, but whatever we choose should be paired with > > account_nx_huge_page()'s param that is currently named "nx_huge_page_possible". > > How about mmu_pages_replaced_by_nx_huge, "replaced" is past tense in this usage, and these are definitely not shadow pages that have already been replaced by NX huge pages. > mmu_pages_replaced_by_possible_nx_huge or something starting with Same issue with "replaced". > possible_pages_, pages_ instead of possible_nx_huge_pages? Reprhasing, I think you're asking that the variable have "mmu_pages" somewhere in the name to clarify that it's a list of kvm_mmu_page structs. I agree that ideally it would have "mmu_pages" somewhere in there, but I also think that having both "nx_huge_pages" and "mmu_pages" in the name makes it unreadable, i.e. we have to choose one use of "pages". The only alternative I can come up with that isn't completely gross is possible_nx_huge_mmu_pages. I can't tell if that's less confusing or more confusing.
On Wed, Aug 17, 2022, Sean Christopherson wrote: > On Tue, Aug 16, 2022, Mingwei Zhang wrote: > > On Mon, Aug 15, 2022, Sean Christopherson wrote: > > > On Sun, Aug 14, 2022, Mingwei Zhang wrote: > > > > On Fri, Aug 05, 2022, Sean Christopherson wrote: > > > > > Rename most of the variables/functions involved in the NX huge page > > > > > mitigation to provide consistency, e.g. lpage vs huge page, and NX huge > > > > > vs huge NX, and also to provide clarity, e.g. to make it obvious the flag > > > > > applies only to the NX huge page mitigation, not to any condition that > > > > > prevents creating a huge page. > > > > > > > > > > Leave the nx_lpage_splits stat alone as the name is ABI and thus set in > > > > > stone. > > > > > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > > > --- > > > > > arch/x86/include/asm/kvm_host.h | 8 ++-- > > > > > arch/x86/kvm/mmu/mmu.c | 70 +++++++++++++++++---------------- > > > > > arch/x86/kvm/mmu/mmu_internal.h | 22 +++++++---- > > > > > arch/x86/kvm/mmu/paging_tmpl.h | 2 +- > > > > > arch/x86/kvm/mmu/tdp_mmu.c | 8 ++-- > > > > > 5 files changed, 59 insertions(+), 51 deletions(-) > > > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > > > index e8281d64a431..5634347e5d05 100644 > > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > > @@ -1143,7 +1143,7 @@ struct kvm_arch { > > > > > struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES]; > > > > > struct list_head active_mmu_pages; > > > > > struct list_head zapped_obsolete_pages; > > > > > - struct list_head lpage_disallowed_mmu_pages; > > > > > + struct list_head possible_nx_huge_pages; > > > > > > > > Honestly, I am struggling to understand this one. 'possible_*' indicates > > > > that there are other possibilities. But what are those possibilities? > > > > > > No, possible is being used as an adjective in this case. possible_nx_huge_pages > > > is the list of shadow pages for which it's possible to replace the shadow page > > > with an NX huge page. > > > > > > The noun version would yield a name like nx_huge_page_possiblities. > > > > Right, but these shadow pages are not NX huge pages, right? IIUC, they > > are pages to be zapped due to NX huge pages, aren't they? > > Yes, they are shadow pages that the NX recovery thread should zap, but the reason > they should be zapped is because (a) the shadow page has at least one execute child > SPTE, (b) zapping the shadow page will also zap its child SPTEs, and (c) eliminating > all executable child SPTEs means KVM _might_ be able to instantiate an NX huge page. > oh, I scratched my head and finaly got your point. hmm. So the shadow pages are the 'blockers' to (re)create a NX huge page because of at least one present child executable spte. So, really, whether these shadow pages themselves are NX huge or not does not really matter. All we need to know is that they will be zapped in the future to help making recovery of an NX huge page possible. > > `nx_huge_page_disallowed` is easy to understand because it literally say > > 'nx_huge_page is not allowed', which is correct. > > No, it's not correct. The list isn't simply the set of shadow pages that disallow > NX huge pages, it's the set of shadow pages that disallow NX huge pages _and_ that > can possibly be replaced by an NX huge page if the shadow page and all its > (executable) children go away. > hmm, I think this naming is correct. The flag is used to talk to the 'fault handler' to say 'hey, don't create nx huge page, stupid'. Of course, it is also used to by the 'nx huge recovery thread', but the recovery thread will only check it for sanity purpose, which really does not matter, i.e., the thread will zap the pages anyway. > > But this one, it says 'possible nx_huge_pages', but they are not > > nx huge pages at all. > > Yes, but they _can be_ NX huge pages, hence the "possible". A super verbose name > would be something like mmu_pages_that_can_possibly_be_replaced_by_nx_huge_pages. > I can make a dramtic example as why 'possible' may not help: /* Flag that decides something important. */ bool possible_one; The information we (readers) gain from reading the above is _0_. With that, since you already mentioned the name: 'mmu_pages_that_can_possibly_be_replaced_by_nx_huge_pages', why can't we shorten it by using 'mmu_pages_to_recover_nx_huge' or 'pages_to_recover_nx_huge'? 'recover' is the word that immediately connects with the 'recovery thread', which I think makes more sense on readability. > > Instead, they are 'shadow pages that are replaced with nx_huge_pages'. So > > that's why updates to this list is done together with stats nx_plage_splits. > > > > Please correct me if I am wrong. I am still struggling to understand the > > meaning of these variables. > > > > > > > > > I feel this name is more confusing than the original one. Maybe just keep > > > > > > Ignoring lpage => huge_page, the current name is terribly inaccurate. The list > > > doesn't contain all disallowed huge pages, nor does it even contain all disallowed > > > NX huge pages, it specifically tracks shadow pages that might be able to be > > > replaced with an NX huge page. > > > > > > I'm open to other names, but whatever we choose should be paired with > > > account_nx_huge_page()'s param that is currently named "nx_huge_page_possible". > > > > How about mmu_pages_replaced_by_nx_huge, > > "replaced" is past tense in this usage, and these are definitely not shadow pages > that have already been replaced by NX huge pages. > > > mmu_pages_replaced_by_possible_nx_huge or something starting with > > Same issue with "replaced". > > > possible_pages_, pages_ instead of possible_nx_huge_pages? > > Reprhasing, I think you're asking that the variable have "mmu_pages" somewhere in > the name to clarify that it's a list of kvm_mmu_page structs. I agree that ideally > it would have "mmu_pages" somewhere in there, but I also think that having both > "nx_huge_pages" and "mmu_pages" in the name makes it unreadable, i.e. we have to > choose one use of "pages". > > The only alternative I can come up with that isn't completely gross is > possible_nx_huge_mmu_pages. I can't tell if that's less confusing or more > confusing. Aside from that. Reviewed-by: Mingwei Zhang <mizhang@google.com>
On Thu, Aug 18, 2022, Mingwei Zhang wrote: > On Wed, Aug 17, 2022, Sean Christopherson wrote: > > Yes, they are shadow pages that the NX recovery thread should zap, but the reason > > they should be zapped is because (a) the shadow page has at least one execute child > > SPTE, (b) zapping the shadow page will also zap its child SPTEs, and (c) eliminating > > all executable child SPTEs means KVM _might_ be able to instantiate an NX huge page. > > > > oh, I scratched my head and finaly got your point. hmm. So the shadow > pages are the 'blockers' to (re)create a NX huge page because of at > least one present child executable spte. So, really, whether these > shadow pages themselves are NX huge or not does not really matter. All > we need to know is that they will be zapped in the future to help making > recovery of an NX huge page possible. More precisely, we want to zap shadow pages with executable children if and only if they can _possibly_ be replaced with an NX huge page. The "possibly" is saying that zapping _may or may not_ result in an NX huge page. And it also conveys that pages that _cannot_ be replaced with an NX huge page are not on the list. If the guest is still using any of the huge page for execution, then KVM can't create an NX huge page (or it may temporarily create one and then zap it when the gets takes an executable fault), but KVM can't know that until it zaps and the guest takes a fault. Thus, possibly. > > > `nx_huge_page_disallowed` is easy to understand because it literally say > > > 'nx_huge_page is not allowed', which is correct. > > > > No, it's not correct. The list isn't simply the set of shadow pages that disallow > > NX huge pages, it's the set of shadow pages that disallow NX huge pages _and_ that > > can possibly be replaced by an NX huge page if the shadow page and all its > > (executable) children go away. > > > > hmm, I think this naming is correct. The flag is used to talk to the > 'fault handler' to say 'hey, don't create nx huge page, stupid'. Of > course, it is also used to by the 'nx huge recovery thread', but the > recovery thread will only check it for sanity purpose, which really does > not matter, i.e., the thread will zap the pages anyway. Ah, sorry, I thought you were suggesting "nx_huge_page_disallowed" for the list name, but you were talking about the flag. Yes, 100% agree that the flag is appropriately named. > > > But this one, it says 'possible nx_huge_pages', but they are not > > > nx huge pages at all. > > > > Yes, but they _can be_ NX huge pages, hence the "possible". A super verbose name > > would be something like mmu_pages_that_can_possibly_be_replaced_by_nx_huge_pages. > > > > I can make a dramtic example as why 'possible' may not help: > > /* Flag that decides something important. */ > bool possible_one; > > The information we (readers) gain from reading the above is _0_. But that's only half the story. If we also had an associated flag bool one_disallowed; a.k.a. nx_huge_page_disallowed, then when viewed together, readers know that the existince of this struct disallows "one", but that structs with one_disallowed=true _and_ possible_one=true _might_ be converted to "one", whereas structs with possible_one=false _cannot_ be converted to "one". > With that, since you already mentioned the name: > 'mmu_pages_that_can_possibly_be_replaced_by_nx_huge_pages', > why can't we shorten it by using 'mmu_pages_to_recover_nx_huge' or > 'pages_to_recover_nx_huge'? 'recover' is the word that immediately > connects with the 'recovery thread', which I think makes more sense on > readability. mmu_pages_to_recover_nx_huge doesn't capture that recovery isn't guaranteed. IMO it also does a poor job of capturing _why_ pages are on the list, i.e. a reader knows they are pages that will be "recovered", but it doesn't clarify that they'll be recovered/zapped because KVM might be able to be replace them with NX huge pages. In other words, it doesn't help the reader understand why some, but not all, nx_huge_page_disallowed are on the recovery list.
On Thu, Aug 18, 2022, Sean Christopherson wrote: > On Thu, Aug 18, 2022, Mingwei Zhang wrote: > > On Wed, Aug 17, 2022, Sean Christopherson wrote: > > > Yes, they are shadow pages that the NX recovery thread should zap, but the reason > > > they should be zapped is because (a) the shadow page has at least one execute child > > > SPTE, (b) zapping the shadow page will also zap its child SPTEs, and (c) eliminating > > > all executable child SPTEs means KVM _might_ be able to instantiate an NX huge page. > > > > > > > oh, I scratched my head and finaly got your point. hmm. So the shadow > > pages are the 'blockers' to (re)create a NX huge page because of at > > least one present child executable spte. So, really, whether these > > shadow pages themselves are NX huge or not does not really matter. All > > we need to know is that they will be zapped in the future to help making > > recovery of an NX huge page possible. > > More precisely, we want to zap shadow pages with executable children if and only > if they can _possibly_ be replaced with an NX huge page. The "possibly" is saying > that zapping _may or may not_ result in an NX huge page. And it also conveys that > pages that _cannot_ be replaced with an NX huge page are not on the list. > > If the guest is still using any of the huge page for execution, then KVM can't > create an NX huge page (or it may temporarily create one and then zap it when the > gets takes an executable fault), but KVM can't know that until it zaps and the > guest takes a fault. Thus, possibly. > Right, I think 'possible' is definitely a correct name for that. In general, using 'possible' can cover the complexity to ensure the description is correct. My only comment here is that 'possible_' might requires extra comments in the code to be more developer friendly. But overall, since I already remembered what was the problem. I no longer think this naming is an issue to me. But just that the name could be better. > > With that, since you already mentioned the name: > > 'mmu_pages_that_can_possibly_be_replaced_by_nx_huge_pages', > > why can't we shorten it by using 'mmu_pages_to_recover_nx_huge' or > > 'pages_to_recover_nx_huge'? 'recover' is the word that immediately > > connects with the 'recovery thread', which I think makes more sense on > > readability. > > mmu_pages_to_recover_nx_huge doesn't capture that recovery isn't guaranteed. > IMO it also does a poor job of capturing _why_ pages are on the list, i.e. a > reader knows they are pages that will be "recovered", but it doesn't clarify that > they'll be recovered/zapped because KVM might be able to be replace them with NX > huge pages. In other words, it doesn't help the reader understand why some, but > not all, nx_huge_page_disallowed are on the recovery list. I think you are right that the name does not call out 'why' the pages are on the list. But on the other hand, I am not sure how much it could help clarifying the situations by just reading the list name. I would propose we add the conditions using the (flag, list). (nx_huge_page_disallowed, possible_nx_huge_pages) case (true, in_list): mitigation for multi-hit iTLB. case (true, not_in_list): dirty logging disabled; address misalignment; guest did not turn on paging. case (false, in_list): not possible. case (false, not_in_list): Any other situation where KVM manipulate SPTEs. Maybe this should be in the commit message of the previous patch.
> (nx_huge_page_disallowed, possible_nx_huge_pages) > > case (true, in_list): mitigation for multi-hit iTLB. > case (true, not_in_list): dirty logging disabled; address misalignment; guest did not turn on paging. > case (false, in_list): not possible. > case (false, not_in_list): Any other situation where KVM manipulate SPTEs. > made a mistake: should be: case (true, in_list): mitigation for multi-hit iTLB. case (true, not_in_list): address misalignment; guest did not turn on paging. case (false, in_list): not possible. case (false, not_in_list): dirty logging disabled; any other situation KVM zaps small SPTEs in replace of a huge one. Anyway, this is not a blocking comment. But I just don't want to leave a mistake in the conversation. Thanks. -Mingwei
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index e8281d64a431..5634347e5d05 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1143,7 +1143,7 @@ struct kvm_arch { struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES]; struct list_head active_mmu_pages; struct list_head zapped_obsolete_pages; - struct list_head lpage_disallowed_mmu_pages; + struct list_head possible_nx_huge_pages; struct kvm_page_track_notifier_node mmu_sp_tracker; struct kvm_page_track_notifier_head track_notifier_head; /* @@ -1259,7 +1259,7 @@ struct kvm_arch { bool sgx_provisioning_allowed; struct kvm_pmu_event_filter __rcu *pmu_event_filter; - struct task_struct *nx_lpage_recovery_thread; + struct task_struct *nx_huge_page_recovery_thread; #ifdef CONFIG_X86_64 /* @@ -1304,8 +1304,8 @@ struct kvm_arch { * - tdp_mmu_roots (above) * - tdp_mmu_pages (above) * - the link field of struct kvm_mmu_pages used by the TDP MMU - * - lpage_disallowed_mmu_pages - * - the lpage_disallowed_link field of struct kvm_mmu_pages used + * - possible_nx_huge_pages; + * - the possible_nx_huge_page_link field of struct kvm_mmu_pages used * by the TDP MMU * It is acceptable, but not necessary, to acquire this lock when * the thread holds the MMU lock in write mode. diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 55dac44f3397..53d0dafa68ff 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -802,20 +802,20 @@ static void account_shadowed(struct kvm *kvm, struct kvm_mmu_page *sp) kvm_flush_remote_tlbs_with_address(kvm, gfn, 1); } -void account_huge_nx_page(struct kvm *kvm, struct kvm_mmu_page *sp, +void account_nx_huge_page(struct kvm *kvm, struct kvm_mmu_page *sp, bool nx_huge_page_possible) { - if (KVM_BUG_ON(!list_empty(&sp->lpage_disallowed_link), kvm)) + if (KVM_BUG_ON(!list_empty(&sp->possible_nx_huge_page_link), kvm)) return; - sp->lpage_disallowed = true; + sp->nx_huge_page_disallowed = true; if (!nx_huge_page_possible) return; ++kvm->stat.nx_lpage_splits; - list_add_tail(&sp->lpage_disallowed_link, - &kvm->arch.lpage_disallowed_mmu_pages); + list_add_tail(&sp->possible_nx_huge_page_link, + &kvm->arch.possible_nx_huge_pages); } static void unaccount_shadowed(struct kvm *kvm, struct kvm_mmu_page *sp) @@ -835,15 +835,15 @@ static void unaccount_shadowed(struct kvm *kvm, struct kvm_mmu_page *sp) kvm_mmu_gfn_allow_lpage(slot, gfn); } -void unaccount_huge_nx_page(struct kvm *kvm, struct kvm_mmu_page *sp) +void unaccount_nx_huge_page(struct kvm *kvm, struct kvm_mmu_page *sp) { - sp->lpage_disallowed = false; + sp->nx_huge_page_disallowed = false; - if (list_empty(&sp->lpage_disallowed_link)) + if (list_empty(&sp->possible_nx_huge_page_link)) return; --kvm->stat.nx_lpage_splits; - list_del_init(&sp->lpage_disallowed_link); + list_del_init(&sp->possible_nx_huge_page_link); } static struct kvm_memory_slot * @@ -2124,7 +2124,7 @@ static struct kvm_mmu_page *kvm_mmu_alloc_shadow_page(struct kvm *kvm, set_page_private(virt_to_page(sp->spt), (unsigned long)sp); - INIT_LIST_HEAD(&sp->lpage_disallowed_link); + INIT_LIST_HEAD(&sp->possible_nx_huge_page_link); /* * active_mmu_pages must be a FIFO list, as kvm_zap_obsolete_pages() @@ -2483,8 +2483,8 @@ static bool __kvm_mmu_prepare_zap_page(struct kvm *kvm, zapped_root = !is_obsolete_sp(kvm, sp); } - if (sp->lpage_disallowed) - unaccount_huge_nx_page(kvm, sp); + if (sp->nx_huge_page_disallowed) + unaccount_nx_huge_page(kvm, sp); sp->role.invalid = 1; @@ -3124,7 +3124,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) link_shadow_page(vcpu, it.sptep, sp); if (fault->is_tdp && fault->huge_page_disallowed) - account_huge_nx_page(vcpu->kvm, sp, + account_nx_huge_page(vcpu->kvm, sp, fault->req_level >= it.level); } @@ -5981,7 +5981,7 @@ int kvm_mmu_init_vm(struct kvm *kvm) INIT_LIST_HEAD(&kvm->arch.active_mmu_pages); INIT_LIST_HEAD(&kvm->arch.zapped_obsolete_pages); - INIT_LIST_HEAD(&kvm->arch.lpage_disallowed_mmu_pages); + INIT_LIST_HEAD(&kvm->arch.possible_nx_huge_pages); spin_lock_init(&kvm->arch.mmu_unsync_pages_lock); r = kvm_mmu_init_tdp_mmu(kvm); @@ -6699,7 +6699,7 @@ static int set_nx_huge_pages(const char *val, const struct kernel_param *kp) kvm_mmu_zap_all_fast(kvm); mutex_unlock(&kvm->slots_lock); - wake_up_process(kvm->arch.nx_lpage_recovery_thread); + wake_up_process(kvm->arch.nx_huge_page_recovery_thread); } mutex_unlock(&kvm_lock); } @@ -6825,7 +6825,7 @@ static int set_nx_huge_pages_recovery_param(const char *val, const struct kernel mutex_lock(&kvm_lock); list_for_each_entry(kvm, &vm_list, vm_list) - wake_up_process(kvm->arch.nx_lpage_recovery_thread); + wake_up_process(kvm->arch.nx_huge_page_recovery_thread); mutex_unlock(&kvm_lock); } @@ -6833,7 +6833,7 @@ static int set_nx_huge_pages_recovery_param(const char *val, const struct kernel return err; } -static void kvm_recover_nx_lpages(struct kvm *kvm) +static void kvm_recover_nx_huge_pages(struct kvm *kvm) { unsigned long nx_lpage_splits = kvm->stat.nx_lpage_splits; int rcu_idx; @@ -6856,23 +6856,25 @@ static void kvm_recover_nx_lpages(struct kvm *kvm) ratio = READ_ONCE(nx_huge_pages_recovery_ratio); to_zap = ratio ? DIV_ROUND_UP(nx_lpage_splits, ratio) : 0; for ( ; to_zap; --to_zap) { - if (list_empty(&kvm->arch.lpage_disallowed_mmu_pages)) + if (list_empty(&kvm->arch.possible_nx_huge_pages)) break; /* * We use a separate list instead of just using active_mmu_pages - * because the number of lpage_disallowed pages is expected to - * be relatively small compared to the total. + * because the number of shadow pages that be replaced with an + * NX huge page is expected to be relatively small compared to + * the total number of shadow pages. And because the TDP MMU + * doesn't use active_mmu_pages. */ - sp = list_first_entry(&kvm->arch.lpage_disallowed_mmu_pages, + sp = list_first_entry(&kvm->arch.possible_nx_huge_pages, struct kvm_mmu_page, - lpage_disallowed_link); - WARN_ON_ONCE(!sp->lpage_disallowed); + possible_nx_huge_page_link); + WARN_ON_ONCE(!sp->nx_huge_page_disallowed); if (is_tdp_mmu_page(sp)) { flush |= kvm_tdp_mmu_zap_sp(kvm, sp); } else { kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list); - WARN_ON_ONCE(sp->lpage_disallowed); + WARN_ON_ONCE(sp->nx_huge_page_disallowed); } if (need_resched() || rwlock_needbreak(&kvm->mmu_lock)) { @@ -6893,7 +6895,7 @@ static void kvm_recover_nx_lpages(struct kvm *kvm) srcu_read_unlock(&kvm->srcu, rcu_idx); } -static long get_nx_lpage_recovery_timeout(u64 start_time) +static long get_nx_huge_page_recovery_timeout(u64 start_time) { bool enabled; uint period; @@ -6904,19 +6906,19 @@ static long get_nx_lpage_recovery_timeout(u64 start_time) : MAX_SCHEDULE_TIMEOUT; } -static int kvm_nx_lpage_recovery_worker(struct kvm *kvm, uintptr_t data) +static int kvm_nx_huge_page_recovery_worker(struct kvm *kvm, uintptr_t data) { u64 start_time; long remaining_time; while (true) { start_time = get_jiffies_64(); - remaining_time = get_nx_lpage_recovery_timeout(start_time); + remaining_time = get_nx_huge_page_recovery_timeout(start_time); set_current_state(TASK_INTERRUPTIBLE); while (!kthread_should_stop() && remaining_time > 0) { schedule_timeout(remaining_time); - remaining_time = get_nx_lpage_recovery_timeout(start_time); + remaining_time = get_nx_huge_page_recovery_timeout(start_time); set_current_state(TASK_INTERRUPTIBLE); } @@ -6925,7 +6927,7 @@ static int kvm_nx_lpage_recovery_worker(struct kvm *kvm, uintptr_t data) if (kthread_should_stop()) return 0; - kvm_recover_nx_lpages(kvm); + kvm_recover_nx_huge_pages(kvm); } } @@ -6933,17 +6935,17 @@ int kvm_mmu_post_init_vm(struct kvm *kvm) { int err; - err = kvm_vm_create_worker_thread(kvm, kvm_nx_lpage_recovery_worker, 0, + err = kvm_vm_create_worker_thread(kvm, kvm_nx_huge_page_recovery_worker, 0, "kvm-nx-lpage-recovery", - &kvm->arch.nx_lpage_recovery_thread); + &kvm->arch.nx_huge_page_recovery_thread); if (!err) - kthread_unpark(kvm->arch.nx_lpage_recovery_thread); + kthread_unpark(kvm->arch.nx_huge_page_recovery_thread); return err; } void kvm_mmu_pre_destroy_vm(struct kvm *kvm) { - if (kvm->arch.nx_lpage_recovery_thread) - kthread_stop(kvm->arch.nx_lpage_recovery_thread); + if (kvm->arch.nx_huge_page_recovery_thread) + kthread_stop(kvm->arch.nx_huge_page_recovery_thread); } diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h index cca1ad75d096..67879459a25c 100644 --- a/arch/x86/kvm/mmu/mmu_internal.h +++ b/arch/x86/kvm/mmu/mmu_internal.h @@ -57,7 +57,13 @@ struct kvm_mmu_page { bool tdp_mmu_page; bool unsync; u8 mmu_valid_gen; - bool lpage_disallowed; /* Can't be replaced by an equiv large page */ + + /* + * The shadow page can't be replaced by an equivalent huge page + * because it is being used to map an executable page in the guest + * and the NX huge page mitigation is enabled. + */ + bool nx_huge_page_disallowed; /* * The following two entries are used to key the shadow page in the @@ -102,12 +108,12 @@ struct kvm_mmu_page { /* * Tracks shadow pages that, if zapped, would allow KVM to create an NX - * huge page. A shadow page will have lpage_disallowed set but not be - * on the list if a huge page is disallowed for other reasons, e.g. - * because KVM is shadowing a PTE at the same gfn, the memslot isn't - * properly aligned, etc... + * huge page. A shadow page will have nx_huge_page_disallowed set but + * not be on the list if a huge page is disallowed for other reasons, + * e.g. because KVM is shadowing a PTE at the same gfn, the memslot + * isn't properly aligned, etc... */ - struct list_head lpage_disallowed_link; + struct list_head possible_nx_huge_page_link; #ifdef CONFIG_X86_32 /* * Used out of the mmu-lock to avoid reading spte values while an @@ -322,8 +328,8 @@ void disallowed_hugepage_adjust(struct kvm_page_fault *fault, u64 spte, int cur_ void *mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc); -void account_huge_nx_page(struct kvm *kvm, struct kvm_mmu_page *sp, +void account_nx_huge_page(struct kvm *kvm, struct kvm_mmu_page *sp, bool nx_huge_page_possible); -void unaccount_huge_nx_page(struct kvm *kvm, struct kvm_mmu_page *sp); +void unaccount_nx_huge_page(struct kvm *kvm, struct kvm_mmu_page *sp); #endif /* __KVM_X86_MMU_INTERNAL_H */ diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h index e450f49f2225..259c0f019f09 100644 --- a/arch/x86/kvm/mmu/paging_tmpl.h +++ b/arch/x86/kvm/mmu/paging_tmpl.h @@ -714,7 +714,7 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault, link_shadow_page(vcpu, it.sptep, sp); if (fault->huge_page_disallowed) - account_huge_nx_page(vcpu->kvm, sp, + account_nx_huge_page(vcpu->kvm, sp, fault->req_level >= it.level); } diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 903d0d3497b6..0e94182c87be 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -284,7 +284,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp(struct kvm_vcpu *vcpu) static void tdp_mmu_init_sp(struct kvm_mmu_page *sp, tdp_ptep_t sptep, gfn_t gfn, union kvm_mmu_page_role role) { - INIT_LIST_HEAD(&sp->lpage_disallowed_link); + INIT_LIST_HEAD(&sp->possible_nx_huge_page_link); set_page_private(virt_to_page(sp->spt), (unsigned long)sp); @@ -392,8 +392,8 @@ static void tdp_mmu_unlink_sp(struct kvm *kvm, struct kvm_mmu_page *sp, lockdep_assert_held_write(&kvm->mmu_lock); list_del(&sp->link); - if (sp->lpage_disallowed) - unaccount_huge_nx_page(kvm, sp); + if (sp->nx_huge_page_disallowed) + unaccount_nx_huge_page(kvm, sp); if (shared) spin_unlock(&kvm->arch.tdp_mmu_pages_lock); @@ -1132,7 +1132,7 @@ static int tdp_mmu_link_sp(struct kvm *kvm, struct tdp_iter *iter, spin_lock(&kvm->arch.tdp_mmu_pages_lock); list_add(&sp->link, &kvm->arch.tdp_mmu_pages); if (account_nx) - account_huge_nx_page(kvm, sp, true); + account_nx_huge_page(kvm, sp, true); spin_unlock(&kvm->arch.tdp_mmu_pages_lock); return 0;
Rename most of the variables/functions involved in the NX huge page mitigation to provide consistency, e.g. lpage vs huge page, and NX huge vs huge NX, and also to provide clarity, e.g. to make it obvious the flag applies only to the NX huge page mitigation, not to any condition that prevents creating a huge page. Leave the nx_lpage_splits stat alone as the name is ABI and thus set in stone. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/kvm_host.h | 8 ++-- arch/x86/kvm/mmu/mmu.c | 70 +++++++++++++++++---------------- arch/x86/kvm/mmu/mmu_internal.h | 22 +++++++---- arch/x86/kvm/mmu/paging_tmpl.h | 2 +- arch/x86/kvm/mmu/tdp_mmu.c | 8 ++-- 5 files changed, 59 insertions(+), 51 deletions(-)