diff mbox

[Xen-devel,for-4.7,v2,2/2] xen/arm: p2m: Release the p2m lock before undoing the mappings

Message ID 1463751462-8015-3-git-send-email-julien.grall@arm.com
State New
Headers show

Commit Message

Julien Grall May 20, 2016, 1:37 p.m. UTC 
Since commit 4b25423a "arch/arm: unmap partially-mapped memory regions",
Xen has been undoing the P2M mappings when an error occurred during
insertion or memory allocation.

This is done by calling recursively apply_p2m_changes, however the
second call is done with the p2m lock taken which will result in a
deadlock for the current processor.

The p2m lock is here to protect 2 threads modifying concurrently the
page tables. However, it does not guarantee the ordering of the
changes. I.e if 2 threads request change on regions that overlaps,
then the result is undefined.

Therefore it is fine to move the recursive call to undo the changes
after the lock is released.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
Tested-by: Wei Chen <Wei.Chen@arm.com>

---
    I think we could unlock the p2m lock before freeing the temporary
    mapping. Although, I played safe as this is a bug fix for Xen 4.7
    and to be backported up to Xen 4.5.

    Changes in v2:
        - Update the commit message to explain why unlocking before
        the recursive call is fine.
        - Add Wei Chen's reviewed-by and tested-by
---
 xen/arch/arm/p2m.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
diff mbox

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 68c67b0..838d004 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1184,6 +1184,14 @@  out:
     while ( (pg = page_list_remove_head(&free_pages)) )
         free_domheap_page(pg);
 
+    for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
+    {
+        if ( mappings[level] )
+            unmap_domain_page(mappings[level]);
+    }
+
+    spin_unlock(&p2m->lock);
+
     if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) &&
          addr != start_gpaddr )
     {
@@ -1196,14 +1204,6 @@  out:
                           mattr, 0, p2m_invalid, d->arch.p2m.default_access);
     }
 
-    for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
-    {
-        if ( mappings[level] )
-            unmap_domain_page(mappings[level]);
-    }
-
-    spin_unlock(&p2m->lock);
-
     return rc;
 }