Re: [Xen-devel] [PATCH v4 3/5] xen/arm: clean and invalidate all guest caches by VMID after domain build.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-02-07 at 12:57 +0000, Jan Beulich wrote:
> >>> On 07.02.14 at 13:12, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> > --- a/xen/arch/arm/domctl.c
> > +++ b/xen/arch/arm/domctl.c
> > @@ -17,6 +17,20 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
> > domain *d,
> >  {
> >      switch ( domctl->cmd )
> >      {
> > +    case XEN_DOMCTL_cacheflush:
> > +    {
> > +        unsigned long s = domctl->u.cacheflush.start_pfn;
> > +        unsigned long e = s + domctl->u.cacheflush.nr_pfns;
> > +
> > +        if ( e < s )
> > +            return -EINVAL;
> > +
> > +        if ( get_order_from_pages(domctl->u.cacheflush.nr_pfns) > 
> > MAX_ORDER )
> > +            return -EINVAL;
> 
> get_order_from_pages() takes an unsigned long, while xen_pfn_t
> is - iirc - 64-bits even on arm32. So you're not checking the full
> passed in value, yet use the full one in the calculation of "e" (which
> is what gets passed down).

Yes, you are right, I should have made nr_mfns be a smaller type.

> Also, did you consider the nr_pfns == 0 case? At present, due to
> the way get_order_from_pages() works, this will produce -EINVAL.
> I'm not sure that's intended.

I think nr == 0 => EINVAL is probably ok.

But actually this made me realise that using get_order_from_pages is a
bit silly here. It seems more logical to compare nr_pfns with
1<<(MAX_ORDER-PAGE_ORDER) now that we have a length in hand.

> 
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -885,6 +885,17 @@ struct xen_domctl_set_max_evtchn {
> >  typedef struct xen_domctl_set_max_evtchn xen_domctl_set_max_evtchn_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_max_evtchn_t);
> >  
> > +/*
> > + * ARM: Clean and invalidate caches associated with given region of
> > + * guest memory.
> > + */
> > +struct xen_domctl_cacheflush {
> > +    /* IN: page range to flush. */
> > +    xen_pfn_t start_pfn, nr_pfns;
> > +};
> 
> The name here (and of the libxc interface) is now certainly
> counterintuitive. But it's a domctl (and an internal interface),
> which we can change post-4.4 (I'd envision it to actually take
> a flags parameter indicating the kind of flush that's wanted).

Sounds OK to me, thanks.

> > --- a/xen/xsm/flask/hooks.c
> > +++ b/xen/xsm/flask/hooks.c
> > @@ -737,6 +737,9 @@ static int flask_domctl(struct domain *d, int cmd)
> >      case XEN_DOMCTL_set_max_evtchn:
> >          return current_has_perm(d, SECCLASS_DOMAIN2, 
> > DOMAIN2__SET_MAX_EVTCHN);
> >  
> > +    case XEN_DOMCTL_cacheflush:
> > +       return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CACHEFLUSH);
> 
> Hard tab.

Well spotted.

This file is missing the emacs magic block. I'll fix this and add one.

Ian.


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