On Wed, 2020-05-20 at 11:46 +0200, Jan Beulich wrote:
> 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)?
I grepped through all alloc_xen_pagetable(). Except the page shattering
logic in x86/mm.c where the whole page table page is written
immediately, all other call sites clear the page right away, so it is
useful to have a helper that clears it for you. I also looked at the
use of VA and MFN from the call. MFN is almost always needed while VA
is not, and if we bundle clearing into the alloc() itself, a lot of
call sites don't even need the VA.
Similar to what you suggested before, we can do:
void* alloc_map_clear_xen_pagetable(mfn_t* mfn)
which needs to be paired with an unmap call, of course.
> > @@ -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.
I was trying to avoid further mapping leaks in the panic path, but it
does not look like panic() does mappings, so these can be removed.
Hongyan
