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

[Julien Grall <julien.grall@linaro.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