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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-06-12 at 14:57 +0100, Stefano Stabellini wrote:
> On Wed, 11 Jun 2014, Ian Campbell wrote:
> > +static bool_t is_mapping_aligned(const paddr_t start_gpaddr,
> > +                                 const paddr_t end_gpaddr,
> > +                                 const paddr_t maddr,
> > +                                 const paddr_t level_size)
> > +{
> > +    const paddr_t level_mask = level_size - 1;
> > +
> > +    /* No hardware superpages at level 0 */
> > +    if ( level_size == ZEROETH_SIZE )
> > +        return false;
> > +
> > +    /*
> > +     * A range smaller than the size of a superpage at this level
> > +     * cannot be superpage aligned.
> > +     */
> > +    if ( ( end_gpaddr - start_gpaddr ) < level_size - 1 )
> > +        return false;
> 
> Shouldn't this be
>        if ( ( end_gpaddr - start_gpaddr ) < level_size )
> ?

Perhaps. 

Some of the callers of the parent function use inclusive and some
exclusive end addresses, this helps handle that until it is fixed (I
think Arianna's series does?).

> > +        return P2M_ONE_DESCEND;
> > +
> > +    case INSERT:
> 
> Shouldn't you add a
> 
>         if ( p2m_valid(orig_pte) )
>             return P2M_ONE_DESCEND;
> 
> test here too?

No because we might be able to replace the table with a mapping. (We
don't actually handle that case now, but I was trying to avoid assuming
that we wouldn't)
> 
> 
> > +        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)) )
> 
> Why the difference with ALLOCATE?

On ALLOCATE everything is guaranteed to either be a table mapping or not
present, which isn't true for INSERT.

> > +    case RELINQUISH:
> > +    case REMOVE:
> > +        if ( !p2m_valid(orig_pte) )
> > +        {
> > +            /* Progress up to next boundary */
> > +            *addr = (*addr + level_size) & level_mask;
> > +            return P2M_ONE_PROGRESS_NOP;
> > +        }
> 
> You might as well have a single !p2m_valid check before the switch

That wouldn't make any sense in the INSERT/ALLOCATE cases, where we do
actually want to do something for the !valid case (i.e. make it valid).

> > @@ -374,22 +695,18 @@ static int apply_p2m_changes(struct domain *d,
> >              cur_first_page = p2m_first_level_index(addr);
> >          }
> >  
> > -        if ( !p2m_valid(first[first_table_offset(addr)]) )
> > -        {
> > -            if ( !populate )
> > -            {
> > -                addr = (addr + FIRST_SIZE) & FIRST_MASK;
> > -                continue;
> > -            }
> > +        /* We only use a 3 level p2m at the moment, so no level 0,
> > +         * current hardware doesn't support super page mappings at
> > +         * level 0 anyway */
> >  
> > -            rc = p2m_create_table(d, &first[first_table_offset(addr)],
> > -                                  flush_pt);
> > -            if ( rc < 0 )
> > -            {
> > -                printk("p2m_populate_ram: L1 failed\n");
> > -                goto out;
> > -            }
> > -        }
> > +        rc = apply_one_level(d, &first[first_table_offset(addr)],
> > +                             1, flush_pt, op,
> > +                             start_gpaddr, end_gpaddr,
> > +                             &addr, &maddr, &flush,
> > +                             mattr, t);
> > +        if ( rc < 0 ) goto out;
> > +        count += rc;
> 
> Adding an rc to a counter looks a bit strange.

The return semantics of apply_one_level are pretty clearly documented.
Would you like me to rename the variable to something else? If so then
what?

> > 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
> > @@ -29,6 +29,14 @@ struct p2m_domain {
> >       * resume the search. Apart from during teardown this can only
> >       * decrease. */
> >      unsigned long lowest_mapped_gfn;
> > +
> > +    struct {
> > +        /* Number of mappings at each p2m tree level */
> > +        unsigned long mappings[4];
> > +        /* Number of times we have shattered a mapping
> > +         * at each p2m tree level. */
> > +        unsigned long shattered[4];
> > +    } stats;
> >  };
> 
> Are you introducing stats.mappings just for statistic gathering?
> If so, you should write it in the comment.

I thought the clue would be in the name, but, yes, the field named stats
is for gathering stats. Does it really need a comment?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel