Add libxc API that sets mem_access type for an array of gfns

PATCH In review Aravindh Puthiyaparambil 3 participants 17 messages Apr 26, 2012 – May 17, 2012

Share & tools

Aravindh Puthiyaparambil

author

2012-04-26 18:33 UTC #0 lore raw

PATCH 1/1 Add libxc API that sets mem_access type for an array of gfns

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.

Signed-off-by Aravindh Puthiyaparambil <[email protected]>

Aravindh Puthiyaparambil

author

2012-04-26 18:33 UTC #1 lore raw

PATCH 1/1 x86/mm: Split ept_set_entry()

 xen/arch/x86/mm/p2m-ept.c |  60 ++++++++++++++++++++++++++++------------------
 1 files changed, 37 insertions(+), 23 deletions(-)

Split ept_set_entry() such that mapping and unmapping of the page tables and ept_sync occurrs outside of it.

Signed-off-by Aravindh Puthiyaparambil <[email protected]>

xen/arch/x86/mm/p2m-ept.c +37 ~~-23~~

diff -r 63eb1343cbdb -r f12cf785738f xen/arch/x86/mm/p2m-ept.c
@@ -272,14 +272,15 @@ 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 *table, ept_entry_t *old_entry, bool_t *needs_sync)
 {
-    ept_entry_t *table, *ept_entry = NULL;
+    ept_entry_t *ept_entry = NULL;
     unsigned long gfn_remainder = gfn;
     unsigned long offset = 0;
     u32 index;

@@ -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,12 +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-- )
     {
         ret = ept_next_level(p2m, 0, &table, &gfn_remainder, i);

@@ -327,7 +320,7 @@ ept_set_entry(struct p2m_domain *p2m, un
 
     ept_entry = table + index;
 
-    /* In case VT-d uses same page table, this flag is needed by VT-d */ 
+    /* In case VT-d uses same page table, this flag is needed by VT-d */
     vtd_pte_present = is_epte_present(ept_entry) ? 1 : 0;
 
     /*

@@ -352,7 +345,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 +362,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);

@@ -435,12 +428,7 @@ ept_set_entry(struct p2m_domain *p2m, un
     /* Success */
     rv = 1;
 
-out:
-    unmap_domain_page(table);
-
-    if ( needs_sync )
-        ept_sync_domain(p2m->domain);
-
+ out:
     if ( rv && iommu_enabled && need_iommu(p2m->domain) && need_modify_vtd_table )
     {
         if ( iommu_hap_pt_share )

@@ -473,6 +461,32 @@ 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)
+{
+    ept_entry_t *table;
+    int target = order / EPT_TABLE_ORDER;
+    int rv = 0;
+    bool_t needs_sync = 1;
+    struct domain *d = p2m->domain;
+    ept_entry_t old_entry = { .epte = 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));
+    rv = __ept_set_entry(p2m, gfn, mfn, order, p2mt, p2ma, table, &old_entry,
+                         &needs_sync);
+    unmap_domain_page(table);
+
+    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

Aravindh Puthiyaparambil

author

2012-04-26 18:33 UTC #2 lore raw

PATCH 1/1 mem_access: Add xc_hvm_mem_access_bulk() API

In reply to Aravindh Puthiyaparambil

 tools/libxc/xc_misc.c           |  51 ++++++++++++++++++++++++++++++++++
 tools/libxc/xenctrl.h           |  11 +++++++
 xen/arch/x86/hvm/hvm.c          |  45 ++++++++++++++++++++++++++++++
 xen/arch/x86/mm/p2m-ept.c       |  61 +++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/p2m-pt.c        |   1 +
 xen/arch/x86/mm/p2m.c           |  36 ++++++++++++++++++++++++
 xen/include/asm-x86/p2m.h       |  18 +++++++++++-
 xen/include/public/hvm/hvm_op.h |  18 ++++++++++++
 8 files changed, 240 insertions(+), 1 deletions(-)

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);

Set the array of guest physical addresses to a specific mem_access type. Allowed types are HVMMEM_access_default, HVMMEM_access_n, any combination of HVM_access_ + (rwx), and HVM_access_rx2rw.
When a gfn cannot be set to the specified access, its respective field in @err is set to the corresponding errno value.

Signed-off-by Aravindh Puthiyaparambil <[email protected]>

tools/libxc/xc_misc.c +51 -0

diff -r f12cf785738f -r 2f695c43ce22 tools/libxc/xc_misc.c
@@ -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)
 {

tools/libxc/xenctrl.h +11 -0

diff -r f12cf785738f -r 2f695c43ce22 tools/libxc/xenctrl.h
@@ -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(

xen/arch/x86/hvm/hvm.c +45 -0

51 lines not on this page · raw message

xen/arch/x86/mm/p2m-ept.c +61 -0

73 lines not on this page · raw message

xen/arch/x86/mm/p2m-pt.c +1 -0

diff -r f12cf785738f -r 2f695c43ce22 xen/arch/x86/mm/p2m-pt.c
@@ -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;

xen/arch/x86/mm/p2m.c +36 -0

diff -r f12cf785738f -r 2f695c43ce22 xen/arch/x86/mm/p2m.c
@@ -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,

xen/include/asm-x86/p2m.h +17 -1

diff -r f12cf785738f -r 2f695c43ce22 xen/include/asm-x86/p2m.h
@@ -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

xen/include/public/hvm/hvm_op.h +18 -0

diff -r f12cf785738f -r 2f695c43ce22 xen/include/public/hvm/hvm_op.h
@@ -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__ */

Christian Limpach

2012-04-26 20:20 UTC #3 lore raw

In reply to Aravindh Puthiyaparambil

Quoted 1 line · Aravindh Puthiyaparambil

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

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.

christian

Aravindh Puthiyaparambil

author

2012-04-26 22:41 UTC #4 lore raw

Quoted 5 lines · Christian Limpach

Quoted 1 line · Aravindh Puthiyaparambil
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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Thanks,
Aravindh

Aravindh Puthiyaparambil

author

2012-04-27 00:15 UTC #5 lore raw

Quoted 24 lines · Aravindh Puthiyaparambil

Quoted 5 lines · Christian Limpach
Quoted 1 line · Aravindh Puthiyaparambil
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.
Quoted 8 lines
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.
Quoted 4 lines
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.

Quoted 5 lines

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()

xen/arch/x86/mm/p2m-ept.c +8 ~~-16~~

diff -r 63eb1343cbdb -r b1bde8691257 xen/arch/x86/mm/p2m-ept.c
@@ -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

tools/libxc/xc_misc.c +16 -0

diff -r b1bde8691257 -r 07225bc67197 tools/libxc/xc_misc.c
@@ -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)
{

tools/libxc/xenctrl.h +0 -0

diff -r b1bde8691257 -r 07225bc67197 tools/libxc/xenctrl.h
@@ -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(

xen/arch/x86/hvm/hvm.c +45 -0

51 lines not on this page · raw message

xen/arch/x86/mm/p2m-ept.c +57 -0

62 lines not on this page · raw message

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;

xen/arch/x86/mm/p2m-pt.c +1 -0

diff -r b1bde8691257 -r 07225bc67197 xen/arch/x86/mm/p2m-pt.c
@@ -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;

xen/arch/x86/mm/p2m.c +36 -0

diff -r b1bde8691257 -r 07225bc67197 xen/arch/x86/mm/p2m.c
@@ -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,

xen/include/asm-x86/p2m.h +17 -1

diff -r b1bde8691257 -r 07225bc67197 xen/include/asm-x86/p2m.h
@@ -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

xen/include/public/hvm/hvm_op.h +18 -0

diff -r b1bde8691257 -r 07225bc67197 xen/include/public/hvm/hvm_op.h
@@ -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__ */

Christian Limpach

2012-04-27 01:06 UTC #6 lore raw

Quoted 1 line · Aravindh Puthiyaparambil

Does this look correct now?

It addresses the issues I've pointed out, but:
- you should leave the ASSERT where it is, or is there a reason to move it?
- this is wrong:

Quoted 2 lines

- old_entry = *ept_entry;
+ old_entry->epte = ept_entry->epte;

You should follow the code and see what uses old_entry and you'll see that within the function old_entry->mfn is used (your diff changes the line that uses it) and ept_free_entry also accesses mfn.
- are you sure you can move the ept_sync_domain call past the iommu code?

I made a similar change a while ago, though it is for a more specific case, updating the ept table to "clean" the vram tracking. My change is:
- clear needs_sync when setting the type to logdirty for a leaf entry

         if ( !is_epte_present(ept_entry) ||
             (!target && p2mt == p2m_ram_logdirty) )
            needs_sync = 0;
- only call ept_free_entry in the non-leaf case
    if ( target && is_epte_present(&old_entry) )
        ept_free_entry(p2m, &old_entry, target);

- call ept_sync_domain from hap_clean_vram_tracking

Maybe you can do something similar, for example passing in a hint whether ept_sync_domain needs to be done or not. In my case, the reasoning is that all the entries changed from hap_clean_vram_tracking are leaf entries, so ept_free_entry will never be called and thus ept_sync_domain can be deferred. I didn't think through/consider the iommu case since that code is commented out in my tree.

christian

Aravindh Puthiyaparambil

author

2012-04-27 01:36 UTC #7 lore raw

Quoted 7 lines · Christian Limpach

Quoted 1 line · Aravindh Puthiyaparambil
Does this look correct now?
It addresses the issues I've pointed out, but:
- you should leave the ASSERT where it is, or is there a reason to move

it?

Ok, I will move the ASSERT back to where it was.

Quoted 6 lines

- this is wrong:
xen/arch/x86/mm/p2m-ept.c ept_set_entry(struct p2m_domain *p2m, un · line 350
Quoted 2 lines
xen/arch/x86/mm/p2m-ept.c +1 -1
-        old_entry = *ept_entry;
+        old_entry->epte = ept_entry->epte;
You should follow the code and see what uses old_entry and you'll see that within the function old_entry->mfn is used (your diff changes the line that uses it) and ept_free_entry also accesses mfn.

I will fix that.

Quoted 2 lines

- are you sure you can move the ept_sync_domain call past the iommu code?

I was hoping Tim would give me feedback about that.

Quoted 10 lines

I made a similar change a while ago, though it is for a more specific case, updating the ept table to "clean" the vram tracking. My change is:
- clear needs_sync when setting the type to logdirty for a leaf entry
        if ( !is_epte_present(ept_entry) ||
            (!target && p2mt == p2m_ram_logdirty) )
           needs_sync = 0;
- only call ept_free_entry in the non-leaf case
   if ( target && is_epte_present(&old_entry) )
       ept_free_entry(p2m, &old_entry, target);

Quoted 9 lines

- call ept_sync_domain from hap_clean_vram_tracking
Maybe you can do something similar, for example passing in a hint whether ept_sync_domain needs to be done or not. In my case, the reasoning is that all the entries changed from hap_clean_vram_tracking are leaf entries, so ept_free_entry will never be called and thus ept_sync_domain can be deferred. I didn't think through/consider the iommu case since that code is commented out in my tree.

I thought about doing that initially. But then in the bulk case I would always have to call ept_sync_domain() to be on the safe side. But if the iommu case forces me down that path, then I guess I have no choice.

Aravindh

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Tim Deegan

2012-04-27 08:48 UTC #8 lore raw

Quoted 7 lines · Aravindh Puthiyaparambil

Quoted 6 lines
- this is wrong:
xen/arch/x86/mm/p2m-ept.c ept_set_entry(struct p2m_domain *p2m, un · line 350
Quoted 2 lines
xen/arch/x86/mm/p2m-ept.c +1 -1
-        old_entry = *ept_entry;
+        old_entry->epte = ept_entry->epte;
You should follow the code and see what uses old_entry and you'll see that within the function old_entry->mfn is used (your diff changes the line that uses it) and ept_free_entry also accesses mfn.

Quoted 6 lines

I will fix that.
Quoted 2 lines
- are you sure you can move the ept_sync_domain call past the iommu code?
I was hoping Tim would give me feedback about that.

I'm afraid I won't be able to review this code until next week. This change is already too late for the 4.2 freeze, though, so there's plenty of time to get it right after we branch.

Cheers,

Tim.

Aravindh Puthiyaparambil

author

2012-04-27 18:26 UTC #9 lore raw

Quoted 10 lines · Tim Deegan

Quoted 9 lines · Aravindh Puthiyaparambil
Quoted 6 lines
- this is wrong:
Quoted 2 lines
- old_entry = *ept_entry;
+ old_entry->epte = ept_entry->epte;
You should follow the code and see what uses old_entry and you'll see that within the function old_entry->mfn is used (your diff changes the line that uses it) and ept_free_entry also accesses mfn.
I will fix that.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
- are you sure you can move the ept_sync_domain call past the iommu code?
I was hoping Tim would give me feedback about that.
I'm afraid I won't be able to review this code until next week. This change is already too late for the 4.2 freeze, though, so there's plenty of time to get it right after we branch.

Not a problem. This can wait for post 4.2. By then I would have reworked it the way Christian was mentioning.

Thanks,
Aravindh

Christian Limpach

2012-04-27 17:37 UTC #10 lore raw

In reply to Aravindh Puthiyaparambil

Quoted 11 lines · Aravindh Puthiyaparambil

Quoted 7 lines · Christian Limpach
Maybe you can do something similar, for example passing in a hint whether ept_sync_domain needs to be done or not. In my case, the reasoning is that all the entries changed from hap_clean_vram_tracking are leaf entries, so ept_free_entry will never be called and thus ept_sync_domain can be deferred. I didn't think through/consider the iommu case since that code is commented out in my tree.

Quoted 3 lines

I thought about doing that initially. But then in the bulk case I would always have to call ept_sync_domain() to be on the safe side. But if the iommu case forces me down that path, then I guess I have no choice.

I think you should re-consider. It would avoid adding the extra wrapper, which makes this code a lot less readable. Plus it avoids the need for the old_entries array.

Let me re-iterate:
- if it's a leaf entry, then there's no need to call ept_free_entry
- if you don't need to call ept_free_entry, then you can defer ept_sync_domain (subject to the iommu case)
- you could pass in a pointer to a bool to indicate to the caller that ept_sync_domain was deferred and that the caller needs to do it, with a NULL pointer indicating that the caller doesn't support defer

christian

Aravindh Puthiyaparambil

author

2012-04-27 18:25 UTC #11 lore raw

Quoted 21 lines · Christian Limpach

Quoted 11 lines · Aravindh Puthiyaparambil
Quoted 7 lines · Christian Limpach
Maybe you can do something similar, for example passing in a hint whether ept_sync_domain needs to be done or not. In my case, the reasoning is that all the entries changed from hap_clean_vram_tracking are leaf entries, so ept_free_entry will never be called and thus ept_sync_domain can be deferred. I didn't think through/consider the iommu case since that code is commented out in my tree.
Quoted 3 lines
I thought about doing that initially. But then in the bulk case I would always have to call ept_sync_domain() to be on the safe side. But if the iommu case forces me down that path, then I guess I have no choice.
I think you should re-consider. It would avoid adding the extra wrapper, which makes this code a lot less readable. Plus it avoids the need for the old_entries array.

Quoted 7 lines

Let me re-iterate:
- if it's a leaf entry, then there's no need to call ept_free_entry
- if you don't need to call ept_free_entry, then you can defer ept_sync_domain (subject to the iommu case)
- you could pass in a pointer to a bool to indicate to the caller that ept_sync_domain was deferred and that the caller needs to do it, with a NULL pointer indicating that the caller doesn't support defer

I understand now and like this approach. I will rework the patch and send it out.

Thanks,
Aravindh

Aravindh Puthiyaparambil

author

2012-04-28 04:22 UTC #12 lore raw

Quoted 23 lines · Aravindh Puthiyaparambil

Quoted 21 lines · Christian Limpach
Quoted 11 lines · Aravindh Puthiyaparambil
Quoted 7 lines · Christian Limpach
Maybe you can do something similar, for example passing in a hint whether ept_sync_domain needs to be done or not. In my case, the reasoning is that all the entries changed from hap_clean_vram_tracking are leaf entries, so ept_free_entry will never be called and thus ept_sync_domain can be deferred. I didn't think through/consider the iommu case since that code is commented out in my tree.
Quoted 3 lines
I thought about doing that initially. But then in the bulk case I would always have to call ept_sync_domain() to be on the safe side. But if the iommu case forces me down that path, then I guess I have no choice.
I think you should re-consider. It would avoid adding the extra wrapper, which makes this code a lot less readable. Plus it avoids the need for the old_entries array.

Quoted 7 lines

Quoted 7 lines
Let me re-iterate:
- if it's a leaf entry, then there's no need to call ept_free_entry
- if you don't need to call ept_free_entry, then you can defer ept_sync_domain (subject to the iommu case)
- you could pass in a pointer to a bool to indicate to the caller that ept_sync_domain was deferred and that the caller needs to do it, with a NULL pointer indicating that the caller doesn't support defer

How does this look?

changeset:   25257:2c05bdb052ea
user:        Aravindh Puthiyaparambil <[email protected]>
date:        Fri Apr 27 20:28:37 2012 -0700
summary:     x86/mm: Add sync deferred option to p2m->set_entry()

xen/arch/x86/mm/mem_sharing.c +1 -1

diff -r 9dda0efd8ce1 -r 2c05bdb052ea xen/arch/x86/mm/mem_sharing.c
@@ -1272,7 +1272,7 @@ int relinquish_shared_pages(struct domai
             /* Clear out the p2m entry so no one else may try to
              * unshare */
             p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
-                            p2m_invalid, p2m_access_rwx);
+                            p2m_invalid, p2m_access_rwx, NULL);
             count++;
         }

xen/arch/x86/mm/p2m-ept.c +12 -1

diff -r 9dda0efd8ce1 -r 2c05bdb052ea xen/arch/x86/mm/p2m-ept.c
@@ -274,10 +274,13 @@ static int ept_next_level(struct p2m_dom
 /*
  * ept_set_entry() computes 'need_modify_vtd_table' for itself,
  * by observing whether any gfn->mfn translations are modified.
+ * If sync_deferred is not NULL, then the caller will take care of
+ * calling ept_sync_domain() in the cases where it can be deferred.
  */
 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)
+              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
+              bool_t *sync_deferred)
 {
     ept_entry_t *table, *ept_entry = NULL;
     unsigned long gfn_remainder = gfn;

@@ -293,6 +296,7 @@ ept_set_entry(struct p2m_domain *p2m, un
     int needs_sync = 1;
     struct domain *d = p2m->domain;
     ept_entry_t old_entry = { .epte = 0 };
+    bool_t _sync_deferred = 0;
 
     /*
      * the caller must make sure:

@@ -309,6 +313,9 @@ ept_set_entry(struct p2m_domain *p2m, un
            (target == 1 && hvm_hap_has_2mb(d)) ||
            (target == 0));
 
+    if (sync_deferred)
+        *sync_deferred = 1;
+
     table = map_domain_page(ept_get_asr(d));
 
     for ( i = ept_get_wl(d); i > target; i-- )

@@ -346,7 +353,11 @@ ept_set_entry(struct p2m_domain *p2m, un
 
         /* No need to flush if the old entry wasn't valid */
         if ( !is_epte_present(ept_entry) )
+        {
             needs_sync = 0;
+            if ( sync_deferred )
+                *sync_deferred = 0;
+        }
 
         /* If we're replacing a non-leaf entry with a leaf entry

(1GiB or 2MiB),
* the intermediate tables will be freed below after the ept flush
@@ -385,6 +396,9 @@ ept_set_entry(struct p2m_domain *p2m, un

         ASSERT(is_epte_superpage(ept_entry));

+ if ( sync_deferred )
+ _sync_deferred = 1;
+
split_ept_entry = atomic_read_ept_entry(ept_entry);

if ( !ept_split_super_page(p2m, &split_ept_entry, i, target) )
@@ -438,7 +452,7 @@ ept_set_entry(struct p2m_domain *p2m, un
out:

     unmap_domain_page(table);

- if ( needs_sync )
+ if ( needs_sync && !_sync_deferred )

         ept_sync_domain(p2m->domain);

     if ( rv && iommu_enabled && need_iommu(p2m->domain) &&
need_modify_vtd_table )
@@ -727,7 +741,8 @@ void ept_change_entry_emt_with_range(str
                     order = level * EPT_TABLE_ORDER;
                     if ( need_modify_ept_entry(p2m, gfn, mfn,
                           e.ipat, e.emt, e.sa_p2mt) )

- ept_set_entry(p2m, gfn, mfn, order,
e.sa_p2mt, e.access);
+ ept_set_entry(p2m, gfn, mfn, order,
e.sa_p2mt, e.access,

+                                      NULL);
                     gfn += trunk;
                     break;
                 }

@@ -737,7 +752,7 @@ void ept_change_entry_emt_with_range(str
else /* gfn assigned with 4k */
{
if ( need_modify_ept_entry(p2m, gfn, mfn, e.ipat, e.emt,
e.sa_p2mt) )
- ept_set_entry(p2m, gfn, mfn, order, e.sa_p2mt, e.access);

+                ept_set_entry(p2m, gfn, mfn, order, e.sa_p2mt, e.access, NULL);
         }
     }
     p2m_unlock(p2m);

xen/arch/x86/mm/p2m-pt.c +2 -1

diff -r 9dda0efd8ce1 -r 2c05bdb052ea xen/arch/x86/mm/p2m-pt.c
@@ -291,7 +291,8 @@ p2m_next_level(struct p2m_domain *p2m, m
 // Returns 0 on error (out of memory)
 static int
 p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
-              unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma)
+              unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma,
+              bool_t *unused)
 {
     // XXX -- this might be able to be faster iff current->domain == d
     mfn_t table_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m));

xen/arch/x86/mm/p2m.c +2 -2

diff -r 9dda0efd8ce1 -r 2c05bdb052ea xen/arch/x86/mm/p2m.c
@@ -227,7 +227,7 @@ int set_p2m_entry(struct p2m_domain *p2m
         else
             order = 0;
 
-        if ( !p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma) )
+        if ( !p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, NULL) )
             rc = 0;
         gfn += 1ul << order;
         if ( mfn_x(mfn) != INVALID_MFN )

@@ -1199,14 +1199,14 @@ bool_t p2m_mem_access_check(unsigned lon
 
     if ( access_w && p2ma == p2m_access_rx2rw )
     {
-        p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rw);
+        p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,

p2m_access_rw, NULL);
         gfn_unlock(p2m, gfn, 0);
         return 1;
     }
     else if ( p2ma == p2m_access_n2rwx )
     {
         ASSERT(access_w || access_r || access_x);
-        p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx);
+        p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,
p2m_access_rwx, NULL);
     }
     gfn_unlock(p2m, gfn, 0);

@@ -1228,7 +1228,8 @@ bool_t p2m_mem_access_check(unsigned lon

             {
                 /* A listener is not required, so clear the access
restrictions */
                 gfn_lock(p2m, gfn, 0);
-                p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,
p2m_access_rwx);
+                p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,
+                               p2m_access_rwx, NULL);
                 gfn_unlock(p2m, gfn, 0);
             }
             return 1;
@@ -1292,6 +1293,7 @@ int p2m_set_mem_access(struct domain *d,
     p2m_type_t t;
     mfn_t mfn;
     int rc = 0;

+ bool_t sync_deferred = 1;

     /* N.B. _not_ static: initializer depends on p2m->default_access */
     p2m_access_t memaccess[] = {
@@ -1324,12 +1326,17 @@ int p2m_set_mem_access(struct domain *d,
     for ( pfn = start_pfn; pfn < start_pfn + nr; pfn++ )
     {
         mfn = p2m->get_entry(p2m, pfn, &t, &_a, 0, NULL);
-        if ( p2m->set_entry(p2m, pfn, mfn, PAGE_ORDER_4K, t, a) == 0 )
+        if ( p2m->set_entry(p2m, pfn, mfn, PAGE_ORDER_4K, t, a, &sync_deferred)
+             == 0 )
         {
             rc = -ENOMEM;
             break;
         }
     }

+
+ if ( sync_deferred )
+ ept_sync_domain(p2m->domain);
+

     p2m_unlock(p2m);
     return rc;

}

xen/include/asm-x86/p2m.h +2 -1

diff -r 9dda0efd8ce1 -r 2c05bdb052ea xen/include/asm-x86/p2m.h
@@ -231,7 +231,8 @@ struct p2m_domain {
                                        unsigned long gfn,
                                        mfn_t mfn, unsigned int page_order,
                                        p2m_type_t p2mt,
-                                       p2m_access_t p2ma);
+                                       p2m_access_t p2ma,
+                                       bool_t *sync_deferred);
     mfn_t              (*get_entry   )(struct p2m_domain *p2m,
                                        unsigned long gfn,
                                        p2m_type_t *p2mt,

changeset:   25258:5a0d60bb536b
user:        Aravindh Puthiyaparambil <[email protected]>
date:        Fri Apr 27 21:10:59 2012 -0700
summary:     mem_access: Add xc_hvm_mem_access_bulk() API

tools/libxc/xc_misc.c +16 -0

diff -r 2c05bdb052ea -r 5a0d60bb536b tools/libxc/xc_misc.c
@@ -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

+    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)
{

tools/libxc/xenctrl.h +0 -0

diff -r 2c05bdb052ea -r 5a0d60bb536b tools/libxc/xenctrl.h
@@ -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);

xen/arch/x86/hvm/hvm.c +45 -0

51 lines not on this page · raw message

xen/arch/x86/mm/p2m.c +55 -0

61 lines not on this page · raw message

xen/include/asm-x86/p2m.h +10 -1

diff -r 2c05bdb052ea -r 5a0d60bb536b xen/include/asm-x86/p2m.h
@@ -574,6 +574,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,

@@ -589,7 +594,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

xen/include/public/hvm/hvm_op.h +18 -0

diff -r 2c05bdb052ea -r 5a0d60bb536b xen/include/public/hvm/hvm_op.h
@@ -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__ */

Christian Limpach

2012-05-03 03:28 UTC #13 lore raw

Quoted 9 lines · Aravindh Puthiyaparambil

Quoted 7 lines
Quoted 7 lines
Let me re-iterate:
- if it's a leaf entry, then there's no need to call ept_free_entry
- if you don't need to call ept_free_entry, then you can defer ept_sync_domain (subject to the iommu case)
- you could pass in a pointer to a bool to indicate to the caller that ept_sync_domain was deferred and that the caller needs to do it, with a NULL pointer indicating that the caller doesn't support defer
How does this look?

It's missing the "leaf entry" part of my description of how I think things should work. Without that, you're effectively ignoring the following comment at the end of ept_set_entry:
/* 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. */

See inline comments in your patch below for how I'd change this, untested...

christian

xen/arch/x86/mm/p2m-ept.c ept_set_entry(struct p2m_domain *p2m, un · line 299

Quoted 5 lines

xen/arch/x86/mm/p2m-ept.c +1 -0

@@ -293,6 +296,7 @@ ept_set_entry(struct p2m_domain *p2m, un
     int needs_sync = 1;
     struct domain *d = p2m->domain;
     ept_entry_t old_entry = { .epte = 0 };
+    bool_t _sync_deferred = 0;

don't need that

Quoted 13 lines

xen/arch/x86/mm/p2m-ept.c +0 -0
     /*
      * the caller must make sure:
@@ -309,6 +313,9 @@ ept_set_entry(struct p2m_domain *p2m, un
(target == 1 && hvm_hap_has_2mb(d)) ||
(target == 0));
+ if (sync_deferred)
+ *sync_deferred = 1;
+
table = map_domain_page(ept_get_asr(d));
for ( i = ept_get_wl(d); i > target; i-- )

xen/arch/x86/mm/p2m-ept.c ept_set_entry(struct p2m_domain *p2m, un · line 356

Quoted 4 lines

xen/arch/x86/mm/p2m-ept.c +0 -0

@@ -346,7 +353,11 @@ ept_set_entry(struct p2m_domain *p2m, un
 
         /* No need to flush if the old entry wasn't valid */
         if ( !is_epte_present(ept_entry) )
            needs_sync = 0;

This should be:

        if ( !is_epte_present(ept_entry) ||
             (!target && sync_deferred) ) {
            needs_sync = 0;
            if (sync_deferred)
                *sync_deferred = 0;
        }

This expresses the notion that we're going to skip the call to ept_free_entry since it's a leaf entry (target == 0) and that the caller will make the ept_sync_domain call (sync_deferred != NULL).

xen/arch/x86/mm/p2m.c ept_set_entry(struct p2m_domain *p2m, un · line 401

Quoted 11 lines

xen/arch/x86/mm/p2m.c +3 -0

         /* If we're replacing a non-leaf entry with a leaf entry
 (1GiB or 2MiB),
          * the intermediate tables will be freed below after the ept flush

@@ -385,6 +396,9 @@ ept_set_entry(struct p2m_domain *p2m, un
 
         ASSERT(is_epte_superpage(ept_entry));
 
+        if ( sync_deferred )
+            _sync_deferred = 1;
+

don't need that

Quoted 10 lines

(quoted diff) +0 -0
         split_ept_entry = atomic_read_ept_entry(ept_entry);
 
         if ( !ept_split_super_page(p2m, &split_ept_entry, i, target) )
@@ -438,7 +452,7 @@ ept_set_entry(struct p2m_domain *p2m, un
out:
unmap_domain_page(table);
- if ( needs_sync )
+ if ( needs_sync && !_sync_deferred )
ept_sync_domain(p2m->domain);

don't need that change, test on needs_sync is sufficient

Quoted 3 lines

if ( rv && iommu_enabled && need_iommu(p2m->domain) && need_modify_vtd_table )

Then at the end of ept_set_pte add the test for non-leaf, which is more like an optimization/clarification since ept_free_entry has the same test already.

    if ( target && is_epte_present(&old_entry) )
        ept_free_entry(p2m, &old_entry, target);

Aravindh Puthiyaparambil

author

2012-05-04 22:02 UTC #14 lore raw

Quoted 20 lines · Christian Limpach

Quoted 9 lines · Aravindh Puthiyaparambil
Quoted 7 lines
Quoted 7 lines
Let me re-iterate:
- if it's a leaf entry, then there's no need to call ept_free_entry
- if you don't need to call ept_free_entry, then you can defer ept_sync_domain (subject to the iommu case)
- you could pass in a pointer to a bool to indicate to the caller that ept_sync_domain was deferred and that the caller needs to do it, with a NULL pointer indicating that the caller doesn't support defer
How does this look?
It's missing the "leaf entry" part of my description of how I think things should work. Without that, you're effectively ignoring the following comment at the end of ept_set_entry:
/* 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. */

Quoted 1 line

See inline comments in your patch below for how I'd change this,

untested...

Quoted 37 lines

christian

Review ept_set_entry(struct p2m_domain *p2m, un · line 299

Quoted 5 lines

(quoted diff) +1 -0

@@ -293,6 +296,7 @@ ept_set_entry(struct p2m_domain *p2m, un
    int needs_sync = 1;
    struct domain *d = p2m->domain;
    ept_entry_t old_entry = { .epte = 0 };
+    bool_t _sync_deferred = 0;

don't need that

Quoted 13 lines

(quoted diff) +3 -0

    /*
     * the caller must make sure:

@@ -309,6 +313,9 @@ ept_set_entry(struct p2m_domain *p2m, un
           (target == 1 && hvm_hap_has_2mb(d)) ||
           (target == 0));
 
+    if (sync_deferred)
+        *sync_deferred = 1;
+
    table = map_domain_page(ept_get_asr(d));
 
    for ( i = ept_get_wl(d); i > target; i-- )

Review ept_set_entry(struct p2m_domain *p2m, un · line 356

Quoted 4 lines

(quoted diff) +0 -0

@@ -346,7 +353,11 @@ ept_set_entry(struct p2m_domain *p2m, un
 
        /* No need to flush if the old entry wasn't valid */
        if ( !is_epte_present(ept_entry) )
            needs_sync = 0;

This should be:

       if ( !is_epte_present(ept_entry) ||
            (!target && sync_deferred) ) {
           needs_sync = 0;
           if (sync_deferred)
               *sync_deferred = 0;
       }

Quoted 8 lines

This expresses the notion that we're going to skip the call to ept_free_entry since it's a leaf entry (target == 0) and that the caller will make the ept_sync_domain call (sync_deferred != NULL).
Quoted 4 lines
(quoted diff) +0 -0
        /* If we're replacing a non-leaf entry with a leaf entry
 (1GiB or 2MiB),
         * the intermediate tables will be freed below after the ept

flush

Quoted 27 lines

Review ept_set_entry(struct p2m_domain *p2m, un · line 401
Quoted 7 lines
(quoted diff) +3 -0
@@ -385,6 +396,9 @@ ept_set_entry(struct p2m_domain *p2m, un
 
        ASSERT(is_epte_superpage(ept_entry));
 
+        if ( sync_deferred )
+            _sync_deferred = 1;
+
don't need that
Review ept_set_entry(struct p2m_domain *p2m, un · line 456
Quoted 10 lines
(quoted diff) +1 -1
        split_ept_entry = atomic_read_ept_entry(ept_entry);
 
        if ( !ept_split_super_page(p2m, &split_ept_entry, i, target) )
@@ -438,7 +452,7 @@ ept_set_entry(struct p2m_domain *p2m, un
 out:
    unmap_domain_page(table);
 
-    if ( needs_sync )
+    if ( needs_sync && !_sync_deferred )
        ept_sync_domain(p2m->domain);
don't need that change, test on needs_sync is sufficient
Review ept_set_entry(struct p2m_domain *p2m, un · line 458
Quoted 3 lines
(quoted diff) +0 -0
    if ( rv && iommu_enabled && need_iommu(p2m->domain) &&
 need_modify_vtd_table )

Quoted 6 lines

Then at the end of ept_set_pte add the test for non-leaf, which is more like an optimization/clarification since ept_free_entry has the same test already.
   if ( target && is_epte_present(&old_entry) )
       ept_free_entry(p2m, &old_entry, target);

Sorry for not following and thanks for your patience. Hopefully this looks correct. I have just included the pertinent bit of the patch you were commenting on.

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +12 -2

diff -r 9dda0efd8ce1 -r c180942992bf xen/arch/x86/mm/p2m-ept.c
@@ -274,10 +274,13 @@ static int ept_next_level(struct p2m_dom
 /*
  * ept_set_entry() computes 'need_modify_vtd_table' for itself,
  * by observing whether any gfn->mfn translations are modified.
+ * If sync_deferred is not NULL, then the caller will take care of
+ * calling ept_sync_domain() in the cases where it can be deferred.
  */
 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)
+              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
+              bool_t *sync_deferred)
 {
     ept_entry_t *table, *ept_entry = NULL;
     unsigned long gfn_remainder = gfn;

@@ -309,6 +312,9 @@ ept_set_entry(struct p2m_domain *p2m, un
            (target == 1 && hvm_hap_has_2mb(d)) ||
            (target == 0));
 
+    if (sync_deferred)
+        *sync_deferred = 1;
+
     table = map_domain_page(ept_get_asr(d));
 
     for ( i = ept_get_wl(d); i > target; i-- )

@@ -345,8 +351,12 @@ ept_set_entry(struct p2m_domain *p2m, un
         ept_entry_t new_entry = { .epte = 0 };
 
         /* No need to flush if the old entry wasn't valid */
-        if ( !is_epte_present(ept_entry) )
+        if ( !is_epte_present(ept_entry) || (!target && sync_deferred) )
+        {
             needs_sync = 0;
+            if ( sync_deferred )
+                *sync_deferred = 0;
+        }
 
         /* If we're replacing a non-leaf entry with a leaf entry (1GiB or

2MiB),
* the intermediate tables will be freed below after the ept flush
@@ -477,7 +487,7 @@ out:
last thing we do, after the ept_sync_domain() and removal from the iommu tables, so as to avoid a potential use-after-free. */
- if ( is_epte_present(&old_entry) )
+ if ( target && is_epte_present(&old_entry) )

         ept_free_entry(p2m, &old_entry, target);

     return rv;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Tim Deegan

2012-05-17 10:05 UTC #15 lore raw

Hi Aravindh,

Quoted 19 lines · Aravindh Puthiyaparambil

diff -r 9dda0efd8ce1 -r c180942992bf xen/arch/x86/mm/p2m-ept.c

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +4 -1

@@ -274,10 +274,13 @@ static int ept_next_level(struct p2m_dom
 /*
  * ept_set_entry() computes 'need_modify_vtd_table' for itself,
  * by observing whether any gfn->mfn translations are modified.
+ * If sync_deferred is not NULL, then the caller will take care of
+ * calling ept_sync_domain() in the cases where it can be deferred.
  */
 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)
+              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
+              bool_t *sync_deferred)
 {
     ept_entry_t *table, *ept_entry = NULL;
     unsigned long gfn_remainder = gfn;

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 ept_set_entry(struct p2m_domain *p2m, un · line 315

Quoted 4 lines

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +1 -0
@@ -309,6 +312,9 @@ ept_set_entry(struct p2m_domain *p2m, un
            (target == 1 && hvm_hap_has_2mb(d)) ||
            (target == 0));
 
+    if (sync_deferred)

Xen coding style has spaces inside those parentheses.

Quoted 6 lines

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +2 -0
+        *sync_deferred = 1;
+
     table = map_domain_page(ept_get_asr(d));
 
     for ( i = ept_get_wl(d); i > target; i-- )

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 ept_set_entry(struct p2m_domain *p2m, un · line 358

Quoted 9 lines

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +4 -1

@@ -345,8 +351,12 @@ ept_set_entry(struct p2m_domain *p2m, un
         ept_entry_t new_entry = { .epte = 0 };
 
         /* No need to flush if the old entry wasn't valid */
-        if ( !is_epte_present(ept_entry) )
+        if ( !is_epte_present(ept_entry) || (!target && sync_deferred) )
+        {
             needs_sync = 0;
+            if ( sync_deferred )
+                *sync_deferred = 0;

I don't think you should do this -- you call this function in a loop and you may need to sync because of an earlier iteration. Better to only write a 1 to *sync_deferred once you know there's a sync to be done, and never to write a zero.

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 ept_set_entry(struct p2m_domain *p2m, un · line 359

Quoted 1 line

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +1 -0
+        }

One comment on the rest of the patch: your calling function calls ept_sync_domain() directly if sync_deferred == 1. That's correct for now but it would be cleaner to use a sync() function pointer in the struct p2m_domain, the same as the other implementation-specific calls.

Other than that, I think this looks pretty good to go in after 4.2 has branched.

Cheers,

Tim.

Aravindh Puthiyaparambil

author

2012-05-17 18:43 UTC #16 lore raw

Quoted 37 lines · Tim Deegan

Hi Aravindh,
Quoted 19 lines · Aravindh Puthiyaparambil
diff -r 9dda0efd8ce1 -r c180942992bf xen/arch/x86/mm/p2m-ept.c
xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +4 -1
@@ -274,10 +274,13 @@ static int ept_next_level(struct p2m_dom
 /*
  * ept_set_entry() computes 'need_modify_vtd_table' for itself,
  * by observing whether any gfn->mfn translations are modified.
+ * If sync_deferred is not NULL, then the caller will take care of
+ * calling ept_sync_domain() in the cases where it can be deferred.
  */
 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)
+              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
+              bool_t *sync_deferred)
 {
     ept_entry_t *table, *ept_entry = NULL;
     unsigned long gfn_remainder = gfn;
xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 ept_set_entry(struct p2m_domain *p2m, un · line 315
Quoted 4 lines
xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +1 -0
@@ -309,6 +312,9 @@ ept_set_entry(struct p2m_domain *p2m, un
            (target == 1 && hvm_hap_has_2mb(d)) ||
            (target == 0));
 
+    if (sync_deferred)
Xen coding style has spaces inside those parentheses.
Quoted 8 lines
xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +2 -0
+        *sync_deferred = 1;
+
     table = map_domain_page(ept_get_asr(d));
 
     for ( i = ept_get_wl(d); i > target; i-- )
@@ -345,8 +351,12 @@ ept_set_entry(struct p2m_domain *p2m, un
         ept_entry_t new_entry = { .epte = 0 };
 

Quoted 13 lines

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 ept_set_entry(struct p2m_domain *p2m, un · line 358
Quoted 8 lines
xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +4 -1
         /* No need to flush if the old entry wasn't valid */
-        if ( !is_epte_present(ept_entry) )
+        if ( !is_epte_present(ept_entry) || (!target && sync_deferred)
 )
+        {
             needs_sync = 0;
+            if ( sync_deferred )
+                *sync_deferred = 0;
I don't think you should do this -- you call this function in a loop and you may need to sync because of an earlier iteration. Better to only write a 1 to *sync_deferred once you know there's a sync to be done, and never to write a zero.

You are right. I will fix that.

Quoted 6 lines

xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 ept_set_entry(struct p2m_domain *p2m, un · line 359
Quoted 1 line
xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700 renamed from xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200 +1 -0
+        }
One comment on the rest of the patch: your calling function calls ept_sync_domain() directly if sync_deferred == 1. That's correct for now but it would be cleaner to use a sync() function pointer in the struct p2m_domain, the same as the other implementation-specific calls.

I can add that too.

Quoted 2 lines

Other than that, I think this looks pretty good to go in after 4.2 has branched.

OK, I will resubmit once 4.2 has branched.

Thanks,
Aravindh

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel