Re: [Xen-devel] [PATCH v5 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On May 23, 2016 10:19 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 18.05.16 at 10:08, <quan.xu@xxxxxxxxx> wrote:
> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2463,11 +2463,12 @@ static int __put_page_type(struct page_info
> > *page,  }
> >
> >
> > -static int __get_page_type(struct page_info *page, unsigned long type,
> > -                           int preemptible)
> > +static int __must_check __get_page_type(struct page_info *page,
> 
> Such a __must_check is relatively pointless when all existing callers already
> check the return value (and surely all code inside mm.c knows it needs to), 
> and
> you don't at the same time make the non-static functions propagating its
> return value also __must_check.

I will drop this __must_check annotation.

> Hence I think best is to limit your effort to IOMMU functions for this patch 
> set.
>

Good idea.
 
> > +                                        unsigned long type,
> > +                                        int preemptible)
> >  {
> >      unsigned long nx, x, y = page->u.inuse.type_info;
> > -    int rc = 0;
> > +    int rc = 0, ret = 0;
> >
> >      ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
> >
> > @@ -2578,11 +2579,11 @@ static int __get_page_type(struct page_info
> *page, unsigned long type,
> >          if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> >          {
> >              if ( (x & PGT_type_mask) == PGT_writable_page )
> > -                iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> > +                ret = iommu_unmap_page(d, mfn_to_gmfn(d,
> > + page_to_mfn(page)));
> >              else if ( type == PGT_writable_page )
> > -                iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> > -                               page_to_mfn(page),
> > -                               IOMMUF_readable|IOMMUF_writable);
> > +                ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> > +                                     page_to_mfn(page),
> > +
> > + IOMMUF_readable|IOMMUF_writable);
> >          }
> >      }
> >
> > @@ -2599,6 +2600,9 @@ static int __get_page_type(struct page_info
> *page, unsigned long type,
> >      if ( (x & PGT_partial) && !(nx & PGT_partial) )
> >          put_page(page);
> >
> > +    if ( !rc )
> > +        rc = ret;
> 
> I know I've seen this a couple of time already, but with the special purpose 
> of
> "ret" I really wonder whether a more specific name wouldn't be warranted -
> e.g. "iommu_rc" or "iommu_ret".


rc is return value for this function, and no directly association with IOMMU 
related code ( rc is only for alloc_page_type() ).
So the rc cannot be "iommu_rc"..

ret can be "iommu_ret", but I think the pair 'rc' / 'ret' may look good.


> 
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -658,7 +658,7 @@ bool_t ept_handle_misconfig(uint64_t gpa)
> >   *
> >   * Returns: 0 for success, -errno for failure
> >   */
> > -static int
> > +static int __must_check
> >  ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
> 
> Now adding the annotation here, without also (first) adding it to the p2m
> method which this gets used for (and which is this function's sole purpose), 
> is
> not useful at all. Please remember - we don't want this annotation because it
> looks good, but in order to make sure errors don't get wrongly ignored. That's
> why, from the beginning, I have been telling you that adding such annotations
> to other than new code means doing it top down (which you clearly don't do
> here).
> 

I will drop this __must_check annotation.

> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -171,20 +171,33 @@ void __hwdom_init iommu_hwdom_init(struct
> domain *d)
> >      {
> >          struct page_info *page;
> >          unsigned int i = 0;
> > +        int rc = 0;
> > +
> >          page_list_for_each ( page, &d->page_list )
> >          {
> >              unsigned long mfn = page_to_mfn(page);
> >              unsigned long gfn = mfn_to_gmfn(d, mfn);
> >              unsigned int mapping = IOMMUF_readable;
> > +            int ret;
> >
> >              if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
> >                   ((page->u.inuse.type_info & PGT_type_mask)
> >                    == PGT_writable_page) )
> >                  mapping |= IOMMUF_writable;
> > -            hd->platform_ops->map_page(d, gfn, mfn, mapping);
> > +
> > +            ret = hd->platform_ops->map_page(d, gfn, mfn, mapping);
> > +
> > +            if ( unlikely(ret) )
> > +                rc = ret;
> 
> This should be good enough, but I think it would be better if, just like
> elsewhere, you returned the first error instead of the last one.
> 

I will also apply it to this series.


(Jan, I know It is not an easy work to review these little things. I very 
appreciate it. Thank you!! )
Quan

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