Re: [PATCH v6 12/15] x86/smpboot: switch pl*e to use new APIs in clone_mapping

[Hongyan Xia <hx242@xen.org>]

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.liu2@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