On Thu, Nov 6, 2008 at 4:14 PM, Avi Kivity <[EMAIL PROTECTED]> wrote:
> What happens here if the both free and dont have nonzero, differnt
> ->userspace_addr values? Is is even possible?
I dont think it can happen in the current kvm code, but I put that test in
order to respect the function behaviour of freeing any memory allocation
pointed to by free and not by dont (as described in the comment).
> Also, the call chain is fishy. set_memory_region calls free_physmem_slot
> which calls arch_set_memory_region. This is turning into pasta.
I agree, that's why I thought it would be better to put this outside
kvm_free_physmem_slot in my first patch. AFAICT, kvm_free_physmem_slot
is called by kvm_set_memory_region in order to free the memory holding
information regarding the slot but not the actual memory region held
by the slot: precisely because it is the role of kvm_set_memory_region
to free it.
So here is an attempt at something cleaner:
1 - Rename kvm_free_physmem_slot to kvm_free_physmem_slot_info
to indicate that it only frees the memory storing information about the slot
and not the memory region.
2- Make kvm_free_physmem free memory regions through
kvm_set_memory_region and let it free the slot info.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a87f45e..e59dc10 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -614,11 +614,24 @@ out:
return kvm;
}
+static void kvm_free_physmem_slot(struct kvm *kvm,
+ struct kvm_memory_slot *slot)
+{
+ struct kvm_userspace_memory_region mem = {
+ .slot = memslot_id(kvm, slot),
+ .guest_phys_addr = slot->base_gfn << PAGE_SHIFT,
+ .memory_size = 0,
+ .flags = 0,
+ };
+
+ kvm_set_memory_region(kvm, &mem, slot->user_alloc);
+}
+
/*
* Free any memory in @free but not in @dont.
*/
-static void kvm_free_physmem_slot(struct kvm_memory_slot *free,
- struct kvm_memory_slot *dont)
+static void kvm_free_physmem_slot_info(struct kvm_memory_slot *free,
+ struct kvm_memory_slot *dont)
{
if (!dont || free->rmap != dont->rmap)
vfree(free->rmap);
@@ -640,7 +653,7 @@ void kvm_free_physmem(struct kvm *kvm)
int i;
for (i = 0; i < kvm->nmemslots; ++i)
- kvm_free_physmem_slot(&kvm->memslots[i], NULL);
+ kvm_free_physmem_slot(kvm, &kvm->memslots[i]);
}
static void kvm_destroy_vm(struct kvm *kvm)
@@ -745,10 +758,14 @@ int __kvm_set_memory_region(struct kvm *kvm,
goto out_free;
}
- /* Free page dirty bitmap if unneeded */
+ /* Free any unneeded data */
if (!(new.flags & KVM_MEM_LOG_DIRTY_PAGES))
new.dirty_bitmap = NULL;
+ if (!npages) {
+ new.rmap = NULL;
+ new.lpage_info = NULL;
+ }
r = -ENOMEM;
/* Allocate if a slot is being created */
@@ -821,7 +838,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
goto out_free;
}
- kvm_free_physmem_slot(&old, &new);
+ kvm_free_physmem_slot_info(&old, &new);
#ifdef CONFIG_DMAR
/* map the pages in iommu page table */
r = kvm_iommu_map_pages(kvm, base_gfn, npages);
@@ -831,7 +848,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
return 0;
out_free:
- kvm_free_physmem_slot(&new, &old);
+ kvm_free_physmem_slot_info(&new, &old);
out:
return r;
Also, I've been reading a bit more about the linux mm and I now think
that to be able to use kvm->mm in arch_set_memory_region we need
to increase mm_users instead of mm_count. However, if we do that,
since memory maps wont be cleared when the process exits the kvm
fds which are still mapped in userspace will not be released so we will
have a bigger memory leak.
Any ideas on how to fix this properly?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
