| Message ID | 20190104085405.40356-8-Tianyu.Lan@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | X86/KVM/Hyper-V: Add HV ept tlb range list flush support in KVM | expand |
On Fri, Jan 04, 2019 at 04:54:01PM +0800, lantianyu1986@gmail.com wrote: > From: Lan Tianyu <Tianyu.Lan@microsoft.com> > > The dirty bits have already been checked in the previous check of > "dirty_bitmap" and mask must be non-zero value at this point. > > Signed-off-by: Lan Tianyu <Tianyu.Lan@microsoft.com> > --- > virt/kvm/kvm_main.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index cf7cc0554094..e75dbb15fd09 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1206,11 +1206,9 @@ int kvm_get_dirty_log_protect(struct kvm *kvm, > mask = xchg(&dirty_bitmap[i], 0); > dirty_bitmap_buffer[i] = mask; > > - if (mask) { > - offset = i * BITS_PER_LONG; > - kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot, > - offset, mask); > - } > + offset = i * BITS_PER_LONG; > + kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot, > + offset, mask); Hmm, the check against mask was explicitly added by commit 58d2930f4ee3 ("KVM: Eliminate extra function calls in kvm_get_dirty_log_protect()"). AFAIK KVM only *sets* bits in dirty_bitmap without holding slots_lock and/or mmu_lock, so I agree that checking mask is redundant, but it'd be nice to elaborate a bit more in the changelog. At the very least this needs a Fixes tag for the aforementioned commit. Tangentially related, does mmu_lock actually need to be held while we walk dirty_bitmap in kvm_{clear,get}_dirty_log_protect()? The bitmap itself is protected by slots_lock (a lockdep assertion would be nice too), e.g. can we grab the lock iff dirty_bitmap[i] != 0? > } > spin_unlock(&kvm->mmu_lock); > } > -- > 2.14.4 >
On Fri, Jan 04, 2019 at 07:50:36AM -0800, Sean Christopherson wrote: > On Fri, Jan 04, 2019 at 04:54:01PM +0800, lantianyu1986@gmail.com wrote: > > From: Lan Tianyu <Tianyu.Lan@microsoft.com> > > > > The dirty bits have already been checked in the previous check of > > "dirty_bitmap" and mask must be non-zero value at this point. > > > > Signed-off-by: Lan Tianyu <Tianyu.Lan@microsoft.com> > > --- > > virt/kvm/kvm_main.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index cf7cc0554094..e75dbb15fd09 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -1206,11 +1206,9 @@ int kvm_get_dirty_log_protect(struct kvm *kvm, > > mask = xchg(&dirty_bitmap[i], 0); > > dirty_bitmap_buffer[i] = mask; > > > > - if (mask) { > > - offset = i * BITS_PER_LONG; > > - kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot, > > - offset, mask); > > - } > > + offset = i * BITS_PER_LONG; > > + kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot, > > + offset, mask); > > Hmm, the check against mask was explicitly added by commit 58d2930f4ee3 > ("KVM: Eliminate extra function calls in kvm_get_dirty_log_protect()"). > AFAIK KVM only *sets* bits in dirty_bitmap without holding slots_lock > and/or mmu_lock, so I agree that checking mask is redundant, but it'd be > nice to elaborate a bit more in the changelog. > > At the very least this needs a Fixes tag for the aforementioned commit. Actually, this can be a straight revert of 58d2930f4ee3.
On 04/01/19 16:50, Sean Christopherson wrote: > Tangentially related, does mmu_lock actually need to be held while we > walk dirty_bitmap in kvm_{clear,get}_dirty_log_protect()? The bitmap > itself is protected by slots_lock (a lockdep assertion would be nice > too), e.g. can we grab the lock iff dirty_bitmap[i] != 0? Yes, we could avoid grabbing it as long as the bitmap is zero. However, without kvm->manual_dirty_log_protect, the granularity of kvm_get_dirty_log_protect() is too coarse so it won't happen in practice. Instead, with the new manual clear, kvm_get_dirty_log_protect() does not take the lock and a well-written userspace is not going to call the clear ioctl unless some bits are set. Paolo
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index cf7cc0554094..e75dbb15fd09 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1206,11 +1206,9 @@ int kvm_get_dirty_log_protect(struct kvm *kvm, mask = xchg(&dirty_bitmap[i], 0); dirty_bitmap_buffer[i] = mask; - if (mask) { - offset = i * BITS_PER_LONG; - kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot, - offset, mask); - } + offset = i * BITS_PER_LONG; + kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot, + offset, mask); } spin_unlock(&kvm->mmu_lock); }