All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] gnttab: recent XSA follow-up
@ 2017-08-15 14:15 Jan Beulich
  2017-08-15 14:38 ` [PATCH RESEND 1/8] gnttab: drop useless locking Jan Beulich
                   ` (7 more replies)
  0 siblings, 8 replies; 18+ messages in thread
From: Jan Beulich @ 2017-08-15 14:15 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan

1: drop useless locking
2: avoid spurious maptrack handle allocation failures
3: type adjustments
4: drop pointless leading double underscores
5: re-arrange struct active_grant_entry
6: move GNTPIN_* out of header file
7: use DIV_ROUND_UP() instead of open-coding it
8: drop struct active_grant_entry's gfn field for release builds

In case it matters this series applies on top on the XSA-226 pair of
patches sent earlier today.

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


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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH RESEND 1/8] gnttab: drop useless locking
  2017-08-15 14:15 [PATCH 0/8] gnttab: recent XSA follow-up Jan Beulich
@ 2017-08-15 14:38 ` Jan Beulich
  2017-08-15 16:08   ` Andrew Cooper
  2017-08-15 14:39 ` [PATCH RESEND 2/8] gnttab: avoid spurious maptrack handle allocation failures Jan Beulich
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2017-08-15 14:38 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan

Holding any lock while accessing the maptrack entry fields is
pointless, as these entries are protected by their associated active
entry lock (which is being acquired later, before re-validating the
fields read without holding the lock).

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

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1140,19 +1140,14 @@ __gnttab_unmap_common(
     smp_rmb();
     map = &maptrack_entry(lgt, op->handle);
 
-    grant_read_lock(lgt);
-
     if ( unlikely(!read_atomic(&map->flags)) )
     {
-        grant_read_unlock(lgt);
         gdprintk(XENLOG_INFO, "Zero flags for handle %#x\n", op->handle);
         op->status = GNTST_bad_handle;
         return;
     }
 
     dom = map->domid;
-    grant_read_unlock(lgt);
-
     if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) )
     {
         /* This can happen when a grant is implicitly unmapped. */




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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH RESEND 2/8] gnttab: avoid spurious maptrack handle allocation failures
  2017-08-15 14:15 [PATCH 0/8] gnttab: recent XSA follow-up Jan Beulich
  2017-08-15 14:38 ` [PATCH RESEND 1/8] gnttab: drop useless locking Jan Beulich
@ 2017-08-15 14:39 ` Jan Beulich
  2017-08-15 16:52   ` Andrew Cooper
  2017-08-15 14:40 ` [PATCH 3/8] gnttab: type adjustments Jan Beulich
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2017-08-15 14:39 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan

When no memory is available in the hypervisor, rather than immediately
failing the request, try to steal a handle from another vCPU.

Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -411,7 +411,7 @@ get_maptrack_handle(
     struct vcpu          *curr = current;
     unsigned int          i, head;
     grant_handle_t        handle;
-    struct grant_mapping *new_mt;
+    struct grant_mapping *new_mt = NULL;
 
     handle = __get_maptrack_handle(lgt, curr);
     if ( likely(handle != -1) )
@@ -422,8 +422,13 @@ get_maptrack_handle(
     /*
      * If we've run out of frames, try stealing an entry from another
      * VCPU (in case the guest isn't mapping across its VCPUs evenly).
+     * Also use this path in case we're out of memory, to avoid spurious
+     * failures.
      */
-    if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
+    if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
+        new_mt = alloc_xenheap_page();
+
+    if ( !new_mt )
     {
         spin_unlock(&lgt->maptrack_lock);
 
@@ -446,12 +451,6 @@ get_maptrack_handle(
         return steal_maptrack_handle(lgt, curr);
     }
 
-    new_mt = alloc_xenheap_page();
-    if ( !new_mt )
-    {
-        spin_unlock(&lgt->maptrack_lock);
-        return -1;
-    }
     clear_page(new_mt);
 
     /*




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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH 3/8] gnttab: type adjustments
  2017-08-15 14:15 [PATCH 0/8] gnttab: recent XSA follow-up Jan Beulich
  2017-08-15 14:38 ` [PATCH RESEND 1/8] gnttab: drop useless locking Jan Beulich
  2017-08-15 14:39 ` [PATCH RESEND 2/8] gnttab: avoid spurious maptrack handle allocation failures Jan Beulich
@ 2017-08-15 14:40 ` Jan Beulich
  2017-08-15 17:04   ` Andrew Cooper
  2017-08-15 14:40 ` [PATCH 4/8] gnttab: drop pointless leading double underscores Jan Beulich
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2017-08-15 14:40 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan

In particular use grant_ref_t and grant_handle_t where appropriate.
Also switch other nearby type uses to their canonical variants where
appropriate and introduce INVALID_MAPTRACK_HANDLE.

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

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -96,7 +96,7 @@ struct gnttab_unmap_common {
     int16_t status;
 
     /* Shared state beteen *_unmap and *_unmap_complete */
-    u16 done;
+    uint16_t done;
     unsigned long frame;
     struct domain *rd;
     grant_ref_t ref;
@@ -118,11 +118,11 @@ struct gnttab_unmap_common {
  * 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 */
+    grant_ref_t ref;        /* grant ref */
+    uint16_t flags;         /* 0-4: GNTMAP_* ; 5-15: unused */
     domid_t  domid;         /* granting domain */
-    u32      vcpu;          /* vcpu which created the grant mapping */
-    u32      pad;           /* round size to a power of 2 */
+    uint32_t vcpu;          /* vcpu which created the grant mapping */
+    uint32_t pad;           /* round size to a power of 2 */
 };
 
 #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
@@ -158,10 +158,10 @@ shared_entry_header(struct grant_table *
 
 /* Active grant entry - used for shadowing GTF_permit_access grants. */
 struct active_grant_entry {
-    u32           pin;    /* Reference count information.             */
+    uint32_t      pin;    /* Reference count information.             */
     domid_t       domid;  /* Domain being granted access.             */
     struct domain *trans_domain;
-    uint32_t      trans_gref;
+    grant_ref_t   trans_gref;
     unsigned long frame;  /* Frame being granted.                     */
     unsigned long gfn;    /* Guest's idea of the frame being granted. */
     unsigned      is_sub_page:1; /* True if this is a sub-page grant. */
@@ -297,7 +297,9 @@ double_gt_unlock(struct grant_table *lgt
         grant_write_unlock(rgt);
 }
 
-static inline int
+#define INVALID_MAPTRACK_HANDLE UINT_MAX
+
+static inline grant_handle_t
 __get_maptrack_handle(
     struct grant_table *t,
     struct vcpu *v)
@@ -312,7 +314,7 @@ __get_maptrack_handle(
         if ( unlikely(head == MAPTRACK_TAIL) )
         {
             spin_unlock(&v->maptrack_freelist_lock);
-            return -1;
+            return INVALID_MAPTRACK_HANDLE;
         }
 
         /*
@@ -323,7 +325,7 @@ __get_maptrack_handle(
         if ( unlikely(next == MAPTRACK_TAIL) )
         {
             spin_unlock(&v->maptrack_freelist_lock);
-            return -1;
+            return INVALID_MAPTRACK_HANDLE;
         }
 
         prev_head = head;
@@ -345,8 +347,8 @@ __get_maptrack_handle(
  * each VCPU and to avoid two VCPU repeatedly stealing entries from
  * each other, the initial victim VCPU is selected randomly.
  */
-static int steal_maptrack_handle(struct grant_table *t,
-                                 const struct vcpu *curr)
+static grant_handle_t steal_maptrack_handle(struct grant_table *t,
+                                            const struct vcpu *curr)
 {
     const struct domain *currd = curr->domain;
     unsigned int first, i;
@@ -357,10 +359,10 @@ static int steal_maptrack_handle(struct
     do {
         if ( currd->vcpu[i] )
         {
-            int handle;
+            grant_handle_t handle;
 
             handle = __get_maptrack_handle(t, currd->vcpu[i]);
-            if ( handle != -1 )
+            if ( handle != INVALID_MAPTRACK_HANDLE )
             {
                 maptrack_entry(t, handle).vcpu = curr->vcpu_id;
                 return handle;
@@ -373,12 +375,12 @@ static int steal_maptrack_handle(struct
     } while ( i != first );
 
     /* No free handles on any VCPU. */
-    return -1;
+    return INVALID_MAPTRACK_HANDLE;
 }
 
 static inline void
 put_maptrack_handle(
-    struct grant_table *t, int handle)
+    struct grant_table *t, grant_handle_t handle)
 {
     struct domain *currd = current->domain;
     struct vcpu *v;
@@ -404,7 +406,7 @@ put_maptrack_handle(
     spin_unlock(&v->maptrack_freelist_lock);
 }
 
-static inline int
+static inline grant_handle_t
 get_maptrack_handle(
     struct grant_table *lgt)
 {
@@ -414,7 +416,7 @@ get_maptrack_handle(
     struct grant_mapping *new_mt = NULL;
 
     handle = __get_maptrack_handle(lgt, curr);
-    if ( likely(handle != -1) )
+    if ( likely(handle != INVALID_MAPTRACK_HANDLE) )
         return handle;
 
     spin_lock(&lgt->maptrack_lock);
@@ -439,8 +441,8 @@ get_maptrack_handle(
         if ( curr->maptrack_tail == MAPTRACK_TAIL )
         {
             handle = steal_maptrack_handle(lgt, curr);
-            if ( handle == -1 )
-                return -1;
+            if ( handle == INVALID_MAPTRACK_HANDLE )
+                return handle;
             spin_lock(&curr->maptrack_freelist_lock);
             maptrack_entry(lgt, handle).ref = MAPTRACK_TAIL;
             curr->maptrack_tail = handle;
@@ -461,6 +463,7 @@ get_maptrack_handle(
 
     for ( i = 0; i < MAPTRACK_PER_PAGE; i++ )
     {
+        BUILD_BUG_ON(sizeof(new_mt->ref) < sizeof(handle));
         new_mt[i].ref = handle + i + 1;
         new_mt[i].vcpu = curr->vcpu_id;
     }
@@ -683,9 +686,9 @@ static int _set_status(unsigned gt_versi
 static int grant_map_exists(const struct domain *ld,
                             struct grant_table *rgt,
                             unsigned long mfn,
-                            unsigned int *ref_count)
+                            grant_ref_t *cur_ref)
 {
-    unsigned int ref, max_iter;
+    grant_ref_t ref, max_iter;
     
     /*
      * The remote grant table should be locked but the percpu rwlock
@@ -695,9 +698,9 @@ static int grant_map_exists(const struct
      *   ASSERT(rw_is_locked(&rgt->lock));
      */
 
-    max_iter = min(*ref_count + (1 << GNTTABOP_CONTINUATION_ARG_SHIFT),
+    max_iter = min(*cur_ref + (1 << GNTTABOP_CONTINUATION_ARG_SHIFT),
                    nr_grant_entries(rgt));
-    for ( ref = *ref_count; ref < max_iter; ref++ )
+    for ( ref = *cur_ref; ref < max_iter; ref++ )
     {
         struct active_grant_entry *act;
         bool_t exists;
@@ -716,7 +719,7 @@ static int grant_map_exists(const struct
 
     if ( ref < nr_grant_entries(rgt) )
     {
-        *ref_count = ref;
+        *cur_ref = ref;
         return 1;
     }
 
@@ -773,7 +776,7 @@ __gnttab_map_grant_ref(
     struct domain *ld, *rd, *owner = NULL;
     struct grant_table *lgt, *rgt;
     struct vcpu   *led;
-    int            handle;
+    grant_handle_t handle;
     unsigned long  frame = 0;
     struct page_info *pg = NULL;
     int            rc = GNTST_okay;
@@ -822,7 +825,8 @@ __gnttab_map_grant_ref(
     }
 
     lgt = ld->grant_table;
-    if ( unlikely((handle = get_maptrack_handle(lgt)) == -1) )
+    handle = get_maptrack_handle(lgt);
+    if ( unlikely(handle == INVALID_MAPTRACK_HANDLE) )
     {
         rcu_unlock_domain(rd);
         gdprintk(XENLOG_INFO, "Failed to obtain maptrack handle.\n");
@@ -2038,7 +2042,7 @@ gnttab_transfer(
    type and reference counts. */
 static void
 __release_grant_for_copy(
-    struct domain *rd, unsigned long gref, int readonly)
+    struct domain *rd, grant_ref_t gref, bool readonly)
 {
     struct grant_table *rgt = rd->grant_table;
     grant_entry_header_t *sha;
@@ -2119,9 +2123,9 @@ static void __fixup_status_for_copy_pin(
    If there is any error, *page = NULL, no ref taken. */
 static int
 __acquire_grant_for_copy(
-    struct domain *rd, unsigned long gref, domid_t ldom, int readonly,
+    struct domain *rd, grant_ref_t gref, domid_t ldom, bool readonly,
     unsigned long *frame, struct page_info **page, 
-    uint16_t *page_off, uint16_t *length, unsigned allow_transitive)
+    uint16_t *page_off, uint16_t *length, bool allow_transitive)
 {
     struct grant_table *rgt = rd->grant_table;
     grant_entry_v2_t *sha2;
@@ -2144,7 +2148,7 @@ __acquire_grant_for_copy(
 
     if ( unlikely(gref >= nr_grant_entries(rgt)) )
         PIN_FAIL(gt_unlock_out, GNTST_bad_gntref,
-                 "Bad grant reference %ld\n", gref);
+                 "Bad grant reference %#x\n", gref);
 
     act = active_entry_acquire(rgt, gref);
     shah = shared_entry_header(rgt, gref);
@@ -2211,7 +2215,8 @@ __acquire_grant_for_copy(
 
         rc = __acquire_grant_for_copy(td, trans_gref, rd->domain_id,
                                       readonly, &grant_frame, page,
-                                      &trans_page_off, &trans_length, 0);
+                                      &trans_page_off, &trans_length,
+                                      false);
 
         grant_read_lock(rgt);
         act = active_entry_acquire(rgt, gref);
@@ -2257,7 +2262,7 @@ __acquire_grant_for_copy(
             act->trans_domain = td;
             act->trans_gref = trans_gref;
             act->frame = grant_frame;
-            act->gfn = -1ul;
+            act->gfn = gfn_x(INVALID_GFN);
             /*
              * The actual remote remote grant may or may not be a sub-page,
              * but we always treat it as one because that blocks mappings of
@@ -2374,12 +2379,12 @@ struct gnttab_copy_buf {
     bool_t have_type;
 };
 
-static int gnttab_copy_lock_domain(domid_t domid, unsigned int gref_flag,
+static int gnttab_copy_lock_domain(domid_t domid, bool is_gref,
                                    struct gnttab_copy_buf *buf)
 {
     int rc;
 
-    if ( domid != DOMID_SELF && !gref_flag )
+    if ( domid != DOMID_SELF && !is_gref )
         PIN_FAIL(out, GNTST_permission_denied,
                  "only allow copy-by-mfn for DOMID_SELF.\n");
 
@@ -2480,7 +2485,7 @@ static int gnttab_copy_claim_buf(const s
                                       current->domain->domain_id,
                                       buf->read_only,
                                       &buf->frame, &buf->page,
-                                      &buf->ptr.offset, &buf->len, 1);
+                                      &buf->ptr.offset, &buf->len, true);
         if ( rc != GNTST_okay )
             goto out;
         buf->ptr.u.ref = ptr->u.ref;
@@ -2985,7 +2990,7 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_P
 }
 
 static int __gnttab_cache_flush(gnttab_cache_flush_t *cflush,
-                                unsigned int *ref_count)
+                                grant_ref_t *cur_ref)
 {
     struct domain *d, *owner;
     struct page_info *page;
@@ -3029,7 +3034,7 @@ static int __gnttab_cache_flush(gnttab_c
     {
         grant_read_lock(owner->grant_table);
 
-        ret = grant_map_exists(d, owner->grant_table, mfn, ref_count);
+        ret = grant_map_exists(d, owner->grant_table, mfn, cur_ref);
         if ( ret != 0 )
         {
             grant_read_unlock(owner->grant_table);
@@ -3061,7 +3066,7 @@ static int __gnttab_cache_flush(gnttab_c
 
 static long
 gnttab_cache_flush(XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) uop,
-                      unsigned int *ref_count,
+                      grant_ref_t *cur_ref,
                       unsigned int count)
 {
     unsigned int i;
@@ -3075,7 +3080,7 @@ gnttab_cache_flush(XEN_GUEST_HANDLE_PARA
             return -EFAULT;
         for ( ; ; )
         {
-            int ret = __gnttab_cache_flush(&op, ref_count);
+            int ret = __gnttab_cache_flush(&op, cur_ref);
 
             if ( ret < 0 )
                 return ret;
@@ -3084,7 +3089,7 @@ gnttab_cache_flush(XEN_GUEST_HANDLE_PARA
             if ( hypercall_preempt_check() )
                 return i;
         }
-        *ref_count = 0;
+        *cur_ref = 0;
         guest_handle_add_offset(uop, 1);
     }
     return 0;



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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH 4/8] gnttab: drop pointless leading double underscores
  2017-08-15 14:15 [PATCH 0/8] gnttab: recent XSA follow-up Jan Beulich
                   ` (2 preceding siblings ...)
  2017-08-15 14:40 ` [PATCH 3/8] gnttab: type adjustments Jan Beulich
@ 2017-08-15 14:40 ` Jan Beulich
  2017-08-15 17:09   ` Andrew Cooper
  2017-08-15 14:41 ` [PATCH 5/8] gnttab: re-arrange struct active_grant_entry Jan Beulich
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2017-08-15 14:40 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan

They're violating name space rules, and we don't really need them. When
followed by "gnttab_", also drop that.

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

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -233,8 +233,9 @@ static inline void active_entry_release(
    If rc == GNTST_okay, *page contains the page struct with a ref taken.
    Caller must do put_page(*page).
    If any error, *page = NULL, *frame = INVALID_MFN, no ref taken. */
-static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct page_info **page,
-                                int readonly, struct domain *rd)
+static int get_paged_frame(unsigned long gfn, unsigned long *frame,
+                           struct page_info **page, bool readonly,
+                           struct domain *rd)
 {
     int rc = GNTST_okay;
 #if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
@@ -300,7 +301,7 @@ double_gt_unlock(struct grant_table *lgt
 #define INVALID_MAPTRACK_HANDLE UINT_MAX
 
 static inline grant_handle_t
-__get_maptrack_handle(
+_get_maptrack_handle(
     struct grant_table *t,
     struct vcpu *v)
 {
@@ -361,7 +362,7 @@ static grant_handle_t steal_maptrack_han
         {
             grant_handle_t handle;
 
-            handle = __get_maptrack_handle(t, currd->vcpu[i]);
+            handle = _get_maptrack_handle(t, currd->vcpu[i]);
             if ( handle != INVALID_MAPTRACK_HANDLE )
             {
                 maptrack_entry(t, handle).vcpu = curr->vcpu_id;
@@ -415,7 +416,7 @@ get_maptrack_handle(
     grant_handle_t        handle;
     struct grant_mapping *new_mt = NULL;
 
-    handle = __get_maptrack_handle(lgt, curr);
+    handle = _get_maptrack_handle(lgt, curr);
     if ( likely(handle != INVALID_MAPTRACK_HANDLE) )
         return handle;
 
@@ -770,7 +771,7 @@ static unsigned int mapkind(
  * update, as indicated by the GNTMAP_contains_pte flag.
  */
 static void
-__gnttab_map_grant_ref(
+map_grant_ref(
     struct gnttab_map_grant_ref *op)
 {
     struct domain *ld, *rd, *owner = NULL;
@@ -869,8 +870,8 @@ __gnttab_map_grant_ref(
                                 shared_entry_v1(rgt, op->ref).frame :
                                 shared_entry_v2(rgt, op->ref).full_page.frame;
 
-            rc = __get_paged_frame(gfn, &frame, &pg, 
-                                    !!(op->flags & GNTMAP_readonly), rd);
+            rc = get_paged_frame(gfn, &frame, &pg,
+                                 op->flags & GNTMAP_readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
             act->gfn = gfn;
@@ -900,7 +901,7 @@ __gnttab_map_grant_ref(
     active_entry_release(act);
     grant_read_unlock(rgt);
 
-    /* pg may be set, with a refcount included, from __get_paged_frame */
+    /* pg may be set, with a refcount included, from get_paged_frame(). */
     if ( !pg )
     {
         pg = mfn_valid(_mfn(frame)) ? mfn_to_page(frame) : NULL;
@@ -1109,7 +1110,7 @@ gnttab_map_grant_ref(
             return i;
         if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
             return -EFAULT;
-        __gnttab_map_grant_ref(&op);
+        map_grant_ref(&op);
         if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )
             return -EFAULT;
     }
@@ -1118,7 +1119,7 @@ gnttab_map_grant_ref(
 }
 
 static void
-__gnttab_unmap_common(
+unmap_common(
     struct gnttab_unmap_common *op)
 {
     domid_t          dom;
@@ -1178,8 +1179,8 @@ __gnttab_unmap_common(
         /*
          * This ought to be impossible, as such a mapping should not have
          * been established (see the nr_grant_entries(rgt) bounds check in
-         * __gnttab_map_grant_ref()). Doing this check only in
-         * __gnttab_unmap_common_complete() - as it used to be done - would,
+         * gnttab_map_grant_ref()). Doing this check only in
+         * gnttab_unmap_common_complete() - as it used to be done - would,
          * however, be too late.
          */
         rc = GNTST_bad_gntref;
@@ -1293,7 +1294,7 @@ __gnttab_unmap_common(
 }
 
 static void
-__gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
+unmap_common_complete(struct gnttab_unmap_common *op)
 {
     struct domain *ld, *rd = op->rd;
     struct grant_table *rgt;
@@ -1304,7 +1305,7 @@ __gnttab_unmap_common_complete(struct gn
 
     if ( !op->done )
     { 
-        /* __gntab_unmap_common() didn't do anything - nothing to complete. */
+        /* unmap_common() didn't do anything - nothing to complete. */
         return;
     }
 
@@ -1373,7 +1374,7 @@ __gnttab_unmap_common_complete(struct gn
 }
 
 static void
-__gnttab_unmap_grant_ref(
+unmap_grant_ref(
     struct gnttab_unmap_grant_ref *op,
     struct gnttab_unmap_common *common)
 {
@@ -1387,7 +1388,7 @@ __gnttab_unmap_grant_ref(
     common->rd = NULL;
     common->frame = 0;
 
-    __gnttab_unmap_common(common);
+    unmap_common(common);
     op->status = common->status;
 }
 
@@ -1409,7 +1410,7 @@ gnttab_unmap_grant_ref(
         {
             if ( unlikely(__copy_from_guest(&op, uop, 1)) )
                 goto fault;
-            __gnttab_unmap_grant_ref(&op, &(common[i]));
+            unmap_grant_ref(&op, &common[i]);
             ++partial_done;
             if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
                 goto fault;
@@ -1419,7 +1420,7 @@ gnttab_unmap_grant_ref(
         gnttab_flush_tlb(current->domain);
 
         for ( i = 0; i < partial_done; i++ )
-            __gnttab_unmap_common_complete(&(common[i]));
+            unmap_common_complete(&common[i]);
 
         count -= c;
         done += c;
@@ -1434,12 +1435,12 @@ fault:
     gnttab_flush_tlb(current->domain);
 
     for ( i = 0; i < partial_done; i++ )
-        __gnttab_unmap_common_complete(&(common[i]));
+        unmap_common_complete(&common[i]);
     return -EFAULT;
 }
 
 static void
-__gnttab_unmap_and_replace(
+unmap_and_replace(
     struct gnttab_unmap_and_replace *op,
     struct gnttab_unmap_common *common)
 {
@@ -1453,7 +1454,7 @@ __gnttab_unmap_and_replace(
     common->rd = NULL;
     common->frame = 0;
 
-    __gnttab_unmap_common(common);
+    unmap_common(common);
     op->status = common->status;
 }
 
@@ -1474,7 +1475,7 @@ gnttab_unmap_and_replace(
         {
             if ( unlikely(__copy_from_guest(&op, uop, 1)) )
                 goto fault;
-            __gnttab_unmap_and_replace(&op, &(common[i]));
+            unmap_and_replace(&op, &common[i]);
             ++partial_done;
             if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
                 goto fault;
@@ -1484,7 +1485,7 @@ gnttab_unmap_and_replace(
         gnttab_flush_tlb(current->domain);
         
         for ( i = 0; i < partial_done; i++ )
-            __gnttab_unmap_common_complete(&(common[i]));
+            unmap_common_complete(&common[i]);
 
         count -= c;
         done += c;
@@ -1499,7 +1500,7 @@ fault:
     gnttab_flush_tlb(current->domain);
 
     for ( i = 0; i < partial_done; i++ )
-        __gnttab_unmap_common_complete(&(common[i]));
+        unmap_common_complete(&common[i]);
     return -EFAULT;    
 }
 
@@ -1849,9 +1850,10 @@ gnttab_transfer(
 
 #ifdef CONFIG_X86
         {
-            p2m_type_t __p2mt;
-            mfn = mfn_x(get_gfn_unshare(d, gop.mfn, &__p2mt));
-            if ( p2m_is_shared(__p2mt) || !p2m_is_valid(__p2mt) )
+            p2m_type_t p2mt;
+
+            mfn = mfn_x(get_gfn_unshare(d, gop.mfn, &p2mt));
+            if ( p2m_is_shared(p2mt) || !p2m_is_valid(p2mt) )
                 mfn = mfn_x(INVALID_MFN);
         }
 #else
@@ -2038,10 +2040,12 @@ gnttab_transfer(
     return 0;
 }
 
-/* Undo __acquire_grant_for_copy.  Again, this has no effect on page
-   type and reference counts. */
+/*
+ * Undo acquire_grant_for_copy().  This has no effect on page type and
+ * reference counts.
+ */
 static void
-__release_grant_for_copy(
+release_grant_for_copy(
     struct domain *rd, grant_ref_t gref, bool readonly)
 {
     struct grant_table *rgt = rd->grant_table;
@@ -2096,7 +2100,7 @@ __release_grant_for_copy(
          * Recursive call, but it is bounded (acquire permits only a single
          * level of transitivity), so it's okay.
          */
-        __release_grant_for_copy(td, trans_gref, readonly);
+        release_grant_for_copy(td, trans_gref, readonly);
 
         rcu_unlock_domain(td);
     }
@@ -2107,8 +2111,8 @@ __release_grant_for_copy(
    under the domain's grant table lock. */
 /* Only safe on transitive grants.  Even then, note that we don't
    attempt to drop any pin on the referent grant. */
-static void __fixup_status_for_copy_pin(const struct active_grant_entry *act,
-                                   uint16_t *status)
+static void fixup_status_for_copy_pin(const struct active_grant_entry *act,
+                                      uint16_t *status)
 {
     if ( !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
         gnttab_clear_flag(_GTF_writing, status);
@@ -2122,7 +2126,7 @@ static void __fixup_status_for_copy_pin(
    take one ref count on the target page, stored in *page.
    If there is any error, *page = NULL, no ref taken. */
 static int
-__acquire_grant_for_copy(
+acquire_grant_for_copy(
     struct domain *rd, grant_ref_t gref, domid_t ldom, bool readonly,
     unsigned long *frame, struct page_info **page, 
     uint16_t *page_off, uint16_t *length, bool allow_transitive)
@@ -2206,24 +2210,24 @@ __acquire_grant_for_copy(
                      trans_domid);
 
         /*
-         * __acquire_grant_for_copy() could take the lock on the
+         * acquire_grant_for_copy() could take the lock on the
          * remote table (if rd == td), so we have to drop the lock
          * here and reacquire.
          */
         active_entry_release(act);
         grant_read_unlock(rgt);
 
-        rc = __acquire_grant_for_copy(td, trans_gref, rd->domain_id,
-                                      readonly, &grant_frame, page,
-                                      &trans_page_off, &trans_length,
-                                      false);
+        rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id,
+                                    readonly, &grant_frame, page,
+                                    &trans_page_off, &trans_length,
+                                    false);
 
         grant_read_lock(rgt);
         act = active_entry_acquire(rgt, gref);
 
         if ( rc != GNTST_okay )
         {
-            __fixup_status_for_copy_pin(act, status);
+            fixup_status_for_copy_pin(act, status);
             rcu_unlock_domain(td);
             active_entry_release(act);
             grant_read_unlock(rgt);
@@ -2244,8 +2248,8 @@ __acquire_grant_for_copy(
                           act->trans_gref != trans_gref ||
                           !act->is_sub_page)) )
         {
-            __release_grant_for_copy(td, trans_gref, readonly);
-            __fixup_status_for_copy_pin(act, status);
+            release_grant_for_copy(td, trans_gref, readonly);
+            fixup_status_for_copy_pin(act, status);
             rcu_unlock_domain(td);
             active_entry_release(act);
             grant_read_unlock(rgt);
@@ -2285,7 +2289,7 @@ __acquire_grant_for_copy(
         {
             unsigned long gfn = shared_entry_v1(rgt, gref).frame;
 
-            rc = __get_paged_frame(gfn, &grant_frame, page, readonly, rd);
+            rc = get_paged_frame(gfn, &grant_frame, page, readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
             act->gfn = gfn;
@@ -2295,7 +2299,8 @@ __acquire_grant_for_copy(
         }
         else if ( !(sha2->hdr.flags & GTF_sub_page) )
         {
-            rc = __get_paged_frame(sha2->full_page.frame, &grant_frame, page, readonly, rd);
+            rc = get_paged_frame(sha2->full_page.frame, &grant_frame, page,
+                                 readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
             act->gfn = sha2->full_page.frame;
@@ -2305,7 +2310,8 @@ __acquire_grant_for_copy(
         }
         else
         {
-            rc = __get_paged_frame(sha2->sub_page.frame, &grant_frame, page, readonly, rd);
+            rc = get_paged_frame(sha2->sub_page.frame, &grant_frame, page,
+                                 readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
             act->gfn = sha2->sub_page.frame;
@@ -2465,7 +2471,7 @@ static void gnttab_copy_release_buf(stru
     }
     if ( buf->have_grant )
     {
-        __release_grant_for_copy(buf->domain, buf->ptr.u.ref, buf->read_only);
+        release_grant_for_copy(buf->domain, buf->ptr.u.ref, buf->read_only);
         buf->have_grant = 0;
     }
 }
@@ -2481,11 +2487,11 @@ static int gnttab_copy_claim_buf(const s
 
     if ( op->flags & gref_flag )
     {
-        rc = __acquire_grant_for_copy(buf->domain, ptr->u.ref,
-                                      current->domain->domain_id,
-                                      buf->read_only,
-                                      &buf->frame, &buf->page,
-                                      &buf->ptr.offset, &buf->len, true);
+        rc = acquire_grant_for_copy(buf->domain, ptr->u.ref,
+                                    current->domain->domain_id,
+                                    buf->read_only,
+                                    &buf->frame, &buf->page,
+                                    &buf->ptr.offset, &buf->len, true);
         if ( rc != GNTST_okay )
             goto out;
         buf->ptr.u.ref = ptr->u.ref;
@@ -2493,8 +2499,8 @@ static int gnttab_copy_claim_buf(const s
     }
     else
     {
-        rc = __get_paged_frame(ptr->u.gmfn, &buf->frame, &buf->page,
-                               buf->read_only, buf->domain);
+        rc = get_paged_frame(ptr->u.gmfn, &buf->frame, &buf->page,
+                             buf->read_only, buf->domain);
         if ( rc != GNTST_okay )
             PIN_FAIL(out, rc,
                      "source frame %"PRI_xen_pfn" invalid.\n", ptr->u.gmfn);
@@ -2905,7 +2911,7 @@ gnttab_get_version(XEN_GUEST_HANDLE_PARA
 }
 
 static s16
-__gnttab_swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)
+swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)
 {
     struct domain *d = rcu_lock_current_domain();
     struct grant_table *gt = d->grant_table;
@@ -2981,7 +2987,7 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_P
             return i;
         if ( unlikely(__copy_from_guest(&op, uop, 1)) )
             return -EFAULT;
-        op.status = __gnttab_swap_grant_ref(op.ref_a, op.ref_b);
+        op.status = swap_grant_ref(op.ref_a, op.ref_b);
         if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
             return -EFAULT;
         guest_handle_add_offset(uop, 1);
@@ -2989,8 +2995,7 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_P
     return 0;
 }
 
-static int __gnttab_cache_flush(gnttab_cache_flush_t *cflush,
-                                grant_ref_t *cur_ref)
+static int cache_flush(gnttab_cache_flush_t *cflush, grant_ref_t *cur_ref)
 {
     struct domain *d, *owner;
     struct page_info *page;
@@ -3080,7 +3085,7 @@ gnttab_cache_flush(XEN_GUEST_HANDLE_PARA
             return -EFAULT;
         for ( ; ; )
         {
-            int ret = __gnttab_cache_flush(&op, cur_ref);
+            int ret = cache_flush(&op, cur_ref);
 
             if ( ret < 0 )
                 return ret;



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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH 5/8] gnttab: re-arrange struct active_grant_entry
  2017-08-15 14:15 [PATCH 0/8] gnttab: recent XSA follow-up Jan Beulich
                   ` (3 preceding siblings ...)
  2017-08-15 14:40 ` [PATCH 4/8] gnttab: drop pointless leading double underscores Jan Beulich
@ 2017-08-15 14:41 ` Jan Beulich
  2017-08-15 17:12   ` Andrew Cooper
  2017-08-15 14:41 ` [PATCH 6/8] gnttab: move GNTPIN_* out of header file Jan Beulich
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2017-08-15 14:41 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan

While benign to 32-bit arches, this shrinks the size from 56 to 48
bytes on 64-bit ones (while still leaving a 16-bit hole).

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

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -160,15 +160,15 @@ shared_entry_header(struct grant_table *
 struct active_grant_entry {
     uint32_t      pin;    /* Reference count information.             */
     domid_t       domid;  /* Domain being granted access.             */
-    struct domain *trans_domain;
+    unsigned int  start:15; /* For sub-page grants, the start offset
+                               in the page.                           */
+    bool          is_sub_page:1; /* True if this is a sub-page grant. */
+    unsigned int  length:16; /* For sub-page grants, the length of the
+                                grant.                                */
     grant_ref_t   trans_gref;
+    struct domain *trans_domain;
     unsigned long frame;  /* Frame being granted.                     */
     unsigned long gfn;    /* Guest's idea of the frame being granted. */
-    unsigned      is_sub_page:1; /* True if this is a sub-page grant. */
-    unsigned      start:15; /* For sub-page grants, the start offset
-                               in the page.                           */
-    unsigned      length:16; /* For sub-page grants, the length of the
-                                grant.                                */
     spinlock_t    lock;      /* lock to protect access of this entry.
                                 see docs/misc/grant-tables.txt for
                                 locking protocol                      */




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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH 6/8] gnttab: move GNTPIN_* out of header file
  2017-08-15 14:15 [PATCH 0/8] gnttab: recent XSA follow-up Jan Beulich
                   ` (4 preceding siblings ...)
  2017-08-15 14:41 ` [PATCH 5/8] gnttab: re-arrange struct active_grant_entry Jan Beulich
@ 2017-08-15 14:41 ` Jan Beulich
  2017-08-15 17:14   ` Andrew Cooper
  2017-08-15 14:42 ` [PATCH 7/8] gnttab: use DIV_ROUND_UP() instead of open-coding it Jan Beulich
  2017-08-15 14:42 ` [PATCH 8/8] gnttab: drop struct active_grant_entry's gfn field for release builds Jan Beulich
  7 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2017-08-15 14:41 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan

They're private to grant_table.c.

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

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -158,7 +158,24 @@ shared_entry_header(struct grant_table *
 
 /* Active grant entry - used for shadowing GTF_permit_access grants. */
 struct active_grant_entry {
-    uint32_t      pin;    /* Reference count information.             */
+    uint32_t      pin;    /* Reference count information:             */
+                          /* Count of writable host-CPU mappings.     */
+#define GNTPIN_hstw_shift    (0)
+#define GNTPIN_hstw_inc      (1 << GNTPIN_hstw_shift)
+#define GNTPIN_hstw_mask     (0xFFU << GNTPIN_hstw_shift)
+                          /* Count of read-only host-CPU mappings.    */
+#define GNTPIN_hstr_shift    (8)
+#define GNTPIN_hstr_inc      (1 << GNTPIN_hstr_shift)
+#define GNTPIN_hstr_mask     (0xFFU << GNTPIN_hstr_shift)
+                          /* Count of writable device-bus mappings.   */
+#define GNTPIN_devw_shift    (16)
+#define GNTPIN_devw_inc      (1 << GNTPIN_devw_shift)
+#define GNTPIN_devw_mask     (0xFFU << GNTPIN_devw_shift)
+                          /* Count of read-only device-bus mappings.  */
+#define GNTPIN_devr_shift    (24)
+#define GNTPIN_devr_inc      (1 << GNTPIN_devr_shift)
+#define GNTPIN_devr_mask     (0xFFU << GNTPIN_devr_shift)
+
     domid_t       domid;  /* Domain being granted access.             */
     unsigned int  start:15; /* For sub-page grants, the start offset
                                in the page.                           */
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -28,23 +28,6 @@
 #include <asm/page.h>
 #include <asm/grant_table.h>
 
- /* Count of writable host-CPU mappings. */
-#define GNTPIN_hstw_shift    (0)
-#define GNTPIN_hstw_inc      (1 << GNTPIN_hstw_shift)
-#define GNTPIN_hstw_mask     (0xFFU << GNTPIN_hstw_shift)
- /* Count of read-only host-CPU mappings. */
-#define GNTPIN_hstr_shift    (8)
-#define GNTPIN_hstr_inc      (1 << GNTPIN_hstr_shift)
-#define GNTPIN_hstr_mask     (0xFFU << GNTPIN_hstr_shift)
- /* Count of writable device-bus mappings. */
-#define GNTPIN_devw_shift    (16)
-#define GNTPIN_devw_inc      (1 << GNTPIN_devw_shift)
-#define GNTPIN_devw_mask     (0xFFU << GNTPIN_devw_shift)
- /* Count of read-only device-bus mappings. */
-#define GNTPIN_devr_shift    (24)
-#define GNTPIN_devr_inc      (1 << GNTPIN_devr_shift)
-#define GNTPIN_devr_mask     (0xFFU << GNTPIN_devr_shift)
-
 #ifndef DEFAULT_MAX_NR_GRANT_FRAMES /* to allow arch to override */
 /* Default maximum size of a grant table. [POLICY] */
 #define DEFAULT_MAX_NR_GRANT_FRAMES   32




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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH 7/8] gnttab: use DIV_ROUND_UP() instead of open-coding it
  2017-08-15 14:15 [PATCH 0/8] gnttab: recent XSA follow-up Jan Beulich
                   ` (5 preceding siblings ...)
  2017-08-15 14:41 ` [PATCH 6/8] gnttab: move GNTPIN_* out of header file Jan Beulich
@ 2017-08-15 14:42 ` Jan Beulich
  2017-08-15 17:15   ` Andrew Cooper
  2017-08-15 14:42 ` [PATCH 8/8] gnttab: drop struct active_grant_entry's gfn field for release builds Jan Beulich
  7 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2017-08-15 14:42 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan

Also adjust formatting of nearby code.

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

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -206,11 +206,13 @@ static inline void gnttab_flush_tlb(cons
 static inline unsigned int
 num_act_frames_from_sha_frames(const unsigned int num)
 {
-    /* How many frames are needed for the active grant table,
-     * given the size of the shared grant table? */
+    /*
+     * How many frames are needed for the active grant table,
+     * given the size of the shared grant table?
+     */
     unsigned int sha_per_page = PAGE_SIZE / sizeof(grant_entry_v1_t);
-    unsigned int num_sha_entries = num * sha_per_page;
-    return (num_sha_entries + (ACGNT_PER_PAGE - 1)) / ACGNT_PER_PAGE;
+
+    return DIV_ROUND_UP(num * sha_per_page, ACGNT_PER_PAGE);
 }
 
 #define max_nr_active_grant_frames \




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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH 8/8] gnttab: drop struct active_grant_entry's gfn field for release builds
  2017-08-15 14:15 [PATCH 0/8] gnttab: recent XSA follow-up Jan Beulich
                   ` (6 preceding siblings ...)
  2017-08-15 14:42 ` [PATCH 7/8] gnttab: use DIV_ROUND_UP() instead of open-coding it Jan Beulich
@ 2017-08-15 14:42 ` Jan Beulich
  2017-08-15 17:18   ` Andrew Cooper
  7 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2017-08-15 14:42 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Ian Jackson, Tim Deegan

This shrinks the size from 48 to 40 bytes bytes on 64-bit builds.

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

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -185,7 +185,12 @@ struct active_grant_entry {
     grant_ref_t   trans_gref;
     struct domain *trans_domain;
     unsigned long frame;  /* Frame being granted.                     */
+#ifndef NDEBUG
     unsigned long gfn;    /* Guest's idea of the frame being granted. */
+# define act_set_gfn(act, val) ((act)->gfn = (val))
+#else
+# define act_set_gfn(act, gfn)
+#endif
     spinlock_t    lock;      /* lock to protect access of this entry.
                                 see docs/misc/grant-tables.txt for
                                 locking protocol                      */
@@ -893,7 +898,7 @@ map_grant_ref(
                                  op->flags & GNTMAP_readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
-            act->gfn = gfn;
+            act_set_gfn(act, gfn);
             act->domid = ld->domain_id;
             act->frame = frame;
             act->start = 0;
@@ -2286,7 +2291,7 @@ acquire_grant_for_copy(
             act->trans_domain = td;
             act->trans_gref = trans_gref;
             act->frame = grant_frame;
-            act->gfn = gfn_x(INVALID_GFN);
+            act_set_gfn(act, gfn_x(INVALID_GFN));
             /*
              * The actual remote remote grant may or may not be a sub-page,
              * but we always treat it as one because that blocks mappings of
@@ -2312,7 +2317,7 @@ acquire_grant_for_copy(
             rc = get_paged_frame(gfn, &grant_frame, page, readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
-            act->gfn = gfn;
+            act_set_gfn(act, gfn);
             is_sub_page = 0;
             trans_page_off = 0;
             trans_length = PAGE_SIZE;
@@ -2323,7 +2328,7 @@ acquire_grant_for_copy(
                                  readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
-            act->gfn = sha2->full_page.frame;
+            act_set_gfn(act, sha2->full_page.frame);
             is_sub_page = 0;
             trans_page_off = 0;
             trans_length = PAGE_SIZE;
@@ -2334,7 +2339,7 @@ acquire_grant_for_copy(
                                  readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
-            act->gfn = sha2->sub_page.frame;
+            act_set_gfn(act, sha2->sub_page.frame);
             is_sub_page = 1;
             trans_page_off = sha2->sub_page.page_off;
             trans_length = sha2->sub_page.length;
@@ -3496,8 +3501,16 @@ void grant_table_warn_active_grants(stru
 
         nr_active++;
         if ( nr_active <= WARN_GRANT_MAX )
-            printk(XENLOG_G_DEBUG "Dom%d has an active grant: GFN: %lx (MFN: %lx)\n",
-                   d->domain_id, act->gfn, act->frame);
+            printk(XENLOG_G_DEBUG "Dom%d has active grant %x ("
+#ifndef NDEBUG
+                   "GFN %lx, "
+#endif
+                   "MFN: %lx)\n",
+                   d->domain_id, ref,
+#ifndef NDEBUG
+                   act->gfn,
+#endif
+                   act->frame);
         active_entry_release(act);
     }
 




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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH RESEND 1/8] gnttab: drop useless locking
  2017-08-15 14:38 ` [PATCH RESEND 1/8] gnttab: drop useless locking Jan Beulich
@ 2017-08-15 16:08   ` Andrew Cooper
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2017-08-15 16:08 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Tim Deegan

On 15/08/17 15:38, Jan Beulich wrote:
> Holding any lock while accessing the maptrack entry fields is
> pointless, as these entries are protected by their associated active
> entry lock (which is being acquired later, before re-validating the
> fields read without holding the lock).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH RESEND 2/8] gnttab: avoid spurious maptrack handle allocation failures
  2017-08-15 14:39 ` [PATCH RESEND 2/8] gnttab: avoid spurious maptrack handle allocation failures Jan Beulich
@ 2017-08-15 16:52   ` Andrew Cooper
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2017-08-15 16:52 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Tim Deegan

On 15/08/17 15:39, Jan Beulich wrote:
> @@ -422,8 +422,13 @@ get_maptrack_handle(
>      /*
>       * If we've run out of frames, try stealing an entry from another
>       * VCPU (in case the guest isn't mapping across its VCPUs evenly).
> +     * Also use this path in case we're out of memory, to avoid spurious
> +     * failures.

This comment isn't strictly correct any more.  It is now "If we've run
out of handles and still have frame headroom, try allocating a new
maptrack frame.  If there is no headroom, or Xen is out of memory, try
stealing an entry from another vcpu".

~Andrew

>       */
> -    if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
> +    if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
> +        new_mt = alloc_xenheap_page();
> +
> +    if ( !new_mt )
>      {
>          spin_unlock(&lgt->maptrack_lock);
>  


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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 3/8] gnttab: type adjustments
  2017-08-15 14:40 ` [PATCH 3/8] gnttab: type adjustments Jan Beulich
@ 2017-08-15 17:04   ` Andrew Cooper
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2017-08-15 17:04 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Tim Deegan

On 15/08/17 15:40, Jan Beulich wrote:
> In particular use grant_ref_t and grant_handle_t where appropriate.
> Also switch other nearby type uses to their canonical variants where
> appropriate and introduce INVALID_MAPTRACK_HANDLE.
>
> Signed-by: Jan Beulich <jbeulich@suse.com>

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

This needs rebasing over my change to simplify
gnttab_copy_lock_domain(), where I ended up with the same net change as
you've proposed.

~Andrew

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 4/8] gnttab: drop pointless leading double underscores
  2017-08-15 14:40 ` [PATCH 4/8] gnttab: drop pointless leading double underscores Jan Beulich
@ 2017-08-15 17:09   ` Andrew Cooper
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2017-08-15 17:09 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Tim Deegan

On 15/08/17 15:40, Jan Beulich wrote:
> They're violating name space rules, and we don't really need them. When
> followed by "gnttab_", also drop that.
>
> Signed-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -233,8 +233,9 @@ static inline void active_entry_release(
>     If rc == GNTST_okay, *page contains the page struct with a ref taken.
>     Caller must do put_page(*page).
>     If any error, *page = NULL, *frame = INVALID_MFN, no ref taken. */
> -static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct page_info **page,
> -                                int readonly, struct domain *rd)
> +static int get_paged_frame(unsigned long gfn, unsigned long *frame,
> +                           struct page_info **page, bool readonly,
> +                           struct domain *rd)
>  {
>      int rc = GNTST_okay;
>  #if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
> @@ -300,7 +301,7 @@ double_gt_unlock(struct grant_table *lgt
>  #define INVALID_MAPTRACK_HANDLE UINT_MAX
>  
>  static inline grant_handle_t
> -__get_maptrack_handle(
> +_get_maptrack_handle(
>      struct grant_table *t,
>      struct vcpu *v)

Any chance of coalescent these parameters?  Theres no need for 4 lines
here, or in similar cases below.

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 5/8] gnttab: re-arrange struct active_grant_entry
  2017-08-15 14:41 ` [PATCH 5/8] gnttab: re-arrange struct active_grant_entry Jan Beulich
@ 2017-08-15 17:12   ` Andrew Cooper
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2017-08-15 17:12 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Tim Deegan

On 15/08/17 15:41, Jan Beulich wrote:
> While benign to 32-bit arches, this shrinks the size from 56 to 48
> bytes on 64-bit ones (while still leaving a 16-bit hole).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

There is some follow-on is_sub_page type cleanup you could do,
especially in acquire_grant_for_copy().

~Andrew

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 6/8] gnttab: move GNTPIN_* out of header file
  2017-08-15 14:41 ` [PATCH 6/8] gnttab: move GNTPIN_* out of header file Jan Beulich
@ 2017-08-15 17:14   ` Andrew Cooper
  2017-08-16  6:54     ` Jan Beulich
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2017-08-15 17:14 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Tim Deegan

On 15/08/17 15:41, Jan Beulich wrote:
> They're private to grant_table.c.
>
> Signed-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -158,7 +158,24 @@ shared_entry_header(struct grant_table *
>  
>  /* Active grant entry - used for shadowing GTF_permit_access grants. */
>  struct active_grant_entry {
> -    uint32_t      pin;    /* Reference count information.             */
> +    uint32_t      pin;    /* Reference count information:             */
> +                          /* Count of writable host-CPU mappings.     */
> +#define GNTPIN_hstw_shift    (0)
> +#define GNTPIN_hstw_inc      (1 << GNTPIN_hstw_shift)
> +#define GNTPIN_hstw_mask     (0xFFU << GNTPIN_hstw_shift)
> +                          /* Count of read-only host-CPU mappings.    */
> +#define GNTPIN_hstr_shift    (8)
> +#define GNTPIN_hstr_inc      (1 << GNTPIN_hstr_shift)
> +#define GNTPIN_hstr_mask     (0xFFU << GNTPIN_hstr_shift)
> +                          /* Count of writable device-bus mappings.   */
> +#define GNTPIN_devw_shift    (16)
> +#define GNTPIN_devw_inc      (1 << GNTPIN_devw_shift)
> +#define GNTPIN_devw_mask     (0xFFU << GNTPIN_devw_shift)
> +                          /* Count of read-only device-bus mappings.  */
> +#define GNTPIN_devr_shift    (24)
> +#define GNTPIN_devr_inc      (1 << GNTPIN_devr_shift)
> +#define GNTPIN_devr_mask     (0xFFU << GNTPIN_devr_shift)

I would recommend taking the opportunity to switch these definitions to
1u << GNTPIN_*, as they are always used with unsigned types.

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 7/8] gnttab: use DIV_ROUND_UP() instead of open-coding it
  2017-08-15 14:42 ` [PATCH 7/8] gnttab: use DIV_ROUND_UP() instead of open-coding it Jan Beulich
@ 2017-08-15 17:15   ` Andrew Cooper
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2017-08-15 17:15 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Tim Deegan

On 15/08/17 15:42, Jan Beulich wrote:
> Also adjust formatting of nearby code.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 8/8] gnttab: drop struct active_grant_entry's gfn field for release builds
  2017-08-15 14:42 ` [PATCH 8/8] gnttab: drop struct active_grant_entry's gfn field for release builds Jan Beulich
@ 2017-08-15 17:18   ` Andrew Cooper
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2017-08-15 17:18 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Tim Deegan

On 15/08/17 15:42, Jan Beulich wrote:
> This shrinks the size from 48 to 40 bytes bytes on 64-bit builds.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -185,7 +185,12 @@ struct active_grant_entry {
>      grant_ref_t   trans_gref;
>      struct domain *trans_domain;
>      unsigned long frame;  /* Frame being granted.                     */
> +#ifndef NDEBUG
>      unsigned long gfn;    /* Guest's idea of the frame being granted. */
> +# define act_set_gfn(act, val) ((act)->gfn = (val))
> +#else
> +# define act_set_gfn(act, gfn)
> +#endif
>      spinlock_t    lock;      /* lock to protect access of this entry.
>                                  see docs/misc/grant-tables.txt for
>                                  locking protocol                      */

IMO, this would be cleaner as

static void act_set_gfn(struct active_grant_entry *act, unsigned long gfn)
{
#ifndef NDEBUG
    act->gfn = gfn;
#endif
}

Which both moves the function out of the struct definition, and takes
care of side effect evaluation differences.

~Andrew

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 6/8] gnttab: move GNTPIN_* out of header file
  2017-08-15 17:14   ` Andrew Cooper
@ 2017-08-16  6:54     ` Jan Beulich
  0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2017-08-16  6:54 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
	Ian Jackson, xen-devel

>>> On 15.08.17 at 19:14, <andrew.cooper3@citrix.com> wrote:
> On 15/08/17 15:41, Jan Beulich wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -158,7 +158,24 @@ shared_entry_header(struct grant_table *
>>  
>>  /* Active grant entry - used for shadowing GTF_permit_access grants. */
>>  struct active_grant_entry {
>> -    uint32_t      pin;    /* Reference count information.             */
>> +    uint32_t      pin;    /* Reference count information:             */
>> +                          /* Count of writable host-CPU mappings.     */
>> +#define GNTPIN_hstw_shift    (0)
>> +#define GNTPIN_hstw_inc      (1 << GNTPIN_hstw_shift)
>> +#define GNTPIN_hstw_mask     (0xFFU << GNTPIN_hstw_shift)
>> +                          /* Count of read-only host-CPU mappings.    */
>> +#define GNTPIN_hstr_shift    (8)
>> +#define GNTPIN_hstr_inc      (1 << GNTPIN_hstr_shift)
>> +#define GNTPIN_hstr_mask     (0xFFU << GNTPIN_hstr_shift)
>> +                          /* Count of writable device-bus mappings.   */
>> +#define GNTPIN_devw_shift    (16)
>> +#define GNTPIN_devw_inc      (1 << GNTPIN_devw_shift)
>> +#define GNTPIN_devw_mask     (0xFFU << GNTPIN_devw_shift)
>> +                          /* Count of read-only device-bus mappings.  */
>> +#define GNTPIN_devr_shift    (24)
>> +#define GNTPIN_devr_inc      (1 << GNTPIN_devr_shift)
>> +#define GNTPIN_devr_mask     (0xFFU << GNTPIN_devr_shift)
> 
> I would recommend taking the opportunity to switch these definitions to
> 1u << GNTPIN_*, as they are always used with unsigned types.

Oh, indeed. I've mechanically moved it without looking at the
actual pieces. I've done as you suggest plus dropped the stray
parentheses from the GNTPIN_*_shift definitions.

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

Thanks.

Jan


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

^ permalink raw reply	[flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-08-16  6:54 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-15 14:15 [PATCH 0/8] gnttab: recent XSA follow-up Jan Beulich
2017-08-15 14:38 ` [PATCH RESEND 1/8] gnttab: drop useless locking Jan Beulich
2017-08-15 16:08   ` Andrew Cooper
2017-08-15 14:39 ` [PATCH RESEND 2/8] gnttab: avoid spurious maptrack handle allocation failures Jan Beulich
2017-08-15 16:52   ` Andrew Cooper
2017-08-15 14:40 ` [PATCH 3/8] gnttab: type adjustments Jan Beulich
2017-08-15 17:04   ` Andrew Cooper
2017-08-15 14:40 ` [PATCH 4/8] gnttab: drop pointless leading double underscores Jan Beulich
2017-08-15 17:09   ` Andrew Cooper
2017-08-15 14:41 ` [PATCH 5/8] gnttab: re-arrange struct active_grant_entry Jan Beulich
2017-08-15 17:12   ` Andrew Cooper
2017-08-15 14:41 ` [PATCH 6/8] gnttab: move GNTPIN_* out of header file Jan Beulich
2017-08-15 17:14   ` Andrew Cooper
2017-08-16  6:54     ` Jan Beulich
2017-08-15 14:42 ` [PATCH 7/8] gnttab: use DIV_ROUND_UP() instead of open-coding it Jan Beulich
2017-08-15 17:15   ` Andrew Cooper
2017-08-15 14:42 ` [PATCH 8/8] gnttab: drop struct active_grant_entry's gfn field for release builds Jan Beulich
2017-08-15 17:18   ` Andrew Cooper

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