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

[Wei Liu <wl@xxxxxxx>]

On Mon, Jan 27, 2020 at 03:43:12PM +0100, Jan Beulich wrote:
> On 27.01.2020 15:33, Xia, Hongyan wrote:
> > On Thu, 2020-01-16 at 13:04 +0100, Jan Beulich wrote:
> >> ...
> >>
> >>> +void free_xen_pagetable(void *v)
> >>> +{
> >>> +    mfn_t mfn = v ? virt_to_mfn(v) : INVALID_MFN;
> >>> +
> >>> +    if ( system_state != SYS_STATE_early_boot )
> >>> +        free_xen_pagetable_new(mfn);
> >>
> >> The condition is (partly) redundant with what
> >> free_xen_pagetable_new() does. Therefore I'd like to ask that
> >> either the if() be dropped here, or be completed by also
> >> checking v to be non-NULL, at which point this would likely
> >> become just
> >>
> >> void free_xen_pagetable(void *v)
> >> {
> >>     if ( v && system_state != SYS_STATE_early_boot )
> >>         free_xen_pagetable_new(virt_to_mfn(v));
> >> }
> > 
> > You are right. Will change in the next revision.
> > 
> >>> +/* v can point to an entry within a table or be NULL */
> >>> +void unmap_xen_pagetable(const void *v)
> >>
> >> Why "entry" in the comment?
> > 
> > I think the comment originally meant pointing to the entry in its
> > parent table, which then meant pointing to this table. It's a bit
> > confusing. Will reword.
> > 
> > Reflecting back to your comment in v3 about whether the new Xen page
> > table mapping APIs (map/unmap_xen_pagetable) are really necessary, I
> > agree in the end they will just do the same thing as
> > map/unmap_domain_page, although one thing is that the latter suggests
> > it is trying to map a `domain' page, whose definition probably does not
> > include Xen page tables, so the name could be a bit confusing (well, we
> > could argue that Xen page tables are just idle `domain' pages so the
> > name still holds). If we are happy with using map_domain_page on Xen
> > PTE tables then I am okay with dropping the new mapping APIs and only
> > include the new alloc APIs.
> 
> Anyone on the To: list - opinions?

I'm not too fussed about this.

I think eventually we will just change map_domain_page to map_page and
use that directly. The intermediate steps aren't terribly interesting to
me.

Wei.

> 
> Thanks, Jan

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