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

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

On April 25, 2016 6:19 PM,  Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@xxxxxxxxx> wrote:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct domain
> *d,
> >      if ( need_iommu(d) )
> >      {
> >          this_cpu(iommu_dont_flush_iotlb) = 0;
> > -        iommu_iotlb_flush(d, xatp->idx - done, done);
> > -        iommu_iotlb_flush(d, xatp->gpfn - done, done);
> > +        rc = iommu_iotlb_flush(d, xatp->idx - done, done);
> > +        if ( !rc )
> > +            rc = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> 
> And again - best effort flushing in all cases. I.e. you shouldn't bypass the
> second one if the first one failed, but instead properly accumulate the return
> value, and also again not clobber any negative value rc may already hold.


Thanks for correcting me. I did like accumulating the return value.  :(
I will follow your suggestion in next v3.

> What's worse (and makes me wonder whether this got tested) - you also
> clobber the positive return value this function may produce, even in the case
> the flushes succeed (and hence the function as a whole was successful).
> 
I have tested it with previous patches (i.e.  1st patch), launching a VM with 
PT ATS device to invoke this call tree.
btw, I fix the positive issue in 1st patch. 
I know this is not strict, as I assume this patch set will be committed at the 
same time.


> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -182,8 +182,8 @@ 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 iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned
> > +int page_count); int iommu_iotlb_flush_all(struct domain *d);
> 
> __must_check
> 

Agreed.

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