[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 08/11] xen/arm: Handle remove foreign mapping





On 12/16/2013 04:33 PM, Ian Campbell wrote:
On Mon, 2013-12-16 at 16:26 +0000, Julien Grall wrote:
I have reworked this patch. I get a simpler patch:

commit aab2e5d2ae7d0fa87c74cae2f22044f87be33f70
Author: Julien Grall <julien.grall@xxxxxxxxxx>
Date:   Fri Dec 13 16:51:03 2013 +0000

     xen/arm: Handle remove foreign mapping

     Modify get_page_from_gfn to take reference on foreign mapping. This will 
avoid
     specific handling in the common code.

     Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>

     ---
         Changes in v5:
             - Remove specific p2m handling in common code
             - Handle foreign mapping in get_page_from_gfn
         Changes in v4:
             - Split patch #6 from dom0 pvh series v6.2 to retrieve only common
             code.
             - Rework commit title
             - Rename xen_rem_foreign_from_p2m in p2m_remove_foreign
             - Get the mfn from the pte. We are not sure that maddr given in
             parameters is valid
         Changes in v3:
             - Move put_page in create_p2m_entries
             - Move xenmem_rem_foreign_from_p2m in arch/arm/p2m.c
         Changes in v2:
             - Introduce the patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 39d8a03..f7bd7e2 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -317,10 +317,21 @@ static int create_p2m_entries(struct domain *d,
                  break;
              case REMOVE:
                  {
-                    lpae_t pte;
+                    lpae_t pte = third[third_table_offset(addr)];
+                    unsigned long mfn;
+
+                    maddr = (pte.bits & PADDR_MASK & PAGE_MASK);

I thought we had a macro for this, but apparently not. While looking for
it I spotted that x86 has pte_to_mfn, which sounds like a useful
innovation... (not essential as part of this series though).

+                    mfn = paddr_to_pfn(maddr);
+
+                    /* TODO: Handle other p2m type */
+                    if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
+                    {
+                        ASSERT(mfn_valid(mfn));

Something somewhere is making sure we don't put foreign MMIO regions
into the p2m, right?

I misread this part. And the answer is still yes because in this case MMIO won't belong to a domain (there is no reference on it), so get_page will return NULL when the foreign mapping is created in xenmem_add_to_physmap_one.

+                        put_page(mfn_to_page(mfn));
+                    }
+
                      memset(&pte, 0x00, sizeof(pte));
                      write_pte(&third[third_table_offset(addr)], pte);
-                    maddr += PAGE_SIZE;
                  }
                  break;
          }
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 0eb07a8..e0b58da 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -122,9 +122,21 @@ static inline struct page_info *get_page_from_gfn(
      if ( !mfn_valid(mfn) )
          return NULL;
      page = mfn_to_page(mfn);
-    if ( !get_page(page, d) )
-        return NULL;
-    return page;
+
+    if ( get_page(page, d) )

This isn't noisy (even at debug level) on failure, I thought so?

Might be safer (and TBH more logical) to move it after the foreign
special case.

+        return page;
+
+    /* get_page won't work on foreign mapping because the page doesn't
+     * belong to the current domain.
+     */
+    if ( p2mt == p2m_map_foreign )
+    {
+        struct domain *fdom = page_get_owner_and_reference(page);
+        ASSERT(fdom != NULL);

ASSERT(fdom != d)
?

+        return page;
+    }
+
+    return NULL;
  }

  int get_page_type(struct page_info *page, unsigned long type);




--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.