Re: [PATCH] xen: arm: Support <32MB frametables

[Julien Grall <julien.grall@citrix.com>]

Hi Chris,

On 06/08/15 18:54, Chris (Christopher) Brand wrote:
> setup_frametable_mappings() rounds frametable_size up to a multiple
> of 32MB. This is wasteful on systems with less than 4GB of RAM,
> although it does allow the "contig" bit to be set in the PTEs.
> 
> Where the frametable is less than 32MB in size, instead round up
> to a multiple of 2MB, not setting the "contig" bit in the PTEs.

OOI, you win 30MB of RAM but how does this affect the performance?

> Signed-off-by: Chris Brand <chris.brand@broadcom.com>
> ---
>  xen/arch/arm/mm.c | 39 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index a91ea774f1f9..47b6d5d44563 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
>  #ifdef CONFIG_ARM_32
> +static void __init create_2mb_mappings(lpae_t *second,
> +                                       unsigned long virt_offset,
> +                                       unsigned long base_mfn,
> +                                       unsigned long nr_mfns)
> +{
> +    unsigned long i, count;
> +    lpae_t pte, *p;
> +
> +    ASSERT(!((virt_offset >> PAGE_SHIFT) % LPAE_ENTRIES));
> +    ASSERT(!(base_mfn % LPAE_ENTRIES));
> +    ASSERT(!(nr_mfns % LPAE_ENTRIES));
> +
> +    count = nr_mfns / LPAE_ENTRIES;
> +    p = second + second_linear_offset(virt_offset);
> +    pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
> +    for ( i = 0; i < count; i++ )
> +    {
> +        write_pte(p + i, pte);
> +        pte.pt.base += 1 << LPAE_SHIFT;
> +    }
> +    flush_xen_data_tlb_local();
> +}
> +

Can you rework create_32mb_mappings to take the size of the mappings in
parameters?

>  /* Set up the xenheap: up to 1GB of contiguous, always-mapped memory. */
>  void __init setup_xenheap_mappings(unsigned long base_mfn,
>                                     unsigned long nr_mfns)
> @@ -749,6 +772,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>      unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
>      unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
>      unsigned long base_mfn;
> +    unsigned long mask;
>  #ifdef CONFIG_ARM_64
>      lpae_t *second, pte;
>      unsigned long nr_second, second_base;
> @@ -757,8 +781,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>  
>      frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
>  
> -    /* Round up to 32M boundary */
> -    frametable_size = (frametable_size + 0x1ffffff) & ~0x1ffffff;
> +    /* Round up to 2M or 32M boundary, as appropriate. */
> +    if ( frametable_size < MB(32) )
> +        mask = MB(2) - 1;
> +    else
> +        mask = MB(32) - 1;
> +    frametable_size = (frametable_size + mask) & ~mask;

You can use ROUNDUP(frametable_size, size) to avoid open-coding the mask.

Also, this code is common with ARM64. If we happen to have a board with
a frametable smaller than 32MB, you will round up to 2MB and crash later
in create_32mb_mappings because you don't support 2MB mapping for ARM64.

It might be good to support 2MB for ARM64 too.

>      base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
>  
>  #ifdef CONFIG_ARM_64
> @@ -773,7 +801,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>      }
>      create_32mb_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT);
>  #else
> -    create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START, base_mfn, frametable_size >> PAGE_SHIFT);
> +    if ( frametable_size < MB(32) )
> +        create_2mb_mappings(xen_second, FRAMETABLE_VIRT_START,
> +                            base_mfn, frametable_size >> PAGE_SHIFT);
> +    else
> +        create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START,
> +                             base_mfn, frametable_size >> PAGE_SHIFT);

Passing the size/alignment in parameter would have avoid to add this
if/else. You can use the new parameter to ASSERT the input and enable or
not the contiguous bit.

Regards,

-- 
Julien Grall