Re: [PATCH 0 of 2] Add libxc API that sets mem_access type for an array of gfns

[Aravindh Puthiyaparambil <aravindh@virtuata.com>]

On Thu, Apr 26, 2012 at 3:41 PM, Aravindh Puthiyaparambil
<aravindh@virtuata.com> wrote:
> On Thu, Apr 26, 2012 at 1:20 PM, Christian Limpach
> <christian.limpach@gmail.com> wrote:
>> On Thu, Apr 26, 2012 at 11:33 AM, Aravindh Puthiyaparambil
>> <aravindh@virtuata.com> wrote:
>>> When the mem_access type needs to be changed for multiple discontiguous gfns, calling xc_hvm_set_mem_access() multiple times does not perform very well. The main pain points are the multiple libxc calls themselves plus the multiple map_domain_page() / unmap_domain_page() and ept_sync_domain() calls for each ept_set_entry(gfn). The following patches adds a new mem_access API that sets the mem_access type for an array of guest physical addresses in one libxc call with minimal map_domain_page() / unmap_domain_page() and ept_sync_domain() calls.
>>
>>
>> Are you sure that your bulk code actually works?  It seems to me that
>> your __ept_set_entry function assumes that table still points to the
>> top of the p2m.  The "for ( i = ept_get_wl(d); i > target; i-- )" loop
>> will walk the table, and so in the subsequent calls from your bulk
>> loop, this won't work?
>>
>> I think you need something like an iterator, the context of which can
>> be passed around...
>
> Duh! You are right. My test code has been giving me a false positive.
> I completely misread how ept_next_level() works.
>
>> Also, the call to ept_get_entry in your bulk loop will do a walk in
>> every iteration, it seems a bit arbitrary to only (try to) avoid one
>> and not the other.  But I guess the "win" is from reducing the number
>> of ept_sync_domain calls.
>
> You are right. I was mainly focusing on removing the multiple libxc
> calls and reducing the ept_sync_domain calls. I thought removing the
> map and unmap of the p2m top page was an extra optimization which I
> obviously messed up. I will rework the patch to only stick with the
> original optimization I had in mind.

Does this look correct now?

Thanks,
Aravindh

changeset:   25252:b1bde8691257
summary:     x86/mm: Split ept_set_entry()

diff -r 63eb1343cbdb -r b1bde8691257 xen/arch/x86/mm/p2m-ept.c
--- a/xen/arch/x86/mm/p2m-ept.c	Wed Apr 25 19:29:53 2012 -0700
+++ b/xen/arch/x86/mm/p2m-ept.c	Thu Apr 26 17:10:29 2012 -0700
@@ -272,12 +272,13 @@ static int ept_next_level(struct p2m_dom
 }

 /*
- * ept_set_entry() computes 'need_modify_vtd_table' for itself,
+ * __ept_set_entry() computes 'need_modify_vtd_table' for itself,
  * by observing whether any gfn->mfn translations are modified.
  */
 static int
-ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
-              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma)
+__ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
+                unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
+                ept_entry_t *old_entry, bool_t *needs_sync)
 {
     ept_entry_t *table, *ept_entry = NULL;
     unsigned long gfn_remainder = gfn;
@@ -288,11 +289,9 @@ ept_set_entry(struct p2m_domain *p2m, un
     int ret = 0;
     bool_t direct_mmio = (p2mt == p2m_mmio_direct);
     uint8_t ipat = 0;
-    int need_modify_vtd_table = 1;
-    int vtd_pte_present = 0;
-    int needs_sync = 1;
+    bool_t need_modify_vtd_table = 1;
+    bool_t vtd_pte_present = 0;
     struct domain *d = p2m->domain;
-    ept_entry_t old_entry = { .epte = 0 };

     /*
      * the caller must make sure:
@@ -305,10 +304,6 @@ ept_set_entry(struct p2m_domain *p2m, un
          (order % EPT_TABLE_ORDER) )
         return 0;

-    ASSERT((target == 2 && hvm_hap_has_1gb(d)) ||
-           (target == 1 && hvm_hap_has_2mb(d)) ||
-           (target == 0));
-
     table = map_domain_page(ept_get_asr(d));

     for ( i = ept_get_wl(d); i > target; i-- )
@@ -352,7 +347,7 @@ ept_set_entry(struct p2m_domain *p2m, un
          * the intermediate tables will be freed below after the ept flush
          *
          * Read-then-write is OK because we hold the p2m lock. */
-        old_entry = *ept_entry;
+        old_entry->epte = ept_entry->epte;

         if ( mfn_valid(mfn_x(mfn)) || direct_mmio || p2m_is_paged(p2mt) ||
              (p2mt == p2m_ram_paging_in) )
@@ -369,7 +364,7 @@ ept_set_entry(struct p2m_domain *p2m, un

             new_entry.mfn = mfn_x(mfn);

-            if ( old_entry.mfn == new_entry.mfn )
+            if ( old_entry->mfn == new_entry.mfn )
                 need_modify_vtd_table = 0;

             ept_p2m_type_to_flags(&new_entry, p2mt, p2ma);
@@ -438,9 +433,6 @@ ept_set_entry(struct p2m_domain *p2m, un
 out:
     unmap_domain_page(table);

-    if ( needs_sync )
-        ept_sync_domain(p2m->domain);
-
     if ( rv && iommu_enabled && need_iommu(p2m->domain) &&
need_modify_vtd_table )
     {
         if ( iommu_hap_pt_share )
@@ -473,6 +465,28 @@ out:
         }
     }

+    return rv;
+}
+
+static int
+ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
+              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma)
+{
+    int target = order / EPT_TABLE_ORDER;
+    int rv = 0;
+    bool_t needs_sync = 1;
+    ept_entry_t old_entry = { .epte = 0 };
+
+    ASSERT((target == 2 && hvm_hap_has_1gb(d)) ||
+           (target == 1 && hvm_hap_has_2mb(d)) ||
+           (target == 0));
+
+    rv = __ept_set_entry(p2m, gfn, mfn, order, p2mt, p2ma, &old_entry,
+                         &needs_sync);
+
+    if ( needs_sync )
+        ept_sync_domain(p2m->domain);
+
     /* Release the old intermediate tables, if any.  This has to be the
        last thing we do, after the ept_sync_domain() and removal
        from the iommu tables, so as to avoid a potential

changeset:   25253:07225bc67197
summary:     mem_access: Add xc_hvm_mem_access_bulk() API

diff -r b1bde8691257 -r 07225bc67197 tools/libxc/xc_misc.c
--- a/tools/libxc/xc_misc.c	Thu Apr 26 17:10:29 2012 -0700
+++ b/tools/libxc/xc_misc.c	Thu Apr 26 17:10:35 2012 -0700
@@ -570,6 +570,57 @@ int xc_hvm_set_mem_access(
     return rc;
 }

+int xc_hvm_set_mem_access_bulk(
+    xc_interface *xch, domid_t dom, hvmmem_access_t mem_access,
+    xen_pfn_t *arr, int *err, uint64_t nr)
+{
+    DECLARE_HYPERCALL;
+    DECLARE_HYPERCALL_BUFFER(struct xen_hvm_set_mem_access_bulk, arg);
+    DECLARE_HYPERCALL_BOUNCE(arr, sizeof(xen_pfn_t) * nr,
+                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    DECLARE_HYPERCALL_BOUNCE(err, sizeof(int) * nr,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    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_bulk hypercall");
+        return -1;
+    }
+
+    if ( xc_hypercall_bounce_pre(xch, arr) ) {
+        PERROR("Could not bounce arr for xc_hvm_set_mem_access_bulk
hypercall");
+        rc = -1;
+        goto out;
+    }
+
+    if ( xc_hypercall_bounce_pre(xch, err) ) {
+        PERROR("Could not bounce err for xc_hvm_set_mem_access_bulk
hypercall");
+        rc = -1;
+        goto out;
+    }
+
+    arg->domid         = dom;
+    arg->hvmmem_access = mem_access;
+    arg->nr            = nr;
+    set_xen_guest_handle(arg->arr, arr);
+    set_xen_guest_handle(arg->err, err);
+
+    hypercall.op     = __HYPERVISOR_hvm_op;
+    hypercall.arg[0] = HVMOP_set_mem_access_bulk;
+    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg);
+
+    rc = do_xen_hypercall(xch, &hypercall);
+
+out:
+    xc_hypercall_buffer_free(xch, arg);
+    xc_hypercall_bounce_post(xch, arr);
+    xc_hypercall_bounce_post(xch, err);
+
+    return rc;
+}
+
 int xc_hvm_get_mem_access(
     xc_interface *xch, domid_t dom, uint64_t pfn, hvmmem_access_t* mem_access)
 {
diff -r b1bde8691257 -r 07225bc67197 tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h	Thu Apr 26 17:10:29 2012 -0700
+++ b/tools/libxc/xenctrl.h	Thu Apr 26 17:10:35 2012 -0700
@@ -1568,6 +1568,17 @@ int xc_hvm_set_mem_access(
     xc_interface *xch, domid_t dom, hvmmem_access_t memaccess,
uint64_t first_pfn, uint64_t nr);

 /*
+ * Set the arry of pfns to a specific access.
+ * When a pfn cannot be set to the specified access, its respective field in
+ * @err is set to the corresponding errno value.
+ * Allowed types are HVMMEM_access_default, HVMMEM_access_n, any combination of
+ * HVM_access_ + (rwx), and HVM_access_rx2rw
+ */
+int xc_hvm_set_mem_access_bulk(
+    xc_interface *xch, domid_t dom, hvmmem_access_t memaccess,
+    xen_pfn_t *arr, int *err, uint64_t num);
+
+/*
  * Gets the mem access for the given page (returned in memacess on success)
  */
 int xc_hvm_get_mem_access(
diff -r b1bde8691257 -r 07225bc67197 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Thu Apr 26 17:10:29 2012 -0700
+++ b/xen/arch/x86/hvm/hvm.c	Thu Apr 26 17:10:35 2012 -0700
@@ -4197,6 +4197,51 @@ long do_hvm_op(unsigned long op, XEN_GUE
         break;
     }

+    case HVMOP_set_mem_access_bulk:
+    {
+        struct xen_hvm_set_mem_access_bulk a;
+        struct domain *d;
+        xen_pfn_t *arr = 0;
+        int *err = 0;
+
+        if ( copy_from_guest(&a, arg, 1) )
+            return -EFAULT;
+
+        rc = rcu_lock_remote_target_domain_by_id(a.domid, &d);
+        if ( rc != 0 )
+            return rc;
+
+        rc = -EINVAL;
+
+        if ( !is_hvm_domain(d) )
+            goto param_fail9;
+
+        rc = -ENOMEM;
+        arr = xmalloc_array(xen_pfn_t, a.nr);
+        if (!arr)
+            goto param_fail9;
+
+        err = xmalloc_array(int, a.nr);
+        if (!err)
+            goto param_fail9;
+
+        if ( copy_from_guest(arr, a.arr, a.nr) )
+            goto param_fail9;
+
+        rc = p2m_set_access_bulk(d, arr, err, a.nr, a.hvmmem_access);
+
+        if ( copy_to_guest(a.err, err, a.nr) )
+            goto param_fail9;
+
+    param_fail9:
+        rcu_unlock_domain(d);
+        if (arr)
+            xfree(arr);
+        if (err)
+            xfree(err);
+        break;
+    }
+
     case HVMOP_get_mem_access:
     {
         struct xen_hvm_get_mem_access a;
diff -r b1bde8691257 -r 07225bc67197 xen/arch/x86/mm/p2m-ept.c
--- a/xen/arch/x86/mm/p2m-ept.c	Thu Apr 26 17:10:29 2012 -0700
+++ b/xen/arch/x86/mm/p2m-ept.c	Thu Apr 26 17:10:35 2012 -0700
@@ -597,6 +597,63 @@ out:
     return mfn;
 }

+static int
+ept_set_entry_bulk(struct p2m_domain *p2m, xen_pfn_t *arr, int *err,
+                   uint64_t nr, unsigned int order, p2m_access_t p2ma)
+{
+    unsigned long pfn;
+    p2m_access_t a;
+    p2m_type_t t;
+    mfn_t mfn;
+    int target = order / EPT_TABLE_ORDER;
+    int rc;
+    bool_t needs_sync, bulk_needs_sync = 0;
+    struct domain *d = p2m->domain;
+    ept_entry_t *old_entries = 0;
+
+    ASSERT((target == 2 && hvm_hap_has_1gb(d)) ||
+           (target == 1 && hvm_hap_has_2mb(d)) ||
+           (target == 0));
+
+    old_entries = xzalloc_array(ept_entry_t, nr);
+    if ( !old_entries )
+        return 0;
+
+    memset(err, 0, nr * sizeof(int));
+    for ( pfn = 0; pfn < nr; pfn++ )
+    {
+        if ( arr[pfn] > domain_get_maximum_gpfn(d) )
+        {
+            err[pfn] = -EINVAL;
+            continue;
+        }
+
+        needs_sync = 1;
+        mfn = ept_get_entry(p2m, arr[pfn], &t, &a, 0, NULL);
+        rc = __ept_set_entry(p2m, arr[pfn], mfn, order, t, p2ma,
+                             &old_entries[pfn], &needs_sync);
+        if (rc == 0)
+            err[pfn] = -ENOMEM;
+
+        if ( needs_sync )
+            bulk_needs_sync = 1;
+    }
+
+    if ( bulk_needs_sync )
+        ept_sync_domain(p2m->domain);
+
+    /* Release the old intermediate tables, if any.  This has to be the
+       last thing we do, after the ept_sync_domain() and removal
+       from the iommu tables, so as to avoid a potential
+       use-after-free. */
+    for ( pfn = 0; pfn < nr; pfn++ )
+        if ( is_epte_present(&old_entries[pfn]) )
+            ept_free_entry(p2m, &old_entries[pfn], target);
+
+    /* Success */
+    return 1;
+}
+
 /* WARNING: Only caller doesn't care about PoD pages.  So this function will
  * always return 0 for PoD pages, not populate them.  If that becomes
necessary,
  * pass a p2m_query_t type along to distinguish. */
@@ -808,6 +865,7 @@ static void ept_change_entry_type_global
 void ept_p2m_init(struct p2m_domain *p2m)
 {
     p2m->set_entry = ept_set_entry;
+    p2m->set_entry_bulk = ept_set_entry_bulk;
     p2m->get_entry = ept_get_entry;
     p2m->change_entry_type_global = ept_change_entry_type_global;
     p2m->audit_p2m = NULL;
diff -r b1bde8691257 -r 07225bc67197 xen/arch/x86/mm/p2m-pt.c
--- a/xen/arch/x86/mm/p2m-pt.c	Thu Apr 26 17:10:29 2012 -0700
+++ b/xen/arch/x86/mm/p2m-pt.c	Thu Apr 26 17:10:35 2012 -0700
@@ -1139,6 +1139,7 @@ long p2m_pt_audit_p2m(struct p2m_domain
 void p2m_pt_init(struct p2m_domain *p2m)
 {
     p2m->set_entry = p2m_set_entry;
+    p2m->set_entry_bulk = NULL;
     p2m->get_entry = p2m_gfn_to_mfn;
     p2m->change_entry_type_global = p2m_change_type_global;
     p2m->write_p2m_entry = paging_write_p2m_entry;
diff -r b1bde8691257 -r 07225bc67197 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Thu Apr 26 17:10:29 2012 -0700
+++ b/xen/arch/x86/mm/p2m.c	Thu Apr 26 17:10:35 2012 -0700
@@ -1334,6 +1334,42 @@ int p2m_set_mem_access(struct domain *d,
     return rc;
 }

+int p2m_set_access_bulk(struct domain *d, xen_pfn_t *arr, int *err,
+                        uint64_t nr, hvmmem_access_t access)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    p2m_access_t a;
+    p2m_access_t memaccess[] = {
+        p2m_access_n,
+        p2m_access_r,
+        p2m_access_w,
+        p2m_access_rw,
+        p2m_access_x,
+        p2m_access_rx,
+        p2m_access_wx,
+        p2m_access_rwx,
+        p2m_access_rx2rw,
+        p2m_access_n2rwx,
+        p2m->default_access,
+    };
+    int rc = 0;
+
+    ASSERT(p2m->set_entry_bulk);
+
+    if ( (unsigned) access >= HVMMEM_access_default )
+        return -EINVAL;
+
+    a = memaccess[access];
+
+    p2m_lock(p2m);
+    if ( p2m->set_entry_bulk(p2m, arr, err, nr, PAGE_ORDER_4K, a) == 0 )
+        rc = -ENOMEM;
+    p2m_unlock(p2m);
+
+    return rc;
+}
+
+
 /* 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,
diff -r b1bde8691257 -r 07225bc67197 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h	Thu Apr 26 17:10:29 2012 -0700
+++ b/xen/include/asm-x86/p2m.h	Thu Apr 26 17:10:35 2012 -0700
@@ -232,6 +232,13 @@ struct p2m_domain {
                                        mfn_t mfn, unsigned int page_order,
                                        p2m_type_t p2mt,
                                        p2m_access_t p2ma);
+    int                (*set_entry_bulk)(struct p2m_domain *p2m,
+                                         xen_pfn_t *arr,
+                                         int *err,
+                                         uint64_t nr,
+                                         unsigned int order,
+                                         p2m_access_t p2ma);
+
     mfn_t              (*get_entry   )(struct p2m_domain *p2m,
                                        unsigned long gfn,
                                        p2m_type_t *p2mt,
@@ -568,6 +575,11 @@ void p2m_mem_access_resume(struct domain
 int p2m_set_mem_access(struct domain *d, unsigned long start_pfn,
                        uint32_t nr, hvmmem_access_t access);

+/* Set access type for an array of pfns. set_mem_access success or failure is
+ * returned in the err array. */
+int p2m_set_access_bulk(struct domain *d, xen_pfn_t *arr, int *err,
+                        uint64_t nr, hvmmem_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,
@@ -583,7 +595,11 @@ static inline int p2m_set_mem_access(str
                                      unsigned long start_pfn,
                                      uint32_t nr, hvmmem_access_t access)
 { return -EINVAL; }
-static inline int p2m_get_mem_access(struct domain *d, unsigned long pfn,
+static inline int p2m_set_access_bulk(struct domain *d, xen_pfn_t *arr,
+                                      int *err, uint64_t nr,
+                                      hvmmem_access_t access)
+{ return -EINVAL; }
+static inline int p2m_get_mem_access(struct domain *d, unsigned long pfn,
                                      hvmmem_access_t *access)
 { return -EINVAL; }
 #endif
diff -r b1bde8691257 -r 07225bc67197 xen/include/public/hvm/hvm_op.h
--- a/xen/include/public/hvm/hvm_op.h	Thu Apr 26 17:10:29 2012 -0700
+++ b/xen/include/public/hvm/hvm_op.h	Thu Apr 26 17:10:35 2012 -0700
@@ -261,4 +261,22 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_inject_m

 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */

+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+#define HVMOP_set_mem_access_bulk      17
+/* Notify that a array of pfns is to have specific access types */
+struct xen_hvm_set_mem_access_bulk {
+    /* Domain to be updated. */
+    domid_t domid;
+    /* Memory type */
+    uint16_t hvmmem_access; /* hvm_access_t */
+    /* Array of pfns */
+    XEN_GUEST_HANDLE_64(xen_pfn_t) arr;
+    XEN_GUEST_HANDLE_64(int) err ;
+    /* Number of entries */
+    uint64_t nr;
+};
+typedef struct xen_hvm_set_mem_access_bulk xen_hvm_set_mem_access_bulk_t;
+DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_mem_access_bulk_t);
+#endif
+
 #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */