| Message ID | 1448907973-36066-5-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Guangrong, I am starting to review this series, and should have some comments or questions, you can determine whether they are valuable :) See below. On 12/01/2015 02:26 AM, Xiao Guangrong wrote: > The array, gfn_track[mode][gfn], is introduced in memory slot for every > guest page, this is the tracking count for the gust page on different > modes. If the page is tracked then the count is increased, the page is > not tracked after the count reaches zero > > Two callbacks, kvm_page_track_create_memslot() and > kvm_page_track_free_memslot() are implemented in this patch, they are > internally used to initialize and reclaim the memory of the array > > Currently, only write track mode is supported > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > arch/x86/include/asm/kvm_host.h | 2 ++ > arch/x86/include/asm/kvm_page_track.h | 13 ++++++++ > arch/x86/kvm/Makefile | 3 +- > arch/x86/kvm/page_track.c | 58 +++++++++++++++++++++++++++++++++++ > arch/x86/kvm/x86.c | 5 +++ > 5 files changed, 80 insertions(+), 1 deletion(-) > create mode 100644 arch/x86/include/asm/kvm_page_track.h > create mode 100644 arch/x86/kvm/page_track.c > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 5aa2dcc..afff1f1 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -32,6 +32,7 @@ > #include <asm/mtrr.h> > #include <asm/msr-index.h> > #include <asm/asm.h> > +#include <asm/kvm_page_track.h> > > #define KVM_MAX_VCPUS 255 > #define KVM_SOFT_MAX_VCPUS 160 > @@ -612,6 +613,7 @@ struct kvm_lpage_info { > struct kvm_arch_memory_slot { > struct kvm_rmap_head *rmap[KVM_NR_PAGE_SIZES]; > struct kvm_lpage_info *lpage_info[KVM_NR_PAGE_SIZES - 1]; > + int *gfn_track[KVM_PAGE_TRACK_MAX]; > }; > > /* > diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h > new file mode 100644 > index 0000000..347d5c9 > --- /dev/null > +++ b/arch/x86/include/asm/kvm_page_track.h > @@ -0,0 +1,13 @@ > +#ifndef _ASM_X86_KVM_PAGE_TRACK_H > +#define _ASM_X86_KVM_PAGE_TRACK_H > + > +enum kvm_page_track_mode { > + KVM_PAGE_TRACK_WRITE, > + KVM_PAGE_TRACK_MAX, > +}; > + > +int kvm_page_track_create_memslot(struct kvm_memory_slot *slot, > + unsigned long npages); > +void kvm_page_track_free_memslot(struct kvm_memory_slot *free, > + struct kvm_memory_slot *dont); > +#endif > diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile > index a1ff508..464fa47 100644 > --- a/arch/x86/kvm/Makefile > +++ b/arch/x86/kvm/Makefile > @@ -13,9 +13,10 @@ kvm-$(CONFIG_KVM_ASYNC_PF) += $(KVM)/async_pf.o > > kvm-y += x86.o mmu.o emulate.o i8259.o irq.o lapic.o \ > i8254.o ioapic.o irq_comm.o cpuid.o pmu.o mtrr.o \ > - hyperv.o > + hyperv.o page_track.o > > kvm-$(CONFIG_KVM_DEVICE_ASSIGNMENT) += assigned-dev.o iommu.o > + > kvm-intel-y += vmx.o pmu_intel.o > kvm-amd-y += svm.o pmu_amd.o > > diff --git a/arch/x86/kvm/page_track.c b/arch/x86/kvm/page_track.c > new file mode 100644 > index 0000000..0338d36 > --- /dev/null > +++ b/arch/x86/kvm/page_track.c > @@ -0,0 +1,58 @@ > +/* > + * Support KVM gust page tracking > + * > + * This feature allows us to track page access in guest. Currently, only > + * write access is tracked. > + * > + * Copyright(C) 2015 Intel Corporation. > + * > + * Author: > + * Xiao Guangrong <guangrong.xiao@linux.intel.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. > + */ > + > +#include <linux/kvm_host.h> > +#include <asm/kvm_host.h> > +#include <asm/kvm_page_track.h> > + > +#include "mmu.h" > + > +static void page_track_slot_free(struct kvm_memory_slot *slot) > +{ > + 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; > +} Is it necessary to use the 'unsigned long npages' pareameter? In my understanding you are going to 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. > + > +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; > + Looks essentially you are allocating one int for all GFNs of the slot unconditionally. In my understanding for most of memory slots, we are not going to track them, so isn't it going to be wasteful of memory? Thanks, -Kai > return 0; > > out_free: -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/15/2015 03:06 PM, Kai Huang wrote: > Hi Guangrong, > > I am starting to review this series, and should have some comments or questions, you can determine > whether they are valuable :) Thank you very much for your review and breaking the silent on this patchset. ;) >> +static void page_track_slot_free(struct kvm_memory_slot *slot) >> +{ >> + 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; >> +} > Is it necessary to use the 'unsigned long npages' pareameter? In my understanding you are going to The type, 'int', is used here as I followed the style of 'struct kvm_lpage_info'. 4 bytes should be enough to track all users and signed type is good to track overflow. > 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. Yes, any page in the memslot may be tracked so that there is a index for every page. > >> + >> +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; >> + > Looks essentially you are allocating one int for all GFNs of the slot unconditionally. In my > understanding for most of memory slots, we are not going to track them, so isn't it going to be > wasteful of memory? > Yes, hmm... maybe we can make the index as "unsigned short" then 1G memory only needs 512k index buffer. It is not so unacceptable. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/15/2015 04:46 PM, Xiao Guangrong wrote: > > > On 12/15/2015 03:06 PM, Kai Huang wrote: >> Hi Guangrong, >> >> I am starting to review this series, and should have some comments or >> questions, you can determine >> whether they are valuable :) > > Thank you very much for your review and breaking the silent on this > patchset. ;) > > >>> +static void page_track_slot_free(struct kvm_memory_slot *slot) >>> +{ >>> + 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; >>> +} >> Is it necessary to use the 'unsigned long npages' pareameter? In my >> understanding you are going to > > The type, 'int', is used here as I followed the style of 'struct > kvm_lpage_info'. > > 4 bytes should be enough to track all users and signed type is good to > track > overflow. > >> 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. > > Yes, any page in the memslot may be tracked so that there is a index > for every > page. > >> >>> + >>> +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; >>> + >> Looks essentially you are allocating one int for all GFNs of the slot >> unconditionally. In my >> understanding for most of memory slots, we are not going to track >> them, so isn't it going to be >> wasteful of memory? >> > > Yes, hmm... maybe we can make the index as "unsigned short" then 1G > memory only needs 512k index > buffer. It is not so unacceptable. Those comments are really minor and don't bother on this :) Thanks, -Kai -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 5aa2dcc..afff1f1 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -32,6 +32,7 @@ #include <asm/mtrr.h> #include <asm/msr-index.h> #include <asm/asm.h> +#include <asm/kvm_page_track.h> #define KVM_MAX_VCPUS 255 #define KVM_SOFT_MAX_VCPUS 160 @@ -612,6 +613,7 @@ struct kvm_lpage_info { struct kvm_arch_memory_slot { struct kvm_rmap_head *rmap[KVM_NR_PAGE_SIZES]; struct kvm_lpage_info *lpage_info[KVM_NR_PAGE_SIZES - 1]; + int *gfn_track[KVM_PAGE_TRACK_MAX]; }; /* diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h new file mode 100644 index 0000000..347d5c9 --- /dev/null +++ b/arch/x86/include/asm/kvm_page_track.h @@ -0,0 +1,13 @@ +#ifndef _ASM_X86_KVM_PAGE_TRACK_H +#define _ASM_X86_KVM_PAGE_TRACK_H + +enum kvm_page_track_mode { + KVM_PAGE_TRACK_WRITE, + KVM_PAGE_TRACK_MAX, +}; + +int kvm_page_track_create_memslot(struct kvm_memory_slot *slot, + unsigned long npages); +void kvm_page_track_free_memslot(struct kvm_memory_slot *free, + struct kvm_memory_slot *dont); +#endif diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile index a1ff508..464fa47 100644 --- a/arch/x86/kvm/Makefile +++ b/arch/x86/kvm/Makefile @@ -13,9 +13,10 @@ kvm-$(CONFIG_KVM_ASYNC_PF) += $(KVM)/async_pf.o kvm-y += x86.o mmu.o emulate.o i8259.o irq.o lapic.o \ i8254.o ioapic.o irq_comm.o cpuid.o pmu.o mtrr.o \ - hyperv.o + hyperv.o page_track.o kvm-$(CONFIG_KVM_DEVICE_ASSIGNMENT) += assigned-dev.o iommu.o + kvm-intel-y += vmx.o pmu_intel.o kvm-amd-y += svm.o pmu_amd.o diff --git a/arch/x86/kvm/page_track.c b/arch/x86/kvm/page_track.c new file mode 100644 index 0000000..0338d36 --- /dev/null +++ b/arch/x86/kvm/page_track.c @@ -0,0 +1,58 @@ +/* + * Support KVM gust page tracking + * + * This feature allows us to track page access in guest. Currently, only + * write access is tracked. + * + * Copyright(C) 2015 Intel Corporation. + * + * Author: + * Xiao Guangrong <guangrong.xiao@linux.intel.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + */ + +#include <linux/kvm_host.h> +#include <asm/kvm_host.h> +#include <asm/kvm_page_track.h> + +#include "mmu.h" + +static void page_track_slot_free(struct kvm_memory_slot *slot) +{ + 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; +} + +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; + return 0; out_free:
The array, gfn_track[mode][gfn], is introduced in memory slot for every guest page, this is the tracking count for the gust page on different modes. If the page is tracked then the count is increased, the page is not tracked after the count reaches zero Two callbacks, kvm_page_track_create_memslot() and kvm_page_track_free_memslot() are implemented in this patch, they are internally used to initialize and reclaim the memory of the array Currently, only write track mode is supported Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> --- arch/x86/include/asm/kvm_host.h | 2 ++ arch/x86/include/asm/kvm_page_track.h | 13 ++++++++ arch/x86/kvm/Makefile | 3 +- arch/x86/kvm/page_track.c | 58 +++++++++++++++++++++++++++++++++++ arch/x86/kvm/x86.c | 5 +++ 5 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 arch/x86/include/asm/kvm_page_track.h create mode 100644 arch/x86/kvm/page_track.c