From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [V9 PATCH 6/8] pvh dom0: Add and remove foreign pages Date: Wed, 23 Apr 2014 19:21:51 -0700 Message-ID: <20140423192151.0b05a91b@mantra.us.oracle.com> References: <1397607172-32065-1-git-send-email-mukesh.rathor@oracle.com> <1397607172-32065-7-git-send-email-mukesh.rathor@oracle.com> <534EC5430200007800009ADD@nat28.tlf.novell.com> <20140416183742.50a98472@mantra.us.oracle.com> <534F95E00200007800009D40@nat28.tlf.novell.com> <20140417123653.GA25395@deinos.phlegethon.org> <534FFA32020000780000A077@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Wd9Id-0003Fo-D2 for xen-devel@lists.xenproject.org; Thu, 24 Apr 2014 02:22:07 +0000 In-Reply-To: <534FFA32020000780000A077@nat28.tlf.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 Deegan , 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 Thu, 17 Apr 2014 14:58:42 +0100 "Jan Beulich" wrote: > >>> On 17.04.14 at 14:36, wrote: > > At 07:50 +0100 on 17 Apr (1397717440), Jan Beulich wrote: > >> >>> On 17.04.14 at 03:37, wrote: > >> > On Wed, 16 Apr 2014 17:00:35 +0100 > >> > "Jan Beulich" wrote: ....... > >> > Well, Tim and I went back and forth several times on this over > >> > the last several months (you were cc'd :) ). > >> > >> I know, but having worked a lot on the P2M code recently my > >> perspective may have changed. > > > > [I'm assuming the objection here is to having ther refcounts updated > > in atomic_write_ept_entry, which was the change I requested.] > > My opinion is still very strongly that reference counting must be > > done when the entries change. Trying to get this kind of thing > > right in the callers is an _enormous_ PITA, as I learned working on > > the shadow pagetables. It would get very messy (see, e.g. the > > myriad places where p2m op callers individually check for > > paged/shared entries) and it'd be nigh impossible to debug where in > > several hours of operation something changed a p2m entry from > > foreign to something else without dropping a page ref. > > > > That said, it should be easy enough only to refcount on leaf > > entries, right? I can't see how that would be incompatible with the > > intermediate-node changes that Jan is working on. > > Right - keeping the macro as is and introducing a derived function to > handle the extra requirements on leaf entries would seem quite okay, > so long as error propagation can be done properly. Ok, how about something like the following? In case of get_page failure, not sure EINVAL is the best one to return, EBUSY? thanks mukesh diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 1fa839a..db2fa3a 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -403,6 +403,21 @@ bool_t ept_handle_misconfig(uint64_t gpa) return !!okay; } +static int ept_get_foreign_refcnt(mfn_t mfn) +{ + struct domain *fdom; + + if ( !mfn_valid(mfn_x(mfn)) ) + return -EINVAL; + + fdom = page_get_owner(mfn_to_page(mfn_x(mfn))); + if ( fdom == NULL ) + return -ESRCH; + + /* get refcount on the page */ + if ( !get_page(mfn_to_page(mfn_x(mfn)), fdom) ) + return -EINVAL; + + return 0; +} + /* * ept_set_entry() computes 'need_modify_vtd_table' for itself, * by observing whether any gfn->mfn translations are modified. @@ -427,6 +442,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, ept_entry_t new_entry = { .epte = 0 }; struct ept_data *ept = &p2m->ept; struct domain *d = p2m->domain; + unsigned long prev_foreign_mfn = INVALID_MFN; ASSERT(ept); /* @@ -460,6 +476,14 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, ASSERT(ret != GUEST_TABLE_POD_PAGE || i != target); + /* foreign p2m types must be refcounted before being added */ + if ( unlikely(p2m_is_foreign(p2mt)) ) + { + rc = ept_get_foreign_refcnt(mfn); + if ( rc ) + goto out; + } + ept_entry = table + (gfn_remainder >> (i * EPT_TABLE_ORDER)); /* In case VT-d uses same page table, this flag is needed by VT-d */ @@ -545,8 +569,14 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, ept_p2m_type_to_flags(&new_entry, p2mt, p2ma); } + if ( unlikely(p2m_is_foreign(ept_entry->sa_p2mt)) ) + prev_foreign_mfn = ept_entry->mfn; + atomic_write_ept_entry(ept_entry, new_entry); + if ( unlikely(prev_foreign_mfn != INVALID_MFN) ) + put_page(mfn_to_page(prev_foreign_mfn)); + /* Track the highest gfn for which we have ever had a valid mapping */ if ( p2mt != p2m_invalid && (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) @@ -576,6 +606,10 @@ out: } } + /* do cleanup for foreign types in case of error */ + if ( unlikely(rc && ept_entry && p2m_is_foreign(p2mt)) ) + put_page(mfn_to_page(mfn_x(mfn))); + /* Release the old intermediate tables, if any. This has to be the last thing we do, after the ept_sync_domain() and removal from the iommu tables, so as to avoid a potential