Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Wed, 10 Jun 2020 10:48:42 +0200
Cornelia Huck wrote:
> On Wed, 10 Jun 2020 14:39:22 +1000
> David Gibson wrote:
>
> > On Tue, Jun 09, 2020 at 12:16:41PM +0200, Cornelia Huck wrote:
> > > On Sun, 7 Jun 2020 13:07:35 +1000
> > > David Gibson wrote:
> > >
> > > > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > > > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> > > > > > The default behaviour for virtio devices is not to use the
> > > > > > platforms normal
> > > > > > DMA paths, but instead to use the fact that it's running in a
> > > > > > hypervisor
> > > > > > to directly access guest memory. That doesn't work if the guest's
> > > > > > memory
> > > > > > is protected from hypervisor access, such as with AMD's SEV or
> > > > > > POWER's PEF.
> > > > > >
> > > > > > So, if a guest memory protection mechanism is enabled, then apply
> > > > > > the
> > > > > > iommu_platform=on option so it will go through normal DMA
> > > > > > mechanisms.
> > > > > > Those will presumably have some way of marking memory as shared
> > > > > > with the
> > > > > > hypervisor or hardware so that DMA will work.
> > > > > >
> > > > > > Signed-off-by: David Gibson
> > > > > > ---
> > > > > > hw/core/machine.c | 11 +++
> > > > > > 1 file changed, 11 insertions(+)
> > > > > >
> > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > > index 88d699bceb..cb6580954e 100644
> > > > > > --- a/hw/core/machine.c
> > > > > > +++ b/hw/core/machine.c
> > > > > > @@ -28,6 +28,8 @@
> > > > > > #include "hw/mem/nvdimm.h"
> > > > > > #include "migration/vmstate.h"
> > > > > > #include "exec/guest-memory-protection.h"
> > > > > > +#include "hw/virtio/virtio.h"
> > > > > > +#include "hw/virtio/virtio-pci.h"
> > > > > >
> > > > > > GlobalProperty hw_compat_5_0[] = {};
> > > > > > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > > > > > @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState
> > > > > > *machine)
> > > > > > * areas.
> > > > > > */
> > > > > > machine_set_mem_merge(OBJECT(machine), false,
> > > > > > _abort);
> > > > > > +
> > > > > > +/*
> > > > > > + * Virtio devices can't count on directly accessing guest
> > > > > > + * memory, so they need iommu_platform=on to use normal DMA
> > > > > > + * mechanisms. That requires disabling legacy virtio
> > > > > > support
> > > > > > + * for virtio pci devices
> > > > > > + */
> > > > > > +object_register_sugar_prop(TYPE_VIRTIO_PCI,
> > > > > > "disable-legacy", "on");
> > > > > > +object_register_sugar_prop(TYPE_VIRTIO_DEVICE,
> > > > > > "iommu_platform", "on");
> > > > > > }
> > > > > >
> > > > >
> > > > > I think it's a reasonable way to address this overall.
> > > > > As Cornelia has commented, addressing ccw as well
> > > >
> > > > Sure. I was assuming somebody who actually knows ccw could do that as
> > > > a follow up.
> > >
> > > FWIW, I think we could simply enable iommu_platform for protected
> > > guests for ccw; no prereqs like pci's disable-legacy.
> >
> > Right, and the code above should in fact already do so, since it
> > applies that to TYPE_VIRTIO_DEVICE, which is common. The
> > disable-legacy part should be harmless for s390, since this is
> > effectively just setting a default, and we don't expect any
> > TYPE_VIRTIO_PCI devices to be instantiated on z.
>
> Well, virtio-pci is available on s390, so people could try to use it --
> however, forcing disable-legacy won't hurt in that case, as it won't
> make the situation worse (I don't expect virtio-pci to work on s390
> protected guests.)
Currently we don't have to worry about virtio-pci when running in
protected mode, because PCI is not supported currently, and this is
enforced by the ultravisor which would inject operation exceptions should
a PV guest try to use our PCI instructions.
[..]
pgpk_G0z1uGn5.pgp
Description: OpenPGP digital signature
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Wed, Jun 10, 2020 at 10:48:42AM +0200, Cornelia Huck wrote:
> On Wed, 10 Jun 2020 14:39:22 +1000
> David Gibson wrote:
>
> > On Tue, Jun 09, 2020 at 12:16:41PM +0200, Cornelia Huck wrote:
> > > On Sun, 7 Jun 2020 13:07:35 +1000
> > > David Gibson wrote:
> > >
> > > > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > > > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> > > > > > The default behaviour for virtio devices is not to use the
> > > > > > platforms normal
> > > > > > DMA paths, but instead to use the fact that it's running in a
> > > > > > hypervisor
> > > > > > to directly access guest memory. That doesn't work if the guest's
> > > > > > memory
> > > > > > is protected from hypervisor access, such as with AMD's SEV or
> > > > > > POWER's PEF.
> > > > > >
> > > > > > So, if a guest memory protection mechanism is enabled, then apply
> > > > > > the
> > > > > > iommu_platform=on option so it will go through normal DMA
> > > > > > mechanisms.
> > > > > > Those will presumably have some way of marking memory as shared
> > > > > > with the
> > > > > > hypervisor or hardware so that DMA will work.
> > > > > >
> > > > > > Signed-off-by: David Gibson
> > > > > > ---
> > > > > > hw/core/machine.c | 11 +++
> > > > > > 1 file changed, 11 insertions(+)
> > > > > >
> > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > > index 88d699bceb..cb6580954e 100644
> > > > > > --- a/hw/core/machine.c
> > > > > > +++ b/hw/core/machine.c
> > > > > > @@ -28,6 +28,8 @@
> > > > > > #include "hw/mem/nvdimm.h"
> > > > > > #include "migration/vmstate.h"
> > > > > > #include "exec/guest-memory-protection.h"
> > > > > > +#include "hw/virtio/virtio.h"
> > > > > > +#include "hw/virtio/virtio-pci.h"
> > > > > >
> > > > > > GlobalProperty hw_compat_5_0[] = {};
> > > > > > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > > > > > @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState
> > > > > > *machine)
> > > > > > * areas.
> > > > > > */
> > > > > > machine_set_mem_merge(OBJECT(machine), false,
> > > > > > _abort);
> > > > > > +
> > > > > > +/*
> > > > > > + * Virtio devices can't count on directly accessing guest
> > > > > > + * memory, so they need iommu_platform=on to use normal DMA
> > > > > > + * mechanisms. That requires disabling legacy virtio
> > > > > > support
> > > > > > + * for virtio pci devices
> > > > > > + */
> > > > > > +object_register_sugar_prop(TYPE_VIRTIO_PCI,
> > > > > > "disable-legacy", "on");
> > > > > > +object_register_sugar_prop(TYPE_VIRTIO_DEVICE,
> > > > > > "iommu_platform", "on");
> > > > > > }
> > > > > >
> > > > >
> > > > > I think it's a reasonable way to address this overall.
> > > > > As Cornelia has commented, addressing ccw as well
> > > >
> > > > Sure. I was assuming somebody who actually knows ccw could do that as
> > > > a follow up.
> > >
> > > FWIW, I think we could simply enable iommu_platform for protected
> > > guests for ccw; no prereqs like pci's disable-legacy.
> >
> > Right, and the code above should in fact already do so, since it
> > applies that to TYPE_VIRTIO_DEVICE, which is common. The
> > disable-legacy part should be harmless for s390, since this is
> > effectively just setting a default, and we don't expect any
> > TYPE_VIRTIO_PCI devices to be instantiated on z.
>
> Well, virtio-pci is available on s390, so people could try to use it --
> however, forcing disable-legacy won't hurt in that case, as it won't
> make the situation worse (I don't expect virtio-pci to work on s390
> protected guests.)
Sure, and if by whatever chance it does work, then you'll need
iommu_platform, and therefore disable-legacy for it.
> > > > > as cases where user has
> > > > > specified the property manually could be worth-while.
> > > >
> > > > I don't really see what's to be done there. I'm assuming that if the
> > > > user specifies it, they know what they're doing - particularly with
> > > > nonstandard guests there are some odd edge cases where those
> > > > combinations might work, they're just not very likely.
> > >
> > > If I understood Halil correctly, devices without iommu_platform
> > > apparently can crash protected guests on s390. Is that supposed to be a
> > > "if it breaks, you get to keep the pieces" situation, or do we really
> > > want to enforce iommu_platform?
> >
> > I actually think "if you broke it, keep the pieces" is an acceptable
> > approach here, but that doesn't preclude some further enforcement to
> > improve UX.
>
> I'm worried about spreading dealing with this over too many code areas,
> though.
--
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_!
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Wed, 10 Jun 2020 14:39:22 +1000
David Gibson wrote:
> On Tue, Jun 09, 2020 at 12:16:41PM +0200, Cornelia Huck wrote:
> > On Sun, 7 Jun 2020 13:07:35 +1000
> > David Gibson wrote:
> >
> > > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> > > > > The default behaviour for virtio devices is not to use the platforms
> > > > > normal
> > > > > DMA paths, but instead to use the fact that it's running in a
> > > > > hypervisor
> > > > > to directly access guest memory. That doesn't work if the guest's
> > > > > memory
> > > > > is protected from hypervisor access, such as with AMD's SEV or
> > > > > POWER's PEF.
> > > > >
> > > > > So, if a guest memory protection mechanism is enabled, then apply the
> > > > > iommu_platform=on option so it will go through normal DMA mechanisms.
> > > > > Those will presumably have some way of marking memory as shared with
> > > > > the
> > > > > hypervisor or hardware so that DMA will work.
> > > > >
> > > > > Signed-off-by: David Gibson
> > > > > ---
> > > > > hw/core/machine.c | 11 +++
> > > > > 1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index 88d699bceb..cb6580954e 100644
> > > > > --- a/hw/core/machine.c
> > > > > +++ b/hw/core/machine.c
> > > > > @@ -28,6 +28,8 @@
> > > > > #include "hw/mem/nvdimm.h"
> > > > > #include "migration/vmstate.h"
> > > > > #include "exec/guest-memory-protection.h"
> > > > > +#include "hw/virtio/virtio.h"
> > > > > +#include "hw/virtio/virtio-pci.h"
> > > > >
> > > > > GlobalProperty hw_compat_5_0[] = {};
> > > > > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > > > > @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState
> > > > > *machine)
> > > > > * areas.
> > > > > */
> > > > > machine_set_mem_merge(OBJECT(machine), false, _abort);
> > > > > +
> > > > > +/*
> > > > > + * Virtio devices can't count on directly accessing guest
> > > > > + * memory, so they need iommu_platform=on to use normal DMA
> > > > > + * mechanisms. That requires disabling legacy virtio support
> > > > > + * for virtio pci devices
> > > > > + */
> > > > > +object_register_sugar_prop(TYPE_VIRTIO_PCI,
> > > > > "disable-legacy", "on");
> > > > > +object_register_sugar_prop(TYPE_VIRTIO_DEVICE,
> > > > > "iommu_platform", "on");
> > > > > }
> > > > >
> > > >
> > > > I think it's a reasonable way to address this overall.
> > > > As Cornelia has commented, addressing ccw as well
> > >
> > > Sure. I was assuming somebody who actually knows ccw could do that as
> > > a follow up.
> >
> > FWIW, I think we could simply enable iommu_platform for protected
> > guests for ccw; no prereqs like pci's disable-legacy.
>
> Right, and the code above should in fact already do so, since it
> applies that to TYPE_VIRTIO_DEVICE, which is common. The
> disable-legacy part should be harmless for s390, since this is
> effectively just setting a default, and we don't expect any
> TYPE_VIRTIO_PCI devices to be instantiated on z.
Well, virtio-pci is available on s390, so people could try to use it --
however, forcing disable-legacy won't hurt in that case, as it won't
make the situation worse (I don't expect virtio-pci to work on s390
protected guests.)
>
> > > > as cases where user has
> > > > specified the property manually could be worth-while.
> > >
> > > I don't really see what's to be done there. I'm assuming that if the
> > > user specifies it, they know what they're doing - particularly with
> > > nonstandard guests there are some odd edge cases where those
> > > combinations might work, they're just not very likely.
> >
> > If I understood Halil correctly, devices without iommu_platform
> > apparently can crash protected guests on s390. Is that supposed to be a
> > "if it breaks, you get to keep the pieces" situation, or do we really
> > want to enforce iommu_platform?
>
> I actually think "if you broke it, keep the pieces" is an acceptable
> approach here, but that doesn't preclude some further enforcement to
> improve UX.
I'm worried about spreading dealing with this over too many code areas,
though.
pgpCjQAvhrL4P.pgp
Description: OpenPGP digital signature
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Tue, Jun 09, 2020 at 05:40:46PM +0200, Halil Pasic wrote:
> On Tue, 9 Jun 2020 12:16:41 +0200
> Cornelia Huck wrote:
>
> > On Sun, 7 Jun 2020 13:07:35 +1000
> > David Gibson wrote:
> >
> > > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> > > > > The default behaviour for virtio devices is not to use the platforms
> > > > > normal
> > > > > DMA paths, but instead to use the fact that it's running in a
> > > > > hypervisor
> > > > > to directly access guest memory. That doesn't work if the guest's
> > > > > memory
> > > > > is protected from hypervisor access, such as with AMD's SEV or
> > > > > POWER's PEF.
> > > > >
> > > > > So, if a guest memory protection mechanism is enabled, then apply the
> > > > > iommu_platform=on option so it will go through normal DMA mechanisms.
> > > > > Those will presumably have some way of marking memory as shared with
> > > > > the
> > > > > hypervisor or hardware so that DMA will work.
> > > > >
> > > > > Signed-off-by: David Gibson
> > > > > ---
> > > > > hw/core/machine.c | 11 +++
> > > > > 1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index 88d699bceb..cb6580954e 100644
> > > > > --- a/hw/core/machine.c
> > > > > +++ b/hw/core/machine.c
> > > > > @@ -28,6 +28,8 @@
> > > > > #include "hw/mem/nvdimm.h"
> > > > > #include "migration/vmstate.h"
> > > > > #include "exec/guest-memory-protection.h"
> > > > > +#include "hw/virtio/virtio.h"
> > > > > +#include "hw/virtio/virtio-pci.h"
> > > > >
> > > > > GlobalProperty hw_compat_5_0[] = {};
> > > > > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > > > > @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState
> > > > > *machine)
> > > > > * areas.
> > > > > */
> > > > > machine_set_mem_merge(OBJECT(machine), false, _abort);
> > > > > +
> > > > > +/*
> > > > > + * Virtio devices can't count on directly accessing guest
> > > > > + * memory, so they need iommu_platform=on to use normal DMA
> > > > > + * mechanisms. That requires disabling legacy virtio support
> > > > > + * for virtio pci devices
> > > > > + */
> > > > > +object_register_sugar_prop(TYPE_VIRTIO_PCI,
> > > > > "disable-legacy", "on");
> > > > > +object_register_sugar_prop(TYPE_VIRTIO_DEVICE,
> > > > > "iommu_platform", "on");
> > > > > }
> > > > >
> > > >
> > > > I think it's a reasonable way to address this overall.
> > > > As Cornelia has commented, addressing ccw as well
> > >
> > > Sure. I was assuming somebody who actually knows ccw could do that as
> > > a follow up.
> >
> > FWIW, I think we could simply enable iommu_platform for protected
> > guests for ccw; no prereqs like pci's disable-legacy.
>
> For s390x having a memory-encryption object is not prereq for doing
> protected virtualization, so the scheme does not work for us right now.
That's basically true for POWER as well - in our case the "memory
encrypt" object (called "host trust limitation" (HTL) object in the
latest version) is basically just a dummy with no parameters. The
same should work for s390x.
I am considering having the machine always create the HTL object with
a well-known name (e.g. "pef0"), so you can just set the machine
property to it to enable PEF. Again, that could also be done on
s390x.
Note also that anything could in principle implement the HTL
interface. So you could have the machine object itelf, or the cpu
implement the interface to avoid creating a dummy object, though that
might get messier that just having a dummy in the long run.
> I hope Jansoch will chime in after he is back from his vacation. IMHO
> having a memory-protection object will come in handy for migration,
> but the presence or absence of this object should be largely transparent
> to the user (and not something that needs to be explicitly managed via
> command line). AFAIU this object is in the end it is just QEMU plumbing.
Yes. However, if either POWER or z ever gets any configurable knobs
for their protection systems, it does provide an obvious place that we
can do that configuration.
> > > > as cases where user has
> > > > specified the property manually could be worth-while.
> > >
> > > I don't really see what's to be done there. I'm assuming that if the
> > > user specifies it, they know what they're doing - particularly with
> > > nonstandard guests there are some odd edge cases where those
> > > combinations might work, they're just not very likely.
> >
> > If I understood Halil correctly, devices without iommu_platform
> > apparently can crash protected guests on s390. Is that supposed to be a
> > "if it breaks, you get to keep the pieces" situation, or do we really
> > want to enforce iommu_platform?
>
> I strongly oppose to adopting the "if it breaks, you
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Tue, Jun 09, 2020 at 12:16:41PM +0200, Cornelia Huck wrote:
> On Sun, 7 Jun 2020 13:07:35 +1000
> David Gibson wrote:
>
> > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> > > > The default behaviour for virtio devices is not to use the platforms
> > > > normal
> > > > DMA paths, but instead to use the fact that it's running in a hypervisor
> > > > to directly access guest memory. That doesn't work if the guest's
> > > > memory
> > > > is protected from hypervisor access, such as with AMD's SEV or POWER's
> > > > PEF.
> > > >
> > > > So, if a guest memory protection mechanism is enabled, then apply the
> > > > iommu_platform=on option so it will go through normal DMA mechanisms.
> > > > Those will presumably have some way of marking memory as shared with the
> > > > hypervisor or hardware so that DMA will work.
> > > >
> > > > Signed-off-by: David Gibson
> > > > ---
> > > > hw/core/machine.c | 11 +++
> > > > 1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index 88d699bceb..cb6580954e 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -28,6 +28,8 @@
> > > > #include "hw/mem/nvdimm.h"
> > > > #include "migration/vmstate.h"
> > > > #include "exec/guest-memory-protection.h"
> > > > +#include "hw/virtio/virtio.h"
> > > > +#include "hw/virtio/virtio-pci.h"
> > > >
> > > > GlobalProperty hw_compat_5_0[] = {};
> > > > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > > > @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState
> > > > *machine)
> > > > * areas.
> > > > */
> > > > machine_set_mem_merge(OBJECT(machine), false, _abort);
> > > > +
> > > > +/*
> > > > + * Virtio devices can't count on directly accessing guest
> > > > + * memory, so they need iommu_platform=on to use normal DMA
> > > > + * mechanisms. That requires disabling legacy virtio support
> > > > + * for virtio pci devices
> > > > + */
> > > > +object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy",
> > > > "on");
> > > > +object_register_sugar_prop(TYPE_VIRTIO_DEVICE,
> > > > "iommu_platform", "on");
> > > > }
> > > >
> > >
> > > I think it's a reasonable way to address this overall.
> > > As Cornelia has commented, addressing ccw as well
> >
> > Sure. I was assuming somebody who actually knows ccw could do that as
> > a follow up.
>
> FWIW, I think we could simply enable iommu_platform for protected
> guests for ccw; no prereqs like pci's disable-legacy.
Right, and the code above should in fact already do so, since it
applies that to TYPE_VIRTIO_DEVICE, which is common. The
disable-legacy part should be harmless for s390, since this is
effectively just setting a default, and we don't expect any
TYPE_VIRTIO_PCI devices to be instantiated on z.
> > > as cases where user has
> > > specified the property manually could be worth-while.
> >
> > I don't really see what's to be done there. I'm assuming that if the
> > user specifies it, they know what they're doing - particularly with
> > nonstandard guests there are some odd edge cases where those
> > combinations might work, they're just not very likely.
>
> If I understood Halil correctly, devices without iommu_platform
> apparently can crash protected guests on s390. Is that supposed to be a
> "if it breaks, you get to keep the pieces" situation, or do we really
> want to enforce iommu_platform?
I actually think "if you broke it, keep the pieces" is an acceptable
approach here, but that doesn't preclude some further enforcement to
improve UX.
--
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
signature.asc
Description: PGP signature
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Tue, Jun 09, 2020 at 05:40:46PM +0200, Halil Pasic wrote: > For s390x having a memory-encryption object is not prereq for doing > protected virtualization, so the scheme does not work for us right now. It does make things much easier implementation-wise while just marginally harder to use though. -- MST
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Tue, 9 Jun 2020 17:40:46 +0200
Halil Pasic wrote:
> On Tue, 9 Jun 2020 12:16:41 +0200
> Cornelia Huck wrote:
>
> > On Sun, 7 Jun 2020 13:07:35 +1000
> > David Gibson wrote:
> >
> > > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> > > > > The default behaviour for virtio devices is not to use the platforms
> > > > > normal
> > > > > DMA paths, but instead to use the fact that it's running in a
> > > > > hypervisor
> > > > > to directly access guest memory. That doesn't work if the guest's
> > > > > memory
> > > > > is protected from hypervisor access, such as with AMD's SEV or
> > > > > POWER's PEF.
> > > > >
> > > > > So, if a guest memory protection mechanism is enabled, then apply the
> > > > > iommu_platform=on option so it will go through normal DMA mechanisms.
> > > > > Those will presumably have some way of marking memory as shared with
> > > > > the
> > > > > hypervisor or hardware so that DMA will work.
> > > > >
> > > > > Signed-off-by: David Gibson
> > > > > ---
> > > > > hw/core/machine.c | 11 +++
> > > > > 1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index 88d699bceb..cb6580954e 100644
> > > > > --- a/hw/core/machine.c
> > > > > +++ b/hw/core/machine.c
> > > > > @@ -28,6 +28,8 @@
> > > > > #include "hw/mem/nvdimm.h"
> > > > > #include "migration/vmstate.h"
> > > > > #include "exec/guest-memory-protection.h"
> > > > > +#include "hw/virtio/virtio.h"
> > > > > +#include "hw/virtio/virtio-pci.h"
> > > > >
> > > > > GlobalProperty hw_compat_5_0[] = {};
> > > > > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > > > > @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState
> > > > > *machine)
> > > > > * areas.
> > > > > */
> > > > > machine_set_mem_merge(OBJECT(machine), false, _abort);
> > > > > +
> > > > > +/*
> > > > > + * Virtio devices can't count on directly accessing guest
> > > > > + * memory, so they need iommu_platform=on to use normal DMA
> > > > > + * mechanisms. That requires disabling legacy virtio support
> > > > > + * for virtio pci devices
> > > > > + */
> > > > > +object_register_sugar_prop(TYPE_VIRTIO_PCI,
> > > > > "disable-legacy", "on");
> > > > > +object_register_sugar_prop(TYPE_VIRTIO_DEVICE,
> > > > > "iommu_platform", "on");
> > > > > }
> > > > >
> > > >
> > > > I think it's a reasonable way to address this overall.
> > > > As Cornelia has commented, addressing ccw as well
> > >
> > > Sure. I was assuming somebody who actually knows ccw could do that as
> > > a follow up.
> >
> > FWIW, I think we could simply enable iommu_platform for protected
> > guests for ccw; no prereqs like pci's disable-legacy.
> >
>
> For s390x having a memory-encryption object is not prereq for doing
> protected virtualization, so the scheme does not work for us right now.
Yeah, that would obviously need to be added first.
>
> I hope Jansoch will chime in after he is back from his vacation. IMHO
> having a memory-protection object will come in handy for migration,
> but the presence or absence of this object should be largely transparent
> to the user (and not something that needs to be explicitly managed via
> command line). AFAIU this object is in the end it is just QEMU plumbing.
>
> > >
> > > > as cases where user has
> > > > specified the property manually could be worth-while.
> > >
> > > I don't really see what's to be done there. I'm assuming that if the
> > > user specifies it, they know what they're doing - particularly with
> > > nonstandard guests there are some odd edge cases where those
> > > combinations might work, they're just not very likely.
> >
> > If I understood Halil correctly, devices without iommu_platform
> > apparently can crash protected guests on s390. Is that supposed to be a
> > "if it breaks, you get to keep the pieces" situation, or do we really
> > want to enforce iommu_platform?
>
> I strongly oppose to adopting the "if it breaks, you get to keep the
> pieces" strategy here. It is borderline acceptable on startup, although
> IMHO not preferable, but a device hotplug bringing down a guest that is
> already running userspace is not acceptable at all.
There's still the option to fail to add such a device, though.
pgpLVYyJGgr8O.pgp
Description: OpenPGP digital signature
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Tue, 9 Jun 2020 12:16:41 +0200
Cornelia Huck wrote:
> On Sun, 7 Jun 2020 13:07:35 +1000
> David Gibson wrote:
>
> > On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> > > > The default behaviour for virtio devices is not to use the platforms
> > > > normal
> > > > DMA paths, but instead to use the fact that it's running in a hypervisor
> > > > to directly access guest memory. That doesn't work if the guest's
> > > > memory
> > > > is protected from hypervisor access, such as with AMD's SEV or POWER's
> > > > PEF.
> > > >
> > > > So, if a guest memory protection mechanism is enabled, then apply the
> > > > iommu_platform=on option so it will go through normal DMA mechanisms.
> > > > Those will presumably have some way of marking memory as shared with the
> > > > hypervisor or hardware so that DMA will work.
> > > >
> > > > Signed-off-by: David Gibson
> > > > ---
> > > > hw/core/machine.c | 11 +++
> > > > 1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index 88d699bceb..cb6580954e 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -28,6 +28,8 @@
> > > > #include "hw/mem/nvdimm.h"
> > > > #include "migration/vmstate.h"
> > > > #include "exec/guest-memory-protection.h"
> > > > +#include "hw/virtio/virtio.h"
> > > > +#include "hw/virtio/virtio-pci.h"
> > > >
> > > > GlobalProperty hw_compat_5_0[] = {};
> > > > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > > > @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState
> > > > *machine)
> > > > * areas.
> > > > */
> > > > machine_set_mem_merge(OBJECT(machine), false, _abort);
> > > > +
> > > > +/*
> > > > + * Virtio devices can't count on directly accessing guest
> > > > + * memory, so they need iommu_platform=on to use normal DMA
> > > > + * mechanisms. That requires disabling legacy virtio support
> > > > + * for virtio pci devices
> > > > + */
> > > > +object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy",
> > > > "on");
> > > > +object_register_sugar_prop(TYPE_VIRTIO_DEVICE,
> > > > "iommu_platform", "on");
> > > > }
> > > >
> > >
> > > I think it's a reasonable way to address this overall.
> > > As Cornelia has commented, addressing ccw as well
> >
> > Sure. I was assuming somebody who actually knows ccw could do that as
> > a follow up.
>
> FWIW, I think we could simply enable iommu_platform for protected
> guests for ccw; no prereqs like pci's disable-legacy.
>
For s390x having a memory-encryption object is not prereq for doing
protected virtualization, so the scheme does not work for us right now.
I hope Jansoch will chime in after he is back from his vacation. IMHO
having a memory-protection object will come in handy for migration,
but the presence or absence of this object should be largely transparent
to the user (and not something that needs to be explicitly managed via
command line). AFAIU this object is in the end it is just QEMU plumbing.
> >
> > > as cases where user has
> > > specified the property manually could be worth-while.
> >
> > I don't really see what's to be done there. I'm assuming that if the
> > user specifies it, they know what they're doing - particularly with
> > nonstandard guests there are some odd edge cases where those
> > combinations might work, they're just not very likely.
>
> If I understood Halil correctly, devices without iommu_platform
> apparently can crash protected guests on s390. Is that supposed to be a
> "if it breaks, you get to keep the pieces" situation, or do we really
> want to enforce iommu_platform?
I strongly oppose to adopting the "if it breaks, you get to keep the
pieces" strategy here. It is borderline acceptable on startup, although
IMHO not preferable, but a device hotplug bringing down a guest that is
already running userspace is not acceptable at all.
Regards,
Halil
pgpzx1UPAP6st.pgp
Description: OpenPGP digital signature
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Sun, 7 Jun 2020 13:07:35 +1000
David Gibson wrote:
> On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> > On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> > > The default behaviour for virtio devices is not to use the platforms
> > > normal
> > > DMA paths, but instead to use the fact that it's running in a hypervisor
> > > to directly access guest memory. That doesn't work if the guest's memory
> > > is protected from hypervisor access, such as with AMD's SEV or POWER's
> > > PEF.
> > >
> > > So, if a guest memory protection mechanism is enabled, then apply the
> > > iommu_platform=on option so it will go through normal DMA mechanisms.
> > > Those will presumably have some way of marking memory as shared with the
> > > hypervisor or hardware so that DMA will work.
> > >
> > > Signed-off-by: David Gibson
> > > ---
> > > hw/core/machine.c | 11 +++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index 88d699bceb..cb6580954e 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -28,6 +28,8 @@
> > > #include "hw/mem/nvdimm.h"
> > > #include "migration/vmstate.h"
> > > #include "exec/guest-memory-protection.h"
> > > +#include "hw/virtio/virtio.h"
> > > +#include "hw/virtio/virtio-pci.h"
> > >
> > > GlobalProperty hw_compat_5_0[] = {};
> > > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > > @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState *machine)
> > > * areas.
> > > */
> > > machine_set_mem_merge(OBJECT(machine), false, _abort);
> > > +
> > > +/*
> > > + * Virtio devices can't count on directly accessing guest
> > > + * memory, so they need iommu_platform=on to use normal DMA
> > > + * mechanisms. That requires disabling legacy virtio support
> > > + * for virtio pci devices
> > > + */
> > > +object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy",
> > > "on");
> > > +object_register_sugar_prop(TYPE_VIRTIO_DEVICE, "iommu_platform",
> > > "on");
> > > }
> > >
> >
> > I think it's a reasonable way to address this overall.
> > As Cornelia has commented, addressing ccw as well
>
> Sure. I was assuming somebody who actually knows ccw could do that as
> a follow up.
FWIW, I think we could simply enable iommu_platform for protected
guests for ccw; no prereqs like pci's disable-legacy.
>
> > as cases where user has
> > specified the property manually could be worth-while.
>
> I don't really see what's to be done there. I'm assuming that if the
> user specifies it, they know what they're doing - particularly with
> nonstandard guests there are some odd edge cases where those
> combinations might work, they're just not very likely.
If I understood Halil correctly, devices without iommu_platform
apparently can crash protected guests on s390. Is that supposed to be a
"if it breaks, you get to keep the pieces" situation, or do we really
want to enforce iommu_platform?
pgpLdkGlEHXqW.pgp
Description: OpenPGP digital signature
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Sat, Jun 06, 2020 at 04:21:31PM -0400, Michael S. Tsirkin wrote:
> On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> > The default behaviour for virtio devices is not to use the platforms normal
> > DMA paths, but instead to use the fact that it's running in a hypervisor
> > to directly access guest memory. That doesn't work if the guest's memory
> > is protected from hypervisor access, such as with AMD's SEV or POWER's PEF.
> >
> > So, if a guest memory protection mechanism is enabled, then apply the
> > iommu_platform=on option so it will go through normal DMA mechanisms.
> > Those will presumably have some way of marking memory as shared with the
> > hypervisor or hardware so that DMA will work.
> >
> > Signed-off-by: David Gibson
> > ---
> > hw/core/machine.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 88d699bceb..cb6580954e 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -28,6 +28,8 @@
> > #include "hw/mem/nvdimm.h"
> > #include "migration/vmstate.h"
> > #include "exec/guest-memory-protection.h"
> > +#include "hw/virtio/virtio.h"
> > +#include "hw/virtio/virtio-pci.h"
> >
> > GlobalProperty hw_compat_5_0[] = {};
> > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState *machine)
> > * areas.
> > */
> > machine_set_mem_merge(OBJECT(machine), false, _abort);
> > +
> > +/*
> > + * Virtio devices can't count on directly accessing guest
> > + * memory, so they need iommu_platform=on to use normal DMA
> > + * mechanisms. That requires disabling legacy virtio support
> > + * for virtio pci devices
> > + */
> > +object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy",
> > "on");
> > +object_register_sugar_prop(TYPE_VIRTIO_DEVICE, "iommu_platform",
> > "on");
> > }
> >
>
> I think it's a reasonable way to address this overall.
> As Cornelia has commented, addressing ccw as well
Sure. I was assuming somebody who actually knows ccw could do that as
a follow up.
> as cases where user has
> specified the property manually could be worth-while.
I don't really see what's to be done there. I'm assuming that if the
user specifies it, they know what they're doing - particularly with
nonstandard guests there are some odd edge cases where those
combinations might work, they're just not very likely.
--
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
signature.asc
Description: PGP signature
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Thu, May 21, 2020 at 01:43:04PM +1000, David Gibson wrote:
> The default behaviour for virtio devices is not to use the platforms normal
> DMA paths, but instead to use the fact that it's running in a hypervisor
> to directly access guest memory. That doesn't work if the guest's memory
> is protected from hypervisor access, such as with AMD's SEV or POWER's PEF.
>
> So, if a guest memory protection mechanism is enabled, then apply the
> iommu_platform=on option so it will go through normal DMA mechanisms.
> Those will presumably have some way of marking memory as shared with the
> hypervisor or hardware so that DMA will work.
>
> Signed-off-by: David Gibson
> ---
> hw/core/machine.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 88d699bceb..cb6580954e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -28,6 +28,8 @@
> #include "hw/mem/nvdimm.h"
> #include "migration/vmstate.h"
> #include "exec/guest-memory-protection.h"
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-pci.h"
>
> GlobalProperty hw_compat_5_0[] = {};
> const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState *machine)
> * areas.
> */
> machine_set_mem_merge(OBJECT(machine), false, _abort);
> +
> +/*
> + * Virtio devices can't count on directly accessing guest
> + * memory, so they need iommu_platform=on to use normal DMA
> + * mechanisms. That requires disabling legacy virtio support
> + * for virtio pci devices
> + */
> +object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy", "on");
> +object_register_sugar_prop(TYPE_VIRTIO_DEVICE, "iommu_platform",
> "on");
> }
>
I think it's a reasonable way to address this overall.
As Cornelia has commented, addressing ccw as well as cases where user has
specified the property manually could be worth-while.
> machine_class->init(machine);
> --
> 2.26.2
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Fri, 5 Jun 2020 12:45:35 +0200 Cornelia Huck wrote: > On Thu, 21 May 2020 13:43:04 +1000 > David Gibson wrote: > > > The default behaviour for virtio devices is not to use the platforms normal > > DMA paths, but instead to use the fact that it's running in a hypervisor > > to directly access guest memory. That doesn't work if the guest's memory > > is protected from hypervisor access, such as with AMD's SEV or POWER's PEF. > > > > So, if a guest memory protection mechanism is enabled, then apply the > > iommu_platform=on option so it will go through normal DMA mechanisms. > > Those will presumably have some way of marking memory as shared with the > > hypervisor or hardware so that DMA will work. > > cc: Halil, who had been looking at the interaction of virtio-ccw > devices and s390 protected virt. > > (IIRC, we wanted to try with a on/off/auto property for virtio-ccw?) Thanks Connie! I will definitely look into this. And regarding on/off/auto I will try to send out something today. Regards, Halil
Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests
On Thu, 21 May 2020 13:43:04 +1000
David Gibson wrote:
> The default behaviour for virtio devices is not to use the platforms normal
> DMA paths, but instead to use the fact that it's running in a hypervisor
> to directly access guest memory. That doesn't work if the guest's memory
> is protected from hypervisor access, such as with AMD's SEV or POWER's PEF.
>
> So, if a guest memory protection mechanism is enabled, then apply the
> iommu_platform=on option so it will go through normal DMA mechanisms.
> Those will presumably have some way of marking memory as shared with the
> hypervisor or hardware so that DMA will work.
cc: Halil, who had been looking at the interaction of virtio-ccw
devices and s390 protected virt.
(IIRC, we wanted to try with a on/off/auto property for virtio-ccw?)
>
> Signed-off-by: David Gibson
> ---
> hw/core/machine.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 88d699bceb..cb6580954e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -28,6 +28,8 @@
> #include "hw/mem/nvdimm.h"
> #include "migration/vmstate.h"
> #include "exec/guest-memory-protection.h"
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-pci.h"
>
> GlobalProperty hw_compat_5_0[] = {};
> const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> @@ -1159,6 +1161,15 @@ void machine_run_board_init(MachineState *machine)
> * areas.
> */
> machine_set_mem_merge(OBJECT(machine), false, _abort);
> +
> +/*
> + * Virtio devices can't count on directly accessing guest
> + * memory, so they need iommu_platform=on to use normal DMA
> + * mechanisms. That requires disabling legacy virtio support
> + * for virtio pci devices
> + */
> +object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy", "on");
> +object_register_sugar_prop(TYPE_VIRTIO_DEVICE, "iommu_platform",
> "on");
> }
>
> machine_class->init(machine);
