Re: [Xen-devel] [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-02-17 at 14:50 +0000, Julien Grall wrote:
> Hello Vijay,
> 
> The title looks more a title for a bug report rather than a patch.
> 
> On 17/02/15 06:07, Vijay Kilari wrote:
> > In vmap_init, map_pages_to_xen() is called for mapping
> > vm_bitmap. Initially one page of vm_bitmap is allocated
> > and mapped using PAGE_HYPERVISOR attribute.
> > For the rest of vm_bitmap pages, MAP_SMALL_PAGES attribute
> > is used to map.
> > 
> > In ARM for both PAGE_HYPERVISOR and MAP_SMALL_PAGES, valid bit
> > is set to 1 in pte entry for these mapping.
> > 
> > In vma_alloc(), map_pages_to_xen() is failing for >128MB because
> > for this next vm_bitmap page the mapping is already set in vm_init()
> > with valid bit set in pte entry. So map_pages_to_xen() in
> > ARM returns error.
> > 
> > On x86, for the pages mapped with MAP_SMALL_PAGES attribute
> > valid bit is not set. So the common vmap code assumes the
> > same behaviour from arm. However this is not the case.
> > 
> > This patch will set valid bit to 0 in pte entry when
> > pages are mapped with MAP_SMALL_PAGES.
> 
> Setting the valid bit to 0 or 1, as long as the 128M limit, are just
> side-effects of the real problem.
> 
> You should explain what does MAP_SMALL_PAGES and PAGE_HYPERVISOR means.
> For now, the reader needs to understand by himself that MAP_SMALL_PAGES
> is used to pre-allocate non-leaf page table.
> 
> [..]
> 
> > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > index 53d4b63..e704ac8 100644
> > --- a/xen/include/asm-arm/page.h
> > +++ b/xen/include/asm-arm/page.h
> > @@ -58,6 +58,7 @@
> >  #define WRITEBACK     0x3
> >  #define DEV_SHARED    0x4
> >  #define WRITEALLOC    0x7
> > +#define WRITEALLOC_INVALID    0xf
> 
> Why WRITEALLOC_INVALID? The mapping is not at all invalid and doesn't
> exist... We just asked to pre-allocate the non-leaf page.

> 
> >  #define DEV_NONSHARED DEV_SHARED
> >  #define DEV_WC        BUFFERABLE
> >  #define DEV_CACHED    WRITEBACK
> > @@ -65,7 +66,7 @@
> >  #define PAGE_HYPERVISOR         (WRITEALLOC)
> >  #define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
> >  #define PAGE_HYPERVISOR_WC      (DEV_WC)
> > -#define MAP_SMALL_PAGES         PAGE_HYPERVISOR
> > +#define MAP_SMALL_PAGES         (WRITEALLOC_INVALID)
> 
> The parameter "flags" of map_pages_to_xen is an unsigned int. It would
> be better to use the top bits for adding new flags (such as a present flag).
> 
> Then you could use this flag in create_xen_entries to know if we need to
> set the present bit or not. Or, even better, skip the creation of the PTE.

The underlying problem here seems to be that PAGE_FOO on x86 is not just
the attributes but is actually the present bit etc too:
#define __PAGE_HYPERVISOR \
    (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
#define PAGE_HYPERVISOR         (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)

changing ARM similarly (i.e. shifting WRITEALLOC etc to the appropriate
place and adding the present bit) would seem like a good way to both
address this issue and make the arch interfaces more consistent.

MAP_SMALL_PAGES could then be an avail bit or something like on x86.

> 
> This would make the code more cleaner and avoid workaround like you did
> in map_pages_to_xen.
> 
> Regards,
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel