| Message ID | 1478119966-13252-6-git-send-email-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/11/2016 21:52, Andrew Jones wrote: > This allows us to remove a bunch of memsets. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > lib/alloc.c | 8 ++++++++ > lib/alloc.h | 1 + > lib/x86/vm.c | 4 +--- > x86/vmx.c | 19 +++++++------------ > x86/vmx_tests.c | 28 ++++++++++------------------ > 5 files changed, 27 insertions(+), 33 deletions(-) > > diff --git a/lib/alloc.c b/lib/alloc.c > index ce1198e2977f..d797690cc458 100644 > --- a/lib/alloc.c > +++ b/lib/alloc.c > @@ -191,6 +191,14 @@ void *alloc_page(void) > return p; > } > > +void *alloc_zeroed_page(void) > +{ > + void *p = alloc_page(); alloc_page() can return NULL. As most of the callers don't check the return value of alloc_zeroed_page(), I think you should assert() here. > + memset(p, 0, PAGE_SIZE); > + return p; > +} > + > void free_page(void *page) > { > spin_lock(&heap_lock); > diff --git a/lib/alloc.h b/lib/alloc.h > index a37330b3088a..f61a5200c829 100644 > --- a/lib/alloc.h > +++ b/lib/alloc.h > @@ -126,6 +126,7 @@ extern void phys_alloc_show(void); > */ > extern void heap_init(void *start, size_t size); > extern void *alloc_page(void); > +extern void *alloc_zeroed_page(void); > extern void free_page(void *page); > > #endif /* _ALLOC_H_ */ > diff --git a/lib/x86/vm.c b/lib/x86/vm.c > index 4e399f80dd31..8b95104ef80f 100644 > --- a/lib/x86/vm.c > +++ b/lib/x86/vm.c > @@ -91,9 +91,7 @@ static void setup_mmu_range(unsigned long *cr3, unsigned long start, > > static void setup_mmu(unsigned long len) > { > - unsigned long *cr3 = alloc_page(); > - > - memset(cr3, 0, PAGE_SIZE); > + unsigned long *cr3 = alloc_zeroed_page(); > > #ifdef __x86_64__ > if (len < (1ul << 32)) > diff --git a/x86/vmx.c b/x86/vmx.c > index 411ed3211d4d..5d333e077a02 100644 > --- a/x86/vmx.c > +++ b/x86/vmx.c > @@ -29,6 +29,7 @@ > */ > > #include "libcflat.h" > +#include "alloc.h" > #include "processor.h" > #include "vm.h" > #include "desc.h" > @@ -276,9 +277,8 @@ static void split_large_ept_entry(unsigned long *ptep, int level) > assert(pte & EPT_LARGE_PAGE); > assert(level == 2 || level == 3); > > - new_pt = alloc_page(); > + new_pt = alloc_zeroed_page(); > assert(new_pt); If alloc_zeroed_page() uses assert(), this one is useless, otherwise add the same to all the other calls of alloc_zeroed_page() below. > - memset(new_pt, 0, PAGE_SIZE); > > prototype = pte & ~EPT_ADDR_MASK; > if (level == 2) > @@ -617,8 +617,7 @@ static void init_vmcs_guest(void) ... Thanks, Laurent -- 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 Thu, Nov 03, 2016 at 01:02:03PM +0100, Laurent Vivier wrote: > > > On 02/11/2016 21:52, Andrew Jones wrote: > > This allows us to remove a bunch of memsets. > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > lib/alloc.c | 8 ++++++++ > > lib/alloc.h | 1 + > > lib/x86/vm.c | 4 +--- > > x86/vmx.c | 19 +++++++------------ > > x86/vmx_tests.c | 28 ++++++++++------------------ > > 5 files changed, 27 insertions(+), 33 deletions(-) > > > > diff --git a/lib/alloc.c b/lib/alloc.c > > index ce1198e2977f..d797690cc458 100644 > > --- a/lib/alloc.c > > +++ b/lib/alloc.c > > @@ -191,6 +191,14 @@ void *alloc_page(void) > > return p; > > } > > > > +void *alloc_zeroed_page(void) > > +{ > > + void *p = alloc_page(); > > alloc_page() can return NULL. > > As most of the callers don't check the return value of > alloc_zeroed_page(), I think you should assert() here. Hmm... both returning NULL (which you're right I should have done here) and asserting have their merits. If a unit test, for whatever reason, wanted to allocate all memory, then it would impossible to know how many alloc_page calls that would take before needing to stop to avoid the assert. It's much easier to loop until NULL. However, most callers won't do that, and, as you point out below, most callers are neglecting to check for NULL. So I think we need both. I propose renaming alloc_page to __alloc_page. Unit tests that want to avoid the assert and check for NULL should use that. But most callers should use a new alloc_page, void *alloc_page(void) { void *p = __alloc_page(); assert(p); return p; } alloc_zeroed_page will be built on alloc_page. __alloc_page users can zero their own pages... Thoughts on that? > > > > + memset(p, 0, PAGE_SIZE); > > + return p; > > +} > > + > > void free_page(void *page) > > { > > spin_lock(&heap_lock); > > diff --git a/lib/alloc.h b/lib/alloc.h > > index a37330b3088a..f61a5200c829 100644 > > --- a/lib/alloc.h > > +++ b/lib/alloc.h > > @@ -126,6 +126,7 @@ extern void phys_alloc_show(void); > > */ > > extern void heap_init(void *start, size_t size); > > extern void *alloc_page(void); > > +extern void *alloc_zeroed_page(void); > > extern void free_page(void *page); > > > > #endif /* _ALLOC_H_ */ > > diff --git a/lib/x86/vm.c b/lib/x86/vm.c > > index 4e399f80dd31..8b95104ef80f 100644 > > --- a/lib/x86/vm.c > > +++ b/lib/x86/vm.c > > @@ -91,9 +91,7 @@ static void setup_mmu_range(unsigned long *cr3, unsigned long start, > > > > static void setup_mmu(unsigned long len) > > { > > - unsigned long *cr3 = alloc_page(); > > - > > - memset(cr3, 0, PAGE_SIZE); > > + unsigned long *cr3 = alloc_zeroed_page(); > > > > #ifdef __x86_64__ > > if (len < (1ul << 32)) > > diff --git a/x86/vmx.c b/x86/vmx.c > > index 411ed3211d4d..5d333e077a02 100644 > > --- a/x86/vmx.c > > +++ b/x86/vmx.c > > @@ -29,6 +29,7 @@ > > */ > > > > #include "libcflat.h" > > +#include "alloc.h" > > #include "processor.h" > > #include "vm.h" > > #include "desc.h" > > @@ -276,9 +277,8 @@ static void split_large_ept_entry(unsigned long *ptep, int level) > > assert(pte & EPT_LARGE_PAGE); > > assert(level == 2 || level == 3); > > > > - new_pt = alloc_page(); > > + new_pt = alloc_zeroed_page(); > > assert(new_pt); > > If alloc_zeroed_page() uses assert(), this one is useless, otherwise add > the same to all the other calls of alloc_zeroed_page() below. > > > - memset(new_pt, 0, PAGE_SIZE); > > > > prototype = pte & ~EPT_ADDR_MASK; > > if (level == 2) > > @@ -617,8 +617,7 @@ static void init_vmcs_guest(void) > ... > > Thanks, > Laurent > -- > 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 -- 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 03/11/2016 13:47, Andrew Jones wrote: > On Thu, Nov 03, 2016 at 01:02:03PM +0100, Laurent Vivier wrote: >> >> >> On 02/11/2016 21:52, Andrew Jones wrote: >>> This allows us to remove a bunch of memsets. >>> >>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>> --- >>> lib/alloc.c | 8 ++++++++ >>> lib/alloc.h | 1 + >>> lib/x86/vm.c | 4 +--- >>> x86/vmx.c | 19 +++++++------------ >>> x86/vmx_tests.c | 28 ++++++++++------------------ >>> 5 files changed, 27 insertions(+), 33 deletions(-) >>> >>> diff --git a/lib/alloc.c b/lib/alloc.c >>> index ce1198e2977f..d797690cc458 100644 >>> --- a/lib/alloc.c >>> +++ b/lib/alloc.c >>> @@ -191,6 +191,14 @@ void *alloc_page(void) >>> return p; >>> } >>> >>> +void *alloc_zeroed_page(void) >>> +{ >>> + void *p = alloc_page(); >> >> alloc_page() can return NULL. >> >> As most of the callers don't check the return value of >> alloc_zeroed_page(), I think you should assert() here. > > Hmm... both returning NULL (which you're right I should have done here) > and asserting have their merits. If a unit test, for whatever reason, > wanted to allocate all memory, then it would impossible to know how many > alloc_page calls that would take before needing to stop to avoid the > assert. It's much easier to loop until NULL. However, most callers won't > do that, and, as you point out below, most callers are neglecting to > check for NULL. > > So I think we need both. I propose renaming alloc_page to __alloc_page. > Unit tests that want to avoid the assert and check for NULL should use > that. But most callers should use a new alloc_page, > > void *alloc_page(void) > { > void *p = __alloc_page(); > assert(p); > return p; > } > > alloc_zeroed_page will be built on alloc_page. __alloc_page users can > zero their own pages... > > Thoughts on that? It's nice to have both. Good idea. Thanks, Laurent -- 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/lib/alloc.c b/lib/alloc.c index ce1198e2977f..d797690cc458 100644 --- a/lib/alloc.c +++ b/lib/alloc.c @@ -191,6 +191,14 @@ void *alloc_page(void) return p; } +void *alloc_zeroed_page(void) +{ + void *p = alloc_page(); + + memset(p, 0, PAGE_SIZE); + return p; +} + void free_page(void *page) { spin_lock(&heap_lock); diff --git a/lib/alloc.h b/lib/alloc.h index a37330b3088a..f61a5200c829 100644 --- a/lib/alloc.h +++ b/lib/alloc.h @@ -126,6 +126,7 @@ extern void phys_alloc_show(void); */ extern void heap_init(void *start, size_t size); extern void *alloc_page(void); +extern void *alloc_zeroed_page(void); extern void free_page(void *page); #endif /* _ALLOC_H_ */ diff --git a/lib/x86/vm.c b/lib/x86/vm.c index 4e399f80dd31..8b95104ef80f 100644 --- a/lib/x86/vm.c +++ b/lib/x86/vm.c @@ -91,9 +91,7 @@ static void setup_mmu_range(unsigned long *cr3, unsigned long start, static void setup_mmu(unsigned long len) { - unsigned long *cr3 = alloc_page(); - - memset(cr3, 0, PAGE_SIZE); + unsigned long *cr3 = alloc_zeroed_page(); #ifdef __x86_64__ if (len < (1ul << 32)) diff --git a/x86/vmx.c b/x86/vmx.c index 411ed3211d4d..5d333e077a02 100644 --- a/x86/vmx.c +++ b/x86/vmx.c @@ -29,6 +29,7 @@ */ #include "libcflat.h" +#include "alloc.h" #include "processor.h" #include "vm.h" #include "desc.h" @@ -276,9 +277,8 @@ static void split_large_ept_entry(unsigned long *ptep, int level) assert(pte & EPT_LARGE_PAGE); assert(level == 2 || level == 3); - new_pt = alloc_page(); + new_pt = alloc_zeroed_page(); assert(new_pt); - memset(new_pt, 0, PAGE_SIZE); prototype = pte & ~EPT_ADDR_MASK; if (level == 2) @@ -617,8 +617,7 @@ static void init_vmcs_guest(void) static int init_vmcs(struct vmcs **vmcs) { - *vmcs = alloc_page(); - memset(*vmcs, 0, PAGE_SIZE); + *vmcs = alloc_zeroed_page(); (*vmcs)->revision_id = basic.revision; /* vmclear first to init vmcs */ if (vmcs_clear(*vmcs)) { @@ -656,8 +655,7 @@ static void init_vmx(void) ulong fix_cr0_set, fix_cr0_clr; ulong fix_cr4_set, fix_cr4_clr; - vmxon_region = alloc_page(); - memset(vmxon_region, 0, PAGE_SIZE); + vmxon_region = alloc_zeroed_page(); fix_cr0_set = rdmsr(MSR_IA32_VMX_CR0_FIXED0); fix_cr0_clr = rdmsr(MSR_IA32_VMX_CR0_FIXED1); @@ -686,10 +684,8 @@ static void init_vmx(void) *vmxon_region = basic.revision; - guest_stack = alloc_page(); - memset(guest_stack, 0, PAGE_SIZE); - guest_syscall_stack = alloc_page(); - memset(guest_syscall_stack, 0, PAGE_SIZE); + guest_stack = alloc_zeroed_page(); + guest_syscall_stack = alloc_zeroed_page(); } static void do_vmxon_off(void *data) @@ -811,8 +807,7 @@ static void test_vmptrst(void) int ret; struct vmcs *vmcs1, *vmcs2; - vmcs1 = alloc_page(); - memset(vmcs1, 0, PAGE_SIZE); + vmcs1 = alloc_zeroed_page(); init_vmcs(&vmcs1); ret = vmcs_save(&vmcs2); report("test vmptrst", (!ret) && (vmcs1 == vmcs2)); diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index 58736d789bd5..ebc220e8329c 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -3,6 +3,7 @@ * * Author : Arthur Chunqi Li <yzt356@gmail.com> */ +#include "alloc.h" #include "vmx.h" #include "msr.h" #include "processor.h" @@ -223,8 +224,7 @@ void msr_bmp_init() void *msr_bitmap; u32 ctrl_cpu0; - msr_bitmap = alloc_page(); - memset(msr_bitmap, 0x0, PAGE_SIZE); + msr_bitmap = alloc_zeroed_page(); ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0); ctrl_cpu0 |= CPU_MSR_BITMAP; vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0); @@ -565,10 +565,8 @@ static int iobmp_init() { u32 ctrl_cpu0; - io_bitmap_a = alloc_page(); - io_bitmap_b = alloc_page(); - memset(io_bitmap_a, 0x0, PAGE_SIZE); - memset(io_bitmap_b, 0x0, PAGE_SIZE); + io_bitmap_a = alloc_zeroed_page(); + io_bitmap_b = alloc_zeroed_page(); ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0); ctrl_cpu0 |= CPU_IO_BITMAP; ctrl_cpu0 &= (~CPU_IO); @@ -937,8 +935,7 @@ static int setup_ept() return 1; } eptp |= (3 << EPTP_PG_WALK_LEN_SHIFT); - pml4 = alloc_page(); - memset(pml4, 0, PAGE_SIZE); + pml4 = alloc_zeroed_page(); eptp |= virt_to_phys(pml4); vmcs_write(EPTP, eptp); support_2m = !!(ept_vpid.val & EPT_CAP_2M_PAGE); @@ -972,10 +969,8 @@ static int ept_init() vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]); if (setup_ept()) return VMX_TEST_EXIT; - data_page1 = alloc_page(); - data_page2 = alloc_page(); - memset(data_page1, 0x0, PAGE_SIZE); - memset(data_page2, 0x0, PAGE_SIZE); + data_page1 = alloc_zeroed_page(); + data_page2 = alloc_zeroed_page(); *((u32 *)data_page1) = MAGIC_VAL_1; *((u32 *)data_page2) = MAGIC_VAL_2; install_ept(pml4, (unsigned long)data_page1, (unsigned long)data_page2, @@ -1538,12 +1533,9 @@ struct vmx_msr_entry *exit_msr_store, *entry_msr_load, *exit_msr_load; static int msr_switch_init(struct vmcs *vmcs) { msr_bmp_init(); - exit_msr_store = alloc_page(); - exit_msr_load = alloc_page(); - entry_msr_load = alloc_page(); - memset(exit_msr_store, 0, PAGE_SIZE); - memset(exit_msr_load, 0, PAGE_SIZE); - memset(entry_msr_load, 0, PAGE_SIZE); + exit_msr_store = alloc_zeroed_page(); + exit_msr_load = alloc_zeroed_page(); + entry_msr_load = alloc_zeroed_page(); entry_msr_load[0].index = MSR_KERNEL_GS_BASE; entry_msr_load[0].value = MSR_MAGIC;
This allows us to remove a bunch of memsets. Signed-off-by: Andrew Jones <drjones@redhat.com> --- lib/alloc.c | 8 ++++++++ lib/alloc.h | 1 + lib/x86/vm.c | 4 +--- x86/vmx.c | 19 +++++++------------ x86/vmx_tests.c | 28 ++++++++++------------------ 5 files changed, 27 insertions(+), 33 deletions(-)