> -----Original Message----- > From: Peter Xu [mailto:peterx@xxxxxxxxxx] > Sent: Friday, February 21, 2020 3:42 AM > To: Sean Christopherson <sean.j.christopherson@xxxxxxxxx> > Cc: Zhoujian (jay) <jianjay.zhou@xxxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; > pbonzini@xxxxxxxxxx; wangxin (U) <wangxinxin.wang@xxxxxxxxxx>; > Huangweidong (C) <weidong.huang@xxxxxxxxxx>; Liujinsong (Paul) > <liu.jinsong@xxxxxxxxxx> > Subject: Re: [PATCH v2] KVM: x86: enable dirty log gradually in small chunks > > On Thu, Feb 20, 2020 at 11:23:50AM -0800, Sean Christopherson wrote: > > On Thu, Feb 20, 2020 at 02:17:06PM -0500, Peter Xu wrote: > > > On Thu, Feb 20, 2020 at 12:28:28PM +0800, Jay Zhou wrote: > > > > @@ -3348,7 +3352,14 @@ static int > kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm, > > > > switch (cap->cap) { > > > > #ifdef CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT > > > > case KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2: > > > > - if (cap->flags || (cap->args[0] & ~1)) > > > > + if (cap->flags || > > > > + (cap->args[0] & ~KVM_DIRTY_LOG_MANUAL_CAPS) || > > > > + /* The capability of KVM_DIRTY_LOG_INITIALLY_SET > depends > > > > + * on KVM_DIRTY_LOG_MANUAL_PROTECT, it should not > be > > > > + * set individually > > > > + */ > > > > + ((cap->args[0] & KVM_DIRTY_LOG_MANUAL_CAPS) == > > > > + KVM_DIRTY_LOG_INITIALLY_SET)) > > > > > > How about something easier to read? :) > > > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index > > > 70f03ce0e5c1..9dfbab2a9929 100644 > > > --- a/virt/kvm/kvm_main.c > > > +++ b/virt/kvm/kvm_main.c > > > @@ -3348,7 +3348,10 @@ static int > kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm, > > > switch (cap->cap) { > > > #ifdef CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT > > > case KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2: > > > - if (cap->flags || (cap->args[0] & ~1)) > > > + if (cap->flags || (cap->args[0] & ~3)) > > > + return -EINVAL; > > > + /* Allow 00, 01, and 11. */ > > > + if (cap->args[0] == KVM_DIRTY_LOG_INITIALLY_SET) > > > return -EINVAL; > > > > Oof, "easier" is subjective :-) How about this? > > > > case KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2: { > > u64 allowed_options = KVM_DIRTY_LOG_MANUAL_PROTECT; > > > > if (cap->args[0] & KVM_DIRTY_LOG_MANUAL_PROTECT) > > allowed_options = KVM_DIRTY_LOG_INITIALLY_SET; I believe you mean if (cap->args[0] & KVM_DIRTY_LOG_MANUAL_PROTECT) allowed_options |= KVM_DIRTY_LOG_INITIALLY_SET; ? > > > > if (cap->flags || (cap->args[0] & ~allowed_options)) > > return -EINVAL; > > kvm->manual_dirty_log_protect = cap->args[0]; > > return 0; > > } > > Fine by me! (But I won't tell I still prefer mine ;) :-) Regards, Jay Zhou > > -- > Peter Xu
