[...]
> > >
> > > 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
