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

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

[Chris (Christopher) Brand]

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.

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
@@ -656,6 +656,29 @@ static void __init create_32mb_mappings(lpae_t *second,
 }
 
 #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();
+}
+
 /* 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;
     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);
 #endif
 
     memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
-- 
1.9.1

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

[Julien Grall]

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

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

[Chris (Christopher) Brand]

Hi Julien,

Thanks for the review.

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

Fair question. :-) All I can say is that I don't see any noticeable difference on my
system. Are there performance tests that you suggest I run ? Also, note that
the new code is only executed if you specify a previously-invalid value for
xenheap_megabytes on the command-line, so it won't affect any existing
systems. Is it worth adding a sentence mentioning performance to the
documentation, do you think ?

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

Yeah, I have a version like that here somewhere, but it wasn't as clean as I'd hoped.
I'll re-visit it and send as v2.

>> -    /* 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.

Will do.

> 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.

Whoops! Yes, I've messed the ARM64 path up at some point while cleaning
up the code. Will fix.

Thanks,

Chris

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

[Julien Grall]

On 07/08/15 18:29, Chris (Christopher) Brand wrote:
> Hi Julien,
> 
> Thanks for the review.
> 
>> OOI, you win 30MB of RAM but how does this affect the performance?
> 
> Fair question. :-) All I can say is that I don't see any noticeable difference on my
> system. Are there performance tests that you suggest I run ?

I'm not sure. Ian, Stefano do you have any suggestion?

Regards,

-- 
Julien Grall

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

[Chris (Christopher) Brand]

>> OOI, you win 30MB of RAM but how does this affect the performance?
>
> Fair question. :-) All I can say is that I don't see any noticeable difference on my system.
> Are there performance tests that you suggest I run ?
> Also, note that the new code is
> only executed if you specify a previously-invalid value for xenheap_megabytes on the
> command-line, so it won't affect any existing systems.

This sentence is a lie, sorry. Please ignore it. :-)

> Is it worth adding a sentence
> mentioning performance to the documentation, do you think ?

Chris