From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tim Deegan <tim@xen.org>
Subject: Re: [PATCH 5 of 9] x86/mm: When removing/adding a page
	from/to the physmap, keep in mind it could be shared
Date: Thu, 2 Feb 2012 12:41:18 +0000
Message-ID: <20120202124118.GI48883@ocelot.phlegethon.org>
References: <patchbomb.1328125912@xdev.gridcentric.ca>
	<1c61573d17650c67448c.1328125917@xdev.gridcentric.ca>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
Content-Disposition: inline
In-Reply-To: <1c61573d17650c67448c.1328125917@xdev.gridcentric.ca>
List-Unsubscribe: <http://lists.xensource.com/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Cc: andres@gridcentric.ca, xen-devel@lists.xensource.com, olaf@aepfle.de, adin@gridcentric.ca
List-Id: xen-devel@lists.xenproject.org

At 14:51 -0500 on 01 Feb (1328107917), Andres Lagar-Cavilla wrote:
>  xen/arch/x86/mm/p2m.c |  21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
> 
> 
> When removing the m2p mapping it is unconditionally set to invalid, which
> breaks sharing.
> 
> When adding to the physmap, if the previous holder of that entry is a shared
> page, we unshare to default to normal case handling.
> 
> And, we cannot add a shared page directly to the physmap. Proper interfaces
> must be employed, otherwise book-keeping goes awry.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> diff -r 8a920bcddd0f -r 1c61573d1765 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -419,7 +419,7 @@ p2m_remove_page(struct p2m_domain *p2m, 
>          for ( i = 0; i < (1UL << page_order); i++ )
>          {
>              mfn_return = p2m->get_entry(p2m, gfn + i, &t, &a, p2m_query, NULL);
> -            if ( !p2m_is_grant(t) )
> +            if ( !p2m_is_grant(t) && !p2m_is_shared(t) )
>                  set_gpfn_from_mfn(mfn+i, INVALID_M2P_ENTRY);
>              ASSERT( !p2m_is_valid(t) || mfn + i == mfn_x(mfn_return) );
>          }
> @@ -481,6 +481,17 @@ guest_physmap_add_entry(struct domain *d
>      for ( i = 0; i < (1UL << page_order); i++ )
>      {
>          omfn = p2m->get_entry(p2m, gfn + i, &ot, &a, p2m_query, NULL);
> +        if ( p2m_is_shared(ot) )
> +        {
> +            /* Do an unshare to cleanly take care of all corner 
> +             * cases. */
> +            int rc;
> +            rc = mem_sharing_unshare_page(p2m->domain, gfn + i, 0);
> +            if ( rc )
> +                return rc;

You're holding the p2m lock here!  Also, I don't think you can call
mem_sharing_unshare_page() with that held - wasn't that the reason for 
cset f6c33cfe7333 ?

Tim.