| Message ID | 20200804134209.8717-7-paul@xen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOMMU cleanup | expand |
On 04.08.2020 15:42, Paul Durrant wrote: > From: Paul Durrant <pdurrant@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. > --- 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 ! )? Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 05 August 2020 17:06 > To: Paul Durrant <paul@xen.org> > Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@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 <pdurrant@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
On 04.08.2020 15:42, Paul Durrant wrote: > --- 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) ) > + flush_flags = 0; Noticed only while looking at patch 9: There's also an indirection missing both here and ... > @@ -330,6 +328,10 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order, > } > } > > + /* Something went wrong so flush everything and clear flush flags */ > + if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) ) > + flush_flags = 0; ... here. Jan
> From: Paul Durrant > Sent: Tuesday, August 4, 2020 9:42 PM > > From: Paul Durrant <pdurrant@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 this statement is different from change in iommu_map... > a stack variable and generally makes the call sites tidier. > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > --- > Cc: Jan Beulich <jbeulich@suse.com> > > v2: > - New in v2 > --- > xen/drivers/passthrough/iommu.c | 28 ++++++++++++---------------- > 1 file changed, 12 insertions(+), 16 deletions(-) > > diff --git a/xen/drivers/passthrough/iommu.c > b/xen/drivers/passthrough/iommu.c > index 660dc5deb2..e2c0193a09 100644 > --- 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) ) > + flush_flags = 0; > + ... earlier you said flush is unnecessary if map fails. But here actually you still need to flush everything so it's just sort of moving error-path flush within the map function? Thanks Kevin > return rc; > } > > @@ -283,14 +287,8 @@ int iommu_legacy_map(struct domain *d, dfn_t dfn, > mfn_t mfn, > unsigned int flush_flags = 0; > int rc = iommu_map(d, dfn, mfn, page_order, flags, &flush_flags); > > - if ( !this_cpu(iommu_dont_flush_iotlb) ) > - { > - int err = iommu_iotlb_flush(d, dfn, (1u << page_order), > - flush_flags); > - > - if ( !rc ) > - rc = err; > - } > + if ( !this_cpu(iommu_dont_flush_iotlb) && !rc ) > + rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags); > > return rc; > } > @@ -330,6 +328,10 @@ int iommu_unmap(struct domain *d, dfn_t dfn, > unsigned int page_order, > } > } > > + /* Something went wrong so flush everything and clear flush flags */ > + if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) ) > + flush_flags = 0; > + > return rc; > } > > @@ -338,14 +340,8 @@ int iommu_legacy_unmap(struct domain *d, dfn_t > dfn, unsigned int page_order) > unsigned int flush_flags = 0; > int rc = iommu_unmap(d, dfn, page_order, &flush_flags); > > - if ( !this_cpu(iommu_dont_flush_iotlb) ) > - { > - int err = iommu_iotlb_flush(d, dfn, (1u << page_order), > - flush_flags); > - > - if ( !rc ) > - rc = err; > - } > + if ( !this_cpu(iommu_dont_flush_iotlb) && ! rc ) > + rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags); > > return rc; > } > -- > 2.20.1 >
> -----Original Message----- > From: Tian, Kevin <kevin.tian@intel.com> > Sent: 14 August 2020 07:53 > To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org > Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Jan Beulich <jbeulich@suse.com> > Subject: RE: [EXTERNAL] [PATCH v4 06/14] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail > > CAUTION: This email originated from outside of the organization. Do not click links or open > attachments unless you can confirm the sender and know the content is safe. > > > > > From: Paul Durrant > > Sent: Tuesday, August 4, 2020 9:42 PM > > > > From: Paul Durrant <pdurrant@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 > > this statement is different from change in iommu_map... > > > a stack variable and generally makes the call sites tidier. > > > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > > --- > > Cc: Jan Beulich <jbeulich@suse.com> > > > > v2: > > - New in v2 > > --- > > xen/drivers/passthrough/iommu.c | 28 ++++++++++++---------------- > > 1 file changed, 12 insertions(+), 16 deletions(-) > > > > diff --git a/xen/drivers/passthrough/iommu.c > > b/xen/drivers/passthrough/iommu.c > > index 660dc5deb2..e2c0193a09 100644 > > --- 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) ) > > + flush_flags = 0; > > + > > ... earlier you said flush is unnecessary if map fails. But here actually you > still need to flush everything so it's just sort of moving error-path flush > within the map function? Yes, that's actually what's happening. The language in the comment is ambiguous I guess. I'll modify it to say "because, if the map or unmap fails an explicit flush will be unnecessary." Hopefully that is clearer. Paul > > Thanks > Kevin > > > return rc; > > } > > > > @@ -283,14 +287,8 @@ int iommu_legacy_map(struct domain *d, dfn_t dfn, > > mfn_t mfn, > > unsigned int flush_flags = 0; > > int rc = iommu_map(d, dfn, mfn, page_order, flags, &flush_flags); > > > > - if ( !this_cpu(iommu_dont_flush_iotlb) ) > > - { > > - int err = iommu_iotlb_flush(d, dfn, (1u << page_order), > > - flush_flags); > > - > > - if ( !rc ) > > - rc = err; > > - } > > + if ( !this_cpu(iommu_dont_flush_iotlb) && !rc ) > > + rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags); > > > > return rc; > > } > > @@ -330,6 +328,10 @@ int iommu_unmap(struct domain *d, dfn_t dfn, > > unsigned int page_order, > > } > > } > > > > + /* Something went wrong so flush everything and clear flush flags */ > > + if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) ) > > + flush_flags = 0; > > + > > return rc; > > } > > > > @@ -338,14 +340,8 @@ int iommu_legacy_unmap(struct domain *d, dfn_t > > dfn, unsigned int page_order) > > unsigned int flush_flags = 0; > > int rc = iommu_unmap(d, dfn, page_order, &flush_flags); > > > > - if ( !this_cpu(iommu_dont_flush_iotlb) ) > > - { > > - int err = iommu_iotlb_flush(d, dfn, (1u << page_order), > > - flush_flags); > > - > > - if ( !rc ) > > - rc = err; > > - } > > + if ( !this_cpu(iommu_dont_flush_iotlb) && ! rc ) > > + rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags); > > > > return rc; > > } > > -- > > 2.20.1 > >
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 660dc5deb2..e2c0193a09 100644 --- 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) ) + flush_flags = 0; + return rc; } @@ -283,14 +287,8 @@ int iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn, unsigned int flush_flags = 0; int rc = iommu_map(d, dfn, mfn, page_order, flags, &flush_flags); - if ( !this_cpu(iommu_dont_flush_iotlb) ) - { - int err = iommu_iotlb_flush(d, dfn, (1u << page_order), - flush_flags); - - if ( !rc ) - rc = err; - } + if ( !this_cpu(iommu_dont_flush_iotlb) && !rc ) + rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags); return rc; } @@ -330,6 +328,10 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order, } } + /* Something went wrong so flush everything and clear flush flags */ + if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) ) + flush_flags = 0; + return rc; } @@ -338,14 +340,8 @@ int iommu_legacy_unmap(struct domain *d, dfn_t dfn, unsigned int page_order) unsigned int flush_flags = 0; int rc = iommu_unmap(d, dfn, page_order, &flush_flags); - if ( !this_cpu(iommu_dont_flush_iotlb) ) - { - int err = iommu_iotlb_flush(d, dfn, (1u << page_order), - flush_flags); - - if ( !rc ) - rc = err; - } + if ( !this_cpu(iommu_dont_flush_iotlb) && ! rc ) + rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags); return rc; }