Re: [PATCH 7/8] xen/arm: Set foreign page type to p2m_map_foreign

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

On 12/05/2013 04:54 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 16:41 +0000, Julien Grall wrote:
>
>>>>
>>>>        switch ( space )
>>>>        {
>>>> @@ -1019,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;
>>>> @@ -1032,15 +1033,18 @@ 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.
>>>
>>> "on the foreign" (and maybe "domain's" or just "foreign page"
>>>
>>>> +         * Refcount will be release in XENMEM_remove_from_physmap */
>>>
>>> "will be released".
>>>
>>> The refcount will also be removed by p2m_teardown. That probably needs
>>> changing to do an actual walk of the p2m tearing things down, which is a
>>> pain.
>>>
>>> Stefano's now obsolete dma pinning series had a patch which added a
>>> fairly generic walker in it which might be reusable.
>>
>> Do you have a link to this patch?
>
> I was afraid you'd say that...
> /scrobbles
>
> "xen/arm: introduce a generic p2m walker and use it in p2m_lookup". v6
> is msgid
> <1380298560-29352-2-git-send-email-stefano.stabellini@eu.citrix.com>. I
> don't recall if that was the last one though.

Actually the generic walker will not be usefull for p2m_teardown. It 
seems to be an extension of p2m_lookup, so will only search a specific gfn.

>>
>>> The walk might need to support continuations though.
>>>
>>> I wonder if this ref ought to be taken in create_p2m_entries for all
>>> entries and not just foreign ones, and then released in the appropriate
>>> places.
>>
>> If I'm not mistaken, Xen already takes a ref when the page is allocated
>> for the domain. Why would we need to take another ref for these pages?
>
> There's a nice symmetry to all p2m entries taking a ref count, and it
> makes the teardown a bit simpler since you can just drop the ref every
> time without worrying about the type.

If I'm not mistaken, even x86 code doesn't have 2 ref for each page. Or 
I didn't see where the ref is taken/release ...

In any case, walk the whole p2m for looking for present page seems a bit 
tough and slow to release memory for a domain.

I'm wondering if we can have a list of foreign page, and browse it when 
the domain is destroyed.

-- 
Julien Grall