[V12 PATCH 0/4] pvh dom0 patches...

[V12 PATCH 0/4] pvh dom0 patches...

[Mukesh Rathor]

Hi,

Centimeters to the finish line with the champagne bottle on the desk
staring at me,  please find v12 of the pvh dom0 patches based on 
c/s: 9ce8b15

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

Changed in v12 from v11:
  patch 1: none

  patch 2:
     - Because of p2m_is_changeable change in p2m_change_type_range, BUG_ON
       for foreign type is no longer needed.
     - ept_change_entry_type_global() is redone, so remove BUG_ON.

  patch 3:
     - re merge changes in p2m-ept.c as lot of emt functions are changed on
       refresh.
     - add ASSERT in atomic_write_ept_entry and add skip_foriegn.
     - Revert back to using rcu* and not export get_pg_owner in p2m_add_foreign
     - fix rc being unset in p2m_add_foreign if get_page_from_gfn fails.

  patch 4: none

thanks,
Mukesh

[V12 PATCH 1/4] pvh dom0: construct_dom0 changes

[Mukesh Rathor]

This patch changes construct_dom0() to boot in pvh mode:
  - Make sure dom0 elf supports pvh mode.
  - Call guest_physmap_add_page for pvh rather than simple p2m setting
  - Map all non-RAM regions 1:1 upto the end region in e820 or 4GB which
    ever is higher.
  - Allocate p2m, copying calculation from toolstack.
  - Allocate shared info page from the virtual space so that dom0 PT
    can be updated. Then update p2m for it with the actual mfn.
  - Since we build the page tables for pvh same as for pv, in
    pvh_fixup_page_tables_for_hap we replace the mfns with pfns.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/domain_build.c | 249 +++++++++++++++++++++++++++++++++++++++++---
 xen/arch/x86/mm/hap/hap.c   |  12 +++
 xen/include/asm-x86/hap.h   |   1 +
 3 files changed, 245 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 1eccead..38ed9f6 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -35,6 +35,7 @@
 #include <asm/setup.h>
 #include <asm/bzimage.h> /* for bzimage_parse */
 #include <asm/io_apic.h>
+#include <asm/hap.h>
 
 #include <public/version.h>
 
@@ -307,6 +308,158 @@ static void __init process_dom0_ioports_disable(void)
     }
 }
 
+static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
+                                       unsigned long mfn, unsigned long nr_mfns)
+{
+    unsigned long i;
+    int rc;
+
+    for ( i = 0; i < nr_mfns; i++ )
+        if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i))) )
+            panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
+                  gfn, mfn, i, rc);
+}
+
+/*
+ * Set the 1:1 map for all non-RAM regions for dom 0. Thus, dom0 will have
+ * the entire io region mapped in the EPT/NPT.
+ *
+ * pvh fixme: The following doesn't map MMIO ranges when they sit above the
+ *            highest E820 covered address.
+ */
+static __init void pvh_map_all_iomem(struct domain *d)
+{
+    unsigned long start_pfn, end_pfn, end = 0, start = 0;
+    const struct e820entry *entry;
+    unsigned int i, nump;
+
+    for ( i = 0, entry = e820.map; i < e820.nr_map; i++, entry++ )
+    {
+        end = entry->addr + entry->size;
+
+        if ( entry->type == E820_RAM || entry->type == E820_UNUSABLE ||
+             i == e820.nr_map - 1 )
+        {
+            start_pfn = PFN_DOWN(start);
+
+            /* Unused RAM areas are marked UNUSABLE, so skip them too */
+            if ( entry->type == E820_RAM || entry->type == E820_UNUSABLE )
+                end_pfn = PFN_UP(entry->addr);
+            else
+                end_pfn = PFN_UP(end);
+
+            if ( start_pfn < end_pfn )
+            {
+                nump = end_pfn - start_pfn;
+                /* Add pages to the mapping */
+                pvh_add_mem_mapping(d, start_pfn, start_pfn, nump);
+            }
+            start = end;
+        }
+    }
+
+    /*
+     * Some BIOSes may not report io space above ram that is less than 4GB. So
+     * we map any non-ram upto 4GB.
+     */
+    if ( end < GB(4) )
+    {
+        start_pfn = PFN_UP(end);
+        end_pfn = (GB(4)) >> PAGE_SHIFT;
+        nump = end_pfn - start_pfn;
+        pvh_add_mem_mapping(d, start_pfn, start_pfn, nump);
+    }
+}
+
+static __init void dom0_update_physmap(struct domain *d, unsigned long pfn,
+                                   unsigned long mfn, unsigned long vphysmap_s)
+{
+    if ( is_pvh_domain(d) )
+    {
+        int rc = guest_physmap_add_page(d, pfn, mfn, 0);
+        BUG_ON(rc);
+        return;
+    }
+    if ( !is_pv_32on64_domain(d) )
+        ((unsigned long *)vphysmap_s)[pfn] = mfn;
+    else
+        ((unsigned int *)vphysmap_s)[pfn] = mfn;
+
+    set_gpfn_from_mfn(mfn, pfn);
+}
+
+/* Replace mfns with pfns in dom0 page tables */
+static __init void pvh_fixup_page_tables_for_hap(struct vcpu *v,
+                                                 unsigned long v_start,
+                                                 unsigned long v_end)
+{
+    int i, j, k;
+    l4_pgentry_t *pl4e, *l4start;
+    l3_pgentry_t *pl3e;
+    l2_pgentry_t *pl2e;
+    l1_pgentry_t *pl1e;
+    unsigned long cr3_pfn;
+
+    ASSERT(paging_mode_enabled(v->domain));
+
+    l4start = map_domain_page(pagetable_get_pfn(v->arch.guest_table));
+
+    /* Clear entries prior to guest L4 start */
+    pl4e = l4start + l4_table_offset(v_start);
+    memset(l4start, 0, (unsigned long)pl4e - (unsigned long)l4start);
+
+    for ( ; pl4e <= l4start + l4_table_offset(v_end - 1); pl4e++ )
+    {
+        pl3e = map_l3t_from_l4e(*pl4e);
+        for ( i = 0; i < PAGE_SIZE / sizeof(*pl3e); i++, pl3e++ )
+        {
+            if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) )
+                continue;
+
+            pl2e = map_l2t_from_l3e(*pl3e);
+            for ( j = 0; j < PAGE_SIZE / sizeof(*pl2e); j++, pl2e++ )
+            {
+                if ( !(l2e_get_flags(*pl2e)  & _PAGE_PRESENT) )
+                    continue;
+
+                pl1e = map_l1t_from_l2e(*pl2e);
+                for ( k = 0; k < PAGE_SIZE / sizeof(*pl1e); k++, pl1e++ )
+                {
+                    if ( !(l1e_get_flags(*pl1e) & _PAGE_PRESENT) )
+                        continue;
+
+                    *pl1e = l1e_from_pfn(get_gpfn_from_mfn(l1e_get_pfn(*pl1e)),
+                                         l1e_get_flags(*pl1e));
+                }
+                unmap_domain_page(pl1e);
+                *pl2e = l2e_from_pfn(get_gpfn_from_mfn(l2e_get_pfn(*pl2e)),
+                                     l2e_get_flags(*pl2e));
+            }
+            unmap_domain_page(pl2e);
+            *pl3e = l3e_from_pfn(get_gpfn_from_mfn(l3e_get_pfn(*pl3e)),
+                                 l3e_get_flags(*pl3e));
+        }
+        unmap_domain_page(pl3e);
+        *pl4e = l4e_from_pfn(get_gpfn_from_mfn(l4e_get_pfn(*pl4e)),
+                             l4e_get_flags(*pl4e));
+    }
+
+    /* Clear entries post guest L4. */
+    if ( (unsigned long)pl4e & (PAGE_SIZE - 1) )
+        memset(pl4e, 0, PAGE_SIZE - ((unsigned long)pl4e & (PAGE_SIZE - 1)));
+
+    unmap_domain_page(l4start);
+
+    cr3_pfn = get_gpfn_from_mfn(paddr_to_pfn(v->arch.cr3));
+    v->arch.hvm_vcpu.guest_cr[3] = pfn_to_paddr(cr3_pfn);
+
+    /*
+     * Finally, we update the paging modes (hap_update_paging_modes). This will
+     * create monitor_table for us, update v->arch.cr3, and update vmcs.cr3.
+     */
+    paging_update_paging_modes(v);
+}
+
 static __init void mark_pv_pt_pages_rdonly(struct domain *d,
                                            l4_pgentry_t *l4start,
                                            unsigned long vpt_start,
@@ -516,6 +669,8 @@ int __init construct_dom0(
     l3_pgentry_t *l3tab = NULL, *l3start = NULL;
     l2_pgentry_t *l2tab = NULL, *l2start = NULL;
     l1_pgentry_t *l1tab = NULL, *l1start = NULL;
+    paddr_t shared_info_paddr = 0;
+    u32 save_pvh_pg_mode = 0;
 
     /*
      * This fully describes the memory layout of the initial domain. All 
@@ -593,12 +748,21 @@ int __init construct_dom0(
         goto out;
     }
 
-    if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE &&
-         !test_bit(XENFEAT_dom0, parms.f_supported) )
+    if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE )
     {
-        printk("Kernel does not support Dom0 operation\n");
-        rc = -EINVAL;
-        goto out;
+        if ( !test_bit(XENFEAT_dom0, parms.f_supported) )
+        {
+            printk("Kernel does not support Dom0 operation\n");
+            rc = -EINVAL;
+            goto out;
+        }
+        if ( is_pvh_domain(d) &&
+             !test_bit(XENFEAT_hvm_callback_vector, parms.f_supported) )
+        {
+            printk("Kernel does not support PVH mode\n");
+            rc = -EINVAL;
+            goto out;
+        }
     }
 
     if ( compat32 )
@@ -663,6 +827,13 @@ int __init construct_dom0(
     vstartinfo_end   = (vstartinfo_start +
                         sizeof(struct start_info) +
                         sizeof(struct dom0_vga_console_info));
+
+    if ( is_pvh_domain(d) )
+    {
+        shared_info_paddr = round_pgup(vstartinfo_end) - v_start;
+        vstartinfo_end   += PAGE_SIZE;
+    }
+
     vpt_start        = round_pgup(vstartinfo_end);
     for ( nr_pt_pages = 2; ; nr_pt_pages++ )
     {
@@ -903,6 +1074,13 @@ int __init construct_dom0(
         (void)alloc_vcpu(d, i, cpu);
     }
 
+    /*
+     * pvh: we temporarily disable d->arch.paging.mode so that we can build cr3
+     * needed to run on dom0's page tables.
+     */
+    save_pvh_pg_mode = d->arch.paging.mode;
+    d->arch.paging.mode = 0;
+
     /* Set up CR3 value for write_ptbase */
     if ( paging_mode_enabled(d) )
         paging_update_paging_modes(v);
@@ -969,6 +1147,22 @@ int __init construct_dom0(
                          nr_pages);
     }
 
+    if ( is_pvh_domain(d) )
+    {
+        unsigned long hap_pages, memkb = nr_pages * (PAGE_SIZE / 1024);
+
+        /* Copied from: libxl_get_required_shadow_memory() */
+        memkb = 4 * (256 * d->max_vcpus + 2 * (memkb / 1024));
+        hap_pages = ( (memkb + 1023) / 1024) << (20 - PAGE_SHIFT);
+        hap_set_alloc_for_pvh_dom0(d, hap_pages);
+    }
+
+    /*
+     * We enable paging mode again so guest_physmap_add_page will do the
+     * right thing for us.
+     */
+    d->arch.paging.mode = save_pvh_pg_mode;
+
     /* Write the phys->machine and machine->phys table entries. */
     for ( pfn = 0; pfn < count; pfn++ )
     {
@@ -985,11 +1179,7 @@ int __init construct_dom0(
         if ( pfn > REVERSE_START && (vinitrd_start || pfn < initrd_pfn) )
             mfn = alloc_epfn - (pfn - REVERSE_START);
 #endif
-        if ( !is_pv_32on64_domain(d) )
-            ((unsigned long *)vphysmap_start)[pfn] = mfn;
-        else
-            ((unsigned int *)vphysmap_start)[pfn] = mfn;
-        set_gpfn_from_mfn(mfn, pfn);
+        dom0_update_physmap(d, pfn, mfn, vphysmap_start);
         if (!(pfn & 0xfffff))
             process_pending_softirqs();
     }
@@ -1005,8 +1195,8 @@ int __init construct_dom0(
             if ( !page->u.inuse.type_info &&
                  !get_page_and_type(page, d, PGT_writable_page) )
                 BUG();
-            ((unsigned long *)vphysmap_start)[pfn] = mfn;
-            set_gpfn_from_mfn(mfn, pfn);
+
+            dom0_update_physmap(d, pfn, mfn, vphysmap_start);
             ++pfn;
             if (!(pfn & 0xfffff))
                 process_pending_softirqs();
@@ -1026,11 +1216,7 @@ int __init construct_dom0(
 #ifndef NDEBUG
 #define pfn (nr_pages - 1 - (pfn - (alloc_epfn - alloc_spfn)))
 #endif
-            if ( !is_pv_32on64_domain(d) )
-                ((unsigned long *)vphysmap_start)[pfn] = mfn;
-            else
-                ((unsigned int *)vphysmap_start)[pfn] = mfn;
-            set_gpfn_from_mfn(mfn, pfn);
+            dom0_update_physmap(d, pfn, mfn, vphysmap_start);
 #undef pfn
             page++; pfn++;
             if (!(pfn & 0xfffff))
@@ -1054,6 +1240,15 @@ int __init construct_dom0(
         si->console.dom0.info_size = sizeof(struct dom0_vga_console_info);
     }
 
+    /*
+     * PVH: We need to update si->shared_info while we are on dom0 page tables,
+     * but need to defer the p2m update until after we have fixed up the
+     * page tables for PVH so that the m2p for the si pte entry returns
+     * correct pfn.
+     */
+    if ( is_pvh_domain(d) )
+        si->shared_info = shared_info_paddr;
+
     if ( is_pv_32on64_domain(d) )
         xlat_start_info(si, XLAT_start_info_console_dom0);
 
@@ -1087,8 +1282,15 @@ int __init construct_dom0(
     regs->eflags = X86_EFLAGS_IF;
 
     if ( opt_dom0_shadow )
+    {
+        if ( is_pvh_domain(d) )
+        {
+            printk("Unsupported option dom0_shadow for PVH\n");
+            return -EINVAL;
+        }
         if ( paging_enable(d, PG_SH_enable) == 0 ) 
             paging_update_paging_modes(v);
+    }
 
     if ( supervisor_mode_kernel )
     {
@@ -1179,6 +1381,19 @@ int __init construct_dom0(
         printk(" Xen warning: dom0 kernel broken ELF: %s\n",
                elf_check_broken(&elf));
 
+    if ( is_pvh_domain(d) )
+    {
+        /* finally, fixup the page table, replacing mfns with pfns */
+        pvh_fixup_page_tables_for_hap(v, v_start, v_end);
+
+        /* the pt has correct pfn for si, now update the mfn in the p2m */
+        mfn = virt_to_mfn(d->shared_info);
+        pfn = shared_info_paddr >> PAGE_SHIFT;
+        dom0_update_physmap(d, pfn, mfn, 0);
+
+        pvh_map_all_iomem(d);
+    }
+
     if ( d->domain_id == hardware_domid )
         iommu_hwdom_init(d);
 
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 38c01d6..21f57c1 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -599,6 +599,18 @@ int hap_domctl(struct domain *d, xen_domctl_shadow_op_t *sc,
     }
 }
 
+void __init hap_set_alloc_for_pvh_dom0(struct domain *d,
+                                       unsigned long hap_pages)
+{
+    int rc;
+
+    paging_lock(d);
+    rc = hap_set_allocation(d, hap_pages, NULL);
+    paging_unlock(d);
+
+    BUG_ON(rc);
+}
+
 static const struct paging_mode hap_paging_real_mode;
 static const struct paging_mode hap_paging_protected_mode;
 static const struct paging_mode hap_paging_pae_mode;
diff --git a/xen/include/asm-x86/hap.h b/xen/include/asm-x86/hap.h
index e03f983..5161927 100644
--- a/xen/include/asm-x86/hap.h
+++ b/xen/include/asm-x86/hap.h
@@ -63,6 +63,7 @@ int   hap_track_dirty_vram(struct domain *d,
                            XEN_GUEST_HANDLE_64(uint8) dirty_bitmap);
 
 extern const struct paging_mode *hap_paging_get_mode(struct vcpu *);
+void hap_set_alloc_for_pvh_dom0(struct domain *d, unsigned long num_pages);
 
 #endif /* XEN_HAP_H */
 
-- 
1.8.3.1

[V12 PATCH 2/4] pvh dom0: Add checks and restrictions for p2m_is_foreign

[Mukesh Rathor]

In this patch, we add some checks and restrictions in the relevant
p2m paths for p2m_is_foreign.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
Acked-by: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/mm/p2m.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 03e9ad5..894275e 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -577,6 +577,10 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
         return 0;
     }
 
+    /* foreign pages are added thru p2m_add_foreign */
+    if ( p2m_is_foreign(t) )
+        return -EINVAL;
+
     p2m_lock(p2m);
 
     P2M_DEBUG("adding gfn=%#lx mfn=%#lx\n", gfn, mfn);
@@ -611,9 +615,9 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
             omfn = p2m->get_entry(p2m, gfn + i, &ot, &a, 0, NULL);
             ASSERT(!p2m_is_shared(ot));
         }
-        if ( p2m_is_grant(ot) )
+        if ( p2m_is_grant(ot) || p2m_is_foreign(ot) )
         {
-            /* Really shouldn't be unmapping grant maps this way */
+            /* Really shouldn't be unmapping grant/foreign maps this way */
             domain_crash(d);
             p2m_unlock(p2m);
             
@@ -715,6 +719,7 @@ p2m_type_t p2m_change_type(struct domain *d, unsigned long gfn,
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
     BUG_ON(p2m_is_grant(ot) || p2m_is_grant(nt));
+    BUG_ON(p2m_is_foreign(ot) || p2m_is_foreign(nt));
 
     gfn_lock(p2m, gfn, 0);
 
@@ -801,7 +806,7 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
 
     gfn_lock(p2m, gfn, 0);
     omfn = p2m->get_entry(p2m, gfn, &ot, &a, 0, NULL);
-    if ( p2m_is_grant(ot) )
+    if ( p2m_is_grant(ot) || p2m_is_foreign(ot) )
     {
         p2m_unlock(p2m);
         domain_crash(d);
-- 
1.8.3.1

[V12 PATCH 3/4] 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.

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/p2m-ept.c |  75 ++++++++++++++++++++-------
 xen/arch/x86/mm/p2m-pt.c  |   7 +++
 xen/arch/x86/mm/p2m.c     | 126 +++++++++++++++++++++++++++++++++++++++++++---
 xen/include/asm-x86/p2m.h |   7 +++
 5 files changed, 194 insertions(+), 25 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/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index bb98945..73f41ac 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,48 @@ 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)
+{
+    unsigned long oldmfn = INVALID_MFN;
+    bool_t skip_foreign = (new.mfn == entryptr->mfn &&
+                           new.sa_p2mt == entryptr->sa_p2mt);
+
+    if ( level )
+    {
+        ASSERT(!new.sp || !p2m_is_foreign(new.sa_p2mt));
+        write_atomic(&entryptr->epte, new.epte);
+        return 0;
+    }
+
+    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
+    {
+        struct domain *fdom;
+
+        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;
+    }
+
+    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));
+
+    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 */
@@ -275,7 +315,7 @@ 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)
 {
     ept_entry_t *epte = map_domain_page(mfn_x(mfn));
     unsigned int i;
@@ -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);
         changed = 1;
     }
 
@@ -342,7 +382,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
             rc = -ENOMEM;
             goto out;
         }
-        atomic_write_ept_entry(&table[index], split_ept_entry);
+        atomic_write_ept_entry(&table[index], split_ept_entry, i);
 
         for ( ; i > target; --i )
             if ( !ept_next_level(p2m, 1, &table, &gfn_remainder, i) )
@@ -361,7 +401,7 @@ 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);
+            atomic_write_ept_entry(&table[index], e, target);
             rc = 1;
         }
     }
@@ -431,7 +471,7 @@ 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);
+                    atomic_write_ept_entry(&epte[i], e, level);
                 }
             }
             else
@@ -465,7 +505,7 @@ 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);
+                        atomic_write_ept_entry(&epte[i], e, level);
                         unmap_domain_page(epte);
                         mfn = e.mfn;
                         continue;
@@ -479,7 +519,7 @@ 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);
+                atomic_write_ept_entry(&epte[i], e, level);
             }
 
             rc = 1;
@@ -489,11 +529,11 @@ 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);
+            atomic_write_ept_entry(&epte[i], e, level);
             unmap_domain_page(epte);
             rc = 1;
         }
@@ -585,6 +625,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 +690,7 @@ 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);
+        atomic_write_ept_entry(ept_entry, split_ept_entry, i);
 
         /* then move to the level we want to make real changes */
         for ( ; i > target; i-- )
@@ -688,10 +729,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 +764,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 +934,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 +992,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 894275e..b932822 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 */
@@ -830,8 +841,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);
 }
@@ -1792,6 +1803,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 9d48775..2ff03a5 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: [V12 PATCH 3/4] pvh dom0: Add and remove foreign pages

[Jan Beulich]

>>> 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)?

> +
> +    if ( level )
> +    {
> +        ASSERT(!new.sp || !p2m_is_foreign(new.sa_p2mt));

Please use is_epte_superpage() here.

> @@ -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().

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...

Jan

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

[Mukesh Rathor]

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-- )

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

[Jan Beulich]

>>> On 13.05.14 at 03:02, <mukesh.rathor@oracle.com> wrote:
> 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.

I didn't say invalid, I said present (i.e. at least one of r, w, or x set).
For example, it needs to be carefully considered whether the second
of the two switch() statements in ept_p2m_type_to_flags() could
have any effect that would require references to be dropped when
all three flags end up being clear.

>> > @@ -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.

Okay, I agree with this argumentation for the moment (where
changes to/from p2m_foreign can only be explicit, and indeed only
through that one path). But the issue raised next may already
yield this invalid once addressed.

>> 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. 

Will it? Where is that being enforced? I just went down (as an
example) subarch_memory_op() -> do_mem_event_op() ->
mem_paging_memop() without spotting any restriction on either
d or current->domain. (I'm anyway surprised by the rather small
amount of matches 'grep -ir "pvh.*fixme"' turns up - did some get
lost in the many patch iterations?

Jan

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

[Mukesh Rathor]

On Tue, 13 May 2014 08:09:42 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 13.05.14 at 03:02, <mukesh.rathor@oracle.com> wrote:
> > 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.
> 
> I didn't say invalid, I said present (i.e. at least one of r, w, or x
> set). For example, it needs to be carefully considered whether the
> second of the two switch() statements in ept_p2m_type_to_flags() could
> have any effect that would require references to be dropped when
> all three flags end up being clear.

That is a valid concern. Here, it's no different than if the type was grant.
But looking at the callers, there's no such case where for grant/foreign
such would happen. However, if it pleases the court, I think it would 
be a good idea to add:

bool_t no_clear_p2mt = type == p2m_grant_map_rw || type == p2m_grant_map_ro || 
                       type == p2m_map_foreign;
...

Then, after second switch:
    BUG_ON(no_clear_p2mt && (entry->r | entry->w | entry->x == 0));

what do you think?


> >> > @@ -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.
> 
> Okay, I agree with this argumentation for the moment (where
> changes to/from p2m_foreign can only be explicit, and indeed only
> through that one path). But the issue raised next may already
> yield this invalid once addressed.
> 
> >> 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. 
> 
> Will it? Where is that being enforced? I just went down (as an

In p2m_add_foreign() we return -EINVAL if the foreign gfn is not one of:
       ram_rw | ram_logdirty | ram_ro | paging_out.

Also, patch 8ff5c1d added checks in set_typed_p2m_entry() and
p2m_change_type_one().

> example) subarch_memory_op() -> do_mem_event_op() ->
> mem_paging_memop() without spotting any restriction on either

Going down further one level, each of the subcalls enforce p2m types: 

  XENMEM_paging_op_nominate -> p2m_mem_paging_nominate():
       if ( !p2m_is_pageable(p2mt) )
           goto out;
etc...

> d or current->domain. (I'm anyway surprised by the rather small
> amount of matches 'grep -ir "pvh.*fixme"' turns up - did some get
> lost in the many patch iterations?

I don't think so, I was made to address lot of them :).. There are
couple in this patch.

thanks
mukesh

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

[Jan Beulich]

>>> On 14.05.14 at 02:55, <mukesh.rathor@oracle.com> wrote:
> On Tue, 13 May 2014 08:09:42 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 13.05.14 at 03:02, <mukesh.rathor@oracle.com> wrote:
>> > 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.
>> 
>> I didn't say invalid, I said present (i.e. at least one of r, w, or x
>> set). For example, it needs to be carefully considered whether the
>> second of the two switch() statements in ept_p2m_type_to_flags() could
>> have any effect that would require references to be dropped when
>> all three flags end up being clear.
> 
> That is a valid concern. Here, it's no different than if the type was grant.
> But looking at the callers, there's no such case where for grant/foreign
> such would happen. However, if it pleases the court, I think it would 
> be a good idea to add:
> 
> bool_t no_clear_p2mt = type == p2m_grant_map_rw || type == p2m_grant_map_ro 
> || 
>                        type == p2m_map_foreign;
> ...
> 
> Then, after second switch:
>     BUG_ON(no_clear_p2mt && (entry->r | entry->w | entry->x == 0));
> 
> what do you think?

I think scattering around new special casing is the wrong approach;
you may recall that I wasn't in favor of the lots of special case
additions elsewhere by the PVH patches. I think you first of all need
to take a step back and understand what the long term picture here
is supposed to be. Which features can and should be supported,
and which ones should be regarded as useless or unsupportable.

Once you determined that, whether the above is the right thing for
now (with a suitable fixme comment) will fall out almost naturally I
assume.

>> >> 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. 
>> 
>> Will it? Where is that being enforced? I just went down (as an
> 
> In p2m_add_foreign() we return -EINVAL if the foreign gfn is not one of:
>        ram_rw | ram_logdirty | ram_ro | paging_out.
> 
> Also, patch 8ff5c1d added checks in set_typed_p2m_entry() and
> p2m_change_type_one().

But that's way too late, isn't it? You ought to disallow paging/sharing
(and whatever else you can't support right now) from the beginning,
i.e. it shouldn't even get enabled on a DomU if the controlling domain
is PVH.

Jan

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

[Mukesh Rathor]

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

> >>> On 14.05.14 at 02:55, <mukesh.rathor@oracle.com> wrote:
> > On Tue, 13 May 2014 08:09:42 +0100
...
> >> >> 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. 
> >> 
> >> Will it? Where is that being enforced? I just went down (as an
> > 
> > In p2m_add_foreign() we return -EINVAL if the foreign gfn is not
> > one of: ram_rw | ram_logdirty | ram_ro | paging_out.
> > 
> > Also, patch 8ff5c1d added checks in set_typed_p2m_entry() and
> > p2m_change_type_one().
> 
> But that's way too late, isn't it? You ought to disallow
> paging/sharing (and whatever else you can't support right now) from
> the beginning, i.e. it shouldn't even get enabled on a DomU if the
> controlling domain is PVH.

Ah, I see. The concern here is an HVM gfn say p2m_ram_rw, being mapped 
foreign into dom0, then going into sharing/paging. Yes, we should just
disable/disallow from the beginning for now. Long term, we would need to 
keep track somehow of guest gfns that are mapped foreign so we could
just disallow operations on those...  I frankly see no reason to support 
these features for foreign types.

Code wise several options, but seems mem_event.c would be the best 
place to put checks (would ENOSYS be more appropriate?):

+++ b/xen/arch/x86/mm/mem_event.c
@@ -538,6 +538,13 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event
         case XEN_DOMCTL_MEM_EVENT_OP_PAGING_ENABLE:
         {
             struct p2m_domain *p2m = p2m_get_hostp2m(d);
+            struct domain *hwdom = rcu_lock_domain_by_id(hardware_domid);
+
+            rcu_unlock_domain(hwdom);
+            rc = -EOPNOTSUPP;
+            /* pvh fixme: support paging */
+            if ( is_pvh_domain(hwdom) )
+                break;
+
             rc = -ENODEV;
             /* Only HAP is supported */
             if ( !hap_enabled(d) )
@@ -620,6 +627,13 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event
         {
         case XEN_DOMCTL_MEM_EVENT_OP_SHARING_ENABLE:
         {
+            struct domain *hwdom = rcu_lock_domain_by_id(hardware_domid);
+
+            rcu_unlock_domain(hwdom);
+            rc = -EOPNOTSUPP;
+            /* pvh fixme: support sharing */
+            if ( is_pvh_domain(hwdom) )
+                break;
+
             rc = -ENODEV;
             /* Only HAP is supported */
             if ( !hap_enabled(d) )


BTW, how come you let these get away without "fixme" tags :).. :

            /* Only HAP is supported */
            if ( !hap_enabled(d) )
                break;

            /* Currently only EPT is supported */
            if ( !cpu_has_vmx )

thanks
Mukesh

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

[Jan Beulich]

>>> On 15.05.14 at 03:59, <mukesh.rathor@oracle.com> wrote:
> Code wise several options, but seems mem_event.c would be the best 
> place to put checks (would ENOSYS be more appropriate?):

I'd prefer -EOPNOTSUPP as you did.

> +++ b/xen/arch/x86/mm/mem_event.c
> @@ -538,6 +538,13 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event
>          case XEN_DOMCTL_MEM_EVENT_OP_PAGING_ENABLE:
>          {
>              struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +            struct domain *hwdom = rcu_lock_domain_by_id(hardware_domid);
> +
> +            rcu_unlock_domain(hwdom);

Why not simply look at hardware_domain? But I think that's wrong
anyway, and you want to look at current->domain instead. And
even that would still assume that a guest controlled by other than
Dom0 would only ever have operations like this carried out by its
controlling domain - i.e. perhaps you need to check both.

> BTW, how come you let these get away without "fixme" tags :).. :
> 
>             /* Only HAP is supported */
>             if ( !hap_enabled(d) )
>                 break;
> 
>             /* Currently only EPT is supported */
>             if ( !cpu_has_vmx )

I don't think I was involved in the addition of these features.

Jan

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

[Mukesh Rathor]

On Thu, 15 May 2014 09:02:00 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 15.05.14 at 03:59, <mukesh.rathor@oracle.com> wrote:
> > Code wise several options, but seems mem_event.c would be the best 
> > place to put checks (would ENOSYS be more appropriate?):
> 
> I'd prefer -EOPNOTSUPP as you did.
> 
> > +++ b/xen/arch/x86/mm/mem_event.c
> > @@ -538,6 +538,13 @@ int mem_event_domctl(struct domain *d,
> > xen_domctl_mem_event case XEN_DOMCTL_MEM_EVENT_OP_PAGING_ENABLE:
> >          {
> >              struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +            struct domain *hwdom =
> > rcu_lock_domain_by_id(hardware_domid); +
> > +            rcu_unlock_domain(hwdom);
> 
> Why not simply look at hardware_domain? But I think that's wrong
> anyway, and you want to look at current->domain instead. And
> even that would still assume that a guest controlled by other than
> Dom0 would only ever have operations like this carried out by its
> controlling domain - i.e. perhaps you need to check both.

Yup, will check for both. With that I think we've resolved all issues
in this patch. I'm again looking at all paths to make sure foreign
entry can't go to non-present, and with that not need the BUG_ON
or ASSERT in ept_p2m_type_to_flags. paging/sharing paths are fenced
now, and I can't think of others. So after done checking all paths
again, will send next version.

thanks
mukesh

[V12 PATCH 4/4] 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 7dc938b..d248c62 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 2e30701..373e236 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -61,6 +61,10 @@ integer_param("maxcpus", max_cpus);
 static bool_t __initdata disable_smep;
 invbool_param("smep", disable_smep);
 
+/* 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.                   */
@@ -541,7 +545,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;
@@ -1338,8 +1342,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