[PATCH v2 0/3] Make mem_access APIs and hypercalls generic

[PATCH v2 0/3] Make mem_access APIs and hypercalls generic

[Aravindh Puthiyaparambil]

This is a precusor patch to the one that enables mem_access for PV domains.

The mem_access APIs, hypercalls and structures all have HVM specific naming. As
a first step to making this work for PV domains, this patch renames them in a
more generic fashion.

Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>

  x86/mem_access: Make the mem_access ops generic
  tools/libxc: Make the mem_access APIs generic
  tools/xen-access: Use the new mem_access APIs

 tools/libxc/xc_mem_access.c         | 54 ++++++++++++++++++++++++---
 tools/libxc/xc_misc.c               | 61 -------------------------------
 tools/libxc/xenctrl.h               | 32 ++++++++--------
 tools/tests/xen-access/xen-access.c | 34 ++++++++---------
 xen/arch/x86/hvm/hvm.c              | 73 +------------------------------------
 xen/arch/x86/mm.c                   |  6 ++-
 xen/arch/x86/mm/mem_access.c        | 71 ++++++++++++++++++++++++++++++++++--
 xen/arch/x86/mm/mem_event.c         |  3 --
 xen/arch/x86/mm/p2m.c               | 31 +++++++++-------
 xen/arch/x86/x86_64/compat/mm.c     | 16 +++++---
 xen/arch/x86/x86_64/mm.c            | 11 +++++-
 xen/common/compat/memory.c          |  2 +
 xen/common/memory.c                 |  2 +-
 xen/include/asm-x86/mem_access.h    |  3 +-
 xen/include/asm-x86/mm.h            |  6 +--
 xen/include/asm-x86/p2m.h           |  6 +--
 xen/include/public/hvm/hvm_op.h     | 42 +--------------------
 xen/include/public/memory.h         | 53 +++++++++++++++++++++++++--
 xen/include/xlat.lst                |  1 +
 19 files changed, 255 insertions(+), 252 deletions(-)

-- 
1.8.3.2

[PATCH v2 1/3] x86/mem_access: Make the mem_access ops generic

[Aravindh Puthiyaparambil]

This patch does the following:
1. Deprecate the HVMOP_[sg]et_mem_access HVM ops.
2. Move the ops under XENMEM_access_opi.
3. Rename enums and structs to be more generic rather than HVM specific.
4. Remove the enums and structs associated with the HVM ops.

Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>

---
Changes from version 1 of the patch:
1. Use MEMOP_CMD_MASK instead of introducing a new mask.
2. Pass "cmd" down from do_memory_op() instead of "op" and "start_extent".
3. Pass typed handle to mem_access_memop() and use __copy_field_to_guest().
4. Use ACCESS() macro to remove ordering dependency.
5. Add compat verification for xen_mem_access_op_t.
6. Fix formatting.

Changes from the RFC version of the patch:
1. Removed pointless braces.
2. Change preemption handling to use upper "cmd" bits from
do_memory_op().
3. Delete old interface enum and structs.
4. Remove xenmem_ prefix.
5. Make access uint8_t and place above domid.

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 38c491e..eeaa72e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4589,79 +4589,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
     }
 
     case HVMOP_set_mem_access:
-    {
-        struct xen_hvm_set_mem_access a;
-        struct domain *d;
-
-        if ( copy_from_guest(&a, arg, 1) )
-            return -EFAULT;
-
-        rc = rcu_lock_remote_domain_by_id(a.domid, &d);
-        if ( rc != 0 )
-            return rc;
-
-        rc = -EINVAL;
-        if ( !is_hvm_domain(d) )
-            goto param_fail5;
-
-        rc = xsm_hvm_param(XSM_TARGET, d, op);
-        if ( rc )
-            goto param_fail5;
-
-        rc = -EINVAL;
-        if ( (a.first_pfn != ~0ull) &&
-             (a.nr < start_iter ||
-              ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
-              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
-            goto param_fail5;
-            
-        rc = p2m_set_mem_access(d, a.first_pfn, a.nr, start_iter,
-                                HVMOP_op_mask, a.hvmmem_access);
-        if ( rc > 0 )
-        {
-            start_iter = rc;
-            rc = -EAGAIN;
-        }
-
-    param_fail5:
-        rcu_unlock_domain(d);
-        break;
-    }
-
     case HVMOP_get_mem_access:
     {
-        struct xen_hvm_get_mem_access a;
-        struct domain *d;
-        hvmmem_access_t access;
-
-        if ( copy_from_guest(&a, arg, 1) )
-            return -EFAULT;
-
-        rc = rcu_lock_remote_domain_by_id(a.domid, &d);
-        if ( rc != 0 )
-            return rc;
-
-        rc = -EINVAL;
-        if ( !is_hvm_domain(d) )
-            goto param_fail6;
-
-        rc = xsm_hvm_param(XSM_TARGET, d, op);
-        if ( rc )
-            goto param_fail6;
-
-        rc = -EINVAL;
-        if ( (a.pfn > domain_get_maximum_gpfn(d)) && a.pfn != ~0ull )
-            goto param_fail6;
-
-        rc = p2m_get_mem_access(d, a.pfn, &access);
-        if ( rc != 0 )
-            goto param_fail6;
-
-        a.hvmmem_access = access;
-        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
-
-    param_fail6:
-        rcu_unlock_domain(d);
+        gdprintk(XENLOG_DEBUG, "Deprecated HVM op %ld.\n", op);
+        rc = -ENOSYS;
         break;
     }
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index fdc5ed3..719b255 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -104,6 +104,7 @@
 #include <xen/xmalloc.h>
 #include <xen/efi.h>
 #include <xen/grant_table.h>
+#include <xen/hypercall.h>
 #include <asm/paging.h>
 #include <asm/shadow.h>
 #include <asm/page.h>
@@ -4631,9 +4632,10 @@ int xenmem_add_to_physmap_one(
     return rc;
 }
 
-long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
+long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     int rc;
+    int op = cmd & MEMOP_CMD_MASK;
 
     switch ( op )
     {
@@ -4853,7 +4855,7 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
     }
 
     default:
-        return subarch_memory_op(op, arg);
+        return subarch_memory_op(cmd, arg);
     }
 
     return 0;
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 50aaf27..e02e08b 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -21,31 +21,94 @@
  */
 
 
+#include <xen/sched.h>
+#include <xen/guest_access.h>
+#include <xen/hypercall.h>
 #include <asm/p2m.h>
 #include <asm/mem_event.h>
+#include <xsm/xsm.h>
 
 
-int mem_access_memop(struct domain *d, xen_mem_event_op_t *meo)
+int mem_access_memop(unsigned long cmd,
+                     XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg)
 {
-    int rc;
+    long rc;
+    xen_mem_access_op_t mao;
+    struct domain *d;
+
+    if ( copy_from_guest(&mao, arg, 1) )
+        return -EFAULT;
+
+    rc = rcu_lock_live_remote_domain_by_id(mao.domid, &d);
+    if ( rc )
+        return rc;
+
+    if ( !is_hvm_domain(d) )
+        return -EINVAL;
+
+    rc = xsm_mem_event_op(XSM_TARGET, d, XENMEM_access_op);
+    if ( rc )
+        goto out;
 
     if ( unlikely(!d->mem_event->access.ring_page) )
         return -ENODEV;
 
-    switch( meo->op )
+    switch ( mao.op )
     {
     case XENMEM_access_op_resume:
     {
         p2m_mem_access_resume(d);
         rc = 0;
+        break;
+    }
+
+    case XENMEM_access_op_set_access:
+    {
+        unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
+
+        rc = -EINVAL;
+        if ( (mao.pfn != ~0ull) &&
+             (mao.nr < start_iter ||
+              ((mao.pfn + mao.nr - 1) < mao.pfn) ||
+              ((mao.pfn + mao.nr - 1) > domain_get_maximum_gpfn(d))) )
+            break;
+
+        rc = p2m_set_mem_access(d, mao.pfn, mao.nr, start_iter,
+                                MEMOP_CMD_MASK, mao.access);
+        if ( rc > 0 )
+        {
+            ASSERT(!(rc & MEMOP_CMD_MASK));
+            rc = hypercall_create_continuation(__HYPERVISOR_memory_op, "lh",
+                                               cmd | rc, arg);
+        }
+        break;
+    }
+
+    case XENMEM_access_op_get_access:
+    {
+        xenmem_access_t access;
+
+        rc = -EINVAL;
+        if ( (mao.pfn > domain_get_maximum_gpfn(d)) && mao.pfn != ~0ull )
+            break;
+
+        rc = p2m_get_mem_access(d, mao.pfn, &access);
+        if ( rc != 0 )
+            break;
+
+        mao.access = access;
+        rc = __copy_field_to_guest(arg, &mao, access) ? -EFAULT : 0;
+
+        break;
     }
-    break;
 
     default:
         rc = -ENOSYS;
         break;
     }
 
+ out:
+    rcu_unlock_domain(d);
     return rc;
 }
 
diff --git a/xen/arch/x86/mm/mem_event.c b/xen/arch/x86/mm/mem_event.c
index d00e404..36b9dba 100644
--- a/xen/arch/x86/mm/mem_event.c
+++ b/xen/arch/x86/mm/mem_event.c
@@ -458,9 +458,6 @@ int do_mem_event_op(int op, uint32_t domain, void *arg)
         case XENMEM_paging_op:
             ret = mem_paging_memop(d, (xen_mem_event_op_t *) arg);
             break;
-        case XENMEM_access_op:
-            ret = mem_access_memop(d, (xen_mem_event_op_t *) arg);
-            break;
         case XENMEM_sharing_op:
             ret = mem_sharing_memop(d, (xen_mem_sharing_op_t *) arg);
             break;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c38f334..be77d7c 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1350,7 +1350,7 @@ void p2m_mem_access_resume(struct domain *d)
 /* Set access type for a region of pfns.
  * If start_pfn == -1ul, sets the default access type */
 long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
-                        uint32_t start, uint32_t mask, hvmmem_access_t access)
+                        uint32_t start, uint32_t mask, xenmem_access_t access)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_access_t a, _a;
@@ -1359,7 +1359,7 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
     long rc = 0;
 
     static const p2m_access_t memaccess[] = {
-#define ACCESS(ac) [HVMMEM_access_##ac] = p2m_access_##ac
+#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
         ACCESS(n),
         ACCESS(r),
         ACCESS(w),
@@ -1378,7 +1378,7 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
     case 0 ... ARRAY_SIZE(memaccess) - 1:
         a = memaccess[access];
         break;
-    case HVMMEM_access_default:
+    case XENMEM_access_default:
         a = p2m->default_access;
         break;
     default:
@@ -1416,23 +1416,26 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
 /* Get access type for a pfn
  * If pfn == -1ul, gets the default access type */
 int p2m_get_mem_access(struct domain *d, unsigned long pfn, 
-                       hvmmem_access_t *access)
+                       xenmem_access_t *access)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_type_t t;
     p2m_access_t a;
     mfn_t mfn;
 
-    static const hvmmem_access_t memaccess[] = {
-        HVMMEM_access_n,
-        HVMMEM_access_r,
-        HVMMEM_access_w,
-        HVMMEM_access_rw,
-        HVMMEM_access_x,
-        HVMMEM_access_rx,
-        HVMMEM_access_wx,
-        HVMMEM_access_rwx,
-        HVMMEM_access_rx2rw
+    static const xenmem_access_t memaccess[] = {
+#define ACCESS(ac) [XENMEM_access_##ac] = XENMEM_access_##ac
+            ACCESS(n),
+            ACCESS(r),
+            ACCESS(w),
+            ACCESS(rw),
+            ACCESS(x),
+            ACCESS(rx),
+            ACCESS(wx),
+            ACCESS(rwx),
+            ACCESS(rx2rw),
+            ACCESS(n2rwx),
+#undef ACCESS
     };
 
     /* If request to get default access */
diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 0a8408b..b768158 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -4,6 +4,7 @@
 #include <compat/xen.h>
 #include <asm/mem_event.h>
 #include <asm/mem_sharing.h>
+#include <asm/mem_access.h>
 
 int compat_set_gdt(XEN_GUEST_HANDLE_PARAM(uint) frame_list, unsigned int entries)
 {
@@ -44,7 +45,7 @@ int compat_update_descriptor(u32 pa_lo, u32 pa_hi, u32 desc_lo, u32 desc_hi)
                                 desc_lo | ((u64)desc_hi << 32));
 }
 
-int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
+int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     struct compat_machphys_mfn_list xmml;
     l2_pgentry_t l2e;
@@ -52,6 +53,7 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
     compat_pfn_t mfn;
     unsigned int i;
     int rc = 0;
+    int op = cmd & MEMOP_CMD_MASK;
 
     switch ( op )
     {
@@ -68,7 +70,7 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
         XLAT_foreign_memory_map(nat, &cmp);
 #undef XLAT_memory_map_HNDL_buffer
 
-        rc = arch_memory_op(op, guest_handle_from_ptr(nat, void));
+        rc = arch_memory_op(cmd, guest_handle_from_ptr(nat, void));
 
         break;
     }
@@ -87,7 +89,7 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
         XLAT_memory_map(nat, &cmp);
 #undef XLAT_memory_map_HNDL_buffer
 
-        rc = arch_memory_op(op, guest_handle_from_ptr(nat, void));
+        rc = arch_memory_op(cmd, guest_handle_from_ptr(nat, void));
         if ( rc < 0 )
             break;
 
@@ -111,7 +113,7 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         XLAT_pod_target(nat, &cmp);
 
-        rc = arch_memory_op(op, guest_handle_from_ptr(nat, void));
+        rc = arch_memory_op(cmd, guest_handle_from_ptr(nat, void));
         if ( rc < 0 )
             break;
 
@@ -185,7 +187,6 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
         return mem_sharing_get_nr_shared_mfns();
 
     case XENMEM_paging_op:
-    case XENMEM_access_op:
     {
         xen_mem_event_op_t meo;
         if ( copy_from_guest(&meo, arg, 1) )
@@ -195,6 +196,11 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
         break;
     }
+
+    case XENMEM_access_op:
+        rc = mem_access_memop(cmd, guest_handle_cast(arg, xen_mem_access_op_t));
+        break;
+
     case XENMEM_sharing_op:
     {
         xen_mem_sharing_op_t mso;
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index f6ea012..264e39f 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -25,6 +25,7 @@
 #include <xen/numa.h>
 #include <xen/nodemask.h>
 #include <xen/guest_access.h>
+#include <xen/hypercall.h>
 #include <asm/current.h>
 #include <asm/asm_defns.h>
 #include <asm/page.h>
@@ -36,6 +37,7 @@
 #include <asm/numa.h>
 #include <asm/mem_event.h>
 #include <asm/mem_sharing.h>
+#include <asm/mem_access.h>
 #include <public/memory.h>
 
 /* Parameters for PFN/MADDR compression. */
@@ -948,7 +950,7 @@ void __init subarch_init_memory(void)
     }
 }
 
-long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
+long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     struct xen_machphys_mfn_list xmml;
     l3_pgentry_t l3e;
@@ -957,6 +959,7 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
     xen_pfn_t mfn, last_mfn;
     unsigned int i;
     long rc = 0;
+    int op = cmd & MEMOP_CMD_MASK;
 
     switch ( op )
     {
@@ -1007,7 +1010,6 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
         return mem_sharing_get_nr_shared_mfns();
 
     case XENMEM_paging_op:
-    case XENMEM_access_op:
     {
         xen_mem_event_op_t meo;
         if ( copy_from_guest(&meo, arg, 1) )
@@ -1017,6 +1019,11 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
         break;
     }
+
+    case XENMEM_access_op:
+        rc = mem_access_memop(cmd, guest_handle_cast(arg, xen_mem_access_op_t));
+        break;
+
     case XENMEM_sharing_op:
     {
         xen_mem_sharing_op_t mso;
diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index daa2e04..25dc016 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -13,6 +13,8 @@ CHECK_TYPE(domid);
 #undef compat_domid_t
 #undef xen_domid_t
 
+CHECK_mem_access_op;
+
 int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
 {
     int split, op = cmd & MEMOP_CMD_MASK;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 4d6ffee..257f4b0 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -964,7 +964,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
 
     default:
-        rc = arch_memory_op(op, arg);
+        rc = arch_memory_op(cmd, arg);
         break;
     }
 
diff --git a/xen/include/asm-x86/mem_access.h b/xen/include/asm-x86/mem_access.h
index 60c2834..5c7c5fd 100644
--- a/xen/include/asm-x86/mem_access.h
+++ b/xen/include/asm-x86/mem_access.h
@@ -23,7 +23,8 @@
 #ifndef _XEN_ASM_MEM_ACCESS_H
 #define _XEN_ASM_MEM_ACCESS_H
 
-int mem_access_memop(struct domain *d, xen_mem_event_op_t *meo);
+int mem_access_memop(unsigned long cmd,
+                     XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
 int mem_access_send_req(struct domain *d, mem_event_request_t *req);
 
 #endif /* _XEN_ASM_MEM_ACCESS_H */
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index c835f76..7059adc 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -561,9 +561,9 @@ void *do_page_walk(struct vcpu *v, unsigned long addr);
 int __sync_local_execstate(void);
 
 /* Arch-specific portion of memory_op hypercall. */
-long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
-long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
-int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void));
+long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
+long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
+int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void));
 int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void));
 
 int steal_page(
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index d644f82..743bb59 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -577,12 +577,12 @@ void p2m_mem_access_resume(struct domain *d);
 /* Set access type for a region of pfns.
  * If start_pfn == -1ul, sets the default access type */
 long p2m_set_mem_access(struct domain *d, unsigned long start_pfn, uint32_t nr,
-                        uint32_t start, uint32_t mask, hvmmem_access_t access);
+                        uint32_t start, uint32_t mask, xenmem_access_t access);
 
 /* Get access type for a pfn
  * If pfn == -1ul, gets the default access type */
-int p2m_get_mem_access(struct domain *d, unsigned long pfn, 
-                       hvmmem_access_t *access);
+int p2m_get_mem_access(struct domain *d, unsigned long pfn,
+                       xenmem_access_t *access);
 
 /* 
  * Internal functions, only called by other p2m code
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 3204ec4..f00f6d2 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -162,49 +162,11 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_xentrace_t);
 /* Following tools-only interfaces may change in future. */
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 
+/* Deprecated by XENMEM_access_op_set_access */
 #define HVMOP_set_mem_access        12
-typedef enum {
-    HVMMEM_access_n,
-    HVMMEM_access_r,
-    HVMMEM_access_w,
-    HVMMEM_access_rw,
-    HVMMEM_access_x,
-    HVMMEM_access_rx,
-    HVMMEM_access_wx,
-    HVMMEM_access_rwx,
-    HVMMEM_access_rx2rw,       /* Page starts off as r-x, but automatically
-                                * change to r-w on a write */
-    HVMMEM_access_n2rwx,       /* Log access: starts off as n, automatically 
-                                * goes to rwx, generating an event without
-                                * pausing the vcpu */
-    HVMMEM_access_default      /* Take the domain default */
-} hvmmem_access_t;
-/* Notify that a region of memory is to have specific access types */
-struct xen_hvm_set_mem_access {
-    /* Domain to be updated. */
-    domid_t domid;
-    /* Memory type */
-    uint16_t hvmmem_access; /* hvm_access_t */
-    /* Number of pages, ignored on setting default access */
-    uint32_t nr;
-    /* First pfn, or ~0ull to set the default access for new pages */
-    uint64_aligned_t first_pfn;
-};
-typedef struct xen_hvm_set_mem_access xen_hvm_set_mem_access_t;
-DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_mem_access_t);
 
+/* Deprecated by XENMEM_access_op_get_access */
 #define HVMOP_get_mem_access        13
-/* Get the specific access type for that region of memory */
-struct xen_hvm_get_mem_access {
-    /* Domain to be queried. */
-    domid_t domid;
-    /* Memory type: OUT */
-    uint16_t hvmmem_access; /* hvm_access_t */
-    /* pfn, or ~0ull for default access for new pages.  IN */
-    uint64_aligned_t pfn;
-};
-typedef struct xen_hvm_get_mem_access xen_hvm_get_mem_access_t;
-DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_mem_access_t);
 
 #define HVMOP_inject_trap            14
 /* Inject a trap into a VCPU, which will get taken up on the next
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index f19ac14..5bcd475 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -363,9 +363,6 @@ typedef struct xen_pod_target xen_pod_target_t;
 #define XENMEM_paging_op_evict              1
 #define XENMEM_paging_op_prep               2
 
-#define XENMEM_access_op                    21
-#define XENMEM_access_op_resume             0
-
 struct xen_mem_event_op {
     uint8_t     op;         /* XENMEM_*_op_* */
     domid_t     domain;
@@ -379,6 +376,56 @@ struct xen_mem_event_op {
 typedef struct xen_mem_event_op xen_mem_event_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_mem_event_op_t);
 
