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

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

On 12/16/2013 03:40 PM, Ian Campbell wrote:
> On Mon, 2013-12-16 at 15:34 +0000, Julien Grall wrote:
>>
>> On 12/16/2013 11:51 AM, Tim Deegan wrote:
>>> At 19:37 +0000 on 13 Dec (1386959858), Julien Grall wrote:
>>>> @@ -693,12 +694,21 @@ 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 autotranslate guest, (eg 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.
>>>> +         */
>>>> +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
>>>>            if ( page )
>>>>            {
>>>>                guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
>>>>                put_page(page);
>>>>            }
>>>> +        else if ( p2m_is_foreign(p2mt) )
>>>> +            rc = p2m_remove_foreign(d, xrfp.gpfn);
>>>
>>> This doesn't seem like the right interface -- having special cases
>>> like this in the callers is something we slipped into in x86 for a lot
>>> of the paging/sharing code and it's not nice.  I think maybe we can
>>> just have get_page_from_gfn() DTRT for foreign (and grant) entries.
>>>
>>> Also, the comment will have been out of data by the time the x86
>>> version of this code is finished, as we won't be handling the refcount
>>> at this level. :)
>>
>> I will remove the comment and modify get_page_from_gfn to handle foreign
>> mapping.
> 
> You'll want to coordinate with Mukesh on that latter I think.
> 
> Ian.
> 
> 

I have reworked this patch. I get a simpler patch:

commit aab2e5d2ae7d0fa87c74cae2f22044f87be33f70
Author: Julien Grall <julien.grall@linaro.org>
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@linaro.org>
    
    ---
        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);
+                    mfn = paddr_to_pfn(maddr);
+
+                    /* TODO: Handle other p2m type */
+                    if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
+                    {
+                        ASSERT(mfn_valid(mfn));
+                        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) )
+        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);
+        return page;
+    }
+
+    return NULL;
 }
 
 int get_page_type(struct page_info *page, unsigned long type);

-- 
Julien Grall