[PATCH 0/2] x86/mem-sharing: a fix and some cleanup

[PATCH 0/2] x86/mem-sharing: a fix and some cleanup

[Jan Beulich]

1: guarantee consistent lock order in get_two_gfns()
2: mov {get,put}_two_gfns()

Jan

[PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns()

[Jan Beulich]

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

[PATCH 2/2] x86/mem-sharing: mov {get,put}_two_gfns()

[Jan Beulich]

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

Ping: [PATCH 0/2] x86/mem-sharing: a fix and some cleanup

[Jan Beulich]

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

Re: [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns()

[Tamas K Lengyel]

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>

Re: [PATCH 2/2] x86/mem-sharing: mov {get,put}_two_gfns()

[Tamas K Lengyel]

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>

Re: [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns()

[Jan Beulich]

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

Re: [PATCH 1/2] x86/mem-sharing: ensure consistent lock order in get_two_gfns()

[Tamas K Lengyel]

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