Re: [PATCH v6 07/15] x86_64/mm: switch to new APIs in paging_init

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.04.2020 16:08, Hongyan Xia wrote:
> @@ -493,22 +494,28 @@ void __init paging_init(void)
>          if ( !(l4e_get_flags(idle_pg_table[l4_table_offset(va)]) &
>                _PAGE_PRESENT) )
>          {
> -            l3_pgentry_t *pl3t = alloc_xen_pagetable();
> +            l3_pgentry_t *pl3t;
> +            mfn_t l3mfn = alloc_xen_pagetable_new();
>  
> -            if ( !pl3t )
> +            if ( mfn_eq(l3mfn, INVALID_MFN) )
>                  goto nomem;
> +
> +            pl3t = map_domain_page(l3mfn);
>              clear_page(pl3t);
>              l4e_write(&idle_pg_table[l4_table_offset(va)],
> -                      l4e_from_paddr(__pa(pl3t), __PAGE_HYPERVISOR_RW));
> +                      l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW));
> +            unmap_domain_page(pl3t);

This can be moved up, and once it is you'll notice that you're
open-coding clear_domain_page(). I wonder whether I didn't spot
the same in other patches of this series.

Besides the previously raised point of possibly having an
allocation function that returns a mapping of the page right
away (not needed here) - are there many cases where allocation
of a new page table isn't accompanied by clearing the page? If
not, should the function perhaps do so (and then, once it has
a mapping anyway, it would be even more so natural to return
it for users wanting a mapping anyway)?

> @@ -662,6 +677,8 @@ void __init paging_init(void)
>      return;
>  
>   nomem:
> +    UNMAP_DOMAIN_PAGE(l2_ro_mpt);
> +    UNMAP_DOMAIN_PAGE(l3_ro_mpt);
>      panic("Not enough memory for m2p table\n");
>  }

I don't think this is a very useful addition.

Jan