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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
> On 4/2/19 5:10 PM, Jan Beulich wrote:
>>>>> On 02.04.19 at 12:26, <julien.grall@arm.com> 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@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?
>> 
>> 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?

Hmm, I'm confused. Afaics the only two types it refuses a request
for are p2m_invalid and p2m_mmio_dm. These represent cases
where there's no p2m entry anyway, i.e. nothing to remove. Am
I perhaps overlooking something you see?

Or are you referring to the mfn_invalid() check (which isn't x86-
specific)? This ought to be covered by the p2m_is_paging() and
p2m_mmio_direct special cases a few lines up from there. Other
cases with invalid MFNs would indeed represent an error condition
imo.

In the end it's actually quite the opposite that I'm thinking: For
the caller it shouldn't, for example, matter whether the
requested page was paged out. We wouldn't even call
guest_physmap_remove_page() in that case, while
guest_remove_page() would take care of it.

Jan



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