[PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Tamas K Lengyel]

The altp2m subsystem in its current form uses its own HVMOP hypercall to set
mem_access permissions, duplicating some of the code already present for
setting regular mem_access permissions. In this patch we consolidate the two,
deprecate the HVMOP hypercall and update the corresponding tools.

Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 tools/libxc/include/xenctrl.h       |   5 +-
 tools/libxc/xc_altp2m.c             |  25 ------
 tools/libxc/xc_mem_access.c         |  14 +--
 tools/tests/xen-access/xen-access.c |  53 ++++-------
 xen/arch/arm/p2m.c                  |   9 +-
 xen/arch/x86/hvm/hvm.c              |   9 --
 xen/arch/x86/mm/p2m.c               | 169 ++++++++++++++++--------------------
 xen/common/mem_access.c             |   2 +-
 xen/include/asm-x86/p2m.h           |   4 -
 xen/include/public/hvm/hvm_op.h     |  15 +---
 xen/include/public/memory.h         |   3 +
 xen/include/xen/p2m-common.h        |   3 +-
 12 files changed, 115 insertions(+), 196 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index e632b1e..b4e57d8 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid,
 /* Switch all vCPUs of the domain to the specified altp2m view */
 int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
                              uint16_t view_id);
-int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
-                             uint16_t view_id, xen_pfn_t gfn,
-                             xenmem_access_t access);
 int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
                          uint16_t view_id, xen_pfn_t old_gfn,
                          xen_pfn_t new_gfn);
@@ -2062,7 +2059,7 @@ int xc_mem_paging_load(xc_interface *xch, domid_t domain_id,
  */
 int xc_set_mem_access(xc_interface *xch, domid_t domain_id,
                       xenmem_access_t access, uint64_t first_pfn,
-                      uint32_t nr);
+                      uint32_t nr, uint16_t altp2m_idx);
 
 /*
  * Gets the mem access for the given page (returned in access on success)
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 0639632..4f44a7b 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -163,31 +163,6 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
     return rc;
 }
 
-int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
-                             uint16_t view_id, xen_pfn_t gfn,
-                             xenmem_access_t access)
-{
-    int rc;
-    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
-
-    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
-    if ( arg == NULL )
-        return -1;
-
-    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
-    arg->cmd = HVMOP_altp2m_set_mem_access;
-    arg->domain = domid;
-    arg->u.set_mem_access.view = view_id;
-    arg->u.set_mem_access.hvmmem_access = access;
-    arg->u.set_mem_access.gfn = gfn;
-
-    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
-		  HYPERCALL_BUFFER_AS_ARG(arg));
-
-    xc_hypercall_buffer_free(handle, arg);
-    return rc;
-}
-
 int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
                          uint16_t view_id, xen_pfn_t old_gfn,
                          xen_pfn_t new_gfn)
diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
index 3634c39..d6fb409 100644
--- a/tools/libxc/xc_mem_access.c
+++ b/tools/libxc/xc_mem_access.c
@@ -27,15 +27,17 @@ int xc_set_mem_access(xc_interface *xch,
                       domid_t domain_id,
                       xenmem_access_t access,
                       uint64_t first_pfn,
-                      uint32_t nr)
+                      uint32_t nr,
+                      uint16_t altp2m_idx)
 {
     xen_mem_access_op_t mao =
     {
-        .op     = XENMEM_access_op_set_access,
-        .domid  = domain_id,
-        .access = access,
-        .pfn    = first_pfn,
-        .nr     = nr
+        .op         = XENMEM_access_op_set_access,
+        .domid      = domain_id,
+        .access     = access,
+        .pfn        = first_pfn,
+        .nr         = nr,
+        .altp2m_idx = altp2m_idx
     };
 
     return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 7993947..2300e9a 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -448,9 +448,6 @@ int main(int argc, char *argv[])
     /* With altp2m we just create a new, restricted view of the memory */
     if ( altp2m )
     {
-        xen_pfn_t gfn = 0;
-        unsigned long perm_set = 0;
-
         rc = xc_altp2m_set_domain_state( xch, domain_id, 1 );
         if ( rc < 0 )
         {
@@ -466,17 +463,6 @@ int main(int argc, char *argv[])
         }
 
         DPRINTF("altp2m view created with id %u\n", altp2m_view_id);
-        DPRINTF("Setting altp2m mem_access permissions.. ");
-
-        for(; gfn < xenaccess->max_gpfn; ++gfn)
-        {
-            rc = xc_altp2m_set_mem_access( xch, domain_id, altp2m_view_id, gfn,
-                                           default_access);
-            if ( !rc )
-                perm_set++;
-        }
-
-        DPRINTF("done! Permissions set on %lu pages.\n", perm_set);
 
         rc = xc_altp2m_switch_to_view( xch, domain_id, altp2m_view_id );
         if ( rc < 0 )
@@ -493,25 +479,22 @@ int main(int argc, char *argv[])
         }
     }
 
-    if ( !altp2m )
+    /* Set the default access type and convert all pages to it */
+    rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0, 0);
+    if ( rc < 0 )
     {
-        /* Set the default access type and convert all pages to it */
-        rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0);
-        if ( rc < 0 )
-        {
-            ERROR("Error %d setting default mem access type\n", rc);
-            goto exit;
-        }
+        ERROR("Error %d setting default mem access type\n", rc);
+        goto exit;
+    }
 
-        rc = xc_set_mem_access(xch, domain_id, default_access, START_PFN,
-                               (xenaccess->max_gpfn - START_PFN) );
+    rc = xc_set_mem_access(xch, domain_id, default_access, START_PFN,
+                           (xenaccess->max_gpfn - START_PFN), altp2m_view_id );
 
-        if ( rc < 0 )
-        {
-            ERROR("Error %d setting all memory to access type %d\n", rc,
-                  default_access);
-            goto exit;
-        }
+    if ( rc < 0 )
+    {
+        ERROR("Error %d setting all memory to access type %d\n", rc,
+              default_access);
+        goto exit;
     }
 
     if ( breakpoint )
@@ -547,12 +530,12 @@ int main(int argc, char *argv[])
                 for ( vcpu_id = 0; vcpu_id<XEN_LEGACY_MAX_VCPUS; vcpu_id++)
                     rc = control_singlestep(xch, domain_id, vcpu_id, 0);
 
-            } else {
-                rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, ~0ull, 0);
-                rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, START_PFN,
-                                       (xenaccess->max_gpfn - START_PFN) );
             }
 
+            rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, ~0ull, 0, 0);
+            rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, START_PFN,
+                                   (xenaccess->max_gpfn - START_PFN), 0 );
+
             shutting_down = 1;
         }
 
@@ -623,7 +606,7 @@ int main(int argc, char *argv[])
                 else if ( default_access != after_first_access )
                 {
                     rc = xc_set_mem_access(xch, domain_id, after_first_access,
-                                           req.u.mem_access.gfn, 1);
+                                           req.u.mem_access.gfn, 1, 0);
                     if (rc < 0)
                     {
                         ERROR("Error %d setting gfn to access_type %d\n", rc,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2190908..8e9b4be 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1709,13 +1709,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
     if ( npfec.write_access && xma == XENMEM_access_rx2rw )
     {
         rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1,
-                                0, ~0, XENMEM_access_rw);
+                                0, ~0, XENMEM_access_rw, 0);
         return false;
     }
     else if ( xma == XENMEM_access_n2rwx )
     {
         rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1,
-                                0, ~0, XENMEM_access_rwx);
+                                0, ~0, XENMEM_access_rwx, 0);
     }
 
     /* Otherwise, check if there is a vm_event monitor subscriber */
@@ -1737,7 +1737,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
                 /* A listener is not required, so clear the access
                  * restrictions. */
                 rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1,
-                                        0, ~0, XENMEM_access_rwx);
+                                        0, ~0, XENMEM_access_rwx, 0);
             }
         }
 
@@ -1788,7 +1788,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
  * If gfn == INVALID_GFN, sets the default access type.
  */
 long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
-                        uint32_t start, uint32_t mask, xenmem_access_t access)
+                        uint32_t start, uint32_t mask, xenmem_access_t access,
+                        unsigned long altp2m_idx)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_access_t a;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 674feea..2262abf 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6394,15 +6394,6 @@ static int do_altp2m_op(
         rc = p2m_switch_domain_altp2m_by_id(d, a.u.view.view);
         break;
 
-    case HVMOP_altp2m_set_mem_access:
-        if ( a.u.set_mem_access.pad )
-            rc = -EINVAL;
-        else
-            rc = p2m_set_altp2m_mem_access(d, a.u.set_mem_access.view,
-                    _gfn(a.u.set_mem_access.gfn),
-                    a.u.set_mem_access.hvmmem_access);
-        break;
-
     case HVMOP_altp2m_change_gfn:
         if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
             rc = -EINVAL;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index a45ee35..95bf7ce 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1777,14 +1777,57 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
     return (p2ma == p2m_access_n2rwx);
 }
 
+static int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
+                                     struct p2m_domain *ap2m, p2m_access_t a,
+                                     unsigned long gfn)
+{
+    mfn_t mfn;
+    p2m_type_t t;
+    p2m_access_t old_a;
+    unsigned int page_order;
+    int rc;
+
+    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
+
+    /* Check host p2m if no valid entry in alternate */
+    if ( !mfn_valid(mfn) )
+    {
+        mfn = hp2m->get_entry(hp2m, gfn, &t, &old_a,
+                              P2M_ALLOC | P2M_UNSHARE, &page_order, NULL);
+
+        rc = -ESRCH;
+        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
+            goto out;
+
+        /* If this is a superpage, copy that first */
+        if ( page_order != PAGE_ORDER_4K )
+        {
+            unsigned long mask = ~((1UL << page_order) - 1);
+            gfn_t gfn2 = _gfn(gfn & mask);
+            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
+
+            rc = ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, t, old_a, 1);
+            if ( rc )
+                goto out;
+        }
+    }
+
+    rc = ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a,
+                         (current->domain != d));
+
+ out:
+    return rc;
+}
+
 /*
  * Set access type for a region of gfns.
  * If gfn == INVALID_GFN, sets the default access type.
  */
 long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
-                        uint32_t start, uint32_t mask, xenmem_access_t access)
+                        uint32_t start, uint32_t mask, xenmem_access_t access,
+                        unsigned long altp2m_idx)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
     p2m_access_t a, _a;
     p2m_type_t t;
     mfn_t mfn;
@@ -1806,6 +1849,16 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
 #undef ACCESS
     };
 
+    /* altp2m view 0 is treated as the hostp2m */
+    if ( altp2m_idx )
+    {
+        if ( altp2m_idx >= MAX_ALTP2M ||
+             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
+            return -EINVAL;
+
+        ap2m = d->arch.altp2m_p2m[altp2m_idx];
+    }
+
     switch ( access )
     {
     case 0 ... ARRAY_SIZE(memaccess) - 1:
@@ -1826,12 +1879,25 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
     }
 
     p2m_lock(p2m);
+    if ( ap2m )
+        p2m_lock(ap2m);
+
     for ( gfn_l = gfn_x(gfn) + start; nr > start; ++gfn_l )
     {
-        mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
-        rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
-        if ( rc )
-            break;
+        if ( ap2m )
+        {
+            rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, gfn_l);
+            /* If the corresponding mfn is invalid we will just skip it */
+            if ( rc && rc != -ESRCH )
+                break;
+        }
+        else
+        {
+            mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
+            rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
+            if ( rc )
+                break;
+        }
 
         /* Check for continuation if it's not the last iteration. */
         if ( nr > ++start && !(start & mask) && hypercall_preempt_check() )
@@ -1840,7 +1906,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
             break;
         }
     }
+
+    if ( ap2m )
+        p2m_unlock(ap2m);
     p2m_unlock(p2m);
+
     return rc;
 }
 
@@ -2395,93 +2465,6 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
     return rc;
 }
 
-int p2m_set_altp2m_mem_access(struct domain *d, unsigned int idx,
-                              gfn_t gfn, xenmem_access_t access)
-{
-    struct p2m_domain *hp2m, *ap2m;
-    p2m_access_t req_a, old_a;
-    p2m_type_t t;
-    mfn_t mfn;
-    unsigned int page_order;
-    int rc = -EINVAL;
-
-    static const p2m_access_t memaccess[] = {
-#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
-        ACCESS(n),
-        ACCESS(r),
-        ACCESS(w),
-        ACCESS(rw),
-        ACCESS(x),
-        ACCESS(rx),
-        ACCESS(wx),
-        ACCESS(rwx),
-#undef ACCESS
-    };
-
-    if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == INVALID_MFN )
-        return rc;
-
-    ap2m = d->arch.altp2m_p2m[idx];
-
-    switch ( access )
-    {
-    case 0 ... ARRAY_SIZE(memaccess) - 1:
-        req_a = memaccess[access];
-        break;
-    case XENMEM_access_default:
-        req_a = ap2m->default_access;
-        break;
-    default:
-        return rc;
-    }
-
-    /* If request to set default access */
-    if ( gfn_x(gfn) == INVALID_GFN )
-    {
-        ap2m->default_access = req_a;
-        return 0;
-    }
-
-    hp2m = p2m_get_hostp2m(d);
-
-    p2m_lock(ap2m);
-
-    mfn = ap2m->get_entry(ap2m, gfn_x(gfn), &t, &old_a, 0, NULL, NULL);
-
-    /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
-    {
-        mfn = hp2m->get_entry(hp2m, gfn_x(gfn), &t, &old_a,
-                              P2M_ALLOC | P2M_UNSHARE, &page_order, NULL);
-
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            goto out;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            gfn_t gfn2;
-            unsigned long mask;
-            mfn_t mfn2;
-
-            mask = ~((1UL << page_order) - 1);
-            gfn2 = _gfn(gfn_x(gfn) & mask);
-            mfn2 = _mfn(mfn_x(mfn) & mask);
-
-            if ( ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, t, old_a, 1) )
-                goto out;
-        }
-    }
-
-    if ( !ap2m->set_entry(ap2m, gfn_x(gfn), mfn, PAGE_ORDER_4K, t, req_a,
-                          (current->domain != d)) )
-        rc = 0;
-
- out:
-    p2m_unlock(ap2m);
-    return rc;
-}
-
 int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
                           gfn_t old_gfn, gfn_t new_gfn)
 {
diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index 159c036..0411443 100644
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -67,7 +67,7 @@ int mem_access_memop(unsigned long cmd,
             break;
 
         rc = p2m_set_mem_access(d, _gfn(mao.pfn), mao.nr, start_iter,
-                                MEMOP_CMD_MASK, mao.access);
+                                MEMOP_CMD_MASK, mao.access, mao.altp2m_idx);
         if ( rc > 0 )
         {
             ASSERT(!(rc & MEMOP_CMD_MASK));
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index fa46dd9..c0df1ea 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -808,10 +808,6 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx);
 /* Switch alternate p2m for entire domain */
 int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx);
 
-/* Set access type for a gfn */
-int p2m_set_altp2m_mem_access(struct domain *d, unsigned int idx,
-                              gfn_t gfn, xenmem_access_t access);
-
 /* Change a gfn->mfn mapping */
 int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
                           gfn_t old_gfn, gfn_t new_gfn);
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 1606185..dfc9db7 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -431,18 +431,6 @@ struct xen_hvm_altp2m_view {
 typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
 
-struct xen_hvm_altp2m_set_mem_access {
-    /* view */
-    uint16_t view;
-    /* Memory type */
-    uint16_t hvmmem_access; /* xenmem_access_t */
-    uint32_t pad;
-    /* gfn */
-    uint64_t gfn;
-};
-typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
-DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
-
 struct xen_hvm_altp2m_change_gfn {
     /* view */
     uint16_t view;
@@ -470,7 +458,7 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_destroy_p2m          5
 /* Switch view for an entire domain */
 #define HVMOP_altp2m_switch_p2m           6
-/* Notify that a page of memory is to have specific access types */
+/* Deprecated by XENMEM_access_op_set_access */
 #define HVMOP_altp2m_set_mem_access       7
 /* Change a p2m entry to have a different gfn->mfn mapping */
 #define HVMOP_altp2m_change_gfn           8
@@ -481,7 +469,6 @@ struct xen_hvm_altp2m_op {
         struct xen_hvm_altp2m_domain_state       domain_state;
         struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
         struct xen_hvm_altp2m_view               view;
-        struct xen_hvm_altp2m_set_mem_access     set_mem_access;
         struct xen_hvm_altp2m_change_gfn         change_gfn;
         uint8_t pad[64];
     } u;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 4df38d6..30ccb41 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -423,11 +423,14 @@ struct xen_mem_access_op {
     /* xenmem_access_t */
     uint8_t access;
     domid_t domid;
+    uint16_t altp2m_idx;
+    uint16_t _pad;
     /*
      * Number of pages for set op
      * Ignored on setting default access and other ops
      */
     uint32_t nr;
+    uint32_t _pad2;
     /*
      * First pfn for set op
      * pfn for get op
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 47c40c7..0643ad5 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d,
  * If gfn == INVALID_GFN, sets the default access type.
  */
 long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
-                        uint32_t start, uint32_t mask, xenmem_access_t access);
+                        uint32_t start, uint32_t mask, xenmem_access_t access,
+                        unsigned long altp2m_idx);
 
 /*
  * Get access type for a gfn.
-- 
2.1.4

[PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Tamas K Lengyel]

Extend the existing get_mem_access memop to allow querying permissions in
altp2m views as well.

Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxc/include/xenctrl.h       |  3 ++-
 tools/libxc/xc_mem_access.c         |  8 +++++---
 tools/tests/xen-access/xen-access.c |  5 ++++-
 xen/arch/arm/p2m.c                  |  4 ++--
 xen/arch/x86/mm/p2m.c               | 23 +++++++++++++++++++----
 xen/common/mem_access.c             |  2 +-
 xen/include/xen/p2m-common.h        |  3 ++-
 7 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index b4e57d8..09d9f62 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2065,7 +2065,8 @@ int xc_set_mem_access(xc_interface *xch, domid_t domain_id,
  * Gets the mem access for the given page (returned in access on success)
  */
 int xc_get_mem_access(xc_interface *xch, domid_t domain_id,
-                      uint64_t pfn, xenmem_access_t *access);
+                      uint64_t pfn, uint16_t altp2m_idx,
+                      xenmem_access_t *access);
 
 /*
  * Instructions causing a mem_access violation can be emulated by Xen
diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
index d6fb409..a44865d 100644
--- a/tools/libxc/xc_mem_access.c
+++ b/tools/libxc/xc_mem_access.c
@@ -46,14 +46,16 @@ int xc_set_mem_access(xc_interface *xch,
 int xc_get_mem_access(xc_interface *xch,
                       domid_t domain_id,
                       uint64_t pfn,
+                      uint16_t altp2m_idx,
                       xenmem_access_t *access)
 {
     int rc;
     xen_mem_access_op_t mao =
     {
-        .op    = XENMEM_access_op_get_access,
-        .domid = domain_id,
-        .pfn   = pfn
+        .op         = XENMEM_access_op_get_access,
+        .domid      = domain_id,
+        .pfn        = pfn,
+        .altp2m_idx = altp2m_idx
     };
 
     rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 2300e9a..d9dda62 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -571,7 +571,10 @@ int main(int argc, char *argv[])
 
             switch (req.reason) {
             case VM_EVENT_REASON_MEM_ACCESS:
-                rc = xc_get_mem_access(xch, domain_id, req.u.mem_access.gfn, &access);
+                rc = xc_get_mem_access(xch, domain_id, req.u.mem_access.gfn,
+                                       ((req.flags & VM_EVENT_FLAG_ALTERNATE_P2M) ? req.altp2m_idx : 0),
+                                       &access
+                                       );
                 if (rc < 0)
                 {
                     ERROR("Error %d getting mem_access event\n", rc);
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8e9b4be..932c6e2 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1666,7 +1666,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
     if ( !p2m->mem_access_enabled )
         return true;
 
-    rc = p2m_get_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), &xma);
+    rc = p2m_get_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 0, &xma);
     if ( rc )
         return true;
 
@@ -1847,7 +1847,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
     return 0;
 }
 
-int p2m_get_mem_access(struct domain *d, gfn_t gfn,
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
                        xenmem_access_t *access)
 {
     int ret;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 95bf7ce..18068e8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
         bool_t violation = 1;
         const struct vm_event_mem_access *data = &rsp->u.mem_access;
 
-        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
+        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
+                                altp2m_active(v->domain) ? vcpu_altp2m(v).p2midx : 0,
+                                &access) == 0 )
         {
             switch ( access )
             {
@@ -1918,9 +1920,10 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
  * Get access type for a gfn.
  * If gfn == INVALID_GFN, gets the default access type.
  */
-int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
+                       xenmem_access_t *access)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *hp2m = p2m_get_hostp2m(d), *p2m = NULL;
     p2m_type_t t;
     p2m_access_t a;
     mfn_t mfn;
@@ -1943,10 +1946,22 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
     /* If request to get default access. */
     if ( gfn_x(gfn) == INVALID_GFN )
     {
-        *access = memaccess[p2m->default_access];
+        *access = memaccess[hp2m->default_access];
         return 0;
     }
 
+    /* altp2m view 0 is treated as the hostp2m */
+    if ( altp2m_idx )
+    {
+        if ( altp2m_idx >= MAX_ALTP2M ||
+             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
+            return -EINVAL;
+
+        p2m = d->arch.altp2m_p2m[altp2m_idx];
+    }
+    else
+        p2m = hp2m;
+
     gfn_lock(p2m, gfn, 0);
     mfn = p2m->get_entry(p2m, gfn_x(gfn), &t, &a, 0, NULL, NULL);
     gfn_unlock(p2m, gfn, 0);
diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index 0411443..a0345a6 100644
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -88,7 +88,7 @@ int mem_access_memop(unsigned long cmd,
         if ( (mao.pfn > domain_get_maximum_gpfn(d)) && mao.pfn != ~0ull )
             break;
 
-        rc = p2m_get_mem_access(d, _gfn(mao.pfn), &access);
+        rc = p2m_get_mem_access(d, _gfn(mao.pfn), mao.altp2m_idx, &access);
         if ( rc != 0 )
             break;
 
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 0643ad5..2835f24 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -56,6 +56,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
  * Get access type for a gfn.
  * If gfn == INVALID_GFN, gets the default access type.
  */
-int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
+                       xenmem_access_t *access);
 
 #endif /* _XEN_P2M_COMMON_H */
-- 
2.1.4

Re: [PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Razvan Cojocaru]

On 01/27/2016 10:06 PM, Tamas K Lengyel wrote:
> The altp2m subsystem in its current form uses its own HVMOP hypercall to set
> mem_access permissions, duplicating some of the code already present for
> setting regular mem_access permissions. In this patch we consolidate the two,
> deprecate the HVMOP hypercall and update the corresponding tools.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  tools/libxc/include/xenctrl.h       |   5 +-
>  tools/libxc/xc_altp2m.c             |  25 ------
>  tools/libxc/xc_mem_access.c         |  14 +--
>  tools/tests/xen-access/xen-access.c |  53 ++++-------
>  xen/arch/arm/p2m.c                  |   9 +-
>  xen/arch/x86/hvm/hvm.c              |   9 --
>  xen/arch/x86/mm/p2m.c               | 169 ++++++++++++++++--------------------
>  xen/common/mem_access.c             |   2 +-
>  xen/include/asm-x86/p2m.h           |   4 -
>  xen/include/public/hvm/hvm_op.h     |  15 +---
>  xen/include/public/memory.h         |   3 +
>  xen/include/xen/p2m-common.h        |   3 +-
>  12 files changed, 115 insertions(+), 196 deletions(-)

For the part of the code in the files we're maintaining:

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Thanks for doing this, as also discussed in private I think something
along these lines is a much needed change here.


Thanks,
Razvan

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Razvan Cojocaru]

On 01/27/2016 10:06 PM, Tamas K Lengyel wrote:
> Extend the existing get_mem_access memop to allow querying permissions in
> altp2m views as well.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/libxc/include/xenctrl.h       |  3 ++-
>  tools/libxc/xc_mem_access.c         |  8 +++++---
>  tools/tests/xen-access/xen-access.c |  5 ++++-
>  xen/arch/arm/p2m.c                  |  4 ++--
>  xen/arch/x86/mm/p2m.c               | 23 +++++++++++++++++++----
>  xen/common/mem_access.c             |  2 +-
>  xen/include/xen/p2m-common.h        |  3 ++-
>  7 files changed, 35 insertions(+), 13 deletions(-)

For the part of the code in the files we're maintaining:

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan

Re: [PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Wei Liu]

On Wed, Jan 27, 2016 at 01:06:40PM -0700, Tamas K Lengyel wrote:
> The altp2m subsystem in its current form uses its own HVMOP hypercall to set
> mem_access permissions, duplicating some of the code already present for
> setting regular mem_access permissions. In this patch we consolidate the two,
> deprecate the HVMOP hypercall and update the corresponding tools.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  tools/libxc/include/xenctrl.h       |   5 +-
>  tools/libxc/xc_altp2m.c             |  25 ------
>  tools/libxc/xc_mem_access.c         |  14 +--
>  tools/tests/xen-access/xen-access.c |  53 ++++-------
>  xen/arch/arm/p2m.c                  |   9 +-
>  xen/arch/x86/hvm/hvm.c              |   9 --
>  xen/arch/x86/mm/p2m.c               | 169 ++++++++++++++++--------------------
>  xen/common/mem_access.c             |   2 +-
>  xen/include/asm-x86/p2m.h           |   4 -
>  xen/include/public/hvm/hvm_op.h     |  15 +---
>  xen/include/public/memory.h         |   3 +
>  xen/include/xen/p2m-common.h        |   3 +-
>  12 files changed, 115 insertions(+), 196 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index e632b1e..b4e57d8 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid,
>  /* Switch all vCPUs of the domain to the specified altp2m view */
>  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>                               uint16_t view_id);
> -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
> -                             uint16_t view_id, xen_pfn_t gfn,
> -                             xenmem_access_t access);

What is the support status of these APIs in libxc? Are they supposed to
be stable now? Do you have opinion on making them stable interfaces?

If you consider them stable, you can't just remove it. Presumably you
can reimplement it as a wrapper to xc_set_mem_access if we decide to
keep it around.

Wei.

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Wei Liu]

On Wed, Jan 27, 2016 at 01:06:41PM -0700, Tamas K Lengyel wrote:
> Extend the existing get_mem_access memop to allow querying permissions in
> altp2m views as well.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/libxc/include/xenctrl.h       |  3 ++-
>  tools/libxc/xc_mem_access.c         |  8 +++++---
>  tools/tests/xen-access/xen-access.c |  5 ++++-

The code looks sensible. Some nits and question below.

>  xen/arch/arm/p2m.c                  |  4 ++--
>  xen/arch/x86/mm/p2m.c               | 23 +++++++++++++++++++----
>  xen/common/mem_access.c             |  2 +-
>  xen/include/xen/p2m-common.h        |  3 ++-
>  7 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index b4e57d8..09d9f62 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2065,7 +2065,8 @@ int xc_set_mem_access(xc_interface *xch, domid_t domain_id,
>   * Gets the mem access for the given page (returned in access on success)
>   */
>  int xc_get_mem_access(xc_interface *xch, domid_t domain_id,
> -                      uint64_t pfn, xenmem_access_t *access);
> +                      uint64_t pfn, uint16_t altp2m_idx,
> +                      xenmem_access_t *access);
>  

The same questions regarding stability of these functions apply here as
well.

>  /*
>   * Instructions causing a mem_access violation can be emulated by Xen
> diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
> index d6fb409..a44865d 100644
> --- a/tools/libxc/xc_mem_access.c
> +++ b/tools/libxc/xc_mem_access.c
> @@ -46,14 +46,16 @@ int xc_set_mem_access(xc_interface *xch,
>  int xc_get_mem_access(xc_interface *xch,
>                        domid_t domain_id,
>                        uint64_t pfn,
> +                      uint16_t altp2m_idx,
>                        xenmem_access_t *access)
>  {
>      int rc;
>      xen_mem_access_op_t mao =
>      {
> -        .op    = XENMEM_access_op_get_access,
> -        .domid = domain_id,
> -        .pfn   = pfn
> +        .op         = XENMEM_access_op_get_access,
> +        .domid      = domain_id,
> +        .pfn        = pfn,
> +        .altp2m_idx = altp2m_idx
>      };
>  
>      rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
> diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
> index 2300e9a..d9dda62 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -571,7 +571,10 @@ int main(int argc, char *argv[])
>  
>              switch (req.reason) {
>              case VM_EVENT_REASON_MEM_ACCESS:
> -                rc = xc_get_mem_access(xch, domain_id, req.u.mem_access.gfn, &access);
> +                rc = xc_get_mem_access(xch, domain_id, req.u.mem_access.gfn,
> +                                       ((req.flags & VM_EVENT_FLAG_ALTERNATE_P2M) ? req.altp2m_idx : 0),

Ling too long.

> +                                       &access
> +                                       );

And fold this ")" to previous line?

>                  if (rc < 0)
>                  {
>                      ERROR("Error %d getting mem_access event\n", rc);
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 8e9b4be..932c6e2 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1666,7 +1666,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
>      if ( !p2m->mem_access_enabled )
>          return true;
>  
> -    rc = p2m_get_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), &xma);
> +    rc = p2m_get_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 0, &xma);
>      if ( rc )
>          return true;
>  
> @@ -1847,7 +1847,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>      return 0;
>  }
>  
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn,
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
>                         xenmem_access_t *access)
>  {
>      int ret;
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 95bf7ce..18068e8 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>          bool_t violation = 1;
>          const struct vm_event_mem_access *data = &rsp->u.mem_access;
>  
> -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
> +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> +                                altp2m_active(v->domain) ? vcpu_altp2m(v).p2midx : 0,

Line too long.

Wei.

Re: [PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Ian Campbell]

--- a/tools/libxc/include/xenctrl.h
> > +++ b/tools/libxc/include/xenctrl.h
> > @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface *handle,
> > domid_t domid,
> >  /* Switch all vCPUs of the domain to the specified altp2m view */
> >  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
> >                               uint16_t view_id);
> > -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
> > -                             uint16_t view_id, xen_pfn_t gfn,
> > -                             xenmem_access_t access);
> 
> What is the support status of these APIs in libxc? Are they supposed to
> be stable now? Do you have opinion on making them stable interfaces?

Nothing in libxc is ever a stable interface. It is always completely fine
to change them if that is what is needed.

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

Re: [PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Wei Liu]

