| Message ID | 4369bbef7f0c2b239da419c917f9a9f2ca6a76f1.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: x86: Fix recording of guest steal time / preempted status | expand |
On 02/11/21 17:38, David Woodhouse wrote: > This kind of makes a mockery of this > repeated map/unmap dance which I thought was supposed to avoid pinning > the page The map/unmap dance is supposed to catch the moment where you'd look at a stale cache, by giving the non-atomic code a chance to update the gfn->pfn mapping. The unmap is also the moment where you can mark the page as dirty. Paolo
On Tue, 2021-11-02 at 18:01 +0100, Paolo Bonzini wrote: > On 02/11/21 17:38, David Woodhouse wrote: > > This kind of makes a mockery of this > > repeated map/unmap dance which I thought was supposed to avoid pinning > > the page > > The map/unmap dance is supposed to catch the moment where you'd look at > a stale cache, by giving the non-atomic code a chance to update the > gfn->pfn mapping. > It might have *chance* to do so, but it doesn't actually do it. As noted, a GFN→PFN mapping is really a GFN→HVA→PFN mapping. And the non-atomic code *does* update the GFN→HVA part of that, correctly looking at the memslots generation etc.. But it pays absolutely no attention to the *second* part, and assumes that the HVA→PFN mapping in the userspace page tables will never change. Which isn't necessarily true, even if the underlying physical page *is* pinned to avoid most cases (ksm, swap, etc.) of the *kernel* changing it. Userspace still can. > The unmap is also the moment where you can mark the page as dirty. Sure, but it's the wrong page :) It's not necessarily the page that is at that userspace HVA, and hence in the guest's EPT at that GFN any more. In my Xen event channel series, I added a 'mmap a page from /dev/zero over the shared_info page after it's active' torture test to demonstrate this and check it was fixed. I suppose we could do the same in the steal_time test...?
On 02/11/21 18:11, David Woodhouse wrote: > On Tue, 2021-11-02 at 18:01 +0100, Paolo Bonzini wrote: >> On 02/11/21 17:38, David Woodhouse wrote: >>> This kind of makes a mockery of this >>> repeated map/unmap dance which I thought was supposed to avoid pinning >>> the page >> >> The map/unmap dance is supposed to catch the moment where you'd look at >> a stale cache, by giving the non-atomic code a chance to update the >> gfn->pfn mapping. >> > > It might have *chance* to do so, but it doesn't actually do it. > > As noted, a GFN→PFN mapping is really a GFN→HVA→PFN mapping. And the > non-atomic code *does* update the GFN→HVA part of that, correctly > looking at the memslots generation etc.. > > But it pays absolutely no attention to the *second* part, and assumes > that the HVA→PFN mapping in the userspace page tables will never > change. > > Which isn't necessarily true, even if the underlying physical page *is* > pinned to avoid most cases (ksm, swap, etc.) of the *kernel* changing > it. Userspace still can. Yes, I agree. What I am saying is that: - the map/unmap dance is not (entirely) about whether to pin the page - the map/unmap API is not a bad API, just an incomplete implementation And I think the above comment confuses both points above. >> The unmap is also the moment where you can mark the page as dirty. > > Sure, but it's the wrong page :) The GFN _also_ has to be marked dirty. Paolo
On Tue, 2021-11-02 at 18:19 +0100, Paolo Bonzini wrote: > Yes, I agree. What I am saying is that: > > - the map/unmap dance is not (entirely) about whether to pin the page > - the map/unmap API is not a bad API, just an incomplete implementation > Yep, fair enough. The Xen evtchn series contains what I believe is necessary to make it a complete implementation. But in *this* case it's fairly gratuitous since, as noted, we already *have* a perfectly serviceable mapping. > The GFN _also_ has to be marked dirty. Argh, yes. I forgot to do that. Will fix in v3. Thanks.
On 2 November 2021 17:19:34 GMT, Paolo Bonzini <pbonzini@redhat.com> wrote: >On 02/11/21 18:11, David Woodhouse wrote: >> On Tue, 2021-11-02 at 18:01 +0100, Paolo Bonzini wrote: >>> On 02/11/21 17:38, David Woodhouse wrote: >>>> This kind of makes a mockery of this >>>> repeated map/unmap dance which I thought was supposed to avoid pinning >>>> the page >>> >>> The map/unmap dance is supposed to catch the moment where you'd look at >>> a stale cache, by giving the non-atomic code a chance to update the >>> gfn->pfn mapping. >>> >> >> It might have *chance* to do so, but it doesn't actually do it. >> >> As noted, a GFN→PFN mapping is really a GFN→HVA→PFN mapping. And the >> non-atomic code *does* update the GFN→HVA part of that, correctly >> looking at the memslots generation etc.. >> >> But it pays absolutely no attention to the *second* part, and assumes >> that the HVA→PFN mapping in the userspace page tables will never >> change. >> >> Which isn't necessarily true, even if the underlying physical page *is* >> pinned to avoid most cases (ksm, swap, etc.) of the *kernel* changing >> it. Userspace still can. > >Yes, I agree. What I am saying is that: > >- the map/unmap dance is not (entirely) about whether to pin the page > >- the map/unmap API is not a bad API, just an incomplete implementation > >And I think the above comment confuses both points above. Sorry, it took me a while to realise that by "above comment" you mean the original commit comment (which you want me to reword) instead of just what I'd said in my previous email. How about this version? If it's OK like this then I can resubmit later today when I get back to a proper keyboard. In commit b043138246a4 ("x86/KVM: Make sure KVM_VCPU_FLUSH_TLB flag is not missed") we switched to using a gfn_to_pfn_cache for accessing the guest steal time structure in order to allow for an atomic xchg of the preempted field. This has a couple of problems. Firstly, kvm_map_gfn() doesn't work at all for IOMEM pages when the atomic flag is set, which it is in kvm_steal_time_set_preempted(). So a guest vCPU using an IOMEM page for its steal time would never have its preempted field set. Secondly, the gfn_to_pfn_cache is not invalidated in all cases where it should have been. There are two stages to the GFN → PFN conversion; first the GFN is converted to a userspace HVA, and then that HVA is looked up in the process page tables to find the underlying host PFN. Correct invalidation of the latter would require being hooked up to the MMU notifiers, but that doesn't happen — so it just keeps mapping and unmapping the *wrong* PFN after the userspace page tables change. In the !IOMEM case at least the stale page *is* pinned all the time it's cached, so it won't be freed and reused by anyone else while still receiving the steal time updates. To support Xen event channel delivery I will be fixing this up and using the MMU notifiers to mark the mapping invalid at appropriate times — giving us a way to use kvm_map_gfn() safely with an atomic fast path via the kernel mapping, and a slow fallback path for when the mapping needs to be refreshed. But for steal time reporting there's no point in a kernel mapping of it anyway, when in all cases we care about, we have a perfectly serviceable (and tautologically not stale) userspace HVA for it. We just need to implement the atomic xchg on the userspace address with appropriate exception handling, which is fairly trivial.
On 11/3/21 10:47, David Woodhouse wrote: > Sorry, it took me a while to realise that by "above comment" you mean > the original commit comment (which you want me to reword) instead of > just what I'd said in my previous email. How about this version? If > it's OK like this then I can resubmit later today when I get back to > a proper keyboard. No need to resubmit, thanks! I'll review the code later and decide whether to include this in 5.16 or go for the "good" solution in 5.16 and submit this one for 5.15 only. Paolo
On 3 November 2021 12:35:11 GMT, Paolo Bonzini <pbonzini@redhat.com> wrote: >On 11/3/21 10:47, David Woodhouse wrote: >> Sorry, it took me a while to realise that by "above comment" you mean >> the original commit comment (which you want me to reword) instead of >> just what I'd said in my previous email. How about this version? If >> it's OK like this then I can resubmit later today when I get back to >> a proper keyboard. > >No need to resubmit, thanks! I'll review the code later and decide >whether to include this in 5.16 or go for the "good" solution in 5.16 >and submit this one for 5.15 only. I would call this the good solution for steal time. We really do always have a userspace HVA for that when it matters, and we should use it. For Xen event channel delivery we have to do it from hardware interrupts under arbitrary current->mm and we need a kernel mapping, and we need the MMU notifiers and all that stuff. But for every mapping we do that way, we need extra checks in the MMU notifiers. For steal time there's just no need.
On 11/3/21 13:56, David Woodhouse wrote: >> No need to resubmit, thanks! I'll review the code later and >> decide whether to include this in 5.16 or go for the "good" >> solution in 5.16 and submit this one for 5.15 only. > I would call this the good solution for steal time. We really do > always have a userspace HVA for that when it matters, and we should > use it. > > For Xen event channel delivery we have to do it from hardware > interrupts under arbitrary current->mm and we need a kernel mapping, > and we need the MMU notifiers and all that stuff. But for every > mapping we do that way, we need extra checks in the MMU notifiers. > > For steal time there's just no need. Yes, but doing things by hand that it is slightly harder to get right, between the asm and the manual user_access_{begin,end}. The good solution would be to handle the remapping of _all_ gfn-to-pfn caches from the MMU notifiers, so that you can still do map/unmap, keep the code simple, and get for free the KVM-specific details such as marking the gfn as dirty. When I was working on it before, I got stuck with wanting to do it not just good but perfect, including the eVMCS page in it. But that makes no sense because really all that needs to be fixed is the _current_ users of the gfn-to-pfn cache. Paolo
On 3 November 2021 13:05:11 GMT, Paolo Bonzini <pbonzini@redhat.com> wrote: >On 11/3/21 13:56, David Woodhouse wrote: >>> No need to resubmit, thanks! I'll review the code later and >>> decide whether to include this in 5.16 or go for the "good" >>> solution in 5.16 and submit this one for 5.15 only. >> I would call this the good solution for steal time. We really do >> always have a userspace HVA for that when it matters, and we should >> use it. >> >> For Xen event channel delivery we have to do it from hardware >> interrupts under arbitrary current->mm and we need a kernel mapping, >> and we need the MMU notifiers and all that stuff. But for every >> mapping we do that way, we need extra checks in the MMU notifiers. >> >> For steal time there's just no need. > >Yes, but doing things by hand that it is slightly harder to get right, >between the asm and the manual user_access_{begin,end}. Yes. Before I embarked on this I did have a fantasy that I could just use the futex asm helpers which already do much of that, but it didn't turn out that way. But once that part is done it shouldn't need to be touched again. It's only for the *locked* accesses like bit set and xchg; for anything else the normal user access works. >The good solution would be to handle the remapping of _all_ gfn-to-pfn >caches from the MMU notifiers, so that you can still do map/unmap, keep >the code simple, and get for free the KVM-specific details such as >marking the gfn as dirty. > >When I was working on it before, I got stuck with wanting to do it not >just good but perfect, including the eVMCS page in it. But that makes >no sense because really all that needs to be fixed is the _current_ >users of the gfn-to-pfn cache. Yeah. Well, let's take a look at the Xen evtchn stuff and heckle the new rwlock I used, and the way we have to hold that lock *while* doing the access. Then we can ponder whether we want to offer that as a "generic" thing for providing a kernel mapping, and have the MMU notifiers walk a list of them to check for invalidation. Or whether we can actually use an HVA after all (and in at least some cases we really can).
On 3 November 2021 13:05:11 GMT, Paolo Bonzini <pbonzini@redhat.com> wrote: >On 11/3/21 13:56, David Woodhouse wrote: >>> No need to resubmit, thanks! I'll review the code later and >>> decide whether to include this in 5.16 or go for the "good" >>> solution in 5.16 and submit this one for 5.15 only. >> I would call this the good solution for steal time. We really do >> always have a userspace HVA for that when it matters, and we should >> use it. >> >> For Xen event channel delivery we have to do it from hardware >> interrupts under arbitrary current->mm and we need a kernel mapping, >> and we need the MMU notifiers and all that stuff. But for every >> mapping we do that way, we need extra checks in the MMU notifiers. >> >> For steal time there's just no need. > >Yes, but doing things by hand that it is slightly harder to get right, >between the asm and the manual user_access_{begin,end}. Right. When I embarked on this I had a fantasy that I could just use the futex asm helpers which do most of it for us. But it didn't turn out that way. On the other hand, it's only needed for the *atomic* accesses (xchg, bit set) and most of the time we can just use normal uaccess stuff (and remember to mark the gfn dirty!) >The good solution would be to handle the remapping of _all_ gfn-to-pfn >caches from the MMU notifiers, so that you can still do map/unmap, keep >the code simple, and get for free the KVM-specific details such as >marking the gfn as dirty. > >When I was working on it before, I got stuck with wanting to do it not >just good but perfect, including the eVMCS page in it. But that makes >no sense because really all that needs to be fixed is the _current_ >users of the gfn-to-pfn cache. Yeah. Well, let's take a look at how I've done it for the Xen event channel delivery, in particular the rwlock that has to be held *while* doing the access via the mapped kernel address. Then we can ponder whether we want to offer something along those lines as a generic facility.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 13f64654dfff..750f74da9793 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -751,7 +751,7 @@ struct kvm_vcpu_arch { u8 preempted; u64 msr_val; u64 last_steal; - struct gfn_to_pfn_cache cache; + struct gfn_to_hva_cache cache; } st; u64 l1_tsc_offset; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index bfe0de3008a6..e6905a1068ad 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3195,8 +3195,11 @@ static void kvm_vcpu_flush_tlb_guest(struct kvm_vcpu *vcpu) static void record_steal_time(struct kvm_vcpu *vcpu) { - struct kvm_host_map map; - struct kvm_steal_time *st; + struct gfn_to_hva_cache *ghc = &vcpu->arch.st.cache; + struct kvm_steal_time __user *st; + struct kvm_memslots *slots; + u64 steal; + u32 version; if (kvm_xen_msr_enabled(vcpu->kvm)) { kvm_xen_runstate_set_running(vcpu); @@ -3206,47 +3209,87 @@ static void record_steal_time(struct kvm_vcpu *vcpu) if (!(vcpu->arch.st.msr_val & KVM_MSR_ENABLED)) return; - /* -EAGAIN is returned in atomic context so we can just return. */ - if (kvm_map_gfn(vcpu, vcpu->arch.st.msr_val >> PAGE_SHIFT, - &map, &vcpu->arch.st.cache, false)) + if (WARN_ON_ONCE(current->mm != vcpu->kvm->mm)) return; - st = map.hva + - offset_in_page(vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS); + slots = kvm_memslots(vcpu->kvm); + + if (unlikely(slots->generation != ghc->generation || + kvm_is_error_hva(ghc->hva) || !ghc->memslot)) { + gfn_t gfn = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS; + + /* We rely on the fact that it fits in a single page. */ + BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS); + + if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gfn, sizeof(*st)) || + kvm_is_error_hva(ghc->hva) || !ghc->memslot) + return; + } + + st = (struct kvm_steal_time __user *)ghc->hva; + if (!user_access_begin(st, sizeof(*st))) + return; /* * Doing a TLB flush here, on the guest's behalf, can avoid * expensive IPIs. */ if (guest_pv_has(vcpu, KVM_FEATURE_PV_TLB_FLUSH)) { - u8 st_preempted = xchg(&st->preempted, 0); + u8 st_preempted = 0; + int err; + + asm volatile("1:\t" LOCK_PREFIX "xchgb %0, %2\n" + "\txor %1, %1\n" + "2:\n" + "\t.section .fixup,\"ax\"\n" + "3:\tmovl %3, %1\n" + "\tjmp\t2b\n" + "\t.previous\n" + _ASM_EXTABLE_UA(1b, 3b) + : "=r" (st_preempted), + "=r" (err) + : "m" (st->preempted), + "i" (-EFAULT), + "0" (st_preempted)); + if (err) + goto out; + + user_access_end(); + + vcpu->arch.st.preempted = 0; trace_kvm_pv_tlb_flush(vcpu->vcpu_id, st_preempted & KVM_VCPU_FLUSH_TLB); if (st_preempted & KVM_VCPU_FLUSH_TLB) kvm_vcpu_flush_tlb_guest(vcpu); + + if (!user_access_begin(st, sizeof(*st))) + return; } else { - st->preempted = 0; + unsafe_put_user(0, &st->preempted, out); + vcpu->arch.st.preempted = 0; } - vcpu->arch.st.preempted = 0; - - if (st->version & 1) - st->version += 1; /* first time write, random junk */ + unsafe_get_user(version, &st->version, out); + if (version & 1) + version += 1; /* first time write, random junk */ - st->version += 1; + version += 1; + unsafe_put_user(version, &st->version, out); smp_wmb(); - st->steal += current->sched_info.run_delay - + unsafe_get_user(steal, &st->steal, out); + steal += current->sched_info.run_delay - vcpu->arch.st.last_steal; vcpu->arch.st.last_steal = current->sched_info.run_delay; + unsafe_put_user(steal, &st->steal, out); - smp_wmb(); - - st->version += 1; + version += 1; + unsafe_put_user(version, &st->version, out); - kvm_unmap_gfn(vcpu, &map, &vcpu->arch.st.cache, true, false); + out: + user_access_end(); } int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) @@ -4285,8 +4328,10 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu) { - struct kvm_host_map map; - struct kvm_steal_time *st; + struct gfn_to_hva_cache *ghc = &vcpu->arch.st.cache; + struct kvm_steal_time __user *st; + struct kvm_memslots *slots; + static const u8 preempted = KVM_VCPU_PREEMPTED; if (!(vcpu->arch.st.msr_val & KVM_MSR_ENABLED)) return; @@ -4294,16 +4339,21 @@ 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, - &vcpu->arch.st.cache, true)) + /* This happens on process exit */ + if (unlikely(current->mm != vcpu->kvm->mm)) return; - st = map.hva + - offset_in_page(vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS); + slots = kvm_memslots(vcpu->kvm); - st->preempted = vcpu->arch.st.preempted = KVM_VCPU_PREEMPTED; + if (unlikely(slots->generation != ghc->generation || + kvm_is_error_hva(ghc->hva) || !ghc->memslot)) + return; - kvm_unmap_gfn(vcpu, &map, &vcpu->arch.st.cache, true, true); + st = (struct kvm_steal_time __user *)ghc->hva; + BUILD_BUG_ON(sizeof(st->preempted) != sizeof(preempted)); + + if (!copy_to_user_nofault(&st->preempted, &preempted, sizeof(preempted))) + vcpu->arch.st.preempted = KVM_VCPU_PREEMPTED; } void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) @@ -10817,11 +10867,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu) void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu) { - struct gfn_to_pfn_cache *cache = &vcpu->arch.st.cache; int idx; - kvm_release_pfn(cache->pfn, cache->dirty, cache); - kvmclock_reset(vcpu); static_call(kvm_x86_vcpu_free)(vcpu);