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

From: Julien Grall <julien.grall@arm.com>
To: Jan Beulich <JBeulich@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Cc: Juergen Gross <jgross@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>
Subject: Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests
Date: Tue, 2 Apr 2019 11:26:55 +0100	[thread overview]
Message-ID: <942c2ec5-69dc-634b-0935-b8713506d32e@arm.com> (raw)
In-Reply-To: <5C7E798E020000780021BB43@prv1-mh.provo.novell.com>

Hi Jan,

Sorry I was meant to answer to this earlier on.

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@suse.com>
> ---
> 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?

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...).

> 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?

Cheers,

-- 
Julien Grall

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