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

[PATCH 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         | 51 ++++++++++++++++++++++---
 tools/libxc/xc_misc.c               | 61 ------------------------------
 tools/libxc/xenctrl.h               | 29 +++++++-------
 tools/tests/xen-access/xen-access.c | 31 +++++++--------
 xen/arch/x86/hvm/hvm.c              | 73 +-----------------------------------
 xen/arch/x86/mm.c                   |  5 ++-
 xen/arch/x86/mm/mem_access.c        | 75 +++++++++++++++++++++++++++++++++++--
 xen/arch/x86/mm/mem_event.c         |  3 --
 xen/arch/x86/mm/p2m.c               | 28 +++++++-------
 xen/arch/x86/x86_64/compat/mm.c     | 15 +++++---
 xen/arch/x86/x86_64/mm.c            |  9 ++++-
 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            |  9 +++--
 xen/include/asm-x86/p2m.h           |  6 +--
 xen/include/public/hvm/hvm_op.h     | 42 +--------------------
 xen/include/public/memory.h         | 53 ++++++++++++++++++++++++--
 18 files changed, 249 insertions(+), 248 deletions(-)

-- 
1.8.3.2

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

Thanks,
Aravindh

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..6f93dc1 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4631,7 +4631,8 @@ int xenmem_add_to_physmap_one(
     return rc;
 }
 
-long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
+long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg,
+                    unsigned long start_extent)
 {
     int rc;
 
@@ -4853,7 +4854,7 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
     }
 
     default:
-        return subarch_memory_op(op, arg);
+        return subarch_memory_op(op, arg, start_extent);
     }
 
     return 0;
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 50aaf27..8eac2bf 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -21,31 +21,98 @@
  */
 
 
+#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)
+#define ACCESS_op_mask  0xff
+
+int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned long start_iter)
 {
-    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:
+        rc = -EINVAL;
+
+        /*
+         * Undo adjustment done in do_memory_op() so that it matches
+         * p2m_set_mem_access()
+         */
+        start_iter <<= MEMOP_EXTENT_SHIFT;
+        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,
+                                ACCESS_op_mask, mao.access);
+        if ( rc > 0 )
+        {
+            ASSERT(!(rc & ACCESS_op_mask));
+            rc = hypercall_create_continuation(__HYPERVISOR_memory_op, "lh",
+                                               XENMEM_access_op | 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_to_guest(arg, &mao, 1) ? -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..3d7c969 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,23 @@ 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[] = {
+        XENMEM_access_n,
+        XENMEM_access_r,
+        XENMEM_access_w,
+        XENMEM_access_rw,
+        XENMEM_access_x,
+        XENMEM_access_rx,
+        XENMEM_access_wx,
+        XENMEM_access_rwx,
+        XENMEM_access_rx2rw
     };
 
     /* 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..40feedd 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,8 @@ 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(int op, XEN_GUEST_HANDLE_PARAM(void) arg,
+                          unsigned long start_extent)
 {
     struct compat_machphys_mfn_list xmml;
     l2_pgentry_t l2e;
@@ -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(op, guest_handle_from_ptr(nat, void), start_extent);
 
         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(op, guest_handle_from_ptr(nat, void), start_extent);
         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(op, guest_handle_from_ptr(nat, void), start_extent);
         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,10 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
         break;
     }
+    case XENMEM_access_op:
+        rc = mem_access_memop(arg, start_extent);
+        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..783a722 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -36,6 +36,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 +949,8 @@ void __init subarch_init_memory(void)
     }
 }
 
-long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
+long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg,
+                       unsigned long start_extent)
 {
     struct xen_machphys_mfn_list xmml;
     l3_pgentry_t l3e;
@@ -1007,7 +1009,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 +1018,10 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
         break;
     }
+    case XENMEM_access_op:
+        rc = mem_access_memop(arg, start_extent);
+        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..a38ecf6 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -271,7 +271,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
         }
 
         default:
-            return compat_arch_memory_op(cmd, compat);
+            return compat_arch_memory_op(cmd, compat, start_extent);
         }
 
         rc = do_memory_op(cmd, nat.hnd);
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 4d6ffee..fd20ecd 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(op, arg, start_extent);
         break;
     }
 
diff --git a/xen/include/asm-x86/mem_access.h b/xen/include/asm-x86/mem_access.h
index 60c2834..f99fd1b 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(XEN_GUEST_HANDLE_PARAM(void) arg,
+                     unsigned long start_iter);
 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..d10f3bd 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -561,9 +561,12 @@ 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(int op, XEN_GUEST_HANDLE_PARAM(void) arg,
+                    unsigned long start_extent);
+long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg,
+                       unsigned long start_extent);
+int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void),
+                          unsigned long start_extent);
 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
-- 
1.8.3.2

[PATCH 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 the RFC version of the patch:
1. Remove xc_mem_access_memop() wrapper.
2. Remove xc_hvm_[sg]et_mem_access() APIs.

Thanks,
Aravindh

diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
index a50c145..0277ab3 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,53 @@ 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)
 {
-    return xc_mem_event_memop(xch, domain_id,
-                                XENMEM_access_op_resume,
-                                XENMEM_access_op,
-                                gfn, NULL);
+    xen_mem_access_op_t mao;
+
+    memset(&mao, 0, sizeof(mao));
+    mao.op    = XENMEM_access_op_resume;
+    mao.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)
+{
+    xen_mem_access_op_t mao;
+
+    mao.op     = XENMEM_access_op_set_access;
+    mao.domid  = domain_id;
+    mao.access = access;
+    mao.pfn    = first_pfn;
+    mao.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)
+{
+    xen_mem_access_op_t mao;
+    int rc;
+
+    mao.op    = XENMEM_access_op_get_access;
+    mao.domid = domain_id;
+    mao.pfn   = pfn;
+
+    rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
+
+    if ( !rc )
+        *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..0be0fd7 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. 
  */
@@ -2062,6 +2048,21 @@ 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);
 
+/*
+ * 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 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>

diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index f6522a1..d889a9c 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -440,8 +440,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 +475,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 +520,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 +554,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 +576,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 +592,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 +612,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 1/3] x86/mem_access: Make the mem_access ops generic

[Jan Beulich]

>>> On 11.04.14 at 05:49, <aravindp@cisco.com> wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -21,31 +21,98 @@
>   */
>  
>  
> +#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)
> +#define ACCESS_op_mask  0xff

I think this needs to be MEMOP_CMD_MASK (and then you wouldn't
need a new #define anyway). Or at least you need to put a
BUILD_BUG_ON() somewhere making sure this mask is at least as
wide as MEMOP_CMD_MASK.

> +
> +int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned long start_iter)
>  {
> -    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:
> +        rc = -EINVAL;
> +
> +        /*
> +         * Undo adjustment done in do_memory_op() so that it matches
> +         * p2m_set_mem_access()
> +         */
> +        start_iter <<= MEMOP_EXTENT_SHIFT;

This is why perhaps it would be better to pass "cmd" down instead of
"op" and "start_extent".

> +        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,
> +                                ACCESS_op_mask, mao.access);
> +        if ( rc > 0 )
> +        {
> +            ASSERT(!(rc & ACCESS_op_mask));
> +            rc = hypercall_create_continuation(__HYPERVISOR_memory_op, "lh",
> +                                               XENMEM_access_op | 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_to_guest(arg, &mao, 1) ? -EFAULT : 0;

If you passed a properly typed handle into this function (which is
possible because it wants only one kind of interface structure) you
would get away with the cheaper __copy_field_to_guest() here.

> @@ -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),
> @@ -1416,23 +1416,23 @@ 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[] = {
> +        XENMEM_access_n,
> +        XENMEM_access_r,
> +        XENMEM_access_w,
> +        XENMEM_access_rw,
> +        XENMEM_access_x,
> +        XENMEM_access_rx,
> +        XENMEM_access_wx,
> +        XENMEM_access_rwx,
> +        XENMEM_access_rx2rw

Please use an ACCESS() macro similar to that added by commit
ca0f2184 (also seen in the quoted hunk above this one) if you
already need to touch this, to remove the ordering dependency.

> @@ -195,6 +196,10 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              return -EFAULT;
>          break;
>      }
> +    case XENMEM_access_op:
> +        rc = mem_access_memop(arg, start_extent);
> +        break;
> +

Blank line also above this block please. Also you'll need to add
verification that the compat and native structures' layouts match
(i.e. to prove that you're allowed to pass the argument here
without translation). Looks like this is also missing for the already
existing xen_mem_event_op_t and xen_mem_sharing_op_t.

> @@ -1017,6 +1018,10 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              return -EFAULT;
>          break;
>      }
> +    case XENMEM_access_op:
> +        rc = mem_access_memop(arg, start_extent);
> +        break;
> +

Again blank line above this block too please.

Jan

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

[Andrew Cooper]

On 11/04/14 04:49, Aravindh Puthiyaparambil wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -21,31 +21,98 @@
>   */
>  
>  
> +#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)
> +#define ACCESS_op_mask  0xff
> +
> +int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned long start_iter)
>  {
> -    int rc;
> +    long rc;

int function and long rc? The function wants promoting to long.

~Andrew

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

[Andrew Cooper]

On 11/04/14 04:49, Aravindh Puthiyaparambil wrote:
> 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 the RFC version of the patch:
> 1. Remove xc_mem_access_memop() wrapper.
> 2. Remove xc_hvm_[sg]et_mem_access() APIs.
>
> Thanks,
> Aravindh
>
> diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
> index a50c145..0277ab3 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,53 @@ int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
>                                  NULL);
>  }
>  
> +

Spurious whitespace change.

>  int xc_mem_access_resume(xc_interface *xch, domid_t domain_id, unsigned long gfn)

What is this gfn parameter doing?  It isn't used in the hypercall.

>  {
> -    return xc_mem_event_memop(xch, domain_id,
> -                                XENMEM_access_op_resume,
> -                                XENMEM_access_op,
> -                                gfn, NULL);
> +    xen_mem_access_op_t mao;
> +
> +    memset(&mao, 0, sizeof(mao));
> +    mao.op    = XENMEM_access_op_resume;
> +    mao.domid = domain_id;

Please use structure initialisation for this.

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)
> +{
> +    xen_mem_access_op_t mao;
> +
> +    mao.op     = XENMEM_access_op_set_access;
> +    mao.domid  = domain_id;
> +    mao.access = access;
> +    mao.pfn    = first_pfn;
> +    mao.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)
> +{
> +    xen_mem_access_op_t mao;
> +    int rc;
> +
> +    mao.op    = XENMEM_access_op_get_access;
> +    mao.domid = domain_id;
> +    mao.pfn   = pfn;
> +
> +    rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
> +
> +    if ( !rc )
> +        *access = mao.access;

Surely you should only write access back in the case of success?

~Andrew

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

[Jan Beulich]

>>> On 11.04.14 at 11:35, <andrew.cooper3@citrix.com> wrote:
> On 11/04/14 04:49, Aravindh Puthiyaparambil wrote:
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -21,31 +21,98 @@
>>   */
>>  
>>  
>> +#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)
>> +#define ACCESS_op_mask  0xff
>> +
>> +int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned long 
> start_iter)
>>  {
>> -    int rc;
>> +    long rc;
> 
> int function and long rc? The function wants promoting to long.

No, what makes it out of the function fits in an "int", but the result
from p2m_set_mem_access() needs the variable to be "long".

Jan

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

[Andrew Cooper]

On 11/04/14 10:48, Jan Beulich wrote:
>>>> On 11.04.14 at 11:35, <andrew.cooper3@citrix.com> wrote:
>> On 11/04/14 04:49, Aravindh Puthiyaparambil wrote:
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -21,31 +21,98 @@
>>>   */
>>>  
>>>  
>>> +#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)
>>> +#define ACCESS_op_mask  0xff
>>> +
>>> +int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned long 
>> start_iter)
>>>  {
>>> -    int rc;
>>> +    long rc;
>> int function and long rc? The function wants promoting to long.
> No, what makes it out of the function fits in an "int", 

Why does that matter?  It is a recipe for accidental truncation with
future changes (and certainly not the first example in Xen).

All hypercall handlers should be returning longs, and leaving it to the
compat layer to apply correct truncation if needed.

~Andrew

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

[Jan Beulich]

>>> On 11.04.14 at 11:53, <andrew.cooper3@citrix.com> wrote:
> On 11/04/14 10:48, Jan Beulich wrote:
>>>>> On 11.04.14 at 11:35, <andrew.cooper3@citrix.com> wrote:
>>> On 11/04/14 04:49, Aravindh Puthiyaparambil wrote:
>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>> @@ -21,31 +21,98 @@
>>>>   */
>>>>  
>>>>  
>>>> +#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)
>>>> +#define ACCESS_op_mask  0xff
>>>> +
>>>> +int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned long 
>>> start_iter)
>>>>  {
>>>> -    int rc;
>>>> +    long rc;
>>> int function and long rc? The function wants promoting to long.
>> No, what makes it out of the function fits in an "int", 
> 
> Why does that matter?  It is a recipe for accidental truncation with
> future changes (and certainly not the first example in Xen).
> 
> All hypercall handlers should be returning longs, and leaving it to the
> compat layer to apply correct truncation if needed.

For top level hypercall handlers I agree, but for helper functions
like this one I don't.

Jan

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

[Aravindh Puthiyaparambil (aravindp)]

>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -21,31 +21,98 @@
>>   */
>>
>>
>> +#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)
>> +#define ACCESS_op_mask  0xff
>
>I think this needs to be MEMOP_CMD_MASK (and then you wouldn't need a
>new #define anyway). Or at least you need to put a
>BUILD_BUG_ON() somewhere making sure this mask is at least as wide as
>MEMOP_CMD_MASK.

I did not go with MEMOP_CMD_MASK as it would mean that only 64 pfns would be set if preemption is needed. I would rather have at least 256 set the way you had it when this was a HVM op. I will add a BUILD_BUG_ON here.  

>> +
>> +int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg,
>unsigned long
>> +start_iter)
>>  {
>> -    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:
>> +        rc = -EINVAL;
>> +
>> +        /*
>> +         * Undo adjustment done in do_memory_op() so that it matches
>> +         * p2m_set_mem_access()
>> +         */
>> +        start_iter <<= MEMOP_EXTENT_SHIFT;
>
>This is why perhaps it would be better to pass "cmd" down instead of "op"
>and "start_extent".

OK, I will pass down "cmd" instead of "op" and "start_extent".

>> +        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,
>> +                                ACCESS_op_mask, mao.access);
>> +        if ( rc > 0 )
>> +        {
>> +            ASSERT(!(rc & ACCESS_op_mask));
>> +            rc = hypercall_create_continuation(__HYPERVISOR_memory_op,
>"lh",
>> +                                               XENMEM_access_op | 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_to_guest(arg, &mao, 1) ? -EFAULT : 0;
>
>If you passed a properly typed handle into this function (which is possible
>because it wants only one kind of interface structure) you would get away
>with the cheaper __copy_field_to_guest() here.

Excellent suggestion. I will use __copy_field_to_guest() here.

>> @@ -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),
>> @@ -1416,23 +1416,23 @@ 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[] = {
>> +        XENMEM_access_n,
>> +        XENMEM_access_r,
>> +        XENMEM_access_w,
>> +        XENMEM_access_rw,
>> +        XENMEM_access_x,
>> +        XENMEM_access_rx,
>> +        XENMEM_access_wx,
>> +        XENMEM_access_rwx,
>> +        XENMEM_access_rx2rw
>
>Please use an ACCESS() macro similar to that added by commit
>ca0f2184 (also seen in the quoted hunk above this one) if you already need to
>touch this, to remove the ordering dependency.

OK, I will do that.

>> @@ -195,6 +196,10 @@ int compat_arch_memory_op(int op,
>XEN_GUEST_HANDLE_PARAM(void) arg)
>>              return -EFAULT;
>>          break;
>>      }
>> +    case XENMEM_access_op:
>> +        rc = mem_access_memop(arg, start_extent);
>> +        break;
>> +
>
>Blank line also above this block please. Also you'll need to add verification that
>the compat and native structures' layouts match (i.e. to prove that you're
>allowed to pass the argument here without translation). Looks like this is also
>missing for the already existing xen_mem_event_op_t and
>xen_mem_sharing_op_t.

Is the following what I need to do?

diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index a38ecf6..005e602 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/include/xlat.lst b/xen/include/xlat.lst
index 5d354d8..0e7cef3 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -62,6 +62,7 @@
 !      memory_reservation              memory.h
 !      pod_target                      memory.h
 !      remove_from_physmap             memory.h
+?      mem_access_op           memory.h
 ?      physdev_eoi                     physdev.h
 ?      physdev_get_free_pirq           physdev.h
 ?      physdev_irq                     physdev.h

Thanks,
Aravindh

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

[Aravindh Puthiyaparambil (aravindp)]

>>  int xc_mem_access_resume(xc_interface *xch, domid_t domain_id,
>> unsigned long gfn)
>
>What is this gfn parameter doing?  It isn't used in the hypercall.

This was the way it was before my change. But I agree that it is pointless and will get rid of it. 

>>  {
>> -    return xc_mem_event_memop(xch, domain_id,
>> -                                XENMEM_access_op_resume,
>> -                                XENMEM_access_op,
>> -                                gfn, NULL);
>> +    xen_mem_access_op_t mao;
>> +
>> +    memset(&mao, 0, sizeof(mao));
>> +    mao.op    = XENMEM_access_op_resume;
>> +    mao.domid = domain_id;
>
>Please use structure initialisation for this.
>
>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)
>> +{
>> +    xen_mem_access_op_t mao;
>> +
>> +    mao.op     = XENMEM_access_op_set_access;
>> +    mao.domid  = domain_id;
>> +    mao.access = access;
>> +    mao.pfn    = first_pfn;
>> +    mao.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) {
>> +    xen_mem_access_op_t mao;
>> +    int rc;
>> +
>> +    mao.op    = XENMEM_access_op_get_access;
>> +    mao.domid = domain_id;
>> +    mao.pfn   = pfn;
>> +
>> +    rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
>> +
>> +    if ( !rc )
>> +        *access = mao.access;
>
>Surely you should only write access back in the case of success?

Ah yes! This is how the original xc_hvm_get_mem_access() was written and I was too judicious in my copying it over. I will fix that here.

Thanks,
Aravindh

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

[Jan Beulich]

>>> On 11.04.14 at 22:07, <aravindp@cisco.com> wrote:
>> > --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -21,31 +21,98 @@
>>>   */
>>>
>>>
>>> +#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)
>>> +#define ACCESS_op_mask  0xff
>>
>>I think this needs to be MEMOP_CMD_MASK (and then you wouldn't need a
>>new #define anyway). Or at least you need to put a
>>BUILD_BUG_ON() somewhere making sure this mask is at least as wide as
>>MEMOP_CMD_MASK.
> 
> I did not go with MEMOP_CMD_MASK as it would mean that only 64 pfns would be 
> set if preemption is needed. I would rather have at least 256 set the way you 
> had it when this was a HVM op. I will add a BUILD_BUG_ON here.  

Not really good a reason - remember that before this recent change
preemption could have kicked in after _every_ iteration. I'd really
prefer to have a separate mask/width here only if really proven to
be providing a significant performance benefit.

>>Also you'll need to add verification that
>>the compat and native structures' layouts match (i.e. to prove that you're
>>allowed to pass the argument here without translation). Looks like this is also
>>missing for the already existing xen_mem_event_op_t and
>>xen_mem_sharing_op_t.
> 
> Is the following what I need to do?
> 
> diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
> index a38ecf6..005e602 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/include/xlat.lst b/xen/include/xlat.lst
> index 5d354d8..0e7cef3 100644
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -62,6 +62,7 @@
>  !      memory_reservation              memory.h
>  !      pod_target                      memory.h
>  !      remove_from_physmap             memory.h
> +?      mem_access_op           memory.h
>  ?      physdev_eoi                     physdev.h
>  ?      physdev_get_free_pirq           physdev.h
>  ?      physdev_irq                     physdev.h

Yes, this looks like what you'd need. Just keep xlat.lst as alphabetically
sorted as possible (iirc there have been other violations, but we should
aim to avoid introducing more).

Jan