| Message ID | 20230311002258.852397-20-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:50PM -0800, Sean Christopherson wrote: > Bury the declaration of the page-track helpers that are intended only for > internal KVM use in a "private" header. In addition to guarding against > unwanted usage of the internal-only helpers, dropping their definitions > avoids exposing other structures that should be KVM-internal, e.g. for > memslots. This is a baby step toward making kvm_host.h a KVM-internal > header in the very distant future. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_page_track.h | 26 ++++----------------- > arch/x86/kvm/mmu/mmu.c | 3 ++- > arch/x86/kvm/mmu/page_track.c | 8 +------ > arch/x86/kvm/mmu/page_track.h | 33 +++++++++++++++++++++++++++ > arch/x86/kvm/x86.c | 1 + > 5 files changed, 42 insertions(+), 29 deletions(-) > create mode 100644 arch/x86/kvm/mmu/page_track.h > > diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h > index e5eb98ca4fce..deece45936a5 100644 > --- a/arch/x86/include/asm/kvm_page_track.h > +++ b/arch/x86/include/asm/kvm_page_track.h A curious question: are arch/x86/include/asm/kvm_*.h all expected to be external accessible? Thanks Yan
On Wed, Mar 15, 2023, Yan Zhao wrote: > On Fri, Mar 10, 2023 at 04:22:50PM -0800, Sean Christopherson wrote: > > Bury the declaration of the page-track helpers that are intended only for > > internal KVM use in a "private" header. In addition to guarding against > > unwanted usage of the internal-only helpers, dropping their definitions > > avoids exposing other structures that should be KVM-internal, e.g. for > > memslots. This is a baby step toward making kvm_host.h a KVM-internal > > header in the very distant future. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/include/asm/kvm_page_track.h | 26 ++++----------------- > > arch/x86/kvm/mmu/mmu.c | 3 ++- > > arch/x86/kvm/mmu/page_track.c | 8 +------ > > arch/x86/kvm/mmu/page_track.h | 33 +++++++++++++++++++++++++++ > > arch/x86/kvm/x86.c | 1 + > > 5 files changed, 42 insertions(+), 29 deletions(-) > > create mode 100644 arch/x86/kvm/mmu/page_track.h > > > > diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h > > index e5eb98ca4fce..deece45936a5 100644 > > --- a/arch/x86/include/asm/kvm_page_track.h > > +++ b/arch/x86/include/asm/kvm_page_track.h > > A curious question: > are arch/x86/include/asm/kvm_*.h all expected to be external accessible? Depends on what you mean by "expected". Currently, yes, everything in there is globally visible. But the vast majority of structs, defines, functions, etc. aren't intended for external non-KVM consumption, things ended up being globally visible largely through carelessness and/or a lack of a forcing function. E.g. there is absolutely no reason anything outside of KVM should need arch/x86/include/asm/kvm-x86-ops.h, but it landed in asm/ because, at the time it was added, nothing would be harmed by making kvm-x86-ops.h "public" and we didn't scrutinize the patches well enough. My primary motivation for this series is to (eventually) get to a state where only select symbols/defines/etc. are exposed by KVM to the outside world, and everything else is internal only. The end goal of tightly restricting KVM's global API is to allow concurrently loading multiple instances of kvm.ko so that userspace can upgrade/rollback KVM without needed to move VMs off the host, i.e. by performing intrahost migration between differenate instances of KVM on the same host. To do that safely, anything that is visible outside of KVM needs to be compatible across different instances of KVM, e.g. if kvm_vcpu is "public" then a KVM upgrade/rollback wouldn't be able to touch "struct kvm_vcpu" in any way. We'll definitely want to be able to modify things like the vCPU structures, thus the push to restrict the API. But even if we never realize that end goal, IMO drastically reducing KVM's "public" API surface is worthy goal in and of itself.
On Wed, Mar 15, 2023 at 08:13:37AM -0700, Sean Christopherson wrote: > > A curious question: > > are arch/x86/include/asm/kvm_*.h all expected to be external accessible? > > Depends on what you mean by "expected". Currently, yes, everything in there is > globally visible. But the vast majority of structs, defines, functions, etc. aren't > intended for external non-KVM consumption, things ended up being globally visible > largely through carelessness and/or a lack of a forcing function. > > E.g. there is absolutely no reason anything outside of KVM should need > arch/x86/include/asm/kvm-x86-ops.h, but it landed in asm/ because, at the time it > was added, nothing would be harmed by making kvm-x86-ops.h "public" and we didn't > scrutinize the patches well enough. > > My primary motivation for this series is to (eventually) get to a state where only > select symbols/defines/etc. are exposed by KVM to the outside world, and everything > else is internal only. The end goal of tightly restricting KVM's global API is to > allow concurrently loading multiple instances of kvm.ko so that userspace can > upgrade/rollback KVM without needed to move VMs off the host, i.e. by performing > intrahost migration between differenate instances of KVM on the same host. To do > that safely, anything that is visible outside of KVM needs to be compatible across > different instances of KVM, e.g. if kvm_vcpu is "public" then a KVM upgrade/rollback > wouldn't be able to touch "struct kvm_vcpu" in any way. We'll definitely want to be > able to modify things like the vCPU structures, thus the push to restrict the API. > > But even if we never realize that end goal, IMO drastically reducing KVM's "public" > API surface is worthy goal in and of itself. Got it. Thanks for explanation!
diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h index e5eb98ca4fce..deece45936a5 100644 --- a/arch/x86/include/asm/kvm_page_track.h +++ b/arch/x86/include/asm/kvm_page_track.h @@ -2,6 +2,8 @@ #ifndef _ASM_X86_KVM_PAGE_TRACK_H #define _ASM_X86_KVM_PAGE_TRACK_H +#include <linux/kvm_types.h> + enum kvm_page_track_mode { KVM_PAGE_TRACK_WRITE, KVM_PAGE_TRACK_MAX, @@ -46,28 +48,15 @@ struct kvm_page_track_notifier_node { struct kvm_page_track_notifier_node *node); }; -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); -enum pg_level kvm_page_track_max_mapping_level(struct kvm *kvm, gfn_t gfn, - enum pg_level max_level); - -void kvm_page_track_free_memslot(struct kvm_memory_slot *slot); -int kvm_page_track_create_memslot(struct kvm *kvm, - struct kvm_memory_slot *slot, - unsigned long npages); - void kvm_slot_page_track_add_page(struct kvm *kvm, struct kvm_memory_slot *slot, gfn_t gfn, enum kvm_page_track_mode mode); void kvm_slot_page_track_remove_page(struct kvm *kvm, struct kvm_memory_slot *slot, gfn_t gfn, enum kvm_page_track_mode mode); -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); + +enum pg_level kvm_page_track_max_mapping_level(struct kvm *kvm, gfn_t gfn, + enum pg_level max_level); void kvm_page_track_register_notifier(struct kvm *kvm, @@ -75,10 +64,5 @@ kvm_page_track_register_notifier(struct kvm *kvm, void kvm_page_track_unregister_notifier(struct kvm *kvm, struct kvm_page_track_notifier_node *n); -void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new, - int bytes); -void kvm_page_track_delete_slot(struct kvm *kvm, struct kvm_memory_slot *slot); - -bool kvm_page_track_has_external_user(struct kvm *kvm); #endif diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 4f2f83d8322e..e192968340bf 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -25,6 +25,7 @@ #include "kvm_cache_regs.h" #include "smm.h" #include "kvm_emulate.h" +#include "page_track.h" #include "cpuid.h" #include "spte.h" @@ -53,7 +54,7 @@ #include <asm/io.h> #include <asm/set_memory.h> #include <asm/vmx.h> -#include <asm/kvm_page_track.h> + #include "trace.h" extern bool itlb_multihit_kvm_mitigation; diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c index 907ab8abb452..a21200df515d 100644 --- a/arch/x86/kvm/mmu/page_track.c +++ b/arch/x86/kvm/mmu/page_track.c @@ -15,10 +15,9 @@ #include <linux/kvm_host.h> #include <linux/rculist.h> -#include <asm/kvm_page_track.h> - #include "mmu.h" #include "mmu_internal.h" +#include "page_track.h" bool kvm_page_track_write_tracking_enabled(struct kvm *kvm) { @@ -318,8 +317,3 @@ 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); - -bool kvm_page_track_has_external_user(struct kvm *kvm) -{ - return hlist_empty(&kvm->arch.track_notifier_head.track_notifier_list); -} diff --git a/arch/x86/kvm/mmu/page_track.h b/arch/x86/kvm/mmu/page_track.h new file mode 100644 index 000000000000..89712f123ad3 --- /dev/null +++ b/arch/x86/kvm/mmu/page_track.h @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __KVM_X86_PAGE_TRACK_H +#define __KVM_X86_PAGE_TRACK_H + +#include <linux/kvm_host.h> + +#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); + +void kvm_page_track_free_memslot(struct kvm_memory_slot *slot); +int kvm_page_track_create_memslot(struct kvm *kvm, + struct kvm_memory_slot *slot, + unsigned long npages); + +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); +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); +} + +#endif /* __KVM_X86_PAGE_TRACK_H */ diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 59b02650cefc..ba61e51c05ed 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -25,6 +25,7 @@ #include "tss.h" #include "kvm_cache_regs.h" #include "kvm_emulate.h" +#include "mmu/page_track.h" #include "x86.h" #include "cpuid.h" #include "pmu.h"
Bury the declaration of the page-track helpers that are intended only for internal KVM use in a "private" header. In addition to guarding against unwanted usage of the internal-only helpers, dropping their definitions avoids exposing other structures that should be KVM-internal, e.g. for memslots. This is a baby step toward making kvm_host.h a KVM-internal header in the very distant future. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/kvm_page_track.h | 26 ++++----------------- arch/x86/kvm/mmu/mmu.c | 3 ++- arch/x86/kvm/mmu/page_track.c | 8 +------ arch/x86/kvm/mmu/page_track.h | 33 +++++++++++++++++++++++++++ arch/x86/kvm/x86.c | 1 + 5 files changed, 42 insertions(+), 29 deletions(-) create mode 100644 arch/x86/kvm/mmu/page_track.h