Re: [V12 PATCH 3/4] pvh dom0: Add and remove foreign pages

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

On Mon, 12 May 2014 11:34:14 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 10.05.14 at 02:50, <mukesh.rathor@oracle.com> wrote:
> > +static int atomic_write_ept_entry(ept_entry_t *entryptr,
> > ept_entry_t new,
> > +                                  int level)
> > +{
> > +    unsigned long oldmfn = INVALID_MFN;
> > +    bool_t skip_foreign = (new.mfn == entryptr->mfn &&
> > +                           new.sa_p2mt == entryptr->sa_p2mt);
> 
> This still seems too weak to me: Shouldn't you also consider whether
> the old and new entries respectively are present (also further down)?

Not sure I understand why. skip_foreign is combined with p2m_is_foreign: 

    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
    {
       ...
so checking for invalid entry seems redundant based on my 
understanding that invalid entries have sa_p2mt == 1, or are 
zeroed, in which case sa_p2mt == 0.


> > @@ -292,7 +332,7 @@ static bool_t ept_invalidate_emt(mfn_t mfn,
> > bool_t recalc) e.emt = MTRR_NUM_TYPES;
> >          if ( recalc )
> >              e.recalc = 1;
> > -        atomic_write_ept_entry(&epte[i], e);
> > +        atomic_write_ept_entry(&epte[i], e, level);
> 
> I'm afraid you mustn't ever ignore this function failing (i.e. unless
> you're in places where you know the non-leaf shortcut is always
> going to be taken, but even there I think you'd be better off
> documenting this via ASSERT()), for security reasons. And yes, I
> realize that this isn't going to be trivial in some cases, especially
> if you want to do better than domain_crash().

Hmmm... since p2m type can only change via ept_set_entry, all other 
callers are guaranteed success, or IOW, the function is effectively same 
as before for other callers. As such, an ASSERT combined with printk 
should be acceptable IMO. Please see below.

> And a more general question: How is the insertion of p2m_foreign
> entries working together with the controlled domain (i.e. the one
> owning the page) being subject to paging/sharing? I only recall
> fixme-s having got added for the two features presently not being
> supported for PVH domains...

Right, the two features are not supported presently, the caller will
get -EINVAL if attempted. No further progress. 

thanks
mukesh


diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 73f41ac..eb11d80 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -48,6 +48,7 @@ static inline bool_t is_epte_valid(ept_entry_t *e)
 static int atomic_write_ept_entry(ept_entry_t *entryptr, ept_entry_t new,
                                   int level)
 {
+    int rc = 0;
     unsigned long oldmfn = INVALID_MFN;
     bool_t skip_foreign = (new.mfn == entryptr->mfn &&
                            new.sa_p2mt == entryptr->sa_p2mt);
@@ -56,23 +57,26 @@ static int atomic_write_ept_entry(ept_entry_t *entryptr, ept_entry_t new,
     {
         ASSERT(!new.sp || !p2m_is_foreign(new.sa_p2mt));
         write_atomic(&entryptr->epte, new.epte);
-        return 0;
+        goto out;
     }
 
     if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
     {
         struct domain *fdom;
 
+        rc = -EINVAL;
         if ( !mfn_valid(new.mfn) )
-            return -EINVAL;
+            goto out;
 
+        rc = -ESRCH;
         fdom = page_get_owner(mfn_to_page(new.mfn));
         if ( fdom == NULL )
-            return -ESRCH;
+            goto out;
 
         /* get refcount on the page */
+        rc = -EBUSY;
         if ( !get_page(mfn_to_page(new.mfn), fdom) )
-            return -EBUSY;
+            goto out;
     }
 
     if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && !skip_foreign )
@@ -83,7 +87,11 @@ static int atomic_write_ept_entry(ept_entry_t *entryptr, ept_entry_t new,
     if ( unlikely(oldmfn != INVALID_MFN) )
         put_page(mfn_to_page(oldmfn));
 
-    return 0;
+ out:
+    if ( rc ) 
+        gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64" rc:%d\n",
+                 entryptr->epte, new.epte, rc);
+    return rc;
 }
 
 static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_access_t access)
@@ -317,6 +325,7 @@ static int ept_next_level(struct p2m_domain *p2m, bool_t read_only,
  */
 static bool_t ept_invalidate_emt(mfn_t mfn, bool_t recalc, int level)
 {
+    int rc;
     ept_entry_t *epte = map_domain_page(mfn_x(mfn));
     unsigned int i;
     bool_t changed = 0;
@@ -332,7 +341,8 @@ static bool_t ept_invalidate_emt(mfn_t mfn, bool_t recalc, int level)
         e.emt = MTRR_NUM_TYPES;
         if ( recalc )
             e.recalc = 1;
-        atomic_write_ept_entry(&epte[i], e, level);
+        rc = atomic_write_ept_entry(&epte[i], e, level);
+        ASSERT(rc == 0);
         changed = 1;
     }
 
@@ -356,7 +366,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
     ept_entry_t *table;
     unsigned long gfn_remainder = first_gfn;
     unsigned int i, index;
-    int rc = 0, ret = GUEST_TABLE_MAP_FAILED;
+    int wrc, rc = 0, ret = GUEST_TABLE_MAP_FAILED;
 
     table = map_domain_page(pagetable_get_pfn(p2m_get_pagetable(p2m)));
     for ( i = ept_get_wl(&p2m->ept); i > target; --i )
@@ -382,7 +392,8 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
             rc = -ENOMEM;
             goto out;
         }
-        atomic_write_ept_entry(&table[index], split_ept_entry, i);
+        wrc = atomic_write_ept_entry(&table[index], split_ept_entry, i);
+        ASSERT(wrc == 0);
 
         for ( ; i > target; --i )
             if ( !ept_next_level(p2m, 1, &table, &gfn_remainder, i) )
@@ -401,7 +412,8 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
         {
             e.emt = MTRR_NUM_TYPES;
             e.recalc = 1;
-            atomic_write_ept_entry(&table[index], e, target);
+            wrc = atomic_write_ept_entry(&table[index], e, target);
+            ASSERT(wrc == 0);
             rc = 1;
         }
     }
