I don't have a significant objection to freeing the memory in
kvm_arch_free_memslot, although I think it's a little harder to
understand. I like the idea of being symmetric (memory is allocated
by calling kvm_set_memory_region and freed using the same technique).
That way if someone changes from vm_mmap to something else it will be
obvious that they need to change both.
Also, it looks like your patch is based on something several commits
behind HEAD on virt/kvm/kvm.git, which significantly affect your
patch. In the HEAD version it assumes that user_alloc is always set
unless it's a private memslot. This appears to already have been the
case and allows a bunch of simplifications, some of which would apply
to your patch.
>
> What about something like this (uncompiled/untested)
>
>
> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> index 8b3a9c0..6706134 100644
> --- a/arch/ia64/kvm/kvm-ia64.c
> +++ b/arch/ia64/kvm/kvm-ia64.c
> @@ -1563,7 +1563,8 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> return VM_FAULT_SIGBUS;
> }
>
> -void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> +void kvm_arch_free_memslot(struct kvm *kvm,
> + struct kvm_memory_slot *free,
> struct kvm_memory_slot *dont)
> {
> }
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 4d213b8..a654580 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -299,7 +299,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
> return -EINVAL;
> }
>
> -void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> +void kvm_arch_free_memslot(struct kvm *kvm,
> + struct kvm_memory_slot *free,
> struct kvm_memory_slot *dont)
> {
> if (!dont || free->arch.rmap != dont->arch.rmap) {
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index ecced9d..e2159c1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -912,7 +912,8 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
> return VM_FAULT_SIGBUS;
> }
>
> -void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> +void kvm_arch_free_memslot(struct kvm *kvm,
> + struct kvm_memory_slot *free,
> struct kvm_memory_slot *dont)
> {
> }
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 224a7e7..f9fa0d1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6357,11 +6367,26 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> kfree(rcu_dereference_check(kvm->arch.apic_map, 1));
> }
>
> -void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> +void kvm_arch_free_memslot(struct kvm *kvm,
> + struct kvm_memory_slot *free,
> struct kvm_memory_slot *dont)
> {
> int i;
>
> + if (current->mm == kvm->mm && free->user_alloc) {
I think you mean !free->user_alloc. Also, you could check the
memslot->id instead so that we can remove the user_alloc field
entirely as it doesn't serve a useful function anymore.
> + if (!dont || !dont->user_alloc ||
> + free->userspace_addr != dont->userspace_addr) {
> + int ret;
> +
> + ret = vm_munmap(free->userspace_addr,
> + free->npages * PAGE_SIZE);
> + if (ret < 0)
> + printk(KERN_WARNING
> + "kvm_vm_ioctl_set_memory_region: "
> + "failed to munmap memory\n");
> + }
> + }
> +
> for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
> if (!dont || free->arch.rmap[i] != dont->arch.rmap[i]) {
> kvm_kvfree(free->arch.rmap[i]);
> @@ -6453,7 +6478,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
> *x86 needs to handle !user_alloc case.
> */
> if (!user_alloc) {
> - if (npages && !old.npages) {
> + if (npages != old.npages) {
> unsigned long userspace_addr;
>
> userspace_addr = vm_mmap(NULL, 0,
> @@ -6466,7 +6491,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
> return PTR_ERR((void *)userspace_addr);
>
> memslot->userspace_addr = userspace_addr;
> - }
> + } else
> + memslot->userspace_addr = old.userspace_addr;
> }
>
>
> @@ -6481,17 +6507,6 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>
> int nr_mmu_pages = 0, npages = mem->memory_size >> PAGE_SHIFT;
>
> - if (!user_alloc && !old.user_alloc && old.npages && !npages) {
> - int ret;
> -
> - ret = vm_munmap(old.userspace_addr,
> - old.npages * PAGE_SIZE);
> - if (ret < 0)
> - printk(KERN_WARNING
> - "kvm_vm_ioctl_set_memory_region: "
> - "failed to munmap memory\n");
> - }
> -
> if (!kvm->arch.n_requested_mmu_pages)
> nr_mmu_pages = kvm_mmu_calculate_mmu_pages(kvm);
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index ecc5543..8f2a863 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -436,7 +436,8 @@ int kvm_set_memory_region(struct kvm *kvm,
> int __kvm_set_memory_region(struct kvm *kvm,
> struct kvm_userspace_memory_region *mem,
> int user_alloc);
> -void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> +void kvm_arch_free_memslot(struct kvm *kvm,
> + struct kvm_memory_slot *free,
> struct kvm_memory_slot *dont);
> int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages);
> int kvm_arch_prepare_memory_region(struct kvm *kvm,
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index be70035..ea63b9c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -546,13 +546,14 @@ static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot)
> /*
> * Free any memory in @free but not in @dont.
> */
> -static void kvm_free_physmem_slot(struct kvm_memory_slot *free,
> +static void kvm_free_physmem_slot(struct kvm *kvm,
> + struct kvm_memory_slot *free,
> struct kvm_memory_slot *dont)
> {
> if (!dont || free->dirty_bitmap != dont->dirty_bitmap)
> kvm_destroy_dirty_bitmap(free);
>
> - kvm_arch_free_memslot(free, dont);
> + kvm_arch_free_memslot(kvm, free, dont);
>
> free->npages = 0;
> }
> @@ -563,7 +564,7 @@ void kvm_free_physmem(struct kvm *kvm)
> struct kvm_memory_slot *memslot;
>
> kvm_for_each_memslot(memslot, slots)
> - kvm_free_physmem_slot(memslot, NULL);
> + kvm_free_physmem_slot(kvm, memslot, NULL);
>
> kfree(kvm->memslots);
> }
> @@ -851,13 +852,13 @@ int __kvm_set_memory_region(struct kvm *kvm,
>
> kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
>
> - kvm_free_physmem_slot(&old, &new);
> + kvm_free_physmem_slot(kvm, &old, &new);
> kfree(old_memslots);
>
> return 0;
>
> out_free:
> - kvm_free_physmem_slot(&new, &old);
> + kvm_free_physmem_slot(kvm, &new, &old);
> out:
> return r;
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
