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

["Xia, Hongyan" <hongyxia@xxxxxxxxxx>]

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.

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