RE: [PATCH v4 06/14] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 05 August 2020 17:06
> To: Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Paul Durrant <pdurrant@xxxxxxxxxx>
> Subject: Re: [PATCH v4 06/14] iommu: flush I/O TLB if iommu_map() or 
> iommu_unmap() fail
> 
> On 04.08.2020 15:42, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@xxxxxxxxxx>
> >
> > This patch adds a full I/O TLB flush to the error paths of iommu_map() and
> > iommu_unmap().
> >
> > Without this change callers need constructs such as:
> >
> > rc = iommu_map/unmap(...)
> > err = iommu_flush(...)
> > if ( !rc )
> >   rc = err;
> >
> > With this change, it can be simplified to:
> >
> > rc = iommu_map/unmap(...)
> > if ( !rc )
> >   rc = iommu_flush(...)
> >
> > because, if the map or unmap fails the flush will be unnecessary. This saves
> > a stack variable and generally makes the call sites tidier.
> 
> I appreciate the intent of tidier code, but I wonder whether this
> flushing doesn't go a little too far: There's a need to flush in
> general when multiple pages were to be (un)mapped, and there was
> at least partial success. Hence e.g. in the order == 0 case I
> don't see why any flushing would be needed. Granted errors aren't
> commonly expected, but anyway.
> 

Yes, I wasn't really worried about optimizing the error case, but I can avoid 
unnecessary flushing in the order 0 case.

> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -274,6 +274,10 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
> >          break;
> >      }
> >
> > +    /* Something went wrong so flush everything and clear flush flags */
> > +    if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) )
> 
> Both here and in the unmap path, did you get the return value
> of iommu_iotlb_flush_all() the wrong way round (i.e. isn't there
> a missing ! )?
> 

Yes, I think you're right. I'll need to re-work anyway to avoid the flush in 
the order 0 case.

  Paul

> Jan