[PATCH v2 0/6] gnttab: XSA-134 follow-up

[PATCH v2 0/6] gnttab: XSA-134 follow-up

[Jan Beulich]

1: eliminate several explicit version checks
2: limit mapcount() looping
3: simplify shared entry v1 vs v2 handling
4: simplify page copying/clearing
5: fix/adjust gnttab_transfer()
6: make struct grant_mapping private

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-base on top of David's locking changes that went in now.

[PATCH v2 1/6] gnttab: eliminate several explicit version checks

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4452 bytes --]

By having nr_grant_entries() return zero when the grant table version
is still unset we can reduce the number of error paths and at once fix
grant_map_exists() running into the being removed ASSERT() when called
for a page owned by a domain not having its grant table set up yet.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Said ASSERT() was pointless in the grant_map_exists() case, since the
function only looks at active entries, which are version independent.
Hence this wasn't a security issue.

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -344,11 +344,15 @@ get_maptrack_handle(
 /* Number of grant table entries. Caller must hold d's grant table lock. */
 static unsigned int nr_grant_entries(struct grant_table *gt)
 {
-    ASSERT(gt->gt_version != 0);
-    if (gt->gt_version == 1)
+    switch ( gt->gt_version )
+    {
+    case 1:
         return (nr_grant_frames(gt) << PAGE_SHIFT) / sizeof(grant_entry_v1_t);
-    else
+    case 2:
         return (nr_grant_frames(gt) << PAGE_SHIFT) / sizeof(grant_entry_v2_t);
+    }
+
+    return 0;
 }
 
 static int _set_status_v1(domid_t  domid,
@@ -668,10 +672,6 @@ __gnttab_map_grant_ref(
     rgt = rd->grant_table;
     read_lock(&rgt->lock);
 
-    if ( rgt->gt_version == 0 )
-        PIN_FAIL(unlock_out, GNTST_general_error,
-                 "remote grant table not yet set up\n");
-
     /* Bounds check on the grant ref */
     if ( unlikely(op->ref >= nr_grant_entries(rgt)))
         PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref (%d).\n", op->ref);
@@ -1570,14 +1570,6 @@ gnttab_prepare_for_transfer(
 
     read_lock(&rgt->lock);
 
-    if ( rgt->gt_version == 0 )
-    {
-        gdprintk(XENLOG_INFO,
-                 "Grant table not ready for transfer to domain(%d).\n",
-                 rd->domain_id);
-        goto fail;
-    }
-
     if ( unlikely(ref >= nr_grant_entries(rgt)) )
     {
         gdprintk(XENLOG_INFO,
@@ -1977,10 +1969,6 @@ __acquire_grant_for_copy(
 
     read_lock(&rgt->lock);
 
-    if ( rgt->gt_version == 0 )
-        PIN_FAIL(gt_unlock_out, GNTST_general_error,
-                 "remote grant table not ready\n");
-
     if ( unlikely(gref >= nr_grant_entries(rgt)) )
         PIN_FAIL(gt_unlock_out, GNTST_bad_gntref,
                  "Bad grant reference %ld\n", gref);
@@ -2482,19 +2470,15 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARA
        change the version number, except for the first 8 entries which
        are allowed to be in use (xenstore/xenconsole keeps them mapped).
        (You need to change the version number for e.g. kexec.) */
-    if ( gt->gt_version != 0 )
+    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
     {
-        for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
+        if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
         {
-            if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
-            {
-                gdprintk(XENLOG_WARNING,
-                         "tried to change grant table version from %d to %d, but some grant entries still in use\n",
-                         gt->gt_version,
-                         op.version);
-                res = -EBUSY;
-                goto out_unlock;
-            }
+            gdprintk(XENLOG_WARNING,
+                     "tried to change grant table version from %d to %d, but some grant entries still in use\n",
+                     gt->gt_version, op.version);
+            res = -EBUSY;
+            goto out_unlock;
         }
     }
 
@@ -2679,9 +2663,6 @@ __gnttab_swap_grant_ref(grant_ref_t ref_
 
     write_lock(&gt->lock);
 
-    if ( gt->gt_version == 0 )
-        PIN_FAIL(out, GNTST_general_error, "grant table not yet set up\n");
-
     /* Bounds check on the grant refs */
     if ( unlikely(ref_a >= nr_grant_entries(d->grant_table)))
         PIN_FAIL(out, GNTST_bad_gntref, "Bad ref-a (%d).\n", ref_a);
@@ -3264,9 +3245,6 @@ static void gnttab_usage_print(struct do
 
     read_lock(&gt->lock);
 
-    if ( gt->gt_version == 0 )
-        goto out;
-
     for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
     {
         struct active_grant_entry *act;
@@ -3314,7 +3292,6 @@ static void gnttab_usage_print(struct do
         active_entry_release(act);
     }
 
- out:
     read_unlock(&gt->lock);
 
     if ( first )



[-- Attachment #2: gnttab-nr-grant-entries-v0.patch --]
[-- Type: text/plain, Size: 4501 bytes --]

gnttab: eliminate several explicit version checks

By having nr_grant_entries() return zero when the grant table version
is still unset we can reduce the number of error paths and at once fix
grant_map_exists() running into the being removed ASSERT() when called
for a page owned by a domain not having its grant table set up yet.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Said ASSERT() was pointless in the grant_map_exists() case, since the
function only looks at active entries, which are version independent.
Hence this wasn't a security issue.

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -344,11 +344,15 @@ get_maptrack_handle(
 /* Number of grant table entries. Caller must hold d's grant table lock. */
 static unsigned int nr_grant_entries(struct grant_table *gt)
 {
-    ASSERT(gt->gt_version != 0);
-    if (gt->gt_version == 1)
+    switch ( gt->gt_version )
+    {
+    case 1:
         return (nr_grant_frames(gt) << PAGE_SHIFT) / sizeof(grant_entry_v1_t);
-    else
+    case 2:
         return (nr_grant_frames(gt) << PAGE_SHIFT) / sizeof(grant_entry_v2_t);
+    }
+
+    return 0;
 }
 
 static int _set_status_v1(domid_t  domid,
@@ -668,10 +672,6 @@ __gnttab_map_grant_ref(
     rgt = rd->grant_table;
     read_lock(&rgt->lock);
 
-    if ( rgt->gt_version == 0 )
-        PIN_FAIL(unlock_out, GNTST_general_error,
-                 "remote grant table not yet set up\n");
-
     /* Bounds check on the grant ref */
     if ( unlikely(op->ref >= nr_grant_entries(rgt)))
         PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref (%d).\n", op->ref);
@@ -1570,14 +1570,6 @@ gnttab_prepare_for_transfer(
 
     read_lock(&rgt->lock);
 
-    if ( rgt->gt_version == 0 )
-    {
-        gdprintk(XENLOG_INFO,
-                 "Grant table not ready for transfer to domain(%d).\n",
-                 rd->domain_id);
-        goto fail;
-    }
-
     if ( unlikely(ref >= nr_grant_entries(rgt)) )
     {
         gdprintk(XENLOG_INFO,
@@ -1977,10 +1969,6 @@ __acquire_grant_for_copy(
 
     read_lock(&rgt->lock);
 
-    if ( rgt->gt_version == 0 )
-        PIN_FAIL(gt_unlock_out, GNTST_general_error,
-                 "remote grant table not ready\n");
-
     if ( unlikely(gref >= nr_grant_entries(rgt)) )
         PIN_FAIL(gt_unlock_out, GNTST_bad_gntref,
                  "Bad grant reference %ld\n", gref);
@@ -2482,19 +2470,15 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARA
        change the version number, except for the first 8 entries which
        are allowed to be in use (xenstore/xenconsole keeps them mapped).
        (You need to change the version number for e.g. kexec.) */
-    if ( gt->gt_version != 0 )
+    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
     {
-        for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
+        if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
         {
-            if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
-            {
-                gdprintk(XENLOG_WARNING,
-                         "tried to change grant table version from %d to %d, but some grant entries still in use\n",
-                         gt->gt_version,
-                         op.version);
-                res = -EBUSY;
-                goto out_unlock;
-            }
+            gdprintk(XENLOG_WARNING,
+                     "tried to change grant table version from %d to %d, but some grant entries still in use\n",
+                     gt->gt_version, op.version);
+            res = -EBUSY;
+            goto out_unlock;
         }
     }
 
@@ -2679,9 +2663,6 @@ __gnttab_swap_grant_ref(grant_ref_t ref_
 
     write_lock(&gt->lock);
 
-    if ( gt->gt_version == 0 )
-        PIN_FAIL(out, GNTST_general_error, "grant table not yet set up\n");
-
     /* Bounds check on the grant refs */
     if ( unlikely(ref_a >= nr_grant_entries(d->grant_table)))
         PIN_FAIL(out, GNTST_bad_gntref, "Bad ref-a (%d).\n", ref_a);
@@ -3264,9 +3245,6 @@ static void gnttab_usage_print(struct do
 
     read_lock(&gt->lock);
 
-    if ( gt->gt_version == 0 )
-        goto out;
-
     for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
     {
         struct active_grant_entry *act;
@@ -3314,7 +3292,6 @@ static void gnttab_usage_print(struct do
         active_entry_release(act);
     }
 
- out:
     read_unlock(&gt->lock);
 
     if ( first )

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH v2 2/6] gnttab: limit mapcount() looping

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3850 bytes --]

The function doesn't need to return counts in the first place; all its
callers are after is whether at least one entry of a certain kind
exists. With that there's no point for that loop to continue once the
looked for condition was found to be met by one entry. Rename the
function to match the changed behavior.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -253,7 +253,7 @@ static inline void
 double_gt_lock(struct grant_table *lgt, struct grant_table *rgt)
 {
     /*
-     * See mapcount() for why the write lock is also required for the
+     * See mapkind() for why the write lock is also required for the
      * remote domain.
      */
     if ( lgt < rgt )
@@ -566,14 +566,14 @@ static int grant_map_exists(const struct
     return -EINVAL;
 }
 
-static void mapcount(
-    struct grant_table *lgt, struct domain *rd, unsigned long mfn,
-    unsigned int *wrc, unsigned int *rdc)
+#define MAPKIND_READ 1
+#define MAPKIND_WRITE 2
+static unsigned int mapkind(
+    struct grant_table *lgt, const struct domain *rd, unsigned long mfn)
 {
     struct grant_mapping *map;
     grant_handle_t handle;
-
-    *wrc = *rdc = 0;
+    unsigned int kind = 0;
 
     /*
      * Must have the local domain's grant table write lock when
@@ -586,15 +586,19 @@ static void mapcount(
      */
     ASSERT(rw_is_write_locked(&rd->grant_table->lock));
 
-    for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
+    for ( handle = 0; !(kind & MAPKIND_WRITE) &&
+                      handle < lgt->maptrack_limit; handle++ )
     {
         map = &maptrack_entry(lgt, handle);
         if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ||
              map->domid != rd->domain_id )
             continue;
         if ( _active_entry(rd->grant_table, map->ref).frame == mfn )
-            (map->flags & GNTMAP_readonly) ? (*rdc)++ : (*wrc)++;
+            kind |= map->flags & GNTMAP_readonly ?
+                    MAPKIND_READ : MAPKIND_WRITE;
     }
+
+    return kind;
 }
 
 /*
@@ -819,24 +823,24 @@ __gnttab_map_grant_ref(
     need_iommu = gnttab_need_iommu_mapping(ld);
     if ( need_iommu )
     {
-        unsigned int wrc, rdc;
+        unsigned int kind;
         int err = 0;
 
         double_gt_lock(lgt, rgt);
 
         /* We're not translated, so we know that gmfns and mfns are
            the same things, so the IOMMU entry is always 1-to-1. */
-        mapcount(lgt, rd, frame, &wrc, &rdc);
+        kind = mapkind(lgt, rd, frame);
         if ( (act_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) &&
              !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
         {
-            if ( wrc == 0 )
+            if ( !(kind & MAPKIND_WRITE) )
                 err = iommu_map_page(ld, frame, frame,
                                      IOMMUF_readable|IOMMUF_writable);
         }
         else if ( act_pin && !old_pin )
         {
-            if ( (wrc + rdc) == 0 )
+            if ( !kind )
                 err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
         }
         if ( err )
@@ -1050,15 +1054,15 @@ __gnttab_unmap_common(
 
     if ( rc == GNTST_okay && gnttab_need_iommu_mapping(ld) )
     {
-        unsigned int wrc, rdc;
+        unsigned int kind;
         int err = 0;
 
         double_gt_lock(lgt, rgt);
 
-        mapcount(lgt, rd, op->frame, &wrc, &rdc);
-        if ( (wrc + rdc) == 0 )
+        kind = mapkind(lgt, rd, op->frame);
+        if ( !kind )
             err = iommu_unmap_page(ld, op->frame);
-        else if ( wrc == 0 )
+        else if ( !(kind & MAPKIND_WRITE) )
             err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
 
         double_gt_unlock(lgt, rgt);




[-- Attachment #2: gnttab-limit-mapcount-looping.patch --]
[-- Type: text/plain, Size: 3880 bytes --]

gnttab: limit mapcount() looping

The function doesn't need to return counts in the first place; all its
callers are after is whether at least one entry of a certain kind
exists. With that there's no point for that loop to continue once the
looked for condition was found to be met by one entry. Rename the
function to match the changed behavior.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -253,7 +253,7 @@ static inline void
 double_gt_lock(struct grant_table *lgt, struct grant_table *rgt)
 {
     /*
-     * See mapcount() for why the write lock is also required for the
+     * See mapkind() for why the write lock is also required for the
      * remote domain.
      */
     if ( lgt < rgt )
@@ -566,14 +566,14 @@ static int grant_map_exists(const struct
     return -EINVAL;
 }
 
-static void mapcount(
-    struct grant_table *lgt, struct domain *rd, unsigned long mfn,
-    unsigned int *wrc, unsigned int *rdc)
+#define MAPKIND_READ 1
+#define MAPKIND_WRITE 2
+static unsigned int mapkind(
+    struct grant_table *lgt, const struct domain *rd, unsigned long mfn)
 {
     struct grant_mapping *map;
     grant_handle_t handle;
-
-    *wrc = *rdc = 0;
+    unsigned int kind = 0;
 
     /*
      * Must have the local domain's grant table write lock when
@@ -586,15 +586,19 @@ static void mapcount(
      */
     ASSERT(rw_is_write_locked(&rd->grant_table->lock));
 
-    for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
+    for ( handle = 0; !(kind & MAPKIND_WRITE) &&
+                      handle < lgt->maptrack_limit; handle++ )
     {
         map = &maptrack_entry(lgt, handle);
         if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ||
              map->domid != rd->domain_id )
             continue;
         if ( _active_entry(rd->grant_table, map->ref).frame == mfn )
-            (map->flags & GNTMAP_readonly) ? (*rdc)++ : (*wrc)++;
+            kind |= map->flags & GNTMAP_readonly ?
+                    MAPKIND_READ : MAPKIND_WRITE;
     }
+
+    return kind;
 }
 
 /*
@@ -819,24 +823,24 @@ __gnttab_map_grant_ref(
     need_iommu = gnttab_need_iommu_mapping(ld);
     if ( need_iommu )
     {
-        unsigned int wrc, rdc;
+        unsigned int kind;
         int err = 0;
 
         double_gt_lock(lgt, rgt);
 
         /* We're not translated, so we know that gmfns and mfns are
            the same things, so the IOMMU entry is always 1-to-1. */
-        mapcount(lgt, rd, frame, &wrc, &rdc);
+        kind = mapkind(lgt, rd, frame);
         if ( (act_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) &&
              !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
         {
-            if ( wrc == 0 )
+            if ( !(kind & MAPKIND_WRITE) )
                 err = iommu_map_page(ld, frame, frame,
                                      IOMMUF_readable|IOMMUF_writable);
         }
         else if ( act_pin && !old_pin )
         {
-            if ( (wrc + rdc) == 0 )
+            if ( !kind )
                 err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
         }
         if ( err )
@@ -1050,15 +1054,15 @@ __gnttab_unmap_common(
 
     if ( rc == GNTST_okay && gnttab_need_iommu_mapping(ld) )
     {
-        unsigned int wrc, rdc;
+        unsigned int kind;
         int err = 0;
 
         double_gt_lock(lgt, rgt);
 
-        mapcount(lgt, rd, op->frame, &wrc, &rdc);
-        if ( (wrc + rdc) == 0 )
+        kind = mapkind(lgt, rd, op->frame);
+        if ( !kind )
             err = iommu_unmap_page(ld, op->frame);
-        else if ( wrc == 0 )
+        else if ( !(kind & MAPKIND_WRITE) )
             err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
 
         double_gt_unlock(lgt, rgt);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH v2 3/6] gnttab: simplify shared entry v1 vs v2 handling

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4813 bytes --]

In a number of places both v1 and v2 pointers are being obtained when
none or just one suffices. Additionally in __acquire_grant_for_copy()
the flow of if/else-if can be slightly improved by re-ordering.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -624,8 +624,6 @@ __gnttab_map_grant_ref(
     unsigned int   cache_flags;
     struct active_grant_entry *act = NULL;
     struct grant_mapping *mt;
-    grant_entry_v1_t *sha1;
-    grant_entry_v2_t *sha2;
     grant_entry_header_t *shah;
     uint16_t *status;
     bool_t need_iommu;
@@ -682,15 +680,7 @@ __gnttab_map_grant_ref(
 
     act = active_entry_acquire(rgt, op->ref);
     shah = shared_entry_header(rgt, op->ref);
-    if (rgt->gt_version == 1) {
-        sha1 = &shared_entry_v1(rgt, op->ref);
-        sha2 = NULL;
-        status = &shah->flags;
-    } else {
-        sha2 = &shared_entry_v2(rgt, op->ref);
-        sha1 = NULL;
-        status = &status_entry(rgt, op->ref);
-    }
+    status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, op->ref);
 
     /* If already pinned, check the active domid and avoid refcnt overflow. */
     if ( act->pin &&
@@ -713,8 +703,10 @@ __gnttab_map_grant_ref(
         if ( !act->pin )
         {
             unsigned long frame;
+            unsigned long gfn = rgt->gt_version == 1 ?
+                                shared_entry_v1(rgt, op->ref).frame :
+                                shared_entry_v2(rgt, op->ref).full_page.frame;
 
-            unsigned long gfn = sha1 ? sha1->frame : sha2->full_page.frame;
             rc = __get_paged_frame(gfn, &frame, &pg, 
                                     !!(op->flags & GNTMAP_readonly), rd);
             if ( rc != GNTST_okay )
@@ -1954,7 +1946,6 @@ __acquire_grant_for_copy(
     uint16_t *page_off, uint16_t *length, unsigned allow_transitive)
 {
     struct grant_table *rgt = rd->grant_table;
-    grant_entry_v1_t *sha1;
     grant_entry_v2_t *sha2;
     grant_entry_header_t *shah;
     struct active_grant_entry *act;
@@ -1981,13 +1972,11 @@ __acquire_grant_for_copy(
     shah = shared_entry_header(rgt, gref);
     if ( rgt->gt_version == 1 )
     {
-        sha1 = &shared_entry_v1(rgt, gref);
         sha2 = NULL;
         status = &shah->flags;
     }
     else
     {
-        sha1 = NULL;
         sha2 = &shared_entry_v2(rgt, gref);
         status = &status_entry(rgt, gref);
     }
@@ -2009,7 +1998,19 @@ __acquire_grant_for_copy(
 
         td = rd;
         trans_gref = gref;
-        if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
+        if ( !sha2 )
+        {
+            unsigned long gfn = shared_entry_v1(rgt, gref).frame;
+
+            rc = __get_paged_frame(gfn, &grant_frame, page, readonly, rd);
+            if ( rc != GNTST_okay )
+                goto unlock_out_clear;
+            act->gfn = gfn;
+            is_sub_page = 0;
+            trans_page_off = 0;
+            trans_length = PAGE_SIZE;
+        }
+        else if ( (shah->flags & GTF_type_mask) == GTF_transitive )
         {
             if ( !allow_transitive )
                 PIN_FAIL(unlock_out_clear, GNTST_general_error,
@@ -2082,16 +2083,6 @@ __acquire_grant_for_copy(
             is_sub_page = 1;
             act->gfn = -1ul;
         }
-        else if ( sha1 )
-        {
-            rc = __get_paged_frame(sha1->frame, &grant_frame, page, readonly, rd);
-            if ( rc != GNTST_okay )
-                goto unlock_out_clear;
-            act->gfn = sha1->frame;
-            is_sub_page = 0;
-            trans_page_off = 0;
-            trans_length = PAGE_SIZE;
-        }
         else if ( !(sha2->hdr.flags & GTF_sub_page) )
         {
             rc = __get_paged_frame(sha2->full_page.frame, &grant_frame, page, readonly, rd);
@@ -3253,8 +3244,6 @@ static void gnttab_usage_print(struct do
     {
         struct active_grant_entry *act;
         struct grant_entry_header *sha;
-        grant_entry_v1_t *sha1;
-        grant_entry_v2_t *sha2;
         uint16_t status;
         uint64_t frame;
 
@@ -3269,16 +3258,12 @@ static void gnttab_usage_print(struct do
 
         if ( gt->gt_version == 1 )
         {
-            sha1 = &shared_entry_v1(gt, ref);
-            sha2 = NULL;
             status = sha->flags;
-            frame = sha1->frame;
+            frame = shared_entry_v1(gt, ref).frame;
         }
         else
         {
-            sha2 = &shared_entry_v2(gt, ref);
-            sha1 = NULL;
-            frame = sha2->full_page.frame;
+            frame = shared_entry_v2(gt, ref).full_page.frame;
             status = status_entry(gt, ref);
         }
 



[-- Attachment #2: gnttab-simplify-usage-print.patch --]
[-- Type: text/plain, Size: 4860 bytes --]

gnttab: simplify shared entry v1 vs v2 handling

In a number of places both v1 and v2 pointers are being obtained when
none or just one suffices. Additionally in __acquire_grant_for_copy()
the flow of if/else-if can be slightly improved by re-ordering.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -624,8 +624,6 @@ __gnttab_map_grant_ref(
     unsigned int   cache_flags;
     struct active_grant_entry *act = NULL;
     struct grant_mapping *mt;
-    grant_entry_v1_t *sha1;
-    grant_entry_v2_t *sha2;
     grant_entry_header_t *shah;
     uint16_t *status;
     bool_t need_iommu;
@@ -682,15 +680,7 @@ __gnttab_map_grant_ref(
 
     act = active_entry_acquire(rgt, op->ref);
     shah = shared_entry_header(rgt, op->ref);
-    if (rgt->gt_version == 1) {
-        sha1 = &shared_entry_v1(rgt, op->ref);
-        sha2 = NULL;
-        status = &shah->flags;
-    } else {
-        sha2 = &shared_entry_v2(rgt, op->ref);
-        sha1 = NULL;
-        status = &status_entry(rgt, op->ref);
-    }
+    status = rgt->gt_version == 1 ? &shah->flags : &status_entry(rgt, op->ref);
 
     /* If already pinned, check the active domid and avoid refcnt overflow. */
     if ( act->pin &&
@@ -713,8 +703,10 @@ __gnttab_map_grant_ref(
         if ( !act->pin )
         {
             unsigned long frame;
+            unsigned long gfn = rgt->gt_version == 1 ?
+                                shared_entry_v1(rgt, op->ref).frame :
+                                shared_entry_v2(rgt, op->ref).full_page.frame;
 
-            unsigned long gfn = sha1 ? sha1->frame : sha2->full_page.frame;
             rc = __get_paged_frame(gfn, &frame, &pg, 
                                     !!(op->flags & GNTMAP_readonly), rd);
             if ( rc != GNTST_okay )
@@ -1954,7 +1946,6 @@ __acquire_grant_for_copy(
     uint16_t *page_off, uint16_t *length, unsigned allow_transitive)
 {
     struct grant_table *rgt = rd->grant_table;
-    grant_entry_v1_t *sha1;
     grant_entry_v2_t *sha2;
     grant_entry_header_t *shah;
     struct active_grant_entry *act;
@@ -1981,13 +1972,11 @@ __acquire_grant_for_copy(
     shah = shared_entry_header(rgt, gref);
     if ( rgt->gt_version == 1 )
     {
-        sha1 = &shared_entry_v1(rgt, gref);
         sha2 = NULL;
         status = &shah->flags;
     }
     else
     {
-        sha1 = NULL;
         sha2 = &shared_entry_v2(rgt, gref);
         status = &status_entry(rgt, gref);
     }
@@ -2009,7 +1998,19 @@ __acquire_grant_for_copy(
 
         td = rd;
         trans_gref = gref;
-        if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
+        if ( !sha2 )
+        {
+            unsigned long gfn = shared_entry_v1(rgt, gref).frame;
+
+            rc = __get_paged_frame(gfn, &grant_frame, page, readonly, rd);
+            if ( rc != GNTST_okay )
+                goto unlock_out_clear;
+            act->gfn = gfn;
+            is_sub_page = 0;
+            trans_page_off = 0;
+            trans_length = PAGE_SIZE;
+        }
+        else if ( (shah->flags & GTF_type_mask) == GTF_transitive )
         {
             if ( !allow_transitive )
                 PIN_FAIL(unlock_out_clear, GNTST_general_error,
@@ -2082,16 +2083,6 @@ __acquire_grant_for_copy(
             is_sub_page = 1;
             act->gfn = -1ul;
         }
-        else if ( sha1 )
-        {
-            rc = __get_paged_frame(sha1->frame, &grant_frame, page, readonly, rd);
-            if ( rc != GNTST_okay )
-                goto unlock_out_clear;
-            act->gfn = sha1->frame;
-            is_sub_page = 0;
-            trans_page_off = 0;
-            trans_length = PAGE_SIZE;
-        }
         else if ( !(sha2->hdr.flags & GTF_sub_page) )
         {
             rc = __get_paged_frame(sha2->full_page.frame, &grant_frame, page, readonly, rd);
@@ -3253,8 +3244,6 @@ static void gnttab_usage_print(struct do
     {
         struct active_grant_entry *act;
         struct grant_entry_header *sha;
-        grant_entry_v1_t *sha1;
-        grant_entry_v2_t *sha2;
         uint16_t status;
         uint64_t frame;
 
@@ -3269,16 +3258,12 @@ static void gnttab_usage_print(struct do
 
         if ( gt->gt_version == 1 )
         {
-            sha1 = &shared_entry_v1(gt, ref);
-            sha2 = NULL;
             status = sha->flags;
-            frame = sha1->frame;
+            frame = shared_entry_v1(gt, ref).frame;
         }
         else
         {
-            sha2 = &shared_entry_v2(gt, ref);
-            sha1 = NULL;
-            frame = sha2->full_page.frame;
+            frame = shared_entry_v2(gt, ref).full_page.frame;
             status = status_entry(gt, ref);
         }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH v2 4/6] gnttab: simplify page copying/clearing

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3184 bytes --]

... by making {copy,clear}_domain_page() available also on other than
x86.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -230,24 +230,6 @@ void unmap_domain_page(const void *ptr)
     local_irq_restore(flags);
 }
 
-void clear_domain_page(unsigned long mfn)
-{
-    void *ptr = map_domain_page(mfn);
-
-    clear_page(ptr);
-    unmap_domain_page(ptr);
-}
-
-void copy_domain_page(unsigned long dmfn, unsigned long smfn)
-{
-    const void *src = map_domain_page(smfn);
-    void *dst = map_domain_page(dmfn);
-
-    copy_page(dst, src);
-    unmap_domain_page(dst);
-    unmap_domain_page(src);
-}
-
 int mapcache_domain_init(struct domain *d)
 {
     struct mapcache_domain *dcache = &d->arch.pv_domain.mapcache;
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1712,7 +1712,6 @@ gnttab_transfer(
         if ( (1UL << (max_bitsize - PAGE_SHIFT)) <= mfn )
         {
             struct page_info *new_page;
-            void *sp, *dp;
 
             new_page = alloc_domheap_page(e, MEMF_no_owner |
                                              MEMF_bits(max_bitsize));
@@ -1722,11 +1721,7 @@ gnttab_transfer(
                 goto unlock_and_copyback;
             }
 
-            sp = map_domain_page(mfn);
-            dp = __map_domain_page(new_page);
-            memcpy(dp, sp, PAGE_SIZE);
-            unmap_domain_page(dp);
-            unmap_domain_page(sp);
+            copy_domain_page(page_to_mfn(new_page), mfn);
 
             page->count_info &= ~(PGC_count_mask|PGC_allocated);
             free_domheap_page(page);
@@ -2520,7 +2515,7 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARA
     /* Make sure there's no crud left over in the table from the
        old version. */
     for ( i = 0; i < nr_grant_frames(gt); i++ )
-        memset(gt->shared_raw[i], 0, PAGE_SIZE);
+        clear_page(gt->shared_raw[i]);
 
     /* Restore the first 8 entries (toolstack reserved grants) */
     if ( gt->gt_version != 0 && op.version == 1 )
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1170,6 +1170,26 @@ long do_memory_op(unsigned long cmd, XEN
     return rc;
 }
 
+#ifdef CONFIG_DOMAIN_PAGE
+void clear_domain_page(unsigned long mfn)
+{
+    void *ptr = map_domain_page(mfn);
+
+    clear_page(ptr);
+    unmap_domain_page(ptr);
+}
+
+void copy_domain_page(unsigned long dmfn, unsigned long smfn)
+{
+    const void *src = map_domain_page(smfn);
+    void *dst = map_domain_page(dmfn);
+
+    copy_page(dst, src);
+    unmap_domain_page(dst);
+    unmap_domain_page(src);
+}
+#endif
+
 void destroy_ring_for_helper(
     void **_va, struct page_info *page)
 {
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -264,6 +264,8 @@ static inline lpae_t mfn_to_xen_entry(un
 /* Actual cacheline size on the boot CPU. */
 extern size_t cacheline_bytes;
 
+#define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
+
 /* Functions for flushing medium-sized areas.
  * if 'range' is large enough we might want to use model-specific
  * full-cache flushes. */




[-- Attachment #2: gnttab-simplify-page-ops.patch --]
[-- Type: text/plain, Size: 3220 bytes --]

gnttab: simplify page copying/clearing

... by making {copy,clear}_domain_page() available also on other than
x86.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -230,24 +230,6 @@ void unmap_domain_page(const void *ptr)
     local_irq_restore(flags);
 }
 
-void clear_domain_page(unsigned long mfn)
-{
-    void *ptr = map_domain_page(mfn);
-
-    clear_page(ptr);
-    unmap_domain_page(ptr);
-}
-
-void copy_domain_page(unsigned long dmfn, unsigned long smfn)
-{
-    const void *src = map_domain_page(smfn);
-    void *dst = map_domain_page(dmfn);
-
-    copy_page(dst, src);
-    unmap_domain_page(dst);
-    unmap_domain_page(src);
-}
-
 int mapcache_domain_init(struct domain *d)
 {
     struct mapcache_domain *dcache = &d->arch.pv_domain.mapcache;
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1712,7 +1712,6 @@ gnttab_transfer(
         if ( (1UL << (max_bitsize - PAGE_SHIFT)) <= mfn )
         {
             struct page_info *new_page;
-            void *sp, *dp;
 
             new_page = alloc_domheap_page(e, MEMF_no_owner |
                                              MEMF_bits(max_bitsize));
@@ -1722,11 +1721,7 @@ gnttab_transfer(
                 goto unlock_and_copyback;
             }
 
-            sp = map_domain_page(mfn);
-            dp = __map_domain_page(new_page);
-            memcpy(dp, sp, PAGE_SIZE);
-            unmap_domain_page(dp);
-            unmap_domain_page(sp);
+            copy_domain_page(page_to_mfn(new_page), mfn);
 
             page->count_info &= ~(PGC_count_mask|PGC_allocated);
             free_domheap_page(page);
@@ -2520,7 +2515,7 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARA
     /* Make sure there's no crud left over in the table from the
        old version. */
     for ( i = 0; i < nr_grant_frames(gt); i++ )
-        memset(gt->shared_raw[i], 0, PAGE_SIZE);
+        clear_page(gt->shared_raw[i]);
 
     /* Restore the first 8 entries (toolstack reserved grants) */
     if ( gt->gt_version != 0 && op.version == 1 )
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1170,6 +1170,26 @@ long do_memory_op(unsigned long cmd, XEN
     return rc;
 }
 
+#ifdef CONFIG_DOMAIN_PAGE
+void clear_domain_page(unsigned long mfn)
+{
+    void *ptr = map_domain_page(mfn);
+
+    clear_page(ptr);
+    unmap_domain_page(ptr);
+}
+
+void copy_domain_page(unsigned long dmfn, unsigned long smfn)
+{
+    const void *src = map_domain_page(smfn);
+    void *dst = map_domain_page(dmfn);
+
+    copy_page(dst, src);
+    unmap_domain_page(dst);
+    unmap_domain_page(src);
+}
+#endif
+
 void destroy_ring_for_helper(
     void **_va, struct page_info *page)
 {
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -264,6 +264,8 @@ static inline lpae_t mfn_to_xen_entry(un
 /* Actual cacheline size on the boot CPU. */
 extern size_t cacheline_bytes;
 
+#define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
+
 /* Functions for flushing medium-sized areas.
  * if 'range' is large enough we might want to use model-specific
  * full-cache flushes. */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH v2 5/6] gnttab: fix/adjust gnttab_transfer()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2628 bytes --]

- don't update shared entry's frame number for translated domains (as
  MFNs shouldn't be exposed to such guests)
- for v1 grant table format, force copying of the page also when the
  intended MFN doesn't fit in 32 bits (and the domain isn't translated)
- fix an apparent off-by-one error (it's unclear to me why commit
  5cc77f9098 ("32-on-64: Fix domain address-size clamping, implement")
  uses BITS_PER_LONG-1 here, while using BITS_PER_LONG in the two other
  invocations of domain_clamp_alloc_bitsize())
- adjust comments accompanying the shared entry's frame field

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1708,7 +1708,8 @@ gnttab_transfer(
         }
 
         max_bitsize = domain_clamp_alloc_bitsize(
-            e, BITS_PER_LONG+PAGE_SHIFT-1);
+            e, e->grant_table->gt_version > 1 || paging_mode_translate(e)
+               ? BITS_PER_LONG + PAGE_SHIFT : 32 + PAGE_SHIFT);
         if ( (1UL << (max_bitsize - PAGE_SHIFT)) <= mfn )
         {
             struct page_info *new_page;
@@ -1806,14 +1807,18 @@ gnttab_transfer(
         if ( e->grant_table->gt_version == 1 )
         {
             grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, gop.ref);
+
             guest_physmap_add_page(e, sha->frame, mfn, 0);
-            sha->frame = mfn;
+            if ( !paging_mode_translate(e) )
+                sha->frame = mfn;
         }
         else
         {
             grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table, gop.ref);
+
             guest_physmap_add_page(e, sha->full_page.frame, mfn, 0);
-            sha->full_page.frame = mfn;
+            if ( !paging_mode_translate(e) )
+                sha->full_page.frame = mfn;
         }
         smp_wmb();
         shared_entry_header(e->grant_table, gop.ref)->flags |=
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -134,8 +134,10 @@ struct grant_entry_v1 {
     /* The domain being granted foreign privileges. [GST] */
     domid_t  domid;
     /*
-     * GTF_permit_access: Frame that @domid is allowed to map and access. [GST]
-     * GTF_accept_transfer: Frame whose ownership transferred by @domid. [XEN]
+     * GTF_permit_access: GFN that @domid is allowed to map and access. [GST]
+     * GTF_accept_transfer: GFN that @domid is allowed to transfer into. [GST]
+     * GTF_transfer_completed: MFN whose ownership transferred by @domid
+     *                         (non-translated guests only). [XEN]
      */
     uint32_t frame;
 };




[-- Attachment #2: gnttab-transfer.patch --]
[-- Type: text/plain, Size: 2662 bytes --]

gnttab: fix/adjust gnttab_transfer()

- don't update shared entry's frame number for translated domains (as
  MFNs shouldn't be exposed to such guests)
- for v1 grant table format, force copying of the page also when the
  intended MFN doesn't fit in 32 bits (and the domain isn't translated)
- fix an apparent off-by-one error (it's unclear to me why commit
  5cc77f9098 ("32-on-64: Fix domain address-size clamping, implement")
  uses BITS_PER_LONG-1 here, while using BITS_PER_LONG in the two other
  invocations of domain_clamp_alloc_bitsize())
- adjust comments accompanying the shared entry's frame field

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1708,7 +1708,8 @@ gnttab_transfer(
         }
 
         max_bitsize = domain_clamp_alloc_bitsize(
-            e, BITS_PER_LONG+PAGE_SHIFT-1);
+            e, e->grant_table->gt_version > 1 || paging_mode_translate(e)
+               ? BITS_PER_LONG + PAGE_SHIFT : 32 + PAGE_SHIFT);
         if ( (1UL << (max_bitsize - PAGE_SHIFT)) <= mfn )
         {
             struct page_info *new_page;
@@ -1806,14 +1807,18 @@ gnttab_transfer(
         if ( e->grant_table->gt_version == 1 )
         {
             grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, gop.ref);
+
             guest_physmap_add_page(e, sha->frame, mfn, 0);
-            sha->frame = mfn;
+            if ( !paging_mode_translate(e) )
+                sha->frame = mfn;
         }
         else
         {
             grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table, gop.ref);
+
             guest_physmap_add_page(e, sha->full_page.frame, mfn, 0);
-            sha->full_page.frame = mfn;
+            if ( !paging_mode_translate(e) )
+                sha->full_page.frame = mfn;
         }
         smp_wmb();
         shared_entry_header(e->grant_table, gop.ref)->flags |=
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -134,8 +134,10 @@ struct grant_entry_v1 {
     /* The domain being granted foreign privileges. [GST] */
     domid_t  domid;
     /*
-     * GTF_permit_access: Frame that @domid is allowed to map and access. [GST]
-     * GTF_accept_transfer: Frame whose ownership transferred by @domid. [XEN]
+     * GTF_permit_access: GFN that @domid is allowed to map and access. [GST]
+     * GTF_accept_transfer: GFN that @domid is allowed to transfer into. [GST]
+     * GTF_transfer_completed: MFN whose ownership transferred by @domid
+     *                         (non-translated guests only). [XEN]
      */
     uint32_t frame;
 };

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH v2 6/6] gnttab: make struct grant_mapping private

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1564 bytes --]

This documents that no entity outside of gnttab.c actually accesses
objects of that type, which is particularly important with the now more
fine grained locking in place.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -113,6 +113,16 @@ struct gnttab_unmap_common {
         goto _lbl;                              \
     } while ( 0 )
 
+/*
+ * Tracks a mapping of another domain's grant reference. Each domain has a
+ * table of these, indexes into which are returned as a 'mapping handle'.
+ */
+struct grant_mapping {
+    u32      ref;           /* grant ref */
+    u16      flags;         /* 0-4: GNTMAP_* ; 5-15: unused */
+    domid_t  domid;         /* granting domain */
+};
+
 #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
 #define maptrack_entry(t, e) \
     ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -52,16 +52,6 @@
 /* The maximum size of a grant table. */
 extern unsigned int max_grant_frames;
 
-/*
- * Tracks a mapping of another domain's grant reference. Each domain has a
- * table of these, indexes into which are returned as a 'mapping handle'.
- */
-struct grant_mapping {
-    u32      ref;           /* grant ref */
-    u16      flags;         /* 0-4: GNTMAP_* ; 5-15: unused */
-    domid_t  domid;         /* granting domain */
-};
-
 /* Per-domain grant information. */
 struct grant_table {
     /*




[-- Attachment #2: gnttab-hide-struct-grant_mapping.patch --]
[-- Type: text/plain, Size: 1603 bytes --]

gnttab: make struct grant_mapping private

This documents that no entity outside of gnttab.c actually accesses
objects of that type, which is particularly important with the now more
fine grained locking in place.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -113,6 +113,16 @@ struct gnttab_unmap_common {
         goto _lbl;                              \
     } while ( 0 )
 
+/*
+ * Tracks a mapping of another domain's grant reference. Each domain has a
+ * table of these, indexes into which are returned as a 'mapping handle'.
+ */
+struct grant_mapping {
+    u32      ref;           /* grant ref */
+    u16      flags;         /* 0-4: GNTMAP_* ; 5-15: unused */
+    domid_t  domid;         /* granting domain */
+};
+
 #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
 #define maptrack_entry(t, e) \
     ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -52,16 +52,6 @@
 /* The maximum size of a grant table. */
 extern unsigned int max_grant_frames;
 
-/*
- * Tracks a mapping of another domain's grant reference. Each domain has a
- * table of these, indexes into which are returned as a 'mapping handle'.
- */
-struct grant_mapping {
-    u32      ref;           /* grant ref */
-    u16      flags;         /* 0-4: GNTMAP_* ; 5-15: unused */
-    domid_t  domid;         /* granting domain */
-};
-
 /* Per-domain grant information. */
 struct grant_table {
     /*

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 1/6] gnttab: eliminate several explicit version checks

[Andrew Cooper]

On 15/06/15 13:24, Jan Beulich wrote:
> @@ -1977,10 +1969,6 @@ __acquire_grant_for_copy(
>  
>      read_lock(&rgt->lock);
>  
> -    if ( rgt->gt_version == 0 )
> -        PIN_FAIL(gt_unlock_out, GNTST_general_error,
> -                 "remote grant table not ready\n");
> -
>      if ( unlikely(gref >= nr_grant_entries(rgt)) )
>          PIN_FAIL(gt_unlock_out, GNTST_bad_gntref,
>                   "Bad grant reference %ld\n", gref);
> @@ -2482,19 +2470,15 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARA
>         change the version number, except for the first 8 entries which
>         are allowed to be in use (xenstore/xenconsole keeps them mapped).
>         (You need to change the version number for e.g. kexec.) */
> -    if ( gt->gt_version != 0 )
> +    for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
>      {
> -        for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
> +        if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
>          {
> -            if ( read_atomic(&_active_entry(gt, i).pin) != 0 )
> -            {
> -                gdprintk(XENLOG_WARNING,
> -                         "tried to change grant table version from %d to %d, but some grant entries still in use\n",
> -                         gt->gt_version,
> -                         op.version);
> -                res = -EBUSY;
> -                goto out_unlock;
> -            }
> +            gdprintk(XENLOG_WARNING,
> +                     "tried to change grant table version from %d to %d, but some grant entries still in use\n",
> +                     gt->gt_version, op.version);

As you are moving this printf, both %d should be %u.

As for the main part of the change, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

Re: [PATCH v2 2/6] gnttab: limit mapcount() looping

[Andrew Cooper]

On 15/06/15 13:26, Jan Beulich wrote:
> The function doesn't need to return counts in the first place; all its
> callers are after is whether at least one entry of a certain kind
> exists. With that there's no point for that loop to continue once the
> looked for condition was found to be met by one entry. Rename the
> function to match the changed behavior.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v2 3/6] gnttab: simplify shared entry v1 vs v2 handling

[Andrew Cooper]

On 15/06/15 13:26, Jan Beulich wrote:
> In a number of places both v1 and v2 pointers are being obtained when
> none or just one suffices. Additionally in __acquire_grant_for_copy()
> the flow of if/else-if can be slightly improved by re-ordering.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v2 4/6] gnttab: simplify page copying/clearing

[Andrew Cooper]

On 15/06/15 13:27, Jan Beulich wrote:
> ... by making {copy,clear}_domain_page() available also on other than
> x86.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v2 5/6] gnttab: fix/adjust gnttab_transfer()

[Andrew Cooper]

On 15/06/15 13:28, Jan Beulich wrote:
> - don't update shared entry's frame number for translated domains (as
>   MFNs shouldn't be exposed to such guests)
> - for v1 grant table format, force copying of the page also when the
>   intended MFN doesn't fit in 32 bits (and the domain isn't translated)
> - fix an apparent off-by-one error (it's unclear to me why commit
>   5cc77f9098 ("32-on-64: Fix domain address-size clamping, implement")
>   uses BITS_PER_LONG-1 here, while using BITS_PER_LONG in the two other
>   invocations of domain_clamp_alloc_bitsize())
> - adjust comments accompanying the shared entry's frame field
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...

>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1708,7 +1708,8 @@ gnttab_transfer(
>          }
>  
>          max_bitsize = domain_clamp_alloc_bitsize(
> -            e, BITS_PER_LONG+PAGE_SHIFT-1);
> +            e, e->grant_table->gt_version > 1 || paging_mode_translate(e)

I would recommend two pairs of brackets here for clarity.

~Andrew

Re: [PATCH v2 6/6] gnttab: make struct grant_mapping private

[Andrew Cooper]

On 15/06/15 13:29, Jan Beulich wrote:
> This documents that no entity outside of gnttab.c actually accesses
> objects of that type, which is particularly important with the now more
> fine grained locking in place.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v2 1/6] gnttab: eliminate several explicit version checks

[Ian Campbell]

On Mon, 2015-06-15 at 13:24 +0100, Jan Beulich wrote:
> By having nr_grant_entries() return zero when the grant table version
> is still unset we can reduce the number of error paths and at once fix
> grant_map_exists() running into the being removed ASSERT() when called
> for a page owned by a domain not having its grant table set up yet.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Aced-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2 2/6] gnttab: limit mapcount() looping

[Ian Campbell]

On Mon, 2015-06-15 at 13:26 +0100, Jan Beulich wrote:
> The function doesn't need to return counts in the first place; all its
> callers are after is whether at least one entry of a certain kind
> exists. With that there's no point for that loop to continue once the
> looked for condition was found to be met by one entry. Rename the
> function to match the changed behavior.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2 3/6] gnttab: simplify shared entry v1 vs v2 handling

[Ian Campbell]

On Mon, 2015-06-15 at 13:26 +0100, Jan Beulich wrote:
> In a number of places both v1 and v2 pointers are being obtained when
> none or just one suffices. Additionally in __acquire_grant_for_copy()
> the flow of if/else-if can be slightly improved by re-ordering.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2 4/6] gnttab: simplify page copying/clearing

[Ian Campbell]

On Mon, 2015-06-15 at 13:27 +0100, Jan Beulich wrote:
> ... by making {copy,clear}_domain_page() available also on other than
> x86.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2 5/6] gnttab: fix/adjust gnttab_transfer()

[Ian Campbell]

On Mon, 2015-06-15 at 13:28 +0100, Jan Beulich wrote:
> - don't update shared entry's frame number for translated domains (as
>   MFNs shouldn't be exposed to such guests)
> - for v1 grant table format, force copying of the page also when the
>   intended MFN doesn't fit in 32 bits (and the domain isn't translated)
> - fix an apparent off-by-one error (it's unclear to me why commit
>   5cc77f9098 ("32-on-64: Fix domain address-size clamping, implement")
>   uses BITS_PER_LONG-1 here, while using BITS_PER_LONG in the two other
>   invocations of domain_clamp_alloc_bitsize())
> - adjust comments accompanying the shared entry's frame field
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2 6/6] gnttab: make struct grant_mapping private

[Ian Campbell]

On Mon, 2015-06-15 at 13:29 +0100, Jan Beulich wrote:
> This documents that no entity outside of gnttab.c actually accesses
> objects of that type, which is particularly important with the now more
> fine grained locking in place.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>