[V13 PATCH 0/2] pvh dom0 patches...

[V13 PATCH 0/2] pvh dom0 patches...

[Mukesh Rathor]

Hi,

Attached please find v13 of dom0 pvh patch series based on 
c/s: 11dba84.

git tree: git://oss.oracle.com/git/mrathor/xen.git  branch: dom0pvh-v13

Changes from v12:
  patch 1:
     - don't allow paging/sharing if dom0 is pvh.
     - don't ignore rc from atomic_write_ept_entry even in paths that don't
       set foreign types.
     - use is_epte_superpage() instead of new.sp bit check in 
       atomic_write_ept_entry

  patch 2: none

thanks,
Mukesh

[V13 PATCH 1/2] pvh dom0: Add and remove foreign pages

[Mukesh Rathor]

In this patch, a new function, p2m_add_foreign(), is added
to map pages from a foreign guest into dom0 for various purposes
like domU creation, running xentrace, etc... Such pages are
typed p2m_map_foreign.  Note, it is the nature of such pages
that a refcnt is held during their stay in the p2m. The
refcnt is added and released in the low level ept function
atomic_write_ept_entry. That macro is converted to a function to allow
for such refcounting, which only applies to leaf entries in the ept.
Furthermore, please note that paging/sharing is disabled if the
controlling or hardware domain is pvh. Any enabling of those features
would need to ensure refcnt are properly maintained for foreign types,
or paging/sharing is skipped for foreign types.

Also, we change get_pg_owner so it allows foreign mappings for pvh.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/mm.c           |   4 +-
 xen/arch/x86/mm/mem_event.c |  11 ++++
 xen/arch/x86/mm/p2m-ept.c   |  98 +++++++++++++++++++++++++++-------
 xen/arch/x86/mm/p2m-pt.c    |   7 +++
 xen/arch/x86/mm/p2m.c       | 126 +++++++++++++++++++++++++++++++++++++++++---
 xen/include/asm-x86/p2m.h   |   7 +++
 6 files changed, 226 insertions(+), 27 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 1a8a5e0..d005c34 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2811,7 +2811,7 @@ static struct domain *get_pg_owner(domid_t domid)
         goto out;
     }
 
-    if ( unlikely(paging_mode_translate(curr)) )
+    if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
     {
         MEM_LOG("Cannot mix foreign mappings with translated domains");
         goto out;
@@ -4584,6 +4584,8 @@ int xenmem_add_to_physmap_one(
             page = mfn_to_page(mfn);
             break;
         }
+        case XENMAPSPACE_gmfn_foreign:
+            return p2m_add_foreign(d, idx, gpfn, foreign_domid);
         default:
             break;
     }
diff --git a/xen/arch/x86/mm/mem_event.c b/xen/arch/x86/mm/mem_event.c
index 36b9dba..7722dcb 100644
--- a/xen/arch/x86/mm/mem_event.c
+++ b/xen/arch/x86/mm/mem_event.c
@@ -538,6 +538,12 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
         case XEN_DOMCTL_MEM_EVENT_OP_PAGING_ENABLE:
         {
             struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+            rc = -EOPNOTSUPP;
+            /* pvh fixme: p2m_is_foreign types need addressing */
+            if ( is_pvh_vcpu(current) || is_pvh_domain(hardware_domain) )
+                break;
+
             rc = -ENODEV;
             /* Only HAP is supported */
             if ( !hap_enabled(d) )
@@ -620,6 +626,11 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
         {
         case XEN_DOMCTL_MEM_EVENT_OP_SHARING_ENABLE:
         {
+            rc = -EOPNOTSUPP;
+            /* pvh fixme: p2m_is_foreign types need addressing */
+            if ( is_pvh_vcpu(current) || is_pvh_domain(hardware_domain) )
+                break;
+
             rc = -ENODEV;
             /* Only HAP is supported */
             if ( !hap_enabled(d) )
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index bb98945..fe6bcd8 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,58 @@ 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)
+{
+    int rc = 0;
+    unsigned long oldmfn = INVALID_MFN;
+    bool_t skip_foreign = (new.mfn == entryptr->mfn &&
+                           new.sa_p2mt == entryptr->sa_p2mt);
+
+    if ( level )
+    {
+        ASSERT(!is_epte_superpage(&new) || !p2m_is_foreign(new.sa_p2mt));
+        write_atomic(&entryptr->epte, new.epte);
+        goto out;
+    }
+
+    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
+    {
+        struct domain *fdom;
+
+        rc = -EINVAL;
+        if ( !mfn_valid(new.mfn) )
+            goto out;
+
+        rc = -ESRCH;
+        fdom = page_get_owner(mfn_to_page(new.mfn));
+        if ( fdom == NULL )
+            goto out;
+
+        /* get refcount on the page */
+        rc = -EBUSY;
+        if ( !get_page(mfn_to_page(new.mfn), fdom) )
+            goto out;
+    }
+
+    if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && !skip_foreign )
+        oldmfn = entryptr->mfn;
+
+    write_atomic(&entryptr->epte, new.epte);
+
+    if ( unlikely(oldmfn != INVALID_MFN) )
+        put_page(mfn_to_page(oldmfn));
+
+    rc = 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)
 {
     /* First apply type permissions */
@@ -275,8 +325,9 @@ static int ept_next_level(struct p2m_domain *p2m, bool_t read_only,
  * present entries in the given page table, optionally marking the entries
  * also for their subtrees needing P2M type re-calculation.
  */
-static bool_t ept_invalidate_emt(mfn_t mfn, bool_t recalc)
+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;
@@ -292,7 +343,8 @@ 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);
+        rc = atomic_write_ept_entry(&epte[i], e, level);
+        ASSERT(rc == 0);
         changed = 1;
     }
 
@@ -316,7 +368,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 )
@@ -342,7 +394,8 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
             rc = -ENOMEM;
             goto out;
         }
