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

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

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 )".

>          *flush = true;
>  
> 

Regards,

-- 
Julien Grall