[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
>>> On 02.04.19 at 12:26, <julien.grall@xxxxxxx> wrote: > On 05/03/2019 13:28, 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 <jbeulich@xxxxxxxx> >> --- >> TBD: It could be further restricted, disallowing its use by a HVM guest >> on itself. > > By HVM guest, do you refer to any auto-translated guest? Yes - sorry for using an x86 term. > The interface XENME_remove_from_physmap is used by Arm to remove foreign > mappings from its p2m. There are potentially other space with similar case > (e.g grant-table...). Oh, I see - this option goes away then. >> 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. >> TBD: Is using guest_physmap_remove_page() here really appropriate? It >> means that e.g. MMIO pages wouldn't be removed. Going through >> guest_remove_page() (while skipping the de-allocation step) would >> seem more appropriate to me, which would address the P2M_ALLOC >> aspect above as well. > > How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO > pages? Well, there's XENMAPSPACE_dev_mmio which xatp handles. But perhaps the MMIO example is more confusing than helpful. The question really just is whether guest_remove_page() shouldn't be used here instead of guest_physmap_remove_page(). But of course - first of all I'd like to get acks (or feedback what to change) on the actual patch here. The further points would all, if anything, result in independent patches. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |