Currently the advanced guest memory protection technologies (AMD SEV,
powerpc secure guest technology and s390 Protected VMs) abuse the
VIRTIO_F_IOMMU_PLATFORM flag to make virtio core use the DMA API, which
is in turn necessary, to make IO work with guest memory protection.
But VIRTIO_F_IOMMU_PLATFORM a.k.a. VIRTIO_F_ACCESS_PLATFORM is really a
different beast: with virtio devices whose implementation runs on an SMP
CPU we are still fine with doing all the usual optimizations, it is just
that we need to make sure that the memory protection mechanism does not
get in the way. The VIRTIO_F_ACCESS_PLATFORM mandates more work on the
side of the guest (and possibly he host side as well) than we actually
need.
An additional benefit of teaching the guest to make the right decision
(and use DMA API) on it's own is: removing the need, to mandate special
VM configuration for guests that may run with protection. This is
especially interesting for s390 as VIRTIO_F_IOMMU_PLATFORM pushes all
the virtio control structures into the first 2G of guest memory:
something we don't necessarily want to do per-default.
Signed-off-by: Halil Pasic <[email protected]>
Tested-by: Ram Pai <[email protected]>
Tested-by: Michael Mueller <[email protected]>
---
drivers/virtio/virtio_ring.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 867c7ebd3f10..fafc8f924955 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
if (!virtio_has_iommu_quirk(vdev))
return true;
+ if (force_dma_unencrypted(&vdev->dev))
+ return true;
+
/* Otherwise, we are left to guess. */
/*
* In theory, it's possible to have a buggy QEMU-supposed
--
2.17.1
On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 867c7ebd3f10..fafc8f924955 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> if (!virtio_has_iommu_quirk(vdev))
> return true;
>
> + if (force_dma_unencrypted(&vdev->dev))
> + return true;
Hell no. This is a detail of the platform DMA direct implementation.
Drivers have no business looking at this flag, and virtio finally needs
to be fixed to use the DMA API properly for everything but legacy devices.
No amount of desparate hacks is going to fix that fundamental problem,
and I'm starting to get really sick and tired of all the crap patches
published in this area.
On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
> Currently the advanced guest memory protection technologies (AMD SEV,
> powerpc secure guest technology and s390 Protected VMs) abuse the
> VIRTIO_F_IOMMU_PLATFORM flag to make virtio core use the DMA API, which
> is in turn necessary, to make IO work with guest memory protection.
>
> But VIRTIO_F_IOMMU_PLATFORM a.k.a. VIRTIO_F_ACCESS_PLATFORM is really a
> different beast: with virtio devices whose implementation runs on an SMP
> CPU we are still fine with doing all the usual optimizations, it is just
> that we need to make sure that the memory protection mechanism does not
> get in the way. The VIRTIO_F_ACCESS_PLATFORM mandates more work on the
> side of the guest (and possibly he host side as well) than we actually
> need.
>
> An additional benefit of teaching the guest to make the right decision
> (and use DMA API) on it's own is: removing the need, to mandate special
> VM configuration for guests that may run with protection. This is
> especially interesting for s390 as VIRTIO_F_IOMMU_PLATFORM pushes all
> the virtio control structures into the first 2G of guest memory:
> something we don't necessarily want to do per-default.
>
> Signed-off-by: Halil Pasic <[email protected]>
> Tested-by: Ram Pai <[email protected]>
> Tested-by: Michael Mueller <[email protected]>
This might work for you but it's fragile, since without
VIRTIO_F_ACCESS_PLATFORM hypervisor assumes it gets
GPA's, not DMA addresses.
IOW this looks like another iteration of:
virtio: Support encrypted memory on powerpc secure guests
which I was under the impression was abandoned as unnecessary.
To summarize, the necessary conditions for a hack along these lines
(using DMA API without VIRTIO_F_ACCESS_PLATFORM) are that we detect that:
- secure guest mode is enabled - so we know that since we don't share
most memory regular virtio code won't
work, even though the buggy hypervisor didn't set VIRTIO_F_ACCESS_PLATFORM
- DMA API is giving us addresses that are actually also physical
addresses
- Hypervisor is buggy and didn't enable VIRTIO_F_ACCESS_PLATFORM
I don't see how this patch does this.
> ---
> drivers/virtio/virtio_ring.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 867c7ebd3f10..fafc8f924955 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> if (!virtio_has_iommu_quirk(vdev))
> return true;
>
> + if (force_dma_unencrypted(&vdev->dev))
> + return true;
> +
> /* Otherwise, we are left to guess. */
> /*
> * In theory, it's possible to have a buggy QEMU-supposed
> --
> 2.17.1
On Thu, Feb 20, 2020 at 03:55:14PM -0500, Michael S. Tsirkin wrote:
> On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
> > Currently the advanced guest memory protection technologies (AMD SEV,
> > powerpc secure guest technology and s390 Protected VMs) abuse the
> > VIRTIO_F_IOMMU_PLATFORM flag to make virtio core use the DMA API, which
> > is in turn necessary, to make IO work with guest memory protection.
> >
> > But VIRTIO_F_IOMMU_PLATFORM a.k.a. VIRTIO_F_ACCESS_PLATFORM is really a
> > different beast: with virtio devices whose implementation runs on an SMP
> > CPU we are still fine with doing all the usual optimizations, it is just
> > that we need to make sure that the memory protection mechanism does not
> > get in the way. The VIRTIO_F_ACCESS_PLATFORM mandates more work on the
> > side of the guest (and possibly he host side as well) than we actually
> > need.
> >
> > An additional benefit of teaching the guest to make the right decision
> > (and use DMA API) on it's own is: removing the need, to mandate special
> > VM configuration for guests that may run with protection. This is
> > especially interesting for s390 as VIRTIO_F_IOMMU_PLATFORM pushes all
> > the virtio control structures into the first 2G of guest memory:
> > something we don't necessarily want to do per-default.
> >
> > Signed-off-by: Halil Pasic <[email protected]>
> > Tested-by: Ram Pai <[email protected]>
> > Tested-by: Michael Mueller <[email protected]>
>
> This might work for you but it's fragile, since without
> VIRTIO_F_ACCESS_PLATFORM hypervisor assumes it gets
> GPA's, not DMA addresses.
>
>
>
> IOW this looks like another iteration of:
>
> virtio: Support encrypted memory on powerpc secure guests
>
> which I was under the impression was abandoned as unnecessary.
It has been abondoned on powerpc. We enabled VIRTIO_F_ACCESS_PLATFORM;
by default, flag on powerpc.
We would like to enable secure guests on powerpc without this flag
aswell enabled, but past experience has educated us that its not a easy
path. However if Halil makes some inroads in this path for s390, we
will like to support him.
RP
On Thu, Feb 20, 2020 at 05:13:09PM +0100, Christoph Hellwig wrote:
> On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 867c7ebd3f10..fafc8f924955 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> > if (!virtio_has_iommu_quirk(vdev))
> > return true;
> >
> > + if (force_dma_unencrypted(&vdev->dev))
> > + return true;
>
> Hell no. This is a detail of the platform DMA direct implementation.
> Drivers have no business looking at this flag, and virtio finally needs
> to be fixed to use the DMA API properly for everything but legacy devices.
So, this patch definitely isn't right as it stands, but I'm struggling
to understand what it is you're saying is the right way.
By "legacy devices" I assume you mean pre-virtio-1.0 devices, that
lack the F_VERSION_1 feature flag. Is that right? Because I don't
see how being a legacy device or not relates to use of the DMA API.
I *think* what you are suggesting here is that virtio devices that
have !F_IOMMU_PLATFORM should have their dma_ops set up so that the
DMA API treats IOVA==PA, which will satisfy what the device expects.
Then the virtio driver can use the DMA API the same way for both
F_IOMMU_PLATFORM and !F_IOMMU_PLATFORM devices.
But if that works for !F_IOMMU_PLATFORM_DEVICES+F_VERSION_1 devices,
then AFAICT it will work equally well for legacy devices.
Using the DMA API for *everything* in virtio, legacy or not, seems
like a reasonable approach to me. But, AFAICT, that does require the
DMA layer to have some kind of explicit call to turn on this
behaviour, which the virtio driver would call during initializsation.
I don't think we can do it 100% within the DMA layer, because only the
driver can reasonably know when a device has this weird non-standard
DMA behaviour.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Thu, Feb 20, 2020 at 05:17:48PM -0800, Ram Pai wrote:
> On Thu, Feb 20, 2020 at 03:55:14PM -0500, Michael S. Tsirkin wrote:
> > On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
> > > Currently the advanced guest memory protection technologies (AMD SEV,
> > > powerpc secure guest technology and s390 Protected VMs) abuse the
> > > VIRTIO_F_IOMMU_PLATFORM flag to make virtio core use the DMA API, which
> > > is in turn necessary, to make IO work with guest memory protection.
> > >
> > > But VIRTIO_F_IOMMU_PLATFORM a.k.a. VIRTIO_F_ACCESS_PLATFORM is really a
> > > different beast: with virtio devices whose implementation runs on an SMP
> > > CPU we are still fine with doing all the usual optimizations, it is just
> > > that we need to make sure that the memory protection mechanism does not
> > > get in the way. The VIRTIO_F_ACCESS_PLATFORM mandates more work on the
> > > side of the guest (and possibly he host side as well) than we actually
> > > need.
> > >
> > > An additional benefit of teaching the guest to make the right decision
> > > (and use DMA API) on it's own is: removing the need, to mandate special
> > > VM configuration for guests that may run with protection. This is
> > > especially interesting for s390 as VIRTIO_F_IOMMU_PLATFORM pushes all
> > > the virtio control structures into the first 2G of guest memory:
> > > something we don't necessarily want to do per-default.
> > >
> > > Signed-off-by: Halil Pasic <[email protected]>
> > > Tested-by: Ram Pai <[email protected]>
> > > Tested-by: Michael Mueller <[email protected]>
> >
> > This might work for you but it's fragile, since without
> > VIRTIO_F_ACCESS_PLATFORM hypervisor assumes it gets
> > GPA's, not DMA addresses.
> >
> >
> >
> > IOW this looks like another iteration of:
> >
> > virtio: Support encrypted memory on powerpc secure guests
> >
> > which I was under the impression was abandoned as unnecessary.
>
> It has been abondoned on powerpc. We enabled VIRTIO_F_ACCESS_PLATFORM;
> by default, flag on powerpc.
Uh... we haven't yet, though we're working on it.
> We would like to enable secure guests on powerpc without this flag
> aswell enabled, but past experience has educated us that its not a easy
> path. However if Halil makes some inroads in this path for s390, we
> will like to support him.
>
>
> RP
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On 2020/2/21 上午10:59, David Gibson wrote:
> On Thu, Feb 20, 2020 at 05:13:09PM +0100, Christoph Hellwig wrote:
>> On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index 867c7ebd3f10..fafc8f924955 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>>> if (!virtio_has_iommu_quirk(vdev))
>>> return true;
>>>
>>> + if (force_dma_unencrypted(&vdev->dev))
>>> + return true;
>> Hell no. This is a detail of the platform DMA direct implementation.
>> Drivers have no business looking at this flag, and virtio finally needs
>> to be fixed to use the DMA API properly for everything but legacy devices.
> So, this patch definitely isn't right as it stands, but I'm struggling
> to understand what it is you're saying is the right way.
>
> By "legacy devices" I assume you mean pre-virtio-1.0 devices, that
> lack the F_VERSION_1 feature flag. Is that right? Because I don't
> see how being a legacy device or not relates to use of the DMA API.
>
> I *think* what you are suggesting here is that virtio devices that
> have !F_IOMMU_PLATFORM should have their dma_ops set up so that the
> DMA API treats IOVA==PA, which will satisfy what the device expects.
Can this work for swiotlb?
Thanks
> Then the virtio driver can use the DMA API the same way for both
> F_IOMMU_PLATFORM and !F_IOMMU_PLATFORM devices.
>
> But if that works for !F_IOMMU_PLATFORM_DEVICES+F_VERSION_1 devices,
> then AFAICT it will work equally well for legacy devices.
>
> Using the DMA API for *everything* in virtio, legacy or not, seems
> like a reasonable approach to me. But, AFAICT, that does require the
> DMA layer to have some kind of explicit call to turn on this
> behaviour, which the virtio driver would call during initializsation.
> I don't think we can do it 100% within the DMA layer, because only the
> driver can reasonably know when a device has this weird non-standard
> DMA behaviour.
>
On Thu, 20 Feb 2020 15:55:14 -0500
"Michael S. Tsirkin" <[email protected]> wrote:
> On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
> > Currently the advanced guest memory protection technologies (AMD SEV,
> > powerpc secure guest technology and s390 Protected VMs) abuse the
> > VIRTIO_F_IOMMU_PLATFORM flag to make virtio core use the DMA API, which
> > is in turn necessary, to make IO work with guest memory protection.
> >
> > But VIRTIO_F_IOMMU_PLATFORM a.k.a. VIRTIO_F_ACCESS_PLATFORM is really a
> > different beast: with virtio devices whose implementation runs on an SMP
> > CPU we are still fine with doing all the usual optimizations, it is just
> > that we need to make sure that the memory protection mechanism does not
> > get in the way. The VIRTIO_F_ACCESS_PLATFORM mandates more work on the
> > side of the guest (and possibly he host side as well) than we actually
> > need.
> >
> > An additional benefit of teaching the guest to make the right decision
> > (and use DMA API) on it's own is: removing the need, to mandate special
> > VM configuration for guests that may run with protection. This is
> > especially interesting for s390 as VIRTIO_F_IOMMU_PLATFORM pushes all
> > the virtio control structures into the first 2G of guest memory:
> > something we don't necessarily want to do per-default.
> >
> > Signed-off-by: Halil Pasic <[email protected]>
> > Tested-by: Ram Pai <[email protected]>
> > Tested-by: Michael Mueller <[email protected]>
>
> This might work for you but it's fragile, since without
> VIRTIO_F_ACCESS_PLATFORM hypervisor assumes it gets
> GPA's, not DMA addresses.
>
Thanks for your constructive approach. I do want the hypervisor to
assume it gets GPA's. My train of thought was that the guys that need
to use IOVA's that are not GPA's when force_dma_unencrypted() will have
to to specify VIRTIO_F_ACCESS_PLATFORM (at the device) anyway, because
otherwise it won't work. But I see your point: in case of a
mis-configuration and provided the DMA API returns IOVA's one could end
up trying to touch wrong memory locations. But this should be similar to
what would happen if DMA ops are not used, and memory is not made accessible.
>
>
> IOW this looks like another iteration of:
>
> virtio: Support encrypted memory on powerpc secure guests
>
> which I was under the impression was abandoned as unnecessary.
Unnecessary for powerpc because they do normal PCI. In the context of
CCW there are only guest physical addresses (CCW I/O has no concept of
IOMMU or IOVAs).
>
>
> To summarize, the necessary conditions for a hack along these lines
> (using DMA API without VIRTIO_F_ACCESS_PLATFORM) are that we detect that:
>
> - secure guest mode is enabled - so we know that since we don't share
> most memory regular virtio code won't
> work, even though the buggy hypervisor didn't set VIRTIO_F_ACCESS_PLATFORM
force_dma_unencrypted(&vdev->dev) is IMHO exactly about this.
> - DMA API is giving us addresses that are actually also physical
> addresses
In case of s390 this is given. I talked with the power people before
posting this, and they ensured me they can are willing to deal with
this. I was hoping to talk abut this with the AMD SEV people here (hence
the cc).
> - Hypervisor is buggy and didn't enable VIRTIO_F_ACCESS_PLATFORM
>
I don't get this point. The argument where the hypervisor is buggy is a
bit hard to follow for me. If hypervisor is buggy we have already lost
anyway most of the time, or?
> I don't see how this patch does this.
I do get your point. I don't know of a good way to check that DMA API
is giving us addresses that are actually physical addresses, and the
situation you describe definitely has some risk to it.
Let me comment on other ideas that came up. I would be very happy to go
with the best one. Thank you very much.
Regards,
Halil
>
>
> > ---
> > drivers/virtio/virtio_ring.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 867c7ebd3f10..fafc8f924955 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> > if (!virtio_has_iommu_quirk(vdev))
> > return true;
> >
> > + if (force_dma_unencrypted(&vdev->dev))
> > + return true;
> > +
> > /* Otherwise, we are left to guess. */
> > /*
> > * In theory, it's possible to have a buggy QEMU-supposed
> > --
> > 2.17.1
>
On Fri, 21 Feb 2020 13:59:15 +1100
David Gibson <[email protected]> wrote:
> On Thu, Feb 20, 2020 at 05:13:09PM +0100, Christoph Hellwig wrote:
> > On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 867c7ebd3f10..fafc8f924955 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> > > if (!virtio_has_iommu_quirk(vdev))
> > > return true;
> > >
> > > + if (force_dma_unencrypted(&vdev->dev))
> > > + return true;
> >
> > Hell no. This is a detail of the platform DMA direct implementation.
> > Drivers have no business looking at this flag, and virtio finally needs
> > to be fixed to use the DMA API properly for everything but legacy devices.
>
> So, this patch definitely isn't right as it stands, but I'm struggling
> to understand what it is you're saying is the right way.
>
> By "legacy devices" I assume you mean pre-virtio-1.0 devices, that
> lack the F_VERSION_1 feature flag. Is that right? Because I don't
> see how being a legacy device or not relates to use of the DMA API.
>
> I *think* what you are suggesting here is that virtio devices that
> have !F_IOMMU_PLATFORM should have their dma_ops set up so that the
> DMA API treats IOVA==PA, which will satisfy what the device expects.
> Then the virtio driver can use the DMA API the same way for both
> F_IOMMU_PLATFORM and !F_IOMMU_PLATFORM devices.
I've considered this idea, and as a matter a fact would be my preferred
solution. It would be, if we could use GFP_DMA when allocating coherent
memory through the DMA API. For CCW devices we *must* have some of the
device accessible memory 31 bit addressable (in physical address space),
because the s390 architecture *mandates it*. I had patches
(https://lkml.org/lkml/2019/9/23/313) that do that but Christoph was not
open to the idea.
Always pushing all the stuff allocated by virtio devices into the DMA
zone is not a good option for us. We did that for protected guests,
because that was what Christoph was willing to accept, but not happy
with it.
I'm willing to go down that rute, but I really need the capability to
tell the DMA API 'do this allocation from ZONE_DMA'. I don't care how.
>
> But if that works for !F_IOMMU_PLATFORM_DEVICES+F_VERSION_1 devices,
> then AFAICT it will work equally well for legacy devices.
I agree.
>
> Using the DMA API for *everything* in virtio, legacy or not, seems
> like a reasonable approach to me. But, AFAICT, that does require the
> DMA layer to have some kind of explicit call to turn on this
> behaviour, which the virtio driver would call during initializsation.
> I don't think we can do it 100% within the DMA layer, because only the
> driver can reasonably know when a device has this weird non-standard
> DMA behaviour.
>
IMHO one could make the DMA API work with different DMA ops. The
virtio transport or the virtio core can probably intervene. But then, we
(virtio-ccw) would need the iommu=off DMA ops to do the bounce buffering
iff force_dma_unencrypted().
Regards,
Halil
On Fri, 21 Feb 2020 11:41:57 +0800
Jason Wang <[email protected]> wrote:
> > I *think* what you are suggesting here is that virtio devices that
> > have !F_IOMMU_PLATFORM should have their dma_ops set up so that the
> > DMA API treats IOVA==PA, which will satisfy what the device expects.
>
>
> Can this work for swiotlb?
It works on s390. I guess it would be the responsibility of however
provides the dma ops for the virtio device to ensure that if
!F_IOMMU_PLATFORM the addresses are GPA like *mandated* by the VIRTIO
specification.
Regards,
Halil
On Thu, 20 Feb 2020 17:13:09 +0100
Christoph Hellwig <[email protected]> wrote:
> On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 867c7ebd3f10..fafc8f924955 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> > if (!virtio_has_iommu_quirk(vdev))
> > return true;
> >
> > + if (force_dma_unencrypted(&vdev->dev))
> > + return true;
>
> Hell no. This is a detail of the platform DMA direct implementation.
I beg to differ. If it was a detail of the DMA direct implementation, it
should have/would have been private to kernel/dma/direct.c.
A look at
$ git grep -e force_dma_unencrypted
arch/powerpc/include/asm/mem_encrypt.h:static inline bool force_dma_unencrypted(struct device *dev)
arch/s390/mm/init.c:bool force_dma_unencrypted(struct device *dev)
arch/x86/mm/mem_encrypt.c:bool force_dma_unencrypted(struct device *dev)
include/linux/dma-direct.h:bool force_dma_unencrypted(struct device *dev);
include/linux/dma-direct.h:static inline bool force_dma_unencrypted(struct device *dev)
kernel/dma/direct.c: if (force_dma_unencrypted(dev))
kernel/dma/direct.c: if (force_dma_unencrypted(dev))
kernel/dma/direct.c: !force_dma_unencrypted(dev)) {
kernel/dma/direct.c: if (force_dma_unencrypted(dev))
kernel/dma/direct.c: if (force_dma_unencrypted(dev))
kernel/dma/direct.c: !force_dma_unencrypted(dev)) {
kernel/dma/direct.c: if (force_dma_unencrypted(dev))
tells you, that force_dma_unencrypted() is *consumed* by dma direct, but
*provided* by the memory encryption or memory management code.
I.e. platform code tells the dma (direct) code what decisions to make
under certain circumstances.
Consider what would we have to do to make PCI devices do I/O trough
pages that were shared when the guest is running in a protected VM. The
s390_pci_dma_ops would also need to know whether to 'force dma uencrypted'
or not, and it's the exact same logic. I doubt simply using DMA direct
for zPCI would do, because we still have to do all the Z specific IOMMU
management.
> Drivers have no business looking at this flag, and virtio finally needs
> to be fixed to use the DMA API properly for everything but legacy devices.
See the follow on discussion with David Gibson. In short: I'm in favor
of always using DMA API iff we keep conformance with the VIRTIO spec and
if it does not imply any degradations for s390 (virtio-ccw), or any
other regressions.
>
> No amount of desparate hacks is going to fix that fundamental problem,
> and I'm starting to get really sick and tired of all the crap patches
> published in this area.
I don't think I deserve the offensive language.
AFAIU you have a positive attitude towards the idea, that
!F_VIRTIO_PLATFORM implies 'no DMA API is used by virtio'
should be scrapped.
I would like to accomplish that without adverse effects to virtio-ccw
(because caring for virtio-ccw is a part of job description).
Regards,
Halil
On 2/21/20 7:12 AM, Halil Pasic wrote:
> On Thu, 20 Feb 2020 15:55:14 -0500
> "Michael S. Tsirkin" <[email protected]> wrote:
>
>> On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
>>> Currently the advanced guest memory protection technologies (AMD SEV,
>>> powerpc secure guest technology and s390 Protected VMs) abuse the
>>> VIRTIO_F_IOMMU_PLATFORM flag to make virtio core use the DMA API, which
>>> is in turn necessary, to make IO work with guest memory protection.
>>>
>>> But VIRTIO_F_IOMMU_PLATFORM a.k.a. VIRTIO_F_ACCESS_PLATFORM is really a
>>> different beast: with virtio devices whose implementation runs on an SMP
>>> CPU we are still fine with doing all the usual optimizations, it is just
>>> that we need to make sure that the memory protection mechanism does not
>>> get in the way. The VIRTIO_F_ACCESS_PLATFORM mandates more work on the
>>> side of the guest (and possibly he host side as well) than we actually
>>> need.
>>>
>>> An additional benefit of teaching the guest to make the right decision
>>> (and use DMA API) on it's own is: removing the need, to mandate special
>>> VM configuration for guests that may run with protection. This is
>>> especially interesting for s390 as VIRTIO_F_IOMMU_PLATFORM pushes all
>>> the virtio control structures into the first 2G of guest memory:
>>> something we don't necessarily want to do per-default.
>>>
>>> Signed-off-by: Halil Pasic <[email protected]>
>>> Tested-by: Ram Pai <[email protected]>
>>> Tested-by: Michael Mueller <[email protected]>
>>
>> This might work for you but it's fragile, since without
>> VIRTIO_F_ACCESS_PLATFORM hypervisor assumes it gets
>> GPA's, not DMA addresses.
>>
>
> Thanks for your constructive approach. I do want the hypervisor to
> assume it gets GPA's. My train of thought was that the guys that need
> to use IOVA's that are not GPA's when force_dma_unencrypted() will have
> to to specify VIRTIO_F_ACCESS_PLATFORM (at the device) anyway, because
> otherwise it won't work. But I see your point: in case of a
> mis-configuration and provided the DMA API returns IOVA's one could end
> up trying to touch wrong memory locations. But this should be similar to
> what would happen if DMA ops are not used, and memory is not made accessible.
>
>>
>>
>> IOW this looks like another iteration of:
>>
>> virtio: Support encrypted memory on powerpc secure guests
>>
>> which I was under the impression was abandoned as unnecessary.
>
> Unnecessary for powerpc because they do normal PCI. In the context of
> CCW there are only guest physical addresses (CCW I/O has no concept of
> IOMMU or IOVAs).
>
>>
>>
>> To summarize, the necessary conditions for a hack along these lines
>> (using DMA API without VIRTIO_F_ACCESS_PLATFORM) are that we detect that:
>>
>> - secure guest mode is enabled - so we know that since we don't share
>> most memory regular virtio code won't
>> work, even though the buggy hypervisor didn't set VIRTIO_F_ACCESS_PLATFORM
>
> force_dma_unencrypted(&vdev->dev) is IMHO exactly about this.
>
>> - DMA API is giving us addresses that are actually also physical
>> addresses
>
> In case of s390 this is given. I talked with the power people before
> posting this, and they ensured me they can are willing to deal with
> this. I was hoping to talk abut this with the AMD SEV people here (hence
> the cc).
Yes, physical addresses are fine for SEV - the key is that the DMA API is
used so that an address for unencrypted, or shared, memory is returned.
E.g. for a dma_alloc_coherent() call this is an allocation that has had
set_memory_decrypted() called or for a dma_map_page() call this is an
address from SWIOTLB, which was mapped shared during boot, where the data
will be bounce-buffered.
We don't currently support an emulated IOMMU in our SEV guest because that
would require a lot of support in the driver to make IOMMU data available
to the hypervisor (I/O page tables, etc.). We would need hardware support
to really make this work easily in the guest.
Thanks,
Tom
>
>> - Hypervisor is buggy and didn't enable VIRTIO_F_ACCESS_PLATFORM
>>
>
> I don't get this point. The argument where the hypervisor is buggy is a
> bit hard to follow for me. If hypervisor is buggy we have already lost
> anyway most of the time, or?
>
>> I don't see how this patch does this.
>
> I do get your point. I don't know of a good way to check that DMA API
> is giving us addresses that are actually physical addresses, and the
> situation you describe definitely has some risk to it.
>
> Let me comment on other ideas that came up. I would be very happy to go
> with the best one. Thank you very much.
>
> Regards,
> Halil
>
>>
>>
>>> ---
>>> drivers/virtio/virtio_ring.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index 867c7ebd3f10..fafc8f924955 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>>> if (!virtio_has_iommu_quirk(vdev))
>>> return true;
>>>
>>> + if (force_dma_unencrypted(&vdev->dev))
>>> + return true;
>>> +
>>> /* Otherwise, we are left to guess. */
>>> /*
>>> * In theory, it's possible to have a buggy QEMU-supposed
>>> --
>>> 2.17.1
>>
>
On Fri, Feb 21, 2020 at 02:12:30PM +0100, Halil Pasic wrote:
> On Thu, 20 Feb 2020 15:55:14 -0500
> "Michael S. Tsirkin" <[email protected]> wrote:
>
> > On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
> > > Currently the advanced guest memory protection technologies (AMD SEV,
> > > powerpc secure guest technology and s390 Protected VMs) abuse the
> > > VIRTIO_F_IOMMU_PLATFORM flag to make virtio core use the DMA API, which
> > > is in turn necessary, to make IO work with guest memory protection.
> > >
> > > But VIRTIO_F_IOMMU_PLATFORM a.k.a. VIRTIO_F_ACCESS_PLATFORM is really a
> > > different beast: with virtio devices whose implementation runs on an SMP
> > > CPU we are still fine with doing all the usual optimizations, it is just
> > > that we need to make sure that the memory protection mechanism does not
> > > get in the way. The VIRTIO_F_ACCESS_PLATFORM mandates more work on the
> > > side of the guest (and possibly he host side as well) than we actually
> > > need.
> > >
> > > An additional benefit of teaching the guest to make the right decision
> > > (and use DMA API) on it's own is: removing the need, to mandate special
> > > VM configuration for guests that may run with protection. This is
> > > especially interesting for s390 as VIRTIO_F_IOMMU_PLATFORM pushes all
> > > the virtio control structures into the first 2G of guest memory:
> > > something we don't necessarily want to do per-default.
> > >
> > > Signed-off-by: Halil Pasic <[email protected]>
> > > Tested-by: Ram Pai <[email protected]>
> > > Tested-by: Michael Mueller <[email protected]>
> >
> > This might work for you but it's fragile, since without
> > VIRTIO_F_ACCESS_PLATFORM hypervisor assumes it gets
> > GPA's, not DMA addresses.
> >
>
> Thanks for your constructive approach. I do want the hypervisor to
> assume it gets GPA's. My train of thought was that the guys that need
> to use IOVA's that are not GPA's when force_dma_unencrypted() will have
> to to specify VIRTIO_F_ACCESS_PLATFORM (at the device) anyway, because
> otherwise it won't work. But I see your point: in case of a
> mis-configuration and provided the DMA API returns IOVA's one could end
> up trying to touch wrong memory locations. But this should be similar to
> what would happen if DMA ops are not used, and memory is not made accessible.
>
> >
> >
> > IOW this looks like another iteration of:
> >
> > virtio: Support encrypted memory on powerpc secure guests
> >
> > which I was under the impression was abandoned as unnecessary.
>
> Unnecessary for powerpc because they do normal PCI. In the context of
> CCW there are only guest physical addresses (CCW I/O has no concept of
> IOMMU or IOVAs).
>
> >
> >
> > To summarize, the necessary conditions for a hack along these lines
> > (using DMA API without VIRTIO_F_ACCESS_PLATFORM) are that we detect that:
> >
> > - secure guest mode is enabled - so we know that since we don't share
> > most memory regular virtio code won't
> > work, even though the buggy hypervisor didn't set VIRTIO_F_ACCESS_PLATFORM
>
> force_dma_unencrypted(&vdev->dev) is IMHO exactly about this.
>
> > - DMA API is giving us addresses that are actually also physical
> > addresses
>
> In case of s390 this is given.
> I talked with the power people before
> posting this, and they ensured me they can are willing to deal with
> this. I was hoping to talk abut this with the AMD SEV people here (hence
> the cc).
We'd need a part of DMA API that promises this though. Platform
maintainers aren't going to go out of their way to do the
right thing just for virtio, and I can't track all arches
to make sure they don't violate virtio requirements.
>
> > - Hypervisor is buggy and didn't enable VIRTIO_F_ACCESS_PLATFORM
> >
>
> I don't get this point. The argument where the hypervisor is buggy is a
> bit hard to follow for me. If hypervisor is buggy we have already lost
> anyway most of the time, or?
If VIRTIO_F_ACCESS_PLATFORM is set then things just work. If
VIRTIO_F_ACCESS_PLATFORM is clear device is supposed to have access to
all of memory. You can argue in various ways but it's easier to just
declare a behaviour that violates this a bug. Which might still be worth
working around, for various reasons.
> > I don't see how this patch does this.
>
> I do get your point. I don't know of a good way to check that DMA API
> is giving us addresses that are actually physical addresses, and the
> situation you describe definitely has some risk to it.
One way would be to extend the DMA API with such an API.
Another would be to make virtio always use DMA API
and hide the logic in there.
This second approach is not easy, in particular since DMA API adds
a bunch of overhead which we need to find ways to
measure and mitigate.
>
> Let me comment on other ideas that came up. I would be very happy to go
> with the best one. Thank you very much.
>
> Regards,
> Halil
>
> >
> >
> > > ---
> > > drivers/virtio/virtio_ring.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 867c7ebd3f10..fafc8f924955 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> > > if (!virtio_has_iommu_quirk(vdev))
> > > return true;
> > >
> > > + if (force_dma_unencrypted(&vdev->dev))
> > > + return true;
> > > +
> > > /* Otherwise, we are left to guess. */
> > > /*
> > > * In theory, it's possible to have a buggy QEMU-supposed
> > > --
> > > 2.17.1
> >
On Fri, Feb 21, 2020 at 10:56:45AM -0500, Michael S. Tsirkin wrote:
> > > - DMA API is giving us addresses that are actually also physical
> > > addresses
> >
> > In case of s390 this is given.
> > I talked with the power people before
> > posting this, and they ensured me they can are willing to deal with
> > this. I was hoping to talk abut this with the AMD SEV people here (hence
> > the cc).
>
> We'd need a part of DMA API that promises this though. Platform
> maintainers aren't going to go out of their way to do the
> right thing just for virtio, and I can't track all arches
> to make sure they don't violate virtio requirements.
There is no way the DMA API is going to promise that you. All kinds
of platforms have offsets to DMA, and they are communicated through
interfaces like the device tree or ACPI. But the good thing is that
your hypervisors is in control of that, neither the DMA API nor
virtio have any business to interfer with that. In fact for harware
virtio device we might need such an offset for them to work on
lots of SOCs.
On Fri, Feb 21, 2020 at 01:59:15PM +1100, David Gibson wrote:
> > Hell no. This is a detail of the platform DMA direct implementation.
> > Drivers have no business looking at this flag, and virtio finally needs
> > to be fixed to use the DMA API properly for everything but legacy devices.
>
> So, this patch definitely isn't right as it stands, but I'm struggling
> to understand what it is you're saying is the right way.
>
> By "legacy devices" I assume you mean pre-virtio-1.0 devices, that
> lack the F_VERSION_1 feature flag. Is that right? Because I don't
> see how being a legacy device or not relates to use of the DMA API.
No. "legacy" is anything that does not set F_ACCESS_PLATFORM.
> I *think* what you are suggesting here is that virtio devices that
> have !F_IOMMU_PLATFORM should have their dma_ops set up so that the
> DMA API treats IOVA==PA, which will satisfy what the device expects.
> Then the virtio driver can use the DMA API the same way for both
> F_IOMMU_PLATFORM and !F_IOMMU_PLATFORM devices.
No. Those should just keep using the existing legacy non-dma ops
case and be done with it. No changes to that and most certainly
no new features.
On Fri, Feb 21, 2020 at 03:33:40PM +0100, Halil Pasic wrote:
> > Hell no. This is a detail of the platform DMA direct implementation.
>
> I beg to differ. If it was a detail of the DMA direct implementation, it
> should have/would have been private to kernel/dma/direct.c.
It can't given that platforms have to implement it. It is an arch hook
for dma-direct.
> Consider what would we have to do to make PCI devices do I/O trough
> pages that were shared when the guest is running in a protected VM. The
> s390_pci_dma_ops would also need to know whether to 'force dma uencrypted'
> or not, and it's the exact same logic. I doubt simply using DMA direct
> for zPCI would do, because we still have to do all the Z specific IOMMU
> management.
And your IOMMU can't deal with the encryption bit? In the case we
could think of allowing IOMMU implementation to access it. But the
point that it is an internal detail of the DMA implementation and by
now means for drivers.
On Fri, 21 Feb 2020 10:56:45 -0500
"Michael S. Tsirkin" <[email protected]> wrote:
> On Fri, Feb 21, 2020 at 02:12:30PM +0100, Halil Pasic wrote:
> > On Thu, 20 Feb 2020 15:55:14 -0500
> > "Michael S. Tsirkin" <[email protected]> wrote:
[..]
> > > To summarize, the necessary conditions for a hack along these lines
> > > (using DMA API without VIRTIO_F_ACCESS_PLATFORM) are that we detect that:
> > >
> > > - secure guest mode is enabled - so we know that since we don't share
> > > most memory regular virtio code won't
> > > work, even though the buggy hypervisor didn't set VIRTIO_F_ACCESS_PLATFORM
> >
> > force_dma_unencrypted(&vdev->dev) is IMHO exactly about this.
> >
> > > - DMA API is giving us addresses that are actually also physical
> > > addresses
> >
> > In case of s390 this is given.
> > I talked with the power people before
> > posting this, and they ensured me they can are willing to deal with
> > this. I was hoping to talk abut this with the AMD SEV people here (hence
> > the cc).
>
> We'd need a part of DMA API that promises this though. Platform
> maintainers aren't going to go out of their way to do the
> right thing just for virtio, and I can't track all arches
> to make sure they don't violate virtio requirements.
>
One would have to track only the arches that have
force_dma_unecrypted(), and generally speaking the arches shall make
sure the DMA ops are suitable, whatever that means in the given context.
> >
> > > - Hypervisor is buggy and didn't enable VIRTIO_F_ACCESS_PLATFORM
> > >
> >
> > I don't get this point. The argument where the hypervisor is buggy is a
> > bit hard to follow for me. If hypervisor is buggy we have already lost
> > anyway most of the time, or?
>
> If VIRTIO_F_ACCESS_PLATFORM is set then things just work. If
> VIRTIO_F_ACCESS_PLATFORM is clear device is supposed to have access to
> all of memory. You can argue in various ways but it's easier to just
> declare a behaviour that violates this a bug. Which might still be worth
> working around, for various reasons.
>
I don't agree. The spec explicitly states "If this feature bit is set
to 0, then the device has same access to memory addresses supplied to it
as the driver has." That ain't any guest memory, but the addresses
supplied to it. BTW this is how channel I/O works as well: the channel
program authorizes the device to use the memory locations specified by
the channel program, for as long as the channel program is executed.
That's how channel I/O does isolation without an architected IOMMU.
Can you please show me the words in the specification that say, the
device has to have access to the entire guest memory all the time?
Maybe I have to understand all the intentions behind
VIRTIO_F_ACCESS_PLATFORM better. I've read the spec several times, but I
still have the feeling, when we discuss, that I didn't get it right.
IOMMUs and PCI style DMA are unfortunately not my bread and butter.
I only know that the devices do not need any new device capability (I
assume VIRTIO_F_ACCESS_PLATFORM does express a device capability), to
work with protected virtualization. Unless one defines
VIRTIO_F_ACCESS_PLATFORM is the capability that the device won't poke
*arbitrary* guest memory. From that perspective mandating the flag
feels wrong. (CCW devices are never allowed to poke arbitrary
memory.)
Yet, if VIRTIO_F_ACCESS_PLATFORM is a flag that every nice and modern
virtio device should have (i.e. a lack of it means kida broken), then I
have the feeling virtio-ccw should probably evolve in the direction, that
having VIRTIO_F_ACCESS_PLATFORM set does not hurt.
I have to think some more.
>
> > > I don't see how this patch does this.
> >
> > I do get your point. I don't know of a good way to check that DMA API
> > is giving us addresses that are actually physical addresses, and the
> > situation you describe definitely has some risk to it.
>
> One way would be to extend the DMA API with such an API.
Seems Christoph does not like that idea.
>
> Another would be to make virtio always use DMA API
> and hide the logic in there.
I thought Christoph wants that, but I was wrong.
> This second approach is not easy, in particular since DMA API adds
> a bunch of overhead which we need to find ways to
> measure and mitigate.
>
Agreed. From s390 perspective, I think it ain't to bad if we get GFP_DMA.
Many thanks for your patience!
Halil
[..]
On Fri, 21 Feb 2020 17:39:38 +0100
Christoph Hellwig <[email protected]> wrote:
> On Fri, Feb 21, 2020 at 03:33:40PM +0100, Halil Pasic wrote:
> > > Hell no. This is a detail of the platform DMA direct implementation.
> >
> > I beg to differ. If it was a detail of the DMA direct implementation, it
> > should have/would have been private to kernel/dma/direct.c.
>
> It can't given that platforms have to implement it. It is an arch hook
> for dma-direct.
>
> > Consider what would we have to do to make PCI devices do I/O trough
> > pages that were shared when the guest is running in a protected VM. The
> > s390_pci_dma_ops would also need to know whether to 'force dma uencrypted'
> > or not, and it's the exact same logic. I doubt simply using DMA direct
> > for zPCI would do, because we still have to do all the Z specific IOMMU
> > management.
>
> And your IOMMU can't deal with the encryption bit?
There is no encrypt bit, and our memory is not encrypted, but protected.
Means e.g. when buggy/malicious hypervisor tries to read a protected
page it wont get ciphertext, but a slap on its finger. In order do make
memory accessible to the hypervisor (or another guest, or a real device)
the guest must make a so called utlravisor call (talk to the firmware)
and share the respective page.
We tapped into the memory encryption infrastructure, because both is
protecting the guest memory form the host (just by different means), and
because it made no sense to build up something in parallel when most of
the stuff we need was already there. But most unfortunately the names
are deceiving when it comes to s390 protected virtualization and it's
guest I/O enablement.
> In the case we
> could think of allowing IOMMU implementation to access it. But the
> point that it is an internal detail of the DMA implementation and by
> now means for drivers.
From the perspective, that any driver that does anything remotely DMAish,
that is, some external entity (possibly a hypervisor, possibly a channel
subsystem, possibly a DMA controller) to should the memory, should do
DMA API first, to make sure, the DMAish goes well, your argument makes
perfect sense. But form that perspective !F_ACCESS_PLATFORM is also a
DMAish. And the virtio spec mandates that !F_ACCESS_PLATFORM implies
GPA's.
For virtio-ccw I want GPA's and not IOVA's on s390, for virtio-pci,
which we also support in general but not with protected virtualization,
well, it's a different story.
With protected visualization however I must make sure all I/O goes
through shared pages. We use swiotlb for that. But then the old
infrastructure won't cut it. Jet we still need GPA's on the ring (with
the extra requirement that the page must be shared).
DMA API is a nice fit there because we can allocate DMA coherent memory
(such that what comes back from our DMA ops is a GPA), so we have shared
memory that the hypervisor and the guest is allowed to look at
concurrently, and for the buffers that are going to be put on the vring,
we can use the streaming API, which uses bounce buffers. The returned
IOVA (in DMA API speak) is a GPA of the bounce buffer, and the guest is
not allowed to peek until it unmaps, so everything is cozy. But for that
to work, we all (AMD SEV, power, and s390) must go through the DMA API,
because the old infrastructure in virtio core simply won't cut it. And
it has nothing to do with the device. David explained it very well.
My series is about controlling virtio-core's usage of DMA API. I believe,
I did it in a way that doesn't hurt any arch at the moment.
Maybe the conflict can be resolved if the transport gets a say in
whether to use the DMA API or not. In the end the VIRTIO spec does say
that "Whether accesses are actually limited or translated is described
by platform-specific means."
Regards,
Halil
On Fri, Feb 21, 2020 at 03:33:40PM +0100, Halil Pasic wrote:
> AFAIU you have a positive attitude towards the idea, that
> !F_VIRTIO_PLATFORM implies 'no DMA API is used by virtio'
> should be scrapped.
>
> I would like to accomplish that without adverse effects to virtio-ccw
> (because caring for virtio-ccw is a part of job description).
>
> Regards,
> Halil
It is possible, in theory. IIRC the main challenge is that DMA API
has overhead of indirect function calls even when all it
does it return back the PA without changes. So that will lead to
a measureable performance degradation. That might be fixable,
possibly using some kind of logic along the lines of
if (iova is pa)
return pa
else
indirect call
and for unmapping, we might need an API that says "unmap
is a nop, safe to skip" so we don't maintain the
dma address until unmap.
--
MST
On Fri, Feb 21, 2020 at 09:39:38AM -0600, Tom Lendacky wrote:
> On 2/21/20 7:12 AM, Halil Pasic wrote:
> > On Thu, 20 Feb 2020 15:55:14 -0500
> > "Michael S. Tsirkin" <[email protected]> wrote:
> >
> >> On Thu, Feb 20, 2020 at 05:06:06PM +0100, Halil Pasic wrote:
> >>> Currently the advanced guest memory protection technologies (AMD SEV,
> >>> powerpc secure guest technology and s390 Protected VMs) abuse the
> >>> VIRTIO_F_IOMMU_PLATFORM flag to make virtio core use the DMA API, which
> >>> is in turn necessary, to make IO work with guest memory protection.
> >>>
> >>> But VIRTIO_F_IOMMU_PLATFORM a.k.a. VIRTIO_F_ACCESS_PLATFORM is really a
> >>> different beast: with virtio devices whose implementation runs on an SMP
> >>> CPU we are still fine with doing all the usual optimizations, it is just
> >>> that we need to make sure that the memory protection mechanism does not
> >>> get in the way. The VIRTIO_F_ACCESS_PLATFORM mandates more work on the
> >>> side of the guest (and possibly he host side as well) than we actually
> >>> need.
> >>>
> >>> An additional benefit of teaching the guest to make the right decision
> >>> (and use DMA API) on it's own is: removing the need, to mandate special
> >>> VM configuration for guests that may run with protection. This is
> >>> especially interesting for s390 as VIRTIO_F_IOMMU_PLATFORM pushes all
> >>> the virtio control structures into the first 2G of guest memory:
> >>> something we don't necessarily want to do per-default.
> >>>
> >>> Signed-off-by: Halil Pasic <[email protected]>
> >>> Tested-by: Ram Pai <[email protected]>
> >>> Tested-by: Michael Mueller <[email protected]>
> >>
> >> This might work for you but it's fragile, since without
> >> VIRTIO_F_ACCESS_PLATFORM hypervisor assumes it gets
> >> GPA's, not DMA addresses.
> >>
> >
> > Thanks for your constructive approach. I do want the hypervisor to
> > assume it gets GPA's. My train of thought was that the guys that need
> > to use IOVA's that are not GPA's when force_dma_unencrypted() will have
> > to to specify VIRTIO_F_ACCESS_PLATFORM (at the device) anyway, because
> > otherwise it won't work. But I see your point: in case of a
> > mis-configuration and provided the DMA API returns IOVA's one could end
> > up trying to touch wrong memory locations. But this should be similar to
> > what would happen if DMA ops are not used, and memory is not made accessible.
> >
> >>
> >>
> >> IOW this looks like another iteration of:
> >>
> >> virtio: Support encrypted memory on powerpc secure guests
> >>
> >> which I was under the impression was abandoned as unnecessary.
> >
> > Unnecessary for powerpc because they do normal PCI. In the context of
> > CCW there are only guest physical addresses (CCW I/O has no concept of
> > IOMMU or IOVAs).
> >
> >>
> >>
> >> To summarize, the necessary conditions for a hack along these lines
> >> (using DMA API without VIRTIO_F_ACCESS_PLATFORM) are that we detect that:
> >>
> >> - secure guest mode is enabled - so we know that since we don't share
> >> most memory regular virtio code won't
> >> work, even though the buggy hypervisor didn't set VIRTIO_F_ACCESS_PLATFORM
> >
> > force_dma_unencrypted(&vdev->dev) is IMHO exactly about this.
> >
> >> - DMA API is giving us addresses that are actually also physical
> >> addresses
> >
> > In case of s390 this is given. I talked with the power people before
> > posting this, and they ensured me they can are willing to deal with
> > this. I was hoping to talk abut this with the AMD SEV people here (hence
> > the cc).
>
> Yes, physical addresses are fine for SEV - the key is that the DMA API is
> used so that an address for unencrypted, or shared, memory is returned.
> E.g. for a dma_alloc_coherent() call this is an allocation that has had
> set_memory_decrypted() called or for a dma_map_page() call this is an
> address from SWIOTLB, which was mapped shared during boot, where the data
> will be bounce-buffered.
>
> We don't currently support an emulated IOMMU in our SEV guest because that
> would require a lot of support in the driver to make IOMMU data available
> to the hypervisor (I/O page tables, etc.). We would need hardware support
> to really make this work easily in the guest.
A tangent here: note that on POWER our IOMMU is paravirtualized
(updated with hypercalls), it's also always enabled. For that reason
we can and do combine vIOMMU translation with the need for bounce
buffering for secure guests.
(We generally statically configure the vIOMMU to have a huge window
which just maps GPAs 1-to-1, which means we can still use dma-direct,
but the vIOMMU is still there from the platform point of view)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Fri, Feb 21, 2020 at 05:36:45PM +0100, Christoph Hellwig wrote:
> On Fri, Feb 21, 2020 at 01:59:15PM +1100, David Gibson wrote:
> > > Hell no. This is a detail of the platform DMA direct implementation.
> > > Drivers have no business looking at this flag, and virtio finally needs
> > > to be fixed to use the DMA API properly for everything but legacy devices.
> >
> > So, this patch definitely isn't right as it stands, but I'm struggling
> > to understand what it is you're saying is the right way.
> >
> > By "legacy devices" I assume you mean pre-virtio-1.0 devices, that
> > lack the F_VERSION_1 feature flag. Is that right? Because I don't
> > see how being a legacy device or not relates to use of the DMA API.
>
> No. "legacy" is anything that does not set F_ACCESS_PLATFORM.
Hm, I see.
The trouble is I think we can only reasonably call things "legacy"
when essentially all currently in use OSes have support for the new,
better way of doing things. That is, alas, not really the case for
F_ACCESS_PLATFORM.
> > I *think* what you are suggesting here is that virtio devices that
> > have !F_IOMMU_PLATFORM should have their dma_ops set up so that the
> > DMA API treats IOVA==PA, which will satisfy what the device expects.
> > Then the virtio driver can use the DMA API the same way for both
> > F_IOMMU_PLATFORM and !F_IOMMU_PLATFORM devices.
>
> No. Those should just keep using the existing legacy non-dma ops
> case and be done with it. No changes to that and most certainly
> no new features.
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Sat, Feb 22, 2020 at 02:07:58PM -0500, Michael S. Tsirkin wrote:
> On Fri, Feb 21, 2020 at 03:33:40PM +0100, Halil Pasic wrote:
> > AFAIU you have a positive attitude towards the idea, that
> > !F_VIRTIO_PLATFORM implies 'no DMA API is used by virtio'
> > should be scrapped.
> >
> > I would like to accomplish that without adverse effects to virtio-ccw
> > (because caring for virtio-ccw is a part of job description).
> >
> > Regards,
> > Halil
>
> It is possible, in theory. IIRC the main challenge is that DMA API
> has overhead of indirect function calls even when all it
> does it return back the PA without changes.
That overhead is gone now, the DMA direct calls are direct calls these
days.
On Fri, 21 Feb 2020 17:36:45 +0100
Christoph Hellwig <[email protected]> wrote:
> > By "legacy devices" I assume you mean pre-virtio-1.0 devices, that
> > lack the F_VERSION_1 feature flag. Is that right? Because I don't
> > see how being a legacy device or not relates to use of the DMA API.
>
> No. "legacy" is anything that does not set F_ACCESS_PLATFORM.
FYI in virtio-speak the term 'legacy devices' is already taken and it
ain't congruent with your intended semantics. Please look it up
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-60001
But with that understood your statement does provide insisting in how do
you see F_ACCESS_PLATFORM. I.e. something that should be enabled in
general, except for legacy reasons. But I'm afraid Michael sees it
differently: i.e. something that should be enabled when necessary, and
otherwise not (because it is likely to cost performance).
Regards,
Halil
On Wed, Oct 28, 2020 at 03:24:03PM +0100, Alexander Graf wrote:
> On 24.02.20 18:16, Christoph Hellwig wrote:
> > On Sat, Feb 22, 2020 at 02:07:58PM -0500, Michael S. Tsirkin wrote:
> > > On Fri, Feb 21, 2020 at 03:33:40PM +0100, Halil Pasic wrote:
> > > > AFAIU you have a positive attitude towards the idea, that
> > > > !F_VIRTIO_PLATFORM implies 'no DMA API is used by virtio'
> > > > should be scrapped.
> > > >
> > > > I would like to accomplish that without adverse effects to virtio-ccw
> > > > (because caring for virtio-ccw is a part of job description).
> > > >
> > > > Regards,
> > > > Halil
> > >
> > > It is possible, in theory. IIRC the main challenge is that DMA API
> > > has overhead of indirect function calls even when all it
> > > does it return back the PA without changes.
> >
> > That overhead is gone now, the DMA direct calls are direct calls these
> > days.
>
> Michael, would that mitigate your concerns to just always use the DMA API?
> If not, wouldn't it make sense to benchmark and pinpoint Christoph to paths
> that do slow things down, so we can finally move virtio into a world where
> it doesn't bypass the kernel DMA infrastructure.
>
>
> Alex
There's no specific concern really. One can in theory move the code
handling !VIRTIO_F_ACCESS_PLATFORM such that instead of having a branch
in virtio code, you instead override platform DMA API and then use DMA
API unconditionally.
The gain from that will probably be marginal, but maybe I'm wrong.
Let's see the patches.
We still need to do the override when !VIRTIO_F_ACCESS_PLATFORM,
according to spec. Encrypted hypervisors still need to set
VIRTIO_F_ACCESS_PLATFORM.
Is VIRTIO_F_ACCESS_PLATFORM a good default? Need to support
legacy guests does not allow that at the qemu level, we
need to be conservative there. But yes, if you have a very
modern guest then it might be a good idea to just always
enable VIRTIO_F_ACCESS_PLATFORM. I say might since unfortunately
most people only use VIRTIO_F_ACCESS_PLATFORM with
vIOMMU, using it without VIRTIO_F_ACCESS_PLATFORM is supported
but is a path less well trodden. Benchmarking that,
fixing issues that surface if any would be imho effort
well spent, and would be a prerequisite to making it
a default in a production system.
>
>
>
>
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
>
>