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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.
Hence I think best is to limit your effort to IOMMU functions for this
patch set.

> +                                        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".

> --- 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).

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

Jan

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