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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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.

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

Regards,

-- 
Julien Grall

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