Re: [PATCH v2 7/8] xen: arm: use superpages in p2m when pages are suitably aligned

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

Hi Ian,

While I was looking closer to this patch I found something strange. Why 
all the callers of guest_physmap_add_page in the directory common don't 
check that the function success to create the mapping?

On 11/06/14 17:40, Ian Campbell wrote:
> +            page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
> +            if ( page )
> +            {
> +                pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t);
> +                if ( level != 3 )
> +                    pte.p2m.table = 0;
> +                p2m_write_pte(entry, pte, flush_cache);
> +                p2m->stats.mappings[level]++;

It looks like you forgot to update the *addr here.

Did you try this code path? With the 1:1 workaround this part is never used.

> +                return P2M_ONE_PROGRESS;
> +            }
> +            else if ( level == 3 )
> +                return -ENOMEM;
> +        }
> +
> +        BUG_ON(level == 3); /* L3 is always superpage aligned */

Did you mean page-aligned?

[..]

> +    case INSERT:
> +        if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) &&
> +           /* We do not handle replacing an existing table with a superpage */
> +             (level == 3 || !p2m_table(orig_pte)) )
> +        {
> +            /* New mapping is superpage aligned, make it */
> +            pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t);
> +            if ( level < 3 )

It's funny, sometimes you use level < 3 and some level != 3 (see in 
ALLOCATE).

I think this pte.p2m.table set can be handled directly in 
mfn_to_p2m_entry. This will avoid duplicating code.

[..]

> +        }
> +        else
> +        {
> +            /* New mapping is not superpage aligned, create a new table entry */
> +            BUG_ON(level == 3); /* L3 is always superpage aligned */

Did you mean page-aligned?

[..]

> -        /* Got the next page */
> -        addr += PAGE_SIZE;
> +        rc = apply_one_level(d, &third[third_table_offset(addr)],
> +                             3, flush_pt, op,
> +                             start_gpaddr, end_gpaddr,
> +                             &addr, &maddr, &flush,
> +                             mattr, t);
> +        if ( rc < 0 ) goto out;

Shall we redo the whole range if the mapping has failed here?

[..]

> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 911d32d..821d9ef 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h

[..]

> +/* */

Did you intend to add a comment here?

> +void p2m_dump_info(struct domain *d);
> +
>   /* Look up the MFN corresponding to a domain's PFN. */
>   paddr_t p2m_lookup(struct domain *d, paddr_t gpfn, p2m_type_t *t);
>
>

-- 
Julien Grall