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

From: Wei Liu <wl@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: "wl@xen.org" <wl@xen.org>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
	"Grall, Julien" <jgrall@amazon.com>,
	"Xia, Hongyan" <hongyxia@amazon.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"roger.pau@citrix.com" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v5 2/7] x86: introduce a new set of APIs to manage Xen page tables
Date: Mon, 27 Jan 2020 17:51:37 +0000	[thread overview]
Message-ID: <20200127175137.vtsd5d3qoik2qwjo@debian> (raw)
In-Reply-To: <1d38a17c-9b6b-a423-1326-33e24c583393@suse.com>

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel