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

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

On Thu, 24 Apr 2014 11:46:41 +0200
Tim Deegan <tim@xen.org> wrote:

> At 19:21 -0700 on 23 Apr (1398277311), Mukesh Rathor wrote:
> > 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:
......
> > > > 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?
> 
> This goes back to having refcounts open-coded.  Having the refcounts
> open-coded around the atomic_write_ept_entry() in ept_set_entry()
> means there are now places where the epte can change without
> maintaining the refcount invariants: ept_change_entry_type_page(), for
> example.

Correct, altho, at present I've checks in p2m paths to not allow foreign
types to come down to such calls.

> I would _much_ prefer to have atomic_write_ept_entry() DTRT -- it
> would have to know the difference between leaf and non-leaf entries,
> and return an error code.  I'd also be OK with having two
> atomic_write ops, one for leaf and one for non-leaf, with appropriate
> ASSERT()s on the contents.

Ok, how about something like shown further below?  (I think
it would be more simpler to have one atomic_write ops, instead of two)

One thing, on returning error code, since at present there are no
paths allowing superpages for foreign types, it appears I'd not need 
to worry about undoing the ept_split_super_page(), so I added
assert in there. Sound right?

Finally, I can leave other callers of atomic_write_ept_entry as is, or 
add ASSERTs for rc == 0. LMK.

thanks for patience,
mukesh


diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 1fa839a..41a8c40 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,43 @@ 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)
+{
+    unsigned long oldmfn;
+    struct domain *fdom;
+    bool_t new_foreign = p2m_is_foreign(new.sa_p2mt);
+    bool_t old_foreign = p2m_is_foreign(entryptr->sa_p2mt);
+
+    if (is_epte_superpage(entryptr) || likely(!new_foreign && !old_foreign) )
+    {
+        write_atomic(&entryptr->epte, new.epte);
+        return 0;
+    }
+    
+    if ( new_foreign )
+    {
+        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;
+    }
+
+    oldmfn = entryptr->mfn;
+    write_atomic(&entryptr->epte, new.epte);
+
+    if ( old_foreign )
+        put_page(mfn_to_page(oldmfn));
+
+    return 0;
+}
+
 static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_access_t access)
 {
     /* First apply type permissions */
@@ -494,6 +529,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
         ept_entry_t split_ept_entry;
 
         ASSERT(is_epte_superpage(ept_entry));
+        ASSERT(!p2m_is_foreign(p2mt));
 
         split_ept_entry = atomic_read_ept_entry(ept_entry);
 
@@ -545,7 +581,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
         ept_p2m_type_to_flags(&new_entry, p2mt, p2ma);
     }
 
-    atomic_write_ept_entry(ept_entry, new_entry);
+    rc = atomic_write_ept_entry(ept_entry, new_entry);
 
     /* Track the highest gfn for which we have ever had a valid mapping */
     if ( p2mt != p2m_invalid &&