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

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

On May 10, 2016 5:04 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> 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 
> &&.
> 

Make sense. Thanks for your careful check.


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

You are right. I will drop these __must_check annotation here.
 
> > --- 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.
> 

Agreed.

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

Quan

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