From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [V11 PATCH 3/4] pvh dom0: Add and remove foreign pages Date: Mon, 5 May 2014 18:21:42 -0700 Message-ID: <20140505182142.62e5151e@mantra.us.oracle.com> References: <1398995707-12657-1-git-send-email-mukesh.rathor@oracle.com> <1398995707-12657-4-git-send-email-mukesh.rathor@oracle.com> <53678A65020000780000EF38@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WhU4z-0008AB-1q for xen-devel@lists.xenproject.org; Tue, 06 May 2014 01:21:57 +0000 In-Reply-To: <53678A65020000780000EF38@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: George.Dunlap@eu.citrix.com, tim@xen.org, eddie.dong@intel.com, keir.xen@gmail.com, jun.nakajima@intel.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On Mon, 05 May 2014 11:56:05 +0100 "Jan Beulich" wrote: > >>> On 02.05.14 at 03:55, wrote: > > @@ -4584,6 +4584,9 @@ int xenmem_add_to_physmap_one( > > page = mfn_to_page(mfn); > > break; > > } > > + case XENMAPSPACE_gmfn_foreign: > > + rc = p2m_add_foreign(d, idx, gpfn, foreign_domid); > > + return rc; > > Any reason not to simply "return p2m_add_foreign();"? Could, rc allows quick adding of printk for debugging. But, I will change it. > > +static int atomic_write_ept_entry(ept_entry_t *entryptr, > > ept_entry_t new, > > + int level) > > +{ > > + bool_t same_mfn = (new.mfn == entryptr->mfn); > > + unsigned long oldmfn = INVALID_MFN; > > + > > + if ( level ) > > + { > > + ASSERT(!p2m_is_foreign(new.sa_p2mt)); > > + write_atomic(&entryptr->epte, new.epte); > > + return 0; > > + } > > + > > + if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !same_mfn ) > > + { > > + struct domain *fdom; > > + > > + if ( !mfn_valid(new.mfn) ) > > + return -EINVAL; > > + > > + fdom = page_get_owner(mfn_to_page(new.mfn)); > > + if ( fdom == NULL ) > > + return -ESRCH; > > + > > + /* get refcount on the page */ > > + if ( !get_page(mfn_to_page(new.mfn), fdom) ) > > + return -EBUSY; > > + } > > + > > + if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && !same_mfn ) > > + oldmfn = entryptr->mfn; > > I'm afraid this "same_mfn" handling is correct only if sa_p2mt also > does not change. Yup, my bad. Will fix it, thanks. > > @@ -287,14 +288,20 @@ struct page_info *get_page_from_gfn_p2m( > > /* Fast path: look up and get out */ > > p2m_read_lock(p2m); > > mfn = __get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0); > > - if ( (p2m_is_ram(*t) || p2m_is_grant(*t)) > > - && mfn_valid(mfn) > > + if ( p2m_is_any_ram(*t) && mfn_valid(mfn) > > && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) ) > > { > > page = mfn_to_page(mfn); > > - if ( !get_page(page, d) > > - /* Page could be shared */ > > - && !get_page(page, dom_cow) ) > > + if ( p2m_is_foreign(*t) ) > > I think here (and possible elsewhere) use of unlikely() is desirable. Ok. > > @@ -444,6 +451,10 @@ int p2m_alloc_table(struct p2m_domain *p2m) > > return rc; > > } > > > > +/* > > + * pvh fixme: when adding support for pvh non-hardware domains, > > this path must > > + * cleanup any foreign p2m types (release refcnts on them). > > + */ > > And I wonder whether you shouldn't enforce this by disallowing non- > hardware domains to create foreign mappings. Hmm... Tim wanted the enforcement. That will ensure the cleanup is implemented without falling thru the cracks. thanks Mukesh