Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall <julien.grall@xxxxxxx>]

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