+#define XENMEM_access_op                    21
+#define XENMEM_access_op_resume             0
+#define XENMEM_access_op_set_access         1
+#define XENMEM_access_op_get_access         2
+
+typedef enum {
+    XENMEM_access_n,
+    XENMEM_access_r,
+    XENMEM_access_w,
+    XENMEM_access_rw,
+    XENMEM_access_x,
+    XENMEM_access_rx,
+    XENMEM_access_wx,
+    XENMEM_access_rwx,
+    /*
+     * Page starts off as r-x, but automatically
+     * change to r-w on a write
+     */
+    XENMEM_access_rx2rw,
+    /*
+     * Log access: starts off as n, automatically
+     * goes to rwx, generating an event without
+     * pausing the vcpu
+     */
+    XENMEM_access_n2rwx,
+    /* Take the domain default */
+    XENMEM_access_default
+} xenmem_access_t;
+
+struct xen_mem_access_op {
+    /* XENMEM_access_op_* */
+    uint8_t op;
+    /* xenmem_access_t */
+    uint8_t access;
+    domid_t domid;
+    /*
+     * Number of pages for set op
+     * Ignored on setting default access and other ops
+     */
+    uint32_t nr;
+    /*
+     * First pfn for set op
+     * pfn for get op
+     * ~0ull is used to set and get the default access for pages
+     */
+    uint64_aligned_t pfn;
+};
+typedef struct xen_mem_access_op xen_mem_access_op_t;
+DEFINE_XEN_GUEST_HANDLE(xen_mem_access_op_t);
+
 #define XENMEM_sharing_op                   22
 #define XENMEM_sharing_op_nominate_gfn      0
 #define XENMEM_sharing_op_nominate_gref     1
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 5d354d8..9a35dd7 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -60,6 +60,7 @@
 !	memory_exchange			memory.h
 !	memory_map			memory.h
 !	memory_reservation		memory.h
+?	mem_access_op		memory.h
 !	pod_target			memory.h
 !	remove_from_physmap		memory.h
 ?	physdev_eoi			physdev.h
-- 
1.8.3.2

[PATCH v2 2/3] tools/libxc: Make the mem_access APIs generic

[Aravindh Puthiyaparambil]

This patch does the following:
1. Add new xc_[sg]et_mem_access APIs.
2. Remove xc_hvm_[sg]et_mem_access() APIs.

Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>

---
Changes from version 1 of the patch:
1. Remove unused "gfn" from the parameter list of xc_mem_access_resume().
2. Use structure initialisation.
3. Write "access" back only in the case of do_memory_op returning success.
4. Fix formatting.

Changes from the RFC version of the patch:
1. Remove xc_mem_access_memop() wrapper.
2. Remove xc_hvm_[sg]et_mem_access() APIs.

diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
index a50c145..f436e69 100644
--- a/tools/libxc/xc_mem_access.c
+++ b/tools/libxc/xc_mem_access.c
@@ -22,7 +22,7 @@
  */
 
 #include "xc_private.h"
-
+#include <xen/memory.h>
 
 int xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
                          uint32_t *port)
@@ -47,12 +47,54 @@ int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
                                 NULL);
 }
 
-int xc_mem_access_resume(xc_interface *xch, domid_t domain_id, unsigned long gfn)
+int xc_mem_access_resume(xc_interface *xch, domid_t domain_id)
+{
+    xen_mem_access_op_t mao =
+    {
+        .op    = XENMEM_access_op_resume,
+        .domid = domain_id
+    };
+
+    return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
+}
+
+int xc_set_mem_access(xc_interface *xch,
+                      domid_t domain_id,
+                      xenmem_access_t access,
+                      uint64_t first_pfn,
+                      uint32_t nr)
 {
-    return xc_mem_event_memop(xch, domain_id,
-                                XENMEM_access_op_resume,
-                                XENMEM_access_op,
-                                gfn, NULL);
+    xen_mem_access_op_t mao =
+    {
+        .op     = XENMEM_access_op_set_access,
+        .domid  = domain_id,
+        .access = access,
+        .pfn    = first_pfn,
+        .nr     = nr
+    };
+
+    return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
+}
+
+int xc_get_mem_access(xc_interface *xch,
+                      domid_t domain_id,
+                      uint64_t pfn,
+                      xenmem_access_t *access)
+{
+    int rc;
+    xen_mem_access_op_t mao =
+    {
+        .op    = XENMEM_access_op_get_access,
+        .domid = domain_id,
+        .pfn   = pfn
+    };
+
+    rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
+
+    if ( rc == 0 )
+        *access = mao.access;
+
+    return rc;
 }
 
 /*
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 3303454..4143de6 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -593,67 +593,6 @@ int xc_hvm_set_mem_type(
     return rc;
 }
 
-int xc_hvm_set_mem_access(
-    xc_interface *xch, domid_t dom, hvmmem_access_t mem_access, uint64_t first_pfn, uint64_t nr)
-{
-    DECLARE_HYPERCALL;
-    DECLARE_HYPERCALL_BUFFER(struct xen_hvm_set_mem_access, arg);
-    int rc;
-
-    arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
-    if ( arg == NULL )
-    {
-        PERROR("Could not allocate memory for xc_hvm_set_mem_access hypercall");
-        return -1;
-    }
-
-    arg->domid         = dom;
-    arg->hvmmem_access = mem_access;
-    arg->first_pfn     = first_pfn;
-    arg->nr            = nr;
-
-    hypercall.op     = __HYPERVISOR_hvm_op;
-    hypercall.arg[0] = HVMOP_set_mem_access;
-    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg);
-
-    rc = do_xen_hypercall(xch, &hypercall);
-
-    xc_hypercall_buffer_free(xch, arg);
-
-    return rc;
-}
-
-int xc_hvm_get_mem_access(
-    xc_interface *xch, domid_t dom, uint64_t pfn, hvmmem_access_t* mem_access)
-{
-    DECLARE_HYPERCALL;
-    DECLARE_HYPERCALL_BUFFER(struct xen_hvm_get_mem_access, arg);
-    int rc;
-
-    arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
-    if ( arg == NULL )
-    {
-        PERROR("Could not allocate memory for xc_hvm_get_mem_access hypercall");
-        return -1;
-    }
-
-    arg->domid       = dom;
-    arg->pfn         = pfn;
-
-    hypercall.op     = __HYPERVISOR_hvm_op;
-    hypercall.arg[0] = HVMOP_get_mem_access;
-    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg);
-
-    rc = do_xen_hypercall(xch, &hypercall);
-
-    if ( !rc )
-        *mem_access = arg->hvmmem_access;
-
-    xc_hypercall_buffer_free(xch, arg);
-
-    return rc;
-}
-
 int xc_hvm_inject_trap(
     xc_interface *xch, domid_t dom, int vcpu, uint32_t vector,
     uint32_t type, uint32_t error_code, uint32_t insn_len,
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index e3a32f2..02129f7 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1729,20 +1729,6 @@ int xc_hvm_set_mem_type(
     xc_interface *xch, domid_t dom, hvmmem_type_t memtype, uint64_t first_pfn, uint64_t nr);
 
 /*
- * Set a range of memory to a specific access.
- * Allowed types are HVMMEM_access_default, HVMMEM_access_n, any combination of 
- * HVM_access_ + (rwx), and HVM_access_rx2rw
- */
-int xc_hvm_set_mem_access(
-    xc_interface *xch, domid_t dom, hvmmem_access_t memaccess, uint64_t first_pfn, uint64_t nr);
-
-/*
- * Gets the mem access for the given page (returned in memacess on success)
- */
-int xc_hvm_get_mem_access(
-    xc_interface *xch, domid_t dom, uint64_t pfn, hvmmem_access_t* memaccess);
-
-/*
  * Injects a hardware/software CPU trap, to take effect the next time the HVM 
  * resumes. 
  */
@@ -2059,8 +2045,22 @@ int xc_mem_paging_load(xc_interface *xch, domid_t domain_id,
  */
 int xc_mem_access_enable(xc_interface *xch, domid_t domain_id, uint32_t *port);
 int xc_mem_access_disable(xc_interface *xch, domid_t domain_id);
-int xc_mem_access_resume(xc_interface *xch, domid_t domain_id,
-                         unsigned long gfn);
+int xc_mem_access_resume(xc_interface *xch, domid_t domain_id);
+
+/*
+ * Set a range of memory to a specific access.
+ * Allowed types are XENMEM_access_default, XENMEM_access_n, any combination of
+ * XENMEM_access_ + (rwx), and XENMEM_access_rx2rw
+ */
+int xc_set_mem_access(xc_interface *xch, domid_t domain_id,
+                      xenmem_access_t access, uint64_t first_pfn,
+                      uint32_t nr);
+
+/*
+ * 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);
 
 /***
  * Memory sharing operations.
-- 
1.8.3.2

[PATCH v2 3/3] tools/xen-access: Use the new mem_access APIs

[Aravindh Puthiyaparambil]

Modify the xen-access test program to use the new mem_access APIs.

Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>

---
Changes from version 1 of the patch:
Remove extra "gfn" parameter from xc_mem_access_resume() call.

diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index f6522a1..0a84bd5 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -410,8 +410,7 @@ static int xenaccess_resume_page(xenaccess_t *paging, mem_event_response_t *rsp)
         goto out;
 
     /* Tell Xen page is ready */
-    ret = xc_mem_access_resume(paging->xc_handle, paging->mem_event.domain_id,
-                               rsp->gfn);
+    ret = xc_mem_access_resume(paging->xc_handle, paging->mem_event.domain_id);
     ret = xc_evtchn_notify(paging->mem_event.xce_handle,
                            paging->mem_event.port);
 
@@ -440,8 +439,8 @@ int main(int argc, char *argv[])
     int rc = -1;
     int rc1;
     xc_interface *xch;
-    hvmmem_access_t default_access = HVMMEM_access_rwx;
-    hvmmem_access_t after_first_access = HVMMEM_access_rwx;
+    xenmem_access_t default_access = XENMEM_access_rwx;
+    xenmem_access_t after_first_access = XENMEM_access_rwx;
     int required = 0;
     int int3 = 0;
     int shutting_down = 0;
@@ -475,13 +474,13 @@ int main(int argc, char *argv[])
 
     if ( !strcmp(argv[0], "write") )
     {
-        default_access = HVMMEM_access_rx;
-        after_first_access = HVMMEM_access_rwx;
+        default_access = XENMEM_access_rx;
+        after_first_access = XENMEM_access_rwx;
     }
     else if ( !strcmp(argv[0], "exec") )
     {
-        default_access = HVMMEM_access_rw;
-        after_first_access = HVMMEM_access_rwx;
+        default_access = XENMEM_access_rw;
+        after_first_access = XENMEM_access_rwx;
     }
     else if ( !strcmp(argv[0], "int3") )
     {
@@ -520,15 +519,15 @@ int main(int argc, char *argv[])
     }
 
     /* Set the default access type and convert all pages to it */
-    rc = xc_hvm_set_mem_access(xch, domain_id, default_access, ~0ull, 0);
+    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;
     }
 
-    rc = xc_hvm_set_mem_access(xch, domain_id, default_access, 0,
-                               xenaccess->domain_info->max_pages);
+    rc = xc_set_mem_access(xch, domain_id, default_access, 0,
+                           xenaccess->domain_info->max_pages);
     if ( rc < 0 )
     {
         ERROR("Error %d setting all memory to access type %d\n", rc,
@@ -554,8 +553,9 @@ int main(int argc, char *argv[])
             DPRINTF("xenaccess shutting down on signal %d\n", interrupted);
 
             /* Unregister for every event */
-            rc = xc_hvm_set_mem_access(xch, domain_id, HVMMEM_access_rwx, ~0ull, 0);
-            rc = xc_hvm_set_mem_access(xch, domain_id, HVMMEM_access_rwx, 0, xenaccess->domain_info->max_pages);
+            rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, ~0ull, 0);
+            rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, 0,
+                                   xenaccess->domain_info->max_pages);
             rc = xc_set_hvm_param(xch, domain_id, HVM_PARAM_MEMORY_EVENT_INT3, HVMPME_mode_disabled);
 
             shutting_down = 1;
@@ -575,7 +575,7 @@ int main(int argc, char *argv[])
 
         while ( RING_HAS_UNCONSUMED_REQUESTS(&xenaccess->mem_event.back_ring) )
         {
-            hvmmem_access_t access;
+            xenmem_access_t access;
 
             rc = get_request(&xenaccess->mem_event, &req);
             if ( rc != 0 )
@@ -591,7 +591,7 @@ int main(int argc, char *argv[])
 
             switch (req.reason) {
             case MEM_EVENT_REASON_VIOLATION:
-                rc = xc_hvm_get_mem_access(xch, domain_id, req.gfn, &access);
+                rc = xc_get_mem_access(xch, domain_id, req.gfn, &access);
                 if (rc < 0)
                 {
                     ERROR("Error %d getting mem_access event\n", rc);
@@ -611,8 +611,8 @@ int main(int argc, char *argv[])
 
                 if ( default_access != after_first_access )
                 {
-                    rc = xc_hvm_set_mem_access(xch, domain_id,
-                                               after_first_access, req.gfn, 1);
+                    rc = xc_set_mem_access(xch, domain_id, after_first_access,
+                                           req.gfn, 1);
                     if (rc < 0)
                     {
                         ERROR("Error %d setting gfn to access_type %d\n", rc,
-- 
1.8.3.2

Re: [PATCH v2 1/3] x86/mem_access: Make the mem_access ops generic

[Jan Beulich]

>>> On 15.04.14 at 00:02, <aravindp@cisco.com> wrote:
> -int mem_access_memop(struct domain *d, xen_mem_event_op_t *meo)
> +int mem_access_memop(unsigned long cmd,
> +                     XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg)
>  {
> -    int rc;
> +    long rc;
> +    xen_mem_access_op_t mao;
> +    struct domain *d;
> +
> +    if ( copy_from_guest(&mao, arg, 1) )
> +        return -EFAULT;
> +
> +    rc = rcu_lock_live_remote_domain_by_id(mao.domid, &d);
> +    if ( rc )
> +        return rc;
> +
> +    if ( !is_hvm_domain(d) )
> +        return -EINVAL;
> +
> +    rc = xsm_mem_event_op(XSM_TARGET, d, XENMEM_access_op);
> +    if ( rc )
> +        goto out;
>  
>      if ( unlikely(!d->mem_event->access.ring_page) )
>          return -ENODEV;

This and the earlier return need to become "goto out".

>  
> -    switch( meo->op )
> +    switch ( mao.op )
>      {
>      case XENMEM_access_op_resume:
>      {
>          p2m_mem_access_resume(d);
>          rc = 0;
> +        break;
> +    }

Please remove the pointless opening brace rather than adding a closing
one here.

> +
> +    case XENMEM_access_op_set_access:
> +    {
> +        unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
> +
> +        rc = -EINVAL;
> +        if ( (mao.pfn != ~0ull) &&
> +             (mao.nr < start_iter ||
> +              ((mao.pfn + mao.nr - 1) < mao.pfn) ||
> +              ((mao.pfn + mao.nr - 1) > domain_get_maximum_gpfn(d))) )
> +            break;
> +
> +        rc = p2m_set_mem_access(d, mao.pfn, mao.nr, start_iter,
> +                                MEMOP_CMD_MASK, mao.access);
> +        if ( rc > 0 )
> +        {
> +            ASSERT(!(rc & MEMOP_CMD_MASK));
> +            rc = hypercall_create_continuation(__HYPERVISOR_memory_op, "lh",
> +                                               cmd | rc, arg);

Either you need to mask cmd properly, or directly use
XENMEM_access_op_set_access.

Jan

Re: [PATCH v2 3/3] tools/xen-access: Use the new mem_access APIs

[Andrew Cooper]

On 14/04/14 23:02, Aravindh Puthiyaparambil wrote:
> Modify the xen-access test program to use the new mem_access APIs.
>
> Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>

This patch must be folded into the previous patch which makes the
changes in libxc, or the build will break.

~Andrew

> ---
> Changes from version 1 of the patch:
> Remove extra "gfn" parameter from xc_mem_access_resume() call.
>
> diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
> index f6522a1..0a84bd5 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -410,8 +410,7 @@ static int xenaccess_resume_page(xenaccess_t *paging, mem_event_response_t *rsp)
>          goto out;
>  
>      /* Tell Xen page is ready */
> -    ret = xc_mem_access_resume(paging->xc_handle, paging->mem_event.domain_id,
> -                               rsp->gfn);
> +    ret = xc_mem_access_resume(paging->xc_handle, paging->mem_event.domain_id);
>      ret = xc_evtchn_notify(paging->mem_event.xce_handle,
>                             paging->mem_event.port);
>  
> @@ -440,8 +439,8 @@ int main(int argc, char *argv[])
>      int rc = -1;
>      int rc1;
>      xc_interface *xch;
> -    hvmmem_access_t default_access = HVMMEM_access_rwx;
> -    hvmmem_access_t after_first_access = HVMMEM_access_rwx;
> +    xenmem_access_t default_access = XENMEM_access_rwx;
> +    xenmem_access_t after_first_access = XENMEM_access_rwx;
>      int required = 0;
>      int int3 = 0;
>      int shutting_down = 0;
> @@ -475,13 +474,13 @@ int main(int argc, char *argv[])
>  
>      if ( !strcmp(argv[0], "write") )
>      {
> -        default_access = HVMMEM_access_rx;
> -        after_first_access = HVMMEM_access_rwx;
> +        default_access = XENMEM_access_rx;
> +        after_first_access = XENMEM_access_rwx;
>      }
>      else if ( !strcmp(argv[0], "exec") )
>      {
> -        default_access = HVMMEM_access_rw;
> -        after_first_access = HVMMEM_access_rwx;
> +        default_access = XENMEM_access_rw;
> +        after_first_access = XENMEM_access_rwx;
>      }
>      else if ( !strcmp(argv[0], "int3") )
>      {
> @@ -520,15 +519,15 @@ int main(int argc, char *argv[])
>      }
>  
>      /* Set the default access type and convert all pages to it */
> -    rc = xc_hvm_set_mem_access(xch, domain_id, default_access, ~0ull, 0);
> +    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;
>      }
>  
> -    rc = xc_hvm_set_mem_access(xch, domain_id, default_access, 0,
> -                               xenaccess->domain_info->max_pages);
> +    rc = xc_set_mem_access(xch, domain_id, default_access, 0,
> +                           xenaccess->domain_info->max_pages);
>      if ( rc < 0 )
>      {
>          ERROR("Error %d setting all memory to access type %d\n", rc,
> @@ -554,8 +553,9 @@ int main(int argc, char *argv[])
>              DPRINTF("xenaccess shutting down on signal %d\n", interrupted);
>  
>              /* Unregister for every event */
> -            rc = xc_hvm_set_mem_access(xch, domain_id, HVMMEM_access_rwx, ~0ull, 0);
> -            rc = xc_hvm_set_mem_access(xch, domain_id, HVMMEM_access_rwx, 0, xenaccess->domain_info->max_pages);
> +            rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, ~0ull, 0);
> +            rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, 0,
> +                                   xenaccess->domain_info->max_pages);
>              rc = xc_set_hvm_param(xch, domain_id, HVM_PARAM_MEMORY_EVENT_INT3, HVMPME_mode_disabled);
>  
>              shutting_down = 1;
> @@ -575,7 +575,7 @@ int main(int argc, char *argv[])
>  
>          while ( RING_HAS_UNCONSUMED_REQUESTS(&xenaccess->mem_event.back_ring) )
>          {
> -            hvmmem_access_t access;
> +            xenmem_access_t access;
>  
>              rc = get_request(&xenaccess->mem_event, &req);
>              if ( rc != 0 )
> @@ -591,7 +591,7 @@ int main(int argc, char *argv[])
>  
>              switch (req.reason) {
>              case MEM_EVENT_REASON_VIOLATION:
> -                rc = xc_hvm_get_mem_access(xch, domain_id, req.gfn, &access);
> +                rc = xc_get_mem_access(xch, domain_id, req.gfn, &access);
>                  if (rc < 0)
>                  {
>                      ERROR("Error %d getting mem_access event\n", rc);
> @@ -611,8 +611,8 @@ int main(int argc, char *argv[])
>  
>                  if ( default_access != after_first_access )
>                  {
> -                    rc = xc_hvm_set_mem_access(xch, domain_id,
> -                                               after_first_access, req.gfn, 1);
> +                    rc = xc_set_mem_access(xch, domain_id, after_first_access,
> +                                           req.gfn, 1);
>                      if (rc < 0)
>                      {
>                          ERROR("Error %d setting gfn to access_type %d\n", rc,

Re: [PATCH v2 1/3] x86/mem_access: Make the mem_access ops generic

[Andrew Cooper]

On 14/04/14 23:02, Aravindh Puthiyaparambil wrote:
> This patch does the following:
> 1. Deprecate the HVMOP_[sg]et_mem_access HVM ops.
> 2. Move the ops under XENMEM_access_opi.
> 3. Rename enums and structs to be more generic rather than HVM specific.
> 4. Remove the enums and structs associated with the HVM ops.
>
> Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Tim Deegan <tim@xen.org>
>
> ---
> Changes from version 1 of the patch:
> 1. Use MEMOP_CMD_MASK instead of introducing a new mask.
> 2. Pass "cmd" down from do_memory_op() instead of "op" and "start_extent".
> 3. Pass typed handle to mem_access_memop() and use __copy_field_to_guest().
> 4. Use ACCESS() macro to remove ordering dependency.
> 5. Add compat verification for xen_mem_access_op_t.
> 6. Fix formatting.
>
> Changes from the RFC version of the patch:
> 1. Removed pointless braces.
> 2. Change preemption handling to use upper "cmd" bits from
> do_memory_op().
> 3. Delete old interface enum and structs.
> 4. Remove xenmem_ prefix.
> 5. Make access uint8_t and place above domid.
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 38c491e..eeaa72e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4589,79 +4589,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>      }
>  
>      case HVMOP_set_mem_access:
> -    {
> -        struct xen_hvm_set_mem_access a;
> -        struct domain *d;
> -
> -        if ( copy_from_guest(&a, arg, 1) )
> -            return -EFAULT;
> -
> -        rc = rcu_lock_remote_domain_by_id(a.domid, &d);
> -        if ( rc != 0 )
> -            return rc;
> -
> -        rc = -EINVAL;
> -        if ( !is_hvm_domain(d) )
> -            goto param_fail5;
> -
> -        rc = xsm_hvm_param(XSM_TARGET, d, op);
> -        if ( rc )
> -            goto param_fail5;
> -
> -        rc = -EINVAL;
> -        if ( (a.first_pfn != ~0ull) &&
> -             (a.nr < start_iter ||
> -              ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> -              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
> -            goto param_fail5;
> -            
> -        rc = p2m_set_mem_access(d, a.first_pfn, a.nr, start_iter,
> -                                HVMOP_op_mask, a.hvmmem_access);
> -        if ( rc > 0 )
> -        {
> -            start_iter = rc;
> -            rc = -EAGAIN;
> -        }
> -
> -    param_fail5:
> -        rcu_unlock_domain(d);
> -        break;
> -    }
> -
>      case HVMOP_get_mem_access:
>      {
> -        struct xen_hvm_get_mem_access a;
> -        struct domain *d;
> -        hvmmem_access_t access;
> -
> -        if ( copy_from_guest(&a, arg, 1) )
> -            return -EFAULT;
> -
> -        rc = rcu_lock_remote_domain_by_id(a.domid, &d);
> -        if ( rc != 0 )
> -            return rc;
> -
> -        rc = -EINVAL;
> -        if ( !is_hvm_domain(d) )
> -            goto param_fail6;
> -
> -        rc = xsm_hvm_param(XSM_TARGET, d, op);
> -        if ( rc )
> -            goto param_fail6;
> -
> -        rc = -EINVAL;
> -        if ( (a.pfn > domain_get_maximum_gpfn(d)) && a.pfn != ~0ull )
> -            goto param_fail6;
> -
> -        rc = p2m_get_mem_access(d, a.pfn, &access);
> -        if ( rc != 0 )
> -            goto param_fail6;
> -
> -        a.hvmmem_access = access;
> -        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> -
> -    param_fail6:
> -        rcu_unlock_domain(d);
> +        gdprintk(XENLOG_DEBUG, "Deprecated HVM op %ld.\n", op);

Is this really sensible?  I suppose it should hopefully be a rare
hypercall, but it is hardly a useful message to print.

>  
>  /* 
>   * Internal functions, only called by other p2m code
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index 3204ec4..f00f6d2 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h

Changes here need to be in sync with libxc as well, also for build reasons.

~Andrew

> @@ -162,49 +162,11 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_xentrace_t);
>  /* Following tools-only interfaces may change in future. */
>  #if defined(__XEN__) || defined(__XEN_TOOLS__)
>  
> +/* Deprecated by XENMEM_access_op_set_access */
>  #define HVMOP_set_mem_access        12
> -typedef enum {
> -    HVMMEM_access_n,
> -    HVMMEM_access_r,
> -    HVMMEM_access_w,
> -    HVMMEM_access_rw,
> -    HVMMEM_access_x,
> -    HVMMEM_access_rx,
> -    HVMMEM_access_wx,
> -    HVMMEM_access_rwx,
> -    HVMMEM_access_rx2rw,       /* Page starts off as r-x, but automatically
> -                                * change to r-w on a write */
> -    HVMMEM_access_n2rwx,       /* Log access: starts off as n, automatically 
> -                                * goes to rwx, generating an event without
> -                                * pausing the vcpu */
> -    HVMMEM_access_default      /* Take the domain default */
> -} hvmmem_access_t;
> -/* Notify that a region of memory is to have specific access types */
> -struct xen_hvm_set_mem_access {
> -    /* Domain to be updated. */
> -    domid_t domid;
> -    /* Memory type */
> -    uint16_t hvmmem_access; /* hvm_access_t */
> -    /* Number of pages, ignored on setting default access */
> -    uint32_t nr;
> -    /* First pfn, or ~0ull to set the default access for new pages */
> -    uint64_aligned_t first_pfn;
> -};
> -typedef struct xen_hvm_set_mem_access xen_hvm_set_mem_access_t;
> -DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_mem_access_t);
>  
> +/* Deprecated by XENMEM_access_op_get_access */
>  #define HVMOP_get_mem_access        13
> -/* Get the specific access type for that region of memory */
> -struct xen_hvm_get_mem_access {
> -    /* Domain to be queried. */
> -    domid_t domid;
> -    /* Memory type: OUT */
> -    uint16_t hvmmem_access; /* hvm_access_t */
> -    /* pfn, or ~0ull for default access for new pages.  IN */
> -    uint64_aligned_t pfn;
> -};
> -typedef struct xen_hvm_get_mem_access xen_hvm_get_mem_access_t;
> -DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_mem_access_t);
>  
>  #define HVMOP_inject_trap            14
>  /* Inject a trap into a VCPU, which will get taken up on the next
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index f19ac14..5bcd475 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -363,9 +363,6 @@ typedef struct xen_pod_target xen_pod_target_t;
>  #define XENMEM_paging_op_evict              1
>  #define XENMEM_paging_op_prep               2
>  
> -#define XENMEM_access_op                    21
> -#define XENMEM_access_op_resume             0
> -
>  struct xen_mem_event_op {
>      uint8_t     op;         /* XENMEM_*_op_* */
>      domid_t     domain;
> @@ -379,6 +376,56 @@ struct xen_mem_event_op {
>  typedef struct xen_mem_event_op xen_mem_event_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_mem_event_op_t);
>  
> +#define XENMEM_access_op                    21
> +#define XENMEM_access_op_resume             0
> +#define XENMEM_access_op_set_access         1
> +#define XENMEM_access_op_get_access         2
> +
> +typedef enum {
> +    XENMEM_access_n,
> +    XENMEM_access_r,
> +    XENMEM_access_w,
> +    XENMEM_access_rw,
> +    XENMEM_access_x,
> +    XENMEM_access_rx,
> +    XENMEM_access_wx,
> +    XENMEM_access_rwx,
> +    /*
> +     * Page starts off as r-x, but automatically
> +     * change to r-w on a write
> +     */
> +    XENMEM_access_rx2rw,
> +    /*
> +     * Log access: starts off as n, automatically
> +     * goes to rwx, generating an event without
> +     * pausing the vcpu
> +     */
> +    XENMEM_access_n2rwx,
> +    /* Take the domain default */
> +    XENMEM_access_default
> +} xenmem_access_t;
> +
> +struct xen_mem_access_op {
> +    /* XENMEM_access_op_* */
> +    uint8_t op;
> +    /* xenmem_access_t */
> +    uint8_t access;
> +    domid_t domid;
> +    /*
> +     * Number of pages for set op
> +     * Ignored on setting default access and other ops
> +     */
> +    uint32_t nr;
> +    /*
> +     * First pfn for set op
> +     * pfn for get op
> +     * ~0ull is used to set and get the default access for pages
> +     */
> +    uint64_aligned_t pfn;
> +};
> +typedef struct xen_mem_access_op xen_mem_access_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_mem_access_op_t);
> +
>  #define XENMEM_sharing_op                   22
>  #define XENMEM_sharing_op_nominate_gfn      0
>  #define XENMEM_sharing_op_nominate_gref     1
> diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
> index 5d354d8..9a35dd7 100644
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -60,6 +60,7 @@
>  !	memory_exchange			memory.h
>  !	memory_map			memory.h
>  !	memory_reservation		memory.h
> +?	mem_access_op		memory.h
>  !	pod_target			memory.h
>  !	remove_from_physmap		memory.h
>  ?	physdev_eoi			physdev.h

Re: [PATCH v2 3/3] tools/xen-access: Use the new mem_access APIs

[Jan Beulich]

>>> On 15.04.14 at 11:47, <andrew.cooper3@citrix.com> wrote:
> On 14/04/14 23:02, Aravindh Puthiyaparambil wrote:
>> Modify the xen-access test program to use the new mem_access APIs.
>>
>> Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
> 
> This patch must be folded into the previous patch which makes the
> changes in libxc, or the build will break.

Does tools/tests/ get built by default? I thought not...

Jan

Re: [PATCH v2 1/3] x86/mem_access: Make the mem_access ops generic

[Aravindh Puthiyaparambil (aravindp)]

>> -int mem_access_memop(struct domain *d, xen_mem_event_op_t *meo)
>> +int mem_access_memop(unsigned long cmd,
>> +                     XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg)
>>  {
>> -    int rc;
>> +    long rc;
>> +    xen_mem_access_op_t mao;
>> +    struct domain *d;
>> +
>> +    if ( copy_from_guest(&mao, arg, 1) )
>> +        return -EFAULT;
>> +
>> +    rc = rcu_lock_live_remote_domain_by_id(mao.domid, &d);
>> +    if ( rc )
>> +        return rc;
>> +
>> +    if ( !is_hvm_domain(d) )
>> +        return -EINVAL;
>> +
>> +    rc = xsm_mem_event_op(XSM_TARGET, d, XENMEM_access_op);
>> +    if ( rc )
>> +        goto out;
>>
>>      if ( unlikely(!d->mem_event->access.ring_page) )
>>          return -ENODEV;
>
>This and the earlier return need to become "goto out".

Ah yes. I will fix that.

>>
>> -    switch( meo->op )
>> +    switch ( mao.op )
>>      {
>>      case XENMEM_access_op_resume:
>>      {
>>          p2m_mem_access_resume(d);
>>          rc = 0;
>> +        break;
>> +    }
>
>Please remove the pointless opening brace rather than adding a closing
>one here.

Sorry, I should have caught that one. 

>> +
>> +    case XENMEM_access_op_set_access:
>> +    {
>> +        unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
>> +
>> +        rc = -EINVAL;
>> +        if ( (mao.pfn != ~0ull) &&
>> +             (mao.nr < start_iter ||
>> +              ((mao.pfn + mao.nr - 1) < mao.pfn) ||
>> +              ((mao.pfn + mao.nr - 1) > domain_get_maximum_gpfn(d))) )
>> +            break;
>> +
>> +        rc = p2m_set_mem_access(d, mao.pfn, mao.nr, start_iter,
>> +                                MEMOP_CMD_MASK, mao.access);
>> +        if ( rc > 0 )
>> +        {
>> +            ASSERT(!(rc & MEMOP_CMD_MASK));
>> +            rc = hypercall_create_continuation(__HYPERVISOR_memory_op,
>"lh",
>> +                                               cmd | rc, arg);
>
>Either you need to mask cmd properly, or directly use
>XENMEM_access_op_set_access.

I will directly use XENMEM_access_op as XENMEM_access_op_set_access is a sub-op.

Thanks,
Aravindh

Re: [PATCH v2 1/3] x86/mem_access: Make the mem_access ops generic

[Aravindh Puthiyaparambil (aravindp)]

>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index
>> 38c491e..eeaa72e 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4589,79 +4589,10 @@ long do_hvm_op(unsigned long op,
>XEN_GUEST_HANDLE_PARAM(void) arg)
>>      }
>>
>>      case HVMOP_set_mem_access:
>> -    {
>> -        struct xen_hvm_set_mem_access a;
>> -        struct domain *d;
>> -
>> -        if ( copy_from_guest(&a, arg, 1) )
>> -            return -EFAULT;
>> -
>> -        rc = rcu_lock_remote_domain_by_id(a.domid, &d);
>> -        if ( rc != 0 )
>> -            return rc;
>> -
>> -        rc = -EINVAL;
>> -        if ( !is_hvm_domain(d) )
>> -            goto param_fail5;
>> -
>> -        rc = xsm_hvm_param(XSM_TARGET, d, op);
>> -        if ( rc )
>> -            goto param_fail5;
>> -
>> -        rc = -EINVAL;
>> -        if ( (a.first_pfn != ~0ull) &&
>> -             (a.nr < start_iter ||
>> -              ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
>> -              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
>> -            goto param_fail5;
>> -
>> -        rc = p2m_set_mem_access(d, a.first_pfn, a.nr, start_iter,
>> -                                HVMOP_op_mask, a.hvmmem_access);
>> -        if ( rc > 0 )
>> -        {
>> -            start_iter = rc;
>> -            rc = -EAGAIN;
>> -        }
>> -
>> -    param_fail5:
>> -        rcu_unlock_domain(d);
>> -        break;
>> -    }
>> -
>>      case HVMOP_get_mem_access:
>>      {
>> -        struct xen_hvm_get_mem_access a;
>> -        struct domain *d;
>> -        hvmmem_access_t access;
>> -
>> -        if ( copy_from_guest(&a, arg, 1) )
>> -            return -EFAULT;
>> -
>> -        rc = rcu_lock_remote_domain_by_id(a.domid, &d);
>> -        if ( rc != 0 )
>> -            return rc;
>> -
>> -        rc = -EINVAL;
>> -        if ( !is_hvm_domain(d) )
>> -            goto param_fail6;
>> -
>> -        rc = xsm_hvm_param(XSM_TARGET, d, op);
>> -        if ( rc )
>> -            goto param_fail6;
>> -
>> -        rc = -EINVAL;
>> -        if ( (a.pfn > domain_get_maximum_gpfn(d)) && a.pfn != ~0ull )
>> -            goto param_fail6;
>> -
>> -        rc = p2m_get_mem_access(d, a.pfn, &access);
>> -        if ( rc != 0 )
>> -            goto param_fail6;
>> -
>> -        a.hvmmem_access = access;
>> -        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>> -
>> -    param_fail6:
>> -        rcu_unlock_domain(d);
>> +        gdprintk(XENLOG_DEBUG, "Deprecated HVM op %ld.\n", op);
>
>Is this really sensible?  I suppose it should hopefully be a rare hypercall, but it
>is hardly a useful message to print.

OK, I will get rid of the message.

>>
>>  /*
>>   * Internal functions, only called by other p2m code diff --git
>> a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
>> index 3204ec4..f00f6d2 100644
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>
>Changes here need to be in sync with libxc as well, also for build reasons.

That is correct. So how should I proceed? Should I submit one patch that has both the hypervisor and libxc changes?

Thanks,
Aravindh

Re: [PATCH v2 3/3] tools/xen-access: Use the new mem_access APIs

[Aravindh Puthiyaparambil (aravindp)]

>>> Modify the xen-access test program to use the new mem_access APIs.
>>>
>>> Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>
>> This patch must be folded into the previous patch which makes the
>> changes in libxc, or the build will break.
>
>Does tools/tests/ get built by default? I thought not...

That is correct. tools/tests do not get built by default.

Thanks,
Aravindh

Re: [PATCH v2 1/3] x86/mem_access: Make the mem_access ops generic

[Jan Beulich]

>>> On 15.04.14 at 18:08, <aravindp@cisco.com> wrote:
>>> --- a/xen/include/public/hvm/hvm_op.h
>>> +++ b/xen/include/public/hvm/hvm_op.h
>>
>>Changes here need to be in sync with libxc as well, also for build reasons.
> 
> That is correct. So how should I proceed? Should I submit one patch that has 
> both the hypervisor and libxc changes?

I think your first patch should leave the bits here in place, and they
should be removed by a separate patch following the libxc one.

Jan