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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Dec 18, 2013 at 10:09 AM, Tim Deegan <tim@xxxxxxx> wrote:
> At 18:44 -0800 on 17 Dec (1387302252), Mukesh Rathor wrote:
>> On Tue, 17 Dec 2013 11:19:57 +0100
>> Tim Deegan <tim@xxxxxxx> wrote:
>>
>> > At 08:42 +0000 on 17 Dec (1387266152), Jan Beulich wrote:
>> > > >>> On 17.12.13 at 02:47, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> > > >>> 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@xxxxxxxxxx>
>> > >
>> ......
>> > >
>> > > 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.

I think most uses of nested virt would want a fully virtualized
environment to run on, wouldn't they?

 -George

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