All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
@ 2016-09-02  8:51 Razvan Cojocaru
  2016-09-02  8:53 ` Wei Liu
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Razvan Cojocaru @ 2016-09-02  8:51 UTC (permalink / raw)
  To: xen-devel
  Cc: tamas, wei.liu2, Razvan Cojocaru, george.dunlap, andrew.cooper3,
	ian.jackson, jbeulich

Currently it is only possible to set mem_access restrictions only for
a contiguous range of GFNs (or, as a particular case, for a single GFN).
This patch introduces a new libxc function taking an array of GFNs.
The alternative would be to set each page in turn, using a userspace-HV
roundtrip for each call, and triggering a TLB flush per page set.

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

---
Changes since V1 / RFC:
 - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
   and XENMEM_access_op_set_access_sparse to
   XENMEM_access_op_set_access_multi.
 - Renamed the 'nr' parameter to 'size'.
 - Wrapped long line in the implementation of xc_set_mem_access_multi().
 - Factored out common code in _p2m_set_mem_access() (and modified
   p2m_set_altp2m_mem_access() in the process, to take an unsigned
   long argument instead of a gfn_t).
 - Factored out xenmem_access_t to p2m_access_t conversion code in
   p2m_xenmem_access_to_p2m_access().
 - Added hypercall continuation support.
 - Added compat translation support.
 - No longer allocating buffers (now using copy_from_guest_offset()).
 - Added support for setting an array of access restrictions, as
   suggested by Tamas Lengyel.
 - This patch incorporates Jan Beulich's "memory: fix compat handling
   of XENMEM_access_op".
---
 tools/libxc/include/xenctrl.h |   4 ++
 tools/libxc/xc_mem_access.c   |  38 ++++++++++
 xen/arch/x86/mm/p2m.c         | 161 ++++++++++++++++++++++++++++++++----------
 xen/common/compat/memory.c    |  24 +++++--
 xen/common/mem_access.c       |  11 +++
 xen/include/public/memory.h   |  14 +++-
 xen/include/xen/p2m-common.h  |   6 ++
 xen/include/xlat.lst          |   2 +-
 8 files changed, 215 insertions(+), 45 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 560ce7b..c2f14a6 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2125,6 +2125,10 @@ int xc_set_mem_access(xc_interface *xch, domid_t domain_id,
                       xenmem_access_t access, uint64_t first_pfn,
                       uint32_t nr);
 
+int xc_set_mem_access_multi(xc_interface *xch, domid_t domain_id,
+                            uint8_t *access, uint64_t *pages,
+                            uint32_t size);
+
 /*
  * Gets the mem access for the given page (returned in access on success)
  */
diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
index eee088c..d69d4f9 100644
--- a/tools/libxc/xc_mem_access.c
+++ b/tools/libxc/xc_mem_access.c
@@ -41,6 +41,44 @@ int xc_set_mem_access(xc_interface *xch,
     return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
 }
 
+int xc_set_mem_access_multi(xc_interface *xch,
+                            domid_t domain_id,
+                            uint8_t *access,
+                            uint64_t *pages,
+                            uint32_t size)
+{
+    DECLARE_HYPERCALL_BOUNCE(access, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    DECLARE_HYPERCALL_BOUNCE(pages, size * sizeof(uint64_t),
+                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    int rc;
+
+    xen_mem_access_op_t mao =
+    {
+        .op       = XENMEM_access_op_set_access_multi,
+        .domid    = domain_id,
+        .access   = XENMEM_access_default + 1, /* Invalid value */
+        .pfn      = ~0UL, /* Invalid GFN */
+        .nr       = size,
+    };
+
+    if ( xc_hypercall_bounce_pre(xch, pages) ||
+         xc_hypercall_bounce_pre(xch, access) )
+    {
+        PERROR("Could not bounce memory for XENMEM_access_op_set_access_multi");
+        return -1;
+    }
+
+    set_xen_guest_handle(mao.pfn_list, pages);
+    set_xen_guest_handle(mao.access_list, access);
+
+    rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
+
+    xc_hypercall_bounce_post(xch, access);
+    xc_hypercall_bounce_post(xch, pages);
+
+    return rc;
+}
+
 int xc_get_mem_access(xc_interface *xch,
                       domid_t domain_id,
                       uint64_t pfn,
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 812dbf6..ee6438b 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -28,6 +28,7 @@
 #include <xen/event.h>
 #include <public/vm_event.h>
 #include <asm/domain.h>
+#include <xen/guest_access.h> /* copy_from_guest() */
 #include <asm/page.h>
 #include <asm/paging.h>
 #include <asm/p2m.h>
@@ -1772,13 +1773,12 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
 static inline
 int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
                               struct p2m_domain *ap2m, p2m_access_t a,
-                              gfn_t gfn)
+                              unsigned long gfn_l)
 {
     mfn_t mfn;
     p2m_type_t t;
     p2m_access_t old_a;
     unsigned int page_order;
-    unsigned long gfn_l = gfn_x(gfn);
     int rc;
 
     mfn = ap2m->get_entry(ap2m, gfn_l, &t, &old_a, 0, NULL, NULL);
@@ -1811,21 +1811,39 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
                          (current->domain != d));
 }
 
-/*
- * Set access type for a region of gfns.
- * If gfn == INVALID_GFN, sets the default access type.
- */
-long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
-                        uint32_t start, uint32_t mask, xenmem_access_t access,
-                        unsigned int altp2m_idx)
+static inline
+int _p2m_set_mem_access(struct domain *d, struct p2m_domain *p2m,
+                        struct p2m_domain *ap2m, p2m_access_t a,
+                        unsigned long gfn_l)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
-    p2m_access_t a, _a;
-    p2m_type_t t;
-    mfn_t mfn;
-    unsigned long gfn_l;
-    long rc = 0;
+    int rc;
 
+    if ( ap2m )
+    {
+        rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, gfn_l);
+        /* If the corresponding mfn is invalid we will want to just skip it */
+        if ( rc && rc != -ESRCH )
+            return rc;
+    }
+    else
+    {
+        mfn_t mfn;
+        p2m_access_t _a;
+        p2m_type_t t;
+
+        mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
+        rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
+        return rc;
+    }
+
+    return 0;
+}
+
+static inline
+int p2m_xenmem_access_to_p2m_access(struct p2m_domain *p2m,
+                                    xenmem_access_t xaccess,
+                                    p2m_access_t *paccess)
+{
     static const p2m_access_t memaccess[] = {
 #define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
         ACCESS(n),
@@ -1841,6 +1859,34 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
 #undef ACCESS
     };
 
+    switch ( xaccess )
+    {
+    case 0 ... ARRAY_SIZE(memaccess) - 1:
+        *paccess = memaccess[xaccess];
+        break;
+    case XENMEM_access_default:
+        *paccess = p2m->default_access;
+        break;
+    default:
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+/*
+ * Set access type for a region of gfns.
+ * If gfn == INVALID_GFN, sets the default access type.
+ */
+long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
+                        uint32_t start, uint32_t mask, xenmem_access_t access,
+                        unsigned int altp2m_idx)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
+    p2m_access_t a;
+    unsigned long gfn_l;
+    long rc = 0;
+
     /* altp2m view 0 is treated as the hostp2m */
     if ( altp2m_idx )
     {
@@ -1851,17 +1897,8 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
         ap2m = d->arch.altp2m_p2m[altp2m_idx];
     }
 
-    switch ( access )
-    {
-    case 0 ... ARRAY_SIZE(memaccess) - 1:
-        a = memaccess[access];
-        break;
-    case XENMEM_access_default:
-        a = p2m->default_access;
-        break;
-    default:
+    if ( p2m_xenmem_access_to_p2m_access(p2m, access, &a) )
         return -EINVAL;
-    }
 
     /* If request to set default access. */
     if ( gfn_eq(gfn, INVALID_GFN) )
@@ -1876,23 +1913,71 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
 
     for ( gfn_l = gfn_x(gfn) + start; nr > start; ++gfn_l )
     {
-        if ( ap2m )
-        {
-            rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, _gfn(gfn_l));
-            /* If the corresponding mfn is invalid we will just skip it */
-            if ( rc && rc != -ESRCH )
-                break;
-        }
-        else
+        rc = _p2m_set_mem_access(d, p2m, ap2m, a, gfn_l);
+
+        if ( rc )
+            break;
+
+        /* Check for continuation if it's not the last iteration. */
+        if ( nr > ++start && !(start & mask) && hypercall_preempt_check() )
         {
-            mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
-            rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
-            if ( rc )
-                break;
+            rc = start;
+            break;
         }
+    }
+
+    if ( ap2m )
+        p2m_unlock(ap2m);
+    p2m_unlock(p2m);
+
+    return rc;
+}
+
+long p2m_set_mem_access_multi(struct domain *d,
+                              XEN_GUEST_HANDLE(uint64_t) pfn_list,
+                              XEN_GUEST_HANDLE(uint8) access_list,
+                              uint32_t size, uint32_t start, uint32_t mask,
+                              unsigned int altp2m_idx)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
+    long rc = 0;
+
+    /* altp2m view 0 is treated as the hostp2m */
+    if ( altp2m_idx )
+    {
+        if ( altp2m_idx >= MAX_ALTP2M ||
+             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+            return -EINVAL;
+
+        ap2m = d->arch.altp2m_p2m[altp2m_idx];
+    }
+
+    p2m_lock(p2m);
+    if ( ap2m )
+        p2m_lock(ap2m);
+
+    while ( start < size )
+    {
+        p2m_access_t a;
+
+        uint8_t access;
+        uint64_t gfn_l;
+
+        copy_from_guest_offset(&gfn_l, pfn_list, start, 1);
+        copy_from_guest_offset(&access, access_list, start, 1);
+
+        rc = p2m_xenmem_access_to_p2m_access(p2m, access, &a);
+
+        if ( rc )
+            break;
+
+        rc = _p2m_set_mem_access(d, p2m, ap2m, a, gfn_l);
+
+        if ( rc )
+            break;
 
         /* Check for continuation if it's not the last iteration. */
-        if ( nr > ++start && !(start & mask) && hypercall_preempt_check() )
+        if ( size > ++start && !(start & mask) && hypercall_preempt_check() )
         {
             rc = start;
             break;
diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index 20c7671..e9732df 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -15,7 +15,6 @@ CHECK_TYPE(domid);
 #undef compat_domid_t
 #undef xen_domid_t
 
-CHECK_mem_access_op;
 CHECK_vmemrange;
 
 #ifdef CONFIG_HAS_PASSTHROUGH
@@ -71,6 +70,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
             struct xen_add_to_physmap_batch *atpb;
             struct xen_remove_from_physmap *xrfp;
             struct xen_vnuma_topology_info *vnuma;
+            struct xen_mem_access_op *mao;
         } nat;
         union {
             struct compat_memory_reservation rsrv;
@@ -78,6 +78,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
             struct compat_add_to_physmap atp;
             struct compat_add_to_physmap_batch atpb;
             struct compat_vnuma_topology_info vnuma;
+            struct compat_mem_access_op mao;
         } cmp;
 
         set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
@@ -320,6 +321,23 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
             break;
         }
 
+        case XENMEM_access_op:
+        {
+#define XLAT_mem_access_op_HNDL_pfn_list(_d_, _s_) \
+            guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
+#define XLAT_mem_access_op_HNDL_access_list(_d_, _s_) \
+            guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
+
+            XLAT_mem_access_op(nat.mao, &cmp.mao);
+
+#undef XLAT_mem_access_op_HNDL_pfn_list
+#undef XLAT_mem_access_op_HNDL_access_list
+
+            return mem_access_memop(cmd,
+                                    guest_handle_cast(compat,
+                                                      xen_mem_access_op_t));
+        }
+
         case XENMEM_get_vnumainfo:
         {
             enum XLAT_vnuma_topology_info_vdistance vdistance =
@@ -495,10 +513,6 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
             break;
         }
 
-        case XENMEM_access_op:
-            rc = mem_access_memop(cmd, guest_handle_cast(compat, xen_mem_access_op_t));
-            break;
-
         case XENMEM_add_to_physmap_batch:
             start_extent = end_extent;
             break;
diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index b4033f0..18b908f 100644
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -76,6 +76,17 @@ int mem_access_memop(unsigned long cmd,
         }
         break;
 
+    case XENMEM_access_op_set_access_multi:
+        rc = p2m_set_mem_access_multi(d, mao.pfn_list, mao.access_list, mao.nr,
+                                      start_iter, MEMOP_CMD_MASK, 0);
+        if ( rc > 0 )
+        {
+            ASSERT(!(rc & MEMOP_CMD_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;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 3badfb9..9cb2568 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -410,6 +410,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_paging_op_t);
  * #define XENMEM_access_op_enable_emulate     2
  * #define XENMEM_access_op_disable_emulate    3
  */
+#define XENMEM_access_op_set_access_multi   4
 
 typedef enum {
     XENMEM_access_n,
@@ -442,7 +443,8 @@ struct xen_mem_access_op {
     uint8_t access;
     domid_t domid;
     /*
-     * Number of pages for set op
+     * Number of pages for set op (or size of pfn_list for
+     * XENMEM_access_op_set_access_multi)
      * Ignored on setting default access and other ops
      */
     uint32_t nr;
@@ -452,6 +454,16 @@ struct xen_mem_access_op {
      * ~0ull is used to set and get the default access for pages
      */
     uint64_aligned_t pfn;
+    /*
+     * List of pfns to set access for
+     * Used only with XENMEM_access_op_set_access_multi
+     */
+    XEN_GUEST_HANDLE(uint64_t) pfn_list;
+    /*
+     * Corresponding list of access settings for pfn_list
+     * Used only with XENMEM_access_op_set_access_multi
+     */
+    XEN_GUEST_HANDLE(uint8) access_list;
 };
 typedef struct xen_mem_access_op xen_mem_access_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_mem_access_op_t);
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index b4f9077..384d2bf 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -53,6 +53,12 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
                         uint32_t start, uint32_t mask, xenmem_access_t access,
                         unsigned int altp2m_idx);
 
+long p2m_set_mem_access_multi(struct domain *d,
+                              XEN_GUEST_HANDLE(uint64_t) pfn_list,
+                              XEN_GUEST_HANDLE(uint8) access,
+                              uint32_t size, uint32_t start, uint32_t mask,
+                              unsigned int altp2m_idx);
+
 /*
  * Get access type for a gfn.
  * If gfn == INVALID_GFN, gets the default access type.
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 801a1c1..bdf1d05 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -68,7 +68,7 @@
 !	memory_exchange			memory.h
 !	memory_map			memory.h
 !	memory_reservation		memory.h
-?	mem_access_op			memory.h
+!	mem_access_op			memory.h
 !	pod_target			memory.h
 !	remove_from_physmap		memory.h
 !	reserved_device_memory_map	memory.h
-- 
1.9.1


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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02  8:51 [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi() Razvan Cojocaru
@ 2016-09-02  8:53 ` Wei Liu
  2016-09-02 10:02 ` Jan Beulich
  2016-09-02 13:17 ` Julien Grall
  2 siblings, 0 replies; 22+ messages in thread
