Re: [PATCH 5/6] KVM: x86: Add new dirty logging kvm_x86_ops for PML

["Radim Krčmář" <rkrcmar@redhat.com>]

2015-01-28 10:54+0800, Kai Huang:
> This patch adds new kvm_x86_ops dirty logging hooks to enable/disable dirty
> logging for particular memory slot, and to flush potentially logged dirty GPAs
> before reporting slot->dirty_bitmap to userspace.
> 
> kvm x86 common code calls these hooks when they are available so PML logic can
> be hidden to VMX specific. Other ARCHs won't be impacted as these hooks are NULL
> for them.
> 
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> ---
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -802,6 +802,31 @@ struct kvm_x86_ops {
> +
> +	/*
> +	 * Arch-specific dirty logging hooks. These hooks are only supposed to
> +	 * be valid if the specific arch has hardware-accelerated dirty logging
> +	 * mechanism. Currently only for PML on VMX.
> +	 *
> +	 *  - slot_enable_log_dirty:
> +	 *	called when enabling log dirty mode for the slot.

(I guess that "log dirty mode" isn't the meaning that people will think
 after seeing 'log_dirty' ...
 I'd at least change 'log_dirty' to 'dirty_log' in these names.)

> +	 *  - slot_disable_log_dirty:
> +	 *	called when disabling log dirty mode for the slot.
> +	 *	also called when slot is created with log dirty disabled.
> +	 *  - flush_log_dirty:
> +	 *	called before reporting dirty_bitmap to userspace.
> +	 *  - enable_log_dirty_pt_masked:
> +	 *	called when reenabling log dirty for the GFNs in the mask after
> +	 *	corresponding bits are cleared in slot->dirty_bitmap.

This name is very confusing ... I think we should hint that this is
called after we learn that the page has been written to and would like
to monitor it again.

Using something like collected/refresh?  (I'd have to do horrible things
to come up with a good name, sorry.)

> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3780,6 +3780,12 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
>  
>  	mutex_lock(&kvm->slots_lock);
>  
> +	/*
> +	 * Flush potentially hardware-cached dirty pages to dirty_bitmap.
> +	 */
> +	if (kvm_x86_ops->flush_log_dirty)
> +		kvm_x86_ops->flush_log_dirty(kvm);

(Flushing would make more sense in kvm_get_dirty_log_protect().)

> +
>  	r = kvm_get_dirty_log_protect(kvm, log, &is_dirty);
>  
>  	/*
> @@ -7533,6 +7539,56 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  	return 0;
>  }
>  
> +static void kvm_mmu_slot_apply_flags(struct kvm *kvm,
> +				     struct kvm_memory_slot *new)
> +{
> +	/* Still write protect RO slot */
> +	if (new->flags & KVM_MEM_READONLY) {
> +		kvm_mmu_slot_remove_write_access(kvm, new);

We didn't write protect RO slots before, does this patch depend on it?

> @@ -7562,16 +7618,15 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
> -	if ((change != KVM_MR_DELETE) && (new->flags & KVM_MEM_LOG_DIRTY_PAGES))
> -		kvm_mmu_slot_remove_write_access(kvm, new);
> +	if (change != KVM_MR_DELETE)
> +		kvm_mmu_slot_apply_flags(kvm, new);