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

[Mario Smarduch <m.smarduch@samsung.com>]

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
>> +
> (...)
>