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

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

On 12/16/2013 04:33 PM, Ian Campbell wrote:
> On Mon, 2013-12-16 at 16:26 +0000, Julien Grall wrote:
>>
>> 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);
>
> 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).

This function is only defined for mini-os 
(extras/mini-os/include/x86/arch_mm.h).

>
>> +                    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?

We retrieve the mfn via page_to_mfn, so the mfn should be valid.

>
>> +                        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.

Will do.

>
>> +        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)
> ?

Both are valid. We need to make sure that the page belongs to a domain, 
and then it's not the current domain.

-- 
Julien Grall