Hi Guangrong,
I am starting to review this series, and should have some comments or questions, you can determine
whether they are valuable :)
+static void page_track_slot_free(struct kvm_memory_slot *slot)Is it necessary to use the 'unsigned long npages' pareameter? In my understanding you are going to
+{
+ int i;
+
+ for (i = 0; i < KVM_PAGE_TRACK_MAX; i++)
+ if (slot->arch.gfn_track[i]) {
+ kvfree(slot->arch.gfn_track[i]);
+ slot->arch.gfn_track[i] = NULL;
+ }
+}
+
+int kvm_page_track_create_memslot(struct kvm_memory_slot *slot,
+ unsigned long npages)
+{
+ int i, pages = gfn_to_index(slot->base_gfn + npages - 1,
+ slot->base_gfn, PT_PAGE_TABLE_LEVEL) + 1;
+
+ for (i = 0; i < KVM_PAGE_TRACK_MAX; i++) {
+ slot->arch.gfn_track[i] = kvm_kvzalloc(pages *
+ sizeof(*slot->arch.gfn_track[i]));
+ if (!slot->arch.gfn_track[i])
+ goto track_free;
+ }
+
+ return 0;
+
+track_free:
+ page_track_slot_free(slot);
+ return -ENOMEM;
+}
track all GFNs in the memory slot anyway, right? If you want to keep npages, I think it's better to
add a base_gfn as well otherwise you are assuming you are going to track the npages GFN starting
from slot->base_gfn.
+Looks essentially you are allocating one int for all GFNs of the slot unconditionally. In my
+void kvm_page_track_free_memslot(struct kvm_memory_slot *free,
+ struct kvm_memory_slot *dont)
+{
+ if (!dont || free->arch.gfn_track != dont->arch.gfn_track)
+ page_track_slot_free(free);
+}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c04987e..ad4888a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7838,6 +7838,8 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
free->arch.lpage_info[i - 1] = NULL;
}
}
+
+ kvm_page_track_free_memslot(free, dont);
}
int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
@@ -7886,6 +7888,9 @@ int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
}
}
+ if (kvm_page_track_create_memslot(slot, npages))
+ goto out_free;
+
understanding for most of memory slots, we are not going to track them, so isn't it going to be
wasteful of memory?