diff mbox series

[v3,9/9] host trust limitation: Alter virtio default properties for protected guests

Message ID 20200619020602.118306-10-david@gibson.dropbear.id.au
State New, archived
Headers show
Series Generalize memory encryption models | expand

Commit Message

David Gibson June 19, 2020, 2:06 a.m. UTC
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 host trust limitation 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 <david@gibson.dropbear.id.au>
---
 hw/core/machine.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Daniel P. Berrangé June 19, 2020, 10:12 a.m. UTC | #1
On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> ---
>  hw/core/machine.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-pci.h"
>  
>  GlobalProperty hw_compat_5_0[] = {
>      { "virtio-balloon-device", "page-poison", "false" },
> @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
>           * areas.
>           */
>          machine_set_mem_merge(OBJECT(machine), false, &error_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");
>      }

Silently changing the user's request configuration like this is a bad idea.
The "disable-legacy" option in particular is undesirable as that switches
the device to virtio-1.0 only mode, which exposes a different PCI ID to
the guest.

If some options are incompatible with encryption, then we should raise a
fatal error at startup, so applications/admins are aware that their requested
config is broken.

Regards,
Daniel
Michael S. Tsirkin June 19, 2020, 11:46 a.m. UTC | #2
On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > ---
> >  hw/core/machine.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > +#include "hw/virtio/virtio.h"
> > +#include "hw/virtio/virtio-pci.h"
> >  
> >  GlobalProperty hw_compat_5_0[] = {
> >      { "virtio-balloon-device", "page-poison", "false" },
> > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> >           * areas.
> >           */
> >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> >      }
> 
> Silently changing the user's request configuration like this is a bad idea.
> The "disable-legacy" option in particular is undesirable as that switches
> the device to virtio-1.0 only mode, which exposes a different PCI ID to
> the guest.
> 
> If some options are incompatible with encryption, then we should raise a
> fatal error at startup, so applications/admins are aware that their requested
> config is broken.
> 
> Regards,
> Daniel

Agreed - my suggestion is an on/off/auto property, auto value
changes automatically, on/off is validated.


> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Michael S. Tsirkin June 19, 2020, 11:47 a.m. UTC | #3
On Fri, Jun 19, 2020 at 07:46:14AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/core/machine.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > +#include "hw/virtio/virtio.h"
> > > +#include "hw/virtio/virtio-pci.h"
> > >  
> > >  GlobalProperty hw_compat_5_0[] = {
> > >      { "virtio-balloon-device", "page-poison", "false" },
> > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> > >           * areas.
> > >           */
> > >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> > >      }
> > 
> > Silently changing the user's request configuration like this is a bad idea.
> > The "disable-legacy" option in particular is undesirable as that switches
> > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > the guest.
> > 
> > If some options are incompatible with encryption, then we should raise a
> > fatal error at startup, so applications/admins are aware that their requested
> > config is broken.
> > 
> > Regards,
> > Daniel
> 
> Agreed - my suggestion is an on/off/auto property, auto value
> changes automatically, on/off is validated.


In fact should we extend all bit properties to allow an auto value?

> 
> > -- 
> > |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> > |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> > |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Daniel P. Berrangé June 19, 2020, 12:16 p.m. UTC | #4
On Fri, Jun 19, 2020 at 07:47:20AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jun 19, 2020 at 07:46:14AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > > > ---
> > > >  hw/core/machine.c | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > > 
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > +#include "hw/virtio/virtio.h"
> > > > +#include "hw/virtio/virtio-pci.h"
> > > >  
> > > >  GlobalProperty hw_compat_5_0[] = {
> > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> > > >           * areas.
> > > >           */
> > > >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> > > >      }
> > > 
> > > Silently changing the user's request configuration like this is a bad idea.
> > > The "disable-legacy" option in particular is undesirable as that switches
> > > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > > the guest.
> > > 
> > > If some options are incompatible with encryption, then we should raise a
> > > fatal error at startup, so applications/admins are aware that their requested
> > > config is broken.
> >
> > Agreed - my suggestion is an on/off/auto property, auto value
> > changes automatically, on/off is validated.
> 
> In fact should we extend all bit properties to allow an auto value?

If "auto" was made the default that creates a similar headache, as to
preserve existing configuration semantics we expose to apps, libvirt
would need to find all the properties changed to use "auto" and manually
set them back to on/off explicitly.

Regards,
Daniel
David Gibson June 19, 2020, 2:45 p.m. UTC | #5
On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > ---
> >  hw/core/machine.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > +#include "hw/virtio/virtio.h"
> > +#include "hw/virtio/virtio-pci.h"
> >  
> >  GlobalProperty hw_compat_5_0[] = {
> >      { "virtio-balloon-device", "page-poison", "false" },
> > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> >           * areas.
> >           */
> >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> >      }
> 
> Silently changing the user's request configuration like this

It doesn't, though.  register_sugar_prop() effectively registers a
default, so if the user has explicitly specified something, that will
take precedence.

> is a bad idea.
> The "disable-legacy" option in particular is undesirable as that switches
> the device to virtio-1.0 only mode, which exposes a different PCI ID to
> the guest.
> 
> If some options are incompatible with encryption, then we should raise a
> fatal error at startup, so applications/admins are aware that their requested
> config is broken.
> 
> Regards,
> Daniel
Daniel P. Berrangé June 19, 2020, 3:05 p.m. UTC | #6
On Sat, Jun 20, 2020 at 12:45:41AM +1000, David Gibson wrote:
> On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/core/machine.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > +#include "hw/virtio/virtio.h"
> > > +#include "hw/virtio/virtio-pci.h"
> > >  
> > >  GlobalProperty hw_compat_5_0[] = {
> > >      { "virtio-balloon-device", "page-poison", "false" },
> > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> > >           * areas.
> > >           */
> > >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> > >      }
> > 
> > Silently changing the user's request configuration like this
> 
> It doesn't, though.  register_sugar_prop() effectively registers a
> default, so if the user has explicitly specified something, that will
> take precedence.

Don't assume that the user has set "disable-legacy=off". People who want to
have a transtional device are almost certainly pasing "-device virtio-blk-pci",
because historical behaviour is that this is sufficient to give you a
transitional device. Changing the default of disable-legacy=on has not
honoured the users' requested config.

Regards,
Daniel
Halil Pasic June 19, 2020, 8:04 p.m. UTC | #7
On Fri, 19 Jun 2020 13:16:38 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

[..]

> > > > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> > > > >           * areas.
> > > > >           */
> > > > >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> > > > >      }  
> > > > 
> > > > Silently changing the user's request configuration like this is a bad idea.
> > > > The "disable-legacy" option in particular is undesirable as that switches
> > > > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > > > the guest.
> > > > 
> > > > If some options are incompatible with encryption, then we should raise a
> > > > fatal error at startup, so applications/admins are aware that their requested
> > > > config is broken.  
> > >
> > > Agreed - my suggestion is an on/off/auto property, auto value
> > > changes automatically, on/off is validated.  
> > 
> > In fact should we extend all bit properties to allow an auto value?  
> 
> If "auto" was made the default that creates a similar headache, as to
> preserve existing configuration semantics we expose to apps, libvirt
> would need to find all the properties changed to use "auto" and manually
> set them back to on/off explicitly.

