>>> On 13.05.19 at 16:35, <george.dun...@citrix.com> wrote: > On 3/5/19 1:28 PM, Jan Beulich wrote: >> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously >> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having >> originally introduced it (d818f3cb7c ["hvm: Use main memory for video >> memory"]) and the one then purging it again (78c3097e4f ["Remove unused >> XENMEM_remove_from_physmap"]) make clear that this operation is intended >> for use on HVM (i.e. translated) guests only. Restrict it at least as >> much, because for PV guests documentation (in the public header) does >> not even match the implementation: It talks about GPFN as input, but >> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands >> back the value passed in). >> >> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up >> directly into top level hypercall handling, and clarify things in the >> public header accordingly. >> >> Take the liberty and also replace a pointless use of "current" with a >> more efficient use of an existing local variable (or function parameter >> to be precise). >> >> Signed-off-by: Jan Beulich <jbeul...@suse.com> > > Looks good, sorry for the delay: > > Reviewed-by: George Dunlap <george.dun...@citrix.com>
Thanks. > A couple of comments: > >> --- >> TBD: Is using P2M_ALLOC here really appropriate? It means e.g. >> pointlessly populating a PoD slot just to unpopulate it again right >> away, with the page then free floating, i.e. no longer available >> for use to replace another PoD slot, and (afaict) no longer >> accessible by the guest in any way. > > Looks like the P2M_ALLOC was introduced in c/s 06e7bfed206. I can't > immediately see any reason to do the allocation -- I think it just must > have been C&P without much thought given as to what was going to happen > next. The question is: If we remove it, what is the -ENOENT condition going to be? >> --- a/xen/arch/x86/mm.c >> +++ b/xen/arch/x86/mm.c >> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one( >> mfn_t mfn = INVALID_MFN; >> p2m_type_t p2mt; >> >> - if ( !paging_mode_translate(d) ) >> - return -EACCES; >> - >> switch ( space ) >> { >> case XENMAPSPACE_shared_info: >> --- a/xen/common/memory.c >> +++ b/xen/common/memory.c >> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain >> long rc = 0; >> union xen_add_to_physmap_batch_extra extra; >> >> + ASSERT(paging_mode_translate(d)); > > So, just trying to think through the principles behind these two. We > know that this is never going to be called w/o first calling > xatp_permission_check(); if we assume that such a change will be tested > (i.e., that something with paging_mode_translate() will call this > hypercall before a release), then a single ASSERT() should be enough to > make sure that both functions are updated properly? Yes, that's my expectation. > I guess that's good enough. (It's hard not to start to get paranoid > when you ask yourself these sorts of questions.) Good. (Indeed.) Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel