Re: [PATCH RFC 6/9] xen: arm: handle variable p2m levels in p2m_lookup

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

Hi Ian,

On 07/30/2014 02:47 PM, Ian Campbell wrote:
> This paves the way for boot-time selection of the number of levels to
> use in the p2m, which is required to support both 40-bit and 48-bit
> systems. For now the starting level remains a compile time constant.
> 
> Implemented by turning the linear sequence of lookups into a loop.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/p2m.c |   70 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 225d125..557663f 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -149,45 +149,69 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
>  paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
> -    lpae_t pte, *first = NULL, *second = NULL, *third = NULL;
> +    const unsigned int offsets[4] = {
> +        zeroeth_table_offset(paddr),
> +        first_table_offset(paddr),
> +        second_table_offset(paddr),
> +        third_table_offset(paddr)
> +    };
> +    const paddr_t masks[4] = {
> +        ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
> +    };
> +    lpae_t pte, *map;
>      paddr_t maddr = INVALID_PADDR;
>      paddr_t mask;
>      p2m_type_t _t;
> +    int level, root_table;

I would use unsigned int here.

> +
> +    BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
>  
>      /* Allow t to be NULL */
>      t = t ?: &_t;
>  
>      *t = p2m_invalid;
>  
> +    if ( P2M_ROOT_PAGES > 1 )
> +    {
> +        /*
> +         * Concatenated root-level tables. The table number will be
> +         * the offset at the previous level. It is not possible to
> +         * concetenate a level-0 root.

concatenate

> +         */
> +        BUG_ON(P2M_ROOT_LEVEL == 0);

I don't think this check is necessary in a production build (ie
debug=n). If people is porting a new board, then it should be done in
debug mode.

Hence it should be a bit "faster" as this code will be called often.

I would switch this BUG_ON to ASSERT.

> +        root_table = offsets[P2M_ROOT_LEVEL - 1];
> +        if ( root_table >= P2M_ROOT_PAGES )
> +            goto err;
> +    }
> +    else
> +        root_table = 0;
> +
>      spin_lock(&p2m->lock);
>  
> -    first = p2m_map_first(p2m, paddr);
> -    if ( !first )
> -        goto err;
> +    map = __map_domain_page(p2m->root + root_table);
>  
> -    mask = FIRST_MASK;
> -    pte = first[first_table_offset(paddr)];
> -    if ( !p2m_table(pte) )
> -        goto done;
> +    for ( level = P2M_ROOT_LEVEL ; level < 4 ; level++ )
> +    {
> +        mask = masks[level];
>  
> -    mask = SECOND_MASK;
> -    second = map_domain_page(pte.p2m.base);
> -    pte = second[second_table_offset(paddr)];
> -    if ( !p2m_table(pte) )
> -        goto done;
> +        pte = map[offsets[level]];
>  
> -    mask = THIRD_MASK;
> +        if ( level == 3 && !p2m_table(pte) )
> +            /* Invalid, clobber the pte */
> +            pte.bits = 0;
> +        if ( level == 3 || !p2m_table(pte) )
> +            /* Done */
> +            break;
>  
> -    BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
> +        BUG_ON(level == 3);

Rather than checking level == 3, I would add
BUG_ON(level < 4) at the end of loop.
The only drawback to be there is we map one more page.

And same remark as the previous BUG_ON().

Regards,

-- 
Julien Grall