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

[Kai Huang <kai.huang@linux.intel.com>]

On 02/03/2015 11:53 PM, Radim Krčmář wrote:
> 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?
No PML doesn't depend on it to work. It's suggested by Paolo.

Thanks,
-Kai
>
>> @@ -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);