Re: [V4 PATCH 6/7] pvh dom0: Add and remove foreign pages

[Julien Grall <julien.grall@linaro.org>]

On 12/04/2013 01:05 AM, Mukesh Rathor wrote:
> On Wed, 04 Dec 2013 00:00:54 +0000
> Julien Grall <julien.grall@linaro.org> wrote:
> 
>>
>>
>> On 12/03/2013 02:30 AM, Mukesh Rathor wrote:
>>> In this patch, a new function, xenmem_add_foreign_to_pmap(), is
>>> added
>>
>> xenmem_add_foreign_to_p2m?
>>
>>> to map pages from foreign guest into current dom0 for domU creation.
>>> Such pages are typed p2m_map_foreign. Also, support is added here to
>>> XENMEM_remove_from_physmap to remove such pages. Note, in the remove
>>> path, we must release the refcount that was taken during the map
>>> phase.
>>
>> Your remove path is very interesting for the ARM port. For now we are 
>> unable to unmap the foreign page because get_page() will always
>> return NULL (as dom0 is not the owner).
>>
>> I will give a try on ARM to see if it could resolve our problem.
>>
> 
> Don't know much about ARM, is the remove path failing to compile on it?
> You can submit ARM modification patch after it's checked in?

So I have reworked common/memory.c part to be able to boot on ARM. What about this fix:

diff --git a/xen/common/memory.c b/xen/common/memory.c
index df36d43..8ceb78b 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -713,9 +713,11 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case XENMEM_remove_from_physmap:
     {
+        unsigned long mfn;
         struct xen_remove_from_physmap xrfp;
         struct page_info *page;
         struct domain *d;
+        p2m_type_t p2mt;
 
         if ( copy_from_guest(&xrfp, arg, 1) )
             return -EFAULT;
@@ -731,11 +733,30 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             return rc;
         }
 
-        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
-        if ( page )
+        /*
+         * if PVH, the gfn could be mapped to a mfn from foreign domain by the
+         * user space tool during domain creation. We need to check for that,
+         * free it up from the p2m, and release refcnt on it. In such a case,
+         * page would be NULL and the following call would not have refcnt'd
+         * the page. See also xenmem_add_foreign_to_pmap().
+         */
+        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
+
+        if ( page || p2m_is_foreign(p2mt) )
         {
-            guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
-            put_page(page);
+            if ( page )
+                mfn = page_to_mfn(page);
+            else
+            {
+                mfn = gmfn_to_mfn(d, xrfp.gpfn);
+                page = mfn_to_page(mfn);
+
+                ASSERT(d != page_get_owner(page));
+            }
+
+            guest_physmap_remove_page(d, xrfp.gpfn, mfn, 0);
+            if (page)
+                put_page(page);
         }
         else
             rc = -ENOENT;

This change is assuming that:
  - p2m_is_foreign is defined on ARM (I have a patch for that)
  - gmfn_to_mfn is defined on x86 (I don't have a patch, but the code should be trivial?)
  - ARM change in xenmem_add_to_physmap to take refcount on the foreign map

I also have a bunch of intrusive ARM patch to handle correctly foreign mapping.

Mukesh: This small fix should go to this patch:
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 177f2e5..fc5ab8f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1020,8 +1020,8 @@ static int xenmem_add_to_physmap_one(
         break;
     case XENMAPSPACE_gmfn_foreign:
     {
-        paddr_t maddr;
         struct domain *od;
+        struct page_info *page;
         od = rcu_lock_domain_by_any_id(foreign_domid);
         if ( od == NULL )
             return -ESRCH;
@@ -1033,15 +1033,17 @@ static int xenmem_add_to_physmap_one(
             return rc;
         }
 
-        maddr = p2m_lookup(od, pfn_to_paddr(idx));
-        if ( maddr == INVALID_PADDR )
+        /* Take refcount to the foreign domain page.
+         * Refcount will be release in XENMEM_remove_from_physmap */
+        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);
+        if ( !page )
         {
             dump_p2m_lookup(od, pfn_to_paddr(idx));
             rcu_unlock_domain(od);
             return -EINVAL;
         }
 
-        mfn = maddr >> PAGE_SHIFT;
+        mfn = page_to_mfn(page);
         t = p2m_map_foreign;
 
         rcu_unlock_domain(od);

-- 
Julien Grall