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

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Mon, Jun 19, 2017 at 9:34 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> On 19/06/17 15:57, Tamas K Lengyel wrote:
>>
>> On Mon, Jun 19, 2017 at 8:52 AM, Julien Grall <julien.grall@xxxxxxx>
>> wrote:
>>>
>>>
>>>
>>> On 19/06/17 15:39, Tamas K Lengyel wrote:
>>>>
>>>>
>>>> On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall <julien.grall@xxxxxxx>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 18.06.17 at 21:19, <tamas.k.lengyel@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper
>>>>>>> <andrew.cooper3@xxxxxxxxxx>
>>>>>>> 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@xxxxxxxx>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>
>>
>> It returns memory that can be mapped into the guest physmap
>> subsequently. So I have been referring to it as a GFN that is not
>> mapped into the physmap - similar to the magic ring pages when they
>> are in use.
>
>
> Reading the implementation, roughly:
>
> * increase_reservation will only allocate host memory and return the
> corresponding MFN
> * populate_physmap will allocate host memory and map to a specific address
>
> So by calling both, you will effectively allocate twice memory and never be
> able to free the memory allocated by increase_reservation until the guest is
> destroyed. This will *never* allocate the corresponding GFN and I think is
> just working by luck in your case.

Ough, yes, you are correct.

After digging into the implementation of populate_physmap more closely
it indeed seems like it was pure luck that my use of it was working
properly. My understanding was the memory allocated by
increase_reservation will be used as a GFN in the guest. This appears
not to be so, it just returns a newly allocated MFN. When called with
populate_physmap that MFN was treated as a GFN to be mapped into the
guest and as you say, another MFN was getting allocated for it. So the
lucky part has been that the MFN returned by increase_reservation has
always been higher then the maximum GFN used by the guests. I had been
freeing the MFN that was returned via increase_reservation by calling
decrease_reservation. However, the page allocated during
populate_physmap is only freed during domain shutdown.

The method I found to work is getting the maximum_gpfn from the guest
and then calling populate_physmap with ++max_gpfn. The only problem
then is that I don't see a way to "unpopulate" the page from the
domain and free the corresponding mfn while the domain is running. Is
that currently possible to do?

Thanks,
Tamas

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