Re: [PATCH v2] KVM: x86: enable dirty log gradually in small chunks

[Sean Christopherson <sean.j.christopherson@intel.com>]

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;
	}

>                 kvm->manual_dirty_log_protect = cap->args[0];
>                 return 0;
> 
> Otherwise it looks good to me!
> 
> Thanks,
> 
> -- 
> Peter Xu
>