[PATCH RFC 0/2] Make mem_access APIs and hypercalls generic

[PATCH RFC 0/2] 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>

---

The main reason for the RFC is that I am not sure if I am doing the hypercall 
preemption continuation correctly. Please take a look and let me know how I 
should proceed. 
          
Thanks,
Aravindh

  x86/mem_access: Make the mem_access ops generic
  tools/libxc: Make the mem_access APIs generic

 tools/libxc/xc_mem_access.c      | 57 ++++++++++++++++++++++++++++---
 tools/libxc/xc_misc.c            | 60 +++++----------------------------
 tools/libxc/xenctrl.h            | 18 ++++++++++
 xen/arch/x86/hvm/hvm.c           | 73 ++--------------------------------------
 xen/arch/x86/mm/mem_access.c     | 68 +++++++++++++++++++++++++++++++++++--
 xen/arch/x86/mm/mem_event.c      |  3 --
 xen/arch/x86/mm/p2m.c            | 34 +++++++++----------
 xen/arch/x86/x86_64/compat/mm.c  |  7 +++-
 xen/arch/x86/x86_64/mm.c         |  7 +++-
 xen/include/asm-x86/mem_access.h |  2 +-
 xen/include/asm-x86/p2m.h        |  6 ++--
 xen/include/public/hvm/hvm_op.h  | 50 +++++++++++++--------------
 xen/include/public/memory.h      | 53 +++++++++++++++++++++++++++--
 13 files changed, 251 insertions(+), 187 deletions(-)

-- 
1.8.3.2

[PATCH RFC 1/2] 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. Change the preemption handling for XENMEM_set_mem_access to use the
interface structures.

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>

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/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 50aaf27..27415c1 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -21,31 +21,93 @@
  */
 
 
+#include <xen/sched.h>
+#include <xen/guest_access.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(XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     int 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;
+
+        if ( (mao.pfn != ~0ull) &&
+             (((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, mao.xenmem_access);
+        if ( rc > 0 )
+        {
+            mao.pfn += mao.nr - rc;
+            mao.nr = rc;
+            if ( __copy_to_guest(arg, &mao, 1) )
+                rc = -EFAULT;
+            else
+                rc = hypercall_create_continuation(__HYPERVISOR_memory_op, "lh",
+                                                   XENMEM_access_op, 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.xenmem_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..d932f19 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)
+                        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:
@@ -1393,7 +1393,7 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
     }
 
     p2m_lock(p2m);
-    for ( pfn += start; nr > start; ++pfn )
+    for ( ; ; ++pfn )
     {
         mfn = p2m->get_entry(p2m, pfn, &t, &_a, 0, NULL);
         if ( p2m->set_entry(p2m, pfn, mfn, PAGE_ORDER_4K, t, a) == 0 )
@@ -1403,9 +1403,9 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
         }
 
         /* Check for continuation if it's not the last interation. */
-        if ( nr > ++start && !(start & mask) && hypercall_preempt_check() )
+        if ( !--nr || hypercall_preempt_check() )
         {
-            rc = start;
+            rc = nr;
             break;
         }
     }
@@ -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..77d593d 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)
 {
@@ -185,7 +186,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 +195,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(arg);
+        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..767bbfb 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. */
@@ -1007,7 +1008,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 +1017,11 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
         break;
     }
+    case XENMEM_access_op:
+    {
+        rc = mem_access_memop(arg);
+        break;
+    }
     case XENMEM_sharing_op:
     {
         xen_mem_sharing_op_t mso;
diff --git a/xen/include/asm-x86/mem_access.h b/xen/include/asm-x86/mem_access.h
index 60c2834..1138ccf 100644
--- a/xen/include/asm-x86/mem_access.h
+++ b/xen/include/asm-x86/mem_access.h
@@ -23,7 +23,7 @@
 #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);
 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/p2m.h b/xen/include/asm-x86/p2m.h
index d644f82..3f8e00d 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);
+                        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..fa9ac17 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -23,6 +23,7 @@
 
 #include "../xen.h"
 #include "../trace.h"
+#include "../memory.h"
 
 /* Get/set subcommands: extra argument == pointer to xen_hvm_param struct. */
 #define HVMOP_set_param           0
@@ -162,36 +163,31 @@ 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_n = XENMEM_access_n,
+    HVMMEM_access_r = XENMEM_access_r,
+    HVMMEM_access_w = XENMEM_access_w,
+    HVMMEM_access_rw = XENMEM_access_rw,
+    HVMMEM_access_x = XENMEM_access_x,
+    HVMMEM_access_rx = XENMEM_access_rx,
+    HVMMEM_access_wx = XENMEM_access_wx,
+    HVMMEM_access_rwx = XENMEM_access_rwx,
+    /*
+     * Page starts off as r-x, but automatically
+     * change to r-w on a write
+     */
+    HVMMEM_access_rx2rw = XENMEM_access_rx2rw,
+    /*
+     * Log access: starts off as n, automatically
+     * goes to rwx, generating an event without
+     * pausing the vcpu
+     */
+    HVMMEM_access_n2rwx = XENMEM_access_n2rwx,
+    /* Take the domain default */
+    HVMMEM_access_default = XENMEM_access_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);
 
 #define HVMOP_get_mem_access        13
 /* Get the specific access type for that region of memory */
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index f19ac14..129d841 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;
+    domid_t     domid;
+    /* xenmem_access_t */
+    uint16_t xenmem_access;
+    /*
+     * 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

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

[Jan Beulich]

>>> On 09.04.14 at 07:50, <aravindp@cisco.com> wrote:
> -int mem_access_memop(struct domain *d, xen_mem_event_op_t *meo)
> +int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
>      int 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:
> +    {

Please no pointless braces.

> +        rc = -EINVAL;
> +
> +        if ( (mao.pfn != ~0ull) &&
> +             (((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, mao.xenmem_access);
> +        if ( rc > 0 )
> +        {
> +            mao.pfn += mao.nr - rc;
> +            mao.nr = rc;
> +            if ( __copy_to_guest(arg, &mao, 1) )
> +                rc = -EFAULT;
> +            else
> +                rc = hypercall_create_continuation(__HYPERVISOR_memory_op, "lh",
> +                                                   XENMEM_access_op, arg);

As already said - this should be done without playing with the interface
structure. All you need for this is to pass down the upper "cmd" bits
from do_memory_op().

> @@ -162,36 +163,31 @@ 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_n = XENMEM_access_n,
> +    HVMMEM_access_r = XENMEM_access_r,
> +    HVMMEM_access_w = XENMEM_access_w,
> +    HVMMEM_access_rw = XENMEM_access_rw,
> +    HVMMEM_access_x = XENMEM_access_x,
> +    HVMMEM_access_rx = XENMEM_access_rx,
> +    HVMMEM_access_wx = XENMEM_access_wx,
> +    HVMMEM_access_rwx = XENMEM_access_rwx,
> +    /*
> +     * Page starts off as r-x, but automatically
> +     * change to r-w on a write
> +     */
> +    HVMMEM_access_rx2rw = XENMEM_access_rx2rw,
> +    /*
> +     * Log access: starts off as n, automatically
> +     * goes to rwx, generating an event without
> +     * pausing the vcpu
> +     */
> +    HVMMEM_access_n2rwx = XENMEM_access_n2rwx,
> +    /* Take the domain default */
> +    HVMMEM_access_default = XENMEM_access_default
>  } hvmmem_access_t;

So what is the reason for you to keep this while deleting the interface
structures?

> @@ -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;
> +    domid_t     domid;
> +    /* xenmem_access_t */
> +    uint16_t xenmem_access;

If you limited this to 8 bits and put it above domid, you'd get away
without any implicit padding (which I would otherwise ask you to make
explicit).

Also I don't see the need for the xenmem_ prefix.

Jan

