Re: [PATCH v5 07/10] xen/arm: Handle remove foreign mapping

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

On 12/17/2013 11:18 AM, Ian Campbell wrote:
> On Mon, 2013-12-16 at 17:37 +0000, Julien Grall wrote:
>> 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 v4.2:
>>         - get_page_from_gfn: move foreign checking before get_page
>>         - add assert fdom != dom
>>     Changes in v4.1:
>>         - 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
>> ---
>>  xen/arch/arm/p2m.c        |   15 +++++++++++++--
>>  xen/include/asm-arm/p2m.h |   12 ++++++++++++
>>  2 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 9bdcacd..d05fdff 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);
> 
> FWIW mfn = pte.p2m.base, I think. I'm not sure we use this everywhere we
> could.

The patch #8 which relinquish memory doesn't give the right mfn in
parameters.
It's because, you will need to look for each gfn the translated mfn (you
can't assumed the mfn are contiguous). So the loop will go back to the
version 3.

> 
>> +
>> +                    /* TODO: Handle other p2m type */
>> +                    if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
> 
> How useful is p2m_is_foreign now that this stuff doesn't need to be in
> common code?

I'm not sure to understand. Do you mean the  macro? If so it's only for
convenience.

> Both of the above are really just observations rather than requests for
> change, so:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks,

-- 
Julien Grall