> -----Original Message----- > From: Jan Beulich <jbeul...@suse.com> > Sent: 05 August 2020 17:06 > To: Paul Durrant <p...@xen.org> > Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurr...@amazon.com> > 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 <pdurr...@amazon.com> > > > > 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