RE: [PATCH v3] KVM: x86: enable dirty log gradually in small chunks

["Zhoujian (jay)" <jianjay.zhou@huawei.com>]

[...]

> > >
> > >   KVM_MANUAL_PROTECT_ENABLE
> > >   KVM_MANUAL_PROTECT_INIT_ALL_SET
> >
> > I think this naming emphasizes more about "manual protect", and the
> > original naming emphasizes more about "dirty log". The object of
> > manual protect and initial-all-set is dirty log, so it seem that the
> > original names are a little more close to the thing we do.
> 
> OK.  Then maybe rename bit 0 of KVM_DIRTY_LOG_MANUAL_PROTECT2 to
> KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE?  No strong opinion but it looks
> weird to have the ending "2" in the sub-caps..

Will do.

[...]

> > > > +bool kvm_manual_dirty_log_init_set(struct kvm *kvm) {
> > > > +	return kvm->manual_dirty_log_protect &
> > > > +KVM_DIRTY_LOG_INITIALLY_SET; }
> > >
> > > Nit: this can be put into kvm_host.h as inlined.
> >
> > I'm afraid not. I tried to do it, but it can't be compiled through.
> > Since this function is shared between the kvm and kvm_intel(vmx part)
> > module, it should be exported.
> 
> What's the error?  Did you add it into the right kvm_host.h (which
> is ./include/linux/kvm_host.h, not per-arch one), and was it with "static inline"?

After adding "static inline" it can be compiled through (I mis-understood and added
"static" only last time, sorry).

[...]

> > > > @@ -3320,6 +3326,10 @@ static long
> > > kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
> > > >  	case KVM_CAP_COALESCED_PIO:
> > > >  		return 1;
> > > >  #endif
> > > > +#ifdef CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT
> > > > +	case KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2:
> > > > +		return KVM_DIRTY_LOG_MANUAL_CAPS;
> > >
> > > We probably can only return the new feature bit when with CONFIG_X86?
> >
> > How about to define different values in different architectures(see
> > KVM_USER_MEM_SLOTS as an example), like this:
> >
> > diff --git a/arch/arm/include/asm/kvm_host.h
> > b/arch/arm/include/asm/kvm_host.h index c3314b2..383a8ae 100644
> > --- a/arch/arm/include/asm/kvm_host.h
> > +++ b/arch/arm/include/asm/kvm_host.h
> > @@ -23,6 +23,10 @@
> >  #define KVM_HAVE_ONE_REG
> >  #define KVM_HALT_POLL_NS_DEFAULT 500000
> >
> > +#define KVM_DIRTY_LOG_MANUAL_PROTECT (1 << 0) #define
> > +KVM_DIRTY_LOG_INITIALLY_SET 0 #define KVM_DIRTY_LOG_MANUAL_CAPS
> > +KVM_DIRTY_LOG_MANUAL_PROTECT
> > +
> >  #define KVM_VCPU_MAX_FEATURES 2
> >
> >  #include <kvm/arm_vgic.h>
> > diff --git a/arch/mips/include/asm/kvm_host.h
> > b/arch/mips/include/asm/kvm_host.h
> > index 41204a4..503ee17 100644
> > --- a/arch/mips/include/asm/kvm_host.h
> > +++ b/arch/mips/include/asm/kvm_host.h
> > @@ -85,6 +85,10 @@
> >
> >  #define KVM_HALT_POLL_NS_DEFAULT 500000
> >
> > +#define KVM_DIRTY_LOG_MANUAL_PROTECT (1 << 0) #define
> > +KVM_DIRTY_LOG_INITIALLY_SET 0 #define KVM_DIRTY_LOG_MANUAL_CAPS
> > +KVM_DIRTY_LOG_MANUAL_PROTECT
> > +
> >  #ifdef CONFIG_KVM_MIPS_VZ
> >  extern unsigned long GUESTID_MASK;
> >  extern unsigned long GUESTID_FIRST_VERSION; diff --git
> > a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 40a0c0f..ac05172 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -49,6 +49,11 @@
> >
> >  #define KVM_IRQCHIP_NUM_PINS  KVM_IOAPIC_NUM_PINS
> >
> > +#define KVM_DIRTY_LOG_MANUAL_PROTECT (1 << 0) #define
> > +KVM_DIRTY_LOG_INITIALLY_SET (1 << 1) #define
> > +KVM_DIRTY_LOG_MANUAL_CAPS (KVM_DIRTY_LOG_MANUAL_PROTECT | \
> > +               KVM_DIRTY_LOG_INITIALLY_SET)
> > +
> >  /* x86-specific vcpu->requests bit members */
> >  #define KVM_REQ_MIGRATE_TIMER          KVM_ARCH_REQ(0)
> >  #define KVM_REQ_REPORT_TPR_ACCESS      KVM_ARCH_REQ(1)
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index
> > e89eb67..ebd3e55 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -360,6 +360,18 @@ static inline unsigned long
> *kvm_second_dirty_bitmap(struct kvm_memory_slot *mem
> >         return memslot->dirty_bitmap + len /
> > sizeof(*memslot->dirty_bitmap);  }
> >
> > +#ifndef KVM_DIRTY_LOG_MANUAL_PROTECT
> > +#define KVM_DIRTY_LOG_MANUAL_PROTECT 0 #endif #ifndef
> > +KVM_DIRTY_LOG_INITIALLY_SET #define KVM_DIRTY_LOG_INITIALLY_SET 0
> > +#endif #ifndef KVM_DIRTY_LOG_MANUAL_CAPS #define
> > +KVM_DIRTY_LOG_MANUAL_CAPS 0 #endif
> 
> This seems a bit more awkward to me... You also reminded me that maybe it's
> good we put the sub-cap definition into uapi.  How about:

Sure.

> 
> ==========
> 
> diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h index 40a0c0fd95ca..fcffaf8a6964 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1697,4 +1697,7 @@ static inline int kvm_cpu_get_apicid(int mps_cpu)
>  #define GET_SMSTATE(type, buf, offset)         \
>         (*(type *)((buf) + (offset) - 0x7e00))
> 
> +#define KVM_DIRTY_LOG_MANUAL_CAPS
> (KVM_DIRTY_LOG_MANUAL_PROTECT | \
> +
> KVM_DIRTY_LOG_INITIALLY_SET)
> +
>  #endif /* _ASM_X86_KVM_HOST_H */
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index
> e89eb67356cb..39d49802ee87 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1410,4 +1410,8 @@ int kvm_vm_create_worker_thread(struct kvm *kvm,
> kvm_vm_thread_fn_t thread_fn,
>                                 uintptr_t data, const char *name,
>                                 struct task_struct **thread_ptr);
> 
> +#ifndef KVM_DIRTY_LOG_MANUAL_CAPS
> +#define KVM_DIRTY_LOG_MANUAL_CAPS
> KVM_DIRTY_LOG_MANUAL_PROTECT
> +#endif
> +
>  #endif
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index
> 4b95f9a31a2f..a83f7627c0c1 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1628,4 +1628,7 @@ struct kvm_hyperv_eventfd {
>  #define KVM_HYPERV_CONN_ID_MASK                0x00ffffff
>  #define KVM_HYPERV_EVENTFD_DEASSIGN    (1 << 0)
> 
> +#define KVM_DIRTY_LOG_MANUAL_PROTECT   (1 << 0)
> +#define KVM_DIRTY_LOG_INITIALLY_SET    (1 << 1)
> +
>  #endif /* __LINUX_KVM_H */
> 

I replied about this change in another thread.


Regards,
Jay Zhou