Re: [PATCH 4/7] KVM: PPC: Book3S HV: Fix dirty map for hugepages

[Alexander Graf <agraf@suse.de>]

On 24.05.14 10:21, Paul Mackerras wrote:
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> The dirty map that we construct for the KVM_GET_DIRTY_LOG ioctl has
> one bit per system page (4K/64K).  Currently, we only set one bit in
> the map for each HPT entry with the Change bit set, even if the HPT is
> for a large page (e.g., 16MB).  Userspace then considers only the
> first system page dirty, though in fact the guest may have modified
> anywhere in the large page.
>
> To fix this, we make kvm_test_clear_dirty() return the actual number
> of pages that are dirty.  In kvmppc_hv_get_dirty_log() we then set
> that many bits in the dirty map.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> ---
>   arch/powerpc/kvm/book3s_64_mmu_hv.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index f32896f..d605dc24 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -1053,6 +1053,7 @@ static int kvm_test_clear_dirty(struct kvm *kvm, unsigned long *rmapp)
>   {
>   	struct revmap_entry *rev = kvm->arch.revmap;
>   	unsigned long head, i, j;
> +	unsigned long n;
>   	unsigned long *hptep;
>   	int ret = 0;
>   
> @@ -1095,7 +1096,10 @@ static int kvm_test_clear_dirty(struct kvm *kvm, unsigned long *rmapp)
>   				rev[i].guest_rpte |= HPTE_R_C;
>   				note_hpte_modification(kvm, &rev[i]);
>   			}
> -			ret = 1;
> +			n = hpte_page_size(hptep[0], hptep[1]);
> +			n = (n + PAGE_SIZE - 1) >> PAGE_SHIFT;
> +			if (n > ret)
> +				ret = n;

Please document in the function header that the return value is the 
number of pages that are dirty. Alternatively rename the function.

>   		}
>   		hptep[0] &= ~HPTE_V_HVLOCK;
>   	} while ((i = j) != head);
> @@ -1125,15 +1129,17 @@ static void harvest_vpa_dirty(struct kvmppc_vpa *vpa,
>   long kvmppc_hv_get_dirty_log(struct kvm *kvm, struct kvm_memory_slot *memslot,
>   			     unsigned long *map)
>   {
> -	unsigned long i;
> +	unsigned long i, j;
>   	unsigned long *rmapp;
>   	struct kvm_vcpu *vcpu;
>   
>   	preempt_disable();
>   	rmapp = memslot->arch.rmap;
>   	for (i = 0; i < memslot->npages; ++i) {
> -		if (kvm_test_clear_dirty(kvm, rmapp) && map)
> -			__set_bit_le(i, map);
> +		int ret = kvm_test_clear_dirty(kvm, rmapp);

Please give this one a better name. npages maybe?

> +		if (ret && map)
> +			for (j = i - (i % ret); ret; ++j, --ret)

Why do we go down from the index?


Alex

> +				__set_bit_le(j, map);
>   		++rmapp;
>   	}
>