-        atomic_write_ept_entry(&table[index], split_ept_entry);
+        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) )
@@ -361,7 +414,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);
+            wrc = atomic_write_ept_entry(&table[index], e, target);
+            ASSERT(wrc == 0);
             rc = 1;
         }
     }
@@ -390,7 +444,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;
@@ -431,7 +485,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);
+                    wrc = atomic_write_ept_entry(&epte[i], e, level);
+                    ASSERT(wrc == 0);
                 }
             }
             else
@@ -465,7 +520,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);
+                        wrc = atomic_write_ept_entry(&epte[i], e, level);
+                        ASSERT(wrc == 0);
                         unmap_domain_page(epte);
                         mfn = e.mfn;
                         continue;
@@ -479,7 +535,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);
+                wrc = atomic_write_ept_entry(&epte[i], e, level);
+                ASSERT(wrc == 0);
             }
 
             rc = 1;
@@ -489,11 +546,12 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
         if ( e.emt == MTRR_NUM_TYPES )
         {
             ASSERT(is_epte_present(&e));
-            ept_invalidate_emt(_mfn(e.mfn), e.recalc);
+            ept_invalidate_emt(_mfn(e.mfn), e.recalc, level);
             smp_wmb();
             e.emt = 0;
             e.recalc = 0;
-            atomic_write_ept_entry(&epte[i], e);
+            wrc = atomic_write_ept_entry(&epte[i], e, level);
+            ASSERT(wrc == 0);
             unmap_domain_page(epte);
             rc = 1;
         }
@@ -585,6 +643,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
     ASSERT((target == 2 && hvm_hap_has_1gb()) ||
            (target == 1 && hvm_hap_has_2mb()) ||
            (target == 0));
+    ASSERT(!p2m_is_foreign(p2mt) || target == 0);
 
     table = map_domain_page(pagetable_get_pfn(p2m_get_pagetable(p2m)));
 
@@ -649,7 +708,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);
+        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-- )
@@ -688,10 +748,10 @@ 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, target);
 
     /* Track the highest gfn for which we have ever had a valid mapping */
-    if ( p2mt != p2m_invalid &&
+    if ( rc == 0 && p2mt != p2m_invalid &&
          (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
         p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
 
@@ -723,7 +783,7 @@ out:
        last thing we do, after the ept_sync_domain() and removal
        from the iommu tables, so as to avoid a potential
        use-after-free. */
-    if ( is_epte_present(&old_entry) )
+    if ( rc == 0 && is_epte_present(&old_entry) )
         ept_free_entry(p2m, &old_entry, target);
 
     return rc;
@@ -893,7 +953,7 @@ static void ept_change_entry_type_global(struct p2m_domain *p2m,
     if ( !mfn )
         return;
 
-    if ( ept_invalidate_emt(_mfn(mfn), 1) )
+    if ( ept_invalidate_emt(_mfn(mfn), 1, ept_get_wl(&p2m->ept)) )
         ept_sync_domain(p2m);
 }
 
@@ -951,7 +1011,7 @@ static void ept_memory_type_changed(struct p2m_domain *p2m)
     if ( !mfn )
         return;
 
-    if ( ept_invalidate_emt(_mfn(mfn), 0) )
+    if ( ept_invalidate_emt(_mfn(mfn), 0, ept_get_wl(&p2m->ept)) )
         ept_sync_domain(p2m);
 }
 
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index cd9867a..a1794d0 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -513,6 +513,13 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
         __trace_var(TRC_MEM_SET_P2M_ENTRY, 0, sizeof(t), &t);
     }
 
+    if ( unlikely(p2m_is_foreign(p2mt)) )
+    {
+        /* pvh fixme: foreign types are only supported on ept at present */
+        gdprintk(XENLOG_WARNING, "Unimplemented foreign p2m type.\n");
+        return -EINVAL;
+    }
+
     /* Carry out any eventually pending earlier changes first. */
     rc = do_recalc(p2m, gfn);
     if ( rc < 0 )
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b50747a..642ec28 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -36,6 +36,7 @@
 #include <xen/event.h>
 #include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/svm/amd-iommu-proto.h>
+#include <xsm/xsm.h>
 
 #include "mm-locks.h"
 
@@ -311,14 +312,20 @@ struct page_info *get_page_from_gfn_p2m(
         /* Fast path: look up and get out */
         p2m_read_lock(p2m);
         mfn = __get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0);
-        if ( (p2m_is_ram(*t) || p2m_is_grant(*t))
-             && mfn_valid(mfn)
+        if ( p2m_is_any_ram(*t) && mfn_valid(mfn)
              && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
         {
             page = mfn_to_page(mfn);
-            if ( !get_page(page, d)
-                 /* Page could be shared */
-                 && !get_page(page, dom_cow) )
+            if ( unlikely(p2m_is_foreign(*t)) )
+            {
+                struct domain *fdom = page_get_owner_and_reference(page);
+                ASSERT(fdom != d);
+                if ( fdom == NULL )
+                    page = NULL;
+            }
+            else if ( !get_page(page, d)
+                      /* Page could be shared */
+                      && !get_page(page, dom_cow) )
                 page = NULL;
         }
         p2m_read_unlock(p2m);
@@ -468,6 +475,10 @@ int p2m_alloc_table(struct p2m_domain *p2m)
     return rc;
 }
 
+/*
+ * pvh fixme: when adding support for pvh non-hardware domains, this path must
+ * cleanup any foreign p2m types (release refcnts on them).
+ */
 void p2m_teardown(struct p2m_domain *p2m)
 /* Return all the p2m pages to Xen.
  * We know we don't have any extra mappings to these pages */
@@ -836,8 +847,8 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
 }
 
 /* Set foreign mfn in the given guest's p2m table. */
-static int __attribute__((unused))
-set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
+static int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
+                                 mfn_t mfn)
 {
     return set_typed_p2m_entry(d, gfn, mfn, p2m_map_foreign);
 }
@@ -1794,6 +1805,107 @@ out_p2m_audit:
 #endif /* P2M_AUDIT */
 
 /*
+ * Add frame from foreign domain to target domain's physmap. Similar to
+ * XENMAPSPACE_gmfn but the frame is foreign being mapped into current,
+ * and is not removed from foreign domain.
+ *
+ * Usage: - libxl on pvh dom0 creating a guest and doing privcmd_ioctl_mmap.
+ *        - xentrace running on dom0 mapping xenheap pages. foreigndom would
+ *          be DOMID_XEN in such a case.
+ *        etc..
+ *
+ * Side Effect: the mfn for fgfn will be refcounted in lower level routines
+ *              so it is not lost while mapped here. The refcnt is released
+ *              via the XENMEM_remove_from_physmap path.
+ *
+ * Returns: 0 ==> success
+ */
+int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
+                    unsigned long gpfn, domid_t foreigndom)
+{
+    p2m_type_t p2mt, p2mt_prev;
+    unsigned long prev_mfn, mfn;
+    struct page_info *page;
+    int rc;
+    struct domain *fdom;
+
+    ASSERT(tdom);
+    if ( foreigndom == DOMID_SELF || !is_pvh_domain(tdom) )
+        return -EINVAL;
+    /*
+     * pvh fixme: until support is added to p2m teardown code to cleanup any
+     * foreign entries, limit this to hardware domain only.
+     */
+    if ( !is_hardware_domain(tdom) )
+        return -EPERM;
+
+    if ( foreigndom == DOMID_XEN )
+        fdom = rcu_lock_domain(dom_xen);
+    else
+        fdom = rcu_lock_domain_by_id(foreigndom);
+    if ( fdom == NULL )
+        return -ESRCH;
+
+    rc = -EINVAL;
+    if ( tdom == fdom )
+        goto out;
+
+    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
+    if ( rc )
+        goto out;
+
+    /*
+     * Take a refcnt on the mfn. NB: following supported for foreign mapping:
+     *     ram_rw | ram_logdirty | ram_ro | paging_out.
+     */
+    page = get_page_from_gfn(fdom, fgfn, &p2mt, P2M_ALLOC);
+    if ( !page ||
+         !p2m_is_ram(p2mt) || p2m_is_shared(p2mt) || p2m_is_hole(p2mt) )
+    {
+        if ( page )
+            put_page(page);
+        rc = -EINVAL;
+        goto out;
+    }
+    mfn = mfn_x(page_to_mfn(page));
+
+    /* Remove previously mapped page if it is present. */
+    prev_mfn = mfn_x(get_gfn(tdom, gpfn, &p2mt_prev));
+    if ( mfn_valid(_mfn(prev_mfn)) )
+    {
+        if ( is_xen_heap_mfn(prev_mfn) )
+            /* Xen heap frames are simply unhooked from this phys slot */
+            guest_physmap_remove_page(tdom, gpfn, prev_mfn, 0);
+        else
+            /* Normal domain memory is freed, to avoid leaking memory. */
+            guest_remove_page(tdom, gpfn);
+    }
+    /*
+     * Create the new mapping. Can't use guest_physmap_add_page() because it
+     * will update the m2p table which will result in  mfn -> gpfn of dom0
+     * and not fgfn of domU.
+     */
+    rc = set_foreign_p2m_entry(tdom, gpfn, _mfn(mfn));
+    if ( rc )
+        gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
+                 "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
+                 gpfn, mfn, fgfn, tdom->domain_id, fdom->domain_id);
+
+    put_page(page);
+
+    /*
+     * This put_gfn for the above get_gfn for prev_mfn.  We must do this
+     * after set_foreign_p2m_entry so another cpu doesn't populate the gpfn
+     * before us.
+     */
+    put_gfn(tdom, gpfn);
+
+out:
+    if ( fdom )
+        rcu_unlock_domain(fdom);
+    return rc;
+}
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 027f011..d0cfdac 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -188,6 +188,10 @@ typedef unsigned int p2m_query_t;
 #define p2m_is_broken(_t)   (p2m_to_mask(_t) & P2M_BROKEN_TYPES)
 #define p2m_is_foreign(_t)  (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign))
 
+#define p2m_is_any_ram(_t)  (p2m_to_mask(_t) &                   \
+                             (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
+                              p2m_to_mask(p2m_map_foreign)))
+
 /* Per-p2m-table state */
 struct p2m_domain {
     /* Lock that protects updates to the p2m */
@@ -532,6 +536,9 @@ int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
 int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
 int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn);
 
+/* Add foreign mapping to the guest's p2m table. */
+int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
+                    unsigned long gpfn, domid_t foreign_domid);
 
 /* 
  * Populate-on-demand
-- 
1.8.3.1

Re: [V13 PATCH 1/2] pvh dom0: Add and remove foreign pages

[Jan Beulich]

>>> On 20.05.14 at 01:51, <mukesh.rathor@oracle.com> wrote:
> +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);
> +
> +    if ( level )
> +    {
> +        ASSERT(!is_epte_superpage(&new) || !p2m_is_foreign(new.sa_p2mt));
> +        write_atomic(&entryptr->epte, new.epte);
> +        goto out;
> +    }
> +
> +    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
> +    {
> +        struct domain *fdom;
> +
> +        rc = -EINVAL;
> +        if ( !mfn_valid(new.mfn) )
> +            goto out;
> +
> +        rc = -ESRCH;
> +        fdom = page_get_owner(mfn_to_page(new.mfn));
> +        if ( fdom == NULL )
> +            goto out;
> +
> +        /* get refcount on the page */
> +        rc = -EBUSY;
> +        if ( !get_page(mfn_to_page(new.mfn), fdom) )
> +            goto out;
> +    }
> +
> +    if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && !skip_foreign )
> +        oldmfn = entryptr->mfn;
> +
> +    write_atomic(&entryptr->epte, new.epte);
> +
> +    if ( unlikely(oldmfn != INVALID_MFN) )
> +        put_page(mfn_to_page(oldmfn));
> +
> +    rc = 0;
> +
> + out:
> +    if ( rc )
> +        gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64" rc:%d\n",
> +                 entryptr->epte, new.epte, rc);
> +    return rc;
> +}

There's still no sign of any use of is_epte_present() here, and also no
mention anywhere that taking refcounts even for inaccessible entries
is correct. I think this is actually okay, but the policy (refcount taken
even for inaccessible pages) should be spelled out somewhere.

> @@ -688,10 +748,10 @@ 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, target);

To me it would seem cleaner to clear old_entry here right away, so
there's no confusion about it needing freeing on eventual new error
paths getting added in the future.

Jan

Re: [V13 PATCH 1/2] pvh dom0: Add and remove foreign pages

[Mukesh Rathor]

On Tue, 20 May 2014 11:33:11 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 20.05.14 at 01:51, <mukesh.rathor@oracle.com> wrote:
> > +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);
> > +
> > +    if ( level )
> > +    {
> > +        ASSERT(!is_epte_superpage(&new)
> > || !p2m_is_foreign(new.sa_p2mt));
> > +        write_atomic(&entryptr->epte, new.epte);
> > +        goto out;
> > +    }
> > +
> > +    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
> > +    {
> > +        struct domain *fdom;
> > +
> > +        rc = -EINVAL;
> > +        if ( !mfn_valid(new.mfn) )
> > +            goto out;
> > +
> > +        rc = -ESRCH;
> > +        fdom = page_get_owner(mfn_to_page(new.mfn));
> > +        if ( fdom == NULL )
> > +            goto out;
> > +
> > +        /* get refcount on the page */
> > +        rc = -EBUSY;
> > +        if ( !get_page(mfn_to_page(new.mfn), fdom) )
> > +            goto out;
> > +    }
> > +
> > +    if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt))
> > && !skip_foreign )
> > +        oldmfn = entryptr->mfn;
> > +
> > +    write_atomic(&entryptr->epte, new.epte);
> > +
> > +    if ( unlikely(oldmfn != INVALID_MFN) )
> > +        put_page(mfn_to_page(oldmfn));
> > +
> > +    rc = 0;
> > +
> > + out:
> > +    if ( rc )
> > +        gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64"
> > rc:%d\n",
> > +                 entryptr->epte, new.epte, rc);
> > +    return rc;
> > +}
> 
> There's still no sign of any use of is_epte_present() here, and also
> no mention anywhere that taking refcounts even for inaccessible
> entries is correct. I think this is actually okay, but the policy

I thought we now have no paths leading to that!  Can you please point 
me to a path that this would happen for foreign type?

In the interest of saving time, are you looking-for/ok-with 
something like:

    unsigned int operm = entryptr->epte & 0x7, nperm = new.epte & 0x7;
    bool_t skip_foreign = (new.mfn == entryptr->mfn &&
                           new.sa_p2mt == entryptr->sa_p2mt &&
                           operm == nperm);
