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;
>
> 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 ;)
--
Peter Xu
