Re: [Xen-devel] [PATCH v3 2/9] x86: introduce a new set of APIs to manage Xen page tables

[Wei Liu <wl@xxxxxxx>]

On Fri, Nov 15, 2019 at 04:23:30PM +0100, Jan Beulich wrote:
> On 02.10.2019 19:16, Hongyan Xia wrote:
> > @@ -4847,22 +4848,50 @@ int mmcfg_intercept_write(
> >  }
> >  
> >  void *alloc_xen_pagetable(void)
> > +{
> > +    mfn_t mfn;
> > +
> > +    mfn = alloc_xen_pagetable_new();
> > +    ASSERT(!mfn_eq(mfn, INVALID_MFN));
> > +
> > +    return map_xen_pagetable_new(mfn);
> > +}
> > +
> > +void free_xen_pagetable(void *v)
> > +{
> > +    if ( system_state != SYS_STATE_early_boot )
> > +        free_xen_pagetable_new(virt_to_mfn(v));
> > +}
> > +
> > +mfn_t alloc_xen_pagetable_new(void)
> >  {
> >      if ( system_state != SYS_STATE_early_boot )
> >      {
> >          void *ptr = alloc_xenheap_page();
> >  
> >          BUG_ON(!hardware_domain && !ptr);
> > -        return ptr;
> > +        return virt_to_mfn(ptr);
> >      }
> >  
> > -    return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1)));
> > +    return alloc_boot_pages(1, 1);
> >  }
> >  
> > -void free_xen_pagetable(void *v)
> > +void *map_xen_pagetable_new(mfn_t mfn)
> 
> There's no need for the map/unmap functions to have a _new
> suffix, is there?
> 

It is more consistent.

> >  {
> > -    if ( system_state != SYS_STATE_early_boot )
> > -        free_xenheap_page(v);
> > +    return mfn_to_virt(mfn_x(mfn));
> > +}
> > +
[...]
> 
> > +{
> > +    /* XXX still using xenheap page, no need to do anything.  */
> 
> I wonder if it wouldn't be a good idea to at least have some
> leak detection here temporarily, such that we have a chance of
> noticing paths not properly doing the necessary unmapping.
> 
> The again a question is why you introduce such a map/unmap pair
> in the first place. This is going to be a thin wrapper around
> {,un}map_domain_page() in the end anyway, and hence callers
> could as well be switched to calling those function directly,
> as they're no-ops on Xen heap pages.


All roads lead to Rome, but some roads are easier than others.  Having a
set of APIs to deal with page tables make the code easier to follow IMO.

And we can potentially do more stuff in this function, for example, make
the unmap function test if the page is really a page table to avoid
misuse; or like you said, have some leak detection check there.

Also, please consider there are dozens of patches that are built on top
of this set of new APIs.  Having to rewrite them just for mechanical
changes is not fun for Hongyan. I would suggest we be more pragmatic
here.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel