From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [V10 PATCH 3/4] pvh dom0: Add and remove foreign pages Date: Fri, 2 May 2014 16:35:56 -0700 Message-ID: <20140502163556.32baa414@mantra.us.oracle.com> References: <1398820008-9005-1-git-send-email-mukesh.rathor@oracle.com> <1398820008-9005-4-git-send-email-mukesh.rathor@oracle.com> <20140501161908.GI86038@deinos.phlegethon.org> <20140501184518.2ebccdb8@mantra.us.oracle.com> <20140502085555.GA64491@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WgMzz-0003Ob-DP for xen-devel@lists.xenproject.org; Fri, 02 May 2014 23:36:11 +0000 In-Reply-To: <20140502085555.GA64491@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: JBeulich@suse.com, George.Dunlap@eu.citrix.com, 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 Fri, 2 May 2014 10:55:55 +0200 Tim Deegan wrote: > Hi, > > At 18:45 -0700 on 01 May (1398966318), Mukesh Rathor wrote: > > On Thu, 1 May 2014 18:19:08 +0200 > > Tim Deegan wrote: ..... > > > > > > At 18:06 -0700 on 29 Apr (1398791207), Mukesh Rathor wrote: > > > > diff --git a/xen/arch/x86/mm/p2m-ept.c > > > > b/xen/arch/x86/mm/p2m-ept.c index c0bfc50..11474e8 100644 > > > > --- a/xen/arch/x86/mm/p2m-ept.c > > > > +++ b/xen/arch/x86/mm/p2m-ept.c > > > > @@ -36,8 +36,6 @@ > > > > > > > > #define > > > > atomic_read_ept_entry(__pepte) \ > > > > ( (ept_entry_t) { .epte = read_atomic(&(__pepte)->epte) } ) > > > > -#define atomic_write_ept_entry(__pepte, > > > > __epte) \ > > > > - write_atomic(&(__pepte)->epte, (__epte).epte) > > > > > > > > #define is_epte_present(ept_entry) ((ept_entry)->epte & > > > > 0x7) #define is_epte_superpage(ept_entry) ((ept_entry)->sp) > > > > @@ -46,6 +44,46 @@ static inline bool_t > > > > is_epte_valid(ept_entry_t *e) return (e->epte != 0 && > > > > e->sa_p2mt != p2m_invalid); } > > > > > > > > +/* returns : 0 for success, -errno otherwise */ > > > > +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(!(new.sp && p2m_is_foreign(new.sa_p2mt))) here? > > > > Yeah, I debated adding p2m_is_foreign ASSERT, but didn't because > > iirc Jan had said he wanted to use up the non-leaf bits for > > something else in future. > > Well, if new.sp is set it's not non-leaf. Though I suppose maybe the > test should be for _valid_ + superpage + foreign. Ok, so looks like: ASSERT(new.sa_p2mt == p2m_invalid || !(new.sp && p2m_is_foreign(new.sa_p2mt))); should do it? ... > > > > + 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)); > > > > > > This needs a matching put_pg_owner() once you're done with it. > > > > Doing page_get_owner, not get_pg_owner here, so don't think so, > > right? > > Oh yes -- sorry, I got confused between this and the case where you do > use get_pg_owner(). But actually on this subject ISTR objecting to > exporting get_pg_owner() from mm.c before. Oh yes, here it is: > > http://lists.xenproject.org/archives/html/xen-devel/2014-01/msg02612.html > > So can you use rcu_lock_live_remote_domain_by_id() here instead? In p2m_add_foreign() we already get the rcu lock, still do it here? And, btw, that function, p2m_add_foreign(), went from + if ( (fdid == DOMID_XEN && (fdom = rcu_lock_domain(dom_xen)) == NULL) || + (fdid != DOMID_XEN && (fdom = rcu_lock_domain_by_id(fdid)) == NULL) ) + goto out; to + fdom = get_pg_owner(foreigndom); (causing get_pg_owner to be exported) at Jan's comment/suggestion in v8. thanks, mukesh