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 10:00 +0100, Julien Grall wrote:
> 
> 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/

I don't know the answer then.


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

All of those other places *conditionally* set table.

> I would move this extra test in the mfn_to_p2m_entry and override only 
> for this specific case.

mfn_to_p2m_entry doesn't know either the level or whether the intention
of the caller is to create a table or a mapping style entry.

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

I'm not sure, you could argue it either way I think. Is Arianna's series
dealing with this in some way?

Anyway, I think changing that behaviour would be a separate patch.

Ian.


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