@@ -430,7 +442,7 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
     unsigned int level = ept_get_wl(ept);
     unsigned long mfn = ept_get_asr(ept);
     ept_entry_t *epte;
-    int rc = 0;
+    int wrc, rc = 0;
 
     if ( !mfn )
         return 0;
@@ -471,7 +483,8 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
                          ept_p2m_type_to_flags(&e, e.sa_p2mt, e.access);
                     }
                     e.recalc = 0;
-                    atomic_write_ept_entry(&epte[i], e, level);
+                    wrc = atomic_write_ept_entry(&epte[i], e, level);
+                    ASSERT(wrc == 0);
                 }
             }
             else
@@ -505,7 +518,8 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
                 {
                     if ( ept_split_super_page(p2m, &e, level, level - 1) )
                     {
-                        atomic_write_ept_entry(&epte[i], e, level);
+                        wrc = atomic_write_ept_entry(&epte[i], e, level);
+                        ASSERT(wrc == 0);
                         unmap_domain_page(epte);
                         mfn = e.mfn;
                         continue;
@@ -519,7 +533,8 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
                 e.recalc = 0;
                 if ( recalc && p2m_is_changeable(e.sa_p2mt) )
                     ept_p2m_type_to_flags(&e, e.sa_p2mt, e.access);
-                atomic_write_ept_entry(&epte[i], e, level);
+                wrc = atomic_write_ept_entry(&epte[i], e, level);
+                ASSERT(wrc == 0);
             }
 
             rc = 1;
@@ -533,7 +548,8 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
             smp_wmb();
             e.emt = 0;
             e.recalc = 0;
-            atomic_write_ept_entry(&epte[i], e, level);
+            wrc = atomic_write_ept_entry(&epte[i], e, level);
+            ASSERT(wrc == 0);
             unmap_domain_page(epte);
             rc = 1;
         }
@@ -690,7 +706,8 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
 
         /* now install the newly split ept sub-tree */
         /* NB: please make sure domian is paused and no in-fly VT-d DMA. */
-        atomic_write_ept_entry(ept_entry, split_ept_entry, i);
+        rc = atomic_write_ept_entry(ept_entry, split_ept_entry, i);
+        ASSERT(rc == 0);
 
         /* then move to the level we want to make real changes */
         for ( ; i > target; i-- )