From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: [V10 PATCH 3/4] pvh dom0: Add and remove foreign pages Date: Fri, 2 May 2014 10:55:55 +0200 Message-ID: <20140502085555.GA64491@deinos.phlegethon.org> 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> 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 1Wg9GB-0008Jk-Is for xen-devel@lists.xenproject.org; Fri, 02 May 2014 08:55:59 +0000 Content-Disposition: inline In-Reply-To: <20140501184518.2ebccdb8@mantra.us.oracle.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: Mukesh Rathor 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 Hi, At 18:45 -0700 on 01 May (1398966318), Mukesh Rathor wrote: > On Thu, 1 May 2014 18:19:08 +0200 > Tim Deegan wrote: > > Nearly there, except that the teardown code is now gone, with nothing > > replacing it. Ideally we'd have some logic to find active EPT L1 > > tables and undo the foreign refcounts. If not that, then: > > - a hard-coded check, in p2m_add_foreign I suppose, that only allows > > foreign mappings to be added to a p2m for the hardware domain, > > with a pvh fixme comment explaining why; and > > - another pvh fixme comment in the p2m teardown code to say that > > once non-hardware domains can have foreign mappings there will > > have to be some cleanup code to handle them. > > Added fixmes... Thanks. > > Also, in the inner refcounting logic: > > > > 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. > I suppose I can add it, it would be easy > to find it anyways.... BTW, there is no path allowing > superpage for foreign at present. Yes, otherwise the ASSERT would be bogus. :) But it will be useful at least as documentation if someone adds such a path. > > > + 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? Tim.