| Message ID | 20190502154038.8267-1-nadav.amit@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] lib/alloc_page: Zero allocated pages | expand |
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
> 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.
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; }