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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

 -George

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