| Message ID | 20221208193857.4090582-21-dmatlack@google.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | KVM: Refactor the KVM/x86 TDP MMU into common code | expand |
On Thu, Dec 8, 2022 at 11:39 AM David Matlack <dmatlack@google.com> wrote: > > Abstract away kvm_mmu_max_mapping_level(), which is an x86-specific > function for computing the max level that a given GFN can be mapped in > KVM's page tables. This will be used in a future commit to enable moving > the TDP MMU to common code. > > Provide a default implementation for non-x86 architectures that just > returns the max level. This will result in more zapping than necessary > when disabling dirty logging (i.e. less than optimal performance) but no > correctness issues. Apologies if you already implemented it in a later patch in this series, but would it not at least be possible to port host_pfn_mapping_level to common code and check that? I'm assuming, though I could be wrong, that all archs map GFNs with at most a host page table granularity mapping. I suppose that doesn't strictly need to be included in this series, but it would be worth addressing in the commit description. > > Signed-off-by: David Matlack <dmatlack@google.com> > --- > arch/x86/kvm/mmu/tdp_mmu.c | 14 ++++++++++---- > arch/x86/kvm/mmu/tdp_pgtable.c | 7 +++++++ > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 7670fbd8e72d..24d1dbd0a1ec 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -1696,6 +1696,13 @@ void kvm_tdp_mmu_clear_dirty_pt_masked(struct kvm *kvm, > clear_dirty_pt_masked(kvm, root, gfn, mask, wrprot); > } > > +__weak int tdp_mmu_max_mapping_level(struct kvm *kvm, > + const struct kvm_memory_slot *slot, > + struct tdp_iter *iter) > +{ > + return TDP_MAX_HUGEPAGE_LEVEL; > +} > + > static void zap_collapsible_spte_range(struct kvm *kvm, > struct kvm_mmu_page *root, > const struct kvm_memory_slot *slot) > @@ -1727,15 +1734,14 @@ static void zap_collapsible_spte_range(struct kvm *kvm, > /* > * If iter.gfn resides outside of the slot, i.e. the page for > * the current level overlaps but is not contained by the slot, > - * then the SPTE can't be made huge. More importantly, trying > - * to query that info from slot->arch.lpage_info will cause an > + * then the SPTE can't be made huge. On x86, trying to query > + * that info from slot->arch.lpage_info will cause an > * out-of-bounds access. > */ > if (iter.gfn < start || iter.gfn >= end) > continue; > > - max_mapping_level = kvm_mmu_max_mapping_level(kvm, slot, > - iter.gfn, PG_LEVEL_NUM); > + max_mapping_level = tdp_mmu_max_mapping_level(kvm, slot, &iter); > if (max_mapping_level < iter.level) > continue; > > diff --git a/arch/x86/kvm/mmu/tdp_pgtable.c b/arch/x86/kvm/mmu/tdp_pgtable.c > index b07ed99b4ab1..840d063c45b8 100644 > --- a/arch/x86/kvm/mmu/tdp_pgtable.c > +++ b/arch/x86/kvm/mmu/tdp_pgtable.c > @@ -163,3 +163,10 @@ void tdp_mmu_arch_unlink_sp(struct kvm *kvm, struct kvm_mmu_page *sp, > if (shared) > spin_unlock(&kvm->arch.tdp_mmu_pages_lock); > } > + > +int tdp_mmu_max_mapping_level(struct kvm *kvm, > + const struct kvm_memory_slot *slot, > + struct tdp_iter *iter) > +{ > + return kvm_mmu_max_mapping_level(kvm, slot, iter->gfn, PG_LEVEL_NUM); > +} > -- > 2.39.0.rc1.256.g54fd8350bd-goog >
On Mon, Dec 12, 2022 at 11:32 AM Ben Gardon <bgardon@google.com> wrote: > > On Thu, Dec 8, 2022 at 11:39 AM David Matlack <dmatlack@google.com> wrote: > > > > Abstract away kvm_mmu_max_mapping_level(), which is an x86-specific > > function for computing the max level that a given GFN can be mapped in > > KVM's page tables. This will be used in a future commit to enable moving > > the TDP MMU to common code. > > > > Provide a default implementation for non-x86 architectures that just > > returns the max level. This will result in more zapping than necessary > > when disabling dirty logging (i.e. less than optimal performance) but no > > correctness issues. > > Apologies if you already implemented it in a later patch in this > series, but would it not at least be possible to port > host_pfn_mapping_level to common code and check that? > I'm assuming, though I could be wrong, that all archs map GFNs with at > most a host page table granularity mapping. > I suppose that doesn't strictly need to be included in this series, > but it would be worth addressing in the commit description. It's not implemented later in this series, but I agree it's something we should do. In fact, it's worth doing regardless of this series as a way to share more code across architectures (e.g. KVM/ARM has it's own version in arch/arm64/kvm/mmu.c:get_user_mapping_size()).
On Mon, Dec 12, 2022, David Matlack wrote: > On Mon, Dec 12, 2022 at 11:32 AM Ben Gardon <bgardon@google.com> wrote: > > > > On Thu, Dec 8, 2022 at 11:39 AM David Matlack <dmatlack@google.com> wrote: > > > > > > Abstract away kvm_mmu_max_mapping_level(), which is an x86-specific > > > function for computing the max level that a given GFN can be mapped in > > > KVM's page tables. This will be used in a future commit to enable moving > > > the TDP MMU to common code. > > > > > > Provide a default implementation for non-x86 architectures that just > > > returns the max level. This will result in more zapping than necessary > > > when disabling dirty logging (i.e. less than optimal performance) but no > > > correctness issues. > > > > Apologies if you already implemented it in a later patch in this > > series, but would it not at least be possible to port > > host_pfn_mapping_level to common code and check that? > > I'm assuming, though I could be wrong, that all archs map GFNs with at > > most a host page table granularity mapping. > > I suppose that doesn't strictly need to be included in this series, > > but it would be worth addressing in the commit description. > > It's not implemented later in this series, but I agree it's something > we should do. In fact, it's worth doing regardless of this series as a > way to share more code across architectures (e.g. KVM/ARM has it's own > version in arch/arm64/kvm/mmu.c:get_user_mapping_size()). Ya, ARM converted to walking the host user page tables largely in response to x86's conversion. After x86 switched, ARM was left holding the bag that was PageTransCompoundMap(). On a related topic, I'm guessing all the comments in transparent_hugepage_adjust() about the code working _only_ for THP are stale. Unless ARM support for HugeTLB works differently, walking host user page tables should Just Work for all hugepage types.
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 7670fbd8e72d..24d1dbd0a1ec 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -1696,6 +1696,13 @@ void kvm_tdp_mmu_clear_dirty_pt_masked(struct kvm *kvm, clear_dirty_pt_masked(kvm, root, gfn, mask, wrprot); } +__weak int tdp_mmu_max_mapping_level(struct kvm *kvm, + const struct kvm_memory_slot *slot, + struct tdp_iter *iter) +{ + return TDP_MAX_HUGEPAGE_LEVEL; +} + static void zap_collapsible_spte_range(struct kvm *kvm, struct kvm_mmu_page *root, const struct kvm_memory_slot *slot) @@ -1727,15 +1734,14 @@ static void zap_collapsible_spte_range(struct kvm *kvm, /* * If iter.gfn resides outside of the slot, i.e. the page for * the current level overlaps but is not contained by the slot, - * then the SPTE can't be made huge. More importantly, trying - * to query that info from slot->arch.lpage_info will cause an + * then the SPTE can't be made huge. On x86, trying to query + * that info from slot->arch.lpage_info will cause an * out-of-bounds access. */ if (iter.gfn < start || iter.gfn >= end) continue; - max_mapping_level = kvm_mmu_max_mapping_level(kvm, slot, - iter.gfn, PG_LEVEL_NUM); + max_mapping_level = tdp_mmu_max_mapping_level(kvm, slot, &iter); if (max_mapping_level < iter.level) continue; diff --git a/arch/x86/kvm/mmu/tdp_pgtable.c b/arch/x86/kvm/mmu/tdp_pgtable.c index b07ed99b4ab1..840d063c45b8 100644 --- a/arch/x86/kvm/mmu/tdp_pgtable.c +++ b/arch/x86/kvm/mmu/tdp_pgtable.c @@ -163,3 +163,10 @@ void tdp_mmu_arch_unlink_sp(struct kvm *kvm, struct kvm_mmu_page *sp, if (shared) spin_unlock(&kvm->arch.tdp_mmu_pages_lock); } + +int tdp_mmu_max_mapping_level(struct kvm *kvm, + const struct kvm_memory_slot *slot, + struct tdp_iter *iter) +{ + return kvm_mmu_max_mapping_level(kvm, slot, iter->gfn, PG_LEVEL_NUM); +}
Abstract away kvm_mmu_max_mapping_level(), which is an x86-specific function for computing the max level that a given GFN can be mapped in KVM's page tables. This will be used in a future commit to enable moving the TDP MMU to common code. Provide a default implementation for non-x86 architectures that just returns the max level. This will result in more zapping than necessary when disabling dirty logging (i.e. less than optimal performance) but no correctness issues. Signed-off-by: David Matlack <dmatlack@google.com> --- arch/x86/kvm/mmu/tdp_mmu.c | 14 ++++++++++---- arch/x86/kvm/mmu/tdp_pgtable.c | 7 +++++++ 2 files changed, 17 insertions(+), 4 deletions(-)