Re: [V9 PATCH 6/8] pvh dom0: Add and remove foreign pages

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Thu, 17 Apr 2014 14:58:42 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 17.04.14 at 14:36, <tim@xen.org> wrote:
> > At 07:50 +0100 on 17 Apr (1397717440), Jan Beulich wrote:
> >> >>> On 17.04.14 at 03:37, <mukesh.rathor@oracle.com> wrote:
> >> > On Wed, 16 Apr 2014 17:00:35 +0100
> >> > "Jan Beulich" <JBeulich@suse.com> 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