Re: [PATCH RFC 1/2] 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(XEN_GUEST_HANDLE_PARAM(void) arg)
>>  {
>>      int 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:
>> +    {
>
>Please no pointless braces.

Sorry, I will get rid of them.
 
>> +        rc = -EINVAL;
>> +
>> +        if ( (mao.pfn != ~0ull) &&
>> +             (((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,
>mao.xenmem_access);
>> +        if ( rc > 0 )
>> +        {
>> +            mao.pfn += mao.nr - rc;
>> +            mao.nr = rc;
>> +            if ( __copy_to_guest(arg, &mao, 1) )
>> +                rc = -EFAULT;
>> +            else
>> +                rc = hypercall_create_continuation(__HYPERVISOR_memory_op,
>"lh",
>> +                                                   XENMEM_access_op,
>> + arg);
>
>As already said - this should be done without playing with the interface
>structure. All you need for this is to pass down the upper "cmd" bits from
>do_memory_op().

OK, I will give that a shot. 

>> @@ -162,36 +163,31 @@
>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_n = XENMEM_access_n,
>> +    HVMMEM_access_r = XENMEM_access_r,
>> +    HVMMEM_access_w = XENMEM_access_w,
>> +    HVMMEM_access_rw = XENMEM_access_rw,
>> +    HVMMEM_access_x = XENMEM_access_x,
>> +    HVMMEM_access_rx = XENMEM_access_rx,
>> +    HVMMEM_access_wx = XENMEM_access_wx,
>> +    HVMMEM_access_rwx = XENMEM_access_rwx,
>> +    /*
>> +     * Page starts off as r-x, but automatically
>> +     * change to r-w on a write
>> +     */
>> +    HVMMEM_access_rx2rw = XENMEM_access_rx2rw,
>> +    /*
>> +     * Log access: starts off as n, automatically
>> +     * goes to rwx, generating an event without
>> +     * pausing the vcpu
>> +     */
>> +    HVMMEM_access_n2rwx = XENMEM_access_n2rwx,
>> +    /* Take the domain default */
>> +    HVMMEM_access_default = XENMEM_access_default
>>  } hvmmem_access_t;
>
>So what is the reason for you to keep this while deleting the interface
>structures?

I was keeping to maintain backward compatibility as I thought I need to retain xc_hvm_[sg]et_mem_access(). But after IanC's asking me to nuke them, there is no reason for the struct and enum to hang around.

>> @@ -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;
>> +    domid_t     domid;
>> +    /* xenmem_access_t */
>> +    uint16_t xenmem_access;
>
>If you limited this to 8 bits and put it above domid, you'd get away without any
>implicit padding (which I would otherwise ask you to make explicit).

OK I will make it a uint8_t and move it above domid.

>Also I don't see the need for the xenmem_ prefix.

I will get rid of the prefix. What about the enum? Should I call it mem_access_t and MEM_access_*?

Thanks,
Aravindh

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

[Jan Beulich]

>>> On 10.04.14 at 02:39, <aravindp@cisco.com> wrote:
>>Also I don't see the need for the xenmem_ prefix.
> 
> I will get rid of the prefix. What about the enum? Should I call it 
> mem_access_t and MEM_access_*?

Definitely not - everything (new; I know we have things violating this)
in global name space should have a XEN, xen, XEN_, or xen_ prefix.

Jan

[PATCH RFC 2/2] 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. Make xc_hvm_[sg]et_mem_access call the newer xc_[sg]et_mem_access
APIs and mark them as deprecated.
3. Make the upper bound for the number of pages to UINT_MAX for
xc_set_mem_access()

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/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
index a50c145..2617c0e 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,59 @@ int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
                                 NULL);
 }
 
+static int xc_mem_access_memop(xc_interface *xch, domid_t domid,
+                               xen_mem_access_op_t *mao)
+{
+    mao->domid = domid;
+
+    return do_memory_op(xch, XENMEM_access_op, mao, sizeof(*mao));
+}
+
 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;
+
+    return xc_mem_access_memop(xch, domain_id, &mao);
+}
+
+int xc_set_mem_access(xc_interface *xch,
+                      domid_t domain_id,
+                      xenmem_access_t mem_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.xenmem_access = mem_access;
+    mao.pfn           = first_pfn;
+    mao.nr            = nr;
+
+    return xc_mem_access_memop(xch, domain_id, &mao);
+}
+
+int xc_get_mem_access(xc_interface *xch,
+                      domid_t domain_id,
+                      uint64_t pfn,
+                      xenmem_access_t* mem_access)
+{
+    xen_mem_access_op_t mao;
+    int rc;
+
+    mao.op          = XENMEM_access_op_get_access;
+    mao.domid       = domain_id;
+    mao.pfn         = pfn;
+
+    rc = xc_mem_access_memop(xch, domain_id, &mao);
+
+    if ( !rc )
+        *mem_access = mao.xenmem_access;
+
+    return rc;
 }
 
 /*
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 3303454..c38fb94 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -19,7 +19,9 @@
  */
 
 #include "xc_private.h"
+#include <limits.h>
 #include <xen/hvm/hvm_op.h>
+#include <xen/memory.h>
 
 int xc_get_max_cpus(xc_interface *xch)
 {
@@ -594,64 +596,18 @@ int xc_hvm_set_mem_type(
 }
 
 int xc_hvm_set_mem_access(
-    xc_interface *xch, domid_t dom, hvmmem_access_t mem_access, uint64_t first_pfn, uint64_t nr)
+    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;
+    if ( nr > UINT_MAX )
+        return -EINVAL;
+    return xc_set_mem_access(xch, dom, mem_access, first_pfn, nr);
 }
 
 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;
+    return xc_get_mem_access(xch, dom, pfn, (xenmem_access_t *)mem_access);
 }
 
 int xc_hvm_inject_trap(
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index e3a32f2..2fc6743 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1730,14 +1730,17 @@ int xc_hvm_set_mem_type(
 
 /*
  * Set a range of memory to a specific access.
+ * Maximum value of nr is UINT_MAX
  * Allowed types are HVMMEM_access_default, HVMMEM_access_n, any combination of 
  * HVM_access_ + (rwx), and HVM_access_rx2rw
+ * Please note that this API has been deprecated by xc_set_mem_access()
  */
 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)
+ * Please note that this API has been deprecated by xc_get_mem_access()
  */
 int xc_hvm_get_mem_access(
     xc_interface *xch, domid_t dom, uint64_t pfn, hvmmem_access_t* memaccess);
@@ -2062,6 +2065,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 mem_access, uint64_t first_pfn,
+                      uint32_t nr);
+
+/*
+ * Gets the mem access for the given page (returned in mem_acess on success)
+ */
+int xc_get_mem_access(xc_interface *xch, domid_t domain_id,
+                      uint64_t pfn, xenmem_access_t* mem_access);
+
 /***
  * Memory sharing operations.
  *
-- 
1.8.3.2

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

[Ian Campbell]

On Tue, 2014-04-08 at 22:50 -0700, Aravindh Puthiyaparambil wrote:
> @@ -47,12 +47,59 @@ int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
>                                  NULL);
>  }
>  
> +static int xc_mem_access_memop(xc_interface *xch, domid_t domid,
> +                               xen_mem_access_op_t *mao)
> +{
> +    mao->domid = domid;
> +
> +    return do_memory_op(xch, XENMEM_access_op, mao, sizeof(*mao));

I'm not sure what this wrapper is bringing to the table over just making
the call in the callers. Also you inconsistently do and don't set
mao->domid in the callers. If you want to keep the wrapper then I'd
suggest making the callers set domid along with all the other fields and
dropping the domid argument here.

> @@ -594,64 +596,18 @@ int xc_hvm_set_mem_type(
>  }
>  
>  int xc_hvm_set_mem_access(
> -    xc_interface *xch, domid_t dom, hvmmem_access_t mem_access, uint64_t first_pfn, uint64_t nr)
> +    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;
> +    if ( nr > UINT_MAX )
> +        return -EINVAL;

I guess the hypervisor must also make a similar check?

> +    return xc_set_mem_access(xch, dom, mem_access, first_pfn, nr);

This will return -1 and set errno, while the EINVAL above returns a
-errno value. 

Personally I would just drop the limit check and rely on the hypervisor
to return an error, but if you want to keep the check then please make
it use the -1 and set errno pattern.

Is this now just a wrapper for xc_set_mem_access? Can we drop the now
pointless xc_hvm_*_mem_access wrappers? We don't guarantee API stability
for libxc.

Speaking of wrappers, how come the UINT_MAX limit check is only done in
this wrapper?

> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index e3a32f2..2fc6743 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1730,14 +1730,17 @@ int xc_hvm_set_mem_type(
>  
>  /*
>   * Set a range of memory to a specific access.
> + * Maximum value of nr is UINT_MAX
>   * Allowed types are HVMMEM_access_default, HVMMEM_access_n, any combination of 
>   * HVM_access_ + (rwx), and HVM_access_rx2rw
> + * Please note that this API has been deprecated by xc_set_mem_access()

Aha, lets nuke it then!

Ian.

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

[Jan Beulich]

>>> On 09.04.14 at 10:23, <Ian.Campbell@citrix.com> wrote:
>>  int xc_hvm_set_mem_access(
>> -    xc_interface *xch, domid_t dom, hvmmem_access_t mem_access, uint64_t first_pfn, uint64_t nr)
>> +    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;
>> +    if ( nr > UINT_MAX )
>> +        return -EINVAL;
> 
> I guess the hypervisor must also make a similar check?
> 
>> +    return xc_set_mem_access(xch, dom, mem_access, first_pfn, nr);
> 
> This will return -1 and set errno, while the EINVAL above returns a
> -errno value. 
> 
> Personally I would just drop the limit check and rely on the hypervisor
> to return an error, but if you want to keep the check then please make
> it use the -1 and set errno pattern.

Actually the hypervisor can't do that check, since it only sees the
truncated (32-bit) value. Question rather is why to retain a 64-bit
function parameter - I recently suggested switching it to 32 bits, and
I think this should definitely be done while anyway fiddling with the
interface.

Jan

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

[Ian Campbell]

On Wed, 2014-04-09 at 09:58 +0100, Jan Beulich wrote:
> >>> On 09.04.14 at 10:23, <Ian.Campbell@citrix.com> wrote:
> >>  int xc_hvm_set_mem_access(
> >> -    xc_interface *xch, domid_t dom, hvmmem_access_t mem_access, uint64_t first_pfn, uint64_t nr)
> >> +    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;
> >> +    if ( nr > UINT_MAX )
> >> +        return -EINVAL;
> > 
> > I guess the hypervisor must also make a similar check?
> > 
> >> +    return xc_set_mem_access(xch, dom, mem_access, first_pfn, nr);
> > 
> > This will return -1 and set errno, while the EINVAL above returns a
> > -errno value. 
> > 
> > Personally I would just drop the limit check and rely on the hypervisor
> > to return an error, but if you want to keep the check then please make
> > it use the -1 and set errno pattern.
> 
> Actually the hypervisor can't do that check, since it only sees the
> truncated (32-bit) value. Question rather is why to retain a 64-bit
> function parameter - I recently suggested switching it to 32 bits, and
> I think this should definitely be done while anyway fiddling with the
> interface.

Ah, nr is correct in xc_set_mem_access() so this is just briding the gap
between the old and new interfaces. THis would be solved by nuking this
now pointless wrapper.

Ian.

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

[Aravindh Puthiyaparambil (aravindp)]

>On Wed, 2014-04-09 at 09:58 +0100, Jan Beulich wrote:
>> >>> On 09.04.14 at 10:23, <Ian.Campbell@citrix.com> wrote:
>> >>  int xc_hvm_set_mem_access(
>> >> -    xc_interface *xch, domid_t dom, hvmmem_access_t mem_access,
>uint64_t first_pfn, uint64_t nr)
>> >> +    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;
>> >> +    if ( nr > UINT_MAX )
>> >> +        return -EINVAL;
>> >
>> > I guess the hypervisor must also make a similar check?
>> >
>> >> +    return xc_set_mem_access(xch, dom, mem_access, first_pfn, nr);
>> >
>> > This will return -1 and set errno, while the EINVAL above returns a
>> > -errno value.
>> >
>> > Personally I would just drop the limit check and rely on the hypervisor
>> > to return an error, but if you want to keep the check then please make
>> > it use the -1 and set errno pattern.
>>
>> Actually the hypervisor can't do that check, since it only sees the
>> truncated (32-bit) value. Question rather is why to retain a 64-bit
>> function parameter - I recently suggested switching it to 32 bits, and
>> I think this should definitely be done while anyway fiddling with the
>> interface.
>
>Ah, nr is correct in xc_set_mem_access() so this is just briding the gap
>between the old and new interfaces. THis would be solved by nuking this
>now pointless wrapper.

OK, I will nuke the wrappers.

Thanks,
Aravindh

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

[Aravindh Puthiyaparambil (aravindp)]

>> @@ -47,12 +47,59 @@ int xc_mem_access_disable(xc_interface *xch,
>domid_t domain_id)
>>                                  NULL);
>>  }
>>
>> +static int xc_mem_access_memop(xc_interface *xch, domid_t domid,
>> +                               xen_mem_access_op_t *mao)
>> +{
>> +    mao->domid = domid;
>> +
>> +    return do_memory_op(xch, XENMEM_access_op, mao, sizeof(*mao));
>
>I'm not sure what this wrapper is bringing to the table over just making
>the call in the callers. Also you inconsistently do and don't set
>mao->domid in the callers. If you want to keep the wrapper then I'd
>suggest making the callers set domid along with all the other fields and
>dropping the domid argument here.

The wrapper is indeed pointless. I will get rid of it.

Thanks,
Aravindh