[PATCH v13 2/7] KVM: Add generic support for dirty page logging

[PATCH v13 2/7] KVM: Add generic support for dirty page logging

[Mario Smarduch]

kvm_get_dirty_log() provides generic handling of dirty bitmap, currently reused
by several architectures. Building on that we intrdoduce 
kvm_get_dirty_log_protect() adding write protection to mark these pages dirty
for future write access, before next KVM_GET_DIRTY_LOG ioctl call from user
space.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 include/linux/kvm_host.h |    9 +++++
 virt/kvm/kvm_main.c      |   95 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index a4c33b3..c55dd75 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -590,6 +590,15 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext);
 
 int kvm_get_dirty_log(struct kvm *kvm,
 			struct kvm_dirty_log *log, int *is_dirty);
+
+int kvm_get_dirty_log_protect(struct kvm *kvm,
+			struct kvm_dirty_log *log, bool *is_dirty);
+
+void kvm_arch_mmu_write_protect_pt_masked(struct kvm *kvm,
+					struct kvm_memory_slot *slot,
+					gfn_t gfn_offset,
+					unsigned long mask);
+
 int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 				struct kvm_dirty_log *log);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 887df87..f017760 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -981,6 +981,101 @@ out:
 }
 EXPORT_SYMBOL_GPL(kvm_get_dirty_log);
 
+#if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS) || \
+    defined(CONFIG_IA64) || defined(CONFIG_X86) || defined(CONFIG_ARM) || \
+    defined(CONFIG_ARM64)
+/*
+ * For architectures that don't use kvm_get_dirty_log_protect() for dirty page
+ * logging, calling this function is illegal. Otherwise the function is defined
+ * in arch subtree and this restriction is removed.
+ */
+void kvm_arch_mmu_write_protect_pt_masked(struct kvm *kvm,
+					struct kvm_memory_slot *slot,
+					gfn_t gfn_offset,
+					unsigned long mask)
+{
+	BUG();
+}
+#endif
+
+/**
+ * kvm_get_dirty_log_protect - get a snapshot of dirty pages, and if any pages
+ *	are dirty write protect them for next write.
+ * @kvm:	pointer to kvm instance
+ * @log:	slot id and address to which we copy the log
+ * @is_dirty:	flag set if any page is dirty
+ *
+ * We need to keep it in mind that VCPU threads can write to the bitmap
+ * concurrently. So, to avoid losing track of dirty pages we keep the
+ * following order:
+ *
+ *    1. Take a snapshot of the bit and clear it if needed.
+ *    2. Write protect the corresponding page.
+ *    3. Copy the snapshot to the userspace.
+ *    4. Upon return caller flushes TLB's if needed.
+ *
+ * Between 2 and 4, the guest may write to the page using the remaining TLB
+ * entry.  This is not a problem because the page is reported dirty using
+ * the snapshot taken before and step 4 ensures that writes done after
+ * exiting to userspace will be logged for the next call.
+ *
+ */
+int kvm_get_dirty_log_protect(struct kvm *kvm,
+			struct kvm_dirty_log *log, bool *is_dirty)
+{
+	struct kvm_memory_slot *memslot;
+	int r, i;
+	unsigned long n;
+	unsigned long *dirty_bitmap;
+	unsigned long *dirty_bitmap_buffer;
+
+	r = -EINVAL;
+	if (log->slot >= KVM_USER_MEM_SLOTS)
+		goto out;
+
+	memslot = id_to_memslot(kvm->memslots, log->slot);
+
+	dirty_bitmap = memslot->dirty_bitmap;
+	r = -ENOENT;
+	if (!dirty_bitmap)
+		goto out;
+
+	n = kvm_dirty_bitmap_bytes(memslot);
+
+	dirty_bitmap_buffer = dirty_bitmap + n / sizeof(long);
+	memset(dirty_bitmap_buffer, 0, n);
+
+	spin_lock(&kvm->mmu_lock);
+	*is_dirty = false;
+	for (i = 0; i < n / sizeof(long); i++) {
+		unsigned long mask;
+		gfn_t offset;
+
+		if (!dirty_bitmap[i])
+			continue;
+
+		*is_dirty = true;
+
+		mask = xchg(&dirty_bitmap[i], 0);
+		dirty_bitmap_buffer[i] = mask;
+
+		offset = i * BITS_PER_LONG;
+		kvm_arch_mmu_write_protect_pt_masked(kvm, memslot, offset,
+								mask);
+	}
+
+	spin_unlock(&kvm->mmu_lock);
+
+	r = -EFAULT;
+	if (copy_to_user(log->dirty_bitmap, dirty_bitmap_buffer, n))
+		goto out;
+
+	r = 0;
+out:
+	return r;
+}
+EXPORT_SYMBOL_GPL(kvm_get_dirty_log_protect);
+
 bool kvm_largepages_enabled(void)
 {
 	return largepages_enabled;
-- 
1.7.9.5

Re: [PATCH v13 2/7] KVM: Add generic support for dirty page logging

[Cornelia Huck]

On Thu, 06 Nov 2014 16:40:43 -0800
Mario Smarduch <m.smarduch@samsung.com> wrote:

> kvm_get_dirty_log() provides generic handling of dirty bitmap, currently reused
> by several architectures. Building on that we intrdoduce 
> kvm_get_dirty_log_protect() adding write protection to mark these pages dirty
> for future write access, before next KVM_GET_DIRTY_LOG ioctl call from user
> space.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> ---
>  include/linux/kvm_host.h |    9 +++++
>  virt/kvm/kvm_main.c      |   95 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
> 

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 887df87..f017760 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -981,6 +981,101 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(kvm_get_dirty_log);
> 
> +#if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS) || \
> +    defined(CONFIG_IA64) || defined(CONFIG_X86) || defined(CONFIG_ARM) || \
> +    defined(CONFIG_ARM64)

Does this deserve a config symbol that can be selected by architectures
actually using kvm_get_dirty_log_protect()? I.e.,

#ifndef CONFIG_KVM_ARCH_DIRTY_LOG_PROTECT

or so?

> +/*
> + * For architectures that don't use kvm_get_dirty_log_protect() for dirty page
> + * logging, calling this function is illegal. Otherwise the function is defined
> + * in arch subtree and this restriction is removed.
> + */

What about

/*
 * For architectures that don't use kvm_get_dirty_log_protect() for dirty page
 * logging, we must never end up calling this function. Architectures that do
 * use it define their own version of this function.
 */

instead?

> +void kvm_arch_mmu_write_protect_pt_masked(struct kvm *kvm,
> +					struct kvm_memory_slot *slot,
> +					gfn_t gfn_offset,
> +					unsigned long mask)
> +{
> +	BUG();
> +}
> +#endif
> +
(...)

Re: [PATCH v13 2/7] KVM: Add generic support for dirty page logging

[Paolo Bonzini]

On 07/11/2014 10:07, Cornelia Huck wrote:
>> > +#if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS) || \
>> > +    defined(CONFIG_IA64) || defined(CONFIG_X86) || defined(CONFIG_ARM) || \
>> > +    defined(CONFIG_ARM64)
> Does this deserve a config symbol that can be selected by architectures
> actually using kvm_get_dirty_log_protect()? I.e.,
> 
> #ifndef CONFIG_KVM_ARCH_DIRTY_LOG_PROTECT
> 
> or so?

Yes, either that or invert the #if to negative logic.

Paolo

Re: [PATCH v13 2/7] KVM: Add generic support for dirty page logging

[Mario Smarduch]

On 11/07/2014 01:07 AM, Cornelia Huck wrote:
> On Thu, 06 Nov 2014 16:40:43 -0800
> Mario Smarduch <m.smarduch@samsung.com> wrote:
> 
>> kvm_get_dirty_log() provides generic handling of dirty bitmap, currently reused
>> by several architectures. Building on that we intrdoduce 
>> kvm_get_dirty_log_protect() adding write protection to mark these pages dirty
>> for future write access, before next KVM_GET_DIRTY_LOG ioctl call from user
>> space.
>>
>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>> ---
>>  include/linux/kvm_host.h |    9 +++++
>>  virt/kvm/kvm_main.c      |   95 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 104 insertions(+)
>>
> 
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 887df87..f017760 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -981,6 +981,101 @@ out:
>>  }
>>  EXPORT_SYMBOL_GPL(kvm_get_dirty_log);
>>
>> +#if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS) || \
>> +    defined(CONFIG_IA64) || defined(CONFIG_X86) || defined(CONFIG_ARM) || \
>> +    defined(CONFIG_ARM64)
> 
> Does this deserve a config symbol that can be selected by architectures
> actually using kvm_get_dirty_log_protect()? I.e.,
> 
> #ifndef CONFIG_KVM_ARCH_DIRTY_LOG_PROTECT
> 
> or so?

Yes definitely, not sure why I went this way after using Kconfig
defines before.

> 
>> +/*
>> + * For architectures that don't use kvm_get_dirty_log_protect() for dirty page
>> + * logging, calling this function is illegal. Otherwise the function is defined
>> + * in arch subtree and this restriction is removed.
>> + */
> 
> What about
> 
> /*
>  * For architectures that don't use kvm_get_dirty_log_protect() for dirty page
>  * logging, we must never end up calling this function. Architectures that do
>  * use it define their own version of this function.
>  */
> 
> instead

Yeah that reads better, getting the illegal out of there.
> 
>> +void kvm_arch_mmu_write_protect_pt_masked(struct kvm *kvm,
>> +					struct kvm_memory_slot *slot,
>> +					gfn_t gfn_offset,
>> +					unsigned long mask)
>> +{
>> +	BUG();
>> +}
>> +#endif
>> +
> (...)
>