Re: [PATCH v3] altp2m: Allow the hostp2m to be shared

From: George Dunlap <george.dunlap@citrix.com>
To: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Tim Deegan <tim@xen.org>, Jun Nakajima <jun.nakajima@intel.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v3] altp2m: Allow the hostp2m to be shared
Date: Wed, 25 May 2016 12:27:43 +0100	[thread overview]
Message-ID: <CAFLBxZbTHopYU4k4vOMs1V8ad7bsRk7cQBBF9C9drA2_rSMHJg@mail.gmail.com> (raw)
In-Reply-To: <1461951776-25956-1-git-send-email-tamas@tklengyel.com>

On Fri, Apr 29, 2016 at 6:42 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> Don't propagate altp2m changes from ept_set_entry for memshare as memshare
> already has the lock. We call altp2m propagate changes once memshare
> successfully finishes. Allow the hostp2m entries to be of type
> p2m_ram_shared when applying mem_access. Also, do not trigger PoD for hostp2m
> when setting altp2m mem_access to be in-line with non-altp2m mem_access path.

Hey Tamas,

Sorry for the long delay in getting back to you on this.

So the main issue here (correct me if I'm wrong) is the locking
discipline: namely, men_sharing_share_pages():
- Grabs the hostp2m lock
- Grabs the appropriate domain memsharing locks
- Calls set_shared_p2m_entry(), which ends up calling ept_set_entry(),
which (when altp2m is active) grabs the altp2mlist and altp2m locks.

This causes an ASSERT(), since the altp2mlist lock is ahead of the
memsharing locks in the list.

But having taken a closer look at the code, I'm not sure the change is
quite correct.  Please correct me if I've misread something:

mem_sharing_share_pages() is passed two <domain,gfn> pairs -- the
<sd,sgfn> (which I assume stands for "shared gfn") and <cd,cgfn>
(which I assume stands for "copy"); and it
1) Looks up smfn and cmfn, which back sgfn and cmfn respectively
2) Looks up cmfn, which backs cgfn then replaces all gfn entries which
point to cmfn with smfn (updating accounting as appropriate)

But this change will only call p2m_altp2m_propagate_change() for the
original cgfn -- any other gfns which are backed by cmfn will not have
the corresponding altp2m entries propagated properly.

This sort of mistake is easy to make, which is why I think we should
try to always update the altp2ms in ept_set_entry() if we can, to
minimize the opportunity for making this sort of mistake.

Is there ever a reason to grab the altp2m lock and *then* grab the
sharing lock?  Could we just move the sharing lock up between the p2m
lock and the altp2mlist lock instead?

 -George

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