[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



Hi Jan,

On 4/2/19 5:10 PM, Jan Beulich wrote:
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()
de-allocation step aside, I am not really convinced you can reuse guest_remove_page() here. On x86, the function will not work on certain p2m types. Is it what we really want?


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.

Make sense. I will have a look at the patch.

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.