Re: [PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign

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

On 12/10/2013 09:37 AM, Ian Campbell wrote:
> On Tue, 2013-12-10 at 01:51 +0000, Julien Grall wrote:
>>
>> On 12/09/2013 04:40 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>>>> Xen needs to know that the current page belongs to another domain. Also take
>>>> the refcount to this page.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>>
>>>> ---
>>>>       Changes in v2:
>>>>           - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
>>>>           define p2m type per mapspace to let the compiler warns about unitialized
>>>>           values.
>>>> ---
>>>>    xen/arch/arm/mm.c |   24 ++++++++++++++++--------
>>>>    1 file changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>>> index ba51f6e..b08ffa0 100644
>>>> --- a/xen/arch/arm/mm.c
>>>> +++ b/xen/arch/arm/mm.c
>>>> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
>>>>    {
>>>>        unsigned long mfn = 0;
>>>>        int rc;
>>>> +    p2m_type_t t;
>>>>
>>>>        switch ( space )
>>>>        {
>>>> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
>>>>
>>>>            d->arch.grant_table_gpfn[idx] = gpfn;
>>>>
>>>> +        t = p2m_ram_rw;
>>>> +
>>>>            spin_unlock(&d->grant_table->lock);
>>>>            break;
>>>>        case XENMAPSPACE_shared_info:
>>>> -        if ( idx == 0 )
>>>> -            mfn = virt_to_mfn(d->shared_info);
>>>> -        else
>>>> +        if ( idx != 0 )
>>>>                return -EINVAL;
>>>> +
>>>> +        mfn = virt_to_mfn(d->shared_info);
>>>> +        t = p2m_ram_rw;
>>>> +
>>>>            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;
>>>> @@ -1048,15 +1053,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.
>>>
>>> "refcount on...". Or "Take a reference to..."
>>>
>>>> +         * Refcount will be release in XENMEM_remove_from_physmap */
>>>
>>> and a few other places...
>>>
>>> Like with the unmap it might be best to push this reference manipulation
>>> down into the eventual function which makes the mapping. That would keep
>>> this stuff much more localised.
>>
>> Forget to answer to this part:
>>
>> We need to take the reference with foreign domain in parameter,
>> otherwise we don't really know if the foreign domain has unlikely
>> removed the mapping between the hypercall was called and now.
>
> Are we not holding the p2m lock at this point?

p2m lock of which domain? For now, neither of foreign and current domain 
lock are taken in this function.

> We probably do need to do the final check and unmap with the lock held.
> Either by moving the unmap within the same lock as the lookup or perhaps
> with some sort of cmpxchg mechanism where we pass in the pte value we
> think we are tearing down. Just checking the type may not be sufficient
> if the guest tears down one foreign mapping and replaces it with
> another.

I'm not sure to follow you, are you talking about remove the foreign 
mapping? If so, I have reworking all the removing code to include the 
put_page (see V3).

-- 
Julien Grall