[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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.

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.