| Message ID | 20200521034304.340040-19-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor configuration of guest memory protection | expand |
On Thu, 21 May 2020 13:43:04 +1000 David Gibson <david@gibson.dropbear.id.au> 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 <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 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, &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);
On Fri, 5 Jun 2020 12:45:35 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Thu, 21 May 2020 13:43:04 +1000 > David Gibson <david@gibson.dropbear.id.au> 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
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 <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 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, &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"); > } > 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
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 <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 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, &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"); > > } > > > > 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.
On Sun, 7 Jun 2020 13:07:35 +1000 David Gibson <david@gibson.dropbear.id.au> 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 <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 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, &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"); > > > } > > > > > > > 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?
On Tue, 9 Jun 2020 12:16:41 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Sun, 7 Jun 2020 13:07:35 +1000 > David Gibson <david@gibson.dropbear.id.au> 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 <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 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, &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"); > > > > } > > > > > > > > > > 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
On Tue, 9 Jun 2020 17:40:46 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On Tue, 9 Jun 2020 12:16:41 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Sun, 7 Jun 2020 13:07:35 +1000 > > David Gibson <david@gibson.dropbear.id.au> 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 <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 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, &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"); > > > > > } > > > > > > > > > > > > > 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.
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.
On Tue, Jun 09, 2020 at 12:16:41PM +0200, Cornelia Huck wrote: > On Sun, 7 Jun 2020 13:07:35 +1000 > David Gibson <david@gibson.dropbear.id.au> 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 <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 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, &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"); > > > > } > > > > > > > > > > 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.
On Tue, Jun 09, 2020 at 05:40:46PM +0200, Halil Pasic wrote: > On Tue, 9 Jun 2020 12:16:41 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Sun, 7 Jun 2020 13:07:35 +1000 > > David Gibson <david@gibson.dropbear.id.au> 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 <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 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, &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"); > > > > > } > > > > > > > > > > > > > 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 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
On Wed, 10 Jun 2020 14:39:22 +1000 David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <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 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, &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"); > > > > > } > > > > > > > > > > > > > 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.
On Wed, Jun 10, 2020 at 10:48:42AM +0200, Cornelia Huck wrote: > On Wed, 10 Jun 2020 14:39:22 +1000 > David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <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 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, &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"); > > > > > > } > > > > > > > > > > > > > > > > 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.
On Wed, 10 Jun 2020 10:48:42 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Wed, 10 Jun 2020 14:39:22 +1000 > David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <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 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, &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"); > > > > > > } > > > > > > > > > > > > > > > > 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. [..]
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, &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 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 <david@gibson.dropbear.id.au> --- hw/core/machine.c | 11 +++++++++++ 1 file changed, 11 insertions(+)