Re: [PATCH RFCv2 10/15] xen/arm: mm: Allocate xen page tables in domheap rather than xenheap

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sun, 25 Apr 2021, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> xen_{un,}map_table() already uses the helper to map/unmap pages
> on-demand (note this is currently a NOP on arm64). So switching to
> domheap don't have any disavantage.
> 
> But this as the benefit:
>     - to keep the page tables unmapped if an arch decided to do so
>     - reduce xenheap use on arm32 which can be pretty small
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Thanks for the patch. It looks OK but let me ask a couple of questions
to clarify my doubts.

This change should have no impact to arm64, right?

For arm32, I wonder why we were using map_domain_page before in
xen_map_table: it wasn't necessary, was it? In fact, one could even say
that it was wrong?


> ---
>     Changes in v2:
>         - New patch
> ---
>  xen/arch/arm/mm.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 19ecf73542ce..ae5a07ea956b 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -969,21 +969,6 @@ void *ioremap(paddr_t pa, size_t len)
>      return ioremap_attr(pa, len, PAGE_HYPERVISOR_NOCACHE);
>  }
>  
> -static int create_xen_table(lpae_t *entry)
> -{
> -    void *p;
> -    lpae_t pte;
> -
> -    p = alloc_xenheap_page();
> -    if ( p == NULL )
> -        return -ENOMEM;
> -    clear_page(p);
> -    pte = mfn_to_xen_entry(virt_to_mfn(p), MT_NORMAL);
> -    pte.pt.table = 1;
> -    write_pte(entry, pte);
> -    return 0;
> -}
> -
>  static lpae_t *xen_map_table(mfn_t mfn)
>  {
>      /*
> @@ -1024,6 +1009,27 @@ static void xen_unmap_table(const lpae_t *table)
>      unmap_domain_page(table);
>  }
>  
> +static int create_xen_table(lpae_t *entry)
> +{
> +    struct page_info *pg;
> +    void *p;
> +    lpae_t pte;
> +
> +    pg = alloc_domheap_page(NULL, 0);
> +    if ( pg == NULL )
> +        return -ENOMEM;
> +
> +    p = xen_map_table(page_to_mfn(pg));
> +    clear_page(p);
> +    xen_unmap_table(p);
> +
> +    pte = mfn_to_xen_entry(page_to_mfn(pg), MT_NORMAL);
> +    pte.pt.table = 1;
> +    write_pte(entry, pte);
> +
> +    return 0;
> +}
> +
>  #define XEN_TABLE_MAP_FAILED 0
>  #define XEN_TABLE_SUPER_PAGE 1
>  #define XEN_TABLE_NORMAL_PAGE 2
> -- 
> 2.17.1
>