Re: [RFC PATCH] PVH: cleanup of p2m upon p2m destroy

[Tim Deegan <tim@xen.org>]

At 18:44 -0800 on 17 Dec (1387302252), Mukesh Rathor wrote:
> On Tue, 17 Dec 2013 11:19:57 +0100
> Tim Deegan <tim@xen.org> wrote:
> 
> > At 08:42 +0000 on 17 Dec (1387266152), Jan Beulich wrote:
> > > >>> On 17.12.13 at 02:47, Mukesh Rathor <mukesh.rathor@oracle.com>
> > > >>> wrote:
> > > > When a controlling domain is destroyed, any p2m_is_foreign pages
> > > > must release the refcnt gotten when the page was added to the p2m.
> > > > 
> > > > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > > 
> ......
> > > 
> > > So it looks like you copied one of the two obvious bugs from
> > > relinquish_shared_pages() _and_ deferred the preemption point
> > > by quite a bit - 10,000 pages is quite a lot, the 512 used there
> > > seems much more reasonable.
> > > 
> > > As to the copied bug: Should hypercall_preempt_check() return
> > > false, you'd never again try to preempt.
> > 
> > Further, looping from 0 to max_mapped_pfn, even preemptibly, is not a
> > good way to do this: the guest can set its own max_mapped_pfn, and we
> > don't want Xen to spend its time counting to 2^63.
> 
> Ok, some p2m code is setting a bad precedent. 

Yes, indeed. :(  As I pointed out before, and has been the subject of
several XSAs.

> An alternative might be to just create a link list then and walk it. In
> general, foreign mappings should be very small, so the overhead of
> 16 bytes per page for the link list might not be too bad. I will code
> it if there is no disagreement from any maintainer... everyone has 
> different ideas :)...

I think it would be best to walk the p2m trie (i.e. bounded by amount
of RAM, rather than max GFN) and do it preemptably.  I'll look into
something like that for the mem_sharing loop today, and foreign
mapping code can reuse it.

> Or better, if I add a count of foreign mappings and hang it off the 
> p2m_domain, then this code would only execute in case of control
> domain going away...

That's a good idea as an optimisation, but it doesn't prevent the bad
case: this teardown has to execute for any domain that has a p2m and
at least one foreign mapping.  Since that domain chooses the GFN of
the mapping, it can still cause the hypervisor to to a lot of work.

> > Further further, it occurs to me that the refcounting change might
> > interact badly with nested EPT, which creates and destroys p2m tables
> > quite regularly.
> 
> nested ept is not supported on pvh.

What, never?  Or as a 'fimxe' for later?  We should avoid painting
ourselves into any more corners here if we can.

Tim.