...
    if ( level )
    {
        ASSERT(!is_epte_superpage(&new) || !p2m_is_foreign(new.sa_p2mt));
        write_atomic(&entryptr->epte, new.epte);
        goto out;
    }

    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
    {
        struct domain *fdom;

        rc = -EINVAL;
        if ( !mfn_valid(new.mfn) ||  !is_epte_present(&new) ) <====
            goto out;
..

thereby just rejecting non-present for foreign types. 

> entries is correct. I think this is actually okay, but the policy
> (refcount taken even for inaccessible pages) should be spelled out
> somewhere.

Can you please point to where this is spelled out for grant types?
They are very similar to foreign types.


> > @@ -688,10 +748,10 @@ 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, target);
> 
> To me it would seem cleaner to clear old_entry here right away, so
> there's no confusion about it needing freeing on eventual new error
> paths getting added in the future.

Not sure I understand why. If the error happens only before the entry
is ever written, leaving the old entry seems reasonable. IOW, if going
from A to B, if there's an error, nothing is changed, A is still around.
Clearing the old entry may make things worse, specially if clearing the 
entry needs any special handling, like clearing old refcnt etc.. Having
an api change state from A to C when failing to set to B seems odd to me.


thanks
mukesh

Re: [V13 PATCH 1/2] pvh dom0: Add and remove foreign pages

[Jan Beulich]

>>> On 21.05.14 at 01:46, <mukesh.rathor@oracle.com> wrote:
> On Tue, 20 May 2014 11:33:11 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 20.05.14 at 01:51, <mukesh.rathor@oracle.com> wrote:
>> > +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);
>> > +
>> > +    if ( level )
>> > +    {
>> > +        ASSERT(!is_epte_superpage(&new)
>> > || !p2m_is_foreign(new.sa_p2mt));
>> > +        write_atomic(&entryptr->epte, new.epte);
>> > +        goto out;
>> > +    }
>> > +
>> > +    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
>> > +    {
>> > +        struct domain *fdom;
>> > +
>> > +        rc = -EINVAL;
>> > +        if ( !mfn_valid(new.mfn) )
>> > +            goto out;
>> > +
>> > +        rc = -ESRCH;
>> > +        fdom = page_get_owner(mfn_to_page(new.mfn));
>> > +        if ( fdom == NULL )
>> > +            goto out;
>> > +
>> > +        /* get refcount on the page */
>> > +        rc = -EBUSY;
>> > +        if ( !get_page(mfn_to_page(new.mfn), fdom) )
>> > +            goto out;
>> > +    }
>> > +
>> > +    if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt))
>> > && !skip_foreign )
>> > +        oldmfn = entryptr->mfn;
>> > +
>> > +    write_atomic(&entryptr->epte, new.epte);
>> > +
>> > +    if ( unlikely(oldmfn != INVALID_MFN) )
>> > +        put_page(mfn_to_page(oldmfn));
>> > +
>> > +    rc = 0;
>> > +
>> > + out:
>> > +    if ( rc )
>> > +        gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64"
>> > rc:%d\n",
>> > +                 entryptr->epte, new.epte, rc);
>> > +    return rc;
>> > +}
>> 
>> There's still no sign of any use of is_epte_present() here, and also
>> no mention anywhere that taking refcounts even for inaccessible
>> entries is correct. I think this is actually okay, but the policy
> 
> I thought we now have no paths leading to that!  Can you please point 
> me to a path that this would happen for foreign type?

No I can't - as implied by the rest of the response seen below. _If_
indeed there are no such paths, a respective ASSERT() might be the
way to go.

>> entries is correct. I think this is actually okay, but the policy
>> (refcount taken even for inaccessible pages) should be spelled out
>> somewhere.
> 
> Can you please point to where this is spelled out for grant types?
> They are very similar to foreign types.

Can you please stop using examples of bad situations as justification
for adding further badness? But then again, it's Tim to decide whether
he's fine with all sorts of things being implicit rather than explicit here.

Apart from that the question would need to be raised to those not
having spelled out properly what the rules for grants are -
considering that it's the second switch() in ept_p2m_type_to_flags()
that can actually lead to non-present entries associated with a valid
MFN, it's likely the addition of mem_access to blame here.

>> > @@ -688,10 +748,10 @@ 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, target);
>> 
>> To me it would seem cleaner to clear old_entry here right away, so
>> there's no confusion about it needing freeing on eventual new error
>> paths getting added in the future.
> 
> Not sure I understand why. If the error happens only before the entry
> is ever written, leaving the old entry seems reasonable. IOW, if going
> from A to B, if there's an error, nothing is changed, A is still around.
> Clearing the old entry may make things worse, specially if clearing the 
> entry needs any special handling, like clearing old refcnt etc.. Having
> an api change state from A to C when failing to set to B seems odd to me.

You're right with what you state, yet you apparently didn't spot that
I talked about "old_entry", since all your response refers to "old entry".
I.e. I was asking for the local variable to be cleared right away, not
an entry in the active table.

Jan

Re: [V13 PATCH 1/2] pvh dom0: Add and remove foreign pages

[Mukesh Rathor]

On Wed, 21 May 2014 08:59:27 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 21.05.14 at 01:46, <mukesh.rathor@oracle.com> wrote:
> > On Tue, 20 May 2014 11:33:11 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 20.05.14 at 01:51, <mukesh.rathor@oracle.com> wrote:
> >> > +static int atomic_write_ept_entry(ept_entry_t *entryptr,
..
> >> > + out:
> >> > +    if ( rc )
> >> > +        gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64"
> >> > rc:%d\n",
> >> > +                 entryptr->epte, new.epte, rc);
> >> > +    return rc;
> >> > +}
> >> 
> >> There's still no sign of any use of is_epte_present() here, and
> >> also no mention anywhere that taking refcounts even for
> >> inaccessible entries is correct. I think this is actually okay,
> >> but the policy
> > 
> > I thought we now have no paths leading to that!  Can you please
> > point me to a path that this would happen for foreign type?
> 
> No I can't - as implied by the rest of the response seen below. _If_
> indeed there are no such paths, a respective ASSERT() might be the
> way to go.
> 
> >> entries is correct. I think this is actually okay, but the policy
> >> (refcount taken even for inaccessible pages) should be spelled out
> >> somewhere.
> > 
> > Can you please point to where this is spelled out for grant types?
> > They are very similar to foreign types.
> 
> Can you please stop using examples of bad situations as justification
> for adding further badness? But then again, it's Tim to decide whether
> he's fine with all sorts of things being implicit rather than
> explicit here.

I feel by just failing atomic_write_ept_entry for non-present entries
we don't need to add any more is_pvh_* checks/asserts which in the past
have been frowned upon.

> Apart from that the question would need to be raised to those not
> having spelled out properly what the rules for grants are -
> considering that it's the second switch() in ept_p2m_type_to_flags()
> that can actually lead to non-present entries associated with a valid
> MFN, it's likely the addition of mem_access to blame here.

That would be denied for pvh as mem_event_enable() would fail for pvh
because only param allowed to be set for pvh is HVM_PARAM_CALLBACK_IRQ.

> >> > @@ -688,10 +748,10 @@ 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, target);
> >> 
> >> To me it would seem cleaner to clear old_entry here right away, so
> >> there's no confusion about it needing freeing on eventual new error
> >> paths getting added in the future.
> > 
> > Not sure I understand why. If the error happens only before the
> > entry is ever written, leaving the old entry seems reasonable. IOW,
> > if going from A to B, if there's an error, nothing is changed, A is
> > still around. Clearing the old entry may make things worse,
> > specially if clearing the entry needs any special handling, like
> > clearing old refcnt etc.. Having an api change state from A to C
> > when failing to set to B seems odd to me.
> 
> You're right with what you state, yet you apparently didn't spot that
> I talked about "old_entry", since all your response refers to "old
> entry". I.e. I was asking for the local variable to be cleared right
> away, not an entry in the active table.

Sorry, my eyes missed the underscore between old and entry... According
to the comment, it needs to be done after sync domain... I'd rather just
leave it as is for now, I'm unable to anticipate how the function might
change in future.

Sendig V14, please take a look.

thanks,
Mukesh

Re: [V13 PATCH 1/2] pvh dom0: Add and remove foreign pages

[Jan Beulich]

>>> On 22.05.14 at 03:15, <mukesh.rathor@oracle.com> wrote:
> On Wed, 21 May 2014 08:59:27 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
>> >>> On 21.05.14 at 01:46, <mukesh.rathor@oracle.com> wrote:
>> > On Tue, 20 May 2014 11:33:11 +0100
>> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> >> >>> On 20.05.14 at 01:51, <mukesh.rathor@oracle.com> wrote:
>> >> > @@ -688,10 +748,10 @@ 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, target);
>> >> 
>> >> To me it would seem cleaner to clear old_entry here right away, so
>> >> there's no confusion about it needing freeing on eventual new error
>> >> paths getting added in the future.
>> > 
>> > Not sure I understand why. If the error happens only before the
>> > entry is ever written, leaving the old entry seems reasonable. IOW,
>> > if going from A to B, if there's an error, nothing is changed, A is
>> > still around. Clearing the old entry may make things worse,
>> > specially if clearing the entry needs any special handling, like
>> > clearing old refcnt etc.. Having an api change state from A to C
>> > when failing to set to B seems odd to me.
>> 
>> You're right with what you state, yet you apparently didn't spot that
>> I talked about "old_entry", since all your response refers to "old
>> entry". I.e. I was asking for the local variable to be cleared right
>> away, not an entry in the active table.
> 
> Sorry, my eyes missed the underscore between old and entry... According
> to the comment, it needs to be done after sync domain... I'd rather just
> leave it as is for now, I'm unable to anticipate how the function might
> change in future.

I think you still didn't understand what I would like to be done:
Rather than adding an rc check to where old_entry is being
checked for presence (to determine whether to call
ept_free_entry() - that check is pointless anyway) I'd like you to
zap old_entry (read: old_entry.epte = 0) right away when the
above atomic_write_ept_entry() fails.

Jan

Re: [V13 PATCH 1/2] pvh dom0: Add and remove foreign pages

