| Message ID | 1430393772-27208-13-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-04-30 13:36+0200, Paolo Bonzini: > This adds an arch-specific memslot flag that hides slots unless the > VCPU is in system management mode. > > Some care is needed in order to limit the overhead of x86_gfn_to_memslot > when compared with gfn_to_memslot. Thankfully, we have __gfn_to_memslot > and search_memslots which are the same, so we can add some extra output > to search_memslots. The compiler will optimize it as dead code in > __gfn_to_memslot, and will use it to thread jumps in x86_gfn_to_memslot. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > @@ -19,10 +19,23 @@ > > #include <linux/module.h> > #include <linux/kvm_host.h> > +#include "kvm_cache_regs.h" > > struct kvm_memory_slot *x86_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn) > { > - struct kvm_memory_slot *slot = gfn_to_memslot(vcpu->kvm, gfn); > + /* By using search_memslots directly the compiler can optimize away > + * the "if (found)" check below. > + * > + * It cannot do the same for gfn_to_memslot because it is not inlined, > + * and it also cannot do the same for __gfn_to_memslot because the > + * kernel is compiled with -fno-delete-null-pointer-checks. > + */ > + bool found; > + struct kvm_memslots *memslots = kvm_memslots(vcpu->kvm); > + struct kvm_memory_slot *slot = search_memslots(memslots, gfn, &found); > + > + if (found && unlikely(slot->flags & KVM_MEM_X86_SMRAM) && !is_smm(vcpu)) > + return NULL; Patch [10/13] made me sad and IIUIC, the line above is the only reason for it ... what about renaming and changing kvm_* memory function to vcpu_* and create bool kvm_arch_vcpu_can_access_slot(vcpu, slot) which could also be inline in arch/*/include/asm/kvm_host.h thanks to the way we build. We could be passing both kvm and vcpu in internal memslot operations and not checking if vcpu is NULL. This should allow all possible operations with little code duplication and the compiler could also optimize the case where vcpu is NULL. Another option is adding something like "vcpu kvm_arch_fake_vcpu(kvm)" for cases where the access doesn't have an associated vcpu, so it would always succeed. (Might not be generic enough.) I prefer everything to copy-pasting code, so I'll try to come up with more ideas if you don't like these :) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/05/2015 19:17, Radim Kr?má? wrote: > 2015-04-30 13:36+0200, Paolo Bonzini: >> This adds an arch-specific memslot flag that hides slots unless the >> VCPU is in system management mode. >> >> Some care is needed in order to limit the overhead of x86_gfn_to_memslot >> when compared with gfn_to_memslot. Thankfully, we have __gfn_to_memslot >> and search_memslots which are the same, so we can add some extra output >> to search_memslots. The compiler will optimize it as dead code in >> __gfn_to_memslot, and will use it to thread jumps in x86_gfn_to_memslot. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt >> @@ -19,10 +19,23 @@ >> >> #include <linux/module.h> >> #include <linux/kvm_host.h> >> +#include "kvm_cache_regs.h" >> >> struct kvm_memory_slot *x86_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn) >> { >> - struct kvm_memory_slot *slot = gfn_to_memslot(vcpu->kvm, gfn); >> + /* By using search_memslots directly the compiler can optimize away >> + * the "if (found)" check below. >> + * >> + * It cannot do the same for gfn_to_memslot because it is not inlined, >> + * and it also cannot do the same for __gfn_to_memslot because the >> + * kernel is compiled with -fno-delete-null-pointer-checks. >> + */ >> + bool found; >> + struct kvm_memslots *memslots = kvm_memslots(vcpu->kvm); >> + struct kvm_memory_slot *slot = search_memslots(memslots, gfn, &found); >> + >> + if (found && unlikely(slot->flags & KVM_MEM_X86_SMRAM) && !is_smm(vcpu)) >> + return NULL; > > Patch [10/13] made me sad and IIUIC, the line above is the only reason > for it ... Yes, all the differences trickle down to using x86_gfn_to_memslot. On the other hand, there are already cut-and-pasted loops for guest memory access, see kvm_write_guest_virt_system or kvm_read_guest_virt_helper. We could add __-prefixed macros like #define __kvm_write_guest(fn_page, gpa, data, len, args...) \ ({ \ gpa_t _gpa = (gpa); \ void *_data = (data); \ int _len = (len); \ gfn_t _gfn = _gpa >> PAGE_SHIFT; \ int _offset = offset_in_page(_gpa); \ int _seg, _ret; \ while ((_seg = next_segment(_len, _offset)) != 0) { \ _ret = (fn_page)(args##, _gfn, _data, _offset, _seg); \ if (_ret < 0) \ break; \ _offset = 0; \ _len -= _seg; \ _data += _seg; \ ++_gfn; \ } \ _ret; \ }) ... int x86_write_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn, const void *data, int offset, unsigned long len) { return __kvm_write_guest_page(x86_gfn_to_memslot, gfn, data, offset, len, vcpu); } int x86_write_guest_cached(struct kvm_vcpu *vcpu, struct gfn_to_hva_cache *ghc, const void *data, unsigned long len) { return __kvm_write_guest_cached(x86_gfn_to_hva_cache_init, x86_write_guest, ghc, data, len, vcpu); } int x86_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data, unsigned long len) { return __kvm_write_guest(x86_write_guest_page, gpa, data, len, vcpu); } but frankly it seems worse than the disease. what about renaming and changing kvm_* memory function to > vcpu_* and create > bool kvm_arch_vcpu_can_access_slot(vcpu, slot) > which could also be inline in arch/*/include/asm/kvm_host.h thanks to > the way we build. > We could be passing both kvm and vcpu in internal memslot operations and > not checking if vcpu is NULL. This should allow all possible operations > with little code duplication and the compiler could also optimize the > case where vcpu is NULL. That would be a huge patch, and most architectures do not (yet) need it. I can change the functions to kvm_vcpu_read_* and when a second architecture needs it, we move it from arch/x86/kvm/ to virt/kvm. I named it x86_ just because it was the same length as kvm_ and thus hardly needed reindentation. > Another option is adding something like "vcpu kvm_arch_fake_vcpu(kvm)" > for cases where the access doesn't have an associated vcpu, so it would > always succeed. (Might not be generic enough.) That's ugly... The question is also how often the copied code is changed, and the answer is that most of it was never changed since it was introduced in 2007 (commit 195aefde9cc2, "KVM: Add general accessors to read and write guest memory"). Before then, KVM used kmap_atomic directly! Only the cache code is more recent, but that also has only been changed a couple of times after introducing it in 2010 (commit 49c7754ce570, "KVM: Add memory slot versioning and use it to provide fast guest write interface"). It is very stable code. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-05-06 11:47+0200, Paolo Bonzini: > On 05/05/2015 19:17, Radim Kr?má? wrote: >> 2015-04-30 13:36+0200, Paolo Bonzini: >>> struct kvm_memory_slot *x86_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn) >>> { >>> - struct kvm_memory_slot *slot = gfn_to_memslot(vcpu->kvm, gfn); >>> + bool found; >>> + struct kvm_memslots *memslots = kvm_memslots(vcpu->kvm); >>> + struct kvm_memory_slot *slot = search_memslots(memslots, gfn, &found); >>> + >>> + if (found && unlikely(slot->flags & KVM_MEM_X86_SMRAM) && !is_smm(vcpu)) >>> + return NULL; >> >> Patch [10/13] made me sad and IIUIC, the line above is the only reason >> for it ... > > Yes, all the differences trickle down to using x86_gfn_to_memslot. > > On the other hand, there are already cut-and-pasted loops for guest > memory access, see kvm_write_guest_virt_system or > kvm_read_guest_virt_helper. (Yeah ... not introducing new problem is a good first step to fixing the existing one. I can accept that both are okay -- the definition is up to us -- but not that we are adding an abomination on purpose.) > We could add __-prefixed macros like > > #define __kvm_write_guest(fn_page, gpa, data, len, args...) \ > ({ \ > gpa_t _gpa = (gpa); \ > void *_data = (data); \ > int _len = (len); \ > gfn_t _gfn = _gpa >> PAGE_SHIFT; \ > int _offset = offset_in_page(_gpa); \ > int _seg, _ret; \ > while ((_seg = next_segment(_len, _offset)) != 0) { \ > _ret = (fn_page)(args##, _gfn, _data, _offset, _seg); \ > if (_ret < 0) \ > break; \ > _offset = 0; \ > _len -= _seg; \ > _data += _seg; \ > ++_gfn; \ > } \ > _ret; \ > }) > > ... > > int x86_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data, > unsigned long len) > { > return __kvm_write_guest(x86_write_guest_page, gpa, data, len, vcpu); > } > > but frankly it seems worse than the disease. Well, it's a good approach, but the C language makes it awkward. (I like first class functions.) > what about renaming and changing kvm_* memory function to >> vcpu_* and create >> bool kvm_arch_vcpu_can_access_slot(vcpu, slot) >> which could also be inline in arch/*/include/asm/kvm_host.h thanks to >> the way we build. >> We could be passing both kvm and vcpu in internal memslot operations and >> not checking if vcpu is NULL. This should allow all possible operations >> with little code duplication and the compiler could also optimize the >> case where vcpu is NULL. > > That would be a huge patch, and most architectures do not (yet) need it. Not that huge ... trivial extension for passing extra argument around and adding few wrappers to keep compatibility and then a bunch of static inline bool .*(vcpu, slot) { return true; } for remaining arches. (We could have a default unless an arch #defines KVM_ARCH_VCPU_SLOT_CHECKING or some other hack to anger programmers.) The hard part is have the same object code and added flexibility in C. > I can change the functions to kvm_vcpu_read_* and when a second architecture > needs it, we move it from arch/x86/kvm/ to virt/kvm. I named it x86_ just > because it was the same length as kvm_ and thus hardly needed reindentation. That doesn't improve the main issue, so x86 is good. >> Another option is adding something like "vcpu kvm_arch_fake_vcpu(kvm)" >> for cases where the access doesn't have an associated vcpu, so it would >> always succeed. (Might not be generic enough.) > > That's ugly... Yes. (And I still prefer it.) > The question is also how often the copied code is changed, and the answer is > that most of it was never changed since it was introduced in 2007 > (commit 195aefde9cc2, "KVM: Add general accessors to read and write guest > memory"). Before then, KVM used kmap_atomic directly! > > Only the cache code is more recent, but that also has only been changed a > couple of times after introducing it in 2010 (commit 49c7754ce570, "KVM: > Add memory slot versioning and use it to provide fast guest write interface"). > It is very stable code. We have different views on code duplication :) The feature you wanted exposed a flaw in the code, so an extension was needed. Copying code is the last resort after all options of abstracting were exhausted ... I might be forcing common paths when writing it twice requires less brain power, but 200 lines of structurally identical code seem far from it. Reworking stable code is simpler, as we can just cover all features needed now and omit the hard thinking about future extensions. (For me, stable code is the first candidate for generalization ... and I wouldn't copy it, even though it's mostly fine in practice.) It's all nice in theory; I'll prepare a patch we can discuss. (And maybe agree with this one after understanding all challenges.) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Radim Kr?má? <rkrcmar@redhat.com> writes: ... > > That doesn't improve the main issue, so x86 is good. > >>> Another option is adding something like "vcpu kvm_arch_fake_vcpu(kvm)" >>> for cases where the access doesn't have an associated vcpu, so it would >>> always succeed. (Might not be generic enough.) >> >> That's ugly... > > Yes. (And I still prefer it.) Ooh, I hope we don't go this route :) I understand the motivation but if there would be cases where we would have to fake the condition to be true, maybe we should reconsider our design. Bandan -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/05/2015 18:24, Radim Kr?má? wrote: > The feature you wanted exposed a flaw in the code, so an extension was > needed. Copying code is the last resort after all options of > abstracting were exhausted ... I might be forcing common paths when > writing it twice requires less brain power, but 200 lines of > structurally identical code seem far from it. Note that it didn't really expose a flaw in the code, just a limitation. There are cases even on x86 where you have no vcpu, for example filling in the EPT identity page tables or VMX TSS. > Reworking stable code is simpler, as we can just cover all features > needed now and omit the hard thinking about future extensions. > (For me, stable code is the first candidate for generalization ... > and I wouldn't copy it, even though it's mostly fine in practice.) Stable simple code is also important to keep simple though. Sometimes code duplication is preferrable to obfuscation. I agree that copying 200 lines of code because of one function three levels down the call chain isn't nice. However, it doesn't seem particularly easy to avoid the duplication even with C++ templates. C is worse. OCaml or Haskell would be nicer. :) Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/30/2015 02:36 PM, Paolo Bonzini wrote: > This adds an arch-specific memslot flag that hides slots unless the > VCPU is in system management mode. > > Some care is needed in order to limit the overhead of x86_gfn_to_memslot > when compared with gfn_to_memslot. Thankfully, we have __gfn_to_memslot > and search_memslots which are the same, so we can add some extra output > to search_memslots. The compiler will optimize it as dead code in > __gfn_to_memslot, and will use it to thread jumps in x86_gfn_to_memslot. > > > struct kvm_memory_slot *x86_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn) > { > - struct kvm_memory_slot *slot = gfn_to_memslot(vcpu->kvm, gfn); > + /* By using search_memslots directly the compiler can optimize away > + * the "if (found)" check below. > + * > + * It cannot do the same for gfn_to_memslot because it is not inlined, > + * and it also cannot do the same for __gfn_to_memslot because the > + * kernel is compiled with -fno-delete-null-pointer-checks. > + */ > + bool found; > + struct kvm_memslots *memslots = kvm_memslots(vcpu->kvm); > + struct kvm_memory_slot *slot = search_memslots(memslots, gfn, &found); > + > + if (found && unlikely(slot->flags & KVM_MEM_X86_SMRAM) && !is_smm(vcpu)) > + return NULL; > Alternative approach: store the memslot pointer in the vcpu, instead of of vcpu->kvm. The uapi interface code can generate two memslot tables to be stored in kvm->, one for smm and one for !smm. This is a little more generic and can be used for other asymmetric memory maps causes (the only one I can think of, mapping the APIC to different physical addresses, can't be efficiently supported). > return slot; > } > @@ -112,7 +125,15 @@ EXPORT_SYMBOL_GPL(x86_read_guest); > int x86_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc, > gpa_t gpa, unsigned long len) > { > - return kvm_gfn_to_hva_cache_init(kvm, ghc, gpa, len); > + int r = kvm_gfn_to_hva_cache_init(kvm, ghc, gpa, len); > + > + if (r < 0) > + return r; > + > + /* Use slow path for reads and writes to SMRAM. */ > + if (ghc->memslot && (ghc->memslot->flags & KVM_MEM_X86_SMRAM)) > + ghc->memslot = NULL; > + return r; > } > EXPORT_SYMBOL_GPL(x86_gfn_to_hva_cache_init); > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index e7cfee09970a..ea3db5d89f67 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -800,16 +800,18 @@ static inline void kvm_guest_exit(void) > * gfn_to_memslot() itself isn't here as an inline because that would > * bloat other code too much. > */ > -static inline struct kvm_memory_slot * > -search_memslots(struct kvm_memslots *slots, gfn_t gfn) > +static __always_inline struct kvm_memory_slot * > +search_memslots(struct kvm_memslots *slots, gfn_t gfn, bool *found) > { > int start = 0, end = slots->used_slots; > int slot = atomic_read(&slots->lru_slot); > struct kvm_memory_slot *memslots = slots->memslots; > > if (gfn >= memslots[slot].base_gfn && > - gfn < memslots[slot].base_gfn + memslots[slot].npages) > + gfn < memslots[slot].base_gfn + memslots[slot].npages) { > + *found = true; > return &memslots[slot]; > + } > > while (start < end) { > slot = start + (end - start) / 2; > @@ -823,16 +825,20 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn) > if (gfn >= memslots[start].base_gfn && > gfn < memslots[start].base_gfn + memslots[start].npages) { > atomic_set(&slots->lru_slot, start); > + *found = true; > return &memslots[start]; > } > > + *found = false; > return NULL; > } > > static inline struct kvm_memory_slot * > __gfn_to_memslot(struct kvm_memslots *slots, gfn_t gfn) > { > - return search_memslots(slots, gfn); > + bool found; > + > + return search_memslots(slots, gfn, &found); > } > > static inline unsigned long > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index e82c46b9860f..b8b76106a003 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -716,6 +716,10 @@ static int check_memory_region_flags(struct kvm_userspace_memory_region *mem) > #ifdef __KVM_HAVE_READONLY_MEM > valid_flags |= KVM_MEM_READONLY; > #endif > +#ifdef __KVM_ARCH_VALID_FLAGS > + BUILD_BUG_ON(__KVM_ARCH_VALID_FLAGS & KVM_MEMSLOT_INVALID); > + valid_flags |= __KVM_ARCH_VALID_FLAGS; > +#endif > > if (mem->flags & ~valid_flags) > return -EINVAL; -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/05/2015 22:32, Avi Kivity wrote: > Alternative approach: store the memslot pointer in the vcpu, instead of > of vcpu->kvm. The uapi interface code can generate two memslot tables > to be stored in kvm->, one for smm and one for !smm. That's a great suggestion---and easy to implement too. Thanks Avi for chiming in! Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index c1afcb2e4b89..bc71f347782d 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -931,18 +931,24 @@ It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr be identical. This allows large pages in the guest to be backed by large pages in the host. -The flags field supports two flags: KVM_MEM_LOG_DIRTY_PAGES and -KVM_MEM_READONLY. The former can be set to instruct KVM to keep track of -writes to memory within the slot. See KVM_GET_DIRTY_LOG ioctl to know how to -use it. The latter can be set, if KVM_CAP_READONLY_MEM capability allows it, -to make a new slot read-only. In this case, writes to this memory will be -posted to userspace as KVM_EXIT_MMIO exits. +The flags field supports two architecture-independent flags: +KVM_MEM_LOG_DIRTY_PAGES and KVM_MEM_READONLY. The former can be set +to instruct KVM to keep track of writes to memory within the slot. +See KVM_GET_DIRTY_LOG ioctl to know how to use it. The latter can +be set, if KVM_CAP_READONLY_MEM capability allows it, to make a new +slot read-only. In this case, writes to this memory will be posted to +userspace as KVM_EXIT_MMIO exits. When the KVM_CAP_SYNC_MMU capability is available, changes in the backing of the memory region are automatically reflected into the guest. For example, an mmap() that affects the region will be made visible immediately. Another example is madvise(MADV_DROP). +Each architectures can support other specific flags. Right now there is +only one defined. On x86, if KVM_CAP_X86_SMM is available, the +KVM_MEM_X86_SMRAM flag will hide the slot to VCPUs that are not +in system management mode. + It is recommended to use this API instead of the KVM_SET_MEMORY_REGION ioctl. The KVM_SET_MEMORY_REGION does not allow fine grained control over memory allocation and is deprecated. diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h index 0bb09e2e549e..093b0dcb4c0c 100644 --- a/arch/x86/include/uapi/asm/kvm.h +++ b/arch/x86/include/uapi/asm/kvm.h @@ -45,6 +45,9 @@ #define __KVM_HAVE_XCRS #define __KVM_HAVE_READONLY_MEM +#define __KVM_ARCH_VALID_FLAGS KVM_MEM_X86_SMRAM +#define KVM_MEM_X86_SMRAM (1 << 24) + /* Architectural interrupt line count. */ #define KVM_NR_INTERRUPTS 256 diff --git a/arch/x86/kvm/smram.c b/arch/x86/kvm/smram.c index 73616edab631..e7dd933673a4 100644 --- a/arch/x86/kvm/smram.c +++ b/arch/x86/kvm/smram.c @@ -19,10 +19,23 @@ #include <linux/module.h> #include <linux/kvm_host.h> +#include "kvm_cache_regs.h" struct kvm_memory_slot *x86_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn) { - struct kvm_memory_slot *slot = gfn_to_memslot(vcpu->kvm, gfn); + /* By using search_memslots directly the compiler can optimize away + * the "if (found)" check below. + * + * It cannot do the same for gfn_to_memslot because it is not inlined, + * and it also cannot do the same for __gfn_to_memslot because the + * kernel is compiled with -fno-delete-null-pointer-checks. + */ + bool found; + struct kvm_memslots *memslots = kvm_memslots(vcpu->kvm); + struct kvm_memory_slot *slot = search_memslots(memslots, gfn, &found); + + if (found && unlikely(slot->flags & KVM_MEM_X86_SMRAM) && !is_smm(vcpu)) + return NULL; return slot; } @@ -112,7 +125,15 @@ EXPORT_SYMBOL_GPL(x86_read_guest); int x86_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc, gpa_t gpa, unsigned long len) { - return kvm_gfn_to_hva_cache_init(kvm, ghc, gpa, len); + int r = kvm_gfn_to_hva_cache_init(kvm, ghc, gpa, len); + + if (r < 0) + return r; + + /* Use slow path for reads and writes to SMRAM. */ + if (ghc->memslot && (ghc->memslot->flags & KVM_MEM_X86_SMRAM)) + ghc->memslot = NULL; + return r; } EXPORT_SYMBOL_GPL(x86_gfn_to_hva_cache_init); diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index e7cfee09970a..ea3db5d89f67 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -800,16 +800,18 @@ static inline void kvm_guest_exit(void) * gfn_to_memslot() itself isn't here as an inline because that would * bloat other code too much. */ -static inline struct kvm_memory_slot * -search_memslots(struct kvm_memslots *slots, gfn_t gfn) +static __always_inline struct kvm_memory_slot * +search_memslots(struct kvm_memslots *slots, gfn_t gfn, bool *found) { int start = 0, end = slots->used_slots; int slot = atomic_read(&slots->lru_slot); struct kvm_memory_slot *memslots = slots->memslots; if (gfn >= memslots[slot].base_gfn && - gfn < memslots[slot].base_gfn + memslots[slot].npages) + gfn < memslots[slot].base_gfn + memslots[slot].npages) { + *found = true; return &memslots[slot]; + } while (start < end) { slot = start + (end - start) / 2; @@ -823,16 +825,20 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn) if (gfn >= memslots[start].base_gfn && gfn < memslots[start].base_gfn + memslots[start].npages) { atomic_set(&slots->lru_slot, start); + *found = true; return &memslots[start]; } + *found = false; return NULL; } static inline struct kvm_memory_slot * __gfn_to_memslot(struct kvm_memslots *slots, gfn_t gfn) { - return search_memslots(slots, gfn); + bool found; + + return search_memslots(slots, gfn, &found); } static inline unsigned long diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index e82c46b9860f..b8b76106a003 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -716,6 +716,10 @@ static int check_memory_region_flags(struct kvm_userspace_memory_region *mem) #ifdef __KVM_HAVE_READONLY_MEM valid_flags |= KVM_MEM_READONLY; #endif +#ifdef __KVM_ARCH_VALID_FLAGS + BUILD_BUG_ON(__KVM_ARCH_VALID_FLAGS & KVM_MEMSLOT_INVALID); + valid_flags |= __KVM_ARCH_VALID_FLAGS; +#endif if (mem->flags & ~valid_flags) return -EINVAL;
This adds an arch-specific memslot flag that hides slots unless the VCPU is in system management mode. Some care is needed in order to limit the overhead of x86_gfn_to_memslot when compared with gfn_to_memslot. Thankfully, we have __gfn_to_memslot and search_memslots which are the same, so we can add some extra output to search_memslots. The compiler will optimize it as dead code in __gfn_to_memslot, and will use it to thread jumps in x86_gfn_to_memslot. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- Documentation/virtual/kvm/api.txt | 18 ++++++++++++------ arch/x86/include/uapi/asm/kvm.h | 3 +++ arch/x86/kvm/smram.c | 25 +++++++++++++++++++++++-- include/linux/kvm_host.h | 14 ++++++++++---- virt/kvm/kvm_main.c | 4 ++++ 5 files changed, 52 insertions(+), 12 deletions(-)