Re: [RFC v2 18/18] guest memory protection: Alter virtio default properties for protected guests

[Halil Pasic]

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

pgpZfVSJ1hvfL.pgp
Description: OpenPGP digital signature