| Message ID | 20200619020602.118306-10-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generalize memory encryption models | expand |
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
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 :|
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 :|
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
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
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
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
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?
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
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.
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).
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
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 --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);
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(+)