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

Sean Christopherson
2020-02-20T19:23:52Z

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
>