Re: [PATCH 2/2] memory: don't hand MFN info to translated guests

[Julien Grall <julien.grall@arm.com>]

On 19/06/17 15:39, Tamas K Lengyel wrote:
> On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi,
>>
>>
>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>>
>>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@gmail.com> wrote:
>>>>
>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.cooper3@citrix.com>
>>>> wrote:
>>>>>
>>>>> On 04/04/17 14:14, Jan Beulich wrote:
>>>>>>
>>>>>> We shouldn't hand MFN info back from increase-reservation for
>>>>>> translated domains, just like we don't for populate-physmap and
>>>>>> memory-exchange. For full symmetry also check for a NULL guest handle
>>>>>> in populate_physmap() (but note this makes no sense in
>>>>>> memory_exchange(), as there the array is also an input).
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>>
>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>
>>>>
>>>> Unfortunately I just had time to do testing with this change and I
>>>> have to report that introduces a critical regression for my tools.
>>>> With this change in-place performing increase_reservation on a target
>>>> domain no longer reports the guest frame number for external tools,
>>>> thus completely breaking advanced use-cases that require this
>>>> information to be able to do altp2m gfn remapping. This is a critical
>>>> step in being able to introduce shadow-pages that are used to hide
>>>> breakpoints and other memory modifications from the guest.
>>>
>>>
>>> While I can see your point, I'm afraid that's not how the
>>> interface was meant to be used. The mere fact that
>>> populate-physmap and memory-exchange didn't return the
>>> MFN(s) suggests to me that you already need to have a way
>>> to deal with having to find out another way. Or are you
>>> suggesting you rely on guests not using these interfaces?
>>>
>>> As to a solution, I could possibly see us relax the change to
>>> return the MFN(s) when the current and subject domains differ,
>>> or even check paging mode of the caller domain instead of the
>>> subject one (which would mean PVH Dom0 still wouldn't get to
>>> see them). But if we do, imo we should do this consistently for
>>> all three operations, rather than just for increase-reservation.
>>>
>>>> If at all possible, I would like to request this change not to be part
>>>> of the 4.9 release.
>>>
>>>
>>> Hmm, it's been there for all of the RCs, so I'm not really happy
>>> to consider the option of reverting at this point in time. But
>>> Julien will have the final say anyway.
>>
>>
>> I am a bit confuse with the description of the problem. I understood "guest
>> frame number" as GFN. But AFAICT, this hypercall was returning MFN even for
>> HVM guests. So how this change is breaking altp2m remapping?
>
> For HVM guests this hypercall returns a GFN that can subsequently be
> populated into the guest physmap:
>
> xc_domain_increase_reservation_exact(xch, domid, 1, 0, 0, &new_gfn);
> xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, &new_gfn);

I am sorry, I can't see how this can return a GFN for the HVM. Looking 
at the implementation of increase_reservation in Xen:

mfn = page_to_mfn(page);
if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1)) )
   goto out;

This is an MFN and not a GFN. Except the strict check before, the code 
has not change for a while.

AFAICT, the purpose of increase_reservation is not to allocate a new 
GFN, it will just allocate the host memory for it. At least on ARM we 
have nothing to say "this GFN region is free". I would be surprised that 
such things exists on x86.

Cheers,

-- 
Julien Grall

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