This patch series is the follow up on the discussions we had before about
the RFC titled [RFC,V2] virtio: Add platform specific DMA API translation
for virito devices (https://patchwork.kernel.org/patch/10417371/). There
were suggestions about doing away with two different paths of transactions
with the host/QEMU, first being the direct GPA and the other being the DMA
API based translations.
First patch attempts to create a direct GPA mapping based DMA operations
structure called 'virtio_direct_dma_ops' with exact same implementation
of the direct GPA path which virtio core currently has but just wrapped in
a DMA API format. Virtio core must use 'virtio_direct_dma_ops' instead of
the arch default in absence of VIRTIO_F_IOMMU_PLATFORM flag to preserve the
existing semantics. The second patch does exactly that inside the function
virtio_finalize_features(). The third patch removes the default direct GPA
path from virtio core forcing it to use DMA API callbacks for all devices.
Now with that change, every device must have a DMA operations structure
associated with it. The fourth patch adds an additional hook which gives
the platform an opportunity to do yet another override if required. This
platform hook can be used on POWER Ultravisor based protected guests to
load up SWIOTLB DMA callbacks to do the required (as discussed previously
in the above mentioned thread how host is allowed to access only parts of
the guest GPA range) bounce buffering into the shared memory for all I/O
scatter gather buffers to be consumed on the host side.
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
Anshuman Khandual (4):
virtio: Define virtio_direct_dma_ops structure
virtio: Override device's DMA OPS with virtio_direct_dma_ops selectively
virtio: Force virtio core to use DMA API callbacks for all virtio devices
virtio: Add platform specific DMA API translation for virito devices
arch/powerpc/include/asm/dma-mapping.h | 6 +++
arch/powerpc/platforms/pseries/iommu.c | 6 +++
drivers/virtio/virtio.c | 72 ++++++++++++++++++++++++++++++++++
drivers/virtio/virtio_pci_common.h | 3 ++
drivers/virtio/virtio_ring.c | 65 +-----------------------------
5 files changed, 89 insertions(+), 63 deletions(-)
--
2.9.3
Now that virtio core always needs all virtio devices to have DMA OPS, we
need to make sure that the structure it points is the right one. In the
absence of VIRTIO_F_IOMMU_PLATFORM flag QEMU expects GPA from guest kernel.
In such case, virtio device must use default virtio_direct_dma_ops DMA OPS
structure which transforms scatter gather buffer addresses as GPA. This
DMA OPS override must happen as early as possible during virtio device
initializatin sequence before virtio core starts using given device's DMA
OPS callbacks for I/O transactions. This change detects device's IOMMU flag
and does the override in case the flag is cleared.
Signed-off-by: Anshuman Khandual <redacted>
---
drivers/virtio/virtio.c | 5 +++++
1 file changed, 5 insertions(+)
Current implementation of DMA API inside virtio core calls device's DMA OPS
callback functions when the flag VIRTIO_F_IOMMU_PLATFORM flag is set. But
in absence of the flag, virtio core falls back calling basic transformation
of the incoming SG addresses as GPA. Going forward virtio should only call
DMA API based transformations generating either GPA or IOVA depending on
QEMU expectations again based on VIRTIO_F_IOMMU_PLATFORM flag. It requires
removing existing fallback code path for GPA transformation and replacing
that with a direct map DMA OPS structure. This adds that direct mapping DMA
OPS structure to be used in later patches which will make virtio core call
DMA API all the time for all virtio devices.
Signed-off-by: Anshuman Khandual <redacted>
---
drivers/virtio/virtio.c | 60 ++++++++++++++++++++++++++++++++++++++
drivers/virtio/virtio_pci_common.h | 3 ++
2 files changed, 63 insertions(+)
@@ -31,6 +31,9 @@#include<linux/highmem.h>#include<linux/spinlock.h>+externstructdma_map_opsvirtio_direct_dma_ops;++structvirtio_pci_vq_info{/* the actual virtqueue */structvirtqueue*vq;
Virtio core should use DMA API callbacks for all virtio devices which may
generate either GAP or IOVA depending on VIRTIO_F_IOMMU_PLATFORM flag and
resulting QEMU expectations. This implies that every virtio device needs
to have a DMA OPS structure. This removes previous GPA fallback code paths.
Signed-off-by: Anshuman Khandual <redacted>
---
drivers/virtio/virtio_ring.c | 65 ++------------------------------------------
1 file changed, 2 insertions(+), 63 deletions(-)
@@ -141,26 +141,6 @@ struct vring_virtqueue {*unconditionallyondatapath.*/-staticboolvring_use_dma_api(structvirtio_device*vdev)-{-if(!virtio_has_iommu_quirk(vdev))-returntrue;--/* Otherwise, we are left to guess. */-/*-*Intheory,it'spossibletohaveabuggyQEMU-supposed-*emulatedQ35IOMMUandXenenabledatthesametime.On-*suchaconfiguration,virtiohasneverworkedandwill-*notworkwithoutanevenlargerkludge.Instead,enable-*theDMAAPIifwe'reaXenguest,whichatleastallows-*allofthesensibleXenconfigurationstoworkcorrectly.-*/-if(xen_domain())-returntrue;--returnfalse;-}-/**TheDMAopsonvariousarchesarerathergnarlyrightnow,and*makingallofthearchDMAopsworkonthevringdeviceitself
This adds a hook which a platform can define in order to allow it to
override virtio device's DMA OPS irrespective of whether it has the
flag VIRTIO_F_IOMMU_PLATFORM set or not. We want to use this to do
bounce-buffering of data on the new secure pSeries platform, currently
under development, where a KVM host cannot access all of the memory
space of a secure KVM guest. The host can only access the pages which
the guest has explicitly requested to be shared with the host, thus
the virtio implementation in the guest has to copy data to and from
shared pages.
With this hook, the platform code in the secure guest can force the
use of swiotlb for virtio buffers, with a back-end for swiotlb which
will use a pool of pre-allocated shared pages. Thus all data being
sent or received by virtio devices will be copied through pages which
the host has access to.
Signed-off-by: Anshuman Khandual <redacted>
---
arch/powerpc/include/asm/dma-mapping.h | 6 ++++++
arch/powerpc/platforms/pseries/iommu.c | 6 ++++++
drivers/virtio/virtio.c | 7 +++++++
3 files changed, 19 insertions(+)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-07-20 13:15:38
On Fri, Jul 20, 2018 at 09:29:41AM +0530, Anshuman Khandual wrote:
Subject: Re: [RFC 4/4] virtio: Add platform specific DMA API translation for
virito devices
s/virito/virtio/
quoted hunk
This adds a hook which a platform can define in order to allow it to
override virtio device's DMA OPS irrespective of whether it has the
flag VIRTIO_F_IOMMU_PLATFORM set or not. We want to use this to do
bounce-buffering of data on the new secure pSeries platform, currently
under development, where a KVM host cannot access all of the memory
space of a secure KVM guest. The host can only access the pages which
the guest has explicitly requested to be shared with the host, thus
the virtio implementation in the guest has to copy data to and from
shared pages.
With this hook, the platform code in the secure guest can force the
use of swiotlb for virtio buffers, with a back-end for swiotlb which
will use a pool of pre-allocated shared pages. Thus all data being
sent or received by virtio devices will be copied through pages which
the host has access to.
Signed-off-by: Anshuman Khandual <redacted>
---
arch/powerpc/include/asm/dma-mapping.h | 6 ++++++
arch/powerpc/platforms/pseries/iommu.c | 6 ++++++
drivers/virtio/virtio.c | 7 +++++++
3 files changed, 19 insertions(+)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-07-20 13:17:07
On Fri, Jul 20, 2018 at 09:29:37AM +0530, Anshuman Khandual wrote:
This patch series is the follow up on the discussions we had before about
the RFC titled [RFC,V2] virtio: Add platform specific DMA API translation
for virito devices (https://patchwork.kernel.org/patch/10417371/). There
were suggestions about doing away with two different paths of transactions
with the host/QEMU, first being the direct GPA and the other being the DMA
API based translations.
First patch attempts to create a direct GPA mapping based DMA operations
structure called 'virtio_direct_dma_ops' with exact same implementation
of the direct GPA path which virtio core currently has but just wrapped in
a DMA API format. Virtio core must use 'virtio_direct_dma_ops' instead of
the arch default in absence of VIRTIO_F_IOMMU_PLATFORM flag to preserve the
existing semantics. The second patch does exactly that inside the function
virtio_finalize_features(). The third patch removes the default direct GPA
path from virtio core forcing it to use DMA API callbacks for all devices.
Now with that change, every device must have a DMA operations structure
associated with it. The fourth patch adds an additional hook which gives
the platform an opportunity to do yet another override if required. This
platform hook can be used on POWER Ultravisor based protected guests to
load up SWIOTLB DMA callbacks to do the required (as discussed previously
in the above mentioned thread how host is allowed to access only parts of
the guest GPA range) bounce buffering into the shared memory for all I/O
scatter gather buffers to be consumed on the host side.
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
I like how patches 1-3 look. Could you test performance
with/without to see whether the extra indirection through
use of DMA ops causes a measurable slow-down?
Anshuman Khandual (4):
virtio: Define virtio_direct_dma_ops structure
virtio: Override device's DMA OPS with virtio_direct_dma_ops selectively
virtio: Force virtio core to use DMA API callbacks for all virtio devices
virtio: Add platform specific DMA API translation for virito devices
arch/powerpc/include/asm/dma-mapping.h | 6 +++
arch/powerpc/platforms/pseries/iommu.c | 6 +++
drivers/virtio/virtio.c | 72 ++++++++++++++++++++++++++++++++++
drivers/virtio/virtio_pci_common.h | 3 ++
drivers/virtio/virtio_ring.c | 65 +-----------------------------
5 files changed, 89 insertions(+), 63 deletions(-)
--
2.9.3
On Fri, Jul 20, 2018 at 09:29:41AM +0530, Anshuman Khandual wrote:
quoted
Subject: Re: [RFC 4/4] virtio: Add platform specific DMA API translation for
virito devices
s/virito/virtio/
Oops, will fix it. Thanks for pointing out.
quoted
This adds a hook which a platform can define in order to allow it to
override virtio device's DMA OPS irrespective of whether it has the
flag VIRTIO_F_IOMMU_PLATFORM set or not. We want to use this to do
bounce-buffering of data on the new secure pSeries platform, currently
under development, where a KVM host cannot access all of the memory
space of a secure KVM guest. The host can only access the pages which
the guest has explicitly requested to be shared with the host, thus
the virtio implementation in the guest has to copy data to and from
shared pages.
With this hook, the platform code in the secure guest can force the
use of swiotlb for virtio buffers, with a back-end for swiotlb which
will use a pool of pre-allocated shared pages. Thus all data being
sent or received by virtio devices will be copied through pages which
the host has access to.
Signed-off-by: Anshuman Khandual <redacted>
---
arch/powerpc/include/asm/dma-mapping.h | 6 ++++++
arch/powerpc/platforms/pseries/iommu.c | 6 ++++++
drivers/virtio/virtio.c | 7 +++++++
3 files changed, 19 insertions(+)
@@ -179,6 +185,7 @@ int virtio_finalize_features(struct virtio_device *dev)if(virtio_has_iommu_quirk(dev))set_dma_ops(dev->dev.parent,&virtio_direct_dma_ops);+platform_override_dma_ops(dev);
Is there a single place where virtio_has_iommu_quirk is called now?
Not other than this one. But in the proposed implementation of
platform_override_dma_ops on powerpc, we will again check on
virtio_has_iommu_quirk before overriding it with SWIOTLB.
void platform_override_dma_ops(struct virtio_device *vdev)
{
if (is_ultravisor_platform() && virtio_has_iommu_quirk(vdev))
set_dma_ops(vdev->dev.parent, &swiotlb_dma_ops);
}
If so, we could put this into virtio_has_iommu_quirk then.
Did you mean platform_override_dma_ops instead ? If so, yes that
is possible. Default implementation of platform_override_dma_ops
should just check on VIRTIO_F_IOMMU_PLATFORM feature and override
with virtio_direct_dma_ops but arch implementation can check on
what ever else they would like and override appropriately.
Default platform_override_dma_ops will be like this
#ifndef platform_override_dma_ops
static inline void platform_override_dma_ops(struct virtio_device *vdev)
{
if(!virtio_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM))
set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
}
#endif
Proposed powerpc implementation will be like this instead
void platform_override_dma_ops(struct virtio_device *vdev)
{
if (virtio_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM))
return;
if (is_ultravisor_platform())
set_dma_ops(vdev->dev.parent, &swiotlb_dma_ops);
else
set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
}
There is a redundant definition of virtio_has_iommu_quirk in the tools
directory (tools/virtio/linux/virtio_config.h) which does not seem to
be used any where. I guess that can be removed without problem.
On Fri, Jul 20, 2018 at 09:29:37AM +0530, Anshuman Khandual wrote:
quoted
This patch series is the follow up on the discussions we had before about
the RFC titled [RFC,V2] virtio: Add platform specific DMA API translation
for virito devices (https://patchwork.kernel.org/patch/10417371/). There
were suggestions about doing away with two different paths of transactions
with the host/QEMU, first being the direct GPA and the other being the DMA
API based translations.
First patch attempts to create a direct GPA mapping based DMA operations
structure called 'virtio_direct_dma_ops' with exact same implementation
of the direct GPA path which virtio core currently has but just wrapped in
a DMA API format. Virtio core must use 'virtio_direct_dma_ops' instead of
the arch default in absence of VIRTIO_F_IOMMU_PLATFORM flag to preserve the
existing semantics. The second patch does exactly that inside the function
virtio_finalize_features(). The third patch removes the default direct GPA
path from virtio core forcing it to use DMA API callbacks for all devices.
Now with that change, every device must have a DMA operations structure
associated with it. The fourth patch adds an additional hook which gives
the platform an opportunity to do yet another override if required. This
platform hook can be used on POWER Ultravisor based protected guests to
load up SWIOTLB DMA callbacks to do the required (as discussed previously
in the above mentioned thread how host is allowed to access only parts of
the guest GPA range) bounce buffering into the shared memory for all I/O
scatter gather buffers to be consumed on the host side.
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
I like how patches 1-3 look. Could you test performance
with/without to see whether the extra indirection through
use of DMA ops causes a measurable slow-down?
I ran this simple DD command 10 times where /dev/vda is a virtio block
device of 10GB size.
dd if=/dev/zero of=/dev/vda bs=8M count=1024 oflag=direct
With and without patches bandwidth which has a bit wide range does not
look that different from each other.
Without patches
===============
---------- 1 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.95557 s, 4.4 GB/s
---------- 2 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.05176 s, 4.2 GB/s
---------- 3 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.88314 s, 4.6 GB/s
---------- 4 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.84899 s, 4.6 GB/s
---------- 5 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 5.37184 s, 1.6 GB/s
---------- 6 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.9205 s, 4.5 GB/s
---------- 7 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 6.85166 s, 1.3 GB/s
---------- 8 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.74049 s, 4.9 GB/s
---------- 9 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 6.31699 s, 1.4 GB/s
---------- 10 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.47057 s, 3.5 GB/s
With patches
============
---------- 1 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.25993 s, 3.8 GB/s
---------- 2 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.82438 s, 4.7 GB/s
---------- 3 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.93856 s, 4.4 GB/s
---------- 4 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.83405 s, 4.7 GB/s
---------- 5 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 7.50199 s, 1.1 GB/s
---------- 6 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.28742 s, 3.8 GB/s
---------- 7 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 5.74958 s, 1.5 GB/s
---------- 8 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.99149 s, 4.3 GB/s
---------- 9 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 5.67647 s, 1.5 GB/s
---------- 10 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.93957 s, 2.9 GB/s
Does this look okay ?
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-07-23 09:08:58
On Mon, Jul 23, 2018 at 11:58:23AM +0530, Anshuman Khandual wrote:
On 07/20/2018 06:46 PM, Michael S. Tsirkin wrote:
quoted
On Fri, Jul 20, 2018 at 09:29:37AM +0530, Anshuman Khandual wrote:
quoted
This patch series is the follow up on the discussions we had before about
the RFC titled [RFC,V2] virtio: Add platform specific DMA API translation
for virito devices (https://patchwork.kernel.org/patch/10417371/). There
were suggestions about doing away with two different paths of transactions
with the host/QEMU, first being the direct GPA and the other being the DMA
API based translations.
First patch attempts to create a direct GPA mapping based DMA operations
structure called 'virtio_direct_dma_ops' with exact same implementation
of the direct GPA path which virtio core currently has but just wrapped in
a DMA API format. Virtio core must use 'virtio_direct_dma_ops' instead of
the arch default in absence of VIRTIO_F_IOMMU_PLATFORM flag to preserve the
existing semantics. The second patch does exactly that inside the function
virtio_finalize_features(). The third patch removes the default direct GPA
path from virtio core forcing it to use DMA API callbacks for all devices.
Now with that change, every device must have a DMA operations structure
associated with it. The fourth patch adds an additional hook which gives
the platform an opportunity to do yet another override if required. This
platform hook can be used on POWER Ultravisor based protected guests to
load up SWIOTLB DMA callbacks to do the required (as discussed previously
in the above mentioned thread how host is allowed to access only parts of
the guest GPA range) bounce buffering into the shared memory for all I/O
scatter gather buffers to be consumed on the host side.
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
I like how patches 1-3 look. Could you test performance
with/without to see whether the extra indirection through
use of DMA ops causes a measurable slow-down?
I ran this simple DD command 10 times where /dev/vda is a virtio block
device of 10GB size.
dd if=/dev/zero of=/dev/vda bs=8M count=1024 oflag=direct
With and without patches bandwidth which has a bit wide range does not
look that different from each other.
Without patches
===============
---------- 1 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.95557 s, 4.4 GB/s
---------- 2 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.05176 s, 4.2 GB/s
---------- 3 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.88314 s, 4.6 GB/s
---------- 4 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.84899 s, 4.6 GB/s
---------- 5 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 5.37184 s, 1.6 GB/s
---------- 6 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.9205 s, 4.5 GB/s
---------- 7 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 6.85166 s, 1.3 GB/s
---------- 8 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.74049 s, 4.9 GB/s
---------- 9 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 6.31699 s, 1.4 GB/s
---------- 10 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.47057 s, 3.5 GB/s
With patches
============
---------- 1 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.25993 s, 3.8 GB/s
---------- 2 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.82438 s, 4.7 GB/s
---------- 3 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.93856 s, 4.4 GB/s
---------- 4 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.83405 s, 4.7 GB/s
---------- 5 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 7.50199 s, 1.1 GB/s
---------- 6 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.28742 s, 3.8 GB/s
---------- 7 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 5.74958 s, 1.5 GB/s
---------- 8 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.99149 s, 4.3 GB/s
---------- 9 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 5.67647 s, 1.5 GB/s
---------- 10 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.93957 s, 2.9 GB/s
Does this look okay ?
You want to test IOPS with lots of small writes and using
raw ramdisk on host.
--
MST
On Mon, Jul 23, 2018 at 11:58:23AM +0530, Anshuman Khandual wrote:
quoted
On 07/20/2018 06:46 PM, Michael S. Tsirkin wrote:
quoted
On Fri, Jul 20, 2018 at 09:29:37AM +0530, Anshuman Khandual wrote:
quoted
This patch series is the follow up on the discussions we had before about
the RFC titled [RFC,V2] virtio: Add platform specific DMA API translation
for virito devices (https://patchwork.kernel.org/patch/10417371/). There
were suggestions about doing away with two different paths of transactions
with the host/QEMU, first being the direct GPA and the other being the DMA
API based translations.
First patch attempts to create a direct GPA mapping based DMA operations
structure called 'virtio_direct_dma_ops' with exact same implementation
of the direct GPA path which virtio core currently has but just wrapped in
a DMA API format. Virtio core must use 'virtio_direct_dma_ops' instead of
the arch default in absence of VIRTIO_F_IOMMU_PLATFORM flag to preserve the
existing semantics. The second patch does exactly that inside the function
virtio_finalize_features(). The third patch removes the default direct GPA
path from virtio core forcing it to use DMA API callbacks for all devices.
Now with that change, every device must have a DMA operations structure
associated with it. The fourth patch adds an additional hook which gives
the platform an opportunity to do yet another override if required. This
platform hook can be used on POWER Ultravisor based protected guests to
load up SWIOTLB DMA callbacks to do the required (as discussed previously
in the above mentioned thread how host is allowed to access only parts of
the guest GPA range) bounce buffering into the shared memory for all I/O
scatter gather buffers to be consumed on the host side.
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
I like how patches 1-3 look. Could you test performance
with/without to see whether the extra indirection through
use of DMA ops causes a measurable slow-down?
I ran this simple DD command 10 times where /dev/vda is a virtio block
device of 10GB size.
dd if=/dev/zero of=/dev/vda bs=8M count=1024 oflag=direct
With and without patches bandwidth which has a bit wide range does not
look that different from each other.
Without patches
===============
---------- 1 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.95557 s, 4.4 GB/s
---------- 2 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.05176 s, 4.2 GB/s
---------- 3 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.88314 s, 4.6 GB/s
---------- 4 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.84899 s, 4.6 GB/s
---------- 5 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 5.37184 s, 1.6 GB/s
---------- 6 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.9205 s, 4.5 GB/s
---------- 7 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 6.85166 s, 1.3 GB/s
---------- 8 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.74049 s, 4.9 GB/s
---------- 9 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 6.31699 s, 1.4 GB/s
---------- 10 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.47057 s, 3.5 GB/s
With patches
============
---------- 1 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.25993 s, 3.8 GB/s
---------- 2 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.82438 s, 4.7 GB/s
---------- 3 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.93856 s, 4.4 GB/s
---------- 4 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.83405 s, 4.7 GB/s
---------- 5 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 7.50199 s, 1.1 GB/s
---------- 6 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.28742 s, 3.8 GB/s
---------- 7 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 5.74958 s, 1.5 GB/s
---------- 8 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 1.99149 s, 4.3 GB/s
---------- 9 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 5.67647 s, 1.5 GB/s
---------- 10 ---------
1024+0 records in
1024+0 records out
8589934592 bytes (8.6 GB, 8.0 GiB) copied, 2.93957 s, 2.9 GB/s
Does this look okay ?
You want to test IOPS with lots of small writes and using
raw ramdisk on host.
Hello Michael,
I have conducted the following experiments and here are the results.
TEST SETUP
==========
A virtio block disk is mounted on the guest as follows.
<disk type='file' device='disk'>
<driver name='qemu' type='raw' ioeventfd='off'/>
<source file='/mnt/disk2.img'/>
<target dev='vdb' bus='virtio'/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
</disk>
In the host back end its an QEMU raw image on tmpfs file system.
disk:
-rw-r--r-- 1 libvirt-qemu kvm 5.0G Jul 24 06:26 disk2.img
mount:
size=21G on /mnt type tmpfs (rw,relatime,size=22020096k)
TEST CONFIG
===========
FIO (https://linux.die.net/man/1/fio) is being run with and without
the patches.
Read test config:
[Sequential]
direct=1
ioengine=libaio
runtime=5m
time_based
filename=/dev/vda
bs=4k
numjobs=16
rw=read
unlink=1
iodepth=256
Write test config:
[Sequential]
direct=1
ioengine=libaio
runtime=5m
time_based
filename=/dev/vda
bs=4k
numjobs=16
rw=write
unlink=1
iodepth=256
The virtio block device comes up as /dev/vda on the guest with
/sys/block/vda/queue/nr_requests=128
TEST RESULTS
============
Without the patches
-------------------
Read test:
Run status group 0 (all jobs):
READ: bw=550MiB/s (577MB/s), 33.2MiB/s-35.6MiB/s (34.9MB/s-37.4MB/s), io=161GiB (173GB), run=300001-300009msec
Disk stats (read/write):
vda: ios=42249926/0, merge=0/0, ticks=1499920/0, in_queue=35672384, util=100.00%
Write test:
Run status group 0 (all jobs):
WRITE: bw=514MiB/s (539MB/s), 31.5MiB/s-34.6MiB/s (33.0MB/s-36.2MB/s), io=151GiB (162GB), run=300001-300009msec
Disk stats (read/write):
vda: ios=29/39459261, merge=0/0, ticks=0/1570580, in_queue=35745992, util=100.00%
With the patches
----------------
Read test:
Run status group 0 (all jobs):
READ: bw=572MiB/s (600MB/s), 35.0MiB/s-37.2MiB/s (36.7MB/s-38.0MB/s), io=168GiB (180GB), run=300001-300006msec
Disk stats (read/write):
vda: ios=43917611/0, merge=0/0, ticks=1934268/0, in_queue=35531688, util=100.00%
Write test:
Run status group 0 (all jobs):
WRITE: bw=546MiB/s (572MB/s), 33.7MiB/s-35.0MiB/s (35.3MB/s-36.7MB/s), io=160GiB (172GB), run=300001-300007msec
Disk stats (read/write):
vda: ios=14/41893878, merge=0/0, ticks=8/2107816, in_queue=35535716, util=100.00%
Results with and without the patches are similar.
On Fri, Jul 20, 2018 at 09:29:41AM +0530, Anshuman Khandual wrote:
quoted
Subject: Re: [RFC 4/4] virtio: Add platform specific DMA API translation for
virito devices
s/virito/virtio/
Oops, will fix it. Thanks for pointing out.
quoted
quoted
This adds a hook which a platform can define in order to allow it to
override virtio device's DMA OPS irrespective of whether it has the
flag VIRTIO_F_IOMMU_PLATFORM set or not. We want to use this to do
bounce-buffering of data on the new secure pSeries platform, currently
under development, where a KVM host cannot access all of the memory
space of a secure KVM guest. The host can only access the pages which
the guest has explicitly requested to be shared with the host, thus
the virtio implementation in the guest has to copy data to and from
shared pages.
With this hook, the platform code in the secure guest can force the
use of swiotlb for virtio buffers, with a back-end for swiotlb which
will use a pool of pre-allocated shared pages. Thus all data being
sent or received by virtio devices will be copied through pages which
the host has access to.
Signed-off-by: Anshuman Khandual <redacted>
---
arch/powerpc/include/asm/dma-mapping.h | 6 ++++++
arch/powerpc/platforms/pseries/iommu.c | 6 ++++++
drivers/virtio/virtio.c | 7 +++++++
3 files changed, 19 insertions(+)
@@ -179,6 +185,7 @@ int virtio_finalize_features(struct virtio_device *dev)if(virtio_has_iommu_quirk(dev))set_dma_ops(dev->dev.parent,&virtio_direct_dma_ops);+platform_override_dma_ops(dev);
Is there a single place where virtio_has_iommu_quirk is called now?
Not other than this one. But in the proposed implementation of
platform_override_dma_ops on powerpc, we will again check on
virtio_has_iommu_quirk before overriding it with SWIOTLB.
void platform_override_dma_ops(struct virtio_device *vdev)
{
if (is_ultravisor_platform() && virtio_has_iommu_quirk(vdev))
set_dma_ops(vdev->dev.parent, &swiotlb_dma_ops);
}
quoted
If so, we could put this into virtio_has_iommu_quirk then.
Did you mean platform_override_dma_ops instead ? If so, yes that
is possible. Default implementation of platform_override_dma_ops
should just check on VIRTIO_F_IOMMU_PLATFORM feature and override
with virtio_direct_dma_ops but arch implementation can check on
what ever else they would like and override appropriately.
Default platform_override_dma_ops will be like this
#ifndef platform_override_dma_ops
static inline void platform_override_dma_ops(struct virtio_device *vdev)
{
if(!virtio_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM))
set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
}
#endif
Proposed powerpc implementation will be like this instead
void platform_override_dma_ops(struct virtio_device *vdev)
{
if (virtio_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM))
return;
if (is_ultravisor_platform())
set_dma_ops(vdev->dev.parent, &swiotlb_dma_ops);
else
set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
}
There is a redundant definition of virtio_has_iommu_quirk in the tools
directory (tools/virtio/linux/virtio_config.h) which does not seem to
be used any where. I guess that can be removed without problem.
Does this sound okay ? It will merge patch 3 and 4 into a single one.
On the other hand it also passes the responsibility of dealing with
VIRTIO_F_IOMMU_PLATFORM flag to the architecture callback which might
be bit problematic. Keeping VIRTIO_F_IOMMU_PLATFORM handling in virtio
core at least makes sure that the device has a working DMA ops to fall
back on if the arch hook fails to take care of it somehow.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-07-25 13:31:28
On Mon, Jul 23, 2018 at 07:46:09AM +0530, Anshuman Khandual wrote:
There is a redundant definition of virtio_has_iommu_quirk in the tools
directory (tools/virtio/linux/virtio_config.h) which does not seem to
be used any where. I guess that can be removed without problem.
It's there just to make tools/virtio build.
Try
make -C tools/virtio/
In fact I see there's a missing definition for
dma_rmb/dma_wmb there, I'll post a patch.
--
MST
From: Will Deacon <hidden> Date: 2018-07-27 09:58:08
Hi Anshuman,
On Fri, Jul 20, 2018 at 09:29:37AM +0530, Anshuman Khandual wrote:
This patch series is the follow up on the discussions we had before about
the RFC titled [RFC,V2] virtio: Add platform specific DMA API translation
for virito devices (https://patchwork.kernel.org/patch/10417371/). There
were suggestions about doing away with two different paths of transactions
with the host/QEMU, first being the direct GPA and the other being the DMA
API based translations.
First patch attempts to create a direct GPA mapping based DMA operations
structure called 'virtio_direct_dma_ops' with exact same implementation
of the direct GPA path which virtio core currently has but just wrapped in
a DMA API format. Virtio core must use 'virtio_direct_dma_ops' instead of
the arch default in absence of VIRTIO_F_IOMMU_PLATFORM flag to preserve the
existing semantics. The second patch does exactly that inside the function
virtio_finalize_features(). The third patch removes the default direct GPA
path from virtio core forcing it to use DMA API callbacks for all devices.
Now with that change, every device must have a DMA operations structure
associated with it. The fourth patch adds an additional hook which gives
the platform an opportunity to do yet another override if required. This
platform hook can be used on POWER Ultravisor based protected guests to
load up SWIOTLB DMA callbacks to do the required (as discussed previously
in the above mentioned thread how host is allowed to access only parts of
the guest GPA range) bounce buffering into the shared memory for all I/O
scatter gather buffers to be consumed on the host side.
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
I just wanted to say that this patch series provides a means for us to
force the coherent DMA ops for legacy virtio devices on arm64, which in turn
means that we can enable the SMMU with legacy devices in our fastmodel
emulation platform (which is slowly being upgraded to virtio 1.0) without
hanging during boot. Patch below.
So:
Acked-by: Will Deacon <redacted>
Tested-by: Will Deacon <redacted>
Thanks!
Will
--->8
From 4ef39e9de2c87c97bf046816ca762832f92e39b5 Mon Sep 17 00:00:00 2001
From: Will Deacon <redacted>
Date: Fri, 27 Jul 2018 10:49:25 +0100
Subject: [PATCH] arm64: dma: Override DMA ops for legacy virtio devices
Virtio devices are always cache-coherent, so force use of the coherent
DMA ops for legacy virtio devices where the dma-coherent is known to
be omitted by QEMU for the MMIO transport.
Signed-off-by: Will Deacon <redacted>
---
arch/arm64/include/asm/dma-mapping.h | 6 ++++++
arch/arm64/mm/dma-mapping.c | 19 +++++++++++++++++++
2 files changed, 25 insertions(+)
@@ -44,6 +44,12 @@ void arch_teardown_dma_ops(struct device *dev);#define arch_teardown_dma_ops arch_teardown_dma_ops#endif+#ifdef CONFIG_VIRTIO+structvirtio_device;+voidplatform_override_dma_ops(structvirtio_device*vdev);+#define platform_override_dma_ops platform_override_dma_ops+#endif+/* do not use this function in a driver */staticinlineboolis_device_dma_coherent(structdevice*dev){
Hi Anshuman,
On Fri, Jul 20, 2018 at 09:29:37AM +0530, Anshuman Khandual wrote:
quoted
This patch series is the follow up on the discussions we had before about
the RFC titled [RFC,V2] virtio: Add platform specific DMA API translation
for virito devices (https://patchwork.kernel.org/patch/10417371/). There
were suggestions about doing away with two different paths of transactions
with the host/QEMU, first being the direct GPA and the other being the DMA
API based translations.
First patch attempts to create a direct GPA mapping based DMA operations
structure called 'virtio_direct_dma_ops' with exact same implementation
of the direct GPA path which virtio core currently has but just wrapped in
a DMA API format. Virtio core must use 'virtio_direct_dma_ops' instead of
the arch default in absence of VIRTIO_F_IOMMU_PLATFORM flag to preserve the
existing semantics. The second patch does exactly that inside the function
virtio_finalize_features(). The third patch removes the default direct GPA
path from virtio core forcing it to use DMA API callbacks for all devices.
Now with that change, every device must have a DMA operations structure
associated with it. The fourth patch adds an additional hook which gives
the platform an opportunity to do yet another override if required. This
platform hook can be used on POWER Ultravisor based protected guests to
load up SWIOTLB DMA callbacks to do the required (as discussed previously
in the above mentioned thread how host is allowed to access only parts of
the guest GPA range) bounce buffering into the shared memory for all I/O
scatter gather buffers to be consumed on the host side.
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
I just wanted to say that this patch series provides a means for us to
force the coherent DMA ops for legacy virtio devices on arm64, which in turn
means that we can enable the SMMU with legacy devices in our fastmodel
emulation platform (which is slowly being upgraded to virtio 1.0) without
hanging during boot. Patch below.
So:
Acked-by: Will Deacon <redacted>
Tested-by: Will Deacon <redacted>
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-07-27 11:31:33
On Wed, Jul 25, 2018 at 08:56:23AM +0530, Anshuman Khandual wrote:
Results with and without the patches are similar.
Thanks! And another thing to try is virtio-net with
a fast NIC backend (40G and up). Unfortunately
at this point loopback tests stress the host
scheduler too much.
--
MST
On Wed, Jul 25, 2018 at 08:56:23AM +0530, Anshuman Khandual wrote:
quoted
Results with and without the patches are similar.
Thanks! And another thing to try is virtio-net with
a fast NIC backend (40G and up). Unfortunately
at this point loopback tests stress the host
scheduler too much.
Sure. Will look around for a 40G NIC system. BTW I have been testing
virtio-net with a TAP device as back end.
ip tuntap add dev tap0 mode tap user $(whoami)
ip link set tap0 master virbr0
ip link set dev virbr0 up
ip link set dev tap0 up
which is exported into the guest as follows
-device virtio-net,netdev=network0,mac=52:55:00:d1:55:01 \
-netdev tap,id=network0,ifname=tap0,script=no,downscript=no \
But I have not run any network benchmarks on it though.
Now that virtio core always needs all virtio devices to have DMA OPS, we
need to make sure that the structure it points is the right one. In the
absence of VIRTIO_F_IOMMU_PLATFORM flag QEMU expects GPA from guest kernel.
In such case, virtio device must use default virtio_direct_dma_ops DMA OPS
structure which transforms scatter gather buffer addresses as GPA. This
DMA OPS override must happen as early as possible during virtio device
initializatin sequence before virtio core starts using given device's DMA
OPS callbacks for I/O transactions. This change detects device's IOMMU flag
and does the override in case the flag is cleared.
Signed-off-by: Anshuman Khandual <redacted>
---
drivers/virtio/virtio.c | 5 +++++
1 file changed, 5 insertions(+)
@@ -174,6 +176,9 @@ int virtio_finalize_features(struct virtio_device *dev)if(ret)returnret;
The previous patch removed the code block for XEN guests which forced
the use of DMA API all the time irrespective of VIRTIO_F_IOMMU_PLATFORM
flag on the device. Here is what I have removed with patch 2/4 which
breaks the existing semantics on XEN guests.
-static bool vring_use_dma_api(struct virtio_device *vdev)
-{
- if (!virtio_has_iommu_quirk(vdev))
- return true;
-
- /* Otherwise, we are left to guess. */
- /*
- * In theory, it's possible to have a buggy QEMU-supposed
- * emulated Q35 IOMMU and Xen enabled at the same time. On
- * such a configuration, virtio has never worked and will
- * not work without an even larger kludge. Instead, enable
- * the DMA API if we're a Xen guest, which at least allows
- * all of the sensible Xen configurations to work correctly.
- */
- if (xen_domain())
- return true;
-
- return false;
-}
XEN guests would not like override with virtio_direct_dma_ops in any
case irrespective of the flag VIRTIO_F_IOMMU_PLATFORM. So the existing
semantics can be preserved with something like this. It just assumes
that dev->dma_ops is non-NULL and a valid one set by the architecture.
If required we can add those tests here before skipping the override.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-07-28 21:16:28
On Sat, Jul 28, 2018 at 02:26:24PM +0530, Anshuman Khandual wrote:
quoted hunk
On 07/20/2018 09:29 AM, Anshuman Khandual wrote:
quoted
Now that virtio core always needs all virtio devices to have DMA OPS, we
need to make sure that the structure it points is the right one. In the
absence of VIRTIO_F_IOMMU_PLATFORM flag QEMU expects GPA from guest kernel.
In such case, virtio device must use default virtio_direct_dma_ops DMA OPS
structure which transforms scatter gather buffer addresses as GPA. This
DMA OPS override must happen as early as possible during virtio device
initializatin sequence before virtio core starts using given device's DMA
OPS callbacks for I/O transactions. This change detects device's IOMMU flag
and does the override in case the flag is cleared.
Signed-off-by: Anshuman Khandual <redacted>
---
drivers/virtio/virtio.c | 5 +++++
1 file changed, 5 insertions(+)
@@ -174,6 +176,9 @@ int virtio_finalize_features(struct virtio_device *dev)if(ret)returnret;
The previous patch removed the code block for XEN guests which forced
the use of DMA API all the time irrespective of VIRTIO_F_IOMMU_PLATFORM
flag on the device. Here is what I have removed with patch 2/4 which
breaks the existing semantics on XEN guests.
-static bool vring_use_dma_api(struct virtio_device *vdev)
-{
- if (!virtio_has_iommu_quirk(vdev))
- return true;
-
- /* Otherwise, we are left to guess. */
- /*
- * In theory, it's possible to have a buggy QEMU-supposed
- * emulated Q35 IOMMU and Xen enabled at the same time. On
- * such a configuration, virtio has never worked and will
- * not work without an even larger kludge. Instead, enable
- * the DMA API if we're a Xen guest, which at least allows
- * all of the sensible Xen configurations to work correctly.
- */
- if (xen_domain())
- return true;
-
- return false;
-}
XEN guests would not like override with virtio_direct_dma_ops in any
case irrespective of the flag VIRTIO_F_IOMMU_PLATFORM. So the existing
semantics can be preserved with something like this. It just assumes
that dev->dma_ops is non-NULL and a valid one set by the architecture.
If required we can add those tests here before skipping the override.
On Sat, Jul 28, 2018 at 02:26:24PM +0530, Anshuman Khandual wrote:
quoted
On 07/20/2018 09:29 AM, Anshuman Khandual wrote:
quoted
Now that virtio core always needs all virtio devices to have DMA OPS, we
need to make sure that the structure it points is the right one. In the
absence of VIRTIO_F_IOMMU_PLATFORM flag QEMU expects GPA from guest kernel.
In such case, virtio device must use default virtio_direct_dma_ops DMA OPS
structure which transforms scatter gather buffer addresses as GPA. This
DMA OPS override must happen as early as possible during virtio device
initializatin sequence before virtio core starts using given device's DMA
OPS callbacks for I/O transactions. This change detects device's IOMMU flag
and does the override in case the flag is cleared.
Signed-off-by: Anshuman Khandual <redacted>
---
drivers/virtio/virtio.c | 5 +++++
1 file changed, 5 insertions(+)
@@ -174,6 +176,9 @@ int virtio_finalize_features(struct virtio_device *dev)if(ret)returnret;
The previous patch removed the code block for XEN guests which forced
the use of DMA API all the time irrespective of VIRTIO_F_IOMMU_PLATFORM
flag on the device. Here is what I have removed with patch 2/4 which
breaks the existing semantics on XEN guests.
-static bool vring_use_dma_api(struct virtio_device *vdev)
-{
- if (!virtio_has_iommu_quirk(vdev))
- return true;
-
- /* Otherwise, we are left to guess. */
- /*
- * In theory, it's possible to have a buggy QEMU-supposed
- * emulated Q35 IOMMU and Xen enabled at the same time. On
- * such a configuration, virtio has never worked and will
- * not work without an even larger kludge. Instead, enable
- * the DMA API if we're a Xen guest, which at least allows
- * all of the sensible Xen configurations to work correctly.
- */
- if (xen_domain())
- return true;
-
- return false;
-}
XEN guests would not like override with virtio_direct_dma_ops in any
case irrespective of the flag VIRTIO_F_IOMMU_PLATFORM. So the existing
semantics can be preserved with something like this. It just assumes
that dev->dma_ops is non-NULL and a valid one set by the architecture.
If required we can add those tests here before skipping the override.
From: Christoph Hellwig <hch@infradead.org> Date: 2018-07-30 09:24:25
+/*
+ * Virtio direct mapping DMA API operations structure
+ *
+ * This defines DMA API structure for all virtio devices which would not
+ * either bring in their own DMA OPS from architecture or they would not
+ * like to use architecture specific IOMMU based DMA OPS because QEMU
+ * expects GPA instead of an IOVA in absence of VIRTIO_F_IOMMU_PLATFORM.
+ */
+dma_addr_t virtio_direct_map_page(struct device *dev, struct page *page,
+ unsigned long offset, size_t size,
+ enum dma_data_direction dir,
+ unsigned long attrs)
All these functions should probably be marked static.
This is missing a dma_map_sg implementation. In general this is
mandatory for dma_ops. So either you implement it or explain in
a common why you think you can skip it.
+EXPORT_SYMBOL(virtio_direct_dma_ops);
EXPORT_SYMBOL_GPL like all virtio symbols, please.
From: Christoph Hellwig <hch@infradead.org> Date: 2018-07-30 09:25:55
+const struct dma_map_ops virtio_direct_dma_ops;
This belongs into a header if it is non-static. If you only
use it in this file anyway please mark it static and avoid a forward
declaration.
quoted hunk
+
int virtio_finalize_features(struct virtio_device *dev)
{
int ret = dev->config->finalize_features(dev);
@@ -174,6 +176,9 @@ int virtio_finalize_features(struct virtio_device *dev) if (ret) return ret;+ if (virtio_has_iommu_quirk(dev))+ set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
This needs a big fat comment explaining what is going on here.
Also not new, but I find the existance of virtio_has_iommu_quirk and its
name horribly confusing. It might be better to open code it here once
only a single caller is left.
From: Christoph Hellwig <hch@infradead.org> Date: 2018-07-30 09:30:32
quoted
+
+ if (xen_domain())
+ goto skip_override;
+
+ if (virtio_has_iommu_quirk(dev))
+ set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
+
+ skip_override:
+
I prefer normal if scoping as opposed to goto spaghetti pls.
Better yet move vring_use_dma_api here and use it.
Less of a chance something will break.
I agree about avoid pointless gotos here, but we can do things
perfectly well without either gotos or a confusing helper here
if we structure it right. E.g.:
// suitably detailed comment here
if (!xen_domain() &&
!virtio_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM))
set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
and while we're at it - modifying dma ops for the parent looks very
dangerous. I don't think we can do that, as it could break iommu
setup interactions. IFF we set a specific dma map ops it has to be
on the virtio device itself, of which we have full control.
From: Christoph Hellwig <hch@infradead.org> Date: 2018-07-30 09:34:19
On Fri, Jul 27, 2018 at 10:58:05AM +0100, Will Deacon wrote:
I just wanted to say that this patch series provides a means for us to
force the coherent DMA ops for legacy virtio devices on arm64, which in turn
means that we can enable the SMMU with legacy devices in our fastmodel
emulation platform (which is slowly being upgraded to virtio 1.0) without
hanging during boot. Patch below.
Yikes, this is a nightmare. That is exactly where I do not want things
to end up. We really need to distinguish between legacy virtual crappy
virtio (and that includes v1) that totally ignores the bus it pretends
to be on, and sane virtio (to be defined) that sit on a real (or
properly emulated including iommu and details for dma mapping) bus.
Having a mumble jumble of arch specific undocumented magic as in
the powerpc patch replied to or this arm patch is a complete no-go.
Nacked-by: Christoph Hellwig [off-list ref]
for both.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-07-30 10:28:13
On Mon, Jul 30, 2018 at 02:34:14AM -0700, Christoph Hellwig wrote:
We really need to distinguish between legacy virtual crappy
virtio (and that includes v1) that totally ignores the bus it pretends
to be on, and sane virtio (to be defined) that sit on a real (or
properly emulated including iommu and details for dma mapping) bus.
Let me reply to the "crappy" part first:
So virtio devices can run on another CPU or on a PCI bus. Configuration
can happen over mupltiple transports. There is a discovery protocol to
figure out where it is. It has some warts but any real system has warts.
So IMHO virtio running on another CPU isn't "legacy virtual crappy
virtio". virtio devices that actually sit on a PCI bus aren't "sane"
simply because the DMA is more convoluted on some architectures.
Performance impact of the optimizations possible when you know
your "device" is in fact just another CPU has been measured,
it is real, so we aren't interested in adding all that overhead back
just so we can use DMA API. The "correct then fast" mantra doesn't
apply to something that is as widely deployed as virtio.
And I can accept an argument that maybe the DMA API isn't designed to
support such virtual DMA. Whether it should I don't know.
With this out of my system:
I agree these approaches are hacky. I think it is generally better to
have virtio feature negotiation tell you whether device runs on a CPU or
not rather than rely on platform specific ways for this. To this end
there was a recent proposal to rename VIRTIO_F_IO_BARRIER to
VIRTIO_F_REAL_DEVICE. It got stuck since "real" sounds vague to people,
e.g. what if it's a VF - is that real or not? But I can see something
like e.g. VIRTIO_F_PLATFORM_DMA gaining support.
We would then rename virtio_has_iommu_quirk to virtio_has_dma_quirk
and test VIRTIO_F_PLATFORM_DMA in addition to the IOMMU thing.
--
MST
From: Christoph Hellwig <hch@infradead.org> Date: 2018-07-30 11:18:09
On Mon, Jul 30, 2018 at 01:28:03PM +0300, Michael S. Tsirkin wrote:
Let me reply to the "crappy" part first:
So virtio devices can run on another CPU or on a PCI bus. Configuration
can happen over mupltiple transports. There is a discovery protocol to
figure out where it is. It has some warts but any real system has warts.
So IMHO virtio running on another CPU isn't "legacy virtual crappy
virtio". virtio devices that actually sit on a PCI bus aren't "sane"
simply because the DMA is more convoluted on some architectures.
All of what you said would be true if virtio didn't claim to be
a PCI device. Once it claims to be a PCI device and we also see
real hardware written to the interface I stand to all what I said
above.
With this out of my system:
I agree these approaches are hacky. I think it is generally better to
have virtio feature negotiation tell you whether device runs on a CPU or
not rather than rely on platform specific ways for this. To this end
there was a recent proposal to rename VIRTIO_F_IO_BARRIER to
VIRTIO_F_REAL_DEVICE. It got stuck since "real" sounds vague to people,
e.g. what if it's a VF - is that real or not? But I can see something
like e.g. VIRTIO_F_PLATFORM_DMA gaining support.
We would then rename virtio_has_iommu_quirk to virtio_has_dma_quirk
and test VIRTIO_F_PLATFORM_DMA in addition to the IOMMU thing.
I don't really care about the exact naming, and indeed a device that
sets the flag doesn't have to be a 'real' device - it just has to act
like one. I explained all the issues that this means (at least relating
to DMA) in one of the previous threads.
The important bit is that we can specify exact behavior for both
devices that sets the "I'm real!" flag and that ones that don't exactly
in the spec. And that very much excludes arch-specific (or
Xen-specific) overrides.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-07-30 13:26:43
On Mon, Jul 30, 2018 at 04:18:02AM -0700, Christoph Hellwig wrote:
On Mon, Jul 30, 2018 at 01:28:03PM +0300, Michael S. Tsirkin wrote:
quoted
Let me reply to the "crappy" part first:
So virtio devices can run on another CPU or on a PCI bus. Configuration
can happen over mupltiple transports. There is a discovery protocol to
figure out where it is. It has some warts but any real system has warts.
So IMHO virtio running on another CPU isn't "legacy virtual crappy
virtio". virtio devices that actually sit on a PCI bus aren't "sane"
simply because the DMA is more convoluted on some architectures.
All of what you said would be true if virtio didn't claim to be
a PCI device.
There's nothing virtio claims to be. It's a PV device that uses PCI for
its configuration. Configuration is enumerated on the virtual PCI bus.
That part of the interface is emulated PCI. Data path is through a
PV device enumerated on the virtio bus.
Once it claims to be a PCI device and we also see
real hardware written to the interface I stand to all what I said
above.
Real hardware would reuse parts of the interface but by necessity it
needs to behave slightly differently on some platforms. However for
some platforms (such as x86) a PV virtio driver will by luck work with a
PCI device backend without changes. As these platforms and drivers are
widely deployed, some people will deploy hardware like that. Should be
a non issue as by definition it's transparent to guests.
quoted
With this out of my system:
I agree these approaches are hacky. I think it is generally better to
have virtio feature negotiation tell you whether device runs on a CPU or
not rather than rely on platform specific ways for this. To this end
there was a recent proposal to rename VIRTIO_F_IO_BARRIER to
VIRTIO_F_REAL_DEVICE. It got stuck since "real" sounds vague to people,
e.g. what if it's a VF - is that real or not? But I can see something
like e.g. VIRTIO_F_PLATFORM_DMA gaining support.
We would then rename virtio_has_iommu_quirk to virtio_has_dma_quirk
and test VIRTIO_F_PLATFORM_DMA in addition to the IOMMU thing.
I don't really care about the exact naming, and indeed a device that
sets the flag doesn't have to be a 'real' device - it just has to act
like one. I explained all the issues that this means (at least relating
to DMA) in one of the previous threads.
I believe you refer to this:
https://lkml.org/lkml/2018/6/7/15
that was a very helpful list outlining the problems we need to solve,
thanks a lot for that!
The important bit is that we can specify exact behavior for both
devices that sets the "I'm real!" flag and that ones that don't exactly
in the spec.
I would very much like that, yes.
And that very much excludes arch-specific (or
Xen-specific) overrides.
We already committed to a xen specific hack but generally I prefer
devices that describe how they work instead of platforms magically
guessing, yes.
However the question people raise is that DMA API is already full of
arch-specific tricks the likes of which are outlined in your post linked
above. How is this one much worse?
--
MST
+/*
+ * Virtio direct mapping DMA API operations structure
+ *
+ * This defines DMA API structure for all virtio devices which would not
+ * either bring in their own DMA OPS from architecture or they would not
+ * like to use architecture specific IOMMU based DMA OPS because QEMU
+ * expects GPA instead of an IOVA in absence of VIRTIO_F_IOMMU_PLATFORM.
+ */
+dma_addr_t virtio_direct_map_page(struct device *dev, struct page *page,
+ unsigned long offset, size_t size,
+ enum dma_data_direction dir,
+ unsigned long attrs)
All these functions should probably be marked static.
This is missing a dma_map_sg implementation. In general this is
mandatory for dma_ops. So either you implement it or explain in
a common why you think you can skip it.
Hmm. IIUC virtio core never used dma_map_sg(). Am I missing something
here ? The only reference to dma_map_sg() is inside a comment.
$git grep dma_map_sg drivers/virtio/
drivers/virtio/virtio_ring.c: * We can't use dma_map_sg, because we don't use scatterlists in
quoted
+EXPORT_SYMBOL(virtio_direct_dma_ops);
EXPORT_SYMBOL_GPL like all virtio symbols, please.
I am planning to drop EXPORT_SYMBOL from virtio_direct_dma_ops structure.
+
+ if (xen_domain())
+ goto skip_override;
+
+ if (virtio_has_iommu_quirk(dev))
+ set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
+
+ skip_override:
+
I prefer normal if scoping as opposed to goto spaghetti pls.
Better yet move vring_use_dma_api here and use it.
Less of a chance something will break.
I agree about avoid pointless gotos here, but we can do things
perfectly well without either gotos or a confusing helper here
if we structure it right. E.g.:
// suitably detailed comment here
if (!xen_domain() &&
!virtio_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM))
set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
I had updated this patch calling vring_use_dma_api() as a helper
as suggested by Michael but yes we can have the above condition
with a comment block. I will change this patch accordingly.
and while we're at it - modifying dma ops for the parent looks very
dangerous. I don't think we can do that, as it could break iommu
setup interactions. IFF we set a specific dma map ops it has to be
on the virtio device itself, of which we have full control.
I understand your concern. At present virtio core calls parent's DMA
ops callbacks when device has VIRTIO_F_IOMMU_PLATFORM flag set. Most
likely those DMA OPS are architecture specific ones which can really
configure IOMMU. Most probably all devices and their parents share
the same DMA ops callback. IIUC as long as the entire system has a
single DMA ops structure, it should be okay. But I may be missing
other implications. I tried changing virtio core so that it always
calls device's DMA ops instead of it's parent DMA ops, it hit the
following WARN_ON for devices without IOMMU flag and hit both the
WARN_ON and BUG_ON for devices with the IOMMU flag.
static inline void *dma_alloc_attrs(struct device *dev, size_t size,
dma_addr_t *dma_handle, gfp_t flag,
unsigned long attrs)
{
const struct dma_map_ops *ops = get_dma_ops(dev);
void *cpu_addr;
BUG_ON(!ops);
WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
--------
Seems like virtio device's DMA ops and coherent_dma_mask was never
set correctly assuming that virtio core always called parent's DMA
OPS all the time. We may have to change virtio device init to fix
this. Any thoughts ?
This belongs into a header if it is non-static. If you only
use it in this file anyway please mark it static and avoid a forward
declaration.
Sure, will make it static, move the definition up in the file to avoid
forward declaration.
quoted
+
int virtio_finalize_features(struct virtio_device *dev)
{
int ret = dev->config->finalize_features(dev);
@@ -174,6 +176,9 @@ int virtio_finalize_features(struct virtio_device *dev) if (ret) return ret;+ if (virtio_has_iommu_quirk(dev))+ set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
This needs a big fat comment explaining what is going on here.
Sure, will do. Also talk about the XEN domain exception as well once
that goes into this conditional statement.
Also not new, but I find the existance of virtio_has_iommu_quirk and its
name horribly confusing. It might be better to open code it here once
only a single caller is left.
Sure will do. There is one definition in the tools directory which can
be removed and then this will be the only one left.
From: Christoph Hellwig <hch@infradead.org> Date: 2018-07-31 17:31:04
On Mon, Jul 30, 2018 at 04:26:32PM +0300, Michael S. Tsirkin wrote:
Real hardware would reuse parts of the interface but by necessity it
needs to behave slightly differently on some platforms. However for
some platforms (such as x86) a PV virtio driver will by luck work with a
PCI device backend without changes. As these platforms and drivers are
widely deployed, some people will deploy hardware like that. Should be
a non issue as by definition it's transparent to guests.
On some x86. As soon as you have an iommu or strange PCI root ports
things are going to start breaking apart.
quoted
And that very much excludes arch-specific (or
Xen-specific) overrides.
We already committed to a xen specific hack but generally I prefer
devices that describe how they work instead of platforms magically
guessing, yes.
For legacy reasons I guess we'll have to keep it, but we really need
to avoid adding more junk than this.
However the question people raise is that DMA API is already full of
arch-specific tricks the likes of which are outlined in your post linked
above. How is this one much worse?
None of these warts is visible to the driver, they are all handled in
the architecture (possibly on a per-bus basis).
So for virtio we really need to decide if it has one set of behavior
as specified in the virtio spec, or if it behaves exactly as if it
was on a PCI bus, or in fact probably both as you lined up. But no
magic arch specific behavior inbetween.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-07-31 20:37:37
On Tue, 2018-07-31 at 10:30 -0700, Christoph Hellwig wrote:
quoted
However the question people raise is that DMA API is already full of
arch-specific tricks the likes of which are outlined in your post linked
above. How is this one much worse?
None of these warts is visible to the driver, they are all handled in
the architecture (possibly on a per-bus basis).
So for virtio we really need to decide if it has one set of behavior
as specified in the virtio spec, or if it behaves exactly as if it
was on a PCI bus, or in fact probably both as you lined up. But no
magic arch specific behavior inbetween.
The only arch specific behaviour is needed in the case where it doesn't
behave like PCI. In this case, the PCI DMA ops are not suitable, but in
our secure VMs, we still need to make it use swiotlb in order to bounce
through non-secure pages.
It would be nice if "real PCI" was the default but it's not, VMs are
created in "legacy" mode all the times and we don't know at VM creation
time whether it will become a secure VM or not. The way our secure VMs
work is that they start as a normal VM, load a secure "payload" and
call the Ultravisor to "become" secure.
So we're in a bit of a bind here. We need that one-liner optional arch
hook to make virtio use swiotlb in that "IOMMU bypass" case.
Ben.
From: Will Deacon <hidden> Date: 2018-08-01 08:16:39
On Tue, Jul 31, 2018 at 03:36:22PM -0500, Benjamin Herrenschmidt wrote:
On Tue, 2018-07-31 at 10:30 -0700, Christoph Hellwig wrote:
quoted
quoted
However the question people raise is that DMA API is already full of
arch-specific tricks the likes of which are outlined in your post linked
above. How is this one much worse?
None of these warts is visible to the driver, they are all handled in
the architecture (possibly on a per-bus basis).
So for virtio we really need to decide if it has one set of behavior
as specified in the virtio spec, or if it behaves exactly as if it
was on a PCI bus, or in fact probably both as you lined up. But no
magic arch specific behavior inbetween.
The only arch specific behaviour is needed in the case where it doesn't
behave like PCI. In this case, the PCI DMA ops are not suitable, but in
our secure VMs, we still need to make it use swiotlb in order to bounce
through non-secure pages.
On arm/arm64, the problem we have is that legacy virtio devices on the MMIO
transport (so definitely not PCI) have historically been advertised by qemu
as not being cache coherent, but because the virtio core has bypassed DMA
ops then everything has happened to work. If we blindly enable the arch DMA
ops, we'll plumb in the non-coherent ops and start getting data corruption,
so we do need a way to quirk virtio as being "always coherent" if we want to
use the DMA ops (which we do, because our emulation platforms have an IOMMU
for all virtio devices).
Will
From: Christoph Hellwig <hch@infradead.org> Date: 2018-08-01 08:36:49
On Wed, Aug 01, 2018 at 09:16:38AM +0100, Will Deacon wrote:
On arm/arm64, the problem we have is that legacy virtio devices on the MMIO
transport (so definitely not PCI) have historically been advertised by qemu
as not being cache coherent, but because the virtio core has bypassed DMA
ops then everything has happened to work. If we blindly enable the arch DMA
ops,
No one is suggesting that as far as I can tell.
we'll plumb in the non-coherent ops and start getting data corruption,
so we do need a way to quirk virtio as being "always coherent" if we want to
use the DMA ops (which we do, because our emulation platforms have an IOMMU
for all virtio devices).
From all that I've gather so far: no you do not want that. We really
need to figure out virtio "dma" interacts with the host / device.
If you look at the current iommu spec it does talk of physical address
with a little careveout for VIRTIO_F_IOMMU_PLATFORM.
So between that and our discussion in this thread and its previous
iterations I think we need to stick to the current always physical,
bypass system dma ops mode of virtio operation as the default.
We just need to figure out how to deal with devices that deviate
from the default. One things is that VIRTIO_F_IOMMU_PLATFORM really
should become VIRTIO_F_PLATFORM_DMA to cover the cases of non-iommu
dma tweaks (offsets, cache flushing), which seems well in spirit of
the original design. The other issue is VIRTIO_F_IO_BARRIER
which is very vaguely defined, and which needs a better definition.
And last but not least we'll need some text explaining the challenges
of hardware devices - I think VIRTIO_F_PLATFORM_DMA + VIRTIO_F_IO_BARRIER
is what would basically cover them, but a good description including
an explanation of why these matter.
From: Will Deacon <hidden> Date: 2018-08-01 09:05:36
Hi Christoph,
On Wed, Aug 01, 2018 at 01:36:39AM -0700, Christoph Hellwig wrote:
On Wed, Aug 01, 2018 at 09:16:38AM +0100, Will Deacon wrote:
quoted
On arm/arm64, the problem we have is that legacy virtio devices on the MMIO
transport (so definitely not PCI) have historically been advertised by qemu
as not being cache coherent, but because the virtio core has bypassed DMA
ops then everything has happened to work. If we blindly enable the arch DMA
ops,
No one is suggesting that as far as I can tell.
Apologies: it's me that wants the DMA ops enabled to handle legacy devices
behind an IOMMU, but see below.
quoted
we'll plumb in the non-coherent ops and start getting data corruption,
so we do need a way to quirk virtio as being "always coherent" if we want to
use the DMA ops (which we do, because our emulation platforms have an IOMMU
for all virtio devices).
From all that I've gather so far: no you do not want that. We really
need to figure out virtio "dma" interacts with the host / device.
If you look at the current iommu spec it does talk of physical address
with a little careveout for VIRTIO_F_IOMMU_PLATFORM.
That's true, although that doesn't exist in the legacy virtio spec, and we
have an existing emulation platform which puts legacy virtio devices behind
an IOMMU. Currently, Linux is unable to boot on this platform unless the
IOMMU is configured as bypass. If we can use the coherent IOMMU DMA ops,
then it works perfectly.
So between that and our discussion in this thread and its previous
iterations I think we need to stick to the current always physical,
bypass system dma ops mode of virtio operation as the default.
As above -- that means we hang during boot because we get stuck trying to
bring up a virtio-block device whose DMA is aborted by the IOMMU. The easy
answer is "just upgrade to latest virtio and advertise the presence of the
IOMMU". I'm pushing for that in future platforms, but it seems a shame not
to support the current platform, especially given that other systems do have
hacks in mainline to get virtio working.
We just need to figure out how to deal with devices that deviate
from the default. One things is that VIRTIO_F_IOMMU_PLATFORM really
should become VIRTIO_F_PLATFORM_DMA to cover the cases of non-iommu
dma tweaks (offsets, cache flushing), which seems well in spirit of
the original design. The other issue is VIRTIO_F_IO_BARRIER
which is very vaguely defined, and which needs a better definition.
And last but not least we'll need some text explaining the challenges
of hardware devices - I think VIRTIO_F_PLATFORM_DMA + VIRTIO_F_IO_BARRIER
is what would basically cover them, but a good description including
an explanation of why these matter.
I agree that this makes sense for future revisions of virtio (or perhaps
it can just be a clarification to virtio 1.0), but we're still left in the
dark with legacy devices and it would be nice to have them work on the
systems which currently exist, even if it's a legacy-only hack in the arch
code.
Will
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-01 21:56:22
On Tue, Jul 31, 2018 at 03:36:22PM -0500, Benjamin Herrenschmidt wrote:
On Tue, 2018-07-31 at 10:30 -0700, Christoph Hellwig wrote:
quoted
quoted
However the question people raise is that DMA API is already full of
arch-specific tricks the likes of which are outlined in your post linked
above. How is this one much worse?
None of these warts is visible to the driver, they are all handled in
the architecture (possibly on a per-bus basis).
So for virtio we really need to decide if it has one set of behavior
as specified in the virtio spec, or if it behaves exactly as if it
was on a PCI bus, or in fact probably both as you lined up. But no
magic arch specific behavior inbetween.
The only arch specific behaviour is needed in the case where it doesn't
behave like PCI. In this case, the PCI DMA ops are not suitable, but in
our secure VMs, we still need to make it use swiotlb in order to bounce
through non-secure pages.
It would be nice if "real PCI" was the default
I think you are mixing "real PCI" which isn't coded up yet and IOMMU
bypass which is. IOMMU bypass will maybe with time become unnecessary
since it seems that one can just program an IOMMU in a bypass mode
instead.
It's hard to blame you since right now if you disable IOMMU bypass
you get a real PCI mode. But they are distinct and to allow people
to enable IOMMU by default we will need to teach someone
(virtio or DMA API) about this mode that does follow
translation and protection rules in the IOMMU but runs
on a CPU and so does not need cache flushes and whatnot.
OTOH real PCI mode as opposed to default hypervisor mode does not perform as
well when what you actually have is a hypervisor.
So we'll likely have a mix of these two modes for a while.
but it's not, VMs are
created in "legacy" mode all the times and we don't know at VM creation
time whether it will become a secure VM or not. The way our secure VMs
work is that they start as a normal VM, load a secure "payload" and
call the Ultravisor to "become" secure.
So we're in a bit of a bind here. We need that one-liner optional arch
hook to make virtio use swiotlb in that "IOMMU bypass" case.
Ben.
And just to make sure I understand, on your platform DMA APIs do include
some of the cache flushing tricks and this is why you don't want to
declare iommu support in the hypervisor?
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-01 22:35:35
On Wed, Aug 01, 2018 at 01:36:39AM -0700, Christoph Hellwig wrote:
On Wed, Aug 01, 2018 at 09:16:38AM +0100, Will Deacon wrote:
quoted
On arm/arm64, the problem we have is that legacy virtio devices on the MMIO
transport (so definitely not PCI) have historically been advertised by qemu
as not being cache coherent, but because the virtio core has bypassed DMA
ops then everything has happened to work. If we blindly enable the arch DMA
ops,
No one is suggesting that as far as I can tell.
quoted
we'll plumb in the non-coherent ops and start getting data corruption,
so we do need a way to quirk virtio as being "always coherent" if we want to
use the DMA ops (which we do, because our emulation platforms have an IOMMU
for all virtio devices).
quoted
From all that I've gather so far: no you do not want that. We really
need to figure out virtio "dma" interacts with the host / device.
If you look at the current iommu spec it does talk of physical address
with a little careveout for VIRTIO_F_IOMMU_PLATFORM.
So between that and our discussion in this thread and its previous
iterations I think we need to stick to the current always physical,
bypass system dma ops mode of virtio operation as the default.
We just need to figure out how to deal with devices that deviate
from the default. One things is that VIRTIO_F_IOMMU_PLATFORM really
should become VIRTIO_F_PLATFORM_DMA to cover the cases of non-iommu
dma tweaks (offsets, cache flushing), which seems well in spirit of
the original design.
Well I wouldn't say that. VIRTIO_F_IOMMU_PLATFORM is for guest
programmable protection which is designed for things like userspace
drivers but still very much which a CPU doing the accesses. I think
VIRTIO_F_IO_BARRIER needs to be extended to VIRTIO_F_PLATFORM_DMA.
The other issue is VIRTIO_F_IO_BARRIER
which is very vaguely defined, and which needs a better definition.
And last but not least we'll need some text explaining the challenges
of hardware devices - I think VIRTIO_F_PLATFORM_DMA + VIRTIO_F_IO_BARRIER
is what would basically cover them, but a good description including
an explanation of why these matter.
I think VIRTIO_F_IOMMU_PLATFORM + VIRTIO_F_PLATFORM_DMA but yea.
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-01 22:41:28
On Wed, Aug 01, 2018 at 10:05:35AM +0100, Will Deacon wrote:
Hi Christoph,
On Wed, Aug 01, 2018 at 01:36:39AM -0700, Christoph Hellwig wrote:
quoted
On Wed, Aug 01, 2018 at 09:16:38AM +0100, Will Deacon wrote:
quoted
On arm/arm64, the problem we have is that legacy virtio devices on the MMIO
transport (so definitely not PCI) have historically been advertised by qemu
as not being cache coherent, but because the virtio core has bypassed DMA
ops then everything has happened to work. If we blindly enable the arch DMA
ops,
No one is suggesting that as far as I can tell.
Apologies: it's me that wants the DMA ops enabled to handle legacy devices
behind an IOMMU, but see below.
quoted
quoted
we'll plumb in the non-coherent ops and start getting data corruption,
so we do need a way to quirk virtio as being "always coherent" if we want to
use the DMA ops (which we do, because our emulation platforms have an IOMMU
for all virtio devices).
From all that I've gather so far: no you do not want that. We really
need to figure out virtio "dma" interacts with the host / device.
If you look at the current iommu spec it does talk of physical address
with a little careveout for VIRTIO_F_IOMMU_PLATFORM.
That's true, although that doesn't exist in the legacy virtio spec, and we
have an existing emulation platform which puts legacy virtio devices behind
an IOMMU. Currently, Linux is unable to boot on this platform unless the
IOMMU is configured as bypass. If we can use the coherent IOMMU DMA ops,
then it works perfectly.
quoted
So between that and our discussion in this thread and its previous
iterations I think we need to stick to the current always physical,
bypass system dma ops mode of virtio operation as the default.
As above -- that means we hang during boot because we get stuck trying to
bring up a virtio-block device whose DMA is aborted by the IOMMU. The easy
answer is "just upgrade to latest virtio and advertise the presence of the
IOMMU". I'm pushing for that in future platforms, but it seems a shame not
to support the current platform, especially given that other systems do have
hacks in mainline to get virtio working.
quoted
We just need to figure out how to deal with devices that deviate
from the default. One things is that VIRTIO_F_IOMMU_PLATFORM really
should become VIRTIO_F_PLATFORM_DMA to cover the cases of non-iommu
dma tweaks (offsets, cache flushing), which seems well in spirit of
the original design. The other issue is VIRTIO_F_IO_BARRIER
which is very vaguely defined, and which needs a better definition.
And last but not least we'll need some text explaining the challenges
of hardware devices - I think VIRTIO_F_PLATFORM_DMA + VIRTIO_F_IO_BARRIER
is what would basically cover them, but a good description including
an explanation of why these matter.
I agree that this makes sense for future revisions of virtio (or perhaps
it can just be a clarification to virtio 1.0), but we're still left in the
dark with legacy devices and it would be nice to have them work on the
systems which currently exist, even if it's a legacy-only hack in the arch
code.
Will
Myself I'm sympathetic to this use-case and I see more uses to this
than just legacy support. But more work is required IMHO.
Will post tomorrow though - it's late here ...
--
MST
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-02 15:26:29
On Wed, 2018-08-01 at 01:36 -0700, Christoph Hellwig wrote:
We just need to figure out how to deal with devices that deviate
from the default. One things is that VIRTIO_F_IOMMU_PLATFORM really
should become VIRTIO_F_PLATFORM_DMA to cover the cases of non-iommu
dma tweaks (offsets, cache flushing), which seems well in spirit of
the original design.
I don't completely agree:
1 - VIRTIO_F_IOMMU_PLATFORM is a property of the "other side", ie qemu
for example. It indicates that the peer bypasses the normal platform
iommu. The platform code in the guest has no real way to know that this
is the case, this is a specific "feature" of the qemu implementation.
2 - VIRTIO_F_PLATFORM_DMA (or whatever you want to call it), is a
property of the guest platform itself (not qemu), there's no way the
"peer" can advertize it via the virtio negociated flags. At least for
us. I don't know for sure whether that would be workable for the ARM
case. In our case, qemu has no idea at VM creation time that the VM
will turn itself into a secure VM and thus will require bounce
buffering for IOs (including virtio).
So unless we have another hook for the arch code to set
VIRTIO_F_PLATFORM_DMA on selected (or all) virtio devices from the
guest itself, I don't see that as a way to deal with it.
The other issue is VIRTIO_F_IO_BARRIER
which is very vaguely defined, and which needs a better definition.
And last but not least we'll need some text explaining the challenges
of hardware devices - I think VIRTIO_F_PLATFORM_DMA + VIRTIO_F_IO_BARRIER
is what would basically cover them, but a good description including
an explanation of why these matter.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-02 15:34:37
On Thu, 2018-08-02 at 00:56 +0300, Michael S. Tsirkin wrote:
quoted
but it's not, VMs are
created in "legacy" mode all the times and we don't know at VM creation
time whether it will become a secure VM or not. The way our secure VMs
work is that they start as a normal VM, load a secure "payload" and
call the Ultravisor to "become" secure.
So we're in a bit of a bind here. We need that one-liner optional arch
hook to make virtio use swiotlb in that "IOMMU bypass" case.
Ben.
And just to make sure I understand, on your platform DMA APIs do include
some of the cache flushing tricks and this is why you don't want to
declare iommu support in the hypervisor?
I'm not sure I parse what you mean.
We don't need cache flushing tricks. The problem we have with our
"secure" VMs is that:
- At VM creation time we have no idea it's going to become a secure
VM, qemu doesn't know anything about it, and thus qemu (or other
management tools, libvirt etc...) are going to create "legacy" (ie
iommu bypass) virtio devices.
- Once the VM goes secure (early during boot but too late for qemu),
it will need to make virtio do bounce-buffering via swiotlb because
qemu cannot physically access most VM pages (blocked by HW security
features), we need to bounce buffer using some unsecure pages that are
accessible to qemu.
That said, I wouldn't object for us to more generally switch long run
to changing qemu so that virtio on powerpc starts using the IOMMU as a
default provided we fix our guest firmware to understand it (it
currently doesn't), and provided we verify that the performance impact
on things like vhost is negligible.
Cheers,
Ben.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-02 15:41:27
On Thu, Aug 02, 2018 at 10:24:57AM -0500, Benjamin Herrenschmidt wrote:
On Wed, 2018-08-01 at 01:36 -0700, Christoph Hellwig wrote:
quoted
We just need to figure out how to deal with devices that deviate
from the default. One things is that VIRTIO_F_IOMMU_PLATFORM really
should become VIRTIO_F_PLATFORM_DMA to cover the cases of non-iommu
dma tweaks (offsets, cache flushing), which seems well in spirit of
the original design.
I don't completely agree:
1 - VIRTIO_F_IOMMU_PLATFORM is a property of the "other side", ie qemu
for example. It indicates that the peer bypasses the normal platform
iommu. The platform code in the guest has no real way to know that this
is the case, this is a specific "feature" of the qemu implementation.
2 - VIRTIO_F_PLATFORM_DMA (or whatever you want to call it), is a
property of the guest platform itself (not qemu), there's no way the
"peer" can advertize it via the virtio negociated flags. At least for
us. I don't know for sure whether that would be workable for the ARM
case. In our case, qemu has no idea at VM creation time that the VM
will turn itself into a secure VM and thus will require bounce
buffering for IOs (including virtio).
So unless we have another hook for the arch code to set
VIRTIO_F_PLATFORM_DMA on selected (or all) virtio devices from the
guest itself, I don't see that as a way to deal with it.
quoted
The other issue is VIRTIO_F_IO_BARRIER
which is very vaguely defined, and which needs a better definition.
And last but not least we'll need some text explaining the challenges
of hardware devices - I think VIRTIO_F_PLATFORM_DMA + VIRTIO_F_IO_BARRIER
is what would basically cover them, but a good description including
an explanation of why these matter.
Ben.
So is it true that from qemu point of view there is nothing special
going on? You pass in a PA, host writes there.
--
MST
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-02 16:24:53
On Thu, 2018-08-02 at 18:41 +0300, Michael S. Tsirkin wrote:
quoted
I don't completely agree:
1 - VIRTIO_F_IOMMU_PLATFORM is a property of the "other side", ie qemu
for example. It indicates that the peer bypasses the normal platform
iommu. The platform code in the guest has no real way to know that this
is the case, this is a specific "feature" of the qemu implementation.
2 - VIRTIO_F_PLATFORM_DMA (or whatever you want to call it), is a
property of the guest platform itself (not qemu), there's no way the
"peer" can advertize it via the virtio negociated flags. At least for
us. I don't know for sure whether that would be workable for the ARM
case. In our case, qemu has no idea at VM creation time that the VM
will turn itself into a secure VM and thus will require bounce
buffering for IOs (including virtio).
So unless we have another hook for the arch code to set
VIRTIO_F_PLATFORM_DMA on selected (or all) virtio devices from the
guest itself, I don't see that as a way to deal with it.
quoted
The other issue is VIRTIO_F_IO_BARRIER
which is very vaguely defined, and which needs a better definition.
And last but not least we'll need some text explaining the challenges
of hardware devices - I think VIRTIO_F_PLATFORM_DMA + VIRTIO_F_IO_BARRIER
is what would basically cover them, but a good description including
an explanation of why these matter.
Ben.
So is it true that from qemu point of view there is nothing special
going on? You pass in a PA, host writes there.
Yes, qemu doesn't see a different. It's the guest that will bounce the
pages via a pool of "insecure" pages that qemu can access. Normal pages
in a secure VM come from PAs that qemu cannot physically access.
Cheers,
Ben.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-02 17:19:51
On Thu, Aug 02, 2018 at 11:01:26AM -0500, Benjamin Herrenschmidt wrote:
On Thu, 2018-08-02 at 18:41 +0300, Michael S. Tsirkin wrote:
quoted
quoted
I don't completely agree:
1 - VIRTIO_F_IOMMU_PLATFORM is a property of the "other side", ie qemu
for example. It indicates that the peer bypasses the normal platform
iommu. The platform code in the guest has no real way to know that this
is the case, this is a specific "feature" of the qemu implementation.
2 - VIRTIO_F_PLATFORM_DMA (or whatever you want to call it), is a
property of the guest platform itself (not qemu), there's no way the
"peer" can advertize it via the virtio negociated flags. At least for
us. I don't know for sure whether that would be workable for the ARM
case. In our case, qemu has no idea at VM creation time that the VM
will turn itself into a secure VM and thus will require bounce
buffering for IOs (including virtio).
So unless we have another hook for the arch code to set
VIRTIO_F_PLATFORM_DMA on selected (or all) virtio devices from the
guest itself, I don't see that as a way to deal with it.
quoted
The other issue is VIRTIO_F_IO_BARRIER
which is very vaguely defined, and which needs a better definition.
And last but not least we'll need some text explaining the challenges
of hardware devices - I think VIRTIO_F_PLATFORM_DMA + VIRTIO_F_IO_BARRIER
is what would basically cover them, but a good description including
an explanation of why these matter.
Ben.
So is it true that from qemu point of view there is nothing special
going on? You pass in a PA, host writes there.
Yes, qemu doesn't see a different. It's the guest that will bounce the
pages via a pool of "insecure" pages that qemu can access. Normal pages
in a secure VM come from PAs that qemu cannot physically access.
Cheers,
Ben.
I see. So yes, given that device does not know or care, using
virtio features is an awkward fit.
So let's say as a quick fix for you maybe we could generalize the
xen_domain hack, instead of just checking xen_domain check some static
branch. Then teach xen and others to enable that.
OK but problem then becomes this: if you do this and virtio device appears
behind a vIOMMU and it does not advertize the IOMMU flag, the
code will try to use the vIOMMU mappings and fail.
It does look like even with trick above, you need a special version of
DMA ops that does just swiotlb but not any of the other things DMA API
might do.
Thoughts?
--
MST
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-02 17:54:59
On Thu, 2018-08-02 at 20:19 +0300, Michael S. Tsirkin wrote:
I see. So yes, given that device does not know or care, using
virtio features is an awkward fit.
So let's say as a quick fix for you maybe we could generalize the
xen_domain hack, instead of just checking xen_domain check some static
branch. Then teach xen and others to enable that.>
OK but problem then becomes this: if you do this and virtio device appears
behind a vIOMMU and it does not advertize the IOMMU flag, the
code will try to use the vIOMMU mappings and fail.
It does look like even with trick above, you need a special version of
DMA ops that does just swiotlb but not any of the other things DMA API
might do.
Thoughts?
Yes, this is the purpose of Anshuman original patch (I haven't looked
at the details of the patch in a while but that's what I told him to
implement ;-) :
- Make virtio always use DMA ops to simplify the code path (with a set
of "transparent" ops for legacy)
and
- Provide an arch hook allowing us to "override" those "transparent"
DMA ops with some custom ones that do the appropriate swiotlb gunk.
Cheers,
Ben.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-02 20:52:21
On Thu, Aug 02, 2018 at 12:53:41PM -0500, Benjamin Herrenschmidt wrote:
On Thu, 2018-08-02 at 20:19 +0300, Michael S. Tsirkin wrote:
quoted
I see. So yes, given that device does not know or care, using
virtio features is an awkward fit.
So let's say as a quick fix for you maybe we could generalize the
xen_domain hack, instead of just checking xen_domain check some static
branch. Then teach xen and others to enable that.>
quoted
OK but problem then becomes this: if you do this and virtio device appears
behind a vIOMMU and it does not advertize the IOMMU flag, the
code will try to use the vIOMMU mappings and fail.
It does look like even with trick above, you need a special version of
DMA ops that does just swiotlb but not any of the other things DMA API
might do.
Thoughts?
Yes, this is the purpose of Anshuman original patch (I haven't looked
at the details of the patch in a while but that's what I told him to
implement ;-) :
- Make virtio always use DMA ops to simplify the code path (with a set
of "transparent" ops for legacy)
and
- Provide an arch hook allowing us to "override" those "transparent"
DMA ops with some custom ones that do the appropriate swiotlb gunk.
Cheers,
Ben.
Right but as I tried to say doing that brings us to a bunch of issues
with using DMA APIs in virtio. Put simply DMA APIs weren't designed for
guest to hypervisor communication.
When we do (as is the case with PLATFORM_IOMMU right now) this adds a
bunch of overhead which we need to get rid of if we are to switch to
PLATFORM_IOMMU by default. We need to fix that.
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-02 20:53:17
On Thu, Aug 02, 2018 at 10:33:05AM -0500, Benjamin Herrenschmidt wrote:
On Thu, 2018-08-02 at 00:56 +0300, Michael S. Tsirkin wrote:
quoted
quoted
but it's not, VMs are
created in "legacy" mode all the times and we don't know at VM creation
time whether it will become a secure VM or not. The way our secure VMs
work is that they start as a normal VM, load a secure "payload" and
call the Ultravisor to "become" secure.
So we're in a bit of a bind here. We need that one-liner optional arch
hook to make virtio use swiotlb in that "IOMMU bypass" case.
Ben.
And just to make sure I understand, on your platform DMA APIs do include
some of the cache flushing tricks and this is why you don't want to
declare iommu support in the hypervisor?
I'm not sure I parse what you mean.
We don't need cache flushing tricks.
You don't but do real devices on same platform need them?
The problem we have with our
"secure" VMs is that:
- At VM creation time we have no idea it's going to become a secure
VM, qemu doesn't know anything about it, and thus qemu (or other
management tools, libvirt etc...) are going to create "legacy" (ie
iommu bypass) virtio devices.
- Once the VM goes secure (early during boot but too late for qemu),
it will need to make virtio do bounce-buffering via swiotlb because
qemu cannot physically access most VM pages (blocked by HW security
features), we need to bounce buffer using some unsecure pages that are
accessible to qemu.
That said, I wouldn't object for us to more generally switch long run
to changing qemu so that virtio on powerpc starts using the IOMMU as a
default provided we fix our guest firmware to understand it (it
currently doesn't), and provided we verify that the performance impact
on things like vhost is negligible.
Cheers,
Ben.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-02 20:55:47
On Fri, Jul 20, 2018 at 09:29:37AM +0530, Anshuman Khandual wrote:
This patch series is the follow up on the discussions we had before about
the RFC titled [RFC,V2] virtio: Add platform specific DMA API translation
for virito devices (https://patchwork.kernel.org/patch/10417371/). There
were suggestions about doing away with two different paths of transactions
with the host/QEMU, first being the direct GPA and the other being the DMA
API based translations.
First patch attempts to create a direct GPA mapping based DMA operations
structure called 'virtio_direct_dma_ops' with exact same implementation
of the direct GPA path which virtio core currently has but just wrapped in
a DMA API format. Virtio core must use 'virtio_direct_dma_ops' instead of
the arch default in absence of VIRTIO_F_IOMMU_PLATFORM flag to preserve the
existing semantics. The second patch does exactly that inside the function
virtio_finalize_features(). The third patch removes the default direct GPA
path from virtio core forcing it to use DMA API callbacks for all devices.
Now with that change, every device must have a DMA operations structure
associated with it. The fourth patch adds an additional hook which gives
the platform an opportunity to do yet another override if required. This
platform hook can be used on POWER Ultravisor based protected guests to
load up SWIOTLB DMA callbacks to do the required (as discussed previously
in the above mentioned thread how host is allowed to access only parts of
the guest GPA range) bounce buffering into the shared memory for all I/O
scatter gather buffers to be consumed on the host side.
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
Jason did some work on profiling this. Unfortunately he reports
about 4% extra overhead from this switch on x86 with no vIOMMU.
I expect he's writing up the data in more detail, but
just wanted to let you know this would be one more
thing to debug before we can just switch to DMA APIs.
Anshuman Khandual (4):
virtio: Define virtio_direct_dma_ops structure
virtio: Override device's DMA OPS with virtio_direct_dma_ops selectively
virtio: Force virtio core to use DMA API callbacks for all virtio devices
virtio: Add platform specific DMA API translation for virito devices
arch/powerpc/include/asm/dma-mapping.h | 6 +++
arch/powerpc/platforms/pseries/iommu.c | 6 +++
drivers/virtio/virtio.c | 72 ++++++++++++++++++++++++++++++++++
drivers/virtio/virtio_pci_common.h | 3 ++
drivers/virtio/virtio_ring.c | 65 +-----------------------------
5 files changed, 89 insertions(+), 63 deletions(-)
--
2.9.3
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-02 21:34:47
On Thu, 2018-08-02 at 23:52 +0300, Michael S. Tsirkin wrote:
quoted
Yes, this is the purpose of Anshuman original patch (I haven't looked
at the details of the patch in a while but that's what I told him to
implement ;-) :
- Make virtio always use DMA ops to simplify the code path (with a set
of "transparent" ops for legacy)
and
- Provide an arch hook allowing us to "override" those "transparent"
DMA ops with some custom ones that do the appropriate swiotlb gunk.
Cheers,
Ben.
Right but as I tried to say doing that brings us to a bunch of issues
with using DMA APIs in virtio. Put simply DMA APIs weren't designed for
guest to hypervisor communication.
I'm not sure I see the problem, see below
When we do (as is the case with PLATFORM_IOMMU right now) this adds a
bunch of overhead which we need to get rid of if we are to switch to
PLATFORM_IOMMU by default. We need to fix that.
So let's differenciate the two problems of having an IOMMU (real or
emulated) which indeeds adds overhead etc... and using the DMA API.
At the moment, virtio does this all over the place:
if (use_dma_api)
dma_map/alloc_something(...)
else
use_pa
The idea of the patch set is to do two, somewhat orthogonal, changes
that together achieve what we want. Let me know where you think there
is "a bunch of issues" because I'm missing it:
1- Replace the above if/else constructs with just calling the DMA API,
and have virtio, at initialization, hookup its own dma_ops that just
"return pa" (roughly) when the IOMMU stuff isn't used.
This adds an indirect function call to the path that previously didn't
have one (the else case above). Is that a significant/measurable
overhead ?
This change stands alone, and imho "cleans" up virtio by avoiding all
that if/else "2 path" and unless it adds a measurable overhead, should
probably be done.
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
This benefits from -1- by making us just plumb in a different set of
DMA ops we would have cooked up specifically for virtio in our arch
code (or in virtio itself but build arch-conditionally in a separate
file). But it doesn't strictly need it -1-:
Now, -2- doesn't strictly needs -1-. We could have just done another
xen-like hack that forces the DMA API "ON" for virtio when running in a
secure VM.
The problem if we do that however is that we also then need the arch
PCI code to make sure it hooks up the virtio PCI devices with the
special "magic" DMA ops that avoid the iommu but still do swiotlb, ie,
not the same as other PCI devices. So it will have to play games such
as checking vendor/device IDs for virtio, checking the IOMMU flag,
etc... from the arch code which really bloody sucks when assigning PCI
DMA ops.
However, if we do it the way we plan here, on top of -1-, with a hook
called from virtio into the arch to "override" the virtio DMA ops, then
we avoid the problem completely: The arch hook would only be called by
virtio if the IOMMU flag is *not* set. IE only when using that special
"hypervisor" iommu bypass. If the IOMMU flag is set, virtio uses normal
PCI dma ops as usual.
That way, we have a very clear semantic: This hook is purely about
replacing those "null" DMA ops that just return PA introduced in -1-
with some arch provided specially cooked up DMA ops for non-IOMMU
virtio that know about the arch special requirements. For us bounce
buffering.
Is there something I'm missing ?
Cheers,
Ben.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-02 21:51:46
On Thu, Aug 02, 2018 at 04:13:09PM -0500, Benjamin Herrenschmidt wrote:
On Thu, 2018-08-02 at 23:52 +0300, Michael S. Tsirkin wrote:
quoted
quoted
Yes, this is the purpose of Anshuman original patch (I haven't looked
at the details of the patch in a while but that's what I told him to
implement ;-) :
- Make virtio always use DMA ops to simplify the code path (with a set
of "transparent" ops for legacy)
and
- Provide an arch hook allowing us to "override" those "transparent"
DMA ops with some custom ones that do the appropriate swiotlb gunk.
Cheers,
Ben.
Right but as I tried to say doing that brings us to a bunch of issues
with using DMA APIs in virtio. Put simply DMA APIs weren't designed for
guest to hypervisor communication.
I'm not sure I see the problem, see below
quoted
When we do (as is the case with PLATFORM_IOMMU right now) this adds a
bunch of overhead which we need to get rid of if we are to switch to
PLATFORM_IOMMU by default. We need to fix that.
So let's differenciate the two problems of having an IOMMU (real or
emulated) which indeeds adds overhead etc... and using the DMA API.
Well actually it's the other way around. An iommu in theory doesn't need
to bring overhead if you set it in bypass mode. Which does imply the
iommu supports bypass mode. Is that universally the case? DMA API does
see Christoph's list of things it does some of which add overhead.
At the moment, virtio does this all over the place:
if (use_dma_api)
dma_map/alloc_something(...)
else
use_pa
The idea of the patch set is to do two, somewhat orthogonal, changes
that together achieve what we want. Let me know where you think there
is "a bunch of issues" because I'm missing it:
1- Replace the above if/else constructs with just calling the DMA API,
and have virtio, at initialization, hookup its own dma_ops that just
"return pa" (roughly) when the IOMMU stuff isn't used.
This adds an indirect function call to the path that previously didn't
have one (the else case above). Is that a significant/measurable
overhead ?
Seems to be :( Jason reports about 4%. I wonder whether we can support
map_sg and friends being NULL, then use that when mapping is an
identity. A conditional branch there is likely very cheap.
Would this cover all platforms with kvm (which is where we care
most about performance)?
This change stands alone, and imho "cleans" up virtio by avoiding all
that if/else "2 path" and unless it adds a measurable overhead, should
probably be done.
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
This benefits from -1- by making us just plumb in a different set of
DMA ops we would have cooked up specifically for virtio in our arch
code (or in virtio itself but build arch-conditionally in a separate
file). But it doesn't strictly need it -1-:
Now, -2- doesn't strictly needs -1-. We could have just done another
xen-like hack that forces the DMA API "ON" for virtio when running in a
secure VM.
The problem if we do that however is that we also then need the arch
PCI code to make sure it hooks up the virtio PCI devices with the
special "magic" DMA ops that avoid the iommu but still do swiotlb, ie,
not the same as other PCI devices. So it will have to play games such
as checking vendor/device IDs for virtio, checking the IOMMU flag,
etc... from the arch code which really bloody sucks when assigning PCI
DMA ops.
However, if we do it the way we plan here, on top of -1-, with a hook
called from virtio into the arch to "override" the virtio DMA ops, then
we avoid the problem completely: The arch hook would only be called by
virtio if the IOMMU flag is *not* set. IE only when using that special
"hypervisor" iommu bypass. If the IOMMU flag is set, virtio uses normal
PCI dma ops as usual.
That way, we have a very clear semantic: This hook is purely about
replacing those "null" DMA ops that just return PA introduced in -1-
with some arch provided specially cooked up DMA ops for non-IOMMU
virtio that know about the arch special requirements. For us bounce
buffering.
Is there something I'm missing ?
Cheers,
Ben.
Right so I was trying to write it up in a systematic way, but just to
give you one example, if there is a system where DMA API handles
coherency issues, or flushing of some buffers, then our PLATFORM_IOMMU
flag causes that to happen.
And we kinda worked around this without the IOMMU by basically saying
"ok we do not really need DMA API so let's just bypass it" and
it was kind of ok except now everyone is switching to vIOMMU
just in case. So now people do want some parts of what DMA API does,
such as the bounce buffer use, or IOMMU mappings.
And maybe in the end the solution is going to be to do something similar
to virt_Xmb except for DMA APIs: add APIs that handle just the
addressing bits but without the overhead. See
commit 6a65d26385bf487926a0616650927303058551e3
asm-generic: implement virt_xxx memory barriers
for reference, it's a similar set of issues.
So it's not a problem with your patches as such, it's just that
they don't solve that harder problem.
--
MST
From: Jason Wang <hidden> Date: 2018-08-03 02:42:00
On 2018年08月03日 04:55, Michael S. Tsirkin wrote:
On Fri, Jul 20, 2018 at 09:29:37AM +0530, Anshuman Khandual wrote:
quoted
This patch series is the follow up on the discussions we had before about
the RFC titled [RFC,V2] virtio: Add platform specific DMA API translation
for virito devices (https://patchwork.kernel.org/patch/10417371/). There
were suggestions about doing away with two different paths of transactions
with the host/QEMU, first being the direct GPA and the other being the DMA
API based translations.
First patch attempts to create a direct GPA mapping based DMA operations
structure called 'virtio_direct_dma_ops' with exact same implementation
of the direct GPA path which virtio core currently has but just wrapped in
a DMA API format. Virtio core must use 'virtio_direct_dma_ops' instead of
the arch default in absence of VIRTIO_F_IOMMU_PLATFORM flag to preserve the
existing semantics. The second patch does exactly that inside the function
virtio_finalize_features(). The third patch removes the default direct GPA
path from virtio core forcing it to use DMA API callbacks for all devices.
Now with that change, every device must have a DMA operations structure
associated with it. The fourth patch adds an additional hook which gives
the platform an opportunity to do yet another override if required. This
platform hook can be used on POWER Ultravisor based protected guests to
load up SWIOTLB DMA callbacks to do the required (as discussed previously
in the above mentioned thread how host is allowed to access only parts of
the guest GPA range) bounce buffering into the shared memory for all I/O
scatter gather buffers to be consumed on the host side.
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
Jason did some work on profiling this. Unfortunately he reports
about 4% extra overhead from this switch on x86 with no vIOMMU.
The test is rather simple, just run pktgen (pktgen_sample01_simple.sh)
in guest and measure PPS on tap on host.
Thanks
I expect he's writing up the data in more detail, but
just wanted to let you know this would be one more
thing to debug before we can just switch to DMA APIs.
quoted
Anshuman Khandual (4):
virtio: Define virtio_direct_dma_ops structure
virtio: Override device's DMA OPS with virtio_direct_dma_ops selectively
virtio: Force virtio core to use DMA API callbacks for all virtio devices
virtio: Add platform specific DMA API translation for virito devices
arch/powerpc/include/asm/dma-mapping.h | 6 +++
arch/powerpc/platforms/pseries/iommu.c | 6 +++
drivers/virtio/virtio.c | 72 ++++++++++++++++++++++++++++++++++
drivers/virtio/virtio_pci_common.h | 3 ++
drivers/virtio/virtio_ring.c | 65 +-----------------------------
5 files changed, 89 insertions(+), 63 deletions(-)
--
2.9.3
From: Christoph Hellwig <hch@infradead.org> Date: 2018-08-03 07:05:17
On Thu, Aug 02, 2018 at 04:13:09PM -0500, Benjamin Herrenschmidt wrote:
So let's differenciate the two problems of having an IOMMU (real or
emulated) which indeeds adds overhead etc... and using the DMA API.
At the moment, virtio does this all over the place:
if (use_dma_api)
dma_map/alloc_something(...)
else
use_pa
The idea of the patch set is to do two, somewhat orthogonal, changes
that together achieve what we want. Let me know where you think there
is "a bunch of issues" because I'm missing it:
1- Replace the above if/else constructs with just calling the DMA API,
and have virtio, at initialization, hookup its own dma_ops that just
"return pa" (roughly) when the IOMMU stuff isn't used.
This adds an indirect function call to the path that previously didn't
have one (the else case above). Is that a significant/measurable
overhead ?
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup. To get swiotlb you'll need to then use the DT/ACPI
dma-range property to limit the addressable range, and a swiotlb
capable plaform will use swiotlb automatically.
From: Christoph Hellwig <hch@infradead.org> Date: 2018-08-03 07:06:44
On Thu, Aug 02, 2018 at 11:53:08PM +0300, Michael S. Tsirkin wrote:
quoted
We don't need cache flushing tricks.
You don't but do real devices on same platform need them?
IBMs power plaforms are always cache coherent. There are some powerpc
platforms have not cache coherent DMA, but I guess this scheme isn't
intended for them.
From: Christoph Hellwig <hch@infradead.org> Date: 2018-08-03 16:02:56
On Fri, Aug 03, 2018 at 10:58:36AM -0500, Benjamin Herrenschmidt wrote:
On Fri, 2018-08-03 at 00:05 -0700, Christoph Hellwig wrote:
quoted
quoted
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup.
Christoph, as I have explained already, we do NOT have a way to provide
such a flag as neither the hypervisor nor qemu knows anything about
this when the VM is created.
Well, if your setup is so fucked up I see no way to support it in Linux.
Let's end the discussion right now then.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-03 16:19:01
On Fri, 2018-08-03 at 00:05 -0700, Christoph Hellwig wrote:
quoted
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup.
Christoph, as I have explained already, we do NOT have a way to provide
such a flag as neither the hypervisor nor qemu knows anything about
this when the VM is created.
To get swiotlb you'll need to then use the DT/ACPI
dma-range property to limit the addressable range, and a swiotlb
capable plaform will use swiotlb automatically.
This cannot be done as you describe it.
The VM is created as a *normal* VM. The DT stuff is generated by qemu
at a point where it has *no idea* that the VM will later become secure
and thus will have to restrict which pages can be used for "DMA".
The VM will *at runtime* turn itself into a secure VM via interactions
with the security HW and the Ultravisor layer (which sits below the
HV). This happens way after the DT has been created and consumed, the
qemu devices instanciated etc...
Only the guest kernel knows because it initates the transition. When
that happens, the virtio devices have already been used by the guest
firmware, bootloader, possibly another kernel that kexeced the "secure"
one, etc...
So instead of running around saying NAK NAK NAK, please explain how we
can solve that differently.
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-03 18:59:57
On Fri, 2018-08-03 at 09:02 -0700, Christoph Hellwig wrote:
On Fri, Aug 03, 2018 at 10:58:36AM -0500, Benjamin Herrenschmidt wrote:
quoted
On Fri, 2018-08-03 at 00:05 -0700, Christoph Hellwig wrote:
quoted
quoted
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup.
Christoph, as I have explained already, we do NOT have a way to provide
such a flag as neither the hypervisor nor qemu knows anything about
this when the VM is created.
Well, if your setup is so fucked up I see no way to support it in Linux.
Let's end the discussion right now then.
You are saying something along the lines of "I don't like an
instruction in your ISA, let's not support your entire CPU architecture
in Linux".
Our setup is not fucked. It makes a LOT of sense and it's a very
sensible design. It's hitting a problem due to a corner case oddity in
virtio bypassing the MMU, we've worked around such corner cases many
times in the past without any problem, I fail to see what the problem
is here.
We aren't going to cancel years of HW and SW development for our
security infrastructure bcs you don't like a 2 lines hook into virtio
to make things work and aren't willing to even consider the options.
Ben.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-03 19:07:48
On Fri, Aug 03, 2018 at 10:58:36AM -0500, Benjamin Herrenschmidt wrote:
On Fri, 2018-08-03 at 00:05 -0700, Christoph Hellwig wrote:
quoted
quoted
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup.
Christoph, as I have explained already, we do NOT have a way to provide
such a flag as neither the hypervisor nor qemu knows anything about
this when the VM is created.
I think the fact you can't add flags from the hypervisor is
a sign of a problematic architecture, you should look at
adding that down the road - you will likely need it at some point.
However in this specific case, the flag does not need to come from the
hypervisor, it can be set by arch boot code I think.
Christoph do you see a problem with that?
quoted
To get swiotlb you'll need to then use the DT/ACPI
dma-range property to limit the addressable range, and a swiotlb
capable plaform will use swiotlb automatically.
This cannot be done as you describe it.
The VM is created as a *normal* VM. The DT stuff is generated by qemu
at a point where it has *no idea* that the VM will later become secure
and thus will have to restrict which pages can be used for "DMA".
The VM will *at runtime* turn itself into a secure VM via interactions
with the security HW and the Ultravisor layer (which sits below the
HV). This happens way after the DT has been created and consumed, the
qemu devices instanciated etc...
Only the guest kernel knows because it initates the transition. When
that happens, the virtio devices have already been used by the guest
firmware, bootloader, possibly another kernel that kexeced the "secure"
one, etc...
So instead of running around saying NAK NAK NAK, please explain how we
can solve that differently.
Ben.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-03 19:08:59
On Fri, Aug 03, 2018 at 10:41:41AM +0800, Jason Wang wrote:
On 2018年08月03日 04:55, Michael S. Tsirkin wrote:
quoted
On Fri, Jul 20, 2018 at 09:29:37AM +0530, Anshuman Khandual wrote:
quoted
This patch series is the follow up on the discussions we had before about
the RFC titled [RFC,V2] virtio: Add platform specific DMA API translation
for virito devices (https://patchwork.kernel.org/patch/10417371/). There
were suggestions about doing away with two different paths of transactions
with the host/QEMU, first being the direct GPA and the other being the DMA
API based translations.
First patch attempts to create a direct GPA mapping based DMA operations
structure called 'virtio_direct_dma_ops' with exact same implementation
of the direct GPA path which virtio core currently has but just wrapped in
a DMA API format. Virtio core must use 'virtio_direct_dma_ops' instead of
the arch default in absence of VIRTIO_F_IOMMU_PLATFORM flag to preserve the
existing semantics. The second patch does exactly that inside the function
virtio_finalize_features(). The third patch removes the default direct GPA
path from virtio core forcing it to use DMA API callbacks for all devices.
Now with that change, every device must have a DMA operations structure
associated with it. The fourth patch adds an additional hook which gives
the platform an opportunity to do yet another override if required. This
platform hook can be used on POWER Ultravisor based protected guests to
load up SWIOTLB DMA callbacks to do the required (as discussed previously
in the above mentioned thread how host is allowed to access only parts of
the guest GPA range) bounce buffering into the shared memory for all I/O
scatter gather buffers to be consumed on the host side.
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
Jason did some work on profiling this. Unfortunately he reports
about 4% extra overhead from this switch on x86 with no vIOMMU.
The test is rather simple, just run pktgen (pktgen_sample01_simple.sh) in
guest and measure PPS on tap on host.
Thanks
Could you supply host configuration involved please?
quoted
I expect he's writing up the data in more detail, but
just wanted to let you know this would be one more
thing to debug before we can just switch to DMA APIs.
quoted
Anshuman Khandual (4):
virtio: Define virtio_direct_dma_ops structure
virtio: Override device's DMA OPS with virtio_direct_dma_ops selectively
virtio: Force virtio core to use DMA API callbacks for all virtio devices
virtio: Add platform specific DMA API translation for virito devices
arch/powerpc/include/asm/dma-mapping.h | 6 +++
arch/powerpc/platforms/pseries/iommu.c | 6 +++
drivers/virtio/virtio.c | 72 ++++++++++++++++++++++++++++++++++
drivers/virtio/virtio_pci_common.h | 3 ++
drivers/virtio/virtio_ring.c | 65 +-----------------------------
5 files changed, 89 insertions(+), 63 deletions(-)
--
2.9.3
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-03 19:17:42
On Fri, Aug 03, 2018 at 12:05:07AM -0700, Christoph Hellwig wrote:
On Thu, Aug 02, 2018 at 04:13:09PM -0500, Benjamin Herrenschmidt wrote:
quoted
So let's differenciate the two problems of having an IOMMU (real or
emulated) which indeeds adds overhead etc... and using the DMA API.
At the moment, virtio does this all over the place:
if (use_dma_api)
dma_map/alloc_something(...)
else
use_pa
The idea of the patch set is to do two, somewhat orthogonal, changes
that together achieve what we want. Let me know where you think there
is "a bunch of issues" because I'm missing it:
1- Replace the above if/else constructs with just calling the DMA API,
and have virtio, at initialization, hookup its own dma_ops that just
"return pa" (roughly) when the IOMMU stuff isn't used.
This adds an indirect function call to the path that previously didn't
have one (the else case above). Is that a significant/measurable
overhead ?
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup. To get swiotlb you'll need to then use the DT/ACPI
dma-range property to limit the addressable range, and a swiotlb
capable plaform will use swiotlb automatically.
It seems reasonable to teach a platform to override dma-range
for a specific device e.g. in case it knows about bugs in ACPI.
--
MST
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-04 01:19:10
On Fri, 2018-08-03 at 22:07 +0300, Michael S. Tsirkin wrote:
On Fri, Aug 03, 2018 at 10:58:36AM -0500, Benjamin Herrenschmidt wrote:
quoted
On Fri, 2018-08-03 at 00:05 -0700, Christoph Hellwig wrote:
quoted
quoted
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup.
Christoph, as I have explained already, we do NOT have a way to provide
such a flag as neither the hypervisor nor qemu knows anything about
this when the VM is created.
I think the fact you can't add flags from the hypervisor is
a sign of a problematic architecture, you should look at
adding that down the road - you will likely need it at some point.
Well, we can later in the boot process. At VM creation time, it's just
a normal VM. The VM firmware, bootloader etc... are just operating
normally etc...
Later on, (we may have even already run Linux at that point,
unsecurely, as we can use Linux as a bootloader under some
circumstances), we start a "secure image".
This is a kernel zImage that includes a "ticket" that has the
appropriate signature etc... so that when that kernel starts, it can
authenticate with the ultravisor, be verified (along with its ramdisk)
etc... and copied (by the UV) into secure memory & run from there.
At that point, the hypervisor is informed that the VM has become
secure.
So at that point, we could exit to qemu to inform it of the change, and
have it walk the qtree and "Switch" all the virtio devices to use the
IOMMU I suppose, but it feels a lot grosser to me.
That's the only other option I can think of.
However in this specific case, the flag does not need to come from the
hypervisor, it can be set by arch boot code I think.
Christoph do you see a problem with that?
The above could do that yes. Another approach would be to do it from a
small virtio "quirk" that pokes a bit in the device to force it to
iommu mode when it detects that we are running in a secure VM. That's a
bit warty on the virito side but probably not as much as having a qemu
one that walks of the virtio devices to change how they behave.
What do you reckon ?
What we want to avoid is to expose any of this to the *end user* or
libvirt or any other higher level of the management stack. We really
want that stuff to remain contained between the VM itself, KVM and
maybe qemu.
We will need some other qemu changes for migration so that's ok. But
the minute you start touching libvirt and the higher levels it becomes
a nightmare.
Cheers,
Ben.
quoted
quoted
To get swiotlb you'll need to then use the DT/ACPI
dma-range property to limit the addressable range, and a swiotlb
capable plaform will use swiotlb automatically.
This cannot be done as you describe it.
The VM is created as a *normal* VM. The DT stuff is generated by qemu
at a point where it has *no idea* that the VM will later become secure
and thus will have to restrict which pages can be used for "DMA".
The VM will *at runtime* turn itself into a secure VM via interactions
with the security HW and the Ultravisor layer (which sits below the
HV). This happens way after the DT has been created and consumed, the
qemu devices instanciated etc...
Only the guest kernel knows because it initates the transition. When
that happens, the virtio devices have already been used by the guest
firmware, bootloader, possibly another kernel that kexeced the "secure"
one, etc...
So instead of running around saying NAK NAK NAK, please explain how we
can solve that differently.
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-04 01:19:21
On Fri, 2018-08-03 at 22:07 +0300, Michael S. Tsirkin wrote:
On Fri, Aug 03, 2018 at 10:58:36AM -0500, Benjamin Herrenschmidt wrote:
quoted
On Fri, 2018-08-03 at 00:05 -0700, Christoph Hellwig wrote:
quoted
quoted
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup.
Christoph, as I have explained already, we do NOT have a way to provide
such a flag as neither the hypervisor nor qemu knows anything about
this when the VM is created.
I think the fact you can't add flags from the hypervisor is
a sign of a problematic architecture, you should look at
adding that down the road - you will likely need it at some point.
Well, we can later in the boot process. At VM creation time, it's just
a normal VM. The VM firmware, bootloader etc... are just operating
normally etc...
Later on, (we may have even already run Linux at that point,
unsecurely, as we can use Linux as a bootloader under some
circumstances), we start a "secure image".
This is a kernel zImage that includes a "ticket" that has the
appropriate signature etc... so that when that kernel starts, it can
authenticate with the ultravisor, be verified (along with its ramdisk)
etc... and copied (by the UV) into secure memory & run from there.
At that point, the hypervisor is informed that the VM has become
secure.
So at that point, we could exit to qemu to inform it of the change, and
have it walk the qtree and "Switch" all the virtio devices to use the
IOMMU I suppose, but it feels a lot grosser to me.
That's the only other option I can think of.
However in this specific case, the flag does not need to come from the
hypervisor, it can be set by arch boot code I think.
Christoph do you see a problem with that?
The above could do that yes. Another approach would be to do it from a
small virtio "quirk" that pokes a bit in the device to force it to
iommu mode when it detects that we are running in a secure VM. That's a
bit warty on the virito side but probably not as much as having a qemu
one that walks of the virtio devices to change how they behave.
What do you reckon ?
What we want to avoid is to expose any of this to the *end user* or
libvirt or any other higher level of the management stack. We really
want that stuff to remain contained between the VM itself, KVM and
maybe qemu.
We will need some other qemu changes for migration so that's ok. But
the minute you start touching libvirt and the higher levels it becomes
a nightmare.
Cheers,
Ben.
quoted
quoted
To get swiotlb you'll need to then use the DT/ACPI
dma-range property to limit the addressable range, and a swiotlb
capable plaform will use swiotlb automatically.
This cannot be done as you describe it.
The VM is created as a *normal* VM. The DT stuff is generated by qemu
at a point where it has *no idea* that the VM will later become secure
and thus will have to restrict which pages can be used for "DMA".
The VM will *at runtime* turn itself into a secure VM via interactions
with the security HW and the Ultravisor layer (which sits below the
HV). This happens way after the DT has been created and consumed, the
qemu devices instanciated etc...
Only the guest kernel knows because it initates the transition. When
that happens, the virtio devices have already been used by the guest
firmware, bootloader, possibly another kernel that kexeced the "secure"
one, etc...
So instead of running around saying NAK NAK NAK, please explain how we
can solve that differently.
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-04 01:23:26
On Fri, 2018-08-03 at 22:07 +0300, Michael S. Tsirkin wrote:
On Fri, Aug 03, 2018 at 10:58:36AM -0500, Benjamin Herrenschmidt wrote:
quoted
On Fri, 2018-08-03 at 00:05 -0700, Christoph Hellwig wrote:
quoted
quoted
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup.
Christoph, as I have explained already, we do NOT have a way to provide
such a flag as neither the hypervisor nor qemu knows anything about
this when the VM is created.
I think the fact you can't add flags from the hypervisor is
a sign of a problematic architecture, you should look at
adding that down the road - you will likely need it at some point.
(Appologies if you got this twice, my mailer had a brain fart and I don't
know if the first one got through & am about to disappear in a plane for 17h)
Well, we can later in the boot process. At VM creation time, it's just
a normal VM. The VM firmware, bootloader etc... are just operating
normally etc...
Later on, (we may have even already run Linux at that point,
unsecurely, as we can use Linux as a bootloader under some
circumstances), we start a "secure image".
This is a kernel zImage that includes a "ticket" that has the
appropriate signature etc... so that when that kernel starts, it can
authenticate with the ultravisor, be verified (along with its ramdisk)
etc... and copied (by the UV) into secure memory & run from there.
At that point, the hypervisor is informed that the VM has become
secure.
So at that point, we could exit to qemu to inform it of the change, and
have it walk the qtree and "Switch" all the virtio devices to use the
IOMMU I suppose, but it feels a lot grosser to me.
That's the only other option I can think of.
However in this specific case, the flag does not need to come from the
hypervisor, it can be set by arch boot code I think.
Christoph do you see a problem with that?
The above could do that yes. Another approach would be to do it from a
small virtio "quirk" that pokes a bit in the device to force it to
iommu mode when it detects that we are running in a secure VM. That's a
bit warty on the virito side but probably not as much as having a qemu
one that walks of the virtio devices to change how they behave.
What do you reckon ?
What we want to avoid is to expose any of this to the *end user* or
libvirt or any other higher level of the management stack. We really
want that stuff to remain contained between the VM itself, KVM and
maybe qemu.
We will need some other qemu changes for migration so that's ok. But
the minute you start touching libvirt and the higher levels it becomes
a nightmare.
Cheers,
Ben.
quoted
quoted
To get swiotlb you'll need to then use the DT/ACPI
dma-range property to limit the addressable range, and a swiotlb
capable plaform will use swiotlb automatically.
This cannot be done as you describe it.
The VM is created as a *normal* VM. The DT stuff is generated by qemu
at a point where it has *no idea* that the VM will later become secure
and thus will have to restrict which pages can be used for "DMA".
The VM will *at runtime* turn itself into a secure VM via interactions
with the security HW and the Ultravisor layer (which sits below the
HV). This happens way after the DT has been created and consumed, the
qemu devices instanciated etc...
Only the guest kernel knows because it initates the transition. When
that happens, the virtio devices have already been used by the guest
firmware, bootloader, possibly another kernel that kexeced the "secure"
one, etc...
So instead of running around saying NAK NAK NAK, please explain how we
can solve that differently.
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-04 02:12:48
On Fri, 2018-08-03 at 22:07 +0300, Michael S. Tsirkin wrote:
On Fri, Aug 03, 2018 at 10:58:36AM -0500, Benjamin Herrenschmidt wrote:
quoted
On Fri, 2018-08-03 at 00:05 -0700, Christoph Hellwig wrote:
quoted
quoted
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup.
Christoph, as I have explained already, we do NOT have a way to provide
such a flag as neither the hypervisor nor qemu knows anything about
this when the VM is created.
I think the fact you can't add flags from the hypervisor is
a sign of a problematic architecture, you should look at
adding that down the road - you will likely need it at some point.
Well, we can later in the boot process. At VM creation time, it's just
a normal VM. The VM firmware, bootloader etc... are just operating
normally etc...
Later on, (we may have even already run Linux at that point,
unsecurely, as we can use Linux as a bootloader under some
circumstances), we start a "secure image".
This is a kernel zImage that includes a "ticket" that has the
appropriate signature etc... so that when that kernel starts, it can
authenticate with the ultravisor, be verified (along with its ramdisk)
etc... and copied (by the UV) into secure memory & run from there.
At that point, the hypervisor is informed that the VM has become
secure.
So at that point, we could exit to qemu to inform it of the change, and
have it walk the qtree and "Switch" all the virtio devices to use the
IOMMU I suppose, but it feels a lot grosser to me.
That's the only other option I can think of.
However in this specific case, the flag does not need to come from the
hypervisor, it can be set by arch boot code I think.
Christoph do you see a problem with that?
The above could do that yes. Another approach would be to do it from a
small virtio "quirk" that pokes a bit in the device to force it to
iommu mode when it detects that we are running in a secure VM. That's a
bit warty on the virito side but probably not as much as having a qemu
one that walks of the virtio devices to change how they behave.
What do you reckon ?
Cheers,
Ben.
quoted
quoted
To get swiotlb you'll need to then use the DT/ACPI
dma-range property to limit the addressable range, and a swiotlb
capable plaform will use swiotlb automatically.
This cannot be done as you describe it.
The VM is created as a *normal* VM. The DT stuff is generated by qemu
at a point where it has *no idea* that the VM will later become secure
and thus will have to restrict which pages can be used for "DMA".
The VM will *at runtime* turn itself into a secure VM via interactions
with the security HW and the Ultravisor layer (which sits below the
HV). This happens way after the DT has been created and consumed, the
qemu devices instanciated etc...
Only the guest kernel knows because it initates the transition. When
that happens, the virtio devices have already been used by the guest
firmware, bootloader, possibly another kernel that kexeced the "secure"
one, etc...
So instead of running around saying NAK NAK NAK, please explain how we
can solve that differently.
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-04 02:41:34
On Fri, 2018-08-03 at 22:08 +0300, Michael S. Tsirkin wrote:
quoted
quoted
quoted
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
Jason did some work on profiling this. Unfortunately he reports
about 4% extra overhead from this switch on x86 with no vIOMMU.
The test is rather simple, just run pktgen (pktgen_sample01_simple.sh) in
guest and measure PPS on tap on host.
Thanks
Could you supply host configuration involved please?
I wonder how much of that could be caused by Spectre mitigations
blowing up indirect function calls...
Cheers,
Ben.
From: Christoph Hellwig <hch@infradead.org> Date: 2018-08-04 08:15:13
On Fri, Aug 03, 2018 at 10:17:32PM +0300, Michael S. Tsirkin wrote:
It seems reasonable to teach a platform to override dma-range
for a specific device e.g. in case it knows about bugs in ACPI.
A platform will be able override dma-range using the dev->bus_dma_mask
field starting in 4.19. But we'll still need a way how to
a) document in the virtio spec that all bus dma quirks are to be
applied
b) a way to document in a virtio-related spec how the bus handles
dma for Ben's totally fucked up hypervisor. Without that there
is not way we'll get interoperable implementations.
From: Christoph Hellwig <hch@infradead.org> Date: 2018-08-04 08:21:29
On Fri, Aug 03, 2018 at 01:58:46PM -0500, Benjamin Herrenschmidt wrote:
You are saying something along the lines of "I don't like an
instruction in your ISA, let's not support your entire CPU architecture
in Linux".
No. I'm saying if you can't describe your architecture in the virtio
spec document it is bogus.
Our setup is not fucked. It makes a LOT of sense and it's a very
sensible design. It's hitting a problem due to a corner case oddity in
virtio bypassing the MMU, we've worked around such corner cases many
times in the past without any problem, I fail to see what the problem
is here.
No matter if you like it or not (I don't!) virtio is defined to bypass
dma translations, it is very clearly stated in the spec. It has some
ill-defined bits to bypass it, so if you want the dma mapping API
to be used you'll have to set that bit (in its original form, a refined
form, or an entirely newly defined sane form) and make sure your
hypersivors always sets it. It's not rocket science, just a little bit
for work to make sure your setup is actually going to work reliably
and portably.
We aren't going to cancel years of HW and SW development for our
Maybe you should have actually read the specs you are claiming to
implemented before spending all that effort.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-05 00:10:05
On Sat, Aug 04, 2018 at 01:15:00AM -0700, Christoph Hellwig wrote:
On Fri, Aug 03, 2018 at 10:17:32PM +0300, Michael S. Tsirkin wrote:
quoted
It seems reasonable to teach a platform to override dma-range
for a specific device e.g. in case it knows about bugs in ACPI.
A platform will be able override dma-range using the dev->bus_dma_mask
field starting in 4.19. But we'll still need a way how to
a) document in the virtio spec that all bus dma quirks are to be
applied
I agree it's a good idea. In particular I suspect that PLATFORM_IOMMU
should be extended to cover that. But see below.
b) a way to document in a virtio-related spec how the bus handles
dma for Ben's totally fucked up hypervisor. Without that there
is not way we'll get interoperable implementations.
So in this case however I'm not sure what exactly do we want to add. It
seems that from point of view of the device, there is nothing special -
it just gets a PA and writes there. It also seems that guest does not
need to get any info from the device either. Instead guest itself needs
device to DMA into specific addresses, for its own reasons.
It seems that the fact that within guest it's implemented using a bounce
buffer and that it's easiest to do by switching virtio to use the DMA API
isn't something virtio spec concerns itself with.
I'm open to suggestions.
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-05 00:22:46
On Fri, Aug 03, 2018 at 08:16:21PM -0500, Benjamin Herrenschmidt wrote:
On Fri, 2018-08-03 at 22:07 +0300, Michael S. Tsirkin wrote:
quoted
On Fri, Aug 03, 2018 at 10:58:36AM -0500, Benjamin Herrenschmidt wrote:
quoted
On Fri, 2018-08-03 at 00:05 -0700, Christoph Hellwig wrote:
quoted
quoted
2- Make virtio use the DMA API with our custom platform-provided
swiotlb callbacks when needed, that is when not using IOMMU *and*
running on a secure VM in our case.
And total NAK the customer platform-provided part of this. We need
a flag passed in from the hypervisor that the device needs all bus
specific dma api treatment, and then just use the normal plaform
dma mapping setup.
Christoph, as I have explained already, we do NOT have a way to provide
such a flag as neither the hypervisor nor qemu knows anything about
this when the VM is created.
I think the fact you can't add flags from the hypervisor is
a sign of a problematic architecture, you should look at
adding that down the road - you will likely need it at some point.
Well, we can later in the boot process. At VM creation time, it's just
a normal VM. The VM firmware, bootloader etc... are just operating
normally etc...
I see the allure of this, but I think down the road you will
discover passing a flag in libvirt XML saying
"please use a secure mode" or whatever is a good idea.
Even thought it is probably not required to address this
specific issue.
For example, I don't think ballooning works in secure mode,
you will be able to teach libvirt not to try to add a
balloon to the guest.
Later on, (we may have even already run Linux at that point,
unsecurely, as we can use Linux as a bootloader under some
circumstances), we start a "secure image".
This is a kernel zImage that includes a "ticket" that has the
appropriate signature etc... so that when that kernel starts, it can
authenticate with the ultravisor, be verified (along with its ramdisk)
etc... and copied (by the UV) into secure memory & run from there.
At that point, the hypervisor is informed that the VM has become
secure.
So at that point, we could exit to qemu to inform it of the change,
That's probably a good idea too.
and
have it walk the qtree and "Switch" all the virtio devices to use the
IOMMU I suppose, but it feels a lot grosser to me.
That part feels gross, yes.
That's the only other option I can think of.
quoted
However in this specific case, the flag does not need to come from the
hypervisor, it can be set by arch boot code I think.
Christoph do you see a problem with that?
The above could do that yes. Another approach would be to do it from a
small virtio "quirk" that pokes a bit in the device to force it to
iommu mode when it detects that we are running in a secure VM. That's a
bit warty on the virito side but probably not as much as having a qemu
one that walks of the virtio devices to change how they behave.
What do you reckon ?
I think you are right that for the dma limit the hypervisor doesn't seem
to need to know.
What we want to avoid is to expose any of this to the *end user* or
libvirt or any other higher level of the management stack. We really
want that stuff to remain contained between the VM itself, KVM and
maybe qemu.
We will need some other qemu changes for migration so that's ok. But
the minute you start touching libvirt and the higher levels it becomes
a nightmare.
Cheers,
Ben.
I don't believe you'll be able to avoid that entirely. The split between
libvirt and qemu is more about community than about code, random bits of
functionality tend to land on random sides of that fence. Better add a
tag in domain XML early is my advice. Having said that, it's your
hypervisor. I'm just suggesting that when hypervisor does somehow need
to care then I suspect most people won't be receptive to the argument
that changing libvirt is a nightmare.
quoted
quoted
quoted
To get swiotlb you'll need to then use the DT/ACPI
dma-range property to limit the addressable range, and a swiotlb
capable plaform will use swiotlb automatically.
This cannot be done as you describe it.
The VM is created as a *normal* VM. The DT stuff is generated by qemu
at a point where it has *no idea* that the VM will later become secure
and thus will have to restrict which pages can be used for "DMA".
The VM will *at runtime* turn itself into a secure VM via interactions
with the security HW and the Ultravisor layer (which sits below the
HV). This happens way after the DT has been created and consumed, the
qemu devices instanciated etc...
Only the guest kernel knows because it initates the transition. When
that happens, the virtio devices have already been used by the guest
firmware, bootloader, possibly another kernel that kexeced the "secure"
one, etc...
So instead of running around saying NAK NAK NAK, please explain how we
can solve that differently.
Ben.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-05 00:23:47
On Fri, Aug 03, 2018 at 08:22:11PM -0500, Benjamin Herrenschmidt wrote:
(Appologies if you got this twice, my mailer had a brain fart and I don't
know if the first one got through & am about to disappear in a plane for 17h)
I got like 3 of these. I hope that's true for everyone as I replied to
1st one.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-05 00:24:36
On Fri, Aug 03, 2018 at 08:21:26PM -0500, Benjamin Herrenschmidt wrote:
On Fri, 2018-08-03 at 22:08 +0300, Michael S. Tsirkin wrote:
quoted
quoted
quoted
quoted
Please go through these patches and review whether this approach broadly
makes sense. I will appreciate suggestions, inputs, comments regarding
the patches or the approach in general. Thank you.
Jason did some work on profiling this. Unfortunately he reports
about 4% extra overhead from this switch on x86 with no vIOMMU.
The test is rather simple, just run pktgen (pktgen_sample01_simple.sh) in
guest and measure PPS on tap on host.
Thanks
Could you supply host configuration involved please?
I wonder how much of that could be caused by Spectre mitigations
blowing up indirect function calls...
Cheers,
Ben.
I won't be surprised. If yes I suggested a way to mitigate the overhead.
--
MSR
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2018-08-05 00:27:52
On Wed, Aug 01, 2018 at 09:16:38AM +0100, Will Deacon wrote:
On Tue, Jul 31, 2018 at 03:36:22PM -0500, Benjamin Herrenschmidt wrote:
quoted
On Tue, 2018-07-31 at 10:30 -0700, Christoph Hellwig wrote:
quoted
quoted
However the question people raise is that DMA API is already full of
arch-specific tricks the likes of which are outlined in your post linked
above. How is this one much worse?
None of these warts is visible to the driver, they are all handled in
the architecture (possibly on a per-bus basis).
So for virtio we really need to decide if it has one set of behavior
as specified in the virtio spec, or if it behaves exactly as if it
was on a PCI bus, or in fact probably both as you lined up. But no
magic arch specific behavior inbetween.
The only arch specific behaviour is needed in the case where it doesn't
behave like PCI. In this case, the PCI DMA ops are not suitable, but in
our secure VMs, we still need to make it use swiotlb in order to bounce
through non-secure pages.
On arm/arm64, the problem we have is that legacy virtio devices on the MMIO
transport (so definitely not PCI) have historically been advertised by qemu
as not being cache coherent, but because the virtio core has bypassed DMA
ops then everything has happened to work. If we blindly enable the arch DMA
ops, we'll plumb in the non-coherent ops and start getting data corruption,
so we do need a way to quirk virtio as being "always coherent" if we want to
use the DMA ops (which we do, because our emulation platforms have an IOMMU
for all virtio devices).
Will
Right that's not very different from placing the device within the IOMMU
domain but in fact bypassing the IOMMU. I wonder whether anyone ever
needs a non coherent virtio-mmio. If yes we can extend
PLATFORM_IOMMU to cover that or add another bit.
What exactly do the non-coherent ops do that causes the corruption?
--
MST
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-05 02:05:31
On Sat, 2018-08-04 at 01:21 -0700, Christoph Hellwig wrote:
No matter if you like it or not (I don't!) virtio is defined to bypass
dma translations, it is very clearly stated in the spec. It has some
ill-defined bits to bypass it, so if you want the dma mapping API
to be used you'll have to set that bit (in its original form, a refined
form, or an entirely newly defined sane form) and make sure your
hypersivors always sets it. It's not rocket science, just a little bit
for work to make sure your setup is actually going to work reliably
and portably.
I think you are conflating completely different things, let me try to
clarify, we might actually be talking past each other.
quoted
We aren't going to cancel years of HW and SW development for our
Maybe you should have actually read the specs you are claiming to
implemented before spending all that effort.
Anyway, let's cool our respective jets and sort that out, there are
indeed other approaches than overriding the DMA ops with special ones,
though I find them less tasty ... but here's my attempt at a (simpler)
description.
Bear with me for the long-ish email, this tries to describe the system
so you get an idea where we come from, and options we can use to get
out of this.
So we *are* implementing the spec, since qemu is currently unmodified:
Default virtio will bypass the iommu emulated by qemu as per spec etc..
On the Linux side, thus, virtio "sees" a normal iommu-bypassing device
and will treat it as such.
The problem is the assumption in the middle that qemu can access all
guest pages directly, which holds true for traditional VMs, but breaks
when the VM in our case turns itself into a secure VM. This isn't under
the action (or due to changes in) the hypervisor. KVM operates (almost)
normally here.
But there's this (very thin and open source btw) layer underneath
called ultravisor, which exploits some HW facilities to maintain a
separate pool of "secure" memory, which cannot be physically accessed
by a non-secure entity.
So in our scenario, qemu and KVM create a VM totally normally, there is
no changes required to the VM firmware, bootloader(s), etc... in fact
we support Linux based bootloaders, and those will work as normal linux
would in a VM, virtio works normally, etc...
Until that VM (via grub or kexec for example) loads a "secure image".
That secure image is a Linux kernel which has been "wrapped" (to simply
imagine a modified zImage wrapper though that's not entirely exact).
When that is run, before it modifies it's .data, it will interact with
the ultravisor using a specific HW facility to make itself secure. What
happens then is that the UV cryptographically verifies the kernel and
ramdisk, and copies them to the secure memory where execution returns.
The Ultravisor is then involved as a small shim for hypercalls between
the secure VM and KVM to prevent leakage of information (sanitize
registers etc...).
Now at this point, qemu can no longer access the secure VM pages
(there's more to this, such as using HMM to allow migration/encryption
accross etc... but let's not get bogged down).
So virtio can no longer access any page in the VM.
Now the VM *can* request from the Ultravisor some selected pages to be
made "insecure" and thus shared with qemu. This is how we handle some
of the pages used in our paravirt stuff, and that's how we want to deal
with virtio, by creating an insecure swiotlb pool.
At this point, thus, there are two options.
- One you have rejected, which is to have a way for "no-iommu" virtio
(which still doesn't use an iommu on the qemu side and doesn't need
to), to be forced to use some custom DMA ops on the VM side.
- One, which sadly has more overhead and will require modifying more
pieces of the puzzle, which is to make qemu uses an emulated iommu.
Once we make qemu do that, we can then layer swiotlb on top of the
emulated iommu on the guest side, and pass that as dma_ops to virtio.
Now, assuming you still absolutely want us to go down the second
option, there are several ways to get there. We would prefer to avoid
requiring the user to pass some special option to qemu. That has an
impact up the food chain (libvirt, management tools etc...) and users
probably won't understand what it's about. In fact the *end user* might
not even need to know a VM is secure, though applications inside might.
There's the additional annoyance that currently our guest FW (SLOF)
cannot deal with virtio in IOMMU mode, but that's fixable.
From there, refer to the email chain between Michael and I where we are
discussing options to "switch" virtio at runtime on the qemu side.
Any comment or suggestion ?
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-05 02:21:38
On Sun, 2018-08-05 at 03:09 +0300, Michael S. Tsirkin wrote:
It seems that the fact that within guest it's implemented using a bounce
buffer and that it's easiest to do by switching virtio to use the DMA API
isn't something virtio spec concerns itself with.
Right, this is my reasoning as well. See this other (long) email I just
sent to Christoph to explain the whole flow.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-05 03:21:57
On Sat, 2018-08-04 at 01:15 -0700, Christoph Hellwig wrote:
b) a way to document in a virtio-related spec how the bus handles
dma for Ben's totally fucked up hypervisor. Without that there
is not way we'll get interoperable implementations.
Christoph, this isn't a totally fucked up hypervisor. It's not even
about the hypervisor itself, I mean seriously, man, can you at least
bother reading what I described is going on with the security
architecture ?
Anyway, Michael is onto what could possibly be an alternative approach,
by having us tell qemu to flip to iommu mode at secure VM boot time.
Let's see where that leads.
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-05 04:57:08
On Sun, 2018-08-05 at 03:22 +0300, Michael S. Tsirkin wrote:
I see the allure of this, but I think down the road you will
discover passing a flag in libvirt XML saying
"please use a secure mode" or whatever is a good idea.
Even thought it is probably not required to address this
specific issue.
For example, I don't think ballooning works in secure mode,
you will be able to teach libvirt not to try to add a
balloon to the guest.
Right, we'll need some quirk to disable balloons in the guest I
suppose.
Passing something from libvirt is cumbersome because the end user may
not even need to know about secure VMs. There are use cases where the
security is a contract down to some special application running inside
the secure VM, the sysadmin knows nothing about.
Also there's repercussions all the way to admin tools, web UIs etc...
so it's fairly wide ranging.
So as long as we only need to quirk a couple of devices, it's much
better contained that way.
quoted
Later on, (we may have even already run Linux at that point,
unsecurely, as we can use Linux as a bootloader under some
circumstances), we start a "secure image".
This is a kernel zImage that includes a "ticket" that has the
appropriate signature etc... so that when that kernel starts, it can
authenticate with the ultravisor, be verified (along with its ramdisk)
etc... and copied (by the UV) into secure memory & run from there.
At that point, the hypervisor is informed that the VM has become
secure.
So at that point, we could exit to qemu to inform it of the change,
That's probably a good idea too.
We probably will have to tell qemu eventually for migration, as we'll
need some kind of key exchange phase etc... to deal with the crypto
aspects (the actual page copy is sorted via encrypting the secure pages
back to normal pages in qemu, but we'll need extra metadata).
quoted
and
have it walk the qtree and "Switch" all the virtio devices to use the
IOMMU I suppose, but it feels a lot grosser to me.
That part feels gross, yes.
quoted
That's the only other option I can think of.
quoted
However in this specific case, the flag does not need to come from the
hypervisor, it can be set by arch boot code I think.
Christoph do you see a problem with that?
The above could do that yes. Another approach would be to do it from a
small virtio "quirk" that pokes a bit in the device to force it to
iommu mode when it detects that we are running in a secure VM. That's a
bit warty on the virito side but probably not as much as having a qemu
one that walks of the virtio devices to change how they behave.
What do you reckon ?
I think you are right that for the dma limit the hypervisor doesn't seem
to need to know.
It's not just a limit mind you. It's a range, at least if we allocate
just a single pool of insecure pages. swiotlb feels like a better
option for us.
quoted
What we want to avoid is to expose any of this to the *end user* or
libvirt or any other higher level of the management stack. We really
want that stuff to remain contained between the VM itself, KVM and
maybe qemu.
We will need some other qemu changes for migration so that's ok. But
the minute you start touching libvirt and the higher levels it becomes
a nightmare.
Cheers,
Ben.
I don't believe you'll be able to avoid that entirely. The split between
libvirt and qemu is more about community than about code, random bits of
functionality tend to land on random sides of that fence. Better add a
tag in domain XML early is my advice. Having said that, it's your
hypervisor. I'm just suggesting that when hypervisor does somehow need
to care then I suspect most people won't be receptive to the argument
that changing libvirt is a nightmare.
It only needs to care at runtime. The problem isn't changing libvirt
per-se, I don't have a problem with that. The problem is that it means
creating two categories of machines "secure" and "non-secure", which is
end-user visible, and thus has to be escalated to all the various
management stacks, UIs, etc... out there.
In addition, there are some cases where the individual creating the VMs
may not have any idea that they are secure.
But yes, if we have to, we'll do it. However, so far, we don't think
it's a great idea.
Cheers,
Ben.
quoted
quoted
quoted
quoted
To get swiotlb you'll need to then use the DT/ACPI
dma-range property to limit the addressable range, and a swiotlb
capable plaform will use swiotlb automatically.
This cannot be done as you describe it.
The VM is created as a *normal* VM. The DT stuff is generated by qemu
at a point where it has *no idea* that the VM will later become secure
and thus will have to restrict which pages can be used for "DMA".
The VM will *at runtime* turn itself into a secure VM via interactions
with the security HW and the Ultravisor layer (which sits below the
HV). This happens way after the DT has been created and consumed, the
qemu devices instanciated etc...
Only the guest kernel knows because it initates the transition. When
that happens, the virtio devices have already been used by the guest
firmware, bootloader, possibly another kernel that kexeced the "secure"
one, etc...
So instead of running around saying NAK NAK NAK, please explain how we
can solve that differently.
Ben.
From: Christoph Hellwig <hch@infradead.org> Date: 2018-08-05 07:26:01
On Sun, Aug 05, 2018 at 03:09:55AM +0300, Michael S. Tsirkin wrote:
So in this case however I'm not sure what exactly do we want to add. It
seems that from point of view of the device, there is nothing special -
it just gets a PA and writes there. It also seems that guest does not
need to get any info from the device either. Instead guest itself needs
device to DMA into specific addresses, for its own reasons.
It seems that the fact that within guest it's implemented using a bounce
buffer and that it's easiest to do by switching virtio to use the DMA API
isn't something virtio spec concerns itself with.
And that is exactly what we added bus_dma_mask for - the case where
the device itself has not limitation (or a bigger limitation), but
the platform limits the accessible dma ranges. One typical case is
a PCIe root port that is only connected to the CPU through an
interconnect that is limited to 32 address bits for example.