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
>
