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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 12/06/14 08:30, Ian Campbell wrote:
> 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.
Sorry, I meant xen/common/

> > > +                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.
Oh ok. In my mind, L3 is using page alignment not superpage alignment. I 
think was confuse because in your cover letter you define superpage as 
2M or 1G mappings.
> > [..]
> >
> > > +    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.
There is only on call where we don't override the pte.p2m.table bit (the 
one at the end of p2m_create table).
I would move this extra test in the mfn_to_p2m_entry and override only 
for this specific case.
> > [..]
> >
> > > +        }
> > > +        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/?
Undo sorry.

Regards,

--
Julien Grall

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