Re: [PATCH v1] xen/arm: Do not allocate pte entries for MAP_SMALL_PAGES

[Julien Grall <julien.grall@linaro.org>]

Hello Vijay,

On 18/02/2015 12:56, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> On x86, for the pages mapped with PAGE_HYPERVISOR attribute
> non-leaf page tables are allocated with valid pte entries.
> and with MAP_SMALL_PAGES attribute only non-leaf page tables are
> allocated with invalid (valid bit set to 0) pte entries.
> However on arm this is not the case. On arm for the pages
> mapped with PAGE_HYPERVISOR and MAP_SMALL_PAGES both
> non-leaf and leaf level page table are allocated with valid bit
> in pte entries.
>
> This behaviour in arm makes common vmap code fail to
> allocate memory beyond 128MB as described below.
>
> 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.
>
> With this patch, MAP_SMALL_PAGES attribute will only allocate
> non-leaf page tables only.
>
> Here we use bit[16] in the attribute flag to know if leaf page
> tables should be allocated or not.
>
> This bit is set only for MAP_SMALL_PAGES attribute.
>
> Signed-off-by: Vijaya Kumar K<Vijaya.Kumar@caviumnetworks.com>
> ---
>   xen/arch/arm/mm.c          |    9 ++++++---
>   xen/include/asm-arm/page.h |    8 +++++++-
>   2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7d4ba0c..a12f3f5 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -865,9 +865,12 @@ static int create_xen_entries(enum xenmap_operation op,
>                              addr, mfn);
>                       return -EINVAL;
>                   }
> -                pte = mfn_to_xen_entry(mfn, ai);
> -                pte.pt.table = 1;
> -                write_pte(&third[third_table_offset(addr)], pte);
> +                if ( !(ai & PTE_INVALID) )

you could do if ( ai & PTE_INVALID ) break; It would avoid a new level 
of indentation.

Also, I would rename ai to flags as the variable is not anymore an 
attribute index.

> +                {
> +                    pte = mfn_to_xen_entry(mfn, (ai & 0xffff));

Please introduce a new macro for the mask.

> +                    pte.pt.table = 1;
> +                    write_pte(&third[third_table_offset(addr)], pte);
> +                }
>                   break;
>               case REMOVE:
>                   if ( !third[third_table_offset(addr)].pt.valid )
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 3e7b0ae..80415b3 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -61,10 +61,16 @@
>   #define DEV_WC        BUFFERABLE
>   #define DEV_CACHED    WRITEBACK
>
> +/* bit 16 in the Attribute index can be used to know if
> + * PTE entry should be added or not. This is useful
> + * when ONLY non-leaf page table entries need to allocated
> + */
> +#define PTE_INVALID   (0x1 << 16)

It makes more sense to introduce a PTE_PRESENT flags compare to 
PTE_INVALID. The former has more meaning that the latter.

Regards,

-- 
Julien Grall