From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Andres Lagar-Cavilla" 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 05:46:04 -0800 Message-ID: <098b8cacfeda78519afc2bbd4a731021.squirrel@webmail.lagarcavilla.org> References: <1c61573d17650c67448c.1328125917@xdev.gridcentric.ca> <20120202124118.GI48883@ocelot.phlegethon.org> Reply-To: andres@lagarcavilla.org Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20120202124118.GI48883@ocelot.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Tim Deegan 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 >> >> 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 ? D'oh! This patch has to follow the locking p2m series. Before reposting, I'll have to make sure nothing breaks when applying after locking p2m. Good catch! Andres > > Tim. > >