Re: [Xen-devel] [PATCH v2] xen: arm: Support <32MB frametables

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Chris,

Sorry for the late reply. The code looks ok, see comment below.

On 07/08/15 21:40, 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.

It would be interesting to add a word about performance impact if it's
relevant.

Although based on the spec (B3.8.4  ARM DDI 0406C.b)the processor is not
force to cache TLB entries in this way. So maybe it doesn't change
anything on some hardware.

> 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@xxxxxxxxxxxx>
> ---
> Changed in v2: merged create_32mb_mappings() and create_2mb_mappings()
> into create_mappings(). A side-effect is to fix the bug in v1 for
> ARM64 systems with <4GB RAM.
> 
>  xen/arch/arm/mm.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index ae0f34c3c480..fd64fbfdfb93 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -628,25 +628,31 @@ void __cpuinit mmu_init_secondary_cpu(void)
>  }
>  
>  /* Create Xen's mappings of memory.
> - * Base and virt must be 32MB aligned and size a multiple of 32MB.
> + * Mapping_size must be either 2MB or 32MB.

I would have generalize the function to support any mapping size. But
I'm also fine with the current solution.

> + * Base and virt must be mapping_size aligned.
> + * Size must be a multiple of mapping_size.
>   * second must be a contiguous set of second level page tables
>   * covering the region starting at virt_offset. */
> -static void __init create_32mb_mappings(lpae_t *second,
> -                                        unsigned long virt_offset,
> -                                        unsigned long base_mfn,
> -                                        unsigned long nr_mfns)
> +static void __init create_mappings(lpae_t *second,
> +                                   unsigned long virt_offset,
> +                                   unsigned long base_mfn,
> +                                   unsigned long nr_mfns,
> +                                   unsigned int mapping_size)
>  {
>      unsigned long i, count;
> +    const unsigned long granularity = mapping_size >> PAGE_SHIFT;

This variable is only used in the ASSERT. On non-debug build the
compiler will throw an error because the variable will be unused.

>      lpae_t pte, *p;
>  
> -    ASSERT(!((virt_offset >> PAGE_SHIFT) % (16 * LPAE_ENTRIES)));
> -    ASSERT(!(base_mfn % (16 * LPAE_ENTRIES)));
> -    ASSERT(!(nr_mfns % (16 * LPAE_ENTRIES)));
> +    ASSERT((mapping_size == MB(2)) || (mapping_size == MB(32)));
> +    ASSERT(!((virt_offset >> PAGE_SHIFT) % granularity));
> +    ASSERT(!(base_mfn % granularity));
> +    ASSERT(!(nr_mfns % granularity));
>  
>      count = nr_mfns / LPAE_ENTRIES;
>      p = second + second_linear_offset(virt_offset);
>      pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
> -    pte.pt.contig = 1;  /* These maps are in 16-entry contiguous chunks. */
> +    if ( mapping_size == MB(32) )
> +        pte.pt.contig = 1;  /* These maps are in 16-entry contiguous chunks. 
> */

mfn_to_xen_entry never set the contig bit to 0 (or anything else). So
the value will be undefined for MB(2) mapping.

It looks like to me that mfn_to_xen_entry should always set the contig
bit to 0. I'm not sure why we didn't see any issue until now. Can you
please send a patch for this?

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel