[PATCH 0 of 3] p2m synchronization second part

[PATCH 0 of 3] p2m synchronization second part

[Andres Lagar-Cavilla]

In this patch series we add actual synchronization. 

We first refine the API update posted previously (I can fold 
the previous and this first patch together, keep separate, whatever)

Then, we make p2m-lookups actually lock the p2m. For now it's still
the global p2m lock. It can be eventually made more fine grained.

Finally, we ensure that within a get_gfn/put_gfn critical section,
the caller has an additional ref on the underlying mfn. This requires
some trickery for manipulations that remove the mfn or swap it
(sharing).

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

 xen/arch/x86/hvm/hvm.c        |   3 +-
 xen/arch/x86/mm/p2m.c         |   8 +++---
 xen/common/grant_table.c      |   2 +-
 xen/common/memory.c           |   6 +++-
 xen/arch/x86/mm/mm-locks.h    |  13 +++++----
 xen/arch/x86/mm/p2m.c         |  18 +++++++++++++-
 xen/include/asm-x86/p2m.h     |  39 ++++++++++++++++++-----------
 xen/arch/x86/mm.c             |  18 +++++++++----
 xen/arch/x86/mm/mem_sharing.c |  13 +++------
 xen/arch/x86/mm/p2m.c         |  56 +++++++++++++++++++++++++++++++++++++++++-
 xen/common/grant_table.c      |   4 +-
 xen/common/memory.c           |  10 +++---
 xen/include/asm-x86/mm.h      |   3 +-
 xen/include/asm-x86/p2m.h     |  10 ++++++-
 xen/include/xen/paging.h      |   2 +-
 xen/include/xen/tmem_xen.h    |   2 +-
 16 files changed, 148 insertions(+), 59 deletions(-)

[PATCH 1 of 3] Improvements over API change for p2m lookups

[Andres Lagar-Cavilla]

 xen/arch/x86/hvm/hvm.c   |  3 +--
 xen/arch/x86/mm/p2m.c    |  8 ++++----
 xen/common/grant_table.c |  2 +-
 xen/common/memory.c      |  6 +++++-
 4 files changed, 11 insertions(+), 8 deletions(-)


Ranging from the cosmetic to actual bug fixing.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 390d6dc6c34b -r a0c55cc5d696 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1791,8 +1791,7 @@ int hvm_virtual_to_linear_addr(
     return 0;
 }
 
-/* We leave this function holding a lock on the p2m entry and a ref
- * on the mapped mfn */
+/* We leave this function holding a lock on the p2m entry */
 static void *__hvm_map_guest_frame(unsigned long gfn, bool_t writable)
 {
     unsigned long mfn;
diff -r 390d6dc6c34b -r a0c55cc5d696 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -349,21 +349,21 @@ void p2m_teardown(struct p2m_domain *p2m
     if (p2m == NULL)
         return;
 
+    p2m_lock(p2m);
+
 #ifdef __x86_64__
     for ( gfn=0; gfn < p2m->max_mapped_pfn; gfn++ )
     {
-        mfn = get_gfn_query(d, gfn, &t);
+        p2m_access_t a;
+        mfn = p2m->get_entry(p2m, gfn, &t, &a, p2m_query, NULL);
         if ( mfn_valid(mfn) && (t == p2m_ram_shared) )
         {
             ASSERT(!p2m_is_nestedp2m(p2m));
             BUG_ON(mem_sharing_unshare_page(d, gfn, MEM_SHARING_DESTROY_GFN));
         }
-        put_gfn(d, gfn);
     }
 #endif
 
-    p2m_lock(p2m);
-
     p2m->phys_table = pagetable_null();
 
     while ( (pg = page_list_remove_head(&p2m->pages)) )
diff -r 390d6dc6c34b -r a0c55cc5d696 xen/common/grant_table.c
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -470,7 +470,7 @@ __gnttab_map_grant_ref(
     struct domain *ld, *rd, *owner;
     struct vcpu   *led;
     int            handle;
-    unsigned long gfn = INVALID_GFN;
+    unsigned long  gfn = INVALID_GFN;
     unsigned long  frame = 0, nr_gets = 0;
     struct page_info *pg;
     int            rc = GNTST_okay;
diff -r 390d6dc6c34b -r a0c55cc5d696 xen/common/memory.c
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -270,7 +270,7 @@ static long memory_exchange(XEN_GUEST_HA
     PAGE_LIST_HEAD(out_chunk_list);
     unsigned long in_chunk_order, out_chunk_order;
     xen_pfn_t     gpfn, gmfn, mfn;
-    unsigned long i, j, k;
+    unsigned long i, j, k = 0; /* gcc ... */
     unsigned int  memflags = 0;
     long          rc = 0;
     struct domain *d;
@@ -496,8 +496,12 @@ static long memory_exchange(XEN_GUEST_HA
  fail:
     /* Reassign any input pages we managed to steal. */
     while ( (page = page_list_remove_head(&in_chunk_list)) )
+    {
+        put_gfn(d, gmfn + k--);
         if ( assign_pages(d, page, 0, MEMF_no_refcount) )
             BUG();
+    }
+
  dying:
     rcu_unlock_domain(d);
     /* Free any output pages we managed to allocate. */

Re: [PATCH 1 of 3] Improvements over API change for p2m lookups

[Tim Deegan]

At 16:42 -0500 on 08 Nov (1320770544), Andres Lagar-Cavilla wrote:
>  xen/arch/x86/hvm/hvm.c   |  3 +--
>  xen/arch/x86/mm/p2m.c    |  8 ++++----
>  xen/common/grant_table.c |  2 +-
>  xen/common/memory.c      |  6 +++++-
>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> 
> Ranging from the cosmetic to actual bug fixing.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

These look fine.  Please fold them into the next version of the API
change patch.

Tim.

[PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/mm-locks.h |  13 +++++++------
 xen/arch/x86/mm/p2m.c      |  18 +++++++++++++++++-
 xen/include/asm-x86/p2m.h  |  39 ++++++++++++++++++++++++---------------
 3 files changed, 48 insertions(+), 22 deletions(-)


We achieve this by locking/unlocking the global p2m_lock in get/put_gfn.
This brings about a few consequences for the p2m_lock:

 - not ordered anymore in mm-locks.h: unshare does get_gfn -> shr_lock,
   there are code paths that do paging_lock -> get_gfn. All of these
   would cause mm-locks.h to panic.
 - the lock is always taken recursively, as there are many paths that
   do get_gfn -> p2m_lock

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r a0c55cc5d696 -r 6203a0549d8a xen/arch/x86/mm/mm-locks.h
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -165,14 +165,15 @@ declare_mm_lock(nestedp2m)
 
 /* P2M lock (per-p2m-table)
  * 
- * This protects all updates to the p2m table.  Updates are expected to
- * be safe against concurrent reads, which do *not* require the lock. */
+ * This protects all queries and updates to the p2m table. Because queries
+ * can happen interleaved with other locks in here, we do not enforce
+ * ordering, and make all locking recursive. */
 
-declare_mm_lock(p2m)
-#define p2m_lock(p)           mm_lock(p2m, &(p)->lock)
-#define p2m_lock_recursive(p) mm_lock_recursive(p2m, &(p)->lock)
-#define p2m_unlock(p)         mm_unlock(&(p)->lock)
+#define p2m_lock(p)           spin_lock_recursive(&(p)->lock.lock)
+#define p2m_lock_recursive(p) spin_lock_recursive(&(p)->lock.lock)
+#define p2m_unlock(p)         spin_unlock_recursive(&(p)->lock.lock)
 #define p2m_locked_by_me(p)   mm_locked_by_me(&(p)->lock)
+#define gfn_locked_by_me(p,g) mm_locked_by_me(&(p)->lock)
 
 /* Page alloc lock (per-domain)
  *
diff -r a0c55cc5d696 -r 6203a0549d8a xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -158,6 +158,9 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
         return _mfn(gfn);
     }
 
+    /* Grab the lock here, don't release until put_gfn */
+    p2m_lock(p2m);
+
     mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order);
 
 #ifdef __x86_64__
@@ -182,6 +185,19 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
     return mfn;
 }
 
+void put_gfn(struct domain *d, unsigned long gfn)
+{
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+    if ( !p2m || !paging_mode_translate(d) )
+        /* Nothing to do in this case */
+        return;
+
+    ASSERT(p2m_locked_by_me(p2m));
+
+    p2m_unlock(p2m);
+}
+
 int set_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, 
                   unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma)
 {
@@ -190,7 +206,7 @@ int set_p2m_entry(struct p2m_domain *p2m
     unsigned int order;
     int rc = 1;
 
-    ASSERT(p2m_locked_by_me(p2m));
+    ASSERT(gfn_locked_by_me(p2m, gfn));
 
     while ( todo )
     {
diff -r a0c55cc5d696 -r 6203a0549d8a xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -305,9 +305,10 @@ struct p2m_domain *p2m_get_p2m(struct vc
 
 #define p2m_get_pagetable(p2m)  ((p2m)->phys_table)
 
-/**** p2m query accessors. After calling any of the variants below, you
- * need to call put_gfn(domain, gfn). If you don't, you'll lock the
- * hypervisor. ****/
+/**** p2m query accessors. They lock p2m_lock, and thus serialize
+ * lookups wrt modifications. They _do not_ release the lock on exit.
+ * After calling any of the variants below, caller needs to use
+ * put_gfn. ****/
 
 /* Read a particular P2M table, mapping pages as we go.  Most callers
  * should _not_ call this directly; use the other get_gfn* functions
@@ -349,19 +350,27 @@ static inline unsigned long get_gfn_unty
     return INVALID_MFN;
 }
 
-/* This is a noop for now. */
-static inline void put_gfn(struct domain *d, unsigned long gfn)
-{
-}
+/* Will release the p2m_lock and put the page behind this mapping. */
+void put_gfn(struct domain *d, unsigned long gfn);
 
-/* These are identical for now. The intent is to have the caller not worry 
- * about put_gfn. To only be used in printk's, crash situations, or to 
- * peek at a type. You're not holding the p2m entry exclsively after calling
- * this. */
-#define get_gfn_unlocked(d, g, t)         get_gfn_type((d), (g), (t), p2m_alloc)
-#define get_gfn_query_unlocked(d, g, t)   get_gfn_type((d), (g), (t), p2m_query)
-#define get_gfn_guest_unlocked(d, g, t)   get_gfn_type((d), (g), (t), p2m_guest)
-#define get_gfn_unshare_unlocked(d, g, t) get_gfn_type((d), (g), (t), p2m_unshare)
+/* The intent is to have the caller not worry about put_gfn. They apply to 
+ * very specific situations: debug printk's, dumps during a domain crash,
+ * or to peek at a p2m entry/type. Caller is not holding the p2m entry 
+ * exclusively after calling this. */
+#define build_unlocked_accessor(name)                                   \
+    static inline mfn_t get_gfn ## name ## _unlocked(struct domain *d,  \
+                                                unsigned long gfn,      \
+                                                p2m_type_t *t)          \
+    {                                                                   \
+        mfn_t mfn = get_gfn ##name (d, gfn, t);                         \
+        put_gfn(d, gfn);                                                \
+        return mfn;                                                     \
+    }
+
+build_unlocked_accessor()
+build_unlocked_accessor(_query)
+build_unlocked_accessor(_guest)
+build_unlocked_accessor(_unshare)
 
 /* General conversion function from mfn to gfn */
 static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn)

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications

[Tim Deegan]

At 16:42 -0500 on 08 Nov (1320770545), Andres Lagar-Cavilla wrote:
>  xen/arch/x86/mm/mm-locks.h |  13 +++++++------
>  xen/arch/x86/mm/p2m.c      |  18 +++++++++++++++++-
>  xen/include/asm-x86/p2m.h  |  39 ++++++++++++++++++++++++---------------
>  3 files changed, 48 insertions(+), 22 deletions(-)
> 
> 
> We achieve this by locking/unlocking the global p2m_lock in get/put_gfn.
> This brings about a few consequences for the p2m_lock:
> 
>  - not ordered anymore in mm-locks.h: unshare does get_gfn -> shr_lock,
>    there are code paths that do paging_lock -> get_gfn. All of these
>    would cause mm-locks.h to panic.

In that case there's a potential deadlock in the sharing code, and
turning off the safety catches is not an acceptable response to that. :)

ISTR you had a plan to get rid of the shr_lock entirely, or am
I misremembering?

Tim.

>  - the lock is always taken recursively, as there are many paths that
>    do get_gfn -> p2m_lock
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> diff -r a0c55cc5d696 -r 6203a0549d8a xen/arch/x86/mm/mm-locks.h
> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -165,14 +165,15 @@ declare_mm_lock(nestedp2m)
>  
>  /* P2M lock (per-p2m-table)
>   * 
> - * This protects all updates to the p2m table.  Updates are expected to
> - * be safe against concurrent reads, which do *not* require the lock. */
> + * This protects all queries and updates to the p2m table. Because queries
> + * can happen interleaved with other locks in here, we do not enforce
> + * ordering, and make all locking recursive. */
>  
> -declare_mm_lock(p2m)
> -#define p2m_lock(p)           mm_lock(p2m, &(p)->lock)
> -#define p2m_lock_recursive(p) mm_lock_recursive(p2m, &(p)->lock)
> -#define p2m_unlock(p)         mm_unlock(&(p)->lock)
> +#define p2m_lock(p)           spin_lock_recursive(&(p)->lock.lock)
> +#define p2m_lock_recursive(p) spin_lock_recursive(&(p)->lock.lock)
> +#define p2m_unlock(p)         spin_unlock_recursive(&(p)->lock.lock)
>  #define p2m_locked_by_me(p)   mm_locked_by_me(&(p)->lock)
> +#define gfn_locked_by_me(p,g) mm_locked_by_me(&(p)->lock)
>  
>  /* Page alloc lock (per-domain)
>   *
> diff -r a0c55cc5d696 -r 6203a0549d8a xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -158,6 +158,9 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
>          return _mfn(gfn);
>      }
>  
> +    /* Grab the lock here, don't release until put_gfn */
> +    p2m_lock(p2m);
> +
>      mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order);
>  
>  #ifdef __x86_64__
> @@ -182,6 +185,19 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
>      return mfn;
>  }
>  
> +void put_gfn(struct domain *d, unsigned long gfn)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> +    if ( !p2m || !paging_mode_translate(d) )
> +        /* Nothing to do in this case */
> +        return;
> +
> +    ASSERT(p2m_locked_by_me(p2m));
> +
> +    p2m_unlock(p2m);
> +}
> +
>  int set_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, 
>                    unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma)
>  {
> @@ -190,7 +206,7 @@ int set_p2m_entry(struct p2m_domain *p2m
>      unsigned int order;
>      int rc = 1;
>  
> -    ASSERT(p2m_locked_by_me(p2m));
> +    ASSERT(gfn_locked_by_me(p2m, gfn));
>  
>      while ( todo )
>      {
> diff -r a0c55cc5d696 -r 6203a0549d8a xen/include/asm-x86/p2m.h
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -305,9 +305,10 @@ struct p2m_domain *p2m_get_p2m(struct vc
>  
>  #define p2m_get_pagetable(p2m)  ((p2m)->phys_table)
>  
> -/**** p2m query accessors. After calling any of the variants below, you
> - * need to call put_gfn(domain, gfn). If you don't, you'll lock the
> - * hypervisor. ****/
> +/**** p2m query accessors. They lock p2m_lock, and thus serialize
> + * lookups wrt modifications. They _do not_ release the lock on exit.
> + * After calling any of the variants below, caller needs to use
> + * put_gfn. ****/
>  
>  /* Read a particular P2M table, mapping pages as we go.  Most callers
>   * should _not_ call this directly; use the other get_gfn* functions
> @@ -349,19 +350,27 @@ static inline unsigned long get_gfn_unty
>      return INVALID_MFN;
>  }
>  
> -/* This is a noop for now. */
> -static inline void put_gfn(struct domain *d, unsigned long gfn)
> -{
> -}
> +/* Will release the p2m_lock and put the page behind this mapping. */
> +void put_gfn(struct domain *d, unsigned long gfn);
>  
> -/* These are identical for now. The intent is to have the caller not worry 
> - * about put_gfn. To only be used in printk's, crash situations, or to 
> - * peek at a type. You're not holding the p2m entry exclsively after calling
> - * this. */
> -#define get_gfn_unlocked(d, g, t)         get_gfn_type((d), (g), (t), p2m_alloc)
> -#define get_gfn_query_unlocked(d, g, t)   get_gfn_type((d), (g), (t), p2m_query)
> -#define get_gfn_guest_unlocked(d, g, t)   get_gfn_type((d), (g), (t), p2m_guest)
> -#define get_gfn_unshare_unlocked(d, g, t) get_gfn_type((d), (g), (t), p2m_unshare)
> +/* The intent is to have the caller not worry about put_gfn. They apply to 
> + * very specific situations: debug printk's, dumps during a domain crash,
> + * or to peek at a p2m entry/type. Caller is not holding the p2m entry 
> + * exclusively after calling this. */
> +#define build_unlocked_accessor(name)                                   \
> +    static inline mfn_t get_gfn ## name ## _unlocked(struct domain *d,  \
> +                                                unsigned long gfn,      \
> +                                                p2m_type_t *t)          \
> +    {                                                                   \
> +        mfn_t mfn = get_gfn ##name (d, gfn, t);                         \
> +        put_gfn(d, gfn);                                                \
> +        return mfn;                                                     \
> +    }
> +
> +build_unlocked_accessor()
> +build_unlocked_accessor(_query)
> +build_unlocked_accessor(_guest)
> +build_unlocked_accessor(_unshare)
>  
>  /* General conversion function from mfn to gfn */
>  static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications

[Andres Lagar-Cavilla]

Hi there,
> At 16:42 -0500 on 08 Nov (1320770545), Andres Lagar-Cavilla wrote:
>>  xen/arch/x86/mm/mm-locks.h |  13 +++++++------
>>  xen/arch/x86/mm/p2m.c      |  18 +++++++++++++++++-
>>  xen/include/asm-x86/p2m.h  |  39
>> ++++++++++++++++++++++++---------------
>>  3 files changed, 48 insertions(+), 22 deletions(-)
>>
>>
>> We achieve this by locking/unlocking the global p2m_lock in get/put_gfn.
>> This brings about a few consequences for the p2m_lock:
>>
>>  - not ordered anymore in mm-locks.h: unshare does get_gfn -> shr_lock,
>>    there are code paths that do paging_lock -> get_gfn. All of these
>>    would cause mm-locks.h to panic.
>
> In that case there's a potential deadlock in the sharing code, and
> turning off the safety catches is not an acceptable response to that. :)
>
> ISTR you had a plan to get rid of the shr_lock entirely, or am
> I misremembering?
Sharing is actually fine, I can reorder those safely until I get rid of
the shr_lock. Except for sharing audits, which basically lock the whole
hypervisor, and _no one is using at all_.

I have a more fundamental problem with the paging lock. sh_update_cr3 can
be called from a variety of situations. It will walk the four top level
PAE mappings, acquiring the p2m entry for each, with the paging lock held.
This is an inversion & deadlock, if I try to synchronize p2m lookups with
the p2m lock.

Any suggestions here? Other than disabling ordering of the p2m lock?

Thanks
Andres

>
> Tim.
>
>>  - the lock is always taken recursively, as there are many paths that
>>    do get_gfn -> p2m_lock
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>>
>> diff -r a0c55cc5d696 -r 6203a0549d8a xen/arch/x86/mm/mm-locks.h
>> --- a/xen/arch/x86/mm/mm-locks.h
>> +++ b/xen/arch/x86/mm/mm-locks.h
>> @@ -165,14 +165,15 @@ declare_mm_lock(nestedp2m)
>>
>>  /* P2M lock (per-p2m-table)
>>   *
>> - * This protects all updates to the p2m table.  Updates are expected to
>> - * be safe against concurrent reads, which do *not* require the lock.
>> */
>> + * This protects all queries and updates to the p2m table. Because
>> queries
>> + * can happen interleaved with other locks in here, we do not enforce
>> + * ordering, and make all locking recursive. */
>>
>> -declare_mm_lock(p2m)
>> -#define p2m_lock(p)           mm_lock(p2m, &(p)->lock)
>> -#define p2m_lock_recursive(p) mm_lock_recursive(p2m, &(p)->lock)
>> -#define p2m_unlock(p)         mm_unlock(&(p)->lock)
>> +#define p2m_lock(p)           spin_lock_recursive(&(p)->lock.lock)
>> +#define p2m_lock_recursive(p) spin_lock_recursive(&(p)->lock.lock)
>> +#define p2m_unlock(p)         spin_unlock_recursive(&(p)->lock.lock)
>>  #define p2m_locked_by_me(p)   mm_locked_by_me(&(p)->lock)
>> +#define gfn_locked_by_me(p,g) mm_locked_by_me(&(p)->lock)
>>
>>  /* Page alloc lock (per-domain)
>>   *
>> diff -r a0c55cc5d696 -r 6203a0549d8a xen/arch/x86/mm/p2m.c
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -158,6 +158,9 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
>>          return _mfn(gfn);
>>      }
>>
>> +    /* Grab the lock here, don't release until put_gfn */
>> +    p2m_lock(p2m);
>> +
>>      mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order);
>>
>>  #ifdef __x86_64__
>> @@ -182,6 +185,19 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
>>      return mfn;
>>  }
>>
>> +void put_gfn(struct domain *d, unsigned long gfn)
>> +{
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +
>> +    if ( !p2m || !paging_mode_translate(d) )
>> +        /* Nothing to do in this case */
>> +        return;
>> +
>> +    ASSERT(p2m_locked_by_me(p2m));
>> +
>> +    p2m_unlock(p2m);
>> +}
>> +
>>  int set_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>>                    unsigned int page_order, p2m_type_t p2mt,
>> p2m_access_t p2ma)
>>  {
>> @@ -190,7 +206,7 @@ int set_p2m_entry(struct p2m_domain *p2m
>>      unsigned int order;
>>      int rc = 1;
>>
>> -    ASSERT(p2m_locked_by_me(p2m));
>> +    ASSERT(gfn_locked_by_me(p2m, gfn));
>>
>>      while ( todo )
>>      {
>> diff -r a0c55cc5d696 -r 6203a0549d8a xen/include/asm-x86/p2m.h
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -305,9 +305,10 @@ struct p2m_domain *p2m_get_p2m(struct vc
>>
>>  #define p2m_get_pagetable(p2m)  ((p2m)->phys_table)
>>
>> -/**** p2m query accessors. After calling any of the variants below, you
>> - * need to call put_gfn(domain, gfn). If you don't, you'll lock the
>> - * hypervisor. ****/
>> +/**** p2m query accessors. They lock p2m_lock, and thus serialize
>> + * lookups wrt modifications. They _do not_ release the lock on exit.
>> + * After calling any of the variants below, caller needs to use
>> + * put_gfn. ****/
>>
>>  /* Read a particular P2M table, mapping pages as we go.  Most callers
>>   * should _not_ call this directly; use the other get_gfn* functions
>> @@ -349,19 +350,27 @@ static inline unsigned long get_gfn_unty
>>      return INVALID_MFN;
>>  }
>>
>> -/* This is a noop for now. */
>> -static inline void put_gfn(struct domain *d, unsigned long gfn)
>> -{
>> -}
>> +/* Will release the p2m_lock and put the page behind this mapping. */
>> +void put_gfn(struct domain *d, unsigned long gfn);
>>
>> -/* These are identical for now. The intent is to have the caller not
>> worry
>> - * about put_gfn. To only be used in printk's, crash situations, or to
>> - * peek at a type. You're not holding the p2m entry exclsively after
>> calling
>> - * this. */
>> -#define get_gfn_unlocked(d, g, t)         get_gfn_type((d), (g), (t),
>> p2m_alloc)
>> -#define get_gfn_query_unlocked(d, g, t)   get_gfn_type((d), (g), (t),
>> p2m_query)
>> -#define get_gfn_guest_unlocked(d, g, t)   get_gfn_type((d), (g), (t),
>> p2m_guest)
>> -#define get_gfn_unshare_unlocked(d, g, t) get_gfn_type((d), (g), (t),
>> p2m_unshare)
>> +/* The intent is to have the caller not worry about put_gfn. They apply
>> to
>> + * very specific situations: debug printk's, dumps during a domain
>> crash,
>> + * or to peek at a p2m entry/type. Caller is not holding the p2m entry
>> + * exclusively after calling this. */
>> +#define build_unlocked_accessor(name)
>> \
>> +    static inline mfn_t get_gfn ## name ## _unlocked(struct domain *d,
>> \
>> +                                                unsigned long gfn,
>> \
>> +                                                p2m_type_t *t)
>> \
>> +    {
>> \
>> +        mfn_t mfn = get_gfn ##name (d, gfn, t);
>> \
>> +        put_gfn(d, gfn);
>> \
>> +        return mfn;
>> \
>> +    }
>> +
>> +build_unlocked_accessor()
>> +build_unlocked_accessor(_query)
>> +build_unlocked_accessor(_guest)
>> +build_unlocked_accessor(_unshare)
>>
>>  /* General conversion function from mfn to gfn */
>>  static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn)
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications

[Tim Deegan]

At 10:03 -0800 on 14 Nov (1321264988), Andres Lagar-Cavilla wrote:
> Hi there,
> > At 16:42 -0500 on 08 Nov (1320770545), Andres Lagar-Cavilla wrote:
> >>  xen/arch/x86/mm/mm-locks.h |  13 +++++++------
> >>  xen/arch/x86/mm/p2m.c      |  18 +++++++++++++++++-
> >>  xen/include/asm-x86/p2m.h  |  39
> >> ++++++++++++++++++++++++---------------
> >>  3 files changed, 48 insertions(+), 22 deletions(-)
> >>
> >>
> >> We achieve this by locking/unlocking the global p2m_lock in get/put_gfn.
> >> This brings about a few consequences for the p2m_lock:
> >>
> >>  - not ordered anymore in mm-locks.h: unshare does get_gfn -> shr_lock,
> >>    there are code paths that do paging_lock -> get_gfn. All of these
> >>    would cause mm-locks.h to panic.
> >
> > In that case there's a potential deadlock in the sharing code, and
> > turning off the safety catches is not an acceptable response to that. :)
> >
> > ISTR you had a plan to get rid of the shr_lock entirely, or am
> > I misremembering?
> Sharing is actually fine, I can reorder those safely until I get rid of
> the shr_lock. Except for sharing audits, which basically lock the whole
> hypervisor, and _no one is using at all_.
> 
> I have a more fundamental problem with the paging lock. sh_update_cr3 can
> be called from a variety of situations. It will walk the four top level
> PAE mappings, acquiring the p2m entry for each, with the paging lock held.
> This is an inversion & deadlock, if I try to synchronize p2m lookups with
> the p2m lock.

Is sh_update_cr3() really called with p2m locks/refs held?  Since it
doesn't take a frame number as an argument it might be possible to
shuffle things around at the callers.

> Any suggestions here? Other than disabling ordering of the p2m lock?

Disabling the ordering won't do, so we need to find something else.

Tim.

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications

[Andres Lagar-Cavilla]

> At 10:03 -0800 on 14 Nov (1321264988), Andres Lagar-Cavilla wrote:
>> Hi there,
>> > At 16:42 -0500 on 08 Nov (1320770545), Andres Lagar-Cavilla wrote:
>> >>  xen/arch/x86/mm/mm-locks.h |  13 +++++++------
>> >>  xen/arch/x86/mm/p2m.c      |  18 +++++++++++++++++-
>> >>  xen/include/asm-x86/p2m.h  |  39
>> >> ++++++++++++++++++++++++---------------
>> >>  3 files changed, 48 insertions(+), 22 deletions(-)
>> >>
>> >>
>> >> We achieve this by locking/unlocking the global p2m_lock in
>> get/put_gfn.
>> >> This brings about a few consequences for the p2m_lock:
>> >>
>> >>  - not ordered anymore in mm-locks.h: unshare does get_gfn ->
>> shr_lock,
>> >>    there are code paths that do paging_lock -> get_gfn. All of these
>> >>    would cause mm-locks.h to panic.
>> >
>> > In that case there's a potential deadlock in the sharing code, and
>> > turning off the safety catches is not an acceptable response to that.
>> :)
>> >
>> > ISTR you had a plan to get rid of the shr_lock entirely, or am
>> > I misremembering?
>> Sharing is actually fine, I can reorder those safely until I get rid of
>> the shr_lock. Except for sharing audits, which basically lock the whole
>> hypervisor, and _no one is using at all_.
>>
>> I have a more fundamental problem with the paging lock. sh_update_cr3
>> can
>> be called from a variety of situations. It will walk the four top level
>> PAE mappings, acquiring the p2m entry for each, with the paging lock
>> held.
>> This is an inversion & deadlock, if I try to synchronize p2m lookups
>> with
>> the p2m lock.
>
> Is sh_update_cr3() really called with p2m locks/refs held?  Since it
> doesn't take a frame number as an argument it might be possible to
> shuffle things around at the callers.

Ok, I've refined this a bit. There are several instances of code that
takes the paging_lock and later needs to perform a p2m lookup. Apart from
the previously mentioned example here are two others:

hap_update_paging_modes -> ... -> vmx_load_pdptrs -> get_gfn(guest_cr3)

sh_x86_emulate_* -> ... -> validate_gl?e -> get_gfn_query()

None of these functions are called with p2m locks/refs held, and likely
they shouldn't. However, they will result in a deadlock panic situation,
if get_gfn takes a lock.

Here is one solution to consider: do not lock the p2m on lookups in shadow
mode. Shadow mode does not support paging out and sharing of pages, which
are primary reasons why we want synchronized p2m lookups. The hap cases
where there is an inversion of the p2m_lock -> paging_lock order are
reasonably simple to handle.

The other option is to invert the order and place paging_lock -> p2m_lock,
but that will raise another set of potential inversions. I think this is a
no go.

Finally, one could audit all these calls and have them perform futurology,
get_gfn the gfn they will end up perhaps looking up, before taking the
paging_lock. I also think this is a no go.

All very unsavory.
Andres

>
>> Any suggestions here? Other than disabling ordering of the p2m lock?
>
> Disabling the ordering won't do, so we need to find something else.
>
> Tim.
>
>

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications

[Tim Deegan]

At 08:41 -0800 on 24 Nov (1322124101), Andres Lagar-Cavilla wrote:
> Here is one solution to consider: do not lock the p2m on lookups in shadow
> mode. Shadow mode does not support paging out and sharing of pages, which
> are primary reasons why we want synchronized p2m lookups. The hap cases
> where there is an inversion of the p2m_lock -> paging_lock order are
> reasonably simple to handle.
> 
> The other option is to invert the order and place paging_lock -> p2m_lock,
> but that will raise another set of potential inversions. I think this is a
> no go.

Is there scope for having the p2m lock split out into the overall one
(needed for allocations &c) and the per-record ones, and have them at
different levels in the lock hierarchy?

Tim.

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications

[Andres Lagar-Cavilla]

> At 08:41 -0800 on 24 Nov (1322124101), Andres Lagar-Cavilla wrote:
>> Here is one solution to consider: do not lock the p2m on lookups in
>> shadow
>> mode. Shadow mode does not support paging out and sharing of pages,
>> which
>> are primary reasons why we want synchronized p2m lookups. The hap cases
>> where there is an inversion of the p2m_lock -> paging_lock order are
>> reasonably simple to handle.
>>
>> The other option is to invert the order and place paging_lock ->
>> p2m_lock,
>> but that will raise another set of potential inversions. I think this is
>> a
>> no go.
>
> Is there scope for having the p2m lock split out into the overall one
> (needed for allocations &c) and the per-record ones, and have them at
> different levels in the lock hierarchy?
I think it's saner to go with one big lock and then shard it if we run
into performance problems. Your proposal, actually :)

Now, I'm thinking of splitting ... the paging lock. It covers way too much
ground right now, and many of these inversions would become more
tractable. Thinking of paging_alloc_lock, paging_logdirty_lock,
paging_lock.

Thanks,
Andres
>
> Tim.
>

Re: [PATCH 2 of 3] Make p2m lookups fully synchronized wrt modifications

[Tim Deegan]

At 08:33 -0800 on 02 Dec (1322814798), Andres Lagar-Cavilla wrote:
> > At 08:41 -0800 on 24 Nov (1322124101), Andres Lagar-Cavilla wrote:
> >> Here is one solution to consider: do not lock the p2m on lookups in
> >> shadow
> >> mode. Shadow mode does not support paging out and sharing of pages,
> >> which
> >> are primary reasons why we want synchronized p2m lookups. The hap cases
> >> where there is an inversion of the p2m_lock -> paging_lock order are
> >> reasonably simple to handle.
> >>
> >> The other option is to invert the order and place paging_lock ->
> >> p2m_lock,
> >> but that will raise another set of potential inversions. I think this is
> >> a
> >> no go.
> >
> > Is there scope for having the p2m lock split out into the overall one
> > (needed for allocations &c) and the per-record ones, and have them at
> > different levels in the lock hierarchy?
> I think it's saner to go with one big lock and then shard it if we run
> into performance problems. Your proposal, actually :)
> 
> Now, I'm thinking of splitting ... the paging lock. It covers way too much
> ground right now, and many of these inversions would become more
> tractable. Thinking of paging_alloc_lock, paging_logdirty_lock,
> paging_lock.

...almost exactly six months after I merged the shadow, hap and
log-dirty locks into one lock, to avoid the deadlocks that were caused
by those sections of code calling in to each other. :)

http://xenbits.xen.org/hg/staging/xen-unstable.hg/log?rev=2bbed

Tim.

[PATCH 3 of 3] Make p2m critical sections hold a ref on the underlying mfn

[Andres Lagar-Cavilla]

 xen/arch/x86/mm.c             |  18 +++++++++----
 xen/arch/x86/mm/mem_sharing.c |  13 +++------
 xen/arch/x86/mm/p2m.c         |  56 +++++++++++++++++++++++++++++++++++++++++-
 xen/common/grant_table.c      |   4 +-
 xen/common/memory.c           |  10 +++---
 xen/include/asm-x86/mm.h      |   3 +-
 xen/include/asm-x86/p2m.h     |  10 ++++++-
 xen/include/xen/paging.h      |   2 +-
 xen/include/xen/tmem_xen.h    |   2 +-
 9 files changed, 89 insertions(+), 29 deletions(-)


For translated domains, critical sections demarcated by a
get_gfn/put_gfn pair will hold an additional ref on the
underlying mfn.

This requires keeping tabs on special manipulations that
change said mfn:
 - physmap remove page
 - sharing
 - steal page

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4128,7 +4128,7 @@ static int replace_grant_p2m_mapping(
                  type, mfn_x(old_mfn), frame);
         return GNTST_general_error;
     }
-    guest_physmap_remove_page(d, gfn, frame, 0);
+    guest_physmap_remove_page(d, gfn, frame, 0, HOLDING_GFN);
 
     put_gfn(d, gfn);
     return GNTST_okay;
@@ -4248,8 +4248,10 @@ int donate_page(
 }
 
 int steal_page(
-    struct domain *d, struct page_info *page, unsigned int memflags)
+    struct domain *d, struct page_info *page, unsigned int memflags,
+    int holding_gfn)
 {
+    unsigned count;
     unsigned long x, y;
     bool_t drop_dom_ref = 0;
 
@@ -4261,11 +4263,14 @@ int steal_page(
     /*
      * We require there is just one reference (PGC_allocated). We temporarily
      * drop this reference now so that we can safely swizzle the owner.
+     * If, however, this is invoked by a caller holding the p2m entry, there
+     * will be another reference.
      */
+    count = (holding_gfn) ? 1 : 2;
     y = page->count_info;
     do {
         x = y;
-        if ( (x & (PGC_count_mask|PGC_allocated)) != (1 | PGC_allocated) )
+        if ( (x & (PGC_count_mask|PGC_allocated)) != (count | PGC_allocated) )
             goto fail;
         y = cmpxchg(&page->count_info, x, x & ~PGC_count_mask);
     } while ( y != x );
@@ -4276,7 +4281,7 @@ int steal_page(
     do {
         x = y;
         BUG_ON((x & (PGC_count_mask|PGC_allocated)) != PGC_allocated);
-    } while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
+    } while ( (y = cmpxchg(&page->count_info, x, x | count)) != x );
 
     /* Unlink from original owner. */
     if ( !(memflags & MEMF_no_refcount) && !--d->tot_pages )
@@ -4779,7 +4784,8 @@ long arch_memory_op(int op, XEN_GUEST_HA
         {
             if ( is_xen_heap_mfn(prev_mfn) )
                 /* Xen heap frames are simply unhooked from this phys slot. */
-                guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0);
+                guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0, 
+                                            HOLDING_GFN);
             else
                 /* Normal domain memory is freed, to avoid leaking memory. */
                 guest_remove_page(d, xatp.gpfn);
@@ -4791,7 +4797,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
         gpfn = get_gpfn_from_mfn(mfn);
         ASSERT( gpfn != SHARED_M2P_ENTRY );
         if ( gpfn != INVALID_M2P_ENTRY )
-            guest_physmap_remove_page(d, gpfn, mfn, 0);
+            guest_physmap_remove_page(d, gpfn, mfn, 0, (gpfn == gfn));
 
         /* Map at new location. */
         rc = guest_physmap_add_page(d, xatp.gpfn, mfn, 0);
diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm/mem_sharing.c
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -561,7 +561,7 @@ int mem_sharing_share_pages(shr_handle_t
         list_del(&gfn->list);
         d = get_domain_by_id(gfn->domain);
         BUG_ON(!d);
-        BUG_ON(set_shared_p2m_entry(d, gfn->gfn, se->mfn) == 0);
+        BUG_ON(set_shared_p2m_entry(d, gfn->gfn, se->mfn, 0) == 0);
         put_domain(d);
         list_add(&gfn->list, &se->gfns);
         put_page_and_type(cpage);
@@ -670,14 +670,9 @@ gfn_found:
     unmap_domain_page(s);
     unmap_domain_page(t);
 
-    /* NOTE: set_shared_p2m_entry will switch the underlying mfn. If
-     * we do get_page withing get_gfn, the correct sequence here
-     * should be
-       get_page(page);
-       put_page(old_page);
-     * so that the ref to the old page is dropped, and a ref to
-     * the new page is obtained to later be dropped in put_gfn */
-    BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page)) == 0);
+    /* NOTE: set_shared_p2m_entry will swap the underlying mfn and the refs. 
+     * It is safe to call put_gfn as usual after this. */
+    BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page), HOLDING_GFN) == 0);
     put_page_and_type(old_page);
 
 private_page_found:    
diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -182,6 +182,13 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
     }
 #endif
 
+    /* Do a get page to get one additional ref on the mfn */
+    if ( mfn_valid(mfn) )
+    {
+        struct page_info *pg = mfn_to_page(mfn);
+        BUG_ON( !page_get_owner_and_reference(pg) );
+    }
+
     return mfn;
 }
 
@@ -195,6 +202,21 @@ void put_gfn(struct domain *d, unsigned 
 
     ASSERT(p2m_locked_by_me(p2m));
 
+    {
+        p2m_access_t a;
+        p2m_type_t t;
+        mfn_t mfn = p2m->get_entry(p2m, gfn, &t, &a, 
+                                    p2m_query, NULL);
+
+        if ( mfn_valid(mfn) )
+        {
+#ifdef __x86_64__
+            if (likely( !(p2m_is_broken(t)) ))
+#endif
+                put_page(mfn_to_page(mfn));
+        }
+    }    
+
     p2m_unlock(p2m);
 }
 
@@ -416,6 +438,28 @@ void p2m_final_teardown(struct domain *d
     p2m_teardown_nestedp2m(d);
 }
 
+/* If the caller has done get_gfn on this entry, then it has a ref on the
+ * old mfn. Swap the refs so put_gfn puts the appropriate ref */
+static inline void __p2m_swap_entry_refs(struct p2m_domain *p2m, 
+                                         unsigned long gfn, mfn_t mfn)
+{
+    p2m_type_t t;
+    p2m_access_t a;
+    mfn_t omfn;
+
+    if ( !paging_mode_translate(p2m->domain) )
+        return;
+
+    ASSERT(gfn_locked_by_me(p2m, gfn));
+
+    omfn = p2m->get_entry(p2m, gfn, &t, &a, p2m_query, NULL);
+
+    if ( mfn_valid(mfn) )
+        BUG_ON( !page_get_owner_and_reference(mfn_to_page(mfn)) );
+
+    if ( mfn_valid(omfn) )
+        put_page(mfn_to_page(omfn));
+}
 
 static void
 p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
@@ -451,11 +495,14 @@ p2m_remove_page(struct p2m_domain *p2m, 
 
 void
 guest_physmap_remove_page(struct domain *d, unsigned long gfn,
-                          unsigned long mfn, unsigned int page_order)
+                          unsigned long mfn, unsigned int page_order,
+                          int holding_gfn)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_lock(p2m);
     audit_p2m(p2m, 1);
+    if (holding_gfn)
+        __p2m_swap_entry_refs(p2m, gfn, _mfn(INVALID_MFN));
     p2m_remove_page(p2m, gfn, mfn, page_order);
     audit_p2m(p2m, 1);
     p2m_unlock(p2m);
@@ -713,7 +760,8 @@ out:
 }
 
 int
-set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
+set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, 
+                        int holding_gfn)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     int rc = 0;
@@ -730,6 +778,10 @@ set_shared_p2m_entry(struct domain *d, u
      * sharable first */
     ASSERT(p2m_is_shared(ot));
     ASSERT(mfn_valid(omfn));
+
+    if ( holding_gfn )
+        __p2m_swap_entry_refs(p2m, gfn, mfn);
+
     /* XXX: M2P translations have to be handled properly for shared pages */
     set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
 
diff -r 6203a0549d8a -r 4b684fa74636 xen/common/grant_table.c
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1497,7 +1497,7 @@ gnttab_transfer(
             goto copyback;
         }
 
-        if ( steal_page(d, page, 0) < 0 )
+        if ( steal_page(d, page, 0, HOLDING_GFN) < 0 )
         {
             put_gfn(d, gop.mfn);
             gop.status = GNTST_bad_page;
@@ -1505,7 +1505,7 @@ gnttab_transfer(
         }
 
 #ifndef __ia64__ /* IA64 implicitly replaces the old page in steal_page(). */
-        guest_physmap_remove_page(d, gop.mfn, mfn, 0);
+        guest_physmap_remove_page(d, gop.mfn, mfn, 0, HOLDING_GFN);
 #endif
         flush_tlb_mask(d->domain_dirty_cpumask);
 
diff -r 6203a0549d8a -r 4b684fa74636 xen/common/memory.c
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -165,7 +165,7 @@ int guest_remove_page(struct domain *d, 
     mfn = mfn_x(get_gfn(d, gmfn, &p2mt)); 
     if ( unlikely(p2m_is_paging(p2mt)) )
     {
-        guest_physmap_remove_page(d, gmfn, mfn, 0);
+        guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
         p2m_mem_paging_drop_page(d, gmfn);
         put_gfn(d, gmfn);
         return 1;
@@ -188,7 +188,7 @@ int guest_remove_page(struct domain *d, 
     if(p2m_is_shared(p2mt))
     {
         put_page_and_type(page);
-        guest_physmap_remove_page(d, gmfn, mfn, 0);
+        guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
         put_gfn(d, gmfn);
         return 1;
     }
@@ -207,7 +207,7 @@ int guest_remove_page(struct domain *d, 
     if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
         put_page(page);
 
-    guest_physmap_remove_page(d, gmfn, mfn, 0);
+    guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
     put_gfn(d, gmfn);
 
     put_page(page);
@@ -387,7 +387,7 @@ static long memory_exchange(XEN_GUEST_HA
 
                 page = mfn_to_page(mfn);
 
-                if ( unlikely(steal_page(d, page, MEMF_no_refcount)) )
+                if ( unlikely(steal_page(d, page, MEMF_no_refcount, HOLDING_GFN)) )
                 {
                     put_gfn(d, gmfn + k);
                     rc = -EINVAL;
@@ -427,7 +427,7 @@ static long memory_exchange(XEN_GUEST_HA
             gfn = mfn_to_gmfn(d, mfn);
             /* Pages were unshared above */
             BUG_ON(SHARED_M2P(gfn));
-            guest_physmap_remove_page(d, gfn, mfn, 0);
+            guest_physmap_remove_page(d, gfn, mfn, 0, 0);
             put_page(page);
         }
 
diff -r 6203a0549d8a -r 4b684fa74636 xen/include/asm-x86/mm.h
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -578,7 +578,8 @@ int compat_arch_memory_op(int op, XEN_GU
 int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE(void));
 
 int steal_page(
-    struct domain *d, struct page_info *page, unsigned int memflags);
+    struct domain *d, struct page_info *page, unsigned int memflags,
+    int holding_gfn);
 int donate_page(
     struct domain *d, struct page_info *page, unsigned int memflags);
 int page_make_sharable(struct domain *d, 
diff -r 6203a0549d8a -r 4b684fa74636 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -353,6 +353,10 @@ static inline unsigned long get_gfn_unty
 /* Will release the p2m_lock and put the page behind this mapping. */
 void put_gfn(struct domain *d, unsigned long gfn);
 
+/* Operations that change the underlying mfn in a p2m entry need to be 
+ * told whether the caller is holding the current gfn */
+#define HOLDING_GFN 1
+
 /* The intent is to have the caller not worry about put_gfn. They apply to 
  * very specific situations: debug printk's, dumps during a domain crash,
  * or to peek at a p2m entry/type. Caller is not holding the p2m entry 
@@ -410,7 +414,8 @@ static inline int guest_physmap_add_page
 /* Remove a page from a domain's p2m table */
 void guest_physmap_remove_page(struct domain *d,
                                unsigned long gfn,
-                               unsigned long mfn, unsigned int page_order);
+                               unsigned long mfn, unsigned int page_order,
+                               int holding_gfn);
 
 /* Set a p2m range as populate-on-demand */
 int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
@@ -471,7 +476,8 @@ p2m_pod_offline_or_broken_replace(struct
 
 #ifdef __x86_64__
 /* Modify p2m table for shared gfn */
-int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
+int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
+                            int holding_gfn);
 
 /* Check if a nominated gfn is valid to be paged out */
 int p2m_mem_paging_nominate(struct domain *d, unsigned long gfn);
diff -r 6203a0549d8a -r 4b684fa74636 xen/include/xen/paging.h
--- a/xen/include/xen/paging.h
+++ b/xen/include/xen/paging.h
@@ -21,7 +21,7 @@
 #define paging_mode_translate(d)              (0)
 #define paging_mode_external(d)               (0)
 #define guest_physmap_add_page(d, p, m, o)    (0)
-#define guest_physmap_remove_page(d, p, m, o) ((void)0)
+#define guest_physmap_remove_page(d, p, m, o, h)    ((void)0)
 
 #endif
 
diff -r 6203a0549d8a -r 4b684fa74636 xen/include/xen/tmem_xen.h
--- a/xen/include/xen/tmem_xen.h
+++ b/xen/include/xen/tmem_xen.h
@@ -198,7 +198,7 @@ static inline void _tmh_free_page_thispo
     struct domain *d = page_get_owner(pi);
 
     ASSERT(IS_VALID_PAGE(pi));
-    if ( (d == NULL) || steal_page(d,pi,0) == 0 )
+    if ( (d == NULL) || steal_page(d,pi,0,0) == 0 )
         tmh_page_list_put(pi);
     else
     {

Re: [PATCH 3 of 3] Make p2m critical sections hold a ref on the underlying mfn

[Tim Deegan]

Hi, 

At 16:42 -0500 on 08 Nov (1320770546), Andres Lagar-Cavilla wrote:
> For translated domains, critical sections demarcated by a
> get_gfn/put_gfn pair will hold an additional ref on the
> underlying mfn.

Remind me what this gets us again?  Is it just belt-and-braces on top of
the locking at the p2m layer?

It should be possible to do this without the extra argument - for
example, the bottom-level function that actually changes a p2m entry
must hold the p2m exclusion lock for that entry, or the master p2m lock,
right?  In either case it knows whether it has a ref on the mfn.

I suppose having these locks be recursive makes that a problem, but in
that case you have another problem -- how many mfn refs need to be
moved?

Cheers,

Tim (confused).

> This requires keeping tabs on special manipulations that
> change said mfn:
>  - physmap remove page
>  - sharing
>  - steal page
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm.c
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4128,7 +4128,7 @@ static int replace_grant_p2m_mapping(
>                   type, mfn_x(old_mfn), frame);
>          return GNTST_general_error;
>      }
> -    guest_physmap_remove_page(d, gfn, frame, 0);
> +    guest_physmap_remove_page(d, gfn, frame, 0, HOLDING_GFN);
>  
>      put_gfn(d, gfn);
>      return GNTST_okay;
> @@ -4248,8 +4248,10 @@ int donate_page(
>  }
>  
>  int steal_page(
> -    struct domain *d, struct page_info *page, unsigned int memflags)
> +    struct domain *d, struct page_info *page, unsigned int memflags,
> +    int holding_gfn)
>  {
> +    unsigned count;
>      unsigned long x, y;
>      bool_t drop_dom_ref = 0;
>  
> @@ -4261,11 +4263,14 @@ int steal_page(
>      /*
>       * We require there is just one reference (PGC_allocated). We temporarily
>       * drop this reference now so that we can safely swizzle the owner.
> +     * If, however, this is invoked by a caller holding the p2m entry, there
> +     * will be another reference.
>       */
> +    count = (holding_gfn) ? 1 : 2;
>      y = page->count_info;
>      do {
>          x = y;
> -        if ( (x & (PGC_count_mask|PGC_allocated)) != (1 | PGC_allocated) )
> +        if ( (x & (PGC_count_mask|PGC_allocated)) != (count | PGC_allocated) )
>              goto fail;
>          y = cmpxchg(&page->count_info, x, x & ~PGC_count_mask);
>      } while ( y != x );
> @@ -4276,7 +4281,7 @@ int steal_page(
>      do {
>          x = y;
>          BUG_ON((x & (PGC_count_mask|PGC_allocated)) != PGC_allocated);
> -    } while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
> +    } while ( (y = cmpxchg(&page->count_info, x, x | count)) != x );
>  
>      /* Unlink from original owner. */
>      if ( !(memflags & MEMF_no_refcount) && !--d->tot_pages )
> @@ -4779,7 +4784,8 @@ long arch_memory_op(int op, XEN_GUEST_HA
>          {
>              if ( is_xen_heap_mfn(prev_mfn) )
>                  /* Xen heap frames are simply unhooked from this phys slot. */
> -                guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0);
> +                guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0, 
> +                                            HOLDING_GFN);
>              else
>                  /* Normal domain memory is freed, to avoid leaking memory. */
>                  guest_remove_page(d, xatp.gpfn);
> @@ -4791,7 +4797,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
>          gpfn = get_gpfn_from_mfn(mfn);
>          ASSERT( gpfn != SHARED_M2P_ENTRY );
>          if ( gpfn != INVALID_M2P_ENTRY )
> -            guest_physmap_remove_page(d, gpfn, mfn, 0);
> +            guest_physmap_remove_page(d, gpfn, mfn, 0, (gpfn == gfn));
>  
>          /* Map at new location. */
>          rc = guest_physmap_add_page(d, xatp.gpfn, mfn, 0);
> diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm/mem_sharing.c
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -561,7 +561,7 @@ int mem_sharing_share_pages(shr_handle_t
>          list_del(&gfn->list);
>          d = get_domain_by_id(gfn->domain);
>          BUG_ON(!d);
> -        BUG_ON(set_shared_p2m_entry(d, gfn->gfn, se->mfn) == 0);
> +        BUG_ON(set_shared_p2m_entry(d, gfn->gfn, se->mfn, 0) == 0);
>          put_domain(d);
>          list_add(&gfn->list, &se->gfns);
>          put_page_and_type(cpage);
> @@ -670,14 +670,9 @@ gfn_found:
>      unmap_domain_page(s);
>      unmap_domain_page(t);
>  
> -    /* NOTE: set_shared_p2m_entry will switch the underlying mfn. If
> -     * we do get_page withing get_gfn, the correct sequence here
> -     * should be
> -       get_page(page);
> -       put_page(old_page);
> -     * so that the ref to the old page is dropped, and a ref to
> -     * the new page is obtained to later be dropped in put_gfn */
> -    BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page)) == 0);
> +    /* NOTE: set_shared_p2m_entry will swap the underlying mfn and the refs. 
> +     * It is safe to call put_gfn as usual after this. */
> +    BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page), HOLDING_GFN) == 0);
>      put_page_and_type(old_page);
>  
>  private_page_found:    
> diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -182,6 +182,13 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
>      }
>  #endif
>  
> +    /* Do a get page to get one additional ref on the mfn */
> +    if ( mfn_valid(mfn) )
> +    {
> +        struct page_info *pg = mfn_to_page(mfn);
> +        BUG_ON( !page_get_owner_and_reference(pg) );
> +    }
> +
>      return mfn;
>  }
>  
> @@ -195,6 +202,21 @@ void put_gfn(struct domain *d, unsigned 
>  
>      ASSERT(p2m_locked_by_me(p2m));
>  
> +    {
> +        p2m_access_t a;
> +        p2m_type_t t;
> +        mfn_t mfn = p2m->get_entry(p2m, gfn, &t, &a, 
> +                                    p2m_query, NULL);
> +
> +        if ( mfn_valid(mfn) )
> +        {
> +#ifdef __x86_64__
> +            if (likely( !(p2m_is_broken(t)) ))
> +#endif
> +                put_page(mfn_to_page(mfn));
> +        }
> +    }    
> +
>      p2m_unlock(p2m);
>  }
>  
> @@ -416,6 +438,28 @@ void p2m_final_teardown(struct domain *d
>      p2m_teardown_nestedp2m(d);
>  }
>  
> +/* If the caller has done get_gfn on this entry, then it has a ref on the
> + * old mfn. Swap the refs so put_gfn puts the appropriate ref */
> +static inline void __p2m_swap_entry_refs(struct p2m_domain *p2m, 
> +                                         unsigned long gfn, mfn_t mfn)
> +{
> +    p2m_type_t t;
> +    p2m_access_t a;
> +    mfn_t omfn;
> +
> +    if ( !paging_mode_translate(p2m->domain) )
> +        return;
> +
> +    ASSERT(gfn_locked_by_me(p2m, gfn));
> +
> +    omfn = p2m->get_entry(p2m, gfn, &t, &a, p2m_query, NULL);
> +
> +    if ( mfn_valid(mfn) )
> +        BUG_ON( !page_get_owner_and_reference(mfn_to_page(mfn)) );
> +
> +    if ( mfn_valid(omfn) )
> +        put_page(mfn_to_page(omfn));
> +}
>  
>  static void
>  p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
> @@ -451,11 +495,14 @@ p2m_remove_page(struct p2m_domain *p2m, 
>  
>  void
>  guest_physmap_remove_page(struct domain *d, unsigned long gfn,
> -                          unsigned long mfn, unsigned int page_order)
> +                          unsigned long mfn, unsigned int page_order,
> +                          int holding_gfn)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      p2m_lock(p2m);
>      audit_p2m(p2m, 1);
> +    if (holding_gfn)
> +        __p2m_swap_entry_refs(p2m, gfn, _mfn(INVALID_MFN));
>      p2m_remove_page(p2m, gfn, mfn, page_order);
>      audit_p2m(p2m, 1);
>      p2m_unlock(p2m);
> @@ -713,7 +760,8 @@ out:
>  }
>  
>  int
> -set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
> +set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, 
> +                        int holding_gfn)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      int rc = 0;
> @@ -730,6 +778,10 @@ set_shared_p2m_entry(struct domain *d, u
>       * sharable first */
>      ASSERT(p2m_is_shared(ot));
>      ASSERT(mfn_valid(omfn));
> +
> +    if ( holding_gfn )
> +        __p2m_swap_entry_refs(p2m, gfn, mfn);
> +
>      /* XXX: M2P translations have to be handled properly for shared pages */
>      set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
>  
> diff -r 6203a0549d8a -r 4b684fa74636 xen/common/grant_table.c
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1497,7 +1497,7 @@ gnttab_transfer(
>              goto copyback;
>          }
>  
> -        if ( steal_page(d, page, 0) < 0 )
> +        if ( steal_page(d, page, 0, HOLDING_GFN) < 0 )
>          {
>              put_gfn(d, gop.mfn);
>              gop.status = GNTST_bad_page;
> @@ -1505,7 +1505,7 @@ gnttab_transfer(
>          }
>  
>  #ifndef __ia64__ /* IA64 implicitly replaces the old page in steal_page(). */
> -        guest_physmap_remove_page(d, gop.mfn, mfn, 0);
> +        guest_physmap_remove_page(d, gop.mfn, mfn, 0, HOLDING_GFN);
>  #endif
>          flush_tlb_mask(d->domain_dirty_cpumask);
>  
> diff -r 6203a0549d8a -r 4b684fa74636 xen/common/memory.c
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -165,7 +165,7 @@ int guest_remove_page(struct domain *d, 
>      mfn = mfn_x(get_gfn(d, gmfn, &p2mt)); 
>      if ( unlikely(p2m_is_paging(p2mt)) )
>      {
> -        guest_physmap_remove_page(d, gmfn, mfn, 0);
> +        guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
>          p2m_mem_paging_drop_page(d, gmfn);
>          put_gfn(d, gmfn);
>          return 1;
> @@ -188,7 +188,7 @@ int guest_remove_page(struct domain *d, 
>      if(p2m_is_shared(p2mt))
>      {
>          put_page_and_type(page);
> -        guest_physmap_remove_page(d, gmfn, mfn, 0);
> +        guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
>          put_gfn(d, gmfn);
>          return 1;
>      }
> @@ -207,7 +207,7 @@ int guest_remove_page(struct domain *d, 
>      if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
>          put_page(page);
>  
> -    guest_physmap_remove_page(d, gmfn, mfn, 0);
> +    guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
>      put_gfn(d, gmfn);
>  
>      put_page(page);
> @@ -387,7 +387,7 @@ static long memory_exchange(XEN_GUEST_HA
>  
>                  page = mfn_to_page(mfn);
>  
> -                if ( unlikely(steal_page(d, page, MEMF_no_refcount)) )
> +                if ( unlikely(steal_page(d, page, MEMF_no_refcount, HOLDING_GFN)) )
>                  {
>                      put_gfn(d, gmfn + k);
>                      rc = -EINVAL;
> @@ -427,7 +427,7 @@ static long memory_exchange(XEN_GUEST_HA
>              gfn = mfn_to_gmfn(d, mfn);
>              /* Pages were unshared above */
>              BUG_ON(SHARED_M2P(gfn));
> -            guest_physmap_remove_page(d, gfn, mfn, 0);
> +            guest_physmap_remove_page(d, gfn, mfn, 0, 0);
>              put_page(page);
>          }
>  
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/asm-x86/mm.h
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -578,7 +578,8 @@ int compat_arch_memory_op(int op, XEN_GU
>  int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE(void));
>  
>  int steal_page(
> -    struct domain *d, struct page_info *page, unsigned int memflags);
> +    struct domain *d, struct page_info *page, unsigned int memflags,
> +    int holding_gfn);
>  int donate_page(
>      struct domain *d, struct page_info *page, unsigned int memflags);
>  int page_make_sharable(struct domain *d, 
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/asm-x86/p2m.h
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -353,6 +353,10 @@ static inline unsigned long get_gfn_unty
>  /* Will release the p2m_lock and put the page behind this mapping. */
>  void put_gfn(struct domain *d, unsigned long gfn);
>  
> +/* Operations that change the underlying mfn in a p2m entry need to be 
> + * told whether the caller is holding the current gfn */
> +#define HOLDING_GFN 1
> +
>  /* The intent is to have the caller not worry about put_gfn. They apply to 
>   * very specific situations: debug printk's, dumps during a domain crash,
>   * or to peek at a p2m entry/type. Caller is not holding the p2m entry 
> @@ -410,7 +414,8 @@ static inline int guest_physmap_add_page
>  /* Remove a page from a domain's p2m table */
>  void guest_physmap_remove_page(struct domain *d,
>                                 unsigned long gfn,
> -                               unsigned long mfn, unsigned int page_order);
> +                               unsigned long mfn, unsigned int page_order,
> +                               int holding_gfn);
>  
>  /* Set a p2m range as populate-on-demand */
>  int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
> @@ -471,7 +476,8 @@ p2m_pod_offline_or_broken_replace(struct
>  
>  #ifdef __x86_64__
>  /* Modify p2m table for shared gfn */
> -int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
> +int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
> +                            int holding_gfn);
>  
>  /* Check if a nominated gfn is valid to be paged out */
>  int p2m_mem_paging_nominate(struct domain *d, unsigned long gfn);
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/xen/paging.h
> --- a/xen/include/xen/paging.h
> +++ b/xen/include/xen/paging.h
> @@ -21,7 +21,7 @@
>  #define paging_mode_translate(d)              (0)
>  #define paging_mode_external(d)               (0)
>  #define guest_physmap_add_page(d, p, m, o)    (0)
> -#define guest_physmap_remove_page(d, p, m, o) ((void)0)
> +#define guest_physmap_remove_page(d, p, m, o, h)    ((void)0)
>  
>  #endif
>  
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/xen/tmem_xen.h
> --- a/xen/include/xen/tmem_xen.h
> +++ b/xen/include/xen/tmem_xen.h
> @@ -198,7 +198,7 @@ static inline void _tmh_free_page_thispo
>      struct domain *d = page_get_owner(pi);
>  
>      ASSERT(IS_VALID_PAGE(pi));
> -    if ( (d == NULL) || steal_page(d,pi,0) == 0 )
> +    if ( (d == NULL) || steal_page(d,pi,0,0) == 0 )
>          tmh_page_list_put(pi);
>      else
>      {
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel