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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
