| Message ID | 20201214083905.2017260-2-dwmw2@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Add minimal support for Xen HVM guests | expand |
David Woodhouse <dwmw2@infradead.org> writes: > From: David Woodhouse <dwmw@amazon.co.uk> > > It shouldn't take a vcpu. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > arch/x86/kvm/x86.c | 8 ++++---- > include/linux/kvm_host.h | 4 ++-- > virt/kvm/kvm_main.c | 8 ++++---- > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index e545a8a613b1..c7f1ba21212e 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2957,7 +2957,7 @@ static void record_steal_time(struct kvm_vcpu *vcpu) > return; > > /* -EAGAIN is returned in atomic context so we can just return. */ > - if (kvm_map_gfn(vcpu, vcpu->arch.st.msr_val >> PAGE_SHIFT, > + if (kvm_map_gfn(vcpu->kvm, vcpu->arch.st.msr_val >> PAGE_SHIFT, > &map, &vcpu->arch.st.cache, false)) > return; > > @@ -2992,7 +2992,7 @@ static void record_steal_time(struct kvm_vcpu *vcpu) > > st->version += 1; > > - kvm_unmap_gfn(vcpu, &map, &vcpu->arch.st.cache, true, false); > + kvm_unmap_gfn(vcpu->kvm, &map, &vcpu->arch.st.cache, true, false); > } > > int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > @@ -3981,7 +3981,7 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu) > if (vcpu->arch.st.preempted) > return; > > - if (kvm_map_gfn(vcpu, vcpu->arch.st.msr_val >> PAGE_SHIFT, &map, > + if (kvm_map_gfn(vcpu->kvm, vcpu->arch.st.msr_val >> PAGE_SHIFT, &map, > &vcpu->arch.st.cache, true)) > return; > > @@ -3990,7 +3990,7 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu) > > st->preempted = vcpu->arch.st.preempted = KVM_VCPU_PREEMPTED; > > - kvm_unmap_gfn(vcpu, &map, &vcpu->arch.st.cache, true, true); > + kvm_unmap_gfn(vcpu->kvm, &map, &vcpu->arch.st.cache, true, true); > } > > void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 7f2e2a09ebbd..8eb5eb1399f5 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -806,11 +806,11 @@ struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn > kvm_pfn_t kvm_vcpu_gfn_to_pfn_atomic(struct kvm_vcpu *vcpu, gfn_t gfn); > kvm_pfn_t kvm_vcpu_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn); > int kvm_vcpu_map(struct kvm_vcpu *vcpu, gpa_t gpa, struct kvm_host_map *map); > -int kvm_map_gfn(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map, > +int kvm_map_gfn(struct kvm *kvm, gfn_t gfn, struct kvm_host_map *map, > struct gfn_to_pfn_cache *cache, bool atomic); > struct page *kvm_vcpu_gfn_to_page(struct kvm_vcpu *vcpu, gfn_t gfn); > void kvm_vcpu_unmap(struct kvm_vcpu *vcpu, struct kvm_host_map *map, bool dirty); > -int kvm_unmap_gfn(struct kvm_vcpu *vcpu, struct kvm_host_map *map, > +int kvm_unmap_gfn(struct kvm *kvm, struct kvm_host_map *map, > struct gfn_to_pfn_cache *cache, bool dirty, bool atomic); > unsigned long kvm_vcpu_gfn_to_hva(struct kvm_vcpu *vcpu, gfn_t gfn); > unsigned long kvm_vcpu_gfn_to_hva_prot(struct kvm_vcpu *vcpu, gfn_t gfn, bool *writable); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2541a17ff1c4..f01a8df7806a 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2181,10 +2181,10 @@ static int __kvm_map_gfn(struct kvm_memslots *slots, gfn_t gfn, > return 0; > } > > -int kvm_map_gfn(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map, > +int kvm_map_gfn(struct kvm *kvm, gfn_t gfn, struct kvm_host_map *map, > struct gfn_to_pfn_cache *cache, bool atomic) > { > - return __kvm_map_gfn(kvm_memslots(vcpu->kvm), gfn, map, > + return __kvm_map_gfn(kvm_memslots(kvm), gfn, map, > cache, atomic); > } > EXPORT_SYMBOL_GPL(kvm_map_gfn); > @@ -2232,10 +2232,10 @@ static void __kvm_unmap_gfn(struct kvm_memory_slot *memslot, > map->page = NULL; > } > > -int kvm_unmap_gfn(struct kvm_vcpu *vcpu, struct kvm_host_map *map, > +int kvm_unmap_gfn(struct kvm *kvm, struct kvm_host_map *map, > struct gfn_to_pfn_cache *cache, bool dirty, bool atomic) > { > - __kvm_unmap_gfn(gfn_to_memslot(vcpu->kvm, map->gfn), map, > + __kvm_unmap_gfn(gfn_to_memslot(kvm, map->gfn), map, > cache, dirty, atomic); > return 0; > } What about different address space ids? gfn_to_memslot() now calls kvm_memslots() which gives memslots for address space id = 0 but what if we want something different? Note, different vCPUs can (in theory) be in different address spaces so we actually need 'vcpu' and not 'kvm' then.
On 14 December 2020 21:13:40 GMT, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >What about different address space ids? > >gfn_to_memslot() now calls kvm_memslots() which gives memslots for >address space id = 0 but what if we want something different? Note, >different vCPUs can (in theory) be in different address spaces so we >actually need 'vcpu' and not 'kvm' then. Sure, but then you want a different function; this one is called 'kvm_map_gfn()' and it operates on kvm_memslots(). It *doesn't* operate on the vcpu at all. Which is why it's so bizarre that its argument is a 'vcpu' which it only ever uses to get vcpu->kvm from it. It should just take the kvm pointer.
David Woodhouse <dwmw2@infradead.org> writes: > On 14 December 2020 21:13:40 GMT, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >>What about different address space ids? >> >>gfn_to_memslot() now calls kvm_memslots() which gives memslots for >>address space id = 0 but what if we want something different? Note, >>different vCPUs can (in theory) be in different address spaces so we >>actually need 'vcpu' and not 'kvm' then. > > Sure, but then you want a different function; this one is called > 'kvm_map_gfn()' and it operates on kvm_memslots(). It *doesn't* > operate on the vcpu at all. > Actually, we already have kvm_vcpu_map() which uses kvm_vcpu_memslots() inside so no new function is needed. > Which is why it's so bizarre that its argument is a 'vcpu' which it > only ever uses to get vcpu->kvm from it. It should just take the kvm > pointer. Your change is correct but I'm not sure that it's entirely clear that kvm_map_gfn() implicitly uses 'as_id=0' and I don't even see a comment about the fact :-(
On 14 December 2020 21:41:23 GMT, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >>Your change is correct but I'm not sure that it's entirely clear that >kvm_map_gfn() implicitly uses 'as_id=0' and I don't even see a comment >about the fact :-( Isn't that true of all the kvm_read_guest and kvm_write_guest functions and indeed of kvm_memslots() itself?
David Woodhouse <dwmw2@infradead.org> writes: > On 14 December 2020 21:41:23 GMT, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >>>Your change is correct but I'm not sure that it's entirely clear that >>kvm_map_gfn() implicitly uses 'as_id=0' and I don't even see a comment >>about the fact :-( > > Isn't that true of all the kvm_read_guest and kvm_write_guest > functions and indeed of kvm_memslots() itself? Yes, sadly. Multiple address spaces support was added to KVM as a generic feature but the only use-case for it at this moment is SMM on x86 which is 'special', i.e. currently there's only one user for kvm_map_gfn()/kvm_unmap_gfn() which is steal time accounting and it's not easy to come up with a use-case when this PV feature needs to be used from SMM. On the other hand, if we try using multiple address spaces in KVM for e.g. emulating something like Hyper-V VSM, it becomes unclear which address space id needs to be used even for steal time. To be entirely correct, we probably need to remember as_id which was active when steal time was enabled and stick to it later when we want to update the data. If we do that, kvm_map_gfn() will lose its only user. All the above doesn't make your patch incorrect of course, I just used it to express my observation that we seem to be using as_id=0 blindly and the API we have contributes to that.
On Tue, 2020-12-15 at 13:07 +0100, Vitaly Kuznetsov wrote: > Yes, sadly. Multiple address spaces support was added to KVM as a > generic feature but the only use-case for it at this moment is SMM on > x86 which is 'special', i.e. currently there's only one user for > kvm_map_gfn()/kvm_unmap_gfn() which is steal time accounting and it's > not easy to come up with a use-case when this PV feature needs to be > used from SMM. On the other hand, if we try using multiple address > spaces in KVM for e.g. emulating something like Hyper-V VSM, it becomes > unclear which address space id needs to be used even for steal > time. To be entirely correct, we probably need to remember as_id which > was active when steal time was enabled and stick to it later when we > want to update the data. If we do that, kvm_map_gfn() will lose its only > user. I'm also OK with just completely deleting kvm_map_gfn(). I'm not even using it any more myself; I just kept this patch in my v3 series because it's a sane cleanup. The steal time accounting doesn't *need* to have a kernel mapping of the page just so it can do atomic xchg on it; we can do that just fine on userspace addresses, and do exactly that for futexes. So I'm probably going to knock up a kvm_cmpxchg_guest_offset_cached() and make the steal time code use that, on the *userspace* address. There's already a futex_atomic_cmpxchg_inatomic() which does the actual operation. (It returns -EFAULT for an absent page and lets the caller worry about faulting it in, which isn't quite what I want, but it's a good start and I can either wrap the existing arch-provided function like futexes do, or use it as a basis for a version that waits.) And by an amazing coincidence that's also the function I need for accelerating Xen event channel support... :)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index e545a8a613b1..c7f1ba21212e 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -2957,7 +2957,7 @@ static void record_steal_time(struct kvm_vcpu *vcpu) return; /* -EAGAIN is returned in atomic context so we can just return. */ - if (kvm_map_gfn(vcpu, vcpu->arch.st.msr_val >> PAGE_SHIFT, + if (kvm_map_gfn(vcpu->kvm, vcpu->arch.st.msr_val >> PAGE_SHIFT, &map, &vcpu->arch.st.cache, false)) return; @@ -2992,7 +2992,7 @@ static void record_steal_time(struct kvm_vcpu *vcpu) st->version += 1; - kvm_unmap_gfn(vcpu, &map, &vcpu->arch.st.cache, true, false); + kvm_unmap_gfn(vcpu->kvm, &map, &vcpu->arch.st.cache, true, false); } int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) @@ -3981,7 +3981,7 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu) if (vcpu->arch.st.preempted) return; - if (kvm_map_gfn(vcpu, vcpu->arch.st.msr_val >> PAGE_SHIFT, &map, + if (kvm_map_gfn(vcpu->kvm, vcpu->arch.st.msr_val >> PAGE_SHIFT, &map, &vcpu->arch.st.cache, true)) return; @@ -3990,7 +3990,7 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu) st->preempted = vcpu->arch.st.preempted = KVM_VCPU_PREEMPTED; - kvm_unmap_gfn(vcpu, &map, &vcpu->arch.st.cache, true, true); + kvm_unmap_gfn(vcpu->kvm, &map, &vcpu->arch.st.cache, true, true); } void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 7f2e2a09ebbd..8eb5eb1399f5 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -806,11 +806,11 @@ struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn kvm_pfn_t kvm_vcpu_gfn_to_pfn_atomic(struct kvm_vcpu *vcpu, gfn_t gfn); kvm_pfn_t kvm_vcpu_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn); int kvm_vcpu_map(struct kvm_vcpu *vcpu, gpa_t gpa, struct kvm_host_map *map); -int kvm_map_gfn(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map, +int kvm_map_gfn(struct kvm *kvm, gfn_t gfn, struct kvm_host_map *map, struct gfn_to_pfn_cache *cache, bool atomic); struct page *kvm_vcpu_gfn_to_page(struct kvm_vcpu *vcpu, gfn_t gfn); void kvm_vcpu_unmap(struct kvm_vcpu *vcpu, struct kvm_host_map *map, bool dirty); -int kvm_unmap_gfn(struct kvm_vcpu *vcpu, struct kvm_host_map *map, +int kvm_unmap_gfn(struct kvm *kvm, struct kvm_host_map *map, struct gfn_to_pfn_cache *cache, bool dirty, bool atomic); unsigned long kvm_vcpu_gfn_to_hva(struct kvm_vcpu *vcpu, gfn_t gfn); unsigned long kvm_vcpu_gfn_to_hva_prot(struct kvm_vcpu *vcpu, gfn_t gfn, bool *writable); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 2541a17ff1c4..f01a8df7806a 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2181,10 +2181,10 @@ static int __kvm_map_gfn(struct kvm_memslots *slots, gfn_t gfn, return 0; } -int kvm_map_gfn(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map, +int kvm_map_gfn(struct kvm *kvm, gfn_t gfn, struct kvm_host_map *map, struct gfn_to_pfn_cache *cache, bool atomic) { - return __kvm_map_gfn(kvm_memslots(vcpu->kvm), gfn, map, + return __kvm_map_gfn(kvm_memslots(kvm), gfn, map, cache, atomic); } EXPORT_SYMBOL_GPL(kvm_map_gfn); @@ -2232,10 +2232,10 @@ static void __kvm_unmap_gfn(struct kvm_memory_slot *memslot, map->page = NULL; } -int kvm_unmap_gfn(struct kvm_vcpu *vcpu, struct kvm_host_map *map, +int kvm_unmap_gfn(struct kvm *kvm, struct kvm_host_map *map, struct gfn_to_pfn_cache *cache, bool dirty, bool atomic) { - __kvm_unmap_gfn(gfn_to_memslot(vcpu->kvm, map->gfn), map, + __kvm_unmap_gfn(gfn_to_memslot(kvm, map->gfn), map, cache, dirty, atomic); return 0; }