[Mukesh Rathor]

On Thu, 22 May 2014 08:33:47 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 22.05.14 at 03:15, <mukesh.rathor@oracle.com> wrote:
> > On Wed, 21 May 2014 08:59:27 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> >> >>> On 21.05.14 at 01:46, <mukesh.rathor@oracle.com> wrote:
> >> > On Tue, 20 May 2014 11:33:11 +0100
> >> > "Jan Beulich" <JBeulich@suse.com> wrote:
> >> >> >>> On 20.05.14 at 01:51, <mukesh.rathor@oracle.com> wrote:
> >> >> > @@ -688,10 +748,10 @@ 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,
> >> >> > target);
> >> >> 
> >> >> To me it would seem cleaner to clear old_entry here right away,
> >> >> so there's no confusion about it needing freeing on eventual
> >> >> new error paths getting added in the future.
> >> > 
> >> > Not sure I understand why. If the error happens only before the
> >> > entry is ever written, leaving the old entry seems reasonable.
> >> > IOW, if going from A to B, if there's an error, nothing is
> >> > changed, A is still around. Clearing the old entry may make
> >> > things worse, specially if clearing the entry needs any special
> >> > handling, like clearing old refcnt etc.. Having an api change
> >> > state from A to C when failing to set to B seems odd to me.
> >> 
> >> You're right with what you state, yet you apparently didn't spot
> >> that I talked about "old_entry", since all your response refers to
> >> "old entry". I.e. I was asking for the local variable to be
> >> cleared right away, not an entry in the active table.
> > 
> > Sorry, my eyes missed the underscore between old and entry...
> > According to the comment, it needs to be done after sync domain...
> > I'd rather just leave it as is for now, I'm unable to anticipate
> > how the function might change in future.
> 
> I think you still didn't understand what I would like to be done:
> Rather than adding an rc check to where old_entry is being
> checked for presence (to determine whether to call
> ept_free_entry() - that check is pointless anyway) I'd like you to
> zap old_entry (read: old_entry.epte = 0) right away when the
> above atomic_write_ept_entry() fails.
> 
> Jan
> 

Got it. V15 with that change.

thanks,
mukesh

[V13 PATCH 2/2] dom0: add opt_dom0pvh to setup.c

[Mukesh Rathor]

Finally last patch in the series to enable creation of pvh dom0.
A pvh dom0 is created by adding dom0pvh to grub xen command line.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 docs/misc/pvh-readme.txt            |  2 ++
 docs/misc/xen-command-line.markdown |  7 +++++++
 xen/arch/x86/setup.c                | 11 +++++++++--
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/docs/misc/pvh-readme.txt b/docs/misc/pvh-readme.txt
index 9fea137..c5b3de4 100644
--- a/docs/misc/pvh-readme.txt
+++ b/docs/misc/pvh-readme.txt
@@ -37,6 +37,8 @@ supported. Phase I patches are broken into three parts:
    - tools changes for creating a PVH guest
    - boot of 64bit dom0 in PVH mode.
 
