| Message ID | 20210202041315.196530-13-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | Generalize memory encryption models | expand |
On Tue, Feb 02, 2021 at 03:13:14PM +1100, 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 confidential guest 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> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Reviewed-by: Greg Kurz <groug@kaod.org> > --- > hw/core/machine.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 94194ab82d..497949899b 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -33,6 +33,8 @@ > #include "migration/global_state.h" > #include "migration/vmstate.h" > #include "exec/confidential-guest-support.h" > +#include "hw/virtio/virtio.h" > +#include "hw/virtio/virtio-pci.h" > > GlobalProperty hw_compat_5_2[] = {}; > const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2); > @@ -1196,6 +1198,17 @@ 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 also disabling legacy virtio > + * support for those virtio pci devices which allow it. > + */ > + object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy", > + "on", true); > + object_register_sugar_prop(TYPE_VIRTIO_DEVICE, "iommu_platform", > + "on", false); So overriding a boolean property always poses a problem: if user does set iommu_platform=off we are ignoring this silently. Can we change iommu_platform to on/off/auto? Then we can change how does auto behave. Bonus points for adding "access_platform" and making it a synonym of platform_iommu. > } > > machine_class->init(machine); > -- > 2.29.2
On Tue, Feb 02, 2021 at 06:06:34PM -0500, Michael S. Tsirkin wrote: > On Tue, Feb 02, 2021 at 03:13:14PM +1100, 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 confidential guest 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> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > > Reviewed-by: Greg Kurz <groug@kaod.org> > > > > --- > > hw/core/machine.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index 94194ab82d..497949899b 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -33,6 +33,8 @@ > > #include "migration/global_state.h" > > #include "migration/vmstate.h" > > #include "exec/confidential-guest-support.h" > > +#include "hw/virtio/virtio.h" > > +#include "hw/virtio/virtio-pci.h" > > > > GlobalProperty hw_compat_5_2[] = {}; > > const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2); > > @@ -1196,6 +1198,17 @@ 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 also disabling legacy virtio > > + * support for those virtio pci devices which allow it. > > + */ > > + object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy", > > + "on", true); > > + object_register_sugar_prop(TYPE_VIRTIO_DEVICE, "iommu_platform", > > + "on", false); > > So overriding a boolean property always poses a problem: > if user does set iommu_platform=off we are ignoring this > silently. No, we don't. That's why this is register_sugar_prop() rather than an outright set_prop(). An explicitly given option will take precedence. > Can we change iommu_platform to on/off/auto? Then we can > change how does auto behave. I've never had a satisfactory explanation of what the semantics of "auto" need to be. > > Bonus points for adding "access_platform" and making it > a synonym of platform_iommu. > > > } > > > > machine_class->init(machine); >
diff --git a/hw/core/machine.c b/hw/core/machine.c index 94194ab82d..497949899b 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -33,6 +33,8 @@ #include "migration/global_state.h" #include "migration/vmstate.h" #include "exec/confidential-guest-support.h" +#include "hw/virtio/virtio.h" +#include "hw/virtio/virtio-pci.h" GlobalProperty hw_compat_5_2[] = {}; const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2); @@ -1196,6 +1198,17 @@ 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 also disabling legacy virtio + * support for those virtio pci devices which allow it. + */ + object_register_sugar_prop(TYPE_VIRTIO_PCI, "disable-legacy", + "on", true); + object_register_sugar_prop(TYPE_VIRTIO_DEVICE, "iommu_platform", + "on", false); } machine_class->init(machine);