From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared Date: Fri, 15 Jul 2016 08:59:10 -0600 Message-ID: References: <1464234910-7321-1-git-send-email-tamas@tklengyel.com> <5746D27F.5050705@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2551889732114005003==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bO4aB-0008IJ-D5 for xen-devel@lists.xenproject.org; Fri, 15 Jul 2016 14:59:15 +0000 Received: by mail-wm0-f53.google.com with SMTP id i5so33632874wmg.0 for ; Fri, 15 Jul 2016 07:59:12 -0700 (PDT) Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com. [74.125.82.51]) by smtp.gmail.com with ESMTPSA id b200sm6369209wmb.9.2016.07.15.07.59.11 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 15 Jul 2016 07:59:11 -0700 (PDT) Received: by mail-wm0-f51.google.com with SMTP id o80so33719289wme.1 for ; Fri, 15 Jul 2016 07:59:11 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: George Dunlap Cc: Andrew Cooper , Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org --===============2551889732114005003== Content-Type: multipart/alternative; boundary=001a114b70226f656f0537addcdc --001a114b70226f656f0537addcdc Content-Type: text/plain; charset=UTF-8 On Jul 15, 2016 02:57, "George Dunlap" wrote: > > On Thu, May 26, 2016 at 5:17 PM, Tamas K Lengyel wrote: > > > > On May 26, 2016 04:40, "George Dunlap" wrote: > >> > >> On 26/05/16 04:55, Tamas K Lengyel wrote: > >> > Move sharing locks above altp2m to avoid locking order violation. Allow > >> > applying altp2m mem_access settings when the hostp2m entries are shared. > >> > Also, do not trigger PoD for hostp2m when setting altp2m mem_access to > >> > be > >> > in-line with non-altp2m mem_access path. Also allow gfn remapping with > >> > p2m_ram_shared type gfns in altp2m views. > >> > > >> > When using mem_sharing in combination with altp2m, unsharing events > >> > overwrite > >> > altp2m entries with that of the hostp2m (both remapped entries and > >> > mem_access > >> > settings). User should take precaution to not share pages where this > >> > behavior > >> > is undesired. > >> > >> I'm afraid this is not acceptable. How could this ever be even remotely > >> usable? If this is a necessary side effect of sharing, then I think the > >> original functionality, of un-sharing when setting an altp2m entry is > >> the only option (and not allowing sharing to happen when an altp2m is > >> present for a particular gfn). > >> > >> Hmm, but actually this also brings up another tricky point: In an altp2m > >> you can change the mfn which backs a gfn. This would need to be handled > >> properly in the reverse map, which it doesn't look like it is at the > >> moment. > >> > >> On the whole, I think if you're going to allow a single gfn to be > >> simultaneously shared and allow an altp2m for it, you're going to need > >> to do a lot more work. > >> > >> (Sorry for not catching a lot of this before...) > >> > > > > Well this patch resolves the locking order violation and allows the > > xen-access tool's altp2m tests to pass, so it does improve on the current > > situation which is a hypervisor crash. To help with the override issue the > > user can apply W mem_access permission on the shared hostp2m entries. That > > way they get notification through vm_event of the event that leads to > > unsharing and can then reapply the altp2m changes. So IMHO this patch is > > already quite workable and while it requires more setup from the userside, > > the VMM side is OK with this change. > > So correct me if I'm wrong here. > > The altp2m stuff was primarily designed for guest operating systems to > be able to make alternate "views" of their own P2M. When enabled, > extra p2ms are allocated which allow a VM to remap a gfn to point to > the gfn of an mfn in its own address space. > > For example, suppose domain 1 host p2m gfns A, B, and C point to mA, > mB, and mC respectively. The guest can create an altp2m a1 such that > gfns O, P, and Q also point to mA, mB, and mC. The guest can also > register to receive #VE for violations of an altp2m which are not > violations under the hostp2m. > > mem sharing allows a process in domain 0 (or some other privileged > domain) to nominate two gfns in different domains to be shared; say, > domain 1 gfn A and domain 2 gfn X (d1gA and d2gX, respectively). This > works by first "nominating" the respective gfns, then sharing them. > "Nominating" a gfn tells the sharing infrastructure to start tracking > mappings of that page in a reverse map; after nomination mA's page > structure will point to d1gA, and mX's page structure will point to > d2gX. At that point they can be "shared", at which point (for > example) both d1gA and d2gX will point to mA, and mA's reverse map > will contain d1gA and d2gX. > > However, the mem sharing code has no visibility into altp2ms. There > are two cases we need to consider: the sharing of a gfn for which > there is another gfn in an altp2m pointing to it (as in the case of > domain 1 gfn A above -- both hostp2m gfn A and alt2m gfn O point to > mA), and the sharing of a gfn for which there is an altp2m if that gfn > pointing to a different gfn (as in the case of domain 1 gfn O above -- > hostp2m gfn O points to mO, altp2m gfn O points to mA). Then we also > have to consider the order in which things happen: altp2m then > sharing, sharing then altp2m. > > Let's first consider the case of domain 1 gfn A being shared. At the > moment, if the situation described above happens in that order (altp2m > set up first, then sharing) then the page nomination of d1gA will most > likely fail because the refcount on mA will be one more than expected. > If it happened in the reverse order (sharing set up first, then > altp2m), then setting the altp2m would unshare d1gA. Both should be > safe. > > Now what happens if we accept your patch as-is? It looks like altp2m > first then sharing will behave the same way -- the refcount will be > too high, so the nomination will fail. But if you share first and > then set the altp2m, then the altp2m gfn O will have a reference to mA > that isn't in your reverse map. If you get an unshare on domain 1 > altp2m gfn O, it looks to me like you'll get an unshare on *hostp2m* > gfn O, which points to mO, which is *not* shared -- at which point it > looks like you'll BUG(). If you get an unshare on domain 1 gfn A, it > looks to me like you'll get a new page, mN, at gfn A, but that altp2m > gfn O will still still point to mA -- effectively breaking whatever > the guest meant to be doing when it was using the altp2ms. > > I could go on in the analysis, but the point is that there's a morass > of interactions here all of which need to be correct, which this patch > does not address. You have a long way to go before sharing and altp2m > can be safely used on the same gfn ranges. > Hi George, certainly there are cornercases where if the user does not setup things in the right order things can still go out of whack. My goal with this patch is not to address all of them. The goal of this patch is to not crash the hypervisor when the user at least tries to experiment with it, which is the current state. So this patch improves the status quo. Also, both mem_sharing and altp2m is considered experimental/tech_preview, so the fact that their combination is also experimental should be assumed as well. As I explained, with this patch in place there is at least one way they can be safely used together if the user tracks unsharing requirements through mem_access and does unsharing and fixup of the altp2m views manually. There are other ways where it would not be safe as after unsharing we don't know how the user would want things to look in altp2ms. I don't think we want to start guessing about that either so I will not be looking to implement that. So I don't agree with this reasoning being grounds for rejecting this patch that does incrementally improve the current state. Tamas --001a114b70226f656f0537addcdc Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Jul 15, 2016 02:57, "George Dunlap" <george.dunlap@citrix.com> wrote= :
>
> On Thu, May 26, 2016 at 5:17 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> >
> > On May 26, 2016 04:40, "George Dunlap" <george.dunlap@citrix.com> wrote:
> >>
> >> On 26/05/16 04:55, Tamas K Lengyel wrote:
> >> > Move sharing locks above altp2m to avoid locking order v= iolation. Allow
> >> > applying altp2m mem_access settings when the hostp2m ent= ries are shared.
> >> > Also, do not trigger PoD for hostp2m when setting altp2m= mem_access to
> >> > be
> >> > in-line with non-altp2m mem_access path. Also allow gfn = remapping with
> >> > p2m_ram_shared type gfns in altp2m views.
> >> >
> >> > When using mem_sharing in combination with altp2m, unsha= ring events
> >> > overwrite
> >> > altp2m entries with that of the hostp2m (both remapped e= ntries and
> >> > mem_access
> >> > settings). User should take precaution to not share page= s where this
> >> > behavior
> >> > is undesired.
> >>
> >> I'm afraid this is not acceptable.=C2=A0 How could this e= ver be even remotely
> >> usable?=C2=A0 If this is a necessary side effect of sharing, = then I think the
> >> original functionality, of un-sharing when setting an altp2m = entry is
> >> the only option (and not allowing sharing to happen when an a= ltp2m is
> >> present for a particular gfn).
> >>
> >> Hmm, but actually this also brings up another tricky point: I= n an altp2m
> >> you can change the mfn which backs a gfn.=C2=A0 This would ne= ed to be handled
> >> properly in the reverse map, which it doesn't look like i= t is at the
> >> moment.
> >>
> >> On the whole, I think if you're going to allow a single g= fn to be
> >> simultaneously shared and allow an altp2m for it, you're = going to need
> >> to do a lot more work.
> >>
> >> (Sorry for not catching a lot of this before...)
> >>
> >
> > Well this patch resolves the locking order violation and allows t= he
> > xen-access tool's altp2m tests to pass, so it does improve on= the current
> > situation which is a hypervisor crash. To help with the override = issue the
> > user can apply W mem_access permission on the shared hostp2m entr= ies. That
> > way they get notification through vm_event of the event that lead= s to
> > unsharing and can then reapply the altp2m changes. So IMHO this p= atch is
> > already quite workable and while it requires more setup from the = userside,
> > the VMM side is OK with this change.
>
> So correct me if I'm wrong here.
>
> The altp2m stuff was primarily designed for guest operating systems to=
> be able to make alternate "views" of their own P2M.=C2=A0 Wh= en enabled,
> extra p2ms are allocated which allow a VM to remap a gfn to point to > the gfn of an mfn in its own address space.
>
> For example, suppose domain 1 host p2m gfns A, B, and C point to mA, > mB, and mC respectively.=C2=A0 The guest can create an altp2m a1 such = that
> gfns O, P, and Q also point to mA, mB, and mC.=C2=A0 The guest can als= o
> register to receive #VE for violations of an altp2m which are not
> violations under the hostp2m.
>
> mem sharing allows a process in domain 0 (or some other privileged
> domain) to nominate two gfns in different domains to be shared; say, > domain 1 gfn A and domain 2 gfn X (d1gA and d2gX, respectively).=C2=A0= This
> works by first "nominating" the respective gfns, then sharin= g them.
> "Nominating" a gfn tells the sharing infrastructure to start= tracking
> mappings of that page in a reverse map; after nomination mA's page=
> structure will point to d1gA, and mX's page structure will point t= o
> d2gX.=C2=A0 At that point they can be "shared", at which poi= nt (for
> example) both d1gA and d2gX will point to mA, and mA's reverse map=
> will contain d1gA and d2gX.
>
> However, the mem sharing code has no visibility into altp2ms.=C2=A0 Th= ere
> are two cases we need to consider: the sharing of a gfn for which
> there is another gfn in an altp2m pointing to it (as in the case of > domain 1 gfn A above -- both hostp2m gfn A and alt2m gfn O point to > mA), and the sharing of a gfn for which there is an altp2m if that gfn=
> pointing to a different gfn (as in the case of domain 1 gfn O above --=
> hostp2m gfn O points to mO, altp2m gfn O points to mA).=C2=A0 Then we = also
> have to consider the order in which things happen: altp2m then
> sharing, sharing then altp2m.
>
> Let's first consider the case of domain 1 gfn A being shared.=C2= =A0 At the
> moment, if the situation described above happens in that order (altp2m=
> set up first, then sharing) then the page nomination of d1gA will most=
> likely fail because the refcount on mA will be one more than expected.=
> If it happened in the reverse order (sharing set up first, then
> altp2m), then setting the altp2m would unshare d1gA.=C2=A0 Both should= be
> safe.
>
> Now what happens if we accept your patch as-is?=C2=A0 It looks like al= tp2m
> first then sharing will behave the same way -- the refcount will be > too high, so the nomination will fail.=C2=A0 But if you share first an= d
> then set the altp2m, then the altp2m gfn O will have a reference to mA=
> that isn't in your reverse map.=C2=A0 If you get an unshare on dom= ain 1
> altp2m gfn O, it looks to me like you'll get an unshare on *hostp2= m*
> gfn O, which points to mO, which is *not* shared -- at which point it<= br> > looks like you'll BUG().=C2=A0 If you get an unshare on domain 1 g= fn A, it
> looks to me like you'll get a new page, mN, at gfn A, but that alt= p2m
> gfn O will still still point to mA -- effectively breaking whatever > the guest meant to be doing when it was using the altp2ms.
>
> I could go on in the analysis, but the point is that there's a mor= ass
> of interactions here all of which need to be correct, which this patch=
> does not address.=C2=A0 You have a long way to go before sharing and a= ltp2m
> can be safely used on the same gfn ranges.
>

Hi George,
certainly there are cornercases where if the user does not setup things in = the right order things can still go out of whack. My goal with this patch i= s not to address all of them. The goal of this patch is to not crash the hy= pervisor when the user at least tries to experiment with it, which is the c= urrent state. So this patch improves the status quo. Also, both mem_sharing= and altp2m is considered experimental/tech_preview, so the fact that their= combination is also experimental should be assumed as well. As I explained= , with this patch in place there is at least one way they can be safely use= d together if the user tracks unsharing requirements through mem_access and= does unsharing and fixup of the altp2m views manually. There are other way= s where it would not be safe as after unsharing we don't know how the u= ser would want things to look in altp2ms. I don't think we want to star= t guessing about that either so I will not be looking to implement that. So= I don't agree with this reasoning being grounds for rejecting this pat= ch that does incrementally improve the current state.

Tamas

--001a114b70226f656f0537addcdc-- --===============2551889732114005003== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============2551889732114005003==--