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

["Daniel P. Berrangé" <berrange@redhat.com>]

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