| Message ID | 20221223005739.1295925-27-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, Dec 23, 2022 at 12:57:38AM +0000, Sean Christopherson wrote: > Add a page-track API to query if a gfn is "valid", i.e. is backed by a > memslot and is visible to the guest. This is one more step toward > removing KVM internal details from the page-track APIs. > > Add a FIXME to call out that intel_gvt_is_valid_gfn() is broken with > respect to 2MiB (or larger) guest entries, e.g. if the starting gfn is > valid but a 2MiB page starting at the gfn covers "invalid" memory due > to running beyond the memslot. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_page_track.h | 1 + > arch/x86/kvm/mmu/page_track.c | 13 +++++++++++++ > drivers/gpu/drm/i915/gvt/gtt.c | 11 ++--------- > 3 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h > index 66a0d7c34311..99e1d6eeb0fb 100644 > --- a/arch/x86/include/asm/kvm_page_track.h > +++ b/arch/x86/include/asm/kvm_page_track.h > @@ -52,6 +52,7 @@ int kvm_page_track_register_notifier(struct kvm *kvm, > void kvm_page_track_unregister_notifier(struct kvm *kvm, > struct kvm_page_track_notifier_node *n); > > +bool kvm_page_track_is_valid_gfn(struct kvm *kvm, gfn_t gfn); > int kvm_write_track_add_gfn(struct kvm *kvm, gfn_t gfn); > int kvm_write_track_remove_gfn(struct kvm *kvm, gfn_t gfn); > #endif /* CONFIG_KVM_EXTERNAL_WRITE_TRACKING */ > diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c > index 1af431a41f71..9da071a514b3 100644 > --- a/arch/x86/kvm/mmu/page_track.c > +++ b/arch/x86/kvm/mmu/page_track.c > @@ -264,6 +264,19 @@ enum pg_level kvm_page_track_max_mapping_level(struct kvm *kvm, gfn_t gfn, > } > EXPORT_SYMBOL_GPL(kvm_page_track_max_mapping_level); > > +bool kvm_page_track_is_valid_gfn(struct kvm *kvm, gfn_t gfn) > +{ > + bool ret; > + int idx; > + > + idx = srcu_read_lock(&kvm->srcu); > + ret = kvm_is_visible_gfn(kvm, gfn); > + srcu_read_unlock(&kvm->srcu, idx); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(kvm_page_track_is_valid_gfn); This implementation is only to check whether a GFN is within a visible kvm memslot. So, why this helper function is named kvm_page_track_xxx()? Don't think it's anything related to page track, and not all of its callers in KVMGT are for page tracking. Thanks Yan > + > /* > * add guest page to the tracking pool so that corresponding access on that > * page will be intercepted. > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c > index 59ba6639e622..43c4fc23205d 100644 > --- a/drivers/gpu/drm/i915/gvt/gtt.c > +++ b/drivers/gpu/drm/i915/gvt/gtt.c > @@ -51,18 +51,11 @@ static int preallocated_oos_pages = 8192; > > static bool intel_gvt_is_valid_gfn(struct intel_vgpu *vgpu, unsigned long gfn) > { > - struct kvm *kvm = vgpu->vfio_device.kvm; > - int idx; > - bool ret; > - > if (!vgpu->attached) > return false; > > - idx = srcu_read_lock(&kvm->srcu); > - ret = kvm_is_visible_gfn(kvm, gfn); > - srcu_read_unlock(&kvm->srcu, idx); > - > - return ret; > + /* FIXME: This doesn't properly handle guest entries larger than 4K. */ > + return kvm_page_track_is_valid_gfn(vgpu->vfio_device.kvm, gfn); > } > > /* > -- > 2.39.0.314.g84b9a713c41-goog >
On Wed, Dec 28, 2022, Yan Zhao wrote: > On Fri, Dec 23, 2022 at 12:57:38AM +0000, Sean Christopherson wrote: > > +bool kvm_page_track_is_valid_gfn(struct kvm *kvm, gfn_t gfn) > > +{ > > + bool ret; > > + int idx; > > + > > + idx = srcu_read_lock(&kvm->srcu); > > + ret = kvm_is_visible_gfn(kvm, gfn); > > + srcu_read_unlock(&kvm->srcu, idx); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(kvm_page_track_is_valid_gfn); > This implementation is only to check whether a GFN is within a visible > kvm memslot. So, why this helper function is named kvm_page_track_xxx()? > Don't think it's anything related to page track, and not all of its callers > in KVMGT are for page tracking. KVMGT is the only user of kvm_page_track_is_valid_gfn(). kvm_is_visible_gfn() has other users, just not in x86. And long term, my goal is to allow building KVM x86 without any exports. Killing off KVM's "internal" (for vendor modules) exports for select Kconfigs is easy enough, add adding a dedicated page-track API solves the KVMGT angle.
On Tue, Jan 03, 2023 at 09:19:01PM +0000, Sean Christopherson wrote: > On Wed, Dec 28, 2022, Yan Zhao wrote: > > On Fri, Dec 23, 2022 at 12:57:38AM +0000, Sean Christopherson wrote: > > > +bool kvm_page_track_is_valid_gfn(struct kvm *kvm, gfn_t gfn) > > > +{ > > > + bool ret; > > > + int idx; > > > + > > > + idx = srcu_read_lock(&kvm->srcu); > > > + ret = kvm_is_visible_gfn(kvm, gfn); > > > + srcu_read_unlock(&kvm->srcu, idx); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL_GPL(kvm_page_track_is_valid_gfn); > > This implementation is only to check whether a GFN is within a visible > > kvm memslot. So, why this helper function is named kvm_page_track_xxx()? > > Don't think it's anything related to page track, and not all of its callers > > in KVMGT are for page tracking. > > KVMGT is the only user of kvm_page_track_is_valid_gfn(). kvm_is_visible_gfn() > has other users, just not in x86. And long term, my goal is to allow building > KVM x86 without any exports. Killing off KVM's "internal" (for vendor modules) > exports for select Kconfigs is easy enough, add adding a dedicated page-track API > solves the KVMGT angle. Understand! But personally, I don't like merging this API into page-track API as it obviously has nothing to do with page-track stuffs, and KVMGT also calls it for non-page-track purpuse.
On Thu, Jan 05, 2023, Yan Zhao wrote: > On Tue, Jan 03, 2023 at 09:19:01PM +0000, Sean Christopherson wrote: > > On Wed, Dec 28, 2022, Yan Zhao wrote: > > > On Fri, Dec 23, 2022 at 12:57:38AM +0000, Sean Christopherson wrote: > > > > +bool kvm_page_track_is_valid_gfn(struct kvm *kvm, gfn_t gfn) > > > > +{ > > > > + bool ret; > > > > + int idx; > > > > + > > > > + idx = srcu_read_lock(&kvm->srcu); > > > > + ret = kvm_is_visible_gfn(kvm, gfn); > > > > + srcu_read_unlock(&kvm->srcu, idx); > > > > + > > > > + return ret; > > > > +} > > > > +EXPORT_SYMBOL_GPL(kvm_page_track_is_valid_gfn); > > > This implementation is only to check whether a GFN is within a visible > > > kvm memslot. So, why this helper function is named kvm_page_track_xxx()? > > > Don't think it's anything related to page track, and not all of its callers > > > in KVMGT are for page tracking. > > > > KVMGT is the only user of kvm_page_track_is_valid_gfn(). kvm_is_visible_gfn() > > has other users, just not in x86. And long term, my goal is to allow building > > KVM x86 without any exports. Killing off KVM's "internal" (for vendor modules) > > exports for select Kconfigs is easy enough, add adding a dedicated page-track API > > solves the KVMGT angle. > Understand! > But personally, I don't like merging this API into page-track API as > it obviously has nothing to do with page-track stuffs, and KVMGT also calls it for > non-page-track purpuse. 100% agreed, but as discussed in the other patch[*], IMO the real issue is that KVMGT is abusing KVM APIs to check the validity of GFNs that are ultimately mapped via VFIO. Once that issue is fixed, kvm_page_track_is_valid_gfn() can go away entirely. I view this as a short/medium term hack-a-fix to limit and encapsulate KVM's API surface that is "needed" by KVMGT. [*] https://lore.kernel.org/all/Y7cLkLUMCy+XLRwm@google.com
diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h index 66a0d7c34311..99e1d6eeb0fb 100644 --- a/arch/x86/include/asm/kvm_page_track.h +++ b/arch/x86/include/asm/kvm_page_track.h @@ -52,6 +52,7 @@ int kvm_page_track_register_notifier(struct kvm *kvm, void kvm_page_track_unregister_notifier(struct kvm *kvm, struct kvm_page_track_notifier_node *n); +bool kvm_page_track_is_valid_gfn(struct kvm *kvm, gfn_t gfn); int kvm_write_track_add_gfn(struct kvm *kvm, gfn_t gfn); int kvm_write_track_remove_gfn(struct kvm *kvm, gfn_t gfn); #endif /* CONFIG_KVM_EXTERNAL_WRITE_TRACKING */ diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c index 1af431a41f71..9da071a514b3 100644 --- a/arch/x86/kvm/mmu/page_track.c +++ b/arch/x86/kvm/mmu/page_track.c @@ -264,6 +264,19 @@ enum pg_level kvm_page_track_max_mapping_level(struct kvm *kvm, gfn_t gfn, } EXPORT_SYMBOL_GPL(kvm_page_track_max_mapping_level); +bool kvm_page_track_is_valid_gfn(struct kvm *kvm, gfn_t gfn) +{ + bool ret; + int idx; + + idx = srcu_read_lock(&kvm->srcu); + ret = kvm_is_visible_gfn(kvm, gfn); + srcu_read_unlock(&kvm->srcu, idx); + + return ret; +} +EXPORT_SYMBOL_GPL(kvm_page_track_is_valid_gfn); + /* * add guest page to the tracking pool so that corresponding access on that * page will be intercepted. diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c index 59ba6639e622..43c4fc23205d 100644 --- a/drivers/gpu/drm/i915/gvt/gtt.c +++ b/drivers/gpu/drm/i915/gvt/gtt.c @@ -51,18 +51,11 @@ static int preallocated_oos_pages = 8192; static bool intel_gvt_is_valid_gfn(struct intel_vgpu *vgpu, unsigned long gfn) { - struct kvm *kvm = vgpu->vfio_device.kvm; - int idx; - bool ret; - if (!vgpu->attached) return false; - idx = srcu_read_lock(&kvm->srcu); - ret = kvm_is_visible_gfn(kvm, gfn); - srcu_read_unlock(&kvm->srcu, idx); - - return ret; + /* FIXME: This doesn't properly handle guest entries larger than 4K. */ + return kvm_page_track_is_valid_gfn(vgpu->vfio_device.kvm, gfn); } /*
Add a page-track API to query if a gfn is "valid", i.e. is backed by a memslot and is visible to the guest. This is one more step toward removing KVM internal details from the page-track APIs. Add a FIXME to call out that intel_gvt_is_valid_gfn() is broken with respect to 2MiB (or larger) guest entries, e.g. if the starting gfn is valid but a 2MiB page starting at the gfn covers "invalid" memory due to running beyond the memslot. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/kvm_page_track.h | 1 + arch/x86/kvm/mmu/page_track.c | 13 +++++++++++++ drivers/gpu/drm/i915/gvt/gtt.c | 11 ++--------- 3 files changed, 16 insertions(+), 9 deletions(-)