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

[Daniel P . Berrangé]

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
-- 
|: 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 :|