Hm, does that mean we can't change the default for any qemu property?

I would expect that the defaults most remain invariant for any
particular machine version. Conceptually, IMHO the default could change
with a new machine version, but we would need some mechanism to ensure
compatibility for old machine versions.

Regards,
Halil
David Gibson June 20, 2020, 8:24 a.m. UTC | #8
On Fri, Jun 19, 2020 at 04:05:56PM +0100, Daniel P. Berrangé wrote:
> On Sat, Jun 20, 2020 at 12:45:41AM +1000, David Gibson wrote:
> > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > > > ---
> > > >  hw/core/machine.c | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > > 
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > +#include "hw/virtio/virtio.h"
> > > > +#include "hw/virtio/virtio-pci.h"
> > > >  
> > > >  GlobalProperty hw_compat_5_0[] = {
> > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> > > >           * areas.
> > > >           */
> > > >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> > > >      }
> > > 
> > > Silently changing the user's request configuration like this
> > 
> > It doesn't, though.  register_sugar_prop() effectively registers a
> > default, so if the user has explicitly specified something, that will
> > take precedence.
> 
> Don't assume that the user has set "disable-legacy=off". People who want to
> have a transtional device are almost certainly pasing "-device virtio-blk-pci",
> because historical behaviour is that this is sufficient to give you a
> transitional device. Changing the default of disable-legacy=on has not
> honoured the users' requested config.

Umm.. by this argument we can never change any default, ever.  But we
do that routinely with new machine versions.  How is changing based on
a machine option different from that?
Daniel P. Berrangé June 22, 2020, 9:09 a.m. UTC | #9
On Sat, Jun 20, 2020 at 06:24:27PM +1000, David Gibson wrote:
> On Fri, Jun 19, 2020 at 04:05:56PM +0100, Daniel P. Berrangé wrote:
> > On Sat, Jun 20, 2020 at 12:45:41AM +1000, David Gibson wrote:
> > > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > > > > ---
> > > > >  hw/core/machine.c | 11 +++++++++++
> > > > >  1 file changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > > +#include "hw/virtio/virtio.h"
> > > > > +#include "hw/virtio/virtio-pci.h"
> > > > >  
> > > > >  GlobalProperty hw_compat_5_0[] = {
> > > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> > > > >           * areas.
> > > > >           */
> > > > >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> > > > >      }
> > > > 
> > > > Silently changing the user's request configuration like this
> > > 
> > > It doesn't, though.  register_sugar_prop() effectively registers a
> > > default, so if the user has explicitly specified something, that will
> > > take precedence.
> > 
> > Don't assume that the user has set "disable-legacy=off". People who want to
> > have a transtional device are almost certainly pasing "-device virtio-blk-pci",
> > because historical behaviour is that this is sufficient to give you a
> > transitional device. Changing the default of disable-legacy=on has not
> > honoured the users' requested config.
> 
> Umm.. by this argument we can never change any default, ever.  But we
> do that routinely with new machine versions.  How is changing based on
> a machine option different from that?

It isn't really different. Most of the time we get away with it and no one
sees a problem. Some of the changes made though, do indeed break things,
and libvirt tries to override QEMU's changes in defaults where they are
especially at risk of causing breakage. The virtio device model is one such
change I'd consider especially risky as there are clear guest OS driver
support compatibility issues there, with it being a completely different
PCI device ID & impl.

Regards,
Daniel
Michael S. Tsirkin June 24, 2020, 7:55 a.m. UTC | #10
On Fri, Jun 19, 2020 at 01:16:38PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 19, 2020 at 07:47:20AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jun 19, 2020 at 07:46:14AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > > > > ---
> > > > >  hw/core/machine.c | 11 +++++++++++
> > > > >  1 file changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > > +#include "hw/virtio/virtio.h"
> > > > > +#include "hw/virtio/virtio-pci.h"
> > > > >  
> > > > >  GlobalProperty hw_compat_5_0[] = {
> > > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> > > > >           * areas.
> > > > >           */
> > > > >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> > > > >      }
> > > > 
> > > > Silently changing the user's request configuration like this is a bad idea.
> > > > The "disable-legacy" option in particular is undesirable as that switches
> > > > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > > > the guest.
> > > > 
> > > > If some options are incompatible with encryption, then we should raise a
> > > > fatal error at startup, so applications/admins are aware that their requested
> > > > config is broken.
> > >
> > > Agreed - my suggestion is an on/off/auto property, auto value
> > > changes automatically, on/off is validated.
> > 
> > In fact should we extend all bit properties to allow an auto value?
> 
> If "auto" was made the default that creates a similar headache, as to
> preserve existing configuration semantics we expose to apps, libvirt
> would need to find all the properties changed to use "auto" and manually
> set them back to on/off explicitly.
> 
> Regards,
> Daniel

