| Message ID | 20230311002258.852397-17-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gvt: KVM: KVMGT fixes and page-track cleanups | expand |
On Fri, Mar 10, 2023 at 04:22:47PM -0800, Sean Christopherson wrote: > From: Yan Zhao <yan.y.zhao@intel.com> > > Add a new page-track hook, track_remove_region(), that is called when a > memslot DELETE operation is about to be committed. The "remove" hook > will be used by KVMGT and will effectively replace the existing > track_flush_slot() altogether now that KVM itself doesn't rely on the > "flush" hook either. > > The "flush" hook is flawed as it's invoked before the memslot operation > is guaranteed to succeed, i.e. KVM might ultimately keep the existing > memslot without notifying external page track users, a.k.a. KVMGT. In > practice, this can't currently happen on x86, but there are no guarantees > that won't change in the future, not to mention that "flush" does a very > poor job of describing what is happening. > > Pass in the gfn+nr_pages instead of the slot itself so external users, > i.e. KVMGT, don't need to exposed to KVM internals (memslots). This will > help set the stage for additional cleanups to the page-track APIs. > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > Co-developed-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> ... > +void kvm_page_track_delete_slot(struct kvm *kvm, struct kvm_memory_slot *slot) > +{ > + struct kvm_page_track_notifier_head *head; > + struct kvm_page_track_notifier_node *n; > + int idx; > + > + head = &kvm->arch.track_notifier_head; > + > + if (hlist_empty(&head->track_notifier_list)) > + return; > + > + idx = srcu_read_lock(&head->track_srcu); > + hlist_for_each_entry_srcu(n, &head->track_notifier_list, node, > + srcu_read_lock_held(&head->track_srcu)) Sorry, not sure why the alignment here is not right. Patchwork just sent me a mail to complain about it. Would you mind helping fix it in the next version? Thanks a lot! > + if (n->track_remove_region) > + n->track_remove_region(slot->base_gfn, slot->npages, n); > + srcu_read_unlock(&head->track_srcu, idx); > +} > +
On Fri, Mar 17, 2023, Yan Zhao wrote: > On Fri, Mar 10, 2023 at 04:22:47PM -0800, Sean Christopherson wrote: > > From: Yan Zhao <yan.y.zhao@intel.com> > > > > Add a new page-track hook, track_remove_region(), that is called when a > > memslot DELETE operation is about to be committed. The "remove" hook > > will be used by KVMGT and will effectively replace the existing > > track_flush_slot() altogether now that KVM itself doesn't rely on the > > "flush" hook either. > > > > The "flush" hook is flawed as it's invoked before the memslot operation > > is guaranteed to succeed, i.e. KVM might ultimately keep the existing > > memslot without notifying external page track users, a.k.a. KVMGT. In > > practice, this can't currently happen on x86, but there are no guarantees > > that won't change in the future, not to mention that "flush" does a very > > poor job of describing what is happening. > > > > Pass in the gfn+nr_pages instead of the slot itself so external users, > > i.e. KVMGT, don't need to exposed to KVM internals (memslots). This will > > help set the stage for additional cleanups to the page-track APIs. > > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > Co-developed-by: Sean Christopherson <seanjc@google.com> > > Signed-off-by: Sean Christopherson <seanjc@google.com> > ... > > > +void kvm_page_track_delete_slot(struct kvm *kvm, struct kvm_memory_slot *slot) > > +{ > > + struct kvm_page_track_notifier_head *head; > > + struct kvm_page_track_notifier_node *n; > > + int idx; > > + > > + head = &kvm->arch.track_notifier_head; > > + > > + if (hlist_empty(&head->track_notifier_list)) > > + return; > > + > > + idx = srcu_read_lock(&head->track_srcu); > > + hlist_for_each_entry_srcu(n, &head->track_notifier_list, node, > > + srcu_read_lock_held(&head->track_srcu)) > Sorry, not sure why the alignment here is not right. > Patchwork just sent me a mail to complain about it. > Would you mind helping fix it in the next version? Ah, it's off by two spaces, should be hlist_for_each_entry_srcu(n, &head->track_notifier_list, node, srcu_read_lock_held(&head->track_srcu)) I'll get it fixed in the next version.
diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h index 6a287bcbe8a9..152c5e7d7868 100644 --- a/arch/x86/include/asm/kvm_page_track.h +++ b/arch/x86/include/asm/kvm_page_track.h @@ -43,6 +43,17 @@ struct kvm_page_track_notifier_node { */ void (*track_flush_slot)(struct kvm *kvm, struct kvm_memory_slot *slot, struct kvm_page_track_notifier_node *node); + + /* + * Invoked when a memory region is removed from the guest. Or in KVM + * terms, when a memslot is deleted. + * + * @gfn: base gfn of the region being removed + * @nr_pages: number of pages in the to-be-removed region + * @node: this node + */ + void (*track_remove_region)(gfn_t gfn, unsigned long nr_pages, + struct kvm_page_track_notifier_node *node); }; int kvm_page_track_init(struct kvm *kvm); @@ -77,6 +88,7 @@ kvm_page_track_unregister_notifier(struct kvm *kvm, void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new, int bytes); void kvm_page_track_flush_slot(struct kvm *kvm, struct kvm_memory_slot *slot); +void kvm_page_track_delete_slot(struct kvm *kvm, struct kvm_memory_slot *slot); bool kvm_page_track_has_external_user(struct kvm *kvm); diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c index 1cfc0a0ccc23..d4a8a995276a 100644 --- a/arch/x86/kvm/mmu/page_track.c +++ b/arch/x86/kvm/mmu/page_track.c @@ -304,6 +304,29 @@ void kvm_page_track_flush_slot(struct kvm *kvm, struct kvm_memory_slot *slot) srcu_read_unlock(&head->track_srcu, idx); } +/* + * Notify external page track nodes that a memory region is being removed from + * the VM, e.g. so that users can free any associated metadata. + */ +void kvm_page_track_delete_slot(struct kvm *kvm, struct kvm_memory_slot *slot) +{ + struct kvm_page_track_notifier_head *head; + struct kvm_page_track_notifier_node *n; + int idx; + + head = &kvm->arch.track_notifier_head; + + if (hlist_empty(&head->track_notifier_list)) + return; + + idx = srcu_read_lock(&head->track_srcu); + hlist_for_each_entry_srcu(n, &head->track_notifier_list, node, + srcu_read_lock_held(&head->track_srcu)) + if (n->track_remove_region) + n->track_remove_region(slot->base_gfn, slot->npages, n); + srcu_read_unlock(&head->track_srcu, idx); +} + enum pg_level kvm_page_track_max_mapping_level(struct kvm *kvm, gfn_t gfn, enum pg_level max_level) { diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 47ac9291cd43..0da5ff007d20 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12645,6 +12645,9 @@ void kvm_arch_commit_memory_region(struct kvm *kvm, const struct kvm_memory_slot *new, enum kvm_mr_change change) { + if (change == KVM_MR_DELETE) + kvm_page_track_delete_slot(kvm, old); + if (!kvm->arch.n_requested_mmu_pages && (change == KVM_MR_CREATE || change == KVM_MR_DELETE)) { unsigned long nr_mmu_pages;