Re: [kvm-unit-tests PATCH v2 5/6] x86: lib/alloc: introduce alloc_zeroed_page

[Laurent Vivier <lvivier@redhat.com>]

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