Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Jul 15, 2016 02:57, "George Dunlap" <george.dunlap@xxxxxxxxxx> wrote:
>
> On Thu, May 26, 2016 at 5:17 PM, Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
> >
> > On May 26, 2016 04:40, "George Dunlap" <george.dunlap@xxxxxxxxxx> 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

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