* [PATCH 0/2] x86/mem-sharing: a fix and some cleanup @ 2021-06-29 12:53 Jan Beulich 2021-06-29 12:53 ` [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns() Jan Beulich ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Jan Beulich @ 2021-06-29 12:53 UTC (permalink / raw) To: xen-devel; +Cc: Tamas K Lengyel, Andrew Cooper, Wei Liu, Roger Pau Monné 1: guarantee consistent lock order in get_two_gfns() 2: mov {get,put}_two_gfns() Jan ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns() 2021-06-29 12:53 [PATCH 0/2] x86/mem-sharing: a fix and some cleanup Jan Beulich @ 2021-06-29 12:53 ` Jan Beulich 2021-07-06 12:36 ` Tamas K Lengyel 2021-06-29 12:54 ` [PATCH 2/2] x86/mem-sharing: mov {get,put}_two_gfns() Jan Beulich 2021-07-06 7:34 ` Ping: [PATCH 0/2] x86/mem-sharing: a fix and some cleanup Jan Beulich 2 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2021-06-29 12:53 UTC (permalink / raw) To: xen-devel; +Cc: Tamas K Lengyel, Andrew Cooper, Wei Liu, Roger Pau Monné While the comment validly says "Sort by domain, if same domain by gfn", the implementation also included equal domain IDs in the first part of the check, thus rending the second part entirely dead and leaving deadlock potential when there's only a single domain involved. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -587,7 +587,7 @@ do { dest ## _t = (source ## t) ?: &scratch_t; \ } while (0) - if ( (rd->domain_id <= ld->domain_id) || + if ( (rd->domain_id < ld->domain_id) || ((rd == ld) && (gfn_x(rgfn) <= gfn_x(lgfn))) ) { assign_pointers(first, r); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns() 2021-06-29 12:53 ` [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns() Jan Beulich @ 2021-07-06 12:36 ` Tamas K Lengyel 2021-07-06 13:13 ` Jan Beulich 0 siblings, 1 reply; 8+ messages in thread From: Tamas K Lengyel @ 2021-07-06 12:36 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné On Tue, Jun 29, 2021 at 8:54 AM Jan Beulich <jbeulich@suse.com> wrote: > > While the comment validly says "Sort by domain, if same domain by gfn", > the implementation also included equal domain IDs in the first part of > the check, thus rending the second part entirely dead and leaving > deadlock potential when there's only a single domain involved. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns() 2021-07-06 12:36 ` Tamas K Lengyel @ 2021-07-06 13:13 ` Jan Beulich 2021-07-06 13:37 ` Tamas K Lengyel 0 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2021-07-06 13:13 UTC (permalink / raw) To: Tamas K Lengyel; +Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné On 06.07.2021 14:36, Tamas K Lengyel wrote: > On Tue, Jun 29, 2021 at 8:54 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> While the comment validly says "Sort by domain, if same domain by gfn", >> the implementation also included equal domain IDs in the first part of >> the check, thus rending the second part entirely dead and leaving >> deadlock potential when there's only a single domain involved. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> Thanks. Do you think I should queue this for backporting (once it got applied)? Jan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns() 2021-07-06 13:13 ` Jan Beulich @ 2021-07-06 13:37 ` Tamas K Lengyel 0 siblings, 0 replies; 8+ messages in thread From: Tamas K Lengyel @ 2021-07-06 13:37 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné On Tue, Jul 6, 2021 at 9:14 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 06.07.2021 14:36, Tamas K Lengyel wrote: > > On Tue, Jun 29, 2021 at 8:54 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> While the comment validly says "Sort by domain, if same domain by gfn", > >> the implementation also included equal domain IDs in the first part of > >> the check, thus rending the second part entirely dead and leaving > >> deadlock potential when there's only a single domain involved. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > > Thanks. Do you think I should queue this for backporting (once it got > applied)? Sure, considering it's a bugfix. Thanks, Tamas ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] x86/mem-sharing: mov {get,put}_two_gfns() 2021-06-29 12:53 [PATCH 0/2] x86/mem-sharing: a fix and some cleanup Jan Beulich 2021-06-29 12:53 ` [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns() Jan Beulich @ 2021-06-29 12:54 ` Jan Beulich 2021-07-06 12:39 ` Tamas K Lengyel 2021-07-06 7:34 ` Ping: [PATCH 0/2] x86/mem-sharing: a fix and some cleanup Jan Beulich 2 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2021-06-29 12:54 UTC (permalink / raw) To: xen-devel; +Cc: Tamas K Lengyel, Andrew Cooper, Wei Liu, Roger Pau Monné There's no reason for every CU including p2m.h to have these two functions compiled, when they're both mem-sharing specific right now and for the foreseeable future. Largely just code movement, with some style tweaks, the inline-s dropped, and "put" being made consistent with "get" as to their NULL checking of the passed in pointer to struct two_gfns. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -432,6 +432,66 @@ static void mem_sharing_gfn_destroy(stru xfree(gfn_info); } +/* Deadlock-avoidance scheme when calling get_gfn on different gfn's */ +struct two_gfns { + struct domain *first_domain, *second_domain; + gfn_t first_gfn, second_gfn; +}; + +/* + * Returns mfn, type and access for potential caller consumption, but any + * of those can be NULL. + */ +static void get_two_gfns(struct domain *rd, gfn_t rgfn, p2m_type_t *rt, + p2m_access_t *ra, mfn_t *rmfn, + struct domain *ld, gfn_t lgfn, p2m_type_t *lt, + p2m_access_t *la, mfn_t *lmfn, + p2m_query_t q, struct two_gfns *rval, bool lock) +{ + mfn_t *first_mfn, *second_mfn, scratch_mfn; + p2m_access_t *first_a, *second_a, scratch_a; + p2m_type_t *first_t, *second_t, scratch_t; + + /* Sort by domain, if same domain by gfn */ + +#define assign_pointers(dest, source) \ +do { \ + rval-> dest ## _domain = source ## d; \ + rval-> dest ## _gfn = source ## gfn; \ + dest ## _mfn = (source ## mfn) ?: &scratch_mfn; \ + dest ## _a = (source ## a) ?: &scratch_a; \ + dest ## _t = (source ## t) ?: &scratch_t; \ +} while ( false ) + + if ( (rd->domain_id < ld->domain_id) || + ((rd == ld) && (gfn_x(rgfn) <= gfn_x(lgfn))) ) + { + assign_pointers(first, r); + assign_pointers(second, l); + } + else + { + assign_pointers(first, l); + assign_pointers(second, r); + } + +#undef assign_pointers + + /* Now do the gets. */ + *first_mfn = __get_gfn_type_access(p2m_get_hostp2m(rval->first_domain), + gfn_x(rval->first_gfn), first_t, + first_a, q, NULL, lock); + *second_mfn = __get_gfn_type_access(p2m_get_hostp2m(rval->second_domain), + gfn_x(rval->second_gfn), second_t, + second_a, q, NULL, lock); +} + +static void put_two_gfns(const struct two_gfns *arg) +{ + put_gfn(arg->second_domain, gfn_x(arg->second_gfn)); + put_gfn(arg->first_domain, gfn_x(arg->first_gfn)); +} + static struct page_info *mem_sharing_lookup(unsigned long mfn) { struct page_info *page; --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -559,62 +559,6 @@ int altp2m_get_effective_entry(struct p2 bool prepopulate); #endif -/* Deadlock-avoidance scheme when calling get_gfn on different gfn's */ -struct two_gfns { - struct domain *first_domain, *second_domain; - gfn_t first_gfn, second_gfn; -}; - -/* Returns mfn, type and access for potential caller consumption, but any - * of those can be NULL */ -static inline void get_two_gfns(struct domain *rd, gfn_t rgfn, - p2m_type_t *rt, p2m_access_t *ra, mfn_t *rmfn, struct domain *ld, - gfn_t lgfn, p2m_type_t *lt, p2m_access_t *la, mfn_t *lmfn, - p2m_query_t q, struct two_gfns *rval, bool lock) -{ - mfn_t *first_mfn, *second_mfn, scratch_mfn; - p2m_access_t *first_a, *second_a, scratch_a; - p2m_type_t *first_t, *second_t, scratch_t; - - /* Sort by domain, if same domain by gfn */ - -#define assign_pointers(dest, source) \ -do { \ - rval-> dest ## _domain = source ## d; \ - rval-> dest ## _gfn = source ## gfn; \ - dest ## _mfn = (source ## mfn) ?: &scratch_mfn; \ - dest ## _a = (source ## a) ?: &scratch_a; \ - dest ## _t = (source ## t) ?: &scratch_t; \ -} while (0) - - if ( (rd->domain_id < ld->domain_id) || - ((rd == ld) && (gfn_x(rgfn) <= gfn_x(lgfn))) ) - { - assign_pointers(first, r); - assign_pointers(second, l); - } else { - assign_pointers(first, l); - assign_pointers(second, r); - } - -#undef assign_pointers - - /* Now do the gets */ - *first_mfn = __get_gfn_type_access(p2m_get_hostp2m(rval->first_domain), - gfn_x(rval->first_gfn), first_t, first_a, q, NULL, lock); - *second_mfn = __get_gfn_type_access(p2m_get_hostp2m(rval->second_domain), - gfn_x(rval->second_gfn), second_t, second_a, q, NULL, lock); -} - -static inline void put_two_gfns(struct two_gfns *arg) -{ - if ( !arg ) - return; - - put_gfn(arg->second_domain, gfn_x(arg->second_gfn)); - put_gfn(arg->first_domain, gfn_x(arg->first_gfn)); -} - /* Init the datastructures for later use by the p2m code */ int p2m_init(struct domain *d); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86/mem-sharing: mov {get,put}_two_gfns() 2021-06-29 12:54 ` [PATCH 2/2] x86/mem-sharing: mov {get,put}_two_gfns() Jan Beulich @ 2021-07-06 12:39 ` Tamas K Lengyel 0 siblings, 0 replies; 8+ messages in thread From: Tamas K Lengyel @ 2021-07-06 12:39 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné On Tue, Jun 29, 2021 at 8:54 AM Jan Beulich <jbeulich@suse.com> wrote: > > There's no reason for every CU including p2m.h to have these two > functions compiled, when they're both mem-sharing specific right now and > for the foreseeable future. > > Largely just code movement, with some style tweaks, the inline-s > dropped, and "put" being made consistent with "get" as to their NULL > checking of the passed in pointer to struct two_gfns. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Ping: [PATCH 0/2] x86/mem-sharing: a fix and some cleanup 2021-06-29 12:53 [PATCH 0/2] x86/mem-sharing: a fix and some cleanup Jan Beulich 2021-06-29 12:53 ` [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns() Jan Beulich 2021-06-29 12:54 ` [PATCH 2/2] x86/mem-sharing: mov {get,put}_two_gfns() Jan Beulich @ 2021-07-06 7:34 ` Jan Beulich 2 siblings, 0 replies; 8+ messages in thread From: Jan Beulich @ 2021-07-06 7:34 UTC (permalink / raw) To: Tamas K Lengyel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, xen-devel Tamas, On 29.06.2021 14:53, Jan Beulich wrote: > 1: guarantee consistent lock order in get_two_gfns() > 2: mov {get,put}_two_gfns() while purely from a code placement perspective you may not be the primary maintainer of this code until the movement in patch 2, it exists solely for mem-sharing purposes (which is also why I'm moving it in patch 2), so I'd appreciate you taking a look. Thanks, Jan ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-06 13:38 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-29 12:53 [PATCH 0/2] x86/mem-sharing: a fix and some cleanup Jan Beulich 2021-06-29 12:53 ` [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns() Jan Beulich 2021-07-06 12:36 ` Tamas K Lengyel 2021-07-06 13:13 ` Jan Beulich 2021-07-06 13:37 ` Tamas K Lengyel 2021-06-29 12:54 ` [PATCH 2/2] x86/mem-sharing: mov {get,put}_two_gfns() Jan Beulich 2021-07-06 12:39 ` Tamas K Lengyel 2021-07-06 7:34 ` Ping: [PATCH 0/2] x86/mem-sharing: a fix and some cleanup Jan Beulich
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.