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

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2014-06-11 at 23:19 +0100, Julien Grall wrote:
> 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?

"directory common"? I don't get your meaning.

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

Yes, this probably hasn't been executed. I'll see about forcing it.

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

What I meant was that an L3 entry is always "superpage aligned" because
it is the smallest possible element. Since I wrote that I renamed my
is_superpage_aligned function to is_mapping_aligned. I should perhaps
update this comment to reflect that, which would make it clearer.

> [..]
> 
> > +    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).

True.

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

It can't because mfn_to_p2m_entry is used to create both table and
mapping style entries.
> 
> [..]
> 
> > +        }
> > +        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?

s/redo/undo/?

> > 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?

I guess one isn't really needed.

> > +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);
> >
> >
>