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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-06-13 at 18:45 +0100, Julien Grall wrote:

> >
> >>>> [..]
> >>>>
> >>>>> +    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.
> 
> AFAIU, you add/remove every callers of this function in this patch. 
> Extending this function (or adding a new helper) would avoid to copy few 
> time the same check.

I prefer it the way it is now.


> 
> >>>>> -        /* 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?
> 
> I think she handled it only for MMIO. I will have to look again into her 
> series.
> 
> > Anyway, I think changing that behaviour would be a separate patch.
> 
> I didn't intend to ask you to undo the mapping in this patch, nor in 
> this series.

Please make it unambiguously clear when you are asking a more general
question rather than suggesting a change to a patch. e.g. by using
phrases like "in a future patch".

> I was just wondering what could happen if we fail to map the whole 
> mapping. Hence, some callers of guest_physmap_add_page is not checking 
> the return of this function. It seems that we may end up to partially 
> map the range in guest and crash it.
> 
> Anyway,  I don't think it's too bad as the guest will likely fail later 
> if we fail to map the region. The only issue is that this won't be 
> trivial to find the source as we don't have any error message.

Right.

Ian.


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