From: Wei Liu @ 2016-09-02  8:53 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson,
	xen-devel, jbeulich

On Fri, Sep 02, 2016 at 11:51:06AM +0300, Razvan Cojocaru wrote:
> Currently it is only possible to set mem_access restrictions only for
> a contiguous range of GFNs (or, as a particular case, for a single GFN).
> This patch introduces a new libxc function taking an array of GFNs.
> The alternative would be to set each page in turn, using a userspace-HV
> roundtrip for each call, and triggering a TLB flush per page set.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> 
> ---
> Changes since V1 / RFC:
>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>    and XENMEM_access_op_set_access_sparse to
>    XENMEM_access_op_set_access_multi.
>  - Renamed the 'nr' parameter to 'size'.
>  - Wrapped long line in the implementation of xc_set_mem_access_multi().
>  - Factored out common code in _p2m_set_mem_access() (and modified
>    p2m_set_altp2m_mem_access() in the process, to take an unsigned
>    long argument instead of a gfn_t).
>  - Factored out xenmem_access_t to p2m_access_t conversion code in
>    p2m_xenmem_access_to_p2m_access().
>  - Added hypercall continuation support.
>  - Added compat translation support.
>  - No longer allocating buffers (now using copy_from_guest_offset()).
>  - Added support for setting an array of access restrictions, as
>    suggested by Tamas Lengyel.
>  - This patch incorporates Jan Beulich's "memory: fix compat handling
>    of XENMEM_access_op".
> ---
>  tools/libxc/include/xenctrl.h |   4 ++
>  tools/libxc/xc_mem_access.c   |  38 ++++++++++

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

FAOD I am happy with hv maintainers handle this patch.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02  8:51 [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi() Razvan Cojocaru
  2016-09-02  8:53 ` Wei Liu
@ 2016-09-02 10:02 ` Jan Beulich
  2016-09-02 10:34   ` Jan Beulich
                     ` (2 more replies)
  2016-09-02 13:17 ` Julien Grall
  2 siblings, 3 replies; 22+ messages in thread
From: Jan Beulich @ 2016-09-02 10:02 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

>>> On 02.09.16 at 10:51, <rcojocaru@bitdefender.com> wrote:
> Changes since V1 / RFC:
>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>    and XENMEM_access_op_set_access_sparse to
>    XENMEM_access_op_set_access_multi.
>  - Renamed the 'nr' parameter to 'size'.

Why?

>  - Wrapped long line in the implementation of xc_set_mem_access_multi().
>  - Factored out common code in _p2m_set_mem_access() (and modified
>    p2m_set_altp2m_mem_access() in the process, to take an unsigned
>    long argument instead of a gfn_t).
>  - Factored out xenmem_access_t to p2m_access_t conversion code in
>    p2m_xenmem_access_to_p2m_access().
>  - Added hypercall continuation support.
>  - Added compat translation support.
>  - No longer allocating buffers (now using copy_from_guest_offset()).
>  - Added support for setting an array of access restrictions, as
>    suggested by Tamas Lengyel.
>  - This patch incorporates Jan Beulich's "memory: fix compat handling
>    of XENMEM_access_op".

I'm not sure that's okay; I'd have preferred you make the other
patch a prereq (not the least because that one may want
backporting, while this one certainly won't). In no event should
such a bug fix go without mentioning in the commit message.

> @@ -1772,13 +1773,12 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>  static inline
>  int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>                                struct p2m_domain *ap2m, p2m_access_t a,
> -                              gfn_t gfn)
> +                              unsigned long gfn_l)
>  {

I'm not really happy about such a step backwards.

> @@ -1811,21 +1811,39 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>                           (current->domain != d));
>  }
>  
> -/*
> - * Set access type for a region of gfns.
> - * If gfn == INVALID_GFN, sets the default access type.
> - */
> -long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
> -                        uint32_t start, uint32_t mask, xenmem_access_t access,
> -                        unsigned int altp2m_idx)
> +static inline
> +int _p2m_set_mem_access(struct domain *d, struct p2m_domain *p2m,

Considering the file we're in, can't this just be set_mem_access()?

Also I'd leave the inlining decision completely to the compiler.

> +                        struct p2m_domain *ap2m, p2m_access_t a,
> +                        unsigned long gfn_l)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
> -    p2m_access_t a, _a;
> -    p2m_type_t t;
> -    mfn_t mfn;
> -    unsigned long gfn_l;
> -    long rc = 0;
> +    int rc;
>  
> +    if ( ap2m )
> +    {
> +        rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, gfn_l);
> +        /* If the corresponding mfn is invalid we will want to just skip it */
> +        if ( rc && rc != -ESRCH )
> +            return rc;

If you just converted -ESRCH to 0 here, you could then ...

> +    }
> +    else
> +    {
> +        mfn_t mfn;
> +        p2m_access_t _a;
> +        p2m_type_t t;
> +
> +        mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
> +        rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
> +        return rc;

... ditch this extra return path and ...

> +    }
> +
> +    return 0;

... have both cases come here.

> @@ -1851,17 +1897,8 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>          ap2m = d->arch.altp2m_p2m[altp2m_idx];
>      }
>  
> -    switch ( access )
> -    {
> -    case 0 ... ARRAY_SIZE(memaccess) - 1:
> -        a = memaccess[access];
> -        break;
> -    case XENMEM_access_default:
> -        a = p2m->default_access;
> -        break;
> -    default:
> +    if ( p2m_xenmem_access_to_p2m_access(p2m, access, &a) )
>          return -EINVAL;

Either you make the helper function return bool, or you pass on
whatever value it returned instead of assuming it's -EINVAL.

> +long p2m_set_mem_access_multi(struct domain *d,
> +                              XEN_GUEST_HANDLE(uint64_t) pfn_list,
> +                              XEN_GUEST_HANDLE(uint8) access_list,
> +                              uint32_t size, uint32_t start, uint32_t mask,
> +                              unsigned int altp2m_idx)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
> +    long rc = 0;
> +
> +    /* altp2m view 0 is treated as the hostp2m */
> +    if ( altp2m_idx )
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
> +
> +    p2m_lock(p2m);
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +    while ( start < size )
> +    {
> +        p2m_access_t a;
> +
> +        uint8_t access;
> +        uint64_t gfn_l;

Stray blank line between declarations.

> @@ -320,6 +321,23 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
>              break;
>          }
>  
> +        case XENMEM_access_op:
> +        {
> +#define XLAT_mem_access_op_HNDL_pfn_list(_d_, _s_) \
> +            guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
> +#define XLAT_mem_access_op_HNDL_access_list(_d_, _s_) \
> +            guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
> +
> +            XLAT_mem_access_op(nat.mao, &cmp.mao);
> +
> +#undef XLAT_mem_access_op_HNDL_pfn_list
> +#undef XLAT_mem_access_op_HNDL_access_list
> +
> +            return mem_access_memop(cmd,
> +                                    guest_handle_cast(compat,
> +                                                      xen_mem_access_op_t));
> +        }

You translate into native format, but then cast the compat handle
to its native counterpart type. Such casting is okay only when
accompanied by a respectively invoked CHECK_* macro. Please
follow the model other code here uses.

And reviewing this I notice that my earlier bug fix also is still not
really correct: It causes hypercall_xlat_continuation() to be
bypassed.

> @@ -452,6 +454,16 @@ struct xen_mem_access_op {
>       * ~0ull is used to set and get the default access for pages
>       */
>      uint64_aligned_t pfn;
> +    /*
> +     * List of pfns to set access for
> +     * Used only with XENMEM_access_op_set_access_multi
> +     */
> +    XEN_GUEST_HANDLE(uint64_t) pfn_list;

I'm not sure why we even have this kind of handle - please use
XEN_GUEST_HANDLE(uint64) instead.

> +    /*
> +     * Corresponding list of access settings for pfn_list
> +     * Used only with XENMEM_access_op_set_access_multi
> +     */
> +    XEN_GUEST_HANDLE(uint8) access_list;

And for both of them - I don't think the arrays are meant to be
used for output? In which case they should be handles of const
types.

Jan

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 10:02 ` Jan Beulich
@ 2016-09-02 10:34   ` Jan Beulich
  2016-09-02 11:18   ` Razvan Cojocaru
  2016-09-02 12:21   ` Razvan Cojocaru
  2 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2016-09-02 10:34 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

>>> On 02.09.16 at 12:02, <JBeulich@suse.com> wrote:
>>>> On 02.09.16 at 10:51, <rcojocaru@bitdefender.com> wrote:
>> @@ -320,6 +321,23 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
>>              break;
>>          }
>>  
>> +        case XENMEM_access_op:
>> +        {
>> +#define XLAT_mem_access_op_HNDL_pfn_list(_d_, _s_) \
>> +            guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
>> +#define XLAT_mem_access_op_HNDL_access_list(_d_, _s_) \
>> +            guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
>> +
>> +            XLAT_mem_access_op(nat.mao, &cmp.mao);
>> +
>> +#undef XLAT_mem_access_op_HNDL_pfn_list
>> +#undef XLAT_mem_access_op_HNDL_access_list
>> +
>> +            return mem_access_memop(cmd,
>> +                                    guest_handle_cast(compat,
>> +                                                      xen_mem_access_op_t));
>> +        }
> 
> You translate into native format, but then cast the compat handle
> to its native counterpart type. Such casting is okay only when
> accompanied by a respectively invoked CHECK_* macro. Please
> follow the model other code here uses.
> 
> And reviewing this I notice that my earlier bug fix also is still not
> really correct: It causes hypercall_xlat_continuation() to be
> bypassed.

Actually no, that patch is fine because it (legitimately) uses a cast.
You would introduce the bypassing problem here as soon as you
properly handed the native handle to mem_access_memop().

Jan


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 10:02 ` Jan Beulich
  2016-09-02 10:34   ` Jan Beulich
@ 2016-09-02 11:18   ` Razvan Cojocaru
  2016-09-02 11:45     ` Jan Beulich
  2016-09-02 12:21   ` Razvan Cojocaru
  2 siblings, 1 reply; 22+ messages in thread
From: Razvan Cojocaru @ 2016-09-02 11:18 UTC (permalink / raw)
  To: Jan Beulich
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

On 09/02/2016 01:02 PM, Jan Beulich wrote:
>>>> On 02.09.16 at 10:51, <rcojocaru@bitdefender.com> wrote:
>> Changes since V1 / RFC:
>>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>>    and XENMEM_access_op_set_access_sparse to
>>    XENMEM_access_op_set_access_multi.
>>  - Renamed the 'nr' parameter to 'size'.
> 
> Why?

Tamas suggested it, size sounded more intuitive to him. I have no
problem with either nr or size.

>>  - Wrapped long line in the implementation of xc_set_mem_access_multi().
>>  - Factored out common code in _p2m_set_mem_access() (and modified
>>    p2m_set_altp2m_mem_access() in the process, to take an unsigned
>>    long argument instead of a gfn_t).
>>  - Factored out xenmem_access_t to p2m_access_t conversion code in
>>    p2m_xenmem_access_to_p2m_access().
>>  - Added hypercall continuation support.
>>  - Added compat translation support.
>>  - No longer allocating buffers (now using copy_from_guest_offset()).
>>  - Added support for setting an array of access restrictions, as
>>    suggested by Tamas Lengyel.
>>  - This patch incorporates Jan Beulich's "memory: fix compat handling
>>    of XENMEM_access_op".
> 
> I'm not sure that's okay; I'd have preferred you make the other
> patch a prereq (not the least because that one may want
> backporting, while this one certainly won't). In no event should
> such a bug fix go without mentioning in the commit message.

I agree, this is here simply because looking at the state of staging,
the patch hadn't yet made it in, and I was trying to be efficient and
have an intermediate review anyway. I'll manually apply your patch
before mine and remove that code from this patch.

>> @@ -1772,13 +1773,12 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>  static inline
>>  int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>                                struct p2m_domain *ap2m, p2m_access_t a,
>> -                              gfn_t gfn)
>> +                              unsigned long gfn_l)
>>  {
> 
> I'm not really happy about such a step backwards.

I'll keep p2m_set_altp2m_mem_access() as it is then. I just thought that
going from unsigned long to gfn_t just to have to go back to unsigned
long slightly clutters up the code, but it's certainly not a huge
difference.

>> @@ -1811,21 +1811,39 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>                           (current->domain != d));
>>  }
>>  
>> -/*
>> - * Set access type for a region of gfns.
>> - * If gfn == INVALID_GFN, sets the default access type.
>> - */
>> -long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>> -                        uint32_t start, uint32_t mask, xenmem_access_t access,
>> -                        unsigned int altp2m_idx)
>> +static inline
>> +int _p2m_set_mem_access(struct domain *d, struct p2m_domain *p2m,
> 
> Considering the file we're in, can't this just be set_mem_access()?

Of course, I'll rename it.

> Also I'd leave the inlining decision completely to the compiler.

I took that cue from p2m_set_altp2m_mem_access() above. I'll remove the
inline.

>> +                        struct p2m_domain *ap2m, p2m_access_t a,
>> +                        unsigned long gfn_l)
>>  {
>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
>> -    p2m_access_t a, _a;
>> -    p2m_type_t t;
>> -    mfn_t mfn;
>> -    unsigned long gfn_l;
>> -    long rc = 0;
>> +    int rc;
>>  
>> +    if ( ap2m )
>> +    {
>> +        rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, gfn_l);
>> +        /* If the corresponding mfn is invalid we will want to just skip it */
>> +        if ( rc && rc != -ESRCH )
>> +            return rc;
> 
> If you just converted -ESRCH to 0 here, you could then ...
> 
>> +    }
>> +    else
>> +    {
>> +        mfn_t mfn;
>> +        p2m_access_t _a;
>> +        p2m_type_t t;
>> +
>> +        mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
>> +        rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
>> +        return rc;
> 
> ... ditch this extra return path and ...
> 
>> +    }
>> +
>> +    return 0;
> 
> ... have both cases come here.

I'll do that.

>> @@ -1851,17 +1897,8 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>>          ap2m = d->arch.altp2m_p2m[altp2m_idx];
>>      }
>>  
>> -    switch ( access )
>> -    {
>> -    case 0 ... ARRAY_SIZE(memaccess) - 1:
>> -        a = memaccess[access];
>> -        break;
>> -    case XENMEM_access_default:
>> -        a = p2m->default_access;
>> -        break;
>> -    default:
>> +    if ( p2m_xenmem_access_to_p2m_access(p2m, access, &a) )
>>          return -EINVAL;
> 
> Either you make the helper function return bool, or you pass on
> whatever value it returned instead of assuming it's -EINVAL.

Fair enough, I'll return bool.

>> +long p2m_set_mem_access_multi(struct domain *d,
>> +                              XEN_GUEST_HANDLE(uint64_t) pfn_list,
>> +                              XEN_GUEST_HANDLE(uint8) access_list,
>> +                              uint32_t size, uint32_t start, uint32_t mask,
>> +                              unsigned int altp2m_idx)
>> +{
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
>> +    long rc = 0;
>> +
>> +    /* altp2m view 0 is treated as the hostp2m */
>> +    if ( altp2m_idx )
>> +    {
>> +        if ( altp2m_idx >= MAX_ALTP2M ||
>> +             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>> +            return -EINVAL;
>> +
>> +        ap2m = d->arch.altp2m_p2m[altp2m_idx];
>> +    }
>> +
>> +    p2m_lock(p2m);
>> +    if ( ap2m )
>> +        p2m_lock(ap2m);
>> +
>> +    while ( start < size )
>> +    {
>> +        p2m_access_t a;
>> +
>> +        uint8_t access;
>> +        uint64_t gfn_l;
> 
> Stray blank line between declarations.

Will remove it.

>> @@ -320,6 +321,23 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
>>              break;
>>          }
>>  
>> +        case XENMEM_access_op:
>> +        {
>> +#define XLAT_mem_access_op_HNDL_pfn_list(_d_, _s_) \
>> +            guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
>> +#define XLAT_mem_access_op_HNDL_access_list(_d_, _s_) \
>> +            guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
>> +
>> +            XLAT_mem_access_op(nat.mao, &cmp.mao);
>> +
>> +#undef XLAT_mem_access_op_HNDL_pfn_list
>> +#undef XLAT_mem_access_op_HNDL_access_list
>> +
>> +            return mem_access_memop(cmd,
>> +                                    guest_handle_cast(compat,
>> +                                                      xen_mem_access_op_t));
>> +        }
> 
> You translate into native format, but then cast the compat handle
> to its native counterpart type. Such casting is okay only when
> accompanied by a respectively invoked CHECK_* macro. Please
> follow the model other code here uses.

I'll follow the model.

> And reviewing this I notice that my earlier bug fix also is still not
> really correct: It causes hypercall_xlat_continuation() to be
> bypassed.

Since you've addressed this comment in a subsequent reply, no need to
continue here.

>> @@ -452,6 +454,16 @@ struct xen_mem_access_op {
>>       * ~0ull is used to set and get the default access for pages
>>       */
>>      uint64_aligned_t pfn;
>> +    /*
>> +     * List of pfns to set access for
>> +     * Used only with XENMEM_access_op_set_access_multi
>> +     */
>> +    XEN_GUEST_HANDLE(uint64_t) pfn_list;
> 
> I'm not sure why we even have this kind of handle - please use
> XEN_GUEST_HANDLE(uint64) instead.

I'll change it.

>> +    /*
>> +     * Corresponding list of access settings for pfn_list
>> +     * Used only with XENMEM_access_op_set_access_multi
>> +     */
>> +    XEN_GUEST_HANDLE(uint8) access_list;
> 
> And for both of them - I don't think the arrays are meant to be
> used for output? In which case they should be handles of const
> types.

No, they're indeed not meant for output. I'll constify the code.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 11:18   ` Razvan Cojocaru
@ 2016-09-02 11:45     ` Jan Beulich
  2016-09-02 14:50       ` Tamas K Lengyel
  0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2016-09-02 11:45 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

>>> On 02.09.16 at 13:18, <rcojocaru@bitdefender.com> wrote:
> On 09/02/2016 01:02 PM, Jan Beulich wrote:
>>>>> On 02.09.16 at 10:51, <rcojocaru@bitdefender.com> wrote:
>>> Changes since V1 / RFC:
>>>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>>>    and XENMEM_access_op_set_access_sparse to
>>>    XENMEM_access_op_set_access_multi.
>>>  - Renamed the 'nr' parameter to 'size'.
>> 
>> Why?
> 
> Tamas suggested it, size sounded more intuitive to him. I have no
> problem with either nr or size.

Size to me means something in bytes, which clearly isn't the case
here. There's not even support for other then 4k pages so far.

Jan


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 10:02 ` Jan Beulich
  2016-09-02 10:34   ` Jan Beulich
  2016-09-02 11:18   ` Razvan Cojocaru
@ 2016-09-02 12:21   ` Razvan Cojocaru
  2016-09-02 12:33     ` Jan Beulich
  2 siblings, 1 reply; 22+ messages in thread
From: Razvan Cojocaru @ 2016-09-02 12:21 UTC (permalink / raw)
  To: Jan Beulich
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

On 09/02/2016 01:02 PM, Jan Beulich wrote:
>> +    /*
>> > +     * Corresponding list of access settings for pfn_list
>> > +     * Used only with XENMEM_access_op_set_access_multi
>> > +     */
>> > +    XEN_GUEST_HANDLE(uint8) access_list;
> And for both of them - I don't think the arrays are meant to be
> used for output? In which case they should be handles of const
> types.

Actually I can't seem to be able to find a magic combination that works.

XEN_GUEST_HANDLE(const uint8) access_list; tells me:

./public/arch-x86/xen.h:53:41: error: '__guest_handle_const' does not
name a type
 #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name

which is fair. I've tried:

XEN_GUEST_HANDLE(const_uint8) access_list;

which does go further with the compilation process, but still kills it with:

xen/include/compat/memory.h:154:5: error: unknown type name
'__compat_handle_const_uint8'
     COMPAT_HANDLE(const_uint8) access_list;

What would be the appropriate const type to use here?


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 12:21   ` Razvan Cojocaru
@ 2016-09-02 12:33     ` Jan Beulich
  2016-09-02 12:41       ` Razvan Cojocaru
  0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2016-09-02 12:33 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

>>> On 02.09.16 at 14:21, <rcojocaru@bitdefender.com> wrote:
> On 09/02/2016 01:02 PM, Jan Beulich wrote:
>>> +    /*
>>> > +     * Corresponding list of access settings for pfn_list
>>> > +     * Used only with XENMEM_access_op_set_access_multi
>>> > +     */
>>> > +    XEN_GUEST_HANDLE(uint8) access_list;
>> And for both of them - I don't think the arrays are meant to be
>> used for output? In which case they should be handles of const
>> types.
> 
> Actually I can't seem to be able to find a magic combination that works.
> 
> XEN_GUEST_HANDLE(const uint8) access_list; tells me:
> 
> ./public/arch-x86/xen.h:53:41: error: '__guest_handle_const' does not
> name a type
>  #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
> 
> which is fair. I've tried:
> 
> XEN_GUEST_HANDLE(const_uint8) access_list;
> 
> which does go further with the compilation process, but still kills it with:
> 
> xen/include/compat/memory.h:154:5: error: unknown type name
> '__compat_handle_const_uint8'
>      COMPAT_HANDLE(const_uint8) access_list;
> 
> What would be the appropriate const type to use here?

This one. Did you check that handle actually gets created? Per
my brief checking it doesn't look so. And neither the native one.

Jan


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 12:33     ` Jan Beulich
@ 2016-09-02 12:41       ` Razvan Cojocaru
  2016-09-02 12:56         ` Jan Beulich
  0 siblings, 1 reply; 22+ messages in thread
From: Razvan Cojocaru @ 2016-09-02 12:41 UTC (permalink / raw)
  To: Jan Beulich
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

On 09/02/2016 03:33 PM, Jan Beulich wrote:
>>>> On 02.09.16 at 14:21, <rcojocaru@bitdefender.com> wrote:
>> On 09/02/2016 01:02 PM, Jan Beulich wrote:
>>>> +    /*
>>>>> +     * Corresponding list of access settings for pfn_list
>>>>> +     * Used only with XENMEM_access_op_set_access_multi
>>>>> +     */
>>>>> +    XEN_GUEST_HANDLE(uint8) access_list;
>>> And for both of them - I don't think the arrays are meant to be
>>> used for output? In which case they should be handles of const
>>> types.
>>
>> Actually I can't seem to be able to find a magic combination that works.
>>
>> XEN_GUEST_HANDLE(const uint8) access_list; tells me:
>>
>> ./public/arch-x86/xen.h:53:41: error: '__guest_handle_const' does not
>> name a type
>>  #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
>>
>> which is fair. I've tried:
>>
>> XEN_GUEST_HANDLE(const_uint8) access_list;
>>
>> which does go further with the compilation process, but still kills it with:
>>
>> xen/include/compat/memory.h:154:5: error: unknown type name
>> '__compat_handle_const_uint8'
>>      COMPAT_HANDLE(const_uint8) access_list;
>>
>> What would be the appropriate const type to use here?
> 
> This one. Did you check that handle actually gets created? Per
> my brief checking it doesn't look so. And neither the native one.

Running ./configure again, followed by 'make clean' and a new 'make
dist' didn't help, so if it's supposed to be generated automatically it
doesn't seem to be (or I'm doing something wrong). I'll investigate more.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 12:41       ` Razvan Cojocaru
@ 2016-09-02 12:56         ` Jan Beulich
  2016-09-02 13:03           ` Razvan Cojocaru
  0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2016-09-02 12:56 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

>>> On 02.09.16 at 14:41, <rcojocaru@bitdefender.com> wrote:
> On 09/02/2016 03:33 PM, Jan Beulich wrote:
>>>>> On 02.09.16 at 14:21, <rcojocaru@bitdefender.com> wrote:
>>> On 09/02/2016 01:02 PM, Jan Beulich wrote:
>>>>> +    /*
>>>>>> +     * Corresponding list of access settings for pfn_list
>>>>>> +     * Used only with XENMEM_access_op_set_access_multi
>>>>>> +     */
>>>>>> +    XEN_GUEST_HANDLE(uint8) access_list;
>>>> And for both of them - I don't think the arrays are meant to be
>>>> used for output? In which case they should be handles of const
>>>> types.
>>>
>>> Actually I can't seem to be able to find a magic combination that works.
>>>
>>> XEN_GUEST_HANDLE(const uint8) access_list; tells me:
>>>
>>> ./public/arch-x86/xen.h:53:41: error: '__guest_handle_const' does not
>>> name a type
>>>  #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
>>>
>>> which is fair. I've tried:
>>>
>>> XEN_GUEST_HANDLE(const_uint8) access_list;
>>>
>>> which does go further with the compilation process, but still kills it with:
>>>
>>> xen/include/compat/memory.h:154:5: error: unknown type name
>>> '__compat_handle_const_uint8'
>>>      COMPAT_HANDLE(const_uint8) access_list;
>>>
>>> What would be the appropriate const type to use here?
>> 
>> This one. Did you check that handle actually gets created? Per
>> my brief checking it doesn't look so. And neither the native one.
> 
> Running ./configure again, followed by 'make clean' and a new 'make
> dist' didn't help, so if it's supposed to be generated automatically it
> doesn't seem to be (or I'm doing something wrong). I'll investigate more.

The compat one is supposed to get auto-generated once the native
one is there.

Jan


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 12:56         ` Jan Beulich
@ 2016-09-02 13:03           ` Razvan Cojocaru
  2016-09-02 13:10             ` Jan Beulich
  0 siblings, 1 reply; 22+ messages in thread
From: Razvan Cojocaru @ 2016-09-02 13:03 UTC (permalink / raw)
  To: Jan Beulich
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

On 09/02/2016 03:56 PM, Jan Beulich wrote:
>>>> On 02.09.16 at 14:41, <rcojocaru@bitdefender.com> wrote:
>> On 09/02/2016 03:33 PM, Jan Beulich wrote:
>>>>>> On 02.09.16 at 14:21, <rcojocaru@bitdefender.com> wrote:
>>>> On 09/02/2016 01:02 PM, Jan Beulich wrote:
>>>>>> +    /*
>>>>>>> +     * Corresponding list of access settings for pfn_list
>>>>>>> +     * Used only with XENMEM_access_op_set_access_multi
>>>>>>> +     */
>>>>>>> +    XEN_GUEST_HANDLE(uint8) access_list;
>>>>> And for both of them - I don't think the arrays are meant to be
>>>>> used for output? In which case they should be handles of const
>>>>> types.
>>>>
>>>> Actually I can't seem to be able to find a magic combination that works.
>>>>
>>>> XEN_GUEST_HANDLE(const uint8) access_list; tells me:
>>>>
>>>> ./public/arch-x86/xen.h:53:41: error: '__guest_handle_const' does not
>>>> name a type
>>>>  #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
>>>>
>>>> which is fair. I've tried:
>>>>
>>>> XEN_GUEST_HANDLE(const_uint8) access_list;
>>>>
>>>> which does go further with the compilation process, but still kills it with:
>>>>
>>>> xen/include/compat/memory.h:154:5: error: unknown type name
>>>> '__compat_handle_const_uint8'
>>>>      COMPAT_HANDLE(const_uint8) access_list;
>>>>
>>>> What would be the appropriate const type to use here?
>>>
>>> This one. Did you check that handle actually gets created? Per
>>> my brief checking it doesn't look so. And neither the native one.
>>
>> Running ./configure again, followed by 'make clean' and a new 'make
>> dist' didn't help, so if it's supposed to be generated automatically it
>> doesn't seem to be (or I'm doing something wrong). I'll investigate more.
> 
> The compat one is supposed to get auto-generated once the native
> one is there.

Changing both handles to XEN_GUEST_HANLDE(const_void) compiles cleanly.
As soon as I change to either XEN_GUEST_HANLDE(const_uint8) or
XEN_GUEST_HANLDE(const_uint64) I start getting errors.

Previously the auto-generation seems to have worked fine, so this is
likely something more subtle.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 13:03           ` Razvan Cojocaru
@ 2016-09-02 13:10             ` Jan Beulich
  2016-09-02 13:27               ` Razvan Cojocaru
  0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2016-09-02 13:10 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

>>> On 02.09.16 at 15:03, <rcojocaru@bitdefender.com> wrote:
> On 09/02/2016 03:56 PM, Jan Beulich wrote:
>>>>> On 02.09.16 at 14:41, <rcojocaru@bitdefender.com> wrote:
>>> On 09/02/2016 03:33 PM, Jan Beulich wrote:
>>>>>>> On 02.09.16 at 14:21, <rcojocaru@bitdefender.com> wrote:
>>>>> On 09/02/2016 01:02 PM, Jan Beulich wrote:
>>>>>>> +    /*
>>>>>>>> +     * Corresponding list of access settings for pfn_list
>>>>>>>> +     * Used only with XENMEM_access_op_set_access_multi
>>>>>>>> +     */
>>>>>>>> +    XEN_GUEST_HANDLE(uint8) access_list;
>>>>>> And for both of them - I don't think the arrays are meant to be
>>>>>> used for output? In which case they should be handles of const
>>>>>> types.
>>>>>
>>>>> Actually I can't seem to be able to find a magic combination that works.
>>>>>
>>>>> XEN_GUEST_HANDLE(const uint8) access_list; tells me:
>>>>>
>>>>> ./public/arch-x86/xen.h:53:41: error: '__guest_handle_const' does not
>>>>> name a type
>>>>>  #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
>>>>>
>>>>> which is fair. I've tried:
>>>>>
>>>>> XEN_GUEST_HANDLE(const_uint8) access_list;
>>>>>
>>>>> which does go further with the compilation process, but still kills it with:
>>>>>
>>>>> xen/include/compat/memory.h:154:5: error: unknown type name
>>>>> '__compat_handle_const_uint8'
>>>>>      COMPAT_HANDLE(const_uint8) access_list;
>>>>>
>>>>> What would be the appropriate const type to use here?
>>>>
>>>> This one. Did you check that handle actually gets created? Per
>>>> my brief checking it doesn't look so. And neither the native one.
>>>
>>> Running ./configure again, followed by 'make clean' and a new 'make
>>> dist' didn't help, so if it's supposed to be generated automatically it
>>> doesn't seem to be (or I'm doing something wrong). I'll investigate more.
>> 
>> The compat one is supposed to get auto-generated once the native
>> one is there.
> 
> Changing both handles to XEN_GUEST_HANLDE(const_void) compiles cleanly.
> As soon as I change to either XEN_GUEST_HANLDE(const_uint8) or
> XEN_GUEST_HANLDE(const_uint64) I start getting errors.
> 
> Previously the auto-generation seems to have worked fine, so this is
> likely something more subtle.

Oh, I see where the issue is: Both DEFINE_XEN_GUEST_HANDLE()
and __DEFINE_XEN_GUEST_HANDLE() auto-magically produce const
counterparts, but while DEFINE_COMPAT_HANDLE() does too,
__DEFINE_COMPAT_HANDLE() doesn't. That'll need fixing, I think.

Jan


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02  8:51 [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi() Razvan Cojocaru
  2016-09-02  8:53 ` Wei Liu
  2016-09-02 10:02 ` Jan Beulich
@ 2016-09-02 13:17 ` Julien Grall
  2016-09-02 13:22   ` Razvan Cojocaru
  2 siblings, 1 reply; 22+ messages in thread
From: Julien Grall @ 2016-09-02 13:17 UTC (permalink / raw)
  To: Razvan Cojocaru, xen-devel
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, jbeulich

Hello Razvan,

On 02/09/16 09:51, Razvan Cojocaru wrote:
> Currently it is only possible to set mem_access restrictions only for
> a contiguous range of GFNs (or, as a particular case, for a single GFN).
> This patch introduces a new libxc function taking an array of GFNs.
> The alternative would be to set each page in turn, using a userspace-HV
> roundtrip for each call, and triggering a TLB flush per page set.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>
> ---
> Changes since V1 / RFC:
>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>    and XENMEM_access_op_set_access_sparse to
>    XENMEM_access_op_set_access_multi.
>  - Renamed the 'nr' parameter to 'size'.
>  - Wrapped long line in the implementation of xc_set_mem_access_multi().
>  - Factored out common code in _p2m_set_mem_access() (and modified
>    p2m_set_altp2m_mem_access() in the process, to take an unsigned
>    long argument instead of a gfn_t).
>  - Factored out xenmem_access_t to p2m_access_t conversion code in
>    p2m_xenmem_access_to_p2m_access().
>  - Added hypercall continuation support.
>  - Added compat translation support.
>  - No longer allocating buffers (now using copy_from_guest_offset()).
>  - Added support for setting an array of access restrictions, as
>    suggested by Tamas Lengyel.
>  - This patch incorporates Jan Beulich's "memory: fix compat handling
>    of XENMEM_access_op".
> ---
>  tools/libxc/include/xenctrl.h |   4 ++
>  tools/libxc/xc_mem_access.c   |  38 ++++++++++
>  xen/arch/x86/mm/p2m.c         | 161 ++++++++++++++++++++++++++++++++----------
>  xen/common/compat/memory.c    |  24 +++++--
>  xen/common/mem_access.c       |  11 +++
>  xen/include/public/memory.h   |  14 +++-
>  xen/include/xen/p2m-common.h  |   6 ++

p2m-common.h contains prototype common between ARM and x86. I would have 
expected to see a change in the ARM port because of that.

Regards,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 13:17 ` Julien Grall
@ 2016-09-02 13:22   ` Razvan Cojocaru
  2016-09-02 13:26     ` Julien Grall
  0 siblings, 1 reply; 22+ messages in thread
From: Razvan Cojocaru @ 2016-09-02 13:22 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, jbeulich

On 09/02/2016 04:17 PM, Julien Grall wrote:
> Hello Razvan,

Hello Julien, thanks for the email!

> On 02/09/16 09:51, Razvan Cojocaru wrote:
>> Currently it is only possible to set mem_access restrictions only for
>> a contiguous range of GFNs (or, as a particular case, for a single GFN).
>> This patch introduces a new libxc function taking an array of GFNs.
>> The alternative would be to set each page in turn, using a userspace-HV
>> roundtrip for each call, and triggering a TLB flush per page set.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>
>> ---
>> Changes since V1 / RFC:
>>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>>    and XENMEM_access_op_set_access_sparse to
>>    XENMEM_access_op_set_access_multi.
>>  - Renamed the 'nr' parameter to 'size'.
>>  - Wrapped long line in the implementation of xc_set_mem_access_multi().
>>  - Factored out common code in _p2m_set_mem_access() (and modified
>>    p2m_set_altp2m_mem_access() in the process, to take an unsigned
>>    long argument instead of a gfn_t).
>>  - Factored out xenmem_access_t to p2m_access_t conversion code in
>>    p2m_xenmem_access_to_p2m_access().
>>  - Added hypercall continuation support.
>>  - Added compat translation support.
>>  - No longer allocating buffers (now using copy_from_guest_offset()).
>>  - Added support for setting an array of access restrictions, as
>>    suggested by Tamas Lengyel.
>>  - This patch incorporates Jan Beulich's "memory: fix compat handling
>>    of XENMEM_access_op".
>> ---
>>  tools/libxc/include/xenctrl.h |   4 ++
>>  tools/libxc/xc_mem_access.c   |  38 ++++++++++
>>  xen/arch/x86/mm/p2m.c         | 161
>> ++++++++++++++++++++++++++++++++----------
>>  xen/common/compat/memory.c    |  24 +++++--
>>  xen/common/mem_access.c       |  11 +++
>>  xen/include/public/memory.h   |  14 +++-
>>  xen/include/xen/p2m-common.h  |   6 ++
> 
> p2m-common.h contains prototype common between ARM and x86. I would have
> expected to see a change in the ARM port because of that.

The only change to p2m-common.h is the addition of a single function,
p2m_set_mem_access_multi(). As long as the ARM bits don't make use of
it, there's nothing to modify there.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 13:22   ` Razvan Cojocaru
@ 2016-09-02 13:26     ` Julien Grall
  2016-09-02 13:38       ` Razvan Cojocaru
  0 siblings, 1 reply; 22+ messages in thread
From: Julien Grall @ 2016-09-02 13:26 UTC (permalink / raw)
  To: Razvan Cojocaru, xen-devel
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, jbeulich



On 02/09/16 14:22, Razvan Cojocaru wrote:
> On 09/02/2016 04:17 PM, Julien Grall wrote:
>> Hello Razvan,
> 
> Hello Julien, thanks for the email!
> 
>> On 02/09/16 09:51, Razvan Cojocaru wrote:
>>> Currently it is only possible to set mem_access restrictions only for
>>> a contiguous range of GFNs (or, as a particular case, for a single GFN).
>>> This patch introduces a new libxc function taking an array of GFNs.
>>> The alternative would be to set each page in turn, using a userspace-HV
>>> roundtrip for each call, and triggering a TLB flush per page set.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>
>>> ---
>>> Changes since V1 / RFC:
>>>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>>>    and XENMEM_access_op_set_access_sparse to
>>>    XENMEM_access_op_set_access_multi.
>>>  - Renamed the 'nr' parameter to 'size'.
>>>  - Wrapped long line in the implementation of xc_set_mem_access_multi().
>>>  - Factored out common code in _p2m_set_mem_access() (and modified
>>>    p2m_set_altp2m_mem_access() in the process, to take an unsigned
>>>    long argument instead of a gfn_t).
>>>  - Factored out xenmem_access_t to p2m_access_t conversion code in
>>>    p2m_xenmem_access_to_p2m_access().
>>>  - Added hypercall continuation support.
>>>  - Added compat translation support.
>>>  - No longer allocating buffers (now using copy_from_guest_offset()).
>>>  - Added support for setting an array of access restrictions, as
>>>    suggested by Tamas Lengyel.
>>>  - This patch incorporates Jan Beulich's "memory: fix compat handling
>>>    of XENMEM_access_op".
>>> ---
>>>  tools/libxc/include/xenctrl.h |   4 ++
>>>  tools/libxc/xc_mem_access.c   |  38 ++++++++++
>>>  xen/arch/x86/mm/p2m.c         | 161
>>> ++++++++++++++++++++++++++++++++----------
>>>  xen/common/compat/memory.c    |  24 +++++--
>>>  xen/common/mem_access.c       |  11 +++
>>>  xen/include/public/memory.h   |  14 +++-
>>>  xen/include/xen/p2m-common.h  |   6 ++
>>
>> p2m-common.h contains prototype common between ARM and x86. I would have
>> expected to see a change in the ARM port because of that.
> 
> The only change to p2m-common.h is the addition of a single function,
> p2m_set_mem_access_multi(). As long as the ARM bits don't make use of
> it, there's nothing to modify there.

p2m_set_mem_access_multi is called from common code. If you had tried
to build Xen on ARM you would have noticed a compilation error:

prelink.o: In function `mem_access_memop':
/local/home/julieng/works/xen/xen/common/mem_access.c:80: undefined reference to `p2m_set_mem_access_multi'
aarch64-linux-gnu-ld: /local/home/julieng/works/xen/xen/.xen-syms.0: hidden symbol `p2m_set_mem_access_multi' isn't defined

Regards,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 13:10             ` Jan Beulich
@ 2016-09-02 13:27               ` Razvan Cojocaru
  0 siblings, 0 replies; 22+ messages in thread
From: Razvan Cojocaru @ 2016-09-02 13:27 UTC (permalink / raw)
  To: Jan Beulich
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, xen-devel

On 09/02/2016 04:10 PM, Jan Beulich wrote:
>>>> On 02.09.16 at 15:03, <rcojocaru@bitdefender.com> wrote:
>> On 09/02/2016 03:56 PM, Jan Beulich wrote:
>>>>>> On 02.09.16 at 14:41, <rcojocaru@bitdefender.com> wrote:
>>>> On 09/02/2016 03:33 PM, Jan Beulich wrote:
>>>>>>>> On 02.09.16 at 14:21, <rcojocaru@bitdefender.com> wrote:
>>>>>> On 09/02/2016 01:02 PM, Jan Beulich wrote:
>>>>>>>> +    /*
>>>>>>>>> +     * Corresponding list of access settings for pfn_list
>>>>>>>>> +     * Used only with XENMEM_access_op_set_access_multi
>>>>>>>>> +     */
>>>>>>>>> +    XEN_GUEST_HANDLE(uint8) access_list;
>>>>>>> And for both of them - I don't think the arrays are meant to be
>>>>>>> used for output? In which case they should be handles of const
>>>>>>> types.
>>>>>>
>>>>>> Actually I can't seem to be able to find a magic combination that works.
>>>>>>
>>>>>> XEN_GUEST_HANDLE(const uint8) access_list; tells me:
>>>>>>
>>>>>> ./public/arch-x86/xen.h:53:41: error: '__guest_handle_const' does not
>>>>>> name a type
>>>>>>  #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
>>>>>>
>>>>>> which is fair. I've tried:
>>>>>>
>>>>>> XEN_GUEST_HANDLE(const_uint8) access_list;
>>>>>>
>>>>>> which does go further with the compilation process, but still kills it with:
>>>>>>
>>>>>> xen/include/compat/memory.h:154:5: error: unknown type name
>>>>>> '__compat_handle_const_uint8'
>>>>>>      COMPAT_HANDLE(const_uint8) access_list;
>>>>>>
>>>>>> What would be the appropriate const type to use here?
>>>>>
>>>>> This one. Did you check that handle actually gets created? Per
>>>>> my brief checking it doesn't look so. And neither the native one.
>>>>
>>>> Running ./configure again, followed by 'make clean' and a new 'make
>>>> dist' didn't help, so if it's supposed to be generated automatically it
>>>> doesn't seem to be (or I'm doing something wrong). I'll investigate more.
>>>
>>> The compat one is supposed to get auto-generated once the native
>>> one is there.
>>
>> Changing both handles to XEN_GUEST_HANLDE(const_void) compiles cleanly.
>> As soon as I change to either XEN_GUEST_HANLDE(const_uint8) or
>> XEN_GUEST_HANLDE(const_uint64) I start getting errors.
>>
>> Previously the auto-generation seems to have worked fine, so this is
>> likely something more subtle.
> 
> Oh, I see where the issue is: Both DEFINE_XEN_GUEST_HANDLE()
> and __DEFINE_XEN_GUEST_HANDLE() auto-magically produce const
> counterparts, but while DEFINE_COMPAT_HANDLE() does too,
> __DEFINE_COMPAT_HANDLE() doesn't. That'll need fixing, I think.

I see it, thanks! I'll submit a small patch shortly.


Thanks,
Razvan


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 13:26     ` Julien Grall
@ 2016-09-02 13:38       ` Razvan Cojocaru
  0 siblings, 0 replies; 22+ messages in thread
From: Razvan Cojocaru @ 2016-09-02 13:38 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: tamas, wei.liu2, george.dunlap, andrew.cooper3, ian.jackson, jbeulich

On 09/02/2016 04:26 PM, Julien Grall wrote:
> 
> 
> On 02/09/16 14:22, Razvan Cojocaru wrote:
>> On 09/02/2016 04:17 PM, Julien Grall wrote:
>>> Hello Razvan,
>>
>> Hello Julien, thanks for the email!
>>
>>> On 02/09/16 09:51, Razvan Cojocaru wrote:
>>>> Currently it is only possible to set mem_access restrictions only for
>>>> a contiguous range of GFNs (or, as a particular case, for a single GFN).
>>>> This patch introduces a new libxc function taking an array of GFNs.
>>>> The alternative would be to set each page in turn, using a userspace-HV
>>>> roundtrip for each call, and triggering a TLB flush per page set.
>>>>
>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>
>>>> ---
>>>> Changes since V1 / RFC:
>>>>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>>>>    and XENMEM_access_op_set_access_sparse to
>>>>    XENMEM_access_op_set_access_multi.
>>>>  - Renamed the 'nr' parameter to 'size'.
>>>>  - Wrapped long line in the implementation of xc_set_mem_access_multi().
>>>>  - Factored out common code in _p2m_set_mem_access() (and modified
>>>>    p2m_set_altp2m_mem_access() in the process, to take an unsigned
>>>>    long argument instead of a gfn_t).
>>>>  - Factored out xenmem_access_t to p2m_access_t conversion code in
>>>>    p2m_xenmem_access_to_p2m_access().
>>>>  - Added hypercall continuation support.
>>>>  - Added compat translation support.
>>>>  - No longer allocating buffers (now using copy_from_guest_offset()).
>>>>  - Added support for setting an array of access restrictions, as
>>>>    suggested by Tamas Lengyel.
>>>>  - This patch incorporates Jan Beulich's "memory: fix compat handling
>>>>    of XENMEM_access_op".
>>>> ---
>>>>  tools/libxc/include/xenctrl.h |   4 ++
>>>>  tools/libxc/xc_mem_access.c   |  38 ++++++++++
>>>>  xen/arch/x86/mm/p2m.c         | 161
>>>> ++++++++++++++++++++++++++++++++----------
>>>>  xen/common/compat/memory.c    |  24 +++++--
>>>>  xen/common/mem_access.c       |  11 +++
>>>>  xen/include/public/memory.h   |  14 +++-
>>>>  xen/include/xen/p2m-common.h  |   6 ++
>>>
>>> p2m-common.h contains prototype common between ARM and x86. I would have
>>> expected to see a change in the ARM port because of that.
>>
>> The only change to p2m-common.h is the addition of a single function,
>> p2m_set_mem_access_multi(). As long as the ARM bits don't make use of
>> it, there's nothing to modify there.
> 
> p2m_set_mem_access_multi is called from common code. If you had tried
> to build Xen on ARM you would have noticed a compilation error:
> 
> prelink.o: In function `mem_access_memop':
> /local/home/julieng/works/xen/xen/common/mem_access.c:80: undefined reference to `p2m_set_mem_access_multi'
> aarch64-linux-gnu-ld: /local/home/julieng/works/xen/xen/.xen-syms.0: hidden symbol `p2m_set_mem_access_multi' isn't defined

I see. Sorry about that. I'll fix that in V3.


Thanks,
Razvan


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 11:45     ` Jan Beulich
@ 2016-09-02 14:50       ` Tamas K Lengyel
  2016-09-02 15:03         ` Jan Beulich
  0 siblings, 1 reply; 22+ messages in thread
From: Tamas K Lengyel @ 2016-09-02 14:50 UTC (permalink / raw)
  To: Jan Beulich
  Cc: wei.liu2, Razvan Cojocaru, George Dunlap, andrew.cooper3,
	Ian Jackson, xen-devel


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

On Sep 2, 2016 05:45, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 02.09.16 at 13:18, <rcojocaru@bitdefender.com> wrote:
> > On 09/02/2016 01:02 PM, Jan Beulich wrote:
> >>>>> On 02.09.16 at 10:51, <rcojocaru@bitdefender.com> wrote:
> >>> Changes since V1 / RFC:
> >>>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
> >>>    and XENMEM_access_op_set_access_sparse to
> >>>    XENMEM_access_op_set_access_multi.
> >>>  - Renamed the 'nr' parameter to 'size'.
> >>
> >> Why?
> >
> > Tamas suggested it, size sounded more intuitive to him. I have no
> > problem with either nr or size.
>
> Size to me means something in bytes, which clearly isn't the case
> here. There's not even support for other then 4k pages so far.
>

Lets make it array_size then to clarify?

Tamas

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

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 14:50       ` Tamas K Lengyel
@ 2016-09-02 15:03         ` Jan Beulich
  2016-09-02 15:13           ` Tamas K Lengyel
  0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2016-09-02 15:03 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: wei.liu2, Razvan Cojocaru, George Dunlap, andrew.cooper3,
	Ian Jackson, xen-devel

>>> On 02.09.16 at 16:50, <tamas@tklengyel.com> wrote:
> On Sep 2, 2016 05:45, "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>> >>> On 02.09.16 at 13:18, <rcojocaru@bitdefender.com> wrote:
>> > On 09/02/2016 01:02 PM, Jan Beulich wrote:
>> >>>>> On 02.09.16 at 10:51, <rcojocaru@bitdefender.com> wrote:
>> >>> Changes since V1 / RFC:
>> >>>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>> >>>    and XENMEM_access_op_set_access_sparse to
>> >>>    XENMEM_access_op_set_access_multi.
>> >>>  - Renamed the 'nr' parameter to 'size'.
>> >>
>> >> Why?
>> >
>> > Tamas suggested it, size sounded more intuitive to him. I have no
>> > problem with either nr or size.
>>
>> Size to me means something in bytes, which clearly isn't the case
>> here. There's not even support for other then 4k pages so far.
> 
> Lets make it array_size then to clarify?

What's wrong with "nr", matching the other (existing) function?

Jan


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 15:03         ` Jan Beulich
@ 2016-09-02 15:13           ` Tamas K Lengyel
  2016-09-02 15:20             ` Razvan Cojocaru
  0 siblings, 1 reply; 22+ messages in thread
From: Tamas K Lengyel @ 2016-09-02 15:13 UTC (permalink / raw)
  To: Jan Beulich
  Cc: wei.liu2, Razvan Cojocaru, George Dunlap, Andrew Cooper,
	Ian Jackson, xen-devel


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

On Sep 2, 2016 09:03, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 02.09.16 at 16:50, <tamas@tklengyel.com> wrote:
> > On Sep 2, 2016 05:45, "Jan Beulich" <JBeulich@suse.com> wrote:
> >>
> >> >>> On 02.09.16 at 13:18, <rcojocaru@bitdefender.com> wrote:
> >> > On 09/02/2016 01:02 PM, Jan Beulich wrote:
> >> >>>>> On 02.09.16 at 10:51, <rcojocaru@bitdefender.com> wrote:
> >> >>> Changes since V1 / RFC:
> >> >>>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
> >> >>>    and XENMEM_access_op_set_access_sparse to
> >> >>>    XENMEM_access_op_set_access_multi.
> >> >>>  - Renamed the 'nr' parameter to 'size'.
> >> >>
> >> >> Why?
> >> >
> >> > Tamas suggested it, size sounded more intuitive to him. I have no
> >> > problem with either nr or size.
> >>
> >> Size to me means something in bytes, which clearly isn't the case
> >> here. There's not even support for other then 4k pages so far.
> >
> > Lets make it array_size then to clarify?
>
> What's wrong with "nr", matching the other (existing) function?
>

IMHO it's too generic. So either a more descriptive name or a comment is
warranted to explain the inputs.

Tamas

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

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 15:20             ` Razvan Cojocaru
@ 2016-09-02 15:19               ` Tamas K Lengyel
  0 siblings, 0 replies; 22+ messages in thread
From: Tamas K Lengyel @ 2016-09-02 15:19 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: wei.liu2, George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
	Jan Beulich


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

On Sep 2, 2016 09:17, "Razvan Cojocaru" <rcojocaru@bitdefender.com> wrote:
>
> On 09/02/2016 06:13 PM, Tamas K Lengyel wrote:
> > On Sep 2, 2016 09:03, "Jan Beulich" <JBeulich@suse.com
> > <mailto:JBeulich@suse.com>> wrote:
> >>
> >> >>> On 02.09.16 at 16:50, <tamas@tklengyel.com
> > <mailto:tamas@tklengyel.com>> wrote:
> >> > On Sep 2, 2016 05:45, "Jan Beulich" <JBeulich@suse.com
> > <mailto:JBeulich@suse.com>> wrote:
> >> >>
> >> >> >>> On 02.09.16 at 13:18, <rcojocaru@bitdefender.com
> > <mailto:rcojocaru@bitdefender.com>> wrote:
> >> >> > On 09/02/2016 01:02 PM, Jan Beulich wrote:
> >> >> >>>>> On 02.09.16 at 10:51, <rcojocaru@bitdefender.com
> > <mailto:rcojocaru@bitdefender.com>> wrote:
> >> >> >>> Changes since V1 / RFC:
> >> >> >>>  - Renamed xc_set_mem_access_sparse() to
xc_set_mem_access_multi(),
> >> >> >>>    and XENMEM_access_op_set_access_sparse to
> >> >> >>>    XENMEM_access_op_set_access_multi.
> >> >> >>>  - Renamed the 'nr' parameter to 'size'.
> >> >> >>
> >> >> >> Why?
> >> >> >
> >> >> > Tamas suggested it, size sounded more intuitive to him. I have no
> >> >> > problem with either nr or size.
> >> >>
> >> >> Size to me means something in bytes, which clearly isn't the case
> >> >> here. There's not even support for other then 4k pages so far.
> >> >
> >> > Lets make it array_size then to clarify?
> >>
> >> What's wrong with "nr", matching the other (existing) function?
> >>
> >
> > IMHO it's too generic. So either a more descriptive name or a comment is
> > warranted to explain the inputs.
>
> If this satisfies everybody, I'll keep 'nr' and add a comment describing
> the function and parameters (which is a good thing anyway).
>
>

SGTM.

Thanks,
Tamas

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

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()
  2016-09-02 15:13           ` Tamas K Lengyel
@ 2016-09-02 15:20             ` Razvan Cojocaru
  2016-09-02 15:19               ` Tamas K Lengyel
  0 siblings, 1 reply; 22+ messages in thread
From: Razvan Cojocaru @ 2016-09-02 15:20 UTC (permalink / raw)
  To: Tamas K Lengyel, Jan Beulich
  Cc: George Dunlap, Andrew Cooper, wei.liu2, Ian Jackson, xen-devel

On 09/02/2016 06:13 PM, Tamas K Lengyel wrote:
> On Sep 2, 2016 09:03, "Jan Beulich" <JBeulich@suse.com
> <mailto:JBeulich@suse.com>> wrote:
>>
>> >>> On 02.09.16 at 16:50, <tamas@tklengyel.com
> <mailto:tamas@tklengyel.com>> wrote:
>> > On Sep 2, 2016 05:45, "Jan Beulich" <JBeulich@suse.com
> <mailto:JBeulich@suse.com>> wrote:
>> >>
>> >> >>> On 02.09.16 at 13:18, <rcojocaru@bitdefender.com
> <mailto:rcojocaru@bitdefender.com>> wrote:
>> >> > On 09/02/2016 01:02 PM, Jan Beulich wrote:
>> >> >>>>> On 02.09.16 at 10:51, <rcojocaru@bitdefender.com
> <mailto:rcojocaru@bitdefender.com>> wrote:
>> >> >>> Changes since V1 / RFC:
>> >> >>>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>> >> >>>    and XENMEM_access_op_set_access_sparse to
>> >> >>>    XENMEM_access_op_set_access_multi.
>> >> >>>  - Renamed the 'nr' parameter to 'size'.
>> >> >>
>> >> >> Why?
>> >> >
>> >> > Tamas suggested it, size sounded more intuitive to him. I have no
>> >> > problem with either nr or size.
>> >>
>> >> Size to me means something in bytes, which clearly isn't the case
>> >> here. There's not even support for other then 4k pages so far.
>> >
>> > Lets make it array_size then to clarify?
>>
>> What's wrong with "nr", matching the other (existing) function?
>>
> 
> IMHO it's too generic. So either a more descriptive name or a comment is
> warranted to explain the inputs.

If this satisfies everybody, I'll keep 'nr' and add a comment describing
the function and parameters (which is a good thing anyway).


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2016-09-02 15:20 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-02  8:51 [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi() Razvan Cojocaru
2016-09-02  8:53 ` Wei Liu
2016-09-02 10:02 ` Jan Beulich
2016-09-02 10:34   ` Jan Beulich
2016-09-02 11:18   ` Razvan Cojocaru
2016-09-02 11:45     ` Jan Beulich
2016-09-02 14:50       ` Tamas K Lengyel
2016-09-02 15:03         ` Jan Beulich
2016-09-02 15:13           ` Tamas K Lengyel
2016-09-02 15:20             ` Razvan Cojocaru
2016-09-02 15:19               ` Tamas K Lengyel
2016-09-02 12:21   ` Razvan Cojocaru
2016-09-02 12:33     ` Jan Beulich
2016-09-02 12:41       ` Razvan Cojocaru
2016-09-02 12:56         ` Jan Beulich
2016-09-02 13:03           ` Razvan Cojocaru
2016-09-02 13:10             ` Jan Beulich
2016-09-02 13:27               ` Razvan Cojocaru
2016-09-02 13:17 ` Julien Grall
2016-09-02 13:22   ` Razvan Cojocaru
2016-09-02 13:26     ` Julien Grall
2016-09-02 13:38       ` Razvan Cojocaru

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.