It's QEMU's job to try and have more or less consistent semantics across
versions. QEMU does not guarantee not to change any option defaults
though.

My point is to add ability to differentiate between property values
set by user and ones set by machine type for compatibility.
David Gibson June 25, 2020, 4:57 a.m. UTC | #11
On Wed, Jun 24, 2020 at 03:55:59AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jun 19, 2020 at 01:16:38PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 19, 2020 at 07:47:20AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Jun 19, 2020 at 07:46:14AM -0400, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > > > > > ---
> > > > > >  hw/core/machine.c | 11 +++++++++++
> > > > > >  1 file changed, 11 insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > > > +#include "hw/virtio/virtio.h"
> > > > > > +#include "hw/virtio/virtio-pci.h"
> > > > > >  
> > > > > >  GlobalProperty hw_compat_5_0[] = {
> > > > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> > > > > >           * areas.
> > > > > >           */
> > > > > >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> > > > > >      }
> > > > > 
> > > > > Silently changing the user's request configuration like this is a bad idea.
> > > > > The "disable-legacy" option in particular is undesirable as that switches
> > > > > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > > > > the guest.
> > > > > 
> > > > > If some options are incompatible with encryption, then we should raise a
> > > > > fatal error at startup, so applications/admins are aware that their requested
> > > > > config is broken.
> > > >
> > > > Agreed - my suggestion is an on/off/auto property, auto value
> > > > changes automatically, on/off is validated.
> > > 
> > > In fact should we extend all bit properties to allow an auto value?
> > 
> > If "auto" was made the default that creates a similar headache, as to
> > preserve existing configuration semantics we expose to apps, libvirt
> > would need to find all the properties changed to use "auto" and manually
> > set them back to on/off explicitly.
> > 
> > Regards,
> > Daniel
> 
> It's QEMU's job to try and have more or less consistent semantics across
> versions. QEMU does not guarantee not to change any option defaults
> though.
> 
> My point is to add ability to differentiate between property values
> set by user and ones set by machine type for compatibility.

At which point are you looking to differentiate these?  The use of
sugar_prop() in my draft code accomplishes this already for the
purposes of resolving a final property value within qemu (an explicit
user set one takes precedence).
David Gibson June 25, 2020, 5:02 a.m. UTC | #12
On Fri, Jun 19, 2020 at 07:46:10AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/core/machine.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > +#include "hw/virtio/virtio.h"
> > > +#include "hw/virtio/virtio-pci.h"
> > >  
> > >  GlobalProperty hw_compat_5_0[] = {
> > >      { "virtio-balloon-device", "page-poison", "false" },
> > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> > >           * areas.
> > >           */
> > >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> > >      }
> > 
> > Silently changing the user's request configuration like this is a bad idea.
> > The "disable-legacy" option in particular is undesirable as that switches
> > the device to virtio-1.0 only mode, which exposes a different PCI ID to
> > the guest.
> > 
> > If some options are incompatible with encryption, then we should raise a
> > fatal error at startup, so applications/admins are aware that their requested
> > config is broken.
> > 
> > Regards,
> > Daniel
> 
> Agreed - my suggestion is an on/off/auto property, auto value
> changes automatically, on/off is validated.

So, I think you're specifically suggesting this for the
"iommu_platform" property, by delaying determining which mode to use
until the guest activates the device.  Is that correct?

That might work on s390, but I don't think it will work on POWER on at
least 2 counts:

1) qemu doesn't actually have a natural way of determining if a guest
   is in secure mode (that's handled directly between the guest and
   the ultravisor).  So even at driver init time, we won't know the
   right value.

2) for virtio-pci, iommu_platform=on requires a "modern" device, not a
   legacy or transitional one.  That changes the PCI ID, which means
   we can't delay deciding it until driver init
David Gibson June 25, 2020, 5:06 a.m. UTC | #13
On Mon, Jun 22, 2020 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
> On Sat, Jun 20, 2020 at 06:24:27PM +1000, David Gibson wrote:
> > On Fri, Jun 19, 2020 at 04:05:56PM +0100, Daniel P. Berrangé wrote:
> > > On Sat, Jun 20, 2020 at 12:45:41AM +1000, David Gibson wrote:
> > > > On Fri, Jun 19, 2020 at 11:12:45AM +0100, Daniel P. Berrangé wrote:
> > > > > On Fri, Jun 19, 2020 at 12:06:02PM +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 host trust limitation 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 <david@gibson.dropbear.id.au>
> > > > > > ---
> > > > > >  hw/core/machine.c | 11 +++++++++++
> > > > > >  1 file changed, 11 insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > > index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
> > > > > > +#include "hw/virtio/virtio.h"
> > > > > > +#include "hw/virtio/virtio-pci.h"
> > > > > >  
> > > > > >  GlobalProperty hw_compat_5_0[] = {
> > > > > >      { "virtio-balloon-device", "page-poison", "false" },
> > > > > > @@ -1165,6 +1167,15 @@ void machine_run_board_init(MachineState *machine)
> > > > > >           * areas.
> > > > > >           */
> > > > > >          machine_set_mem_merge(OBJECT(machine), false, &error_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");
> > > > > >      }
> > > > > 
> > > > > Silently changing the user's request configuration like this
> > > > 
> > > > It doesn't, though.  register_sugar_prop() effectively registers a
> > > > default, so if the user has explicitly specified something, that will
> > > > take precedence.
> > > 
> > > Don't assume that the user has set "disable-legacy=off". People who want to
> > > have a transtional device are almost certainly pasing "-device virtio-blk-pci",
> > > because historical behaviour is that this is sufficient to give you a
> > > transitional device. Changing the default of disable-legacy=on has not
> > > honoured the users' requested config.
> > 
> > Umm.. by this argument we can never change any default, ever.  But we
> > do that routinely with new machine versions.  How is changing based on
> > a machine option different from that?
> 
> It isn't really different. Most of the time we get away with it and no one
> sees a problem. Some of the changes made though, do indeed break things,
> and libvirt tries to override QEMU's changes in defaults where they are
> especially at risk of causing breakage. The virtio device model is one such
> change I'd consider especially risky as there are clear guest OS driver
> support compatibility issues there, with it being a completely different
> PCI device ID & impl.

If it were possible to drop an existing supported guest into secure VM
mode, that would make sense.  But AFAICT, a guest always need to be
aware of the secure mode - it certainly does on POWER.  Plus, support
for secure guest mode is way newer than support for modern virtio
devices.

Even then, I don't see that this is really anything new.  Updating
machine type version can absolutely change system devices in a way
which could break guests (though it mostly won't).  If you really want
stable support for a given guest, use a versioned machine type.  Doing
that will work just as well for the secure VM stuff as for anything
else.
diff mbox series

Patch

diff --git a/hw/core/machine.c b/hw/core/machine.c
index a71792bc16..8dfc1bb3f8 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/host-trust-limitation.h"
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-pci.h"
 
 GlobalProperty hw_compat_5_0[] = {
     { "virtio-balloon-device", "page-poison", "false" },
@@ -1165,6 +1167,15 @@  void machine_run_board_init(MachineState *machine)
          * areas.
          */
         machine_set_mem_merge(OBJECT(machine), false, &error_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);