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