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

[PATCH v2 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
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>
---
v2: Don't deprecate the HVMOP hypercall for setting mem_access
    Use unsigned int instead of unsigned long
---
 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              |   6 +-
 xen/arch/x86/mm/p2m.c               | 169 ++++++++++++++++--------------------
 xen/common/mem_access.c             |   2 +-
 xen/include/asm-x86/p2m.h           |   4 -
 xen/include/public/memory.h         |   3 +
 xen/include/xen/p2m-common.h        |   3 +-
 11 files changed, 117 insertions(+), 176 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..8568087 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 int 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..37305fb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6398,9 +6398,9 @@ static int do_altp2m_op(
         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);
+            rc = p2m_set_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0,
+                                    a.u.set_mem_access.hvmmem_access,
+                                    a.u.set_mem_access.view);
         break;
 
     case HVMOP_altp2m_change_gfn:
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index a45ee35..a3181f3 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 int 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/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..8b70459 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 int altp2m_idx);
 
 /*
  * Get access type for a gfn.
-- 
2.1.4

[PATCH v2 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>
---
v2: Use unsigned int instead of unsigned long
    Use a single p2m pointer
---
 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               | 17 +++++++++++++++--
 xen/common/mem_access.c             |  2 +-
 xen/include/xen/p2m-common.h        |  3 ++-
 7 files changed, 31 insertions(+), 11 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 8568087..957fa57 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 int 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 a3181f3..b994564 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,7 +1920,8 @@ 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 int altp2m_idx,
+                       xenmem_access_t *access)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_type_t t;
@@ -1947,6 +1950,16 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *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];
+    }
+
     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 8b70459..342bc4b 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 int altp2m_idx,
+                       xenmem_access_t *access);
 
 #endif /* _XEN_P2M_COMMON_H */
-- 
2.1.4

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

[Jan Beulich]

>>> On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
> --- 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)

I think new functions would better not use "unsigned long" for frame
numbers.

> +{
> +    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;
> +}

With there not being any involved error handling here, I don't think
using a label and goto is warranted here. But I'll leave the ultimate
decision to George, of course.

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

Repeating what I had said on v1: 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.

Jan

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

[Lengyel, Tamas]

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

On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
> > --- 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)
>
> I think new functions would better not use "unsigned long" for frame
> numbers.
>

The only place this is called from the gfn is already converted to unsigned
long. I don't see much point in converting it back to gfn_t and then back
to unsigned long again.. I was thinking this function may even be declared
as inline?


>
> > +{
> > +    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;
> > +}
>
> With there not being any involved error handling here, I don't think
> using a label and goto is warranted here. But I'll leave the ultimate
> decision to George, of course.
>

RIght, this is a remnant from the previous version of this function where
out also had the p2m_unlock. Now that it is just a return I could do the
return in place of the gotos. Let me know which one is preferred.


>
> > --- 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;
>
> Repeating what I had said on v1: 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.
>

Not sure what I can do to address this.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4705 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 v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Jan Beulich]

>>> On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
> On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
>> > --- 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)
>>
>> I think new functions would better not use "unsigned long" for frame
>> numbers.
>>
> 
> The only place this is called from the gfn is already converted to unsigned
> long. I don't see much point in converting it back to gfn_t and then back
> to unsigned long again..

If you recall, the goal is to have all frame numbers passed around
have distinguishable types.

> I was thinking this function may even be declared as inline?

This is orthogonal (and I really don't care much).

>> > +{
>> > +    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;
>> > +}
>>
>> With there not being any involved error handling here, I don't think
>> using a label and goto is warranted here. But I'll leave the ultimate
>> decision to George, of course.
> 
> RIght, this is a remnant from the previous version of this function where
> out also had the p2m_unlock. Now that it is just a return I could do the
> return in place of the gotos. Let me know which one is preferred.

Since you sent this to me - my general request is to avoid goto
wherever possible.

>> > --- 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;
>>
>> Repeating what I had said on v1: 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.
> 
> Not sure what I can do to address this.

Deprecate the old interface and introduce a new one. But other
maintainers' opinions would be welcome.

Jan

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

[Lengyel, Tamas]

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

On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
> > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
> >> > --- 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)
> >>
> >> I think new functions would better not use "unsigned long" for frame
> >> numbers.
> >>
> >
> > The only place this is called from the gfn is already converted to
> unsigned
> > long. I don't see much point in converting it back to gfn_t and then back
> > to unsigned long again..
>
> If you recall, the goal is to have all frame numbers passed around
> have distinguishable types.
>

Fine by me, seems a bit of a waste but probably not noticeable.


>
> > I was thinking this function may even be declared as inline?
>
> This is orthogonal (and I really don't care much).
>

Well, AFAIK with an inline it wouldn't technically be passed as the code
would be compiled into the other function. But I figure the idea is to have
the compiler catch type related problems in general. I'll make it inline
and just let the compiler optimization figure out if it really needs the
conversion or not while.


>
> >> > +{
> >> > +    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;
> >> > +}
> >>
> >> With there not being any involved error handling here, I don't think
> >> using a label and goto is warranted here. But I'll leave the ultimate
> >> decision to George, of course.
> >
> > RIght, this is a remnant from the previous version of this function where
> > out also had the p2m_unlock. Now that it is just a return I could do the
> > return in place of the gotos. Let me know which one is preferred.
>
> Since you sent this to me - my general request is to avoid goto
> wherever possible.
>

Sure.


>
> >> > --- 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;
> >>
> >> Repeating what I had said on v1: 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.
> >
> > Not sure what I can do to address this.
>
> Deprecate the old interface and introduce a new one. But other
> maintainers' opinions would be welcome.
>

That seems like a very heavy handed solution to me.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 6839 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 v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Jan Beulich]

>>> On 29.01.16 at 17:32, <tlengyel@novetta.com> wrote:
> On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
>> > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
>> >> > --- 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;
>> >>
>> >> Repeating what I had said on v1: 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.
>> >
>> > Not sure what I can do to address this.
>>
>> Deprecate the old interface and introduce a new one. But other
>> maintainers' opinions would be welcome.
> 
> That seems like a very heavy handed solution to me.

I understand that - hence the request for others' opinions.

Jan

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

[Wei Liu]

On Thu, Jan 28, 2016 at 01:58:07PM -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
> 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>
> ---
> v2: Don't deprecate the HVMOP hypercall for setting mem_access
>     Use unsigned int instead of unsigned long
> ---
>  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 ++++-------

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[Ian Campbell]

On Fri, 2016-01-29 at 09:47 -0700, Jan Beulich wrote:
> > > > On 29.01.16 at 17:32, <tlengyel@novetta.com> wrote:
> > On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
> > > > > > On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
> > > > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@suse.com>
> > > > wrote:
> > > > > > > > On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
> > > > > > --- 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;
> > > > > 
> > > > > Repeating what I had said on v1: 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.
> > > > 
> > > > Not sure what I can do to address this.
> > > 
> > > Deprecate the old interface and introduce a new one. But other
> > > maintainers' opinions would be welcome.
> > 
> > That seems like a very heavy handed solution to me.
> 
> I understand that - hence the request for others' opinions.

It's unfortunate that we've found ourselves here, but I think rather than
deprecating the current and adding a new op alongside we should just accept
the one-time fragility this time around, add the version field as part of
this set of changes and try and remember to include a version number for
next time we add a tools only interface. I don't think xenaccess is yet
widely used outside of Tamas and the Bitdfender folks, who I would assume
can cope with such a change.

I could accept changing the op number would make sense, but I don't think
we should deprecate the old one (which implies continuing to support it in
parallel), if we go this route we should just retire the old number to
straight away to return -ENOSYS (or maybe -EACCESS, which is what a version
mismatch would have resulted in).

Ian.


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

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

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access"):
> It's unfortunate that we've found ourselves here, but I think rather than
> deprecating the current and adding a new op alongside we should just accept
> the one-time fragility this time around, add the version field as part of
> this set of changes and try and remember to include a version number for
> next time we add a tools only interface. I don't think xenaccess is yet
> widely used outside of Tamas and the Bitdfender folks, who I would assume
> can cope with such a change.

I'm not sure what a separate version number buys us.

> I could accept changing the op number would make sense, but I don't think
> we should deprecate the old one (which implies continuing to support it in
> parallel), if we go this route we should just retire the old number to
> straight away to return -ENOSYS (or maybe -EACCESS, which is what a version
> mismatch would have resulted in).

If we simply want to detect the mismatch that seems like the best
approach.

It's not like we're short of memory op values.

Ian.

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

[Jan Beulich]

>>> On 01.02.16 at 15:45, <ian.campbell@citrix.com> wrote:
> On Fri, 2016-01-29 at 09:47 -0700, Jan Beulich wrote:
>> > > > On 29.01.16 at 17:32, <tlengyel@novetta.com> wrote:
>> > On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> > > > > > On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
>> > > > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@suse.com>
>> > > > wrote:
>> > > > > > > > On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
>> > > > > > --- 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;
>> > > > > 
>> > > > > Repeating what I had said on v1: 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.
>> > > > 
>> > > > Not sure what I can do to address this.
>> > > 
>> > > Deprecate the old interface and introduce a new one. But other
>> > > maintainers' opinions would be welcome.
>> > 
>> > That seems like a very heavy handed solution to me.
>> 
>> I understand that - hence the request for others' opinions.
> 
> It's unfortunate that we've found ourselves here, but I think rather than
> deprecating the current and adding a new op alongside we should just accept
> the one-time fragility this time around, add the version field as part of
> this set of changes and try and remember to include a version number for
> next time we add a tools only interface. I don't think xenaccess is yet
> widely used outside of Tamas and the Bitdfender folks, who I would assume
> can cope with such a change.
> 
> I could accept changing the op number would make sense, but I don't think
> we should deprecate the old one (which implies continuing to support it in
> parallel), if we go this route we should just retire the old number to
> straight away to return -ENOSYS (or maybe -EACCESS, which is what a version
> mismatch would have resulted in).

That actually looks like a reasonable compromise, until we finally
manage to get around to morph the tools-only HVM-ops into a
new hvmctl hypercall (leaving only guest accessible ones in the
current interface).

Jan

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

[Ian Campbell]

On Mon, 2016-02-01 at 09:21 -0700, Jan Beulich wrote:
> > > > On 01.02.16 at 15:45, <ian.campbell@citrix.com> wrote:
> > On Fri, 2016-01-29 at 09:47 -0700, Jan Beulich wrote:
> > > > > > On 29.01.16 at 17:32, <tlengyel@novetta.com> wrote:
> > > > On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@suse.com>
> > > > wrote:
> > > > > > > > On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
> > > > > > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@suse.com
> > > > > > >
> > > > > > wrote:
> > > > > > > > > > On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
> > > > > > > > --- 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;
> > > > > > > 
> > > > > > > Repeating what I had said on v1: 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.
> > > > > > 
> > > > > > Not sure what I can do to address this.
> > > > > 
> > > > > Deprecate the old interface and introduce a new one. But other
> > > > > maintainers' opinions would be welcome.
> > > > 
> > > > That seems like a very heavy handed solution to me.
> > > 
> > > I understand that - hence the request for others' opinions.
> > 
> > It's unfortunate that we've found ourselves here, but I think rather
> > than
> > deprecating the current and adding a new op alongside we should just
> > accept
> > the one-time fragility this time around, add the version field as part
> > of
> > this set of changes and try and remember to include a version number
> > for
> > next time we add a tools only interface. I don't think xenaccess is yet
> > widely used outside of Tamas and the Bitdfender folks, who I would
> > assume
> > can cope with such a change.
> > 
> > I could accept changing the op number would make sense, but I don't
> > think
> > we should deprecate the old one (which implies continuing to support it
> > in
> > parallel), if we go this route we should just retire the old number to
> > straight away to return -ENOSYS (or maybe -EACCESS, which is what a
> > version
> > mismatch would have resulted in).
> 
> That actually looks like a reasonable compromise, until we finally
> manage to get around to morph the tools-only HVM-ops into a
> new hvmctl hypercall (leaving only guest accessible ones in the
> current interface).

Aren't the ones being discussed here xenmem subops rather than hvmops?

Ian.

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

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

[Lengyel, Tamas]

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

On Mon, Feb 1, 2016 at 9:30 AM, Ian Campbell <ian.campbell@citrix.com>
wrote:

> On Mon, 2016-02-01 at 09:21 -0700, Jan Beulich wrote:
> > > > > On 01.02.16 at 15:45, <ian.campbell@citrix.com> wrote:
> > > On Fri, 2016-01-29 at 09:47 -0700, Jan Beulich wrote:
> > > > > > > On 29.01.16 at 17:32, <tlengyel@novetta.com> wrote:
> > > > > On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@suse.com>
> > > > > wrote:
> > > > > > > > > On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
> > > > > > > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <
> JBeulich@suse.com
> > > > > > > >
> > > > > > > wrote:
> > > > > > > > > > > On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
> > > > > > > > > --- 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;
> > > > > > > >
> > > > > > > > Repeating what I had said on v1: 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.
> > > > > > >
> > > > > > > Not sure what I can do to address this.
> > > > > >
> > > > > > Deprecate the old interface and introduce a new one. But other
> > > > > > maintainers' opinions would be welcome.
> > > > >
> > > > > That seems like a very heavy handed solution to me.
> > > >
> > > > I understand that - hence the request for others' opinions.
> > >
> > > It's unfortunate that we've found ourselves here, but I think rather
> > > than
> > > deprecating the current and adding a new op alongside we should just
> > > accept
> > > the one-time fragility this time around, add the version field as part
> > > of
> > > this set of changes and try and remember to include a version number
> > > for
> > > next time we add a tools only interface. I don't think xenaccess is yet
> > > widely used outside of Tamas and the Bitdfender folks, who I would
> > > assume
> > > can cope with such a change.
> > >
> > > I could accept changing the op number would make sense, but I don't
> > > think
> > > we should deprecate the old one (which implies continuing to support it
> > > in
> > > parallel), if we go this route we should just retire the old number to
> > > straight away to return -ENOSYS (or maybe -EACCESS, which is what a
> > > version
> > > mismatch would have resulted in).
> >
> > That actually looks like a reasonable compromise, until we finally
> > manage to get around to morph the tools-only HVM-ops into a
> > new hvmctl hypercall (leaving only guest accessible ones in the
> > current interface).
>
> Aren't the ones being discussed here xenmem subops rather than hvmops?
>

Yes, in v2 I'm not touching the guest-visible altp2m_set_mem_access hvmop,
just how it is handled internal to Xen. I do wonder if it was intended to
be guest-visible operation though or if that was an overlook of not putting
it into tools-only.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 5407 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 v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Jan Beulich]

>>> On 01.02.16 at 17:30, <ian.campbell@citrix.com> wrote:
> On Mon, 2016-02-01 at 09:21 -0700, Jan Beulich wrote:
>> > > > On 01.02.16 at 15:45, <ian.campbell@citrix.com> wrote:
>> > On Fri, 2016-01-29 at 09:47 -0700, Jan Beulich wrote:
>> > > > > > On 29.01.16 at 17:32, <tlengyel@novetta.com> wrote:
>> > > > On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@suse.com>
>> > > > wrote:
>> > > > > > > > On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
>> > > > > > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@suse.com 
>> > > > > > >
>> > > > > > wrote:
>> > > > > > > > > > On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
>> > > > > > > > --- 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;
>> > > > > > > 
>> > > > > > > Repeating what I had said on v1: 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.
>> > > > > > 
>> > > > > > Not sure what I can do to address this.
>> > > > > 
>> > > > > Deprecate the old interface and introduce a new one. But other
>> > > > > maintainers' opinions would be welcome.
>> > > > 
>> > > > That seems like a very heavy handed solution to me.
>> > > 
>> > > I understand that - hence the request for others' opinions.
>> > 
>> > It's unfortunate that we've found ourselves here, but I think rather
>> > than
>> > deprecating the current and adding a new op alongside we should just
>> > accept
>> > the one-time fragility this time around, add the version field as part
>> > of
>> > this set of changes and try and remember to include a version number
>> > for
>> > next time we add a tools only interface. I don't think xenaccess is yet
>> > widely used outside of Tamas and the Bitdfender folks, who I would
>> > assume
>> > can cope with such a change.
>> > 
>> > I could accept changing the op number would make sense, but I don't
>> > think
>> > we should deprecate the old one (which implies continuing to support it
>> > in
>> > parallel), if we go this route we should just retire the old number to
>> > straight away to return -ENOSYS (or maybe -EACCESS, which is what a
>> > version
>> > mismatch would have resulted in).
>> 
>> That actually looks like a reasonable compromise, until we finally
>> manage to get around to morph the tools-only HVM-ops into a
>> new hvmctl hypercall (leaving only guest accessible ones in the
>> current interface).
> 
> Aren't the ones being discussed here xenmem subops rather than hvmops?

Oh, yes, right. I'm sorry for confusing things.

Jan

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

[Lengyel, Tamas]

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

On Mon, Feb 1, 2016 at 9:21 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 01.02.16 at 15:45, <ian.campbell@citrix.com> wrote:
> > On Fri, 2016-01-29 at 09:47 -0700, Jan Beulich wrote:
> >> > > > On 29.01.16 at 17:32, <tlengyel@novetta.com> wrote:
> >> > On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@suse.com>
> wrote:
> >> > > > > > On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
> >> > > > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@suse.com>
> >> > > > wrote:
> >> > > > > > > > On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
> >> > > > > > --- 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;
> >> > > > >
> >> > > > > Repeating what I had said on v1: 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.
> >> > > >
> >> > > > Not sure what I can do to address this.
> >> > >
> >> > > Deprecate the old interface and introduce a new one. But other
> >> > > maintainers' opinions would be welcome.
> >> >
> >> > That seems like a very heavy handed solution to me.
> >>
> >> I understand that - hence the request for others' opinions.
> >
> > It's unfortunate that we've found ourselves here, but I think rather than
> > deprecating the current and adding a new op alongside we should just
> accept
> > the one-time fragility this time around, add the version field as part of
> > this set of changes and try and remember to include a version number for
> > next time we add a tools only interface. I don't think xenaccess is yet
> > widely used outside of Tamas and the Bitdfender folks, who I would assume
> > can cope with such a change.
> >
> > I could accept changing the op number would make sense, but I don't think
> > we should deprecate the old one (which implies continuing to support it
> in
> > parallel)
>

Supporting the two versions in parallel is not much effort, we would just
have to add an extra check to the current version to fail if altp2m is
enabled.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4121 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 v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Jan Beulich]

>>> On 01.02.16 at 16:22, <Ian.Jackson@eu.citrix.com> wrote:
> Ian Campbell writes ("Re: [PATCH v2 1/2] altp2m: Merge 
> p2m_set_altp2m_mem_access and p2m_set_mem_access"):
>> It's unfortunate that we've found ourselves here, but I think rather than
>> deprecating the current and adding a new op alongside we should just accept
>> the one-time fragility this time around, add the version field as part of
>> this set of changes and try and remember to include a version number for
>> next time we add a tools only interface. I don't think xenaccess is yet
>> widely used outside of Tamas and the Bitdfender folks, who I would assume
>> can cope with such a change.
> 
> I'm not sure what a separate version number buys us.
> 
>> I could accept changing the op number would make sense, but I don't think
>> we should deprecate the old one (which implies continuing to support it in
>> parallel), if we go this route we should just retire the old number to
>> straight away to return -ENOSYS (or maybe -EACCESS, which is what a version
>> mismatch would have resulted in).
> 
> If we simply want to detect the mismatch that seems like the best
> approach.
> 
> It's not like we're short of memory op values.

Are we not? They need to fit in 6 bits (unless we want to play tricks),
and numbers up to 27 are already in use.

Jan

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

[Jan Beulich]

>>> On 01.02.16 at 17:35, <tlengyel@novetta.com> wrote:
> On Mon, Feb 1, 2016 at 9:30 AM, Ian Campbell <ian.campbell@citrix.com>
> wrote:
> 
>> On Mon, 2016-02-01 at 09:21 -0700, Jan Beulich wrote:
>> > > > > On 01.02.16 at 15:45, <ian.campbell@citrix.com> wrote:
>> > > On Fri, 2016-01-29 at 09:47 -0700, Jan Beulich wrote:
>> > > > > > > On 29.01.16 at 17:32, <tlengyel@novetta.com> wrote:
>> > > > > On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@suse.com>
>> > > > > wrote:
>> > > > > > > > > On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
>> > > > > > > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <
>> JBeulich@suse.com 
>> > > > > > > >
>> > > > > > > wrote:
>> > > > > > > > > > > On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote:
>> > > > > > > > > --- 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;
>> > > > > > > >
>> > > > > > > > Repeating what I had said on v1: 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.
>> > > > > > >
>> > > > > > > Not sure what I can do to address this.
>> > > > > >
>> > > > > > Deprecate the old interface and introduce a new one. But other
>> > > > > > maintainers' opinions would be welcome.
>> > > > >
>> > > > > That seems like a very heavy handed solution to me.
>> > > >
>> > > > I understand that - hence the request for others' opinions.
>> > >
>> > > It's unfortunate that we've found ourselves here, but I think rather
>> > > than
>> > > deprecating the current and adding a new op alongside we should just
>> > > accept
>> > > the one-time fragility this time around, add the version field as part
>> > > of
>> > > this set of changes and try and remember to include a version number
>> > > for
>> > > next time we add a tools only interface. I don't think xenaccess is yet
>> > > widely used outside of Tamas and the Bitdfender folks, who I would
>> > > assume
>> > > can cope with such a change.
>> > >
>> > > I could accept changing the op number would make sense, but I don't
>> > > think
>> > > we should deprecate the old one (which implies continuing to support it
>> > > in
>> > > parallel), if we go this route we should just retire the old number to
>> > > straight away to return -ENOSYS (or maybe -EACCESS, which is what a
>> > > version
>> > > mismatch would have resulted in).
>> >
>> > That actually looks like a reasonable compromise, until we finally
>> > manage to get around to morph the tools-only HVM-ops into a
>> > new hvmctl hypercall (leaving only guest accessible ones in the
>> > current interface).
>>
>> Aren't the ones being discussed here xenmem subops rather than hvmops?
> 
> Yes, in v2 I'm not touching the guest-visible altp2m_set_mem_access hvmop,
> just how it is handled internal to Xen. I do wonder if it was intended to
> be guest-visible operation though or if that was an overlook of not putting
> it into tools-only.

I think this was intentional, so a guest could control the access
rights in the various views of its P2M.

Jan

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

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access"):
> On 01.02.16 at 16:22, <Ian.Jackson@eu.citrix.com> wrote:
> > It's not like we're short of memory op values.
> 
> Are we not? They need to fit in 6 bits (unless we want to play tricks),
> and numbers up to 27 are already in use.

Maybe I am confused.  It's hard to make sense of the actual ABI which
doesn't seem to be documented yet.

...

I have just read the docs some more and found this:

  /*
   * To allow safe resume of do_memory_op() after preemption, we need
   * to know at what point in the page list to resume. For this
   * purpose I steal the high-order bits of the @cmd parameter, which
   * are otherwise unused and zero.
   *
   * Note that both of these values are effectively part of the ABI,
   * even if we don't need to make them a formal part of it: A guest
   * suspended for migration in the middle of a continuation would
   * fail to work if resumed on a hypervisor using different values.
   */
  #define MEMOP_EXTENT_SHIFT 6 /* cmd[:6] == start_extent */
  #define MEMOP_CMD_MASK     ((1 << MEMOP_EXTENT_SHIFT) - 1)

Urrrrggh!

If we run out of memory_op numbers, can't we just invent a new
hypercall ?

Ian.

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

[Jan Beulich]

>>> On 01.02.16 at 17:58, <Ian.Jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH v2 1/2] altp2m: Merge 
> p2m_set_altp2m_mem_access and p2m_set_mem_access"):
>> On 01.02.16 at 16:22, <Ian.Jackson@eu.citrix.com> wrote:
>> > It's not like we're short of memory op values.
>> 
>> Are we not? They need to fit in 6 bits (unless we want to play tricks),
>> and numbers up to 27 are already in use.
> 
> Maybe I am confused.  It's hard to make sense of the actual ABI which
> doesn't seem to be documented yet.
> 
> ...
> 
> I have just read the docs some more and found this:
> 
>   /*
>    * To allow safe resume of do_memory_op() after preemption, we need
>    * to know at what point in the page list to resume. For this
>    * purpose I steal the high-order bits of the @cmd parameter, which
>    * are otherwise unused and zero.
>    *
>    * Note that both of these values are effectively part of the ABI,
>    * even if we don't need to make them a formal part of it: A guest
>    * suspended for migration in the middle of a continuation would
>    * fail to work if resumed on a hypervisor using different values.
>    */
>   #define MEMOP_EXTENT_SHIFT 6 /* cmd[:6] == start_extent */
>   #define MEMOP_CMD_MASK     ((1 << MEMOP_EXTENT_SHIFT) - 1)
> 
> Urrrrggh!

No-one said this is nice. And we had an issue already when this
once got widened from (iirc) 4 to 6 bits.

> If we run out of memory_op numbers, can't we just invent a new
> hypercall ?

I suppose we would need to, yes.

Jan