From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC PATCH] xen/arm: Vmap allocator fails to allocate beyond 128M Date: Mon, 23 Feb 2015 21:45:02 +0000 Message-ID: <54EB9F5E.8060807@linaro.org> References: <54E3551A.3090709@linaro.org> <1424711240.27930.229.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1424711240.27930.229.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Vijay Kilari , Stefano Stabellini , Tim Deegan , "xen-devel@lists.xen.org" , Stefano Stabellini , Jan Beulich List-Id: xen-devel@lists.xenproject.org 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