+To boot 64bit dom0 in PVH mode, add dom0pvh to grub xen command line.
+
 Following fixme's exist in the code:
   - arch/x86/time.c: support more tsc modes.
 
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index a7ac53d..b45ba7e 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -494,6 +494,13 @@ Practices](http://wiki.xen.org/wiki/Xen_Best_Practices#Xen_dom0_dedicated_memory
 
 Pin dom0 vcpus to their respective pcpus
 
+### dom0pvh
+> `= <boolean>`
+
+> Default: `false`
+
+Flag that makes a 64bit dom0 boot in PVH mode. No 32bit support at present.
+
 ### e820-mtrr-clip
 > `= <boolean>`
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d8598a3..8e9db9e 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -65,6 +65,10 @@ invbool_param("smep", disable_smep);
 static bool_t __initdata disable_smap;
 invbool_param("smap", disable_smap);
 
+/* Boot dom0 in pvh mode */
+static bool_t __initdata opt_dom0pvh;
+boolean_param("dom0pvh", opt_dom0pvh);
+
 /* **** Linux config option: propagated to domain0. */
 /* "acpi=off":    Sisables both ACPI table parsing and interpreter. */
 /* "acpi=force":  Override the disable blacklist.                   */
@@ -545,7 +549,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 {
     char *memmap_type = NULL;
     char *cmdline, *kextra, *loader;
-    unsigned int initrdidx;
+    unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
     multiboot_info_t *mbi = __va(mbi_p);
     module_t *mod = (module_t *)__va(mbi->mods_addr);
     unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
@@ -1347,8 +1351,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     if ( !tboot_protect_mem_regions() )
         panic("Could not protect TXT memory regions");
 
+    if ( opt_dom0pvh )
+        domcr_flags |= DOMCRF_pvh | DOMCRF_hap;
+
     /* Create initial domain 0. */
-    dom0 = domain_create(0, DOMCRF_s3_integrity, 0);
+    dom0 = domain_create(0, domcr_flags, 0);
     if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
         panic("Error creating domain 0");
 
-- 
1.8.3.1

Re: [V13 PATCH 0/2] pvh dom0 patches...

[Jan Beulich]

>>> On 20.05.14 at 01:51, <mukesh.rathor@oracle.com> wrote:
> Attached please find v13 of dom0 pvh patch series based on 
> c/s: 11dba84.

I continue to be confused by these statements: Saying that a series
is based on a week old c/s may mean two things - either it is not meant
to be applied even if otherwise considered good (in which case I would
think it should be marked RFC), or it is meant to be applied and you
leave it to the eventual committer to find out if it actually applies
(which isn't really the way things ought to work, i.e. it should be you
to make sure that it still applies fine to the tip of the tree at the time
of submission and no other conflicting changes went in). Please clarify.

Jan

Re: [V13 PATCH 0/2] pvh dom0 patches...

[Roger Pau Monné]

On 20/05/14 01:51, Mukesh Rathor wrote:
> Hi,
> 
> Attached please find v13 of dom0 pvh patch series based on 
> c/s: 11dba84.
> 
> git tree: git://oss.oracle.com/git/mrathor/xen.git  branch: dom0pvh-v13

I think you forgot to push the series to the git repo (or at least I'm
not able to see branch dom0pvh-v13).

Thanks, Roger.

Re: [V13 PATCH 0/2] pvh dom0 patches...

[Roger Pau Monné]

On 22/05/14 19:44, Roger Pau Monné wrote:
> On 20/05/14 01:51, Mukesh Rathor wrote:
>> Hi,
>>
>> Attached please find v13 of dom0 pvh patch series based on 
>> c/s: 11dba84.
>>
>> git tree: git://oss.oracle.com/git/mrathor/xen.git  branch: dom0pvh-v13
> 
> I think you forgot to push the series to the git repo (or at least I'm
> not able to see branch dom0pvh-v13).

Also a Linux branch that works as PVH Dom0 would be interesting to me,
since I've been hitting the vioapic crash when creating HVM guests with
Linux 3.14 plus your patch to map foreign memory.

Roger.

Re: [V13 PATCH 0/2] pvh dom0 patches...

[Mukesh Rathor]

On Thu, 22 May 2014 20:19:54 +0200
Roger Pau Monné <roger.pau@citrix.com> wrote:

> On 22/05/14 19:44, Roger Pau Monné wrote:
> > On 20/05/14 01:51, Mukesh Rathor wrote:
> >> Hi,
> >>
> >> Attached please find v13 of dom0 pvh patch series based on 
> >> c/s: 11dba84.
> >>
> >> git tree: git://oss.oracle.com/git/mrathor/xen.git  branch:
> >> dom0pvh-v13
> > 
> > I think you forgot to push the series to the git repo (or at least
> > I'm not able to see branch dom0pvh-v13).
> Also a Linux branch that works as PVH Dom0 would be interesting to me,

Hey Roger,

git://oss.oracle.com/git/mrathor/xen.git  branch: dom0pvh-v15
git://oss.oracle.com/git/mrathor/linux.git  branch: dom0pvh-1

> since I've been hitting the vioapic crash when creating HVM guests

Just apply following patch to get around vioapic crash so you can debug 
your real ept issue:

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index b69f164..c687d18 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2079,6 +2079,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
          (access_w && (p2mt == p2m_ram_ro)) )
     {
         put_gfn(p2m->domain, gfn);
+
+        rc = 0;
+        if ( unlikely(is_pvh_vcpu(v)) )
+            goto out;
+
         if ( !handle_mmio() )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         rc = 1;


thanks,
Mukesh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [V13 PATCH 0/2] pvh dom0 patches...

[Roger Pau Monné]

On 23/05/14 03:35, Mukesh Rathor wrote:
> On Thu, 22 May 2014 20:19:54 +0200
> Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
>> On 22/05/14 19:44, Roger Pau Monné wrote:
>>> On 20/05/14 01:51, Mukesh Rathor wrote:
>>>> Hi,
>>>>
>>>> Attached please find v13 of dom0 pvh patch series based on 
>>>> c/s: 11dba84.
>>>>
>>>> git tree: git://oss.oracle.com/git/mrathor/xen.git  branch:
>>>> dom0pvh-v13
>>>
>>> I think you forgot to push the series to the git repo (or at least
>>> I'm not able to see branch dom0pvh-v13).
>> Also a Linux branch that works as PVH Dom0 would be interesting to me,
> 
> Hey Roger,
> 
> git://oss.oracle.com/git/mrathor/xen.git  branch: dom0pvh-v15
> git://oss.oracle.com/git/mrathor/linux.git  branch: dom0pvh-1
> 
>> since I've been hitting the vioapic crash when creating HVM guests
> 
> Just apply following patch to get around vioapic crash so you can debug 
> your real ept issue:

Thanks, I think your Linux side patch is missing something like:

diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 8efc066..690b289 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -2533,7 +2533,7 @@ static int xlate_add_to_p2m(unsigned long lpfn,
unsigned long fgmfn,
        set_xen_guest_handle(xatp.errs, &err);

        rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
-       return rc;
+       return rc ? : err;
 }

Or else the error value is lost and Linux thinks this page is actually
mapped when it is not, trigger the vioapic crash when trying to access
it. Without this fix I cannot boot HVM domains, with the fix it seems to
be quite stable AFAICT.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel