| Message ID | 1394239574-2389-4-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Laurent, On 03/07/2014 06:46 PM, Laurent Pinchart wrote: > Flushing the TLB before updating translation entries creates a race > condition and can lead to stale TLB entries if a translation request > arrives between flushing the TLB and updating the translation entries. > As there's no requirement to flush the TLB before updating the entries, > flush it after only. I do not expect a device to be actively using a region if we are about to change its mapping, we expect the access from the device to be only between a map and an unmap. The race condition (if any in such scenarios) would exist even after this patch, like after you programmed the entry, and the translation request comes before you flush the page. Then it is still operating on an older address, while you have already programmed a new one. An unmap currently clears the entry and flushes any TLB as well, so I don't think this patch makes a big difference. regards Suman > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/iommu/omap-iommu.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c > index cb1e1de..fedd303 100644 > --- a/drivers/iommu/omap-iommu.c > +++ b/drivers/iommu/omap-iommu.c > @@ -662,10 +662,11 @@ int omap_iopgtable_store_entry(struct omap_iommu *obj, struct iotlb_entry *e) > { > int err; > > - flush_iotlb_page(obj, e->da); > err = iopgtable_store_entry_core(obj, e); > - if (!err) > + if (!err) { > + flush_iotlb_page(obj, e->da); > prefetch_iotlb_entry(obj, e); > + } > return err; > } > EXPORT_SYMBOL_GPL(omap_iopgtable_store_entry); > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Suman, On Thursday 13 March 2014 17:27:38 Suman Anna wrote: > Hi Laurent, > > On 03/07/2014 06:46 PM, Laurent Pinchart wrote: > > Flushing the TLB before updating translation entries creates a race > > condition and can lead to stale TLB entries if a translation request > > arrives between flushing the TLB and updating the translation entries. > > As there's no requirement to flush the TLB before updating the entries, > > flush it after only. > > I do not expect a device to be actively using a region if we are about > to change its mapping, we expect the access from the device to be only > between a map and an unmap. The race condition (if any in such > scenarios) would exist even after this patch, like after you programmed > the entry, and the translation request comes before you flush the page. > Then it is still operating on an older address, while you have already > programmed a new one. An unmap currently clears the entry and flushes > any TLB as well, so I don't think this patch makes a big difference. I agree that the patch won't make a difference in practice. Should I drop it from v2 ? > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > > > drivers/iommu/omap-iommu.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c > > index cb1e1de..fedd303 100644 > > --- a/drivers/iommu/omap-iommu.c > > +++ b/drivers/iommu/omap-iommu.c > > @@ -662,10 +662,11 @@ int omap_iopgtable_store_entry(struct omap_iommu > > *obj, struct iotlb_entry *e)> > > { > > int err; > > > > - flush_iotlb_page(obj, e->da); > > > > err = iopgtable_store_entry_core(obj, e); > > - if (!err) > > + if (!err) { > > + flush_iotlb_page(obj, e->da); > > prefetch_iotlb_entry(obj, e); > > + } > > > > return err; > > } > > EXPORT_SYMBOL_GPL(omap_iopgtable_store_entry);
Hi Laurent, >> >> On 03/07/2014 06:46 PM, Laurent Pinchart wrote: >>> Flushing the TLB before updating translation entries creates a race >>> condition and can lead to stale TLB entries if a translation request >>> arrives between flushing the TLB and updating the translation entries. >>> As there's no requirement to flush the TLB before updating the entries, >>> flush it after only. >> >> I do not expect a device to be actively using a region if we are about >> to change its mapping, we expect the access from the device to be only >> between a map and an unmap. The race condition (if any in such >> scenarios) would exist even after this patch, like after you programmed >> the entry, and the translation request comes before you flush the page. >> Then it is still operating on an older address, while you have already >> programmed a new one. An unmap currently clears the entry and flushes >> any TLB as well, so I don't think this patch makes a big difference. > > I agree that the patch won't make a difference in practice. Should I drop it > from v2 ? Yes, that should be safe. regards Suman > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> >>> drivers/iommu/omap-iommu.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c >>> index cb1e1de..fedd303 100644 >>> --- a/drivers/iommu/omap-iommu.c >>> +++ b/drivers/iommu/omap-iommu.c >>> @@ -662,10 +662,11 @@ int omap_iopgtable_store_entry(struct omap_iommu >>> *obj, struct iotlb_entry *e)> >>> { >>> int err; >>> >>> - flush_iotlb_page(obj, e->da); >>> >>> err = iopgtable_store_entry_core(obj, e); >>> - if (!err) >>> + if (!err) { >>> + flush_iotlb_page(obj, e->da); >>> prefetch_iotlb_entry(obj, e); >>> + } >>> >>> return err; >>> } >>> EXPORT_SYMBOL_GPL(omap_iopgtable_store_entry); > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index cb1e1de..fedd303 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -662,10 +662,11 @@ int omap_iopgtable_store_entry(struct omap_iommu *obj, struct iotlb_entry *e) { int err; - flush_iotlb_page(obj, e->da); err = iopgtable_store_entry_core(obj, e); - if (!err) + if (!err) { + flush_iotlb_page(obj, e->da); prefetch_iotlb_entry(obj, e); + } return err; } EXPORT_SYMBOL_GPL(omap_iopgtable_store_entry);
Flushing the TLB before updating translation entries creates a race condition and can lead to stale TLB entries if a translation request arrives between flushing the TLB and updating the translation entries. As there's no requirement to flush the TLB before updating the entries, flush it after only. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/iommu/omap-iommu.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)