On Thu, Jan 28, 2016 at 10:55:36AM +0000, Ian Campbell wrote:
> --- a/tools/libxc/include/xenctrl.h
> > > +++ b/tools/libxc/include/xenctrl.h
> > > @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface *handle,
> > > domid_t domid,
> > >  /* Switch all vCPUs of the domain to the specified altp2m view */
> > >  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
> > >                               uint16_t view_id);
> > > -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
> > > -                             uint16_t view_id, xen_pfn_t gfn,
> > > -                             xenmem_access_t access);
> > 
> > What is the support status of these APIs in libxc? Are they supposed to
> > be stable now? Do you have opinion on making them stable interfaces?
> 
> Nothing in libxc is ever a stable interface. It is always completely fine
> to change them if that is what is needed.
> 

Right. Ignore the first question and this patch is ready to go in when
those nits are fixed.

It would still be nice if we can get a libmemaccess or some sort so that
people can build on top of it. Just curious if that's feasible.

Wei.

> Ian.

Re: [PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Jan Beulich]

>>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote:
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -431,18 +431,6 @@ struct xen_hvm_altp2m_view {
>  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
>  
> -struct xen_hvm_altp2m_set_mem_access {
> -    /* view */
> -    uint16_t view;
> -    /* Memory type */
> -    uint16_t hvmmem_access; /* xenmem_access_t */
> -    uint32_t pad;
> -    /* gfn */
> -    uint64_t gfn;
> -};
> -typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
> -DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);

This is a guest visible interface, and hence can't be removed (nor
be replaced by a tools only one).

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -423,11 +423,14 @@ struct xen_mem_access_op {
>      /* xenmem_access_t */
>      uint8_t access;
>      domid_t domid;
> +    uint16_t altp2m_idx;

So this is a tools only interface, yes. But it's not versioned (other
than e.g. domctl and sysctl), so altering the interface structure is
at least fragile.

And then, with this ...

> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d,
>   * If gfn == INVALID_GFN, sets the default access type.
>   */
>  long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
> -                        uint32_t start, uint32_t mask, xenmem_access_t access);
> +                        uint32_t start, uint32_t mask, xenmem_access_t access,
> +                        unsigned long altp2m_idx);

... why "unsigned long" instead of e.g. "unsigned int" here?

Jan

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Jan Beulich]

>>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>          bool_t violation = 1;
>          const struct vm_event_mem_access *data = &rsp->u.mem_access;
>  
> -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
> +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> +                                altp2m_active(v->domain) ? vcpu_altp2m(v).p2midx : 0,
> +                                &access) == 0 )

This looks to be a behavioral change beyond what title and
description say, and it's not clear whether that's actually the
behavior everyone wants.

> @@ -1918,9 +1920,10 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>   * Get access type for a gfn.
>   * If gfn == INVALID_GFN, gets the default access type.
>   */
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
> +                       xenmem_access_t *access)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *hp2m = p2m_get_hostp2m(d), *p2m = NULL;
>      p2m_type_t t;
>      p2m_access_t a;
>      mfn_t mfn;
> @@ -1943,10 +1946,22 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
>      /* If request to get default access. */
>      if ( gfn_x(gfn) == INVALID_GFN )
>      {
> -        *access = memaccess[p2m->default_access];
> +        *access = memaccess[hp2m->default_access];
>          return 0;
>      }

And following the above - why would this not use the altp2m's
default access?

> +    /* altp2m view 0 is treated as the hostp2m */
> +    if ( altp2m_idx )
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
> +            return -EINVAL;
> +
> +        p2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
> +    else
> +        p2m = hp2m;

And I don't see why you need separate p2m and hp2m local
variables.

> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -56,6 +56,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>   * Get access type for a gfn.
>   * If gfn == INVALID_GFN, gets the default access type.
>   */
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
> +                       xenmem_access_t *access);

Same question as for patch 1 regarding the "unsigned long" here.

Jan

Re: [PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 1395 bytes --]

On Jan 28, 2016 4:00 AM, "Wei Liu" <wei.liu2@citrix.com> wrote:
>
> On Thu, Jan 28, 2016 at 10:55:36AM +0000, Ian Campbell wrote:
> > --- a/tools/libxc/include/xenctrl.h
> > > > +++ b/tools/libxc/include/xenctrl.h
> > > > @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface
*handle,
> > > > domid_t domid,
> > > >  /* Switch all vCPUs of the domain to the specified altp2m view */
> > > >  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
> > > >                               uint16_t view_id);
> > > > -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
> > > > -                             uint16_t view_id, xen_pfn_t gfn,
> > > > -                             xenmem_access_t access);
> > >
> > > What is the support status of these APIs in libxc? Are they supposed
to
> > > be stable now? Do you have opinion on making them stable interfaces?
> >
> > Nothing in libxc is ever a stable interface. It is always completely
fine
> > to change them if that is what is needed.
> >
>
> Right. Ignore the first question and this patch is ready to go in when
> those nits are fixed.
>
> It would still be nice if we can get a libmemaccess or some sort so that
> people can build on top of it. Just curious if that's feasible.
>
> Wei.

There is LibVMI and Bitdefender also released its library so there are
choices if folks find libxc too volatile.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1940 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 2101 bytes --]

On Jan 28, 2016 6:18 AM, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote:
> > --- a/xen/include/public/hvm/hvm_op.h
> > +++ b/xen/include/public/hvm/hvm_op.h
> > @@ -431,18 +431,6 @@ struct xen_hvm_altp2m_view {
> >  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
> >
> > -struct xen_hvm_altp2m_set_mem_access {
> > -    /* view */
> > -    uint16_t view;
> > -    /* Memory type */
> > -    uint16_t hvmmem_access; /* xenmem_access_t */
> > -    uint32_t pad;
> > -    /* gfn */
> > -    uint64_t gfn;
> > -};
> > -typedef struct xen_hvm_altp2m_set_mem_access
xen_hvm_altp2m_set_mem_access_t;
> > -DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>
> This is a guest visible interface, and hence can't be removed (nor
> be replaced by a tools only one).

What is your suggestion?

>
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -423,11 +423,14 @@ struct xen_mem_access_op {
> >      /* xenmem_access_t */
> >      uint8_t access;
> >      domid_t domid;
> > +    uint16_t altp2m_idx;
>
> So this is a tools only interface, yes. But it's not versioned (other
> than e.g. domctl and sysctl), so altering the interface structure is
> at least fragile.
>
> And then, with this ...
>
> > --- a/xen/include/xen/p2m-common.h
> > +++ b/xen/include/xen/p2m-common.h
> > @@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d,
> >   * If gfn == INVALID_GFN, sets the default access type.
> >   */
> >  long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
> > -                        uint32_t start, uint32_t mask, xenmem_access_t
access);
> > +                        uint32_t start, uint32_t mask, xenmem_access_t
access,
> > +                        unsigned long altp2m_idx);
>
> ... why "unsigned long" instead of e.g. "unsigned int" here?
>

These were used interchangebly in the code, I've just picked on. The max
value it can legitimetly have is 10 so there is not much difference in
going with int instead of long.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 2897 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Wei Liu]

On Thu, Jan 28, 2016 at 07:30:31AM -0700, Lengyel, Tamas wrote:
> On Jan 28, 2016 4:00 AM, "Wei Liu" <wei.liu2@citrix.com> wrote:
> >
> > On Thu, Jan 28, 2016 at 10:55:36AM +0000, Ian Campbell wrote:
> > > --- a/tools/libxc/include/xenctrl.h
> > > > > +++ b/tools/libxc/include/xenctrl.h
> > > > > @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface
> *handle,
> > > > > domid_t domid,
> > > > >  /* Switch all vCPUs of the domain to the specified altp2m view */
> > > > >  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
> > > > >                               uint16_t view_id);
> > > > > -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
> > > > > -                             uint16_t view_id, xen_pfn_t gfn,
> > > > > -                             xenmem_access_t access);
> > > >
> > > > What is the support status of these APIs in libxc? Are they supposed
> to
> > > > be stable now? Do you have opinion on making them stable interfaces?
> > >
> > > Nothing in libxc is ever a stable interface. It is always completely
> fine
> > > to change them if that is what is needed.
> > >
> >
> > Right. Ignore the first question and this patch is ready to go in when
> > those nits are fixed.
> >
> > It would still be nice if we can get a libmemaccess or some sort so that
> > people can build on top of it. Just curious if that's feasible.
> >
> > Wei.
> 
> There is LibVMI and Bitdefender also released its library so there are
> choices if folks find libxc too volatile.

Thanks, this makes sense.

Wei.

> 
> Tamas

Re: [PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Jan Beulich]

>>> On 28.01.16 at 15:34, <tlengyel@novetta.com> wrote:
> On Jan 28, 2016 6:18 AM, "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote:
>> > --- a/xen/include/public/hvm/hvm_op.h
>> > +++ b/xen/include/public/hvm/hvm_op.h
>> > @@ -431,18 +431,6 @@ struct xen_hvm_altp2m_view {
>> >  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
>> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
>> >
>> > -struct xen_hvm_altp2m_set_mem_access {
>> > -    /* view */
>> > -    uint16_t view;
>> > -    /* Memory type */
>> > -    uint16_t hvmmem_access; /* xenmem_access_t */
>> > -    uint32_t pad;
>> > -    /* gfn */
>> > -    uint64_t gfn;
>> > -};
>> > -typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>> > -DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>>
>> This is a guest visible interface, and hence can't be removed (nor
>> be replaced by a tools only one).
> 
> What is your suggestion?

At the very least keep the old interface, perhaps backed by the
new implementation.

>> > --- a/xen/include/public/memory.h
>> > +++ b/xen/include/public/memory.h
>> > @@ -423,11 +423,14 @@ struct xen_mem_access_op {
>> >      /* xenmem_access_t */
>> >      uint8_t access;
>> >      domid_t domid;
>> > +    uint16_t altp2m_idx;
>>
>> So this is a tools only interface, yes. But it's not versioned (other
>> than e.g. domctl and sysctl), so altering the interface structure is
>> at least fragile.
>>
>> And then, with this ...
>>
>> > --- a/xen/include/xen/p2m-common.h
>> > +++ b/xen/include/xen/p2m-common.h
>> > @@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d,
>> >   * If gfn == INVALID_GFN, sets the default access type.
>> >   */
>> >  long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>> > -                        uint32_t start, uint32_t mask, xenmem_access_t access);
>> > +                        uint32_t start, uint32_t mask, xenmem_access_t access,
>> > +                        unsigned long altp2m_idx);
>>
>> ... why "unsigned long" instead of e.g. "unsigned int" here?
> 
> These were used interchangebly in the code, I've just picked on. The max
> value it can legitimetly have is 10 so there is not much difference in
> going with int instead of long.

Since "int" is slightly less expensive to operate on, please use it
whenever possible.

Jan

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 3432 bytes --]

On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote:
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
> >          bool_t violation = 1;
> >          const struct vm_event_mem_access *data = &rsp->u.mem_access;
> >
> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access)
== 0 )
> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> > +                                altp2m_active(v->domain) ?
vcpu_altp2m(v).p2midx : 0,
> > +                                &access) == 0 )
>
> This looks to be a behavioral change beyond what title and
> description say, and it's not clear whether that's actually the
> behavior everyone wants.

I'm fairly comfident its exactly the expected behavior when one uses
mem_access in altp2m tables and emulation. Right now because the lack of
this AFAIK emulation would not work correctly with altp2m. But Razvan
probably can chime in as he uses this path actively.

>
> > @@ -1918,9 +1920,10 @@ long p2m_set_mem_access(struct domain *d, gfn_t
gfn, uint32_t nr,
> >   * Get access type for a gfn.
> >   * If gfn == INVALID_GFN, gets the default access type.
> >   */
> > -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t
*access)
> > +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long
altp2m_idx,
> > +                       xenmem_access_t *access)
> >  {
> > -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +    struct p2m_domain *hp2m = p2m_get_hostp2m(d), *p2m = NULL;
> >      p2m_type_t t;
> >      p2m_access_t a;
> >      mfn_t mfn;
> > @@ -1943,10 +1946,22 @@ int p2m_get_mem_access(struct domain *d, gfn_t
gfn, xenmem_access_t *access)
> >      /* If request to get default access. */
> >      if ( gfn_x(gfn) == INVALID_GFN )
> >      {
> > -        *access = memaccess[p2m->default_access];
> > +        *access = memaccess[hp2m->default_access];
> >          return 0;
> >      }
>
> And following the above - why would this not use the altp2m's
> default access?

Altp2m default access is currently ignored. You can only set default access
for hp2m.

>
> > +    /* altp2m view 0 is treated as the hostp2m */
> > +    if ( altp2m_idx )
> > +    {
> > +        if ( altp2m_idx >= MAX_ALTP2M ||
> > +             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
> > +            return -EINVAL;
> > +
> > +        p2m = d->arch.altp2m_p2m[altp2m_idx];
> > +    }
> > +    else
> > +        p2m = hp2m;
>
> And I don't see why you need separate p2m and hp2m local
> variables.

It was also used above for returning default access, while p2m is the
actually active one here.

>
> > --- a/xen/include/xen/p2m-common.h
> > +++ b/xen/include/xen/p2m-common.h
> > @@ -56,6 +56,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
uint32_t nr,
> >   * Get access type for a gfn.
> >   * If gfn == INVALID_GFN, gets the default access type.
> >   */
> > -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t
*access);
> > +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long
altp2m_idx,
> > +                       xenmem_access_t *access);
>
> Same question as for patch 1 regarding the "unsigned long" here.
>
As stated on the other patch, it could be either for our purposes. Any
benefit in using int rather then long?

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4624 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Jan Beulich]

>>> On 28.01.16 at 15:42, <tlengyel@novetta.com> wrote:
> On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com> wrote:
>> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote:
>> > --- a/xen/arch/x86/mm/p2m.c
>> > +++ b/xen/arch/x86/mm/p2m.c
>> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>> >          bool_t violation = 1;
>> >          const struct vm_event_mem_access *data = &rsp->u.mem_access;
>> >
>> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access)
> == 0 )
>> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
>> > +                                altp2m_active(v->domain) ?
> vcpu_altp2m(v).p2midx : 0,
>> > +                                &access) == 0 )
>>
>> This looks to be a behavioral change beyond what title and
>> description say, and it's not clear whether that's actually the
>> behavior everyone wants.
> 
> I'm fairly comfident its exactly the expected behavior when one uses
> mem_access in altp2m tables and emulation. Right now because the lack of
> this AFAIK emulation would not work correctly with altp2m. But Razvan
> probably can chime in as he uses this path actively.

But this should then be mentioned in the description.

>> > +    /* altp2m view 0 is treated as the hostp2m */
>> > +    if ( altp2m_idx )
>> > +    {
>> > +        if ( altp2m_idx >= MAX_ALTP2M ||
>> > +             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
>> > +            return -EINVAL;
>> > +
>> > +        p2m = d->arch.altp2m_p2m[altp2m_idx];
>> > +    }
>> > +    else
>> > +        p2m = hp2m;
>>
>> And I don't see why you need separate p2m and hp2m local
>> variables.
> 
> It was also used above for returning default access, while p2m is the
> actually active one here.

But all that could be done with just the one variable that was already
there.

Jan

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 2033 bytes --]

On Jan 28, 2016 7:56 AM, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 28.01.16 at 15:42, <tlengyel@novetta.com> wrote:
> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com> wrote:
> >> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote:
> >> > --- a/xen/arch/x86/mm/p2m.c
> >> > +++ b/xen/arch/x86/mm/p2m.c
> >> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu
*v,
> >> >          bool_t violation = 1;
> >> >          const struct vm_event_mem_access *data = &rsp->u.mem_access;
> >> >
> >> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access)
> > == 0 )
> >> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> >> > +                                altp2m_active(v->domain) ?
> > vcpu_altp2m(v).p2midx : 0,
> >> > +                                &access) == 0 )
> >>
> >> This looks to be a behavioral change beyond what title and
> >> description say, and it's not clear whether that's actually the
> >> behavior everyone wants.
> >
> > I'm fairly comfident its exactly the expected behavior when one uses
> > mem_access in altp2m tables and emulation. Right now because the lack of
> > this AFAIK emulation would not work correctly with altp2m. But Razvan
> > probably can chime in as he uses this path actively.
>
> But this should then be mentioned in the description.
>
> >> > +    /* altp2m view 0 is treated as the hostp2m */
> >> > +    if ( altp2m_idx )
> >> > +    {
> >> > +        if ( altp2m_idx >= MAX_ALTP2M ||
> >> > +             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
> >> > +            return -EINVAL;
> >> > +
> >> > +        p2m = d->arch.altp2m_p2m[altp2m_idx];
> >> > +    }
> >> > +    else
> >> > +        p2m = hp2m;
> >>
> >> And I don't see why you need separate p2m and hp2m local
> >> variables.
> >
> > It was also used above for returning default access, while p2m is the
> > actually active one here.
>
> But all that could be done with just the one variable that was already
> there.
>
> Jan

Sure, SGTM.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 3131 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Razvan Cojocaru]

On 01/28/2016 04:42 PM, Lengyel, Tamas wrote:
> 
> On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com
> <mailto:JBeulich@suse.com>> wrote:
>>
>> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com
> <mailto:tlengyel@novetta.com>> wrote:
>> > --- a/xen/arch/x86/mm/p2m.c
>> > +++ b/xen/arch/x86/mm/p2m.c
>> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>> >          bool_t violation = 1;
>> >          const struct vm_event_mem_access *data = &rsp->u.mem_access;
>> >
>> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> &access) == 0 )
>> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
>> > +                                altp2m_active(v->domain) ?
> vcpu_altp2m(v).p2midx : 0,
>> > +                                &access) == 0 )
>>
>> This looks to be a behavioral change beyond what title and
>> description say, and it's not clear whether that's actually the
>> behavior everyone wants.
> 
> I'm fairly comfident its exactly the expected behavior when one uses
> mem_access in altp2m tables and emulation. Right now because the lack of
> this AFAIK emulation would not work correctly with altp2m. But Razvan
> probably can chime in as he uses this path actively.

I've done an experiment to see how much slower using altp2m would be as
compared to emulation - so I'm not a big user of the feature, but I did
find it cumbersome to have to work with two sets of APIs (one for what
could arguably be called the default altp2m view, i.e. the regular
xc_set_mem_access(), and one for altp2m, i.e.
xc_altp2m_set_mem_access()). Furthermore, the APIs do not currently
offer the same features (most notably, xc_altp2m_get_mem_access() is
completely missing). I've mentioned this to Tamas while initially trying
to get it to work.

Now, whether the behaviour I expect is what everyone expects is, of
course, wide open to debate. But I think we can all agree that the
altp2m interface can, and probably should, be improved.


Thanks,
Razvan

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 2537 bytes --]

On Jan 28, 2016 8:02 AM, "Razvan Cojocaru" <rcojocaru@bitdefender.com>
wrote:
>
> On 01/28/2016 04:42 PM, Lengyel, Tamas wrote:
> >
> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com
> > <mailto:JBeulich@suse.com>> wrote:
> >>
> >> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com
> > <mailto:tlengyel@novetta.com>> wrote:
> >> > --- a/xen/arch/x86/mm/p2m.c
> >> > +++ b/xen/arch/x86/mm/p2m.c
> >> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu
*v,
> >> >          bool_t violation = 1;
> >> >          const struct vm_event_mem_access *data = &rsp->u.mem_access;
> >> >
> >> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> > &access) == 0 )
> >> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> >> > +                                altp2m_active(v->domain) ?
> > vcpu_altp2m(v).p2midx : 0,
> >> > +                                &access) == 0 )
> >>
> >> This looks to be a behavioral change beyond what title and
> >> description say, and it's not clear whether that's actually the
> >> behavior everyone wants.
> >
> > I'm fairly comfident its exactly the expected behavior when one uses
> > mem_access in altp2m tables and emulation. Right now because the lack of
> > this AFAIK emulation would not work correctly with altp2m. But Razvan
> > probably can chime in as he uses this path actively.
>
> I've done an experiment to see how much slower using altp2m would be as
> compared to emulation - so I'm not a big user of the feature, but I did
> find it cumbersome to have to work with two sets of APIs (one for what
> could arguably be called the default altp2m view, i.e. the regular
> xc_set_mem_access(), and one for altp2m, i.e.
> xc_altp2m_set_mem_access()). Furthermore, the APIs do not currently
> offer the same features (most notably, xc_altp2m_get_mem_access() is
> completely missing). I've mentioned this to Tamas while initially trying
> to get it to work.
>
> Now, whether the behaviour I expect is what everyone expects is, of
> course, wide open to debate. But I think we can all agree that the
> altp2m interface can, and probably should, be improved.
>

There is that, but also, what is the exact logic behind doing this check
before emulation? AFAIU emulation happens in response to a vm_event so we
should be fairly certain that this check succeeds as it just verifies that
indeed the permissions are restricted by mem_access in the p2m (and with
altp2m this should be the active one). But when is this check normally
expected to fail?

Tamas

[-- Attachment #1.2: Type: text/html, Size: 3478 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Razvan Cojocaru]

On 01/28/2016 05:12 PM, Lengyel, Tamas wrote:
> 
> On Jan 28, 2016 8:02 AM, "Razvan Cojocaru" <rcojocaru@bitdefender.com
> <mailto:rcojocaru@bitdefender.com>> wrote:
>>
>> On 01/28/2016 04:42 PM, Lengyel, Tamas wrote:
>> >
>> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com
> <mailto:JBeulich@suse.com>
>> > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>> wrote:
>> >>
>> >> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com
> <mailto:tlengyel@novetta.com>
>> > <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>> wrote:
>> >> > --- a/xen/arch/x86/mm/p2m.c
>> >> > +++ b/xen/arch/x86/mm/p2m.c
>> >> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct
> vcpu *v,
>> >> >          bool_t violation = 1;
>> >> >          const struct vm_event_mem_access *data = &rsp->u.mem_access;
>> >> >
>> >> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
>> > &access) == 0 )
>> >> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
>> >> > +                                altp2m_active(v->domain) ?
>> > vcpu_altp2m(v).p2midx : 0,
>> >> > +                                &access) == 0 )
>> >>
>> >> This looks to be a behavioral change beyond what title and
>> >> description say, and it's not clear whether that's actually the
>> >> behavior everyone wants.
>> >
>> > I'm fairly comfident its exactly the expected behavior when one uses
>> > mem_access in altp2m tables and emulation. Right now because the lack of
>> > this AFAIK emulation would not work correctly with altp2m. But Razvan
>> > probably can chime in as he uses this path actively.
>>
>> I've done an experiment to see how much slower using altp2m would be as
>> compared to emulation - so I'm not a big user of the feature, but I did
>> find it cumbersome to have to work with two sets of APIs (one for what
>> could arguably be called the default altp2m view, i.e. the regular
>> xc_set_mem_access(), and one for altp2m, i.e.
>> xc_altp2m_set_mem_access()). Furthermore, the APIs do not currently
>> offer the same features (most notably, xc_altp2m_get_mem_access() is
>> completely missing). I've mentioned this to Tamas while initially trying
>> to get it to work.
>>
>> Now, whether the behaviour I expect is what everyone expects is, of
>> course, wide open to debate. But I think we can all agree that the
>> altp2m interface can, and probably should, be improved.
>>
> 
> There is that, but also, what is the exact logic behind doing this check
> before emulation? AFAIU emulation happens in response to a vm_event so
> we should be fairly certain that this check succeeds as it just verifies
> that indeed the permissions are restricted by mem_access in the p2m (and
> with altp2m this should be the active one). But when is this check
> normally expected to fail?

That check is important, please do not remove it. A vm_event is sent
into userspace to our monitoring application, but the monitoring
application can actually remove the page restrictions before replying,
so in that case emulation is pointless - there will be no more page
faults for that instruction.


Thanks,
Razvan

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 3509 bytes --]

On Thu, Jan 28, 2016 at 8:20 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 01/28/2016 05:12 PM, Lengyel, Tamas wrote:
> >
> > On Jan 28, 2016 8:02 AM, "Razvan Cojocaru" <rcojocaru@bitdefender.com
> > <mailto:rcojocaru@bitdefender.com>> wrote:
> >>
> >> On 01/28/2016 04:42 PM, Lengyel, Tamas wrote:
> >> >
> >> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com
> > <mailto:JBeulich@suse.com>
> >> > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>> wrote:
> >> >>
> >> >> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com
> > <mailto:tlengyel@novetta.com>
> >> > <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>> wrote:
> >> >> > --- a/xen/arch/x86/mm/p2m.c
> >> >> > +++ b/xen/arch/x86/mm/p2m.c
> >> >> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct
> > vcpu *v,
> >> >> >          bool_t violation = 1;
> >> >> >          const struct vm_event_mem_access *data =
> &rsp->u.mem_access;
> >> >> >
> >> >> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> >> > &access) == 0 )
> >> >> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> >> >> > +                                altp2m_active(v->domain) ?
> >> > vcpu_altp2m(v).p2midx : 0,
> >> >> > +                                &access) == 0 )
> >> >>
> >> >> This looks to be a behavioral change beyond what title and
> >> >> description say, and it's not clear whether that's actually the
> >> >> behavior everyone wants.
> >> >
> >> > I'm fairly comfident its exactly the expected behavior when one uses
> >> > mem_access in altp2m tables and emulation. Right now because the lack
> of
> >> > this AFAIK emulation would not work correctly with altp2m. But Razvan
> >> > probably can chime in as he uses this path actively.
> >>
> >> I've done an experiment to see how much slower using altp2m would be as
> >> compared to emulation - so I'm not a big user of the feature, but I did
> >> find it cumbersome to have to work with two sets of APIs (one for what
> >> could arguably be called the default altp2m view, i.e. the regular
> >> xc_set_mem_access(), and one for altp2m, i.e.
> >> xc_altp2m_set_mem_access()). Furthermore, the APIs do not currently
> >> offer the same features (most notably, xc_altp2m_get_mem_access() is
> >> completely missing). I've mentioned this to Tamas while initially trying
> >> to get it to work.
> >>
> >> Now, whether the behaviour I expect is what everyone expects is, of
> >> course, wide open to debate. But I think we can all agree that the
> >> altp2m interface can, and probably should, be improved.
> >>
> >
> > There is that, but also, what is the exact logic behind doing this check
> > before emulation? AFAIU emulation happens in response to a vm_event so
> > we should be fairly certain that this check succeeds as it just verifies
> > that indeed the permissions are restricted by mem_access in the p2m (and
> > with altp2m this should be the active one). But when is this check
> > normally expected to fail?
>
> That check is important, please do not remove it. A vm_event is sent
> into userspace to our monitoring application, but the monitoring
> application can actually remove the page restrictions before replying,
> so in that case emulation is pointless - there will be no more page
> faults for that instruction.
>

I see, but then why would you reply with VM_EVENT_FLAG_EMULATE? You know
you removed the permission before sending the reply, so this sounds like
something specific to your application.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 5237 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Razvan Cojocaru]

On 01/28/2016 05:58 PM, Lengyel, Tamas wrote:
> 
> 
> On Thu, Jan 28, 2016 at 8:20 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     On 01/28/2016 05:12 PM, Lengyel, Tamas wrote:
>     >
>     > On Jan 28, 2016 8:02 AM, "Razvan Cojocaru" <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
>     > <mailto:rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>>> wrote:
>     >>
>     >> On 01/28/2016 04:42 PM, Lengyel, Tamas wrote:
>     >> >
>     >> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com <mailto:JBeulich@suse.com>
>     > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>
>     >> > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>
>     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>>> wrote:
>     >> >>
>     >> >> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com <mailto:tlengyel@novetta.com>
>     > <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>
>     >> > <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>
>     <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>>> wrote:
>     >> >> > --- a/xen/arch/x86/mm/p2m.c
>     >> >> > +++ b/xen/arch/x86/mm/p2m.c
>     >> >> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct
>     > vcpu *v,
>     >> >> >          bool_t violation = 1;
>     >> >> >          const struct vm_event_mem_access *data =
>     &rsp->u.mem_access;
>     >> >> >
>     >> >> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
>     >> > &access) == 0 )
>     >> >> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
>     >> >> > +                                altp2m_active(v->domain) ?
>     >> > vcpu_altp2m(v).p2midx : 0,
>     >> >> > +                                &access) == 0 )
>     >> >>
>     >> >> This looks to be a behavioral change beyond what title and
>     >> >> description say, and it's not clear whether that's actually the
>     >> >> behavior everyone wants.
>     >> >
>     >> > I'm fairly comfident its exactly the expected behavior when one
>     uses
>     >> > mem_access in altp2m tables and emulation. Right now because
>     the lack of
>     >> > this AFAIK emulation would not work correctly with altp2m. But
>     Razvan
>     >> > probably can chime in as he uses this path actively.
>     >>
>     >> I've done an experiment to see how much slower using altp2m would
>     be as
>     >> compared to emulation - so I'm not a big user of the feature, but
>     I did
>     >> find it cumbersome to have to work with two sets of APIs (one for
>     what
>     >> could arguably be called the default altp2m view, i.e. the regular
>     >> xc_set_mem_access(), and one for altp2m, i.e.
>     >> xc_altp2m_set_mem_access()). Furthermore, the APIs do not currently
>     >> offer the same features (most notably, xc_altp2m_get_mem_access() is
>     >> completely missing). I've mentioned this to Tamas while initially
>     trying
>     >> to get it to work.
>     >>
>     >> Now, whether the behaviour I expect is what everyone expects is, of
>     >> course, wide open to debate. But I think we can all agree that the
>     >> altp2m interface can, and probably should, be improved.
>     >>
>     >
>     > There is that, but also, what is the exact logic behind doing this
>     check
>     > before emulation? AFAIU emulation happens in response to a vm_event so
>     > we should be fairly certain that this check succeeds as it just
>     verifies
>     > that indeed the permissions are restricted by mem_access in the
>     p2m (and
>     > with altp2m this should be the active one). But when is this check
>     > normally expected to fail?
> 
>     That check is important, please do not remove it. A vm_event is sent
>     into userspace to our monitoring application, but the monitoring
>     application can actually remove the page restrictions before replying,
>     so in that case emulation is pointless - there will be no more page
>     faults for that instruction.
> 
> 
> I see, but then why would you reply with VM_EVENT_FLAG_EMULATE? You know
> you removed the permission before sending the reply, so this sounds like
> something specific to your application.

It's cheap insurance that things go right. If there's some issue with
page rights, or some external tool somehow does an xc_set_mem_access(),
things won't go wrong. And they will go wrong if Xen thinks it should
emulate the next instruction and the next instruction is not the one
that has caused the original fault. I would think that benefits any
application.


Thanks,
Razvan

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 5537 bytes --]

On Thu, Jan 28, 2016 at 9:32 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 01/28/2016 05:58 PM, Lengyel, Tamas wrote:
> >
> >
> > On Thu, Jan 28, 2016 at 8:20 AM, Razvan Cojocaru
> > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> >
> >     On 01/28/2016 05:12 PM, Lengyel, Tamas wrote:
> >     >
> >     > On Jan 28, 2016 8:02 AM, "Razvan Cojocaru" <
> rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
> >     > <mailto:rcojocaru@bitdefender.com <mailto:
> rcojocaru@bitdefender.com>>> wrote:
> >     >>
> >     >> On 01/28/2016 04:42 PM, Lengyel, Tamas wrote:
> >     >> >
> >     >> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com
> <mailto:JBeulich@suse.com>
> >     > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>
> >     >> > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>
> >     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>>> wrote:
> >     >> >>
> >     >> >> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com <mailto:
> tlengyel@novetta.com>
> >     > <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>
> >     >> > <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>
> >     <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>>> wrote:
> >     >> >> > --- a/xen/arch/x86/mm/p2m.c
> >     >> >> > +++ b/xen/arch/x86/mm/p2m.c
> >     >> >> > @@ -1572,7 +1572,9 @@ void
> p2m_mem_access_emulate_check(struct
> >     > vcpu *v,
> >     >> >> >          bool_t violation = 1;
> >     >> >> >          const struct vm_event_mem_access *data =
> >     &rsp->u.mem_access;
> >     >> >> >
> >     >> >> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> >     >> > &access) == 0 )
> >     >> >> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> >     >> >> > +                                altp2m_active(v->domain) ?
> >     >> > vcpu_altp2m(v).p2midx : 0,
> >     >> >> > +                                &access) == 0 )
> >     >> >>
> >     >> >> This looks to be a behavioral change beyond what title and
> >     >> >> description say, and it's not clear whether that's actually the
> >     >> >> behavior everyone wants.
> >     >> >
> >     >> > I'm fairly comfident its exactly the expected behavior when one
> >     uses
> >     >> > mem_access in altp2m tables and emulation. Right now because
> >     the lack of
> >     >> > this AFAIK emulation would not work correctly with altp2m. But
> >     Razvan
> >     >> > probably can chime in as he uses this path actively.
> >     >>
> >     >> I've done an experiment to see how much slower using altp2m would
> >     be as
> >     >> compared to emulation - so I'm not a big user of the feature, but
> >     I did
> >     >> find it cumbersome to have to work with two sets of APIs (one for
> >     what
> >     >> could arguably be called the default altp2m view, i.e. the regular
> >     >> xc_set_mem_access(), and one for altp2m, i.e.
> >     >> xc_altp2m_set_mem_access()). Furthermore, the APIs do not
> currently
> >     >> offer the same features (most notably, xc_altp2m_get_mem_access()
> is
> >     >> completely missing). I've mentioned this to Tamas while initially
> >     trying
> >     >> to get it to work.
> >     >>
> >     >> Now, whether the behaviour I expect is what everyone expects is,
> of
> >     >> course, wide open to debate. But I think we can all agree that the
> >     >> altp2m interface can, and probably should, be improved.
> >     >>
> >     >
> >     > There is that, but also, what is the exact logic behind doing this
> >     check
> >     > before emulation? AFAIU emulation happens in response to a
> vm_event so
> >     > we should be fairly certain that this check succeeds as it just
> >     verifies
> >     > that indeed the permissions are restricted by mem_access in the
> >     p2m (and
> >     > with altp2m this should be the active one). But when is this check
> >     > normally expected to fail?
> >
> >     That check is important, please do not remove it. A vm_event is sent
> >     into userspace to our monitoring application, but the monitoring
> >     application can actually remove the page restrictions before
> replying,
> >     so in that case emulation is pointless - there will be no more page
> >     faults for that instruction.
> >
> >
> > I see, but then why would you reply with VM_EVENT_FLAG_EMULATE? You know
> > you removed the permission before sending the reply, so this sounds like
> > something specific to your application.
>
> It's cheap insurance that things go right. If there's some issue with
> page rights, or some external tool somehow does an xc_set_mem_access(),
> things won't go wrong.


I can see this working for your application if you don't cache the
mem_access permissions locally and you don't want to query for it before
deciding to send the emulate flag in the response or not. Although, I think
that would be the best way to go here.


> And they will go wrong if Xen thinks it should
> emulate the next instruction and the next instruction is not the one
> that has caused the original fault.


How could that happen? When the vCPU is resumed after the fault, isn't the
same instruction guaranteed to be retried?


> I would think that benefits any
> application.
>

It's just a bit of an obscure exception. From an API perspective I would
rather have Xen do what I tell it to do - in this case emulate - rather
then it doing something else silently behind the scenes that you really
only find out about if you read the code.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 8696 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Razvan Cojocaru]

On 01/28/2016 06:40 PM, Lengyel, Tamas wrote:
> 
> 
> On Thu, Jan 28, 2016 at 9:32 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     On 01/28/2016 05:58 PM, Lengyel, Tamas wrote:
>     >
>     >
>     > On Thu, Jan 28, 2016 at 8:20 AM, Razvan Cojocaru
>     > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
>     <mailto:rcojocaru@bitdefender.com
>     <mailto:rcojocaru@bitdefender.com>>> wrote:
>     >
>     >     On 01/28/2016 05:12 PM, Lengyel, Tamas wrote:
>     >     >
>     >     > On Jan 28, 2016 8:02 AM, "Razvan Cojocaru" <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
>     <mailto:rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>>
>     >     > <mailto:rcojocaru@bitdefender.com
>     <mailto:rcojocaru@bitdefender.com> <mailto:rcojocaru@bitdefender.com
>     <mailto:rcojocaru@bitdefender.com>>>> wrote:
>     >     >>
>     >     >> On 01/28/2016 04:42 PM, Lengyel, Tamas wrote:
>     >     >> >
>     >     >> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com
>     <mailto:JBeulich@suse.com> <mailto:JBeulich@suse.com
>     <mailto:JBeulich@suse.com>>
>     >     > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>
>     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>>
>     >     >> > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>
>     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>
>     >     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>
>     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>>>> wrote:
>     >     >> >>
>     >     >> >> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com
>     <mailto:tlengyel@novetta.com> <mailto:tlengyel@novetta.com
>     <mailto:tlengyel@novetta.com>>
>     >     > <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>
>     <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>>
>     >     >> > <mailto:tlengyel@novetta.com
>     <mailto:tlengyel@novetta.com> <mailto:tlengyel@novetta.com
>     <mailto:tlengyel@novetta.com>>
>     >     <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>
>     <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>>>> wrote:
>     >     >> >> > --- a/xen/arch/x86/mm/p2m.c
>     >     >> >> > +++ b/xen/arch/x86/mm/p2m.c
>     >     >> >> > @@ -1572,7 +1572,9 @@ void
>     p2m_mem_access_emulate_check(struct
>     >     > vcpu *v,
>     >     >> >> >          bool_t violation = 1;
>     >     >> >> >          const struct vm_event_mem_access *data =
>     >     &rsp->u.mem_access;
>     >     >> >> >
>     >     >> >> > -        if ( p2m_get_mem_access(v->domain,
>     _gfn(data->gfn),
>     >     >> > &access) == 0 )
>     >     >> >> > +        if ( p2m_get_mem_access(v->domain,
>     _gfn(data->gfn),
>     >     >> >> > +                               
>     altp2m_active(v->domain) ?
>     >     >> > vcpu_altp2m(v).p2midx : 0,
>     >     >> >> > +                                &access) == 0 )
>     >     >> >>
>     >     >> >> This looks to be a behavioral change beyond what title and
>     >     >> >> description say, and it's not clear whether that's
>     actually the
>     >     >> >> behavior everyone wants.
>     >     >> >
>     >     >> > I'm fairly comfident its exactly the expected behavior
>     when one
>     >     uses
>     >     >> > mem_access in altp2m tables and emulation. Right now because
>     >     the lack of
>     >     >> > this AFAIK emulation would not work correctly with
>     altp2m. But
>     >     Razvan
>     >     >> > probably can chime in as he uses this path actively.
>     >     >>
>     >     >> I've done an experiment to see how much slower using altp2m
>     would
>     >     be as
>     >     >> compared to emulation - so I'm not a big user of the
>     feature, but
>     >     I did
>     >     >> find it cumbersome to have to work with two sets of APIs
>     (one for
>     >     what
>     >     >> could arguably be called the default altp2m view, i.e. the
>     regular
>     >     >> xc_set_mem_access(), and one for altp2m, i.e.
>     >     >> xc_altp2m_set_mem_access()). Furthermore, the APIs do not
>     currently
>     >     >> offer the same features (most notably,
>     xc_altp2m_get_mem_access() is
>     >     >> completely missing). I've mentioned this to Tamas while
>     initially
>     >     trying
>     >     >> to get it to work.
>     >     >>
>     >     >> Now, whether the behaviour I expect is what everyone
>     expects is, of
>     >     >> course, wide open to debate. But I think we can all agree
>     that the
>     >     >> altp2m interface can, and probably should, be improved.
>     >     >>
>     >     >
>     >     > There is that, but also, what is the exact logic behind
>     doing this
>     >     check
>     >     > before emulation? AFAIU emulation happens in response to a
>     vm_event so
>     >     > we should be fairly certain that this check succeeds as it just
>     >     verifies
>     >     > that indeed the permissions are restricted by mem_access in the
>     >     p2m (and
>     >     > with altp2m this should be the active one). But when is this
>     check
>     >     > normally expected to fail?
>     >
>     >     That check is important, please do not remove it. A vm_event
>     is sent
>     >     into userspace to our monitoring application, but the monitoring
>     >     application can actually remove the page restrictions before
>     replying,
>     >     so in that case emulation is pointless - there will be no more
>     page
>     >     faults for that instruction.
>     >
>     >
>     > I see, but then why would you reply with VM_EVENT_FLAG_EMULATE?
>     You know
>     > you removed the permission before sending the reply, so this
>     sounds like
>     > something specific to your application.
> 
>     It's cheap insurance that things go right. If there's some issue with
>     page rights, or some external tool somehow does an xc_set_mem_access(),
>     things won't go wrong.
> 
> 
> I can see this working for your application if you don't cache the
> mem_access permissions locally and you don't want to query for it before
> deciding to send the emulate flag in the response or not. Although, I
> think that would be the best way to go here.

Querying is out of the question, for obvious performance reasons. That's
why we've cached the registers in the vm_event request - we could have
not done that and instead just query them via libxc. But one small
decision like that and the monitored guest is running twice as slow.
This way, you can just set the emulate flag and have the hypervisor do
the right thing anyway, with no extra userspace <-> hypervisor roundtrips.

Caching might work, but then again that's extra work, memory used in the
application (in _each_ application, not just ours). So on one hand, we
have the current scenario where things can't go wrong and the solution
is in one place, vs. the other scenario, where each application needs to
solve the problem by doing tracking / caching / querying that the HV
does anyway in p2m, and pay with a possible guest crash or freeze for
failure.

>     And they will go wrong if Xen thinks it should
>     emulate the next instruction and the next instruction is not the one
>     that has caused the original fault.
> 
> 
> How could that happen? When the vCPU is resumed after the fault, isn't
> the same instruction guaranteed to be retried?

The instruction is the same, but if the page restrictions have been
lifted (somehow) and the EMULATE flag is still set, the original
instruction will run normally (because it won't trigger another page
fault). But the HV will still think that it needs to emulate the next
page fault, and so it will emulate whatever instruction causes the next
page fault (if it matches the emulate conditions).

>     I would think that benefits any
>     application.
> 
> 
> It's just a bit of an obscure exception. From an API perspective I would
> rather have Xen do what I tell it to do - in this case emulate - rather
> then it doing something else silently behind the scenes that you really
> only find out about if you read the code.

But the way the emulation code works now, it _can't_ emulate (see above
explanation). Emulation currently only happens as a result of a page
fault, and there will be no page fault if the page restriction are
lifted. I am thinking about a better way to achieve this, but until then
I think it's a good idea to keep the check in.

I hope I've been able to shed more light on this.


Thanks,
Razvan

Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 9739 bytes --]

On Thu, Jan 28, 2016 at 10:04 AM, Razvan Cojocaru <rcojocaru@bitdefender.com
> wrote:

> On 01/28/2016 06:40 PM, Lengyel, Tamas wrote:
> >
> >
> > On Thu, Jan 28, 2016 at 9:32 AM, Razvan Cojocaru
> > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> >
> >     On 01/28/2016 05:58 PM, Lengyel, Tamas wrote:
> >     >
> >     >
> >     > On Thu, Jan 28, 2016 at 8:20 AM, Razvan Cojocaru
> >     > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
> >     <mailto:rcojocaru@bitdefender.com
> >     <mailto:rcojocaru@bitdefender.com>>> wrote:
> >     >
> >     >     On 01/28/2016 05:12 PM, Lengyel, Tamas wrote:
> >     >     >
> >     >     > On Jan 28, 2016 8:02 AM, "Razvan Cojocaru" <
> rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
> >     <mailto:rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com
> >>
> >     >     > <mailto:rcojocaru@bitdefender.com
> >     <mailto:rcojocaru@bitdefender.com> <mailto:rcojocaru@bitdefender.com
> >     <mailto:rcojocaru@bitdefender.com>>>> wrote:
> >     >     >>
> >     >     >> On 01/28/2016 04:42 PM, Lengyel, Tamas wrote:
> >     >     >> >
> >     >     >> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com
> >     <mailto:JBeulich@suse.com> <mailto:JBeulich@suse.com
> >     <mailto:JBeulich@suse.com>>
> >     >     > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>
> >     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>>
> >     >     >> > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>
> >     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>
> >     >     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>
> >     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>>>> wrote:
> >     >     >> >>
> >     >     >> >> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com
> >     <mailto:tlengyel@novetta.com> <mailto:tlengyel@novetta.com
> >     <mailto:tlengyel@novetta.com>>
> >     >     > <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>
> >     <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>>
> >     >     >> > <mailto:tlengyel@novetta.com
> >     <mailto:tlengyel@novetta.com> <mailto:tlengyel@novetta.com
> >     <mailto:tlengyel@novetta.com>>
> >     >     <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>
> >     <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>>>>
> wrote:
> >     >     >> >> > --- a/xen/arch/x86/mm/p2m.c
> >     >     >> >> > +++ b/xen/arch/x86/mm/p2m.c
> >     >     >> >> > @@ -1572,7 +1572,9 @@ void
> >     p2m_mem_access_emulate_check(struct
> >     >     > vcpu *v,
> >     >     >> >> >          bool_t violation = 1;
> >     >     >> >> >          const struct vm_event_mem_access *data =
> >     >     &rsp->u.mem_access;
> >     >     >> >> >
> >     >     >> >> > -        if ( p2m_get_mem_access(v->domain,
> >     _gfn(data->gfn),
> >     >     >> > &access) == 0 )
> >     >     >> >> > +        if ( p2m_get_mem_access(v->domain,
> >     _gfn(data->gfn),
> >     >     >> >> > +
> >     altp2m_active(v->domain) ?
> >     >     >> > vcpu_altp2m(v).p2midx : 0,
> >     >     >> >> > +                                &access) == 0 )
> >     >     >> >>
> >     >     >> >> This looks to be a behavioral change beyond what title
> and
> >     >     >> >> description say, and it's not clear whether that's
> >     actually the
> >     >     >> >> behavior everyone wants.
> >     >     >> >
> >     >     >> > I'm fairly comfident its exactly the expected behavior
> >     when one
> >     >     uses
> >     >     >> > mem_access in altp2m tables and emulation. Right now
> because
> >     >     the lack of
> >     >     >> > this AFAIK emulation would not work correctly with
> >     altp2m. But
> >     >     Razvan
> >     >     >> > probably can chime in as he uses this path actively.
> >     >     >>
> >     >     >> I've done an experiment to see how much slower using altp2m
> >     would
> >     >     be as
> >     >     >> compared to emulation - so I'm not a big user of the
> >     feature, but
> >     >     I did
> >     >     >> find it cumbersome to have to work with two sets of APIs
> >     (one for
> >     >     what
> >     >     >> could arguably be called the default altp2m view, i.e. the
> >     regular
> >     >     >> xc_set_mem_access(), and one for altp2m, i.e.
> >     >     >> xc_altp2m_set_mem_access()). Furthermore, the APIs do not
> >     currently
> >     >     >> offer the same features (most notably,
> >     xc_altp2m_get_mem_access() is
> >     >     >> completely missing). I've mentioned this to Tamas while
> >     initially
> >     >     trying
> >     >     >> to get it to work.
> >     >     >>
> >     >     >> Now, whether the behaviour I expect is what everyone
> >     expects is, of
> >     >     >> course, wide open to debate. But I think we can all agree
> >     that the
> >     >     >> altp2m interface can, and probably should, be improved.
> >     >     >>
> >     >     >
> >     >     > There is that, but also, what is the exact logic behind
> >     doing this
> >     >     check
> >     >     > before emulation? AFAIU emulation happens in response to a
> >     vm_event so
> >     >     > we should be fairly certain that this check succeeds as it
> just
> >     >     verifies
> >     >     > that indeed the permissions are restricted by mem_access in
> the
> >     >     p2m (and
> >     >     > with altp2m this should be the active one). But when is this
> >     check
> >     >     > normally expected to fail?
> >     >
> >     >     That check is important, please do not remove it. A vm_event
> >     is sent
> >     >     into userspace to our monitoring application, but the
> monitoring
> >     >     application can actually remove the page restrictions before
> >     replying,
> >     >     so in that case emulation is pointless - there will be no more
> >     page
> >     >     faults for that instruction.
> >     >
> >     >
> >     > I see, but then why would you reply with VM_EVENT_FLAG_EMULATE?
> >     You know
> >     > you removed the permission before sending the reply, so this
> >     sounds like
> >     > something specific to your application.
> >
> >     It's cheap insurance that things go right. If there's some issue with
> >     page rights, or some external tool somehow does an
> xc_set_mem_access(),
> >     things won't go wrong.
> >
> >
> > I can see this working for your application if you don't cache the
> > mem_access permissions locally and you don't want to query for it before
> > deciding to send the emulate flag in the response or not. Although, I
> > think that would be the best way to go here.
>
> Querying is out of the question, for obvious performance reasons. That's
> why we've cached the registers in the vm_event request - we could have
> not done that and instead just query them via libxc. But one small
> decision like that and the monitored guest is running twice as slow.
> This way, you can just set the emulate flag and have the hypervisor do
> the right thing anyway, with no extra userspace <-> hypervisor roundtrips.
>
> Caching might work, but then again that's extra work, memory used in the
> application (in _each_ application, not just ours). So on one hand, we
> have the current scenario where things can't go wrong and the solution
> is in one place, vs. the other scenario, where each application needs to
> solve the problem by doing tracking / caching / querying that the HV
> does anyway in p2m, and pay with a possible guest crash or freeze for
> failure.
>
> >     And they will go wrong if Xen thinks it should
> >     emulate the next instruction and the next instruction is not the one
> >     that has caused the original fault.
> >
> >
> > How could that happen? When the vCPU is resumed after the fault, isn't
> > the same instruction guaranteed to be retried?
>
> The instruction is the same, but if the page restrictions have been
> lifted (somehow) and the EMULATE flag is still set, the original
> instruction will run normally (because it won't trigger another page
> fault). But the HV will still think that it needs to emulate the next
> page fault, and so it will emulate whatever instruction causes the next
> page fault (if it matches the emulate conditions).
>
> >     I would think that benefits any
> >     application.
> >
> >
> > It's just a bit of an obscure exception. From an API perspective I would
> > rather have Xen do what I tell it to do - in this case emulate - rather
> > then it doing something else silently behind the scenes that you really
> > only find out about if you read the code.
>
> But the way the emulation code works now, it _can't_ emulate (see above
> explanation). Emulation currently only happens as a result of a page
> fault, and there will be no page fault if the page restriction are
> lifted. I am thinking about a better way to achieve this, but until then
> I think it's a good idea to keep the check in.
>
> I hope I've been able to shed more light on this.
>

Sure, make sense. Since AFAIK you guys are the only one really using this
path I'm cool with keeping it as it is, was really just wondering for the
logic behind it. Without a reference implementation using this path it's
not exactly trivial trying to figure out why things are the way they are.

Jan,
with the explanation above by Razvan, when using emulation with altp2m the
correct check here is to see if the altp2m permissions are still
restricted, otherwise no need to emulate. So this patch actually makes the
two systems correctly work together. Without this patch only the hostp2m
permissions are checked which may not have the restrictions that actually
caused the fault and lead to infinite faults and hanging the VM.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 15030 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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