Just like on paged s390 guests with their virtual IOMMU, syncing
mappings via virtio-iommu is quite expensive. It can thus benefit from
queueing unmapped IOVAs and flushing them in batches but less so from
parallel flushes which is what the shadow_on_flush flag introduced for
s390 tunes dma-iommu to do.
For this to work .flush_iotlb_all is implemented. Furthermore
.iotlb_sync_map is also implemented and used to pull the sync out of the
mapping operation for some additional batching and performance gain.
In a basic test with NVMe pass-through to a KVM guest on a Ryzen 3900X
these changes together lead to about 19% more IOPS in a fio test and
slightly more bandwidth too.
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Note:
The idea of using the single flush queue scheme from my series "iommu/dma: s390
DMA API conversion and optimized IOTLB flushing"[0] for virtio-iommu was already
mentioned in the cover letter. I now wanted to explore this with this patch
which may also serve as a test vehicle for the single flush queue scheme usable
on non-s390.
Besides limited testing, this is marked experimental mainly because the use of
queuing needs to be a concious decision as it allows continued access to
unmapped pages for up to a second with the currently proposed single flush
queue mechanism. Also it might make sense to split this patch to do the
introduction and use of .iotlb_sync_map separately but as a test vehicle
I found it easier to consume as a single patch.
[0]: https://lore.kernel.org/linux-iommu/20230717-dma_iommu-v11-0-a7a0b83c355c@linux.ibm.com/
drivers/iommu/virtio-iommu.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 3551ed057774..f29eb4ce2b88 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -843,7 +843,7 @@ static int viommu_map_pages(struct iommu_domain *domain, unsigned long iova,
.flags = cpu_to_le32(flags),
};
- ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
+ ret = viommu_add_req(vdomain->viommu, &map, sizeof(map));
if (ret) {
viommu_del_mappings(vdomain, iova, end);
return ret;
@@ -909,6 +909,27 @@ static void viommu_iotlb_sync(struct iommu_domain *domain,
{
struct viommu_domain *vdomain = to_viommu_domain(domain);
+ if (!vdomain->nr_endpoints)
+ return;
+ viommu_sync_req(vdomain->viommu);
+}
+
+static int viommu_iotlb_sync_map(struct iommu_domain *domain,
+ unsigned long iova, size_t size)
+{
+ struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+ if (!vdomain->nr_endpoints)
+ return 0;
+ return viommu_sync_req(vdomain->viommu);
+}
+
+static void viommu_flush_iotlb_all(struct iommu_domain *domain)
+{
+ struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+ if (!vdomain->nr_endpoints)
+ return;
viommu_sync_req(vdomain->viommu);
}
@@ -991,6 +1012,7 @@ static struct iommu_device *viommu_probe_device(struct device *dev)
if (ret)
goto err_free_dev;
}
+ dev->iommu->shadow_on_flush = 1;
return &viommu->iommu;
@@ -1037,6 +1059,8 @@ static bool viommu_capable(struct device *dev, enum iommu_cap cap)
switch (cap) {
case IOMMU_CAP_CACHE_COHERENCY:
return true;
+ case IOMMU_CAP_DEFERRED_FLUSH:
+ return true;
default:
return false;
}
@@ -1057,7 +1081,9 @@ static struct iommu_ops viommu_ops = {
.map_pages = viommu_map_pages,
.unmap_pages = viommu_unmap_pages,
.iova_to_phys = viommu_iova_to_phys,
+ .flush_iotlb_all = viommu_flush_iotlb_all,
.iotlb_sync = viommu_iotlb_sync,
+ .iotlb_sync_map = viommu_iotlb_sync_map,
.free = viommu_domain_free,
}
};
base-commit: 5514392fe77cd45b0d33bf239f13ba594a6759e5
--
2.39.2Hi Niklas,
On Wed, Jul 26, 2023 at 01:14:33PM +0200, Niklas Schnelle wrote:
> Just like on paged s390 guests with their virtual IOMMU, syncing
> mappings via virtio-iommu is quite expensive. It can thus benefit from
> queueing unmapped IOVAs and flushing them in batches but less so from
> parallel flushes which is what the shadow_on_flush flag introduced for
> s390 tunes dma-iommu to do.
>
> For this to work .flush_iotlb_all is implemented. Furthermore
> .iotlb_sync_map is also implemented and used to pull the sync out of the
> mapping operation for some additional batching and performance gain.
>
> In a basic test with NVMe pass-through to a KVM guest on a Ryzen 3900X
> these changes together lead to about 19% more IOPS in a fio test and
> slightly more bandwidth too.
Nice, thank you for testing this. I played with a NVMe on an Intel desktop
and can confirm similar results. With "sq" meaning single flush queue and
"mq" percpu flush queue, "+map" is with .iotlb_sync_map() enabled.
Multithread block randwrite job [1]:
BW compared to host Confidence
(higher better)
host 100.0% ±0.0%
noviommu 99.9 0.0
viommu lazy sq +map 99.9 0.1
viommu lazy mq +map 99.9 0.1
viommu lazy sq 92.2 0.9
viommu lazy mq 91.5 0.9
viommu strict +map 92.7 0.9
viommu strict 81.3 1.0
Single page randrw:
Latency compared to host Confidence
(lower better)
host x1.00 ±.04
noviommu 1.23 .04
viommu lazy sq +map 7.09 .05
viommu lazy mq +map 7.07 .07
viommu lazy sq 7.15 .04
viommu lazy mq 7.11 .05
viommu strict +map 8.82 .05
viommu strict 8.82 .04
So with lazy+map we get the maximum bandwidth reachable on this disk
(2.5GiB/s) even with a heavy iommu_map/unmap usage, which is cool.
Random access latency also improves with lazy mode.
The difference between single and percpu flush queue isn't really
measurable in my multithread test. There is a difference between Lazy sq
and mq but the variation between samples outweighs it.
>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
> Note:
> The idea of using the single flush queue scheme from my series "iommu/dma: s390
> DMA API conversion and optimized IOTLB flushing"[0] for virtio-iommu was already
> mentioned in the cover letter. I now wanted to explore this with this patch
> which may also serve as a test vehicle for the single flush queue scheme usable
> on non-s390.
>
> Besides limited testing, this is marked experimental mainly because the use of
> queuing needs to be a concious decision as it allows continued access to
> unmapped pages for up to a second with the currently proposed single flush
> queue mechanism.
It fits with the iommu.strict=0 / CONFIG_IOMMU_DEFAULT_DMA_LAZY setting,
which selects DMA_FQ domains. That option allows a misbehaving device to
access memory that has been freed/reallocated, which is what we're
enabling here. I believe the risk is pretty much the same for deferred
UNMAP as for deferred TLBI, since mappings that we're removing were likely
cached in the TLB. Increasing the timeout does make it easier to exploit,
but I don't think that changes the policy from an admin's perspective:
only enable lazy mode if you trust device and driver.
On bare metal, we disable DMA_FQ for devices that can be easily hotplugged
into unattended machines (through external-facing ports like thunderbolt).
On VMs, the concern isn't really about external devices, since they don't
automatically get plugged into a VM without user intervention. Here I
guess the devices we don't trust will be virtual devices implemented by
other VMs. We don't have any method to identify them yet, so
iommu.strict=1 and CONFIG_IOMMU_DEFAULT_DMA_STRICT is the best we can do
at the moment.
I'm not so sure about enabling shadow_on_flush by default, since the
performance difference was too small in my tests. Maybe a module parameter
for dma-iommu could configure the flush queue?
> Also it might make sense to split this patch to do the
> introduction and use of .iotlb_sync_map separately but as a test vehicle
> I found it easier to consume as a single patch.
Yes, both changes are useful but should be in two patches
Thanks,
Jean
[1] (Running one fio process for each of the two tests, sequentially, 30
or more times, with 1 warmup.)
[global]
filename=/dev/nvme0n1
runtime=10
ioengine=libaio
direct=1
time_based
[randwrite_multi]
group_reporting
rw=randwrite
numjobs=16
iodepth=16
bs=64k
size=1G
[randrw_page]
rw=randrw
rwmixread=50
rwmixwrite=50
iodepth=128
bs=4k
size=1G
>
> [0]: https://lore.kernel.org/linux-iommu/20230717-dma_iommu-v11-0-a7a0b83c355c@linux.ibm.com/
>
> drivers/iommu/virtio-iommu.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 3551ed057774..f29eb4ce2b88 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -843,7 +843,7 @@ static int viommu_map_pages(struct iommu_domain *domain, unsigned long iova,
> .flags = cpu_to_le32(flags),
> };
>
> - ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
> + ret = viommu_add_req(vdomain->viommu, &map, sizeof(map));
> if (ret) {
> viommu_del_mappings(vdomain, iova, end);
> return ret;
> @@ -909,6 +909,27 @@ static void viommu_iotlb_sync(struct iommu_domain *domain,
> {
> struct viommu_domain *vdomain = to_viommu_domain(domain);
>
> + if (!vdomain->nr_endpoints)
> + return;
> + viommu_sync_req(vdomain->viommu);
> +}
> +
> +static int viommu_iotlb_sync_map(struct iommu_domain *domain,
> + unsigned long iova, size_t size)
> +{
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + if (!vdomain->nr_endpoints)
> + return 0;
> + return viommu_sync_req(vdomain->viommu);
> +}
> +
> +static void viommu_flush_iotlb_all(struct iommu_domain *domain)
> +{
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + if (!vdomain->nr_endpoints)
> + return;
> viommu_sync_req(vdomain->viommu);
> }
>
> @@ -991,6 +1012,7 @@ static struct iommu_device *viommu_probe_device(struct device *dev)
> if (ret)
> goto err_free_dev;
> }
> + dev->iommu->shadow_on_flush = 1;
>
> return &viommu->iommu;
>
> @@ -1037,6 +1059,8 @@ static bool viommu_capable(struct device *dev, enum iommu_cap cap)
> switch (cap) {
> case IOMMU_CAP_CACHE_COHERENCY:
> return true;
> + case IOMMU_CAP_DEFERRED_FLUSH:
> + return true;
> default:
> return false;
> }
> @@ -1057,7 +1081,9 @@ static struct iommu_ops viommu_ops = {
> .map_pages = viommu_map_pages,
> .unmap_pages = viommu_unmap_pages,
> .iova_to_phys = viommu_iova_to_phys,
> + .flush_iotlb_all = viommu_flush_iotlb_all,
> .iotlb_sync = viommu_iotlb_sync,
> + .iotlb_sync_map = viommu_iotlb_sync_map,
> .free = viommu_domain_free,
> }
> };
>
> base-commit: 5514392fe77cd45b0d33bf239f13ba594a6759e5
> --
> 2.39.2
>
>
On Wed, 2023-08-02 at 13:36 +0100, Jean-Philippe Brucker wrote: > Hi Niklas, > > On Wed, Jul 26, 2023 at 01:14:33PM +0200, Niklas Schnelle wrote: > > Just like on paged s390 guests with their virtual IOMMU, syncing > > mappings via virtio-iommu is quite expensive. It can thus benefit from > > queueing unmapped IOVAs and flushing them in batches but less so from > > parallel flushes which is what the shadow_on_flush flag introduced for > > s390 tunes dma-iommu to do. > > > > For this to work .flush_iotlb_all is implemented. Furthermore > > .iotlb_sync_map is also implemented and used to pull the sync out of the > > mapping operation for some additional batching and performance gain. > > > > In a basic test with NVMe pass-through to a KVM guest on a Ryzen 3900X > > these changes together lead to about 19% more IOPS in a fio test and > > slightly more bandwidth too. > > Nice, thank you for testing this. I played with a NVMe on an Intel desktop > and can confirm similar results. With "sq" meaning single flush queue and > "mq" percpu flush queue, "+map" is with .iotlb_sync_map() enabled. > > Multithread block randwrite job [1]: > > BW compared to host Confidence > (higher better) > host 100.0% ±0.0% > noviommu 99.9 0.0 > viommu lazy sq +map 99.9 0.1 > viommu lazy mq +map 99.9 0.1 > viommu lazy sq 92.2 0.9 > viommu lazy mq 91.5 0.9 > viommu strict +map 92.7 0.9 > viommu strict 81.3 1.0 > > > Single page randrw: > > Latency compared to host Confidence > (lower better) > host x1.00 ±.04 > noviommu 1.23 .04 > viommu lazy sq +map 7.09 .05 > viommu lazy mq +map 7.07 .07 > viommu lazy sq 7.15 .04 > viommu lazy mq 7.11 .05 > viommu strict +map 8.82 .05 > viommu strict 8.82 .04 > > So with lazy+map we get the maximum bandwidth reachable on this disk > (2.5GiB/s) even with a heavy iommu_map/unmap usage, which is cool. > Random access latency also improves with lazy mode. > > The difference between single and percpu flush queue isn't really > measurable in my multithread test. There is a difference between Lazy sq > and mq but the variation between samples outweighs it. Interesting that there is no advantage when using a single large queue. Maybe virtio-iommu does handle the per-CPU flushing well because of a stronger CPU affinity than on s390x or maybe it has to do with the sync being just a queue drain making only some syncs slow. > > > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> > > --- > > Note: > > The idea of using the single flush queue scheme from my series "iommu/dma: s390 > > DMA API conversion and optimized IOTLB flushing"[0] for virtio-iommu was already > > mentioned in the cover letter. I now wanted to explore this with this patch > > which may also serve as a test vehicle for the single flush queue scheme usable > > on non-s390. > > > > Besides limited testing, this is marked experimental mainly because the use of > > queuing needs to be a concious decision as it allows continued access to > > unmapped pages for up to a second with the currently proposed single flush > > queue mechanism. > > It fits with the iommu.strict=0 / CONFIG_IOMMU_DEFAULT_DMA_LAZY setting, > which selects DMA_FQ domains. That option allows a misbehaving device to > access memory that has been freed/reallocated, which is what we're > enabling here. I believe the risk is pretty much the same for deferred > UNMAP as for deferred TLBI, since mappings that we're removing were likely > cached in the TLB. Increasing the timeout does make it easier to exploit, > but I don't think that changes the policy from an admin's perspective: > only enable lazy mode if you trust device and driver. I'm with Jason here. I think for non-CoCo hypervisor's the hypervisor is trusted so may just forward the trust level of the relevant device. This may of course be different for CoCo VMs but then the VM knows and could e.g. default to iommu.strict=1 or it needs to use swiotlb anyway. > > On bare metal, we disable DMA_FQ for devices that can be easily hotplugged > into unattended machines (through external-facing ports like thunderbolt). > On VMs, the concern isn't really about external devices, since they don't > automatically get plugged into a VM without user intervention. Here I > guess the devices we don't trust will be virtual devices implemented by > other VMs. We don't have any method to identify them yet, so > iommu.strict=1 and CONFIG_IOMMU_DEFAULT_DMA_STRICT is the best we can do > at the moment. > > I'm not so sure about enabling shadow_on_flush by default, since the > performance difference was too small in my tests. Maybe a module parameter > for dma-iommu could configure the flush queue? > > > Also it might make sense to split this patch to do the > > introduction and use of .iotlb_sync_map separately but as a test vehicle > > I found it easier to consume as a single patch. > > Yes, both changes are useful but should be in two patches > > Thanks, > Jean > After sending v12 of my series[0] I'm now looking into sending this separately. Rebasing on v6.5-rc7 however I noticed that without my patch "iommu: Allow .iotlb_sync_map to fail and handle s390's -ENOMEM return" we not only get a conflict with my series but also potentially miss errors on map since just like on s390x virtio-iommu's .iotlb_sync_map can fail. So I think it makes most sense to send this based on my series. I hope that's okay for you. [0] https://lore.kernel.org/lkml/20230825-dma_iommu-v12-0-4134455994a7@linux.ibm.com/
On Wed, Aug 02, 2023 at 01:36:12PM +0100, Jean-Philippe Brucker wrote: > automatically get plugged into a VM without user intervention. Here I > guess the devices we don't trust will be virtual devices implemented by > other VMs. We don't have any method to identify them yet, so > iommu.strict=1 and CONFIG_IOMMU_DEFAULT_DMA_STRICT is the best we can do > at the moment. VM's should work the same way as bare metal. The hypervisor should pass in an ACPI/etc indication if specific devices are to be untrusted. Otherwise the VM should assume trusted devices. The hypervisor can already read all the VM's memory, it doesn't make alot of sense for the VM to try and be defensive here in the general case. Jason
© 2016 - 2024 Red Hat, Inc.