Re: [PATCH 3/3] xen: arm: correctly handle removing a subset of a superpage mapping.

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

On 15/07/14 10:38, Ian Campbell wrote:
> On Mon, 2014-07-14 at 19:34 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 07/14/2014 05:27 PM, Ian Campbell wrote:
>>> This can be exercised for example via ballooning which will remove 4K
>>> regions fromanywhere in the address space.
>>
>> Missing space between "from" and "anywhere"
>>
>>>
>>> Reported-by: Julien Grall <julien.grall@linaro.org>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>   xen/arch/arm/p2m.c |   29 +++++++++++++++++++++++++++--
>>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index a10cbaf..68a19bd 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -604,8 +604,33 @@ static int apply_one_level(struct domain *d,
>>>               return P2M_ONE_PROGRESS_NOP;
>>>           }
>>>
>>> -        if ( level < 3 && p2m_table(orig_pte) )
>>> -            return P2M_ONE_DESCEND;
>>> +        if ( level < 3 )
>>> +        {
>>> +            if ( p2m_table(orig_pte) )
>>> +                return P2M_ONE_DESCEND;
>>> +
>>> +            if ( op == REMOVE &&
>>> +                 !is_mapping_aligned(*addr, end_gpaddr,
>>> +                                     0, /* maddr doesn't matter for remove */
>>> +                                     level_size) )
>>> +            {
>>> +                /*
>>> +                 * Removing a mapping from the middle of a superpage. Shatter
>>> +                 * and descend.
>>> +                 */
>>> +                *flush = true;
>>> +                rc = p2m_create_table(d, entry,
>>> +                                      level_shift - PAGE_SHIFT, flush_cache);
>>> +                if ( rc < 0 )
>>> +                    return rc;
>>> +
>>> +                p2m->stats.shattered[level]++;
>>> +                p2m->stats.mappings[level]--;
>>> +                p2m->stats.mappings[level+1] += LPAE_ENTRIES;
>>> +
>>> +                return P2M_ONE_DESCEND;
>>> +            }
>>> +        }
>>
>> I would put the code below (which is only for level3) in the else part.
>> This will also remove now useless "if ( level == 3 )".
>
> This would break the relinquish case, I think.
>
> Although thinking about it perhaps relinquish should behave like remove
> and shatter. My original thinking was that we only ever relinquish the
> entire address space so we shouldn't really be hitting that case.

Oh right, I though there was a return on every part of this if. Sorry 
for the noise.

With the minor change in the commit message:

Acked-by: Julien Grall <julien.grall@linaro.org>


Regards,

-- 
Julien Grall