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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 23/02/2015 17:07, Ian Campbell wrote:
> 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.
Actually the v2 is going on this sense, though I had some comment on it.

Regards,

--
Julien Grall

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