Re: [PATCH v2 2/5] xen/arm32: mm: Consolidate the domheap mappings initialization

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 20 Jul 2022, at 19:44, Julien Grall <julien@xxxxxxx> wrote:
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> At the moment, the domheap mappings initialization is done separately for
> the boot CPU and secondary CPUs. The main difference is for the former
> the pages are part of Xen binary whilst for the latter they are
> dynamically allocated.
> 
> It would be good to have a single helper so it is easier to rework
> on the domheap is initialized.
> 
> For CPU0, we still need to use pre-allocated pages because the
> allocators may use domain_map_page(), so we need to have the domheap
> area ready first. But we can still delay the initialization to setup_mm().
> 
> Introduce a new helper init_domheap_mappings() that will be called
> from setup_mm() for the boot CPU and from init_secondary_pagetables()
> for secondary CPUs.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

With a small typo mentioned after which can be fixed on commit:

Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

> 
> ----
>    Changes in v2:
>        - Fix function name in the commit message
>        - Remove duplicated 'been' in the comment
> ---
> xen/arch/arm/include/asm/arm32/mm.h |  2 +
> xen/arch/arm/mm.c                   | 92 +++++++++++++++++++----------
> xen/arch/arm/setup.c                |  8 +++
> 3 files changed, 71 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/arm32/mm.h 
> b/xen/arch/arm/include/asm/arm32/mm.h
> index 6b039d9ceaa2..575373aeb985 100644
> --- a/xen/arch/arm/include/asm/arm32/mm.h
> +++ b/xen/arch/arm/include/asm/arm32/mm.h
> @@ -10,6 +10,8 @@ static inline bool arch_mfns_in_directmap(unsigned long 
> mfn, unsigned long nr)
>     return false;
> }
> 
> +bool init_domheap_mappings(unsigned int cpu);
> +
> #endif /* __ARM_ARM32_MM_H__ */
> 
> /*
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 0177bc6b34d2..9311f3530066 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -372,6 +372,58 @@ void clear_fixmap(unsigned int map)
> }
> 
> #ifdef CONFIG_DOMAIN_PAGE
> +/*
> + * Prepare the area that will be used to map domheap pages. They are
> + * mapped in 2MB chunks, so we need to allocate the page-tables up to
> + * the 2nd level.
> + *
> + * The caller should make sure the root page-table for @cpu has been
> + * allocated.
> + */
> +bool init_domheap_mappings(unsigned int cpu)
> +{
> +    unsigned int order = get_order_from_pages(DOMHEAP_SECOND_PAGES);
> +    lpae_t *root = per_cpu(xen_pgtable, cpu);
> +    unsigned int i, first_idx;
> +    lpae_t *domheap;
> +    mfn_t mfn;
> +
> +    ASSERT(root);
> +    ASSERT(!per_cpu(xen_dommap, cpu));
> +
> +    /*
> +     * The domheap for cpu0 is before the heap is initialized. So we

There is a “initialized” missing.

Cheers
Bertrand

> +     * need to use pre-allocated pages.
> +     */
> +    if ( !cpu )
> +        domheap = cpu0_dommap;
> +    else
> +        domheap = alloc_xenheap_pages(order, 0);
> +
> +    if ( !domheap )
> +        return false;
> +
> +    /* Ensure the domheap has no stray mappings */
> +    memset(domheap, 0, DOMHEAP_SECOND_PAGES * PAGE_SIZE);
> +
> +    /*
> +     * Update the first level mapping to reference the local CPUs
> +     * domheap mapping pages.
> +     */
> +    mfn = virt_to_mfn(domheap);
> +    first_idx = first_table_offset(DOMHEAP_VIRT_START);
> +    for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
> +    {
> +        lpae_t pte = mfn_to_xen_entry(mfn_add(mfn, i), MT_NORMAL);
> +        pte.pt.table = 1;
> +        write_pte(&root[first_idx + i], pte);
> +    }
> +
> +    per_cpu(xen_dommap, cpu) = domheap;
> +
> +    return true;
> +}
> +
> void *map_domain_page_global(mfn_t mfn)
> {
>     return vmap(&mfn, 1);
> @@ -633,16 +685,6 @@ void __init setup_pagetables(unsigned long 
> boot_phys_offset)
>         p[i].pt.xn = 0;
>     }
> 
> -#ifdef CONFIG_ARM_32
> -    for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
> -    {
> -        p[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)]
> -            = pte_of_xenaddr((uintptr_t)(cpu0_dommap +
> -                                         i * XEN_PT_LPAE_ENTRIES));
> -        p[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)].pt.table = 1;
> -    }
> -#endif
> -
>     /* Break up the Xen mapping into 4k pages and protect them separately. */
>     for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
>     {
> @@ -686,7 +728,6 @@ void __init setup_pagetables(unsigned long 
> boot_phys_offset)
> 
> #ifdef CONFIG_ARM_32
>     per_cpu(xen_pgtable, 0) = cpu0_pgtable;
> -    per_cpu(xen_dommap, 0) = cpu0_dommap;
> #endif
> }
> 
> @@ -719,39 +760,28 @@ int init_secondary_pagetables(int cpu)
> #else
> int init_secondary_pagetables(int cpu)
> {
> -    lpae_t *first, *domheap, pte;
> -    int i;
> +    lpae_t *first;
> 
>     first = alloc_xenheap_page(); /* root == first level on 32-bit 3-level 
> trie */
> -    domheap = 
> alloc_xenheap_pages(get_order_from_pages(DOMHEAP_SECOND_PAGES), 0);
> 
> -    if ( domheap == NULL || first == NULL )
> +    if ( !first )
>     {
> -        printk("Not enough free memory for secondary CPU%d pagetables\n", 
> cpu);
> -        free_xenheap_pages(domheap, 
> get_order_from_pages(DOMHEAP_SECOND_PAGES));
> -        free_xenheap_page(first);
> +        printk("CPU%u: Unable to allocate the first page-table\n", cpu);
>         return -ENOMEM;
>     }
> 
>     /* Initialise root pagetable from root of boot tables */
>     memcpy(first, cpu0_pgtable, PAGE_SIZE);
> +    per_cpu(xen_pgtable, cpu) = first;
> 
> -    /* Ensure the domheap has no stray mappings */
> -    memset(domheap, 0, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
> -
> -    /* Update the first level mapping to reference the local CPUs
> -     * domheap mapping pages. */
> -    for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
> +    if ( !init_domheap_mappings(cpu) )
>     {
> -        pte = mfn_to_xen_entry(virt_to_mfn(domheap + i * 
> XEN_PT_LPAE_ENTRIES),
> -                               MT_NORMAL);
> -        pte.pt.table = 1;
> -        
> write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
> +        printk("CPU%u: Unable to prepare the domheap page-tables\n", cpu);
> +        per_cpu(xen_pgtable, cpu) = NULL;
> +        free_xenheap_page(first);
> +        return -ENOMEM;
>     }
> 
> -    per_cpu(xen_pgtable, cpu) = first;
> -    per_cpu(xen_dommap, cpu) = domheap;
> -
>     clear_boot_pagetables();
> 
>     /* Set init_ttbr for this CPU coming up */
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 85ff956ec2e3..068e84b10335 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -783,6 +783,14 @@ static void __init setup_mm(void)
>     setup_frametable_mappings(ram_start, ram_end);
>     max_page = PFN_DOWN(ram_end);
> 
> +    /*
> +     * The allocators may need to use map_domain_page() (such as for
> +     * scrubbing pages). So we need to prepare the domheap area first.
> +     */
> +    if ( !init_domheap_mappings(smp_processor_id()) )
> +        panic("CPU%u: Unable to prepare the domheap page-tables\n",
> +              smp_processor_id());
> +
>     /* Add xenheap memory that was not already added to the boot allocator. */
>     init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start),
>                        mfn_to_maddr(xenheap_mfn_end));
> -- 
> 2.32.0
>