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

[Julien Grall <julien.grall@xxxxxxx>]

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