| Message ID | 20230311002258.852397-21-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gvt: KVM: KVMGT fixes and page-track cleanups | expand |
Nit: there is a typo in the commit header: "iff" -> "if" > -void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new, > - int bytes) > +void __kvm_page_track_write(struct kvm *kvm, gpa_t gpa, const u8 *new, int bytes) Line length is 81 characters. A little longer than 80 :) > +static inline bool kvm_page_track_has_external_user(struct kvm *kvm) { return false; } This line is also too long.
On Fri, Mar 10, 2023 at 04:22:51PM -0800, Sean Christopherson wrote: > Disable the page-track notifier code at compile time if there are no > external users, i.e. if CONFIG_KVM_EXTERNAL_WRITE_TRACKING=n. KVM itself > now hooks emulated writes directly instead of relying on the page-track > mechanism. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_host.h | 2 ++ > arch/x86/include/asm/kvm_page_track.h | 2 ++ > arch/x86/kvm/mmu/page_track.c | 9 ++++----- > arch/x86/kvm/mmu/page_track.h | 29 +++++++++++++++++++++++---- > 4 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 1a4225237564..a3423711e403 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1265,7 +1265,9 @@ struct kvm_arch { > * create an NX huge page (without hanging the guest). > */ > struct list_head possible_nx_huge_pages; > +#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING > struct kvm_page_track_notifier_head track_notifier_head; > +#endif > /* > * Protects marking pages unsync during page faults, as TDP MMU page > * faults only take mmu_lock for read. For simplicity, the unsync > diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h > index deece45936a5..53c2adb25a07 100644 > --- a/arch/x86/include/asm/kvm_page_track.h > +++ b/arch/x86/include/asm/kvm_page_track.h The "#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING" can be moved to the front of this file? All the structures are only exposed for external users now. > @@ -55,6 +55,7 @@ void kvm_slot_page_track_remove_page(struct kvm *kvm, > struct kvm_memory_slot *slot, gfn_t gfn, > enum kvm_page_track_mode mode); > > +#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING > enum pg_level kvm_page_track_max_mapping_level(struct kvm *kvm, gfn_t gfn, > enum pg_level max_level); > > @@ -64,5 +65,6 @@ kvm_page_track_register_notifier(struct kvm *kvm, > void > kvm_page_track_unregister_notifier(struct kvm *kvm, > struct kvm_page_track_notifier_node *n); > +#endif /* CONFIG_KVM_EXTERNAL_WRITE_TRACKING */ > > #endif
On Wed, Mar 15, 2023, Yan Zhao wrote: > Nit: there is a typo in the commit header: "iff" -> "if" > > > -void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new, > > - int bytes) > > +void __kvm_page_track_write(struct kvm *kvm, gpa_t gpa, const u8 *new, int bytes) > Line length is 81 characters. A little longer than 80 :) > > > +static inline bool kvm_page_track_has_external_user(struct kvm *kvm) { return false; } > This line is also too long. The 80 character limit is a "soft" limit these days, e.g. checkpatch only complains if a line is 100+. In KVM x86, the preferred style is to treat the 80 char limit as "firm", for lack of a better word. E.g. let a line run over if it's just a char or two and there's no other wrapping in the declaration, but don't create long lines just because checkpatch no longer yells. There's obviously a fair bit of subjectivity, but the guideline has worked well so far (hopefully I didn't just jinx us).
On Wed, Mar 15, 2023, Yan Zhao wrote: > On Fri, Mar 10, 2023 at 04:22:51PM -0800, Sean Christopherson wrote: > > Disable the page-track notifier code at compile time if there are no > > external users, i.e. if CONFIG_KVM_EXTERNAL_WRITE_TRACKING=n. KVM itself > > now hooks emulated writes directly instead of relying on the page-track > > mechanism. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/include/asm/kvm_host.h | 2 ++ > > arch/x86/include/asm/kvm_page_track.h | 2 ++ > > arch/x86/kvm/mmu/page_track.c | 9 ++++----- > > arch/x86/kvm/mmu/page_track.h | 29 +++++++++++++++++++++++---- > > 4 files changed, 33 insertions(+), 9 deletions(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index 1a4225237564..a3423711e403 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -1265,7 +1265,9 @@ struct kvm_arch { > > * create an NX huge page (without hanging the guest). > > */ > > struct list_head possible_nx_huge_pages; > > +#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING > > struct kvm_page_track_notifier_head track_notifier_head; > > +#endif > > /* > > * Protects marking pages unsync during page faults, as TDP MMU page > > * faults only take mmu_lock for read. For simplicity, the unsync > > diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h > > index deece45936a5..53c2adb25a07 100644 > > --- a/arch/x86/include/asm/kvm_page_track.h > > +++ b/arch/x86/include/asm/kvm_page_track.h > The "#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING" can be moved to the > front of this file? > All the structures are only exposed for external users now. Huh. I've no idea why I didn't do that. IIRC, the entire reason past me wrapped track_notifier_head in an #ifdef was to allow this change in kvm_page_track.h. I'll do this in the next version unless I discover an edge case I'm overlooking. Thanks yet again!
On Wed, Mar 15, 2023 at 09:21:34AM -0700, Sean Christopherson wrote: > On Wed, Mar 15, 2023, Yan Zhao wrote: > > Nit: there is a typo in the commit header: "iff" -> "if" > > > > > -void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new, > > > - int bytes) > > > +void __kvm_page_track_write(struct kvm *kvm, gpa_t gpa, const u8 *new, int bytes) > > Line length is 81 characters. A little longer than 80 :) > > > > > +static inline bool kvm_page_track_has_external_user(struct kvm *kvm) { return false; } > > This line is also too long. > > The 80 character limit is a "soft" limit these days, e.g. checkpatch only complains > if a line is 100+. In KVM x86, the preferred style is to treat the 80 char limit > as "firm", for lack of a better word. E.g. let a line run over if it's just a > char or two and there's no other wrapping in the declaration, but don't create long > lines just because checkpatch no longer yells. > Got it. It's helpful to me! > There's obviously a fair bit of subjectivity, but the guideline has worked well > so far (hopefully I didn't just jinx us).
On Wed, Mar 15, 2023, Sean Christopherson wrote: > On Wed, Mar 15, 2023, Yan Zhao wrote: > > On Fri, Mar 10, 2023 at 04:22:51PM -0800, Sean Christopherson wrote: > > > Disable the page-track notifier code at compile time if there are no > > > external users, i.e. if CONFIG_KVM_EXTERNAL_WRITE_TRACKING=n. KVM itself > > > now hooks emulated writes directly instead of relying on the page-track > > > mechanism. > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > arch/x86/include/asm/kvm_host.h | 2 ++ > > > arch/x86/include/asm/kvm_page_track.h | 2 ++ > > > arch/x86/kvm/mmu/page_track.c | 9 ++++----- > > > arch/x86/kvm/mmu/page_track.h | 29 +++++++++++++++++++++++---- > > > 4 files changed, 33 insertions(+), 9 deletions(-) > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > index 1a4225237564..a3423711e403 100644 > > > --- a/arch/x86/include/asm/kvm_host.h > > > +++ b/arch/x86/include/asm/kvm_host.h > > > @@ -1265,7 +1265,9 @@ struct kvm_arch { > > > * create an NX huge page (without hanging the guest). > > > */ > > > struct list_head possible_nx_huge_pages; > > > +#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING > > > struct kvm_page_track_notifier_head track_notifier_head; > > > +#endif > > > /* > > > * Protects marking pages unsync during page faults, as TDP MMU page > > > * faults only take mmu_lock for read. For simplicity, the unsync > > > diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h > > > index deece45936a5..53c2adb25a07 100644 > > > --- a/arch/x86/include/asm/kvm_page_track.h > > > +++ b/arch/x86/include/asm/kvm_page_track.h > > The "#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING" can be moved to the > > front of this file? > > All the structures are only exposed for external users now. > > Huh. I've no idea why I didn't do that. IIRC, the entire reason past me wrapped > track_notifier_head in an #ifdef was to allow this change in kvm_page_track.h. > > I'll do this in the next version unless I discover an edge case I'm overlooking. Ah, deja vu. I tried this first time around, and got yelled at by the kernel test robot. Unsuprisingly, my second attempt yielded the same result :-) HDRTEST drivers/gpu/drm/i915/gvt/gvt.h In file included from <command-line>: gpu/drivers/gpu/drm/i915/gvt/gvt.h:236:45: error: field ‘track_node’ has incomplete type 236 | struct kvm_page_track_notifier_node track_node; | ^~~~~~~~~~ The problem is direct header inclusion. Nothing in the kernel includes gvt.h when CONFIG_DRM_I915_GVT=n, but the header include guard tests include headers directly on the command line. I think I'll define a "stub" specifically to play nice with this sort of testing. Guarding the guts of gvt.h with CONFIG_DRM_I915_GVT would just propagate the problem, and guarding the node definition in "struct intel_vgpu" would be confusing since the guard would be dead code for all intents and purposes. The obvious alternative would be to leave kvm_page_track_notifier_node outside of the #ifdef, but I really want to bury kvm_page_track_notifier_head for KVM's sake, and having "head" buried but not "node" would also be weird and confusing. diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h index 33f087437209..3d040741044b 100644 --- a/arch/x86/include/asm/kvm_page_track.h +++ b/arch/x86/include/asm/kvm_page_track.h @@ -51,6 +51,12 @@ void kvm_page_track_unregister_notifier(struct kvm *kvm, int kvm_write_track_add_gfn(struct kvm *kvm, gfn_t gfn); int kvm_write_track_remove_gfn(struct kvm *kvm, gfn_t gfn); +#else +/* + * Allow defining a node in a structure even if page tracking is disabled, e.g. + * to play nice with testing headers via direct inclusion from the command line. + */ +struct kvm_page_track_notifier_node {}; #endif /* CONFIG_KVM_EXTERNAL_WRITE_TRACKING */ #endif
On Thu, May 04, 2023 at 12:54:40PM -0700, Sean Christopherson wrote: > On Wed, Mar 15, 2023, Sean Christopherson wrote: > > On Wed, Mar 15, 2023, Yan Zhao wrote: > > > On Fri, Mar 10, 2023 at 04:22:51PM -0800, Sean Christopherson wrote: > > > > Disable the page-track notifier code at compile time if there are no > > > > external users, i.e. if CONFIG_KVM_EXTERNAL_WRITE_TRACKING=n. KVM itself > > > > now hooks emulated writes directly instead of relying on the page-track > > > > mechanism. > > > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > > --- > > > > arch/x86/include/asm/kvm_host.h | 2 ++ > > > > arch/x86/include/asm/kvm_page_track.h | 2 ++ > > > > arch/x86/kvm/mmu/page_track.c | 9 ++++----- > > > > arch/x86/kvm/mmu/page_track.h | 29 +++++++++++++++++++++++---- > > > > 4 files changed, 33 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > > index 1a4225237564..a3423711e403 100644 > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > @@ -1265,7 +1265,9 @@ struct kvm_arch { > > > > * create an NX huge page (without hanging the guest). > > > > */ > > > > struct list_head possible_nx_huge_pages; > > > > +#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING > > > > struct kvm_page_track_notifier_head track_notifier_head; > > > > +#endif > > > > /* > > > > * Protects marking pages unsync during page faults, as TDP MMU page > > > > * faults only take mmu_lock for read. For simplicity, the unsync > > > > diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h > > > > index deece45936a5..53c2adb25a07 100644 > > > > --- a/arch/x86/include/asm/kvm_page_track.h > > > > +++ b/arch/x86/include/asm/kvm_page_track.h > > > The "#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING" can be moved to the > > > front of this file? > > > All the structures are only exposed for external users now. > > > > Huh. I've no idea why I didn't do that. IIRC, the entire reason past me wrapped > > track_notifier_head in an #ifdef was to allow this change in kvm_page_track.h. > > > > I'll do this in the next version unless I discover an edge case I'm overlooking. > > Ah, deja vu. I tried this first time around, and got yelled at by the kernel test > robot. Unsuprisingly, my second attempt yielded the same result :-) > > HDRTEST drivers/gpu/drm/i915/gvt/gvt.h > In file included from <command-line>: > gpu/drivers/gpu/drm/i915/gvt/gvt.h:236:45: error: field ‘track_node’ has incomplete type > 236 | struct kvm_page_track_notifier_node track_node; > | ^~~~~~~~~~ > > The problem is direct header inclusion. Nothing in the kernel includes gvt.h > when CONFIG_DRM_I915_GVT=n, but the header include guard tests include headers > directly on the command line. I think I'll define a "stub" specifically to play > nice with this sort of testing. Guarding the guts of gvt.h with CONFIG_DRM_I915_GVT > would just propagate the problem, and guarding the node definition in "struct > intel_vgpu" would be confusing since the guard would be dead code for all intents > and purposes. > > The obvious alternative would be to leave kvm_page_track_notifier_node outside of > the #ifdef, but I really want to bury kvm_page_track_notifier_head for KVM's sake, > and having "head" buried but not "node" would also be weird and confusing. > > diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h > index 33f087437209..3d040741044b 100644 > --- a/arch/x86/include/asm/kvm_page_track.h > +++ b/arch/x86/include/asm/kvm_page_track.h > @@ -51,6 +51,12 @@ void kvm_page_track_unregister_notifier(struct kvm *kvm, > > int kvm_write_track_add_gfn(struct kvm *kvm, gfn_t gfn); > int kvm_write_track_remove_gfn(struct kvm *kvm, gfn_t gfn); > +#else > +/* > + * Allow defining a node in a structure even if page tracking is disabled, e.g. > + * to play nice with testing headers via direct inclusion from the command line. > + */ > +struct kvm_page_track_notifier_node {}; > #endif /* CONFIG_KVM_EXTERNAL_WRITE_TRACKING */ > > #endif > Or check CONFIG_KVM_EXTERNAL_WRITE_TRACKING in gvt.h ? e.g. diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h index 53a0a42a50db..005cdc4fb66a 100644 --- a/drivers/gpu/drm/i915/gvt/gvt.h +++ b/drivers/gpu/drm/i915/gvt/gvt.h @@ -233,7 +233,9 @@ struct intel_vgpu { unsigned long nr_cache_entries; struct mutex cache_lock; +#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING struct kvm_page_track_notifier_node track_node; +#endif #define NR_BKT (1 << 18) struct hlist_head ptable[NR_BKT]; #undef NR_BKT The justification is that gvt.h can be include without kvmgt, e.g. xengt previously. But given currently there's no such case, I'm fine with both way :)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 1a4225237564..a3423711e403 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1265,7 +1265,9 @@ struct kvm_arch { * create an NX huge page (without hanging the guest). */ struct list_head possible_nx_huge_pages; +#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING struct kvm_page_track_notifier_head track_notifier_head; +#endif /* * Protects marking pages unsync during page faults, as TDP MMU page * faults only take mmu_lock for read. For simplicity, the unsync diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h index deece45936a5..53c2adb25a07 100644 --- a/arch/x86/include/asm/kvm_page_track.h +++ b/arch/x86/include/asm/kvm_page_track.h @@ -55,6 +55,7 @@ void kvm_slot_page_track_remove_page(struct kvm *kvm, struct kvm_memory_slot *slot, gfn_t gfn, enum kvm_page_track_mode mode); +#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING enum pg_level kvm_page_track_max_mapping_level(struct kvm *kvm, gfn_t gfn, enum pg_level max_level); @@ -64,5 +65,6 @@ kvm_page_track_register_notifier(struct kvm *kvm, void kvm_page_track_unregister_notifier(struct kvm *kvm, struct kvm_page_track_notifier_node *n); +#endif /* CONFIG_KVM_EXTERNAL_WRITE_TRACKING */ #endif diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c index a21200df515d..619ec8e5fd32 100644 --- a/arch/x86/kvm/mmu/page_track.c +++ b/arch/x86/kvm/mmu/page_track.c @@ -194,6 +194,7 @@ bool kvm_slot_page_track_is_active(struct kvm *kvm, return !!READ_ONCE(slot->arch.gfn_track[mode][index]); } +#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING void kvm_page_track_cleanup(struct kvm *kvm) { struct kvm_page_track_notifier_head *head; @@ -255,14 +256,13 @@ EXPORT_SYMBOL_GPL(kvm_page_track_unregister_notifier); * The node should figure out if the written page is the one that node is * interested in by itself. */ -void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new, - int bytes) +void __kvm_page_track_write(struct kvm *kvm, gpa_t gpa, const u8 *new, int bytes) { struct kvm_page_track_notifier_head *head; struct kvm_page_track_notifier_node *n; int idx; - head = &vcpu->kvm->arch.track_notifier_head; + head = &kvm->arch.track_notifier_head; if (hlist_empty(&head->track_notifier_list)) return; @@ -273,8 +273,6 @@ void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new, if (n->track_write) n->track_write(gpa, new, bytes, n); srcu_read_unlock(&head->track_srcu, idx); - - kvm_mmu_track_write(vcpu, gpa, new, bytes); } /* @@ -317,3 +315,4 @@ enum pg_level kvm_page_track_max_mapping_level(struct kvm *kvm, gfn_t gfn, return max_level; } EXPORT_SYMBOL_GPL(kvm_page_track_max_mapping_level); +#endif diff --git a/arch/x86/kvm/mmu/page_track.h b/arch/x86/kvm/mmu/page_track.h index 89712f123ad3..931b26b8fc8f 100644 --- a/arch/x86/kvm/mmu/page_track.h +++ b/arch/x86/kvm/mmu/page_track.h @@ -6,8 +6,6 @@ #include <asm/kvm_page_track.h> -int kvm_page_track_init(struct kvm *kvm); -void kvm_page_track_cleanup(struct kvm *kvm); bool kvm_page_track_write_tracking_enabled(struct kvm *kvm); int kvm_page_track_write_tracking_alloc(struct kvm_memory_slot *slot); @@ -21,13 +19,36 @@ bool kvm_slot_page_track_is_active(struct kvm *kvm, const struct kvm_memory_slot *slot, gfn_t gfn, enum kvm_page_track_mode mode); -void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new, - int bytes); +#ifdef CONFIG_KVM_EXTERNAL_WRITE_TRACKING +int kvm_page_track_init(struct kvm *kvm); +void kvm_page_track_cleanup(struct kvm *kvm); + +void __kvm_page_track_write(struct kvm *kvm, gpa_t gpa, const u8 *new, int bytes); void kvm_page_track_delete_slot(struct kvm *kvm, struct kvm_memory_slot *slot); static inline bool kvm_page_track_has_external_user(struct kvm *kvm) { return hlist_empty(&kvm->arch.track_notifier_head.track_notifier_list); } +#else +static inline int kvm_page_track_init(struct kvm *kvm) { return 0; } +static inline void kvm_page_track_cleanup(struct kvm *kvm) { } + +static inline void __kvm_page_track_write(struct kvm *kvm, gpa_t gpa, + const u8 *new, int bytes) { } +static inline void kvm_page_track_delete_slot(struct kvm *kvm, + struct kvm_memory_slot *slot) { } + +static inline bool kvm_page_track_has_external_user(struct kvm *kvm) { return false; } + +#endif /* CONFIG_KVM_EXTERNAL_WRITE_TRACKING */ + +static inline void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, + const u8 *new, int bytes) +{ + __kvm_page_track_write(vcpu->kvm, gpa, new, bytes); + + kvm_mmu_track_write(vcpu, gpa, new, bytes); +} #endif /* __KVM_X86_PAGE_TRACK_H */
Disable the page-track notifier code at compile time if there are no external users, i.e. if CONFIG_KVM_EXTERNAL_WRITE_TRACKING=n. KVM itself now hooks emulated writes directly instead of relying on the page-track mechanism. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/kvm_host.h | 2 ++ arch/x86/include/asm/kvm_page_track.h | 2 ++ arch/x86/kvm/mmu/page_track.c | 9 ++++----- arch/x86/kvm/mmu/page_track.h | 29 +++++++++++++++++++++++---- 4 files changed, 33 insertions(+), 9 deletions(-)