Re: [Xen-devel] [PATCH v4 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -677,9 +677,19 @@ static int xenmem_add_to_physmap(struct domain *d,
>  #ifdef CONFIG_HAS_PASSTHROUGH
>      if ( need_iommu(d) )
>      {
> +        int ret;
> +
>          this_cpu(iommu_dont_flush_iotlb) = 0;
> -        iommu_iotlb_flush(d, xatp->idx - done, done);
> -        iommu_iotlb_flush(d, xatp->gpfn - done, done);
> +
> +        ret = iommu_iotlb_flush(d, xatp->idx - done, done);
> +
> +        if ( rc >= 0 && unlikely(ret) )
> +            rc = ret;
> +
> +        ret = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> +
> +        if ( rc >= 0 && unlikely(ret) )
> +            rc = ret;

In both cases I think it would be preferable to switch the two sides
of the &&.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -312,24 +312,29 @@ static void iommu_free_pagetables(unsigned long unused)
>                              cpumask_cycle(smp_processor_id(), 
> &cpu_online_map));
>  }
>  
> -void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int 
> page_count)
> +int __must_check iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> +                                   unsigned int page_count)

The annotation generally belongs on declarations; the only exception
are static functions which don't have any (i.e. they get defined before
first getting used).

> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -200,8 +200,9 @@ int iommu_do_pci_domctl(struct xen_domctl *, struct 
> domain *d,
>  int iommu_do_domctl(struct xen_domctl *, struct domain *d,
>                      XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>  
> -void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int 
> page_count);
> -void iommu_iotlb_flush_all(struct domain *d);
> +int __must_check iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> +                                   unsigned int page_count);
> +int __must_check iommu_iotlb_flush_all(struct domain *d);

I.e. these changes suffice, repeating the annotations on the
function definitions is redundant with the ones put here.

With respective adjustments done
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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