> -----Original Message----- > From: Peter Xu [mailto:peterx@xxxxxxxxxx] > Sent: Friday, February 21, 2020 11:19 PM > To: Zhoujian (jay) <jianjay.zhou@xxxxxxxxxx> > Cc: kvm@xxxxxxxxxxxxxxx; pbonzini@xxxxxxxxxx; wangxin (U) > <wangxinxin.wang@xxxxxxxxxx>; Huangweidong (C) > <weidong.huang@xxxxxxxxxx>; sean.j.christopherson@xxxxxxxxx; Liujinsong > (Paul) <liu.jinsong@xxxxxxxxxx> > Subject: Re: [PATCH v2] KVM: x86: enable dirty log gradually in small chunks > > On Fri, Feb 21, 2020 at 09:31:52AM +0000, Zhoujian (jay) wrote: > > [...] > > > > > diff --git a/tools/testing/selftests/kvm/dirty_log_test.c > > > > b/tools/testing/selftests/kvm/dirty_log_test.c > > > > index 5614222..2a493c1 100644 > > > > --- a/tools/testing/selftests/kvm/dirty_log_test.c > > > > +++ b/tools/testing/selftests/kvm/dirty_log_test.c > > > > @@ -317,10 +317,11 @@ static void run_test(enum vm_guest_mode > > > > mode, > > > unsigned long iterations, > > > > host_bmap_track = bitmap_alloc(host_num_pages); > > > > > > > > #ifdef USE_CLEAR_DIRTY_LOG > > > > + int ret = > kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2); > > > > struct kvm_enable_cap cap = {}; > > > > > > > > cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2; > > > > - cap.args[0] = 1; > > > > + cap.args[0] = ret; > > > > > > You enabled the initial-all-set but didn't really check it, so it > > > didn't help much from the testcase pov... > > > > vm_enable_cap is called afterwards, the return value is checked inside > > it, may I ask this check is enough, or it is needed to get the value > > through something like vm_get_cap ? > > I meant to check what has offered by the initial-all-set feature bit, which is, we > should get the bitmap before dirtying and verify that it's all ones. OK, I got your meaning now. > > > > > > I'd suggest you drop this change, and you can work on top after this > > > patch can be accepted. > > > > OK, some input parameters for cap.args[0] should be tested I think: 0, > > 1, 3 should be accepted, the other numbers will not. > > Yes. I think it'll be fine too if you want to put the test changes into this patch. > It's just not required so this patch could potentially get merged easier, since > the test may not be an oneliner change. Yeah, will drop it in the next version. > > > > > > > > > (Not to mention the original test actually verified that we don't > > > break, which seems good..) > > > > > > > vm_enable_cap(vm, &cap); > > > > #endif > > > > > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index > > > > 70f03ce..f2631d0 100644 > > > > --- a/virt/kvm/kvm_main.c > > > > +++ b/virt/kvm/kvm_main.c > > > > @@ -862,7 +862,7 @@ static int kvm_vm_release(struct inode *inode, > > > struct file *filp) > > > > * Allocation size is twice as large as the actual dirty bitmap size. > > > > * See x86's kvm_vm_ioctl_get_dirty_log() why this is needed. > > > > */ > > > > -static int kvm_create_dirty_bitmap(struct kvm_memory_slot > > > > *memslot) > > > > +static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot > > > > +*memslot) > > > > > > This change seems irrelevant.. > > > > > > > { > > > > unsigned long dirty_bytes = 2 * kvm_dirty_bitmap_bytes(memslot); > > > > > > > > @@ -1094,8 +1094,11 @@ int __kvm_set_memory_region(struct kvm > > > > *kvm, > > > > > > > > /* Allocate page dirty bitmap if needed */ > > > > if ((new.flags & KVM_MEM_LOG_DIRTY_PAGES) > && !new.dirty_bitmap) { > > > > - if (kvm_create_dirty_bitmap(&new) < 0) > > > > + if (kvm_alloc_dirty_bitmap(&new)) > > > > > > Same here. > > > > s/kvm_create_dirty_bitmap/kvm_alloc_dirty_bitmap is Sean's suggestion > > to make it clear that the helper is only responsible for allocation, > > then set all the bitmap bits to 1 using bitmap_set afterwards, which > > seems reasonable. Do you still think it's better to keep this name untouched? > > No strong opinion, feel free to take your preference (because we've got three > here and if you like it too then it's already 2:1 :). > > > > > > > > > > goto out_free; > > > > + > > > > + if (kvm->manual_dirty_log_protect & > > > KVM_DIRTY_LOG_INITIALLY_SET) > > > > > > (Maybe time to introduce a helper to shorten this check. :) > > > > Yeah, but could this be done on top of this patch? > > You're going to repost after all, right? If so, IMO it's easier to just add the > helper in the same patch. OK, will do in v3. Regards, Jay Zhou > > Thanks, > > -- > Peter Xu
