Re: [Xen-devel] [RFC 14/22] xen/arm: p2m: Re-implement p2m_cache_flush using p2m_get_entry

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 05/09/16 22:13, Stefano Stabellini wrote:
> On Thu, 28 Jul 2016, Julien Grall wrote:
> > The function p2m_cache_flush can be re-implemented using the generic
> > function p2m_get_entry by iterating over the range and using the mapping
> > order given by the callee.
> >
> > As the current implementation, no preemption is implemented, although
> > the comment in the current code claimed it. As the function is called by
> > a DOMCTL with a region of 1GB maximum, I think the preemption can be
> > left unimplemented for now.
> >
> > Finally drop the operation CACHEFLUSH in apply_one_level as nobody is
> > using it anymore. Note that the function could have been dropped in one
> > go at the end, however I find easier to drop the operations one by one
> > avoiding a big deletion in the patch that convert the last operation.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> >
> > ---
> >     The loop pattern will be very for the reliquish function. It might
> >     be possible to extract it in a separate function.
> > ---
> >  xen/arch/arm/p2m.c | 67 +++++++++++++++++++++++++++---------------------------
> >  1 file changed, 34 insertions(+), 33 deletions(-)
> >
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 9a9c85c..e7697bb 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -722,7 +722,6 @@ enum p2m_operation {
> >      INSERT,
> >      REMOVE,
> >      RELINQUISH,
> > -    CACHEFLUSH,
> >      MEMACCESS,
> >  };
> >
> > @@ -978,36 +977,6 @@ static int apply_one_level(struct domain *d,
> >           */
> >          return P2M_ONE_PROGRESS;
> >
> > -    case CACHEFLUSH:
> > -        if ( !p2m_valid(orig_pte) )
> > -        {
> > -            *addr = (*addr + level_size) & level_mask;
> > -            return P2M_ONE_PROGRESS_NOP;
> > -        }
> > -
> > -        if ( level < 3 && p2m_table(orig_pte) )
> > -            return P2M_ONE_DESCEND;
> > -
> > -        /*
> > -         * could flush up to the next superpage boundary, but would
> > -         * need to be careful about preemption, so just do one 4K page
> > -         * now and return P2M_ONE_PROGRESS{,_NOP} so that the caller will
> > -         * continue to loop over the rest of the range.
> > -         */
> > -        if ( p2m_is_ram(orig_pte.p2m.type) )
> > -        {
> > -            unsigned long offset = paddr_to_pfn(*addr & ~level_mask);
> > -            flush_page_to_ram(orig_pte.p2m.base + offset);
> > -
> > -            *addr += PAGE_SIZE;
> > -            return P2M_ONE_PROGRESS;
> > -        }
> > -        else
> > -        {
> > -            *addr += PAGE_SIZE;
> > -            return P2M_ONE_PROGRESS_NOP;
> > -        }
> > -
> >      case MEMACCESS:
> >          if ( level < 3 )
> >          {
> > @@ -1555,12 +1524,44 @@ int p2m_cache_flush(struct domain *d, gfn_t start, 
> > unsigned long nr)
> >  {
> >      struct p2m_domain *p2m = &d->arch.p2m;
> >      gfn_t end = gfn_add(start, nr);
> > +    p2m_type_t t;
> > +    unsigned int order;
> >
> >      start = gfn_max(start, p2m->lowest_mapped_gfn);
> >      end = gfn_min(end, p2m->max_mapped_gfn);
> >
> > -    return apply_p2m_changes(d, CACHEFLUSH, start, nr, INVALID_MFN,
> > -                             0, p2m_invalid, d->arch.p2m.default_access);
> > +    /* XXX: Should we use write lock here? */
> Good question. As the p2m is left unchanged by this function, I think
> that the read lock is sufficient.
It is what I thought. I will replace the todo by a comment explain why 
read-lock is used here.
> > +    p2m_read_lock(p2m);
> > +
> > +    for ( ; gfn_x(start) < gfn_x(end); start = gfn_add(start, 1UL << order) )
> > +    {
> > +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
> > +
> > +        /* Skip hole and non-RAM page */
> > +        if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_ram(t) )
> > +        {
> > +            /*
> > +             * the order corresponds to the order of the mapping in the
> > +             * page table. so we need to align the gfn before
> > +             * incrementing.
> > +             */
> > +            start = _gfn(gfn_x(start) & ~((1UL << order) - 1));
> > +            continue;
> > +        }
> > +
> > +        /*
> > +         * Could flush up to the next superpage boundary, but we would
> > +         * need to be careful about preemption, so just do one 4K page
> > +         * now.
> I think that even without preemption you should implement flushing up to
> the next superpage boundary (but not beyond "end"). You can still do it
> 4K at a time, but only call p2m_get_entry once per "order". Could be a
> decent performance improvement as cacheflush is a performance critical
> hypercall.
Good point. I will give a look for the next version.

Regards,

--
Julien Grall

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