[kvm-unit-tests PATCH] lib/alloc_page: Zero allocated pages

[kvm-unit-tests PATCH] lib/alloc_page: Zero allocated pages

[nadav.amit]

From: Nadav Amit <nadav.amit@gmail.com>

One of the most important properties of tests is reproducibility. For
tests to be reproducible, the same environment should be set on each
test invocation.

When it comes to memory content, this is not exactly the case in
kvm-unit-tests. The tests might, mistakenly or intentionally, assume
that memory is zeroed, which apparently is the case after seabios runs.
However, failures might not be reproducible if this assumption is
broken.

As an example, consider x86 do_iret(), which mistakenly does not push
SS:RSP onto the stack on 64-bit mode, although they are popped
unconditionally.

Do not assume that memory is zeroed. Clear it once it is allocated to
allow tests to easily be reproducible.

Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 lib/alloc_page.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 730f2b5..b0f4515 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -65,6 +65,7 @@ void *alloc_page()
 	freelist = *(void **)freelist;
 	spin_unlock(&lock);
 
+	memset(p, 0, PAGE_SIZE);
 	return p;
 }
 
-- 
2.17.1

Re: [kvm-unit-tests PATCH] lib/alloc_page: Zero allocated pages

[Andrew Jones]

On Thu, May 02, 2019 at 08:40:38AM -0700, nadav.amit@gmail.com wrote:
> From: Nadav Amit <nadav.amit@gmail.com>
> 
> One of the most important properties of tests is reproducibility. For
> tests to be reproducible, the same environment should be set on each
> test invocation.
> 
> When it comes to memory content, this is not exactly the case in
> kvm-unit-tests. The tests might, mistakenly or intentionally, assume
> that memory is zeroed, which apparently is the case after seabios runs.
> However, failures might not be reproducible if this assumption is
> broken.
> 
> As an example, consider x86 do_iret(), which mistakenly does not push
> SS:RSP onto the stack on 64-bit mode, although they are popped
> unconditionally.
> 
> Do not assume that memory is zeroed. Clear it once it is allocated to
> allow tests to easily be reproducible.
> 
> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> ---
>  lib/alloc_page.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index 730f2b5..b0f4515 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -65,6 +65,7 @@ void *alloc_page()
>  	freelist = *(void **)freelist;
>  	spin_unlock(&lock);
>  
> +	memset(p, 0, PAGE_SIZE);
>  	return p;
>  }
>  
> -- 
> 2.17.1
>

I think this is reasonable, but if we make this change then we should
remove the now redundant page zeroing too. A quick grep shows 20
instances

 $ git grep -A1 alloc_page | grep memset | grep 0, | wc -l
 20

There may be more.

Thanks,
drew

Re: [kvm-unit-tests PATCH] lib/alloc_page: Zero allocated pages

[Nadav Amit]

> On May 3, 2019, at 1:19 AM, Andrew Jones <drjones@redhat.com> wrote:
> 
> On Thu, May 02, 2019 at 08:40:38AM -0700, nadav.amit@gmail.com wrote:
>> From: Nadav Amit <nadav.amit@gmail.com>
>> 
>> One of the most important properties of tests is reproducibility. For
>> tests to be reproducible, the same environment should be set on each
>> test invocation.
>> 
>> When it comes to memory content, this is not exactly the case in
>> kvm-unit-tests. The tests might, mistakenly or intentionally, assume
>> that memory is zeroed, which apparently is the case after seabios runs.
>> However, failures might not be reproducible if this assumption is
>> broken.
>> 
>> As an example, consider x86 do_iret(), which mistakenly does not push
>> SS:RSP onto the stack on 64-bit mode, although they are popped
>> unconditionally.
>> 
>> Do not assume that memory is zeroed. Clear it once it is allocated to
>> allow tests to easily be reproducible.
>> 
>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
>> ---
>> lib/alloc_page.c | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
>> index 730f2b5..b0f4515 100644
>> --- a/lib/alloc_page.c
>> +++ b/lib/alloc_page.c
>> @@ -65,6 +65,7 @@ void *alloc_page()
>> 	freelist = *(void **)freelist;
>> 	spin_unlock(&lock);
>> 
>> +	memset(p, 0, PAGE_SIZE);
>> 	return p;
>> }
>> 
>> -- 
>> 2.17.1
> 
> I think this is reasonable, but if we make this change then we should
> remove the now redundant page zeroing too. A quick grep shows 20
> instances
> 
> $ git grep -A1 alloc_page | grep memset | grep 0, | wc -l
> 20
> 
> There may be more.

There are. I’ll send it in v2.