On Thu, 2020-04-30 at 17:15 +0200, Jan Beulich wrote:
> On 24.04.2020 16:09, Hongyan Xia wrote:
> > From: Wei Liu <wei.l...@citrix.com>
>
> Nit: Why the emphasis on pl*e in the title? Is there anything left
> unconverted in the function? IOW how about "switch clone_mapping()
> to new page table APIs"?
The title seems stale. Will fix.
> ...
> > @@ -724,48 +724,61 @@ static int clone_mapping(const void *ptr,
> > root_pgentry_t *rpt)
> > }
> > }
> >
> > + UNMAP_DOMAIN_PAGE(pl1e);
> > + UNMAP_DOMAIN_PAGE(pl2e);
> > + UNMAP_DOMAIN_PAGE(pl3e);
> > +
> > if ( !(root_get_flags(rpt[root_table_offset(linear)]) &
> > _PAGE_PRESENT) )
> > {
> > - pl3e = alloc_xen_pagetable();
> > - if ( !pl3e )
> > + mfn_t l3mfn = alloc_xen_pagetable_new();
> > +
> > + if ( mfn_eq(l3mfn, INVALID_MFN) )
> > goto out;
> > +
> > + pl3e = map_domain_page(l3mfn);
>
> Seeing this recur (from other patches) I wonder whether we wouldn't
> better make map_domain_page() accept INVALID_MFN and return NULL in
> this case. In cases like the one here it would eliminate the need
> for several local variables. Of course the downside of this is that
> then we'll have to start checking map_domain_page()'s return value.
> A middle ground could be to have
>
> void *alloc_mapped_pagetable(mfn_t *mfn);
>
> allowing to pass in NULL if the MFN is of no interest.
I would say that when the caller requires a new Xen page table
allocation, almost all the time both the mfn and the virt are needed
(on top of my head I cannot think of a case where we pass in NULL, you
almost always need the mfn to write new page table entries), so I think
the benefit of this is just compressing two calls into one, which I am
not quite sure is worth it.
> > @@ -781,6 +794,9 @@ static int clone_mapping(const void *ptr,
> > root_pgentry_t *rpt)
> >
> > rc = 0;
> > out:
> > + UNMAP_DOMAIN_PAGE(pl1e);
> > + UNMAP_DOMAIN_PAGE(pl2e);
> > + UNMAP_DOMAIN_PAGE(pl3e);
> > return rc;
> > }
>
> I don't think the writing of NULL into the variables is necessary
> here. And if the needed if()-s are of concern, then perhaps we
> should consider making unmap_domain_page() finally accept NULL as
> input?
I usually don't have a problem with this because a sane compiler would
definitely remove the unnecessary clearing, so I would use the macro
version as much as possible. I am okay with moving the NULL check into
unmap() itself, but note that this also needs changes on Arm side.
Hongyan
