Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1234725 > unrolled thread
| Started by | Tomasz Figa <tfiga@chromium.org> |
|---|---|
| First post | 2015-09-29 07:30 +0200 |
| Last post | 2015-09-29 19:20 +0200 |
| Articles | 10 — 5 participants |
Back to article view | Back to linux.kernel
[RFC PATCH 0/3] iommu: Add range flush operation Tomasz Figa <tfiga@chromium.org> - 2015-09-29 07:30 +0200
Re: [RFC PATCH 0/3] iommu: Add range flush operation Thierry Reding <thierry.reding@gmail.com> - 2015-09-29 11:30 +0200
Re: [RFC PATCH 0/3] iommu: Add range flush operation Tomasz Figa <tfiga@chromium.org> - 2015-09-29 14:00 +0200
Re: [RFC PATCH 0/3] iommu: Add range flush operation Joerg Roedel <joro@8bytes.org> - 2015-09-29 14:30 +0200
Re: [RFC PATCH 0/3] iommu: Add range flush operation Joerg Roedel <joro@8bytes.org> - 2015-09-29 14:30 +0200
Re: [RFC PATCH 0/3] iommu: Add range flush operation Robin Murphy <robin.murphy@arm.com> - 2015-09-29 16:30 +0200
Re: [RFC PATCH 0/3] iommu: Add range flush operation Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-09-29 16:40 +0200
Re: [RFC PATCH 0/3] iommu: Add range flush operation Robin Murphy <robin.murphy@arm.com> - 2015-09-29 18:30 +0200
Re: [RFC PATCH 0/3] iommu: Add range flush operation Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-09-29 18:50 +0200
Re: [RFC PATCH 0/3] iommu: Add range flush operation Robin Murphy <robin.murphy@arm.com> - 2015-09-29 19:20 +0200
| From | Tomasz Figa <tfiga@chromium.org> |
|---|---|
| Date | 2015-09-29 07:30 +0200 |
| Subject | [RFC PATCH 0/3] iommu: Add range flush operation |
| Message-ID | <qdVEe-5sa-15@gated-at.bofh.it> |
Currently the IOMMU subsystem provides 3 basic operations: iommu_map(),
iommu_map_sg() and iommu_unmap(). iommu_map() can be used to map memory
page by page, however it involves flushing the caches (CPU and IOMMU) for
every mapped page separately, which is unsuitable for use cases that
require low mapping latency. Similarly iommu_unmap(), even though it
takes a full IOVA range as its argument, performs unmapping in a page
by page manner.
To make mapping operation more suitable for such use cases, iommu_map_sg()
and .map_sg() callback in iommu_ops struct were introduced, which allowed
particular IOMMU drivers to directly iterate over SG entries, create
necessary mappings and flush everything in one go.
This approach, however, has two drawbacks:
1) it does not do anything about unmap performance,
2) it requires each driver willing to have fast map to implement its
own SG iteration code, even though this is a mostly generic operation.
This series tries to mitigate the two issues above, while acknowledging
the fact that the .map_sg() callback might be still necessary for some
specific platforms, which could have the need to iterate over SG elements
inside driver code. Proposed solution introduces a new .flush() callback,
which expects IOVA range as its argument and is expected to flush all
respective caches (be it CPU, IOMMU TLB or whatever) to make the given
IOVA area mapping change visible to IOMMU clients. Then all the 3 basic
map/unmap operations are modified to call the .flush() callback at the end
of the operation.
Advantages of proposed approach include:
1) ability to use default_iommu_map_sg() helper if all the driver needs
for performance optimization is batching the flush,
2) completely no effect on existing code - the .flush() callback is made
optional and if it isn't implemented drivers are expected to do
necessary flushes on a page by page basis in respective (un)mapping
callbakcs,
3) possibility of exporting the iommu_flush() operation and providing
unsynchronized map/unmap operations for subsystems with even higher
requirements for performance (e.g. drivers/gpu/drm).
The series includes a generic patch implementing necessary changes in
IOMMU API and two Tegra-specific patches that demonstrate implementation
on driver side and which can be used for further testing.
Last, but not least, some performance numbers on Tegra210:
+-----------+--------------+-------------+------------+
| Operation | Size [bytes] | Before [us] | After [us] |
+-----------+--------------+-------------+------------+
| Map | 128K | 139 | 40 |
| | | 136 | 34 |
| | | 137 | 38 |
| | | 136 | 36 |
| | 4M | 3939 | 1163 |
| | | 3730 | 2389 |
| | | 3613 | 997 |
| | | 3622 | 1620 |
| | ~18M | 18635 | 4741 |
| | | 19261 | 6550 |
| | | 18473 | 9304 |
| | | 18125 | 5120 |
| Unmap | 128K | 128 | 7 |
| | | 122 | 8 |
| | | 119 | 10 |
| | | 123 | 12 |
| | 4M | 3829 | 151 |
| | | 3964 | 150 |
| | | 3908 | 145 |
| | | 3875 | 155 |
| | ~18M | 18570 | 683 |
| | | 18473 | 806 |
| | | 21020 | 643 |
| | | 21764 | 652 |
+-----------+--------------+-------------+------------+
The values are obtained by surrounding the calls to iommu_map_sg()
(with default_iommu_map_sg() helper used as .map_sg() callback) and
iommu_unmap() with ktime-based time measurement code. Taken 4 samples
of every buffer size. ~18M means around 17-19M due do the variance
in requested buffer sizes.
Tomasz Figa (2):
iommu: Add support for out of band flushing
iommu/tegra-smmu: Make the driver use out of band flushing
Vince Hsu (1):
memory: tegra: add TLB cache line size
drivers/iommu/iommu.c | 33 +++++++++++++--
drivers/iommu/tegra-smmu.c | 91 +++++++++++++++++++++++++++++++++++++----
drivers/memory/tegra/tegra114.c | 1 +
drivers/memory/tegra/tegra124.c | 3 ++
drivers/memory/tegra/tegra210.c | 1 +
drivers/memory/tegra/tegra30.c | 1 +
include/linux/iommu.h | 2 +
include/soc/tegra/mc.h | 1 +
8 files changed, 122 insertions(+), 11 deletions(-)
--
2.6.0.rc2.230.g3dd15c0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Thierry Reding <thierry.reding@gmail.com> |
|---|---|
| Date | 2015-09-29 11:30 +0200 |
| Message-ID | <qdZou-2mn-17@gated-at.bofh.it> |
| In reply to | #1234725 |
[Multipart message — attachments visible in raw view] — view raw
On Tue, Sep 29, 2015 at 02:25:23PM +0900, Tomasz Figa wrote: > Currently the IOMMU subsystem provides 3 basic operations: iommu_map(), > iommu_map_sg() and iommu_unmap(). iommu_map() can be used to map memory > page by page, however it involves flushing the caches (CPU and IOMMU) for > every mapped page separately, which is unsuitable for use cases that > require low mapping latency. Similarly iommu_unmap(), even though it > takes a full IOVA range as its argument, performs unmapping in a page > by page manner. > > To make mapping operation more suitable for such use cases, iommu_map_sg() > and .map_sg() callback in iommu_ops struct were introduced, which allowed > particular IOMMU drivers to directly iterate over SG entries, create > necessary mappings and flush everything in one go. > > This approach, however, has two drawbacks: > 1) it does not do anything about unmap performance, > 2) it requires each driver willing to have fast map to implement its > own SG iteration code, even though this is a mostly generic operation. > > This series tries to mitigate the two issues above, while acknowledging > the fact that the .map_sg() callback might be still necessary for some > specific platforms, which could have the need to iterate over SG elements > inside driver code. Proposed solution introduces a new .flush() callback, > which expects IOVA range as its argument and is expected to flush all > respective caches (be it CPU, IOMMU TLB or whatever) to make the given > IOVA area mapping change visible to IOMMU clients. Then all the 3 basic > map/unmap operations are modified to call the .flush() callback at the end > of the operation. > > Advantages of proposed approach include: > 1) ability to use default_iommu_map_sg() helper if all the driver needs > for performance optimization is batching the flush, > 2) completely no effect on existing code - the .flush() callback is made > optional and if it isn't implemented drivers are expected to do > necessary flushes on a page by page basis in respective (un)mapping > callbakcs, > 3) possibility of exporting the iommu_flush() operation and providing > unsynchronized map/unmap operations for subsystems with even higher > requirements for performance (e.g. drivers/gpu/drm). That would require passing in some sort of flag that the core shouldn't be flushing itself, right? Currently it would flush on every map/unmap. > > The series includes a generic patch implementing necessary changes in > IOMMU API and two Tegra-specific patches that demonstrate implementation > on driver side and which can be used for further testing. > > Last, but not least, some performance numbers on Tegra210: > +-----------+--------------+-------------+------------+ > | Operation | Size [bytes] | Before [us] | After [us] | > +-----------+--------------+-------------+------------+ > | Map | 128K | 139 | 40 | > | | | 136 | 34 | > | | | 137 | 38 | > | | | 136 | 36 | > | | 4M | 3939 | 1163 | > | | | 3730 | 2389 | > | | | 3613 | 997 | > | | | 3622 | 1620 | > | | ~18M | 18635 | 4741 | > | | | 19261 | 6550 | > | | | 18473 | 9304 | > | | | 18125 | 5120 | > | Unmap | 128K | 128 | 7 | > | | | 122 | 8 | > | | | 119 | 10 | > | | | 123 | 12 | > | | 4M | 3829 | 151 | > | | | 3964 | 150 | > | | | 3908 | 145 | > | | | 3875 | 155 | > | | ~18M | 18570 | 683 | > | | | 18473 | 806 | > | | | 21020 | 643 | > | | | 21764 | 652 | > +-----------+--------------+-------------+------------+ > The values are obtained by surrounding the calls to iommu_map_sg() > (with default_iommu_map_sg() helper used as .map_sg() callback) and > iommu_unmap() with ktime-based time measurement code. Taken 4 samples > of every buffer size. ~18M means around 17-19M due do the variance > in requested buffer sizes. Those are pretty impressive numbers. Thierry
[toc] | [prev] | [next] | [standalone]
| From | Tomasz Figa <tfiga@chromium.org> |
|---|---|
| Date | 2015-09-29 14:00 +0200 |
| Message-ID | <qe1JE-5BG-25@gated-at.bofh.it> |
| In reply to | #1234910 |
On Tue, Sep 29, 2015 at 6:27 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Tue, Sep 29, 2015 at 02:25:23PM +0900, Tomasz Figa wrote: > > Currently the IOMMU subsystem provides 3 basic operations: iommu_map(), > > iommu_map_sg() and iommu_unmap(). iommu_map() can be used to map memory > > page by page, however it involves flushing the caches (CPU and IOMMU) for > > every mapped page separately, which is unsuitable for use cases that > > require low mapping latency. Similarly iommu_unmap(), even though it > > takes a full IOVA range as its argument, performs unmapping in a page > > by page manner. > > > > To make mapping operation more suitable for such use cases, iommu_map_sg() > > and .map_sg() callback in iommu_ops struct were introduced, which allowed > > particular IOMMU drivers to directly iterate over SG entries, create > > necessary mappings and flush everything in one go. > > > > This approach, however, has two drawbacks: > > 1) it does not do anything about unmap performance, > > 2) it requires each driver willing to have fast map to implement its > > own SG iteration code, even though this is a mostly generic operation. > > > > This series tries to mitigate the two issues above, while acknowledging > > the fact that the .map_sg() callback might be still necessary for some > > specific platforms, which could have the need to iterate over SG elements > > inside driver code. Proposed solution introduces a new .flush() callback, > > which expects IOVA range as its argument and is expected to flush all > > respective caches (be it CPU, IOMMU TLB or whatever) to make the given > > IOVA area mapping change visible to IOMMU clients. Then all the 3 basic > > map/unmap operations are modified to call the .flush() callback at the end > > of the operation. > > > > Advantages of proposed approach include: > > 1) ability to use default_iommu_map_sg() helper if all the driver needs > > for performance optimization is batching the flush, > > 2) completely no effect on existing code - the .flush() callback is made > > optional and if it isn't implemented drivers are expected to do > > necessary flushes on a page by page basis in respective (un)mapping > > callbakcs, > > 3) possibility of exporting the iommu_flush() operation and providing > > unsynchronized map/unmap operations for subsystems with even higher > > requirements for performance (e.g. drivers/gpu/drm). > > That would require passing in some sort of flag that the core shouldn't > be flushing itself, right? Currently it would flush on every map/unmap. > Are you asking about 3) in particular? If so, I was thinking about iommu_map_noflush(), iommu_unmap_noflush(), which could be then wrapped by iommu_map() with call to iommu_flush() added at the end. > > > > > The series includes a generic patch implementing necessary changes in > > IOMMU API and two Tegra-specific patches that demonstrate implementation > > on driver side and which can be used for further testing. > > > > Last, but not least, some performance numbers on Tegra210: > > +-----------+--------------+-------------+------------+ > > | Operation | Size [bytes] | Before [us] | After [us] | > > +-----------+--------------+-------------+------------+ > > | Map | 128K | 139 | 40 | > > | | | 136 | 34 | > > | | | 137 | 38 | > > | | | 136 | 36 | > > | | 4M | 3939 | 1163 | > > | | | 3730 | 2389 | > > | | | 3613 | 997 | > > | | | 3622 | 1620 | > > | | ~18M | 18635 | 4741 | > > | | | 19261 | 6550 | > > | | | 18473 | 9304 | > > | | | 18125 | 5120 | > > | Unmap | 128K | 128 | 7 | > > | | | 122 | 8 | > > | | | 119 | 10 | > > | | | 123 | 12 | > > | | 4M | 3829 | 151 | > > | | | 3964 | 150 | > > | | | 3908 | 145 | > > | | | 3875 | 155 | > > | | ~18M | 18570 | 683 | > > | | | 18473 | 806 | > > | | | 21020 | 643 | > > | | | 21764 | 652 | > > +-----------+--------------+-------------+------------+ > > The values are obtained by surrounding the calls to iommu_map_sg() > > (with default_iommu_map_sg() helper used as .map_sg() callback) and > > iommu_unmap() with ktime-based time measurement code. Taken 4 samples > > of every buffer size. ~18M means around 17-19M due do the variance > > in requested buffer sizes. > > Those are pretty impressive numbers. I was surprised myself that there is so much difference on this platform, but it seems to converge with downstream kernel. Moreover we can supposedly get even better results by simply invalidating full TLB above some length threshold. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Joerg Roedel <joro@8bytes.org> |
|---|---|
| Date | 2015-09-29 14:30 +0200 |
| Message-ID | <qe2cI-6p2-23@gated-at.bofh.it> |
| In reply to | #1234910 |
On Tue, Sep 29, 2015 at 11:27:14AM +0200, Thierry Reding wrote: > On Tue, Sep 29, 2015 at 02:25:23PM +0900, Tomasz Figa wrote: > > 3) possibility of exporting the iommu_flush() operation and providing > > unsynchronized map/unmap operations for subsystems with even higher > > requirements for performance (e.g. drivers/gpu/drm). > > That would require passing in some sort of flag that the core shouldn't > be flushing itself, right? Currently it would flush on every map/unmap. Not necessarily. Introducing a flag would require updating all callers, so while at it we could also just change the semantics of map/unmap to require an explicit flush afterwards (only if it turns out that such an iommu_flush() function is really needed). Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Joerg Roedel <joro@8bytes.org> |
|---|---|
| Date | 2015-09-29 14:30 +0200 |
| Message-ID | <qe2cH-6p2-11@gated-at.bofh.it> |
| In reply to | #1234725 |
Hi Tomasz, On Tue, Sep 29, 2015 at 02:25:23PM +0900, Tomasz Figa wrote: > This series tries to mitigate the two issues above, while acknowledging > the fact that the .map_sg() callback might be still necessary for some > specific platforms, which could have the need to iterate over SG elements > inside driver code. Proposed solution introduces a new .flush() callback, > which expects IOVA range as its argument and is expected to flush all > respective caches (be it CPU, IOMMU TLB or whatever) to make the given > IOVA area mapping change visible to IOMMU clients. Then all the 3 basic > map/unmap operations are modified to call the .flush() callback at the end > of the operation. > > Advantages of proposed approach include: > 1) ability to use default_iommu_map_sg() helper if all the driver needs > for performance optimization is batching the flush, > 2) completely no effect on existing code - the .flush() callback is made > optional and if it isn't implemented drivers are expected to do > necessary flushes on a page by page basis in respective (un)mapping > callbakcs, > 3) possibility of exporting the iommu_flush() operation and providing > unsynchronized map/unmap operations for subsystems with even higher > requirements for performance (e.g. drivers/gpu/drm). Thanks for the patches, I really like the idea. The VT-d driver probably also benefits a lot from this. In the past I also proposed something like this, it was called a new iommu_commit() API function for making changes from map/unmap visible to the hardware. But this requires updating all callers first, so your approach of doing an implicit flush at the end of map/unmap is better. Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2015-09-29 16:30 +0200 |
| Message-ID | <qe44O-D4-23@gated-at.bofh.it> |
| In reply to | #1234725 |
Hi Tomasz, On 29/09/15 06:25, Tomasz Figa wrote: > Currently the IOMMU subsystem provides 3 basic operations: iommu_map(), > iommu_map_sg() and iommu_unmap(). iommu_map() can be used to map memory > page by page, however it involves flushing the caches (CPU and IOMMU) for > every mapped page separately, which is unsuitable for use cases that > require low mapping latency. Similarly iommu_unmap(), even though it > takes a full IOVA range as its argument, performs unmapping in a page > by page manner. This isn't necessarily the general case, though. If the IOMMU has a coherent page table walk interface and its architecture prohibits caching invalid PTEs, then the overhead on an unmap is only a TLB invalidation and the overhead on a map is nothing. > To make mapping operation more suitable for such use cases, iommu_map_sg() > and .map_sg() callback in iommu_ops struct were introduced, which allowed > particular IOMMU drivers to directly iterate over SG entries, create > necessary mappings and flush everything in one go. > > This approach, however, has two drawbacks: > 1) it does not do anything about unmap performance, > 2) it requires each driver willing to have fast map to implement its > own SG iteration code, even though this is a mostly generic operation. > > This series tries to mitigate the two issues above, while acknowledging > the fact that the .map_sg() callback might be still necessary for some > specific platforms, which could have the need to iterate over SG elements > inside driver code. Proposed solution introduces a new .flush() callback, > which expects IOVA range as its argument and is expected to flush all > respective caches (be it CPU, IOMMU TLB or whatever) to make the given > IOVA area mapping change visible to IOMMU clients. Then all the 3 basic > map/unmap operations are modified to call the .flush() callback at the end > of the operation. > > Advantages of proposed approach include: > 1) ability to use default_iommu_map_sg() helper if all the driver needs > for performance optimization is batching the flush, > 2) completely no effect on existing code - the .flush() callback is made > optional and if it isn't implemented drivers are expected to do > necessary flushes on a page by page basis in respective (un)mapping > callbakcs, > 3) possibility of exporting the iommu_flush() operation and providing > unsynchronized map/unmap operations for subsystems with even higher > requirements for performance (e.g. drivers/gpu/drm). A single callback doesn't really generalise well enough: If we wanted to implement this in the ARM SMMU drivers to optimise the unmap() case [ask Will how long he spends waiting for a software model to tear down an entire VFIO domain invalidating one page at a time ;)], then we'd either regress performance in the map() case with an unnecessary TLB flush, or have to do a table walk in every flush() call to infer what actually needs doing. Personally I think it would be nicest to have two separate callbacks, e.g. .map_sync/.unmap_sync, but at the very least some kind of additional 'direction' kind of parameter would be necessary. > The series includes a generic patch implementing necessary changes in > IOMMU API and two Tegra-specific patches that demonstrate implementation > on driver side and which can be used for further testing. > > Last, but not least, some performance numbers on Tegra210: > +-----------+--------------+-------------+------------+ > | Operation | Size [bytes] | Before [us] | After [us] | > +-----------+--------------+-------------+------------+ > | Map | 128K | 139 | 40 | > | | | 136 | 34 | > | | | 137 | 38 | > | | | 136 | 36 | > | | 4M | 3939 | 1163 | > | | | 3730 | 2389 | > | | | 3613 | 997 | > | | | 3622 | 1620 | > | | ~18M | 18635 | 4741 | > | | | 19261 | 6550 | > | | | 18473 | 9304 | > | | | 18125 | 5120 | > | Unmap | 128K | 128 | 7 | > | | | 122 | 8 | > | | | 119 | 10 | > | | | 123 | 12 | > | | 4M | 3829 | 151 | > | | | 3964 | 150 | > | | | 3908 | 145 | > | | | 3875 | 155 | > | | ~18M | 18570 | 683 | > | | | 18473 | 806 | > | | | 21020 | 643 | > | | | 21764 | 652 | > +-----------+--------------+-------------+------------+ > The values are obtained by surrounding the calls to iommu_map_sg() > (with default_iommu_map_sg() helper used as .map_sg() callback) and > iommu_unmap() with ktime-based time measurement code. Taken 4 samples > of every buffer size. ~18M means around 17-19M due do the variance > in requested buffer sizes. It would be interesting to know how much of the gain here is due to batching up the TLB maintenance vs. doing the DMA sync in fewer total pieces (with correspondingly fewer barriers). For the drivers which use the io-pgtable framework, trying to do anything about the latter (where it's relevant) looks essentially impossible without rewriting the whole thing, but the former is definitely something we should be able to handle and benefit from. It certainly seems like a reasonable way to get closer to the kind of iommu_map_range()/iommu_unmap_range() operations proposed before, but with less trampling on the external API. Plus it's nicer than the alternative workaround of having the driver claim anything larger than your basic page size is valid so you can batch up most of your pagetable updates behind the API's back. Robin. > Tomasz Figa (2): > iommu: Add support for out of band flushing > iommu/tegra-smmu: Make the driver use out of band flushing > > Vince Hsu (1): > memory: tegra: add TLB cache line size > > drivers/iommu/iommu.c | 33 +++++++++++++-- > drivers/iommu/tegra-smmu.c | 91 +++++++++++++++++++++++++++++++++++++---- > drivers/memory/tegra/tegra114.c | 1 + > drivers/memory/tegra/tegra124.c | 3 ++ > drivers/memory/tegra/tegra210.c | 1 + > drivers/memory/tegra/tegra30.c | 1 + > include/linux/iommu.h | 2 + > include/soc/tegra/mc.h | 1 + > 8 files changed, 122 insertions(+), 11 deletions(-) > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Russell King - ARM Linux <linux@arm.linux.org.uk> |
|---|---|
| Date | 2015-09-29 16:40 +0200 |
| Message-ID | <qe4eu-Oh-37@gated-at.bofh.it> |
| In reply to | #1235157 |
On Tue, Sep 29, 2015 at 03:20:38PM +0100, Robin Murphy wrote: > A single callback doesn't really generalise well enough: If we wanted to > implement this in the ARM SMMU drivers to optimise the unmap() case [ask > Will how long he spends waiting for a software model to tear down an entire > VFIO domain invalidating one page at a time ;)], then we'd either regress > performance in the map() case with an unnecessary TLB flush, or have to do a > table walk in every flush() call to infer what actually needs doing. And this is the problem of frameworks. They get in the way of doing things efficiently. Fine, we have the DMA ops, and that calls a map_sg() method. What we then need is to have a series of standardised library functions which can be called to perform various actions. Consider this: an IOMMU driver gets the raw scatterlist which the driver passed. The IOMMU driver walks the scatterlist, creating the IOMMU side mapping, and writing the device DMA addresses and DMA lengths to the scatterlist, possibly coalescing some of the entries. It remembers the number of scatterlist entries that the DMA operation now requires. The IOMMU code can setup whatever mappings it wants using whatever sizes it wants to satisfy the requested scatterlist. It then goes on to call the arch backend with the original scatterlist, asking it to _only_ deal with the CPU coherency for the mapping. The arch code walks the scatterlist again, this time dealing with the CPU coherency part. Finally, the IOMMU code returns the number of DMA scatterlist entries. When it comes to tearing it down, it's a similar operation to the above, except reversing those actions. The only issue with this approach is that it opens up some of the cache handling to the entire kernel, and that will be _too_ big a target for idiotic driver writers to think they have permission to directly use those interfaces. To solve this, I'd love to be able to have the linker link together certain objects in the kernel build, and then convert some global symbols to be local symbols, thus denying access to functions that driver authors have no business what so ever touching. > Personally I think it would be nicest to have two separate callbacks, e.g. > .map_sync/.unmap_sync, but at the very least some kind of additional > 'direction' kind of parameter would be necessary. No, not more callbacks - that's the framework thinking, not the library thinking. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2015-09-29 18:30 +0200 |
| Message-ID | <qe5WV-3k4-1@gated-at.bofh.it> |
| In reply to | #1235163 |
On 29/09/15 15:32, Russell King - ARM Linux wrote: > On Tue, Sep 29, 2015 at 03:20:38PM +0100, Robin Murphy wrote: >> A single callback doesn't really generalise well enough: If we wanted to >> implement this in the ARM SMMU drivers to optimise the unmap() case [ask >> Will how long he spends waiting for a software model to tear down an entire >> VFIO domain invalidating one page at a time ;)], then we'd either regress >> performance in the map() case with an unnecessary TLB flush, or have to do a >> table walk in every flush() call to infer what actually needs doing. > > And this is the problem of frameworks. They get in the way of doing > things efficiently. > > Fine, we have the DMA ops, and that calls a map_sg() method. What we > then need is to have a series of standardised library functions which > can be called to perform various actions. > Consider this: an IOMMU driver gets the raw scatterlist which the > driver passed. The IOMMU driver walks the scatterlist, creating the > IOMMU side mapping, and writing the device DMA addresses and DMA lengths > to the scatterlist, possibly coalescing some of the entries. It > remembers the number of scatterlist entries that the DMA operation now > requires. The IOMMU code can setup whatever mappings it wants using ... and making that elided "setup whatever mappings it wants" step more efficient is the sole thing that this patch set is trying to address. I apologise for not really following what you're getting at here. > whatever sizes it wants to satisfy the requested scatterlist. > > It then goes on to call the arch backend with the original scatterlist, > asking it to _only_ deal with the CPU coherency for the mapping. The > arch code walks the scatterlist again, this time dealing with the CPU > coherency part. > > Finally, the IOMMU code returns the number of DMA scatterlist entries. > > When it comes to tearing it down, it's a similar operation to the above, > except reversing those actions. > > The only issue with this approach is that it opens up some of the cache > handling to the entire kernel, and that will be _too_ big a target for > idiotic driver writers to think they have permission to directly use > those interfaces. To solve this, I'd love to be able to have the linker > link together certain objects in the kernel build, and then convert some > global symbols to be local symbols, thus denying access to functions that > driver authors have no business what so ever touching. > >> Personally I think it would be nicest to have two separate callbacks, e.g. >> .map_sync/.unmap_sync, but at the very least some kind of additional >> 'direction' kind of parameter would be necessary. > > No, not more callbacks - that's the framework thinking, not the library > thinking. Eh, swings and roundabouts. An argument denoting whether the flush is being called on the map or unmap path would be fine, it just means some implementations will be doing an extra no-op function call half the time. On closer inspection, the code in patch 3 _is_ using a table walk to figure out if the IOVA has been mapped or unmapped, it just happens that this particular implementation needs to do that walk anyway to sync the PTE updates, so gets away with it. Robin. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Russell King - ARM Linux <linux@arm.linux.org.uk> |
|---|---|
| Date | 2015-09-29 18:50 +0200 |
| Message-ID | <qe6gi-3Gy-23@gated-at.bofh.it> |
| In reply to | #1235361 |
On Tue, Sep 29, 2015 at 05:27:12PM +0100, Robin Murphy wrote: > Eh, swings and roundabouts. An argument denoting whether the flush is being > called on the map or unmap path would be fine, Sorry, that statement is wrong. It's not about whether you flush before or after the DMA operation. I'm afraid I'm probably going to tell you how to suck eggs here, because I don't think you quite "get it" with non-dma-coherent modern CPUs. Modern CPUs prefetch data into their caches, and they also randomly write back data from their caches to memory. When performing a DMA operation from device to memory, you need to do two things with CPU caches which aren't coherent: 1. Before starting the DMA operation, you need to walk over the memory to be mapped, ensuring that any dirty cache lines are written back. This is to prevent dirty cache lines overwriting data that has already been DMA'd from the device. 2. After the DMA operation has completed, you need to walk over the memory again, invalidating any cache lines which may have been speculatively loaded from that memory while DMA was running. These cache lines may have been loaded prior to the DMA operation placing the new data into memory. So, it's not a before-or-after, you have to always perform write-back cache maintanence prior to any DMA operation, and then invalidate cache maintanence after the DMA operation has completed for any mapping which the DMA may have written to (which means device-to-memory and bidirectional mappings.) -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2015-09-29 19:20 +0200 |
| Message-ID | <qe6Jk-4tk-1@gated-at.bofh.it> |
| In reply to | #1235389 |
On 29/09/15 17:40, Russell King - ARM Linux wrote: > On Tue, Sep 29, 2015 at 05:27:12PM +0100, Robin Murphy wrote: >> Eh, swings and roundabouts. An argument denoting whether the flush is being >> called on the map or unmap path would be fine, > > Sorry, that statement is wrong. It's not about whether you flush before > or after the DMA operation. I'm afraid I'm probably going to tell you > how to suck eggs here, because I don't think you quite "get it" with > non-dma-coherent modern CPUs. > > Modern CPUs prefetch data into their caches, and they also randomly write > back data from their caches to memory. When performing a DMA operation > from device to memory, you need to do two things with CPU caches which > aren't coherent: > > 1. Before starting the DMA operation, you need to walk over the memory to > be mapped, ensuring that any dirty cache lines are written back. This > is to prevent dirty cache lines overwriting data that has already been > DMA'd from the device. > > 2. After the DMA operation has completed, you need to walk over the > memory again, invalidating any cache lines which may have been > speculatively loaded from that memory while DMA was running. These > cache lines may have been loaded prior to the DMA operation placing > the new data into memory. > > So, it's not a before-or-after, you have to always perform write-back > cache maintanence prior to any DMA operation, and then invalidate cache > maintanence after the DMA operation has completed for any mapping which > the DMA may have written to (which means device-to-memory and > bidirectional mappings.) Yup, I'm well aware of all that; in fact you and I have already agreed elsewhere that we can only really get away with using the streaming DMA API to flush IOMMU page table updates _because_ they aren't written back to, thus data only ever goes from CPU->IOMMU and we can skip the problem of where to put an invalidation; you wrote the tegra-smmu code that does this. The coherency of whatever device which made a DMA API call for which the IOMMU API is creating/removing a mapping is irrelevant at this point - this is the DMA operation within the DMA operation. None of which has anything to do with the point I raised, which is that if iommu_unmap() calls iommu_flush(), I want to issue TLB invalidations, but if iommu_map() calls iommu_flush(), I don't. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web