Re: [for-5.2 v4 10/10] s390: Recognize host-trust-limitation option

From: Greg Kurz <groug@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: "Halil Pasic" <pasic@linux.ibm.com>,
	pair@us.ibm.com, brijesh.singh@amd.com, frankja@linux.ibm.com,
	kvm@vger.kernel.org, "Michael S. Tsirkin" <mst@redhat.com>,
	"Cornelia Huck" <cohuck@redhat.com>,
	"David Hildenbrand" <david@redhat.com>,
	qemu-devel@nongnu.org, dgilbert@redhat.com,
	"Christian Borntraeger" <borntraeger@de.ibm.com>,
	qemu-s390x@nongnu.org, qemu-ppc@nongnu.org,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	marcel.apfelbaum@gmail.com, "Thomas Huth" <thuth@redhat.com>,
	pbonzini@redhat.com, "Richard Henderson" <rth@twiddle.net>,
	mdroth@linux.vnet.ibm.com, ehabkost@redhat.com
Subject: Re: [for-5.2 v4 10/10] s390: Recognize host-trust-limitation option
Date: Fri, 11 Sep 2020 08:25:52 +0200	[thread overview]
Message-ID: <20200911082552.1b7d7bde@bahia.lan> (raw)
In-Reply-To: <20200911000718.GF66834@yekko.fritz.box>

[-- Attachment #1: Type: text/plain, Size: 4487 bytes --]

On Fri, 11 Sep 2020 10:07:18 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Sep 10, 2020 at 08:29:24PM +0200, Halil Pasic wrote:
> > On Thu, 10 Sep 2020 13:36:09 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > On Mon, 7 Sep 2020 17:22:53 +0200
> > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > > 
> > > > On Fri, 24 Jul 2020 12:57:44 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > 
> > > > > At least some s390 cpu models support "Protected Virtualization" (PV),
> > > > > a mechanism to protect guests from eavesdropping by a compromised
> > > > > hypervisor.
> > > > > 
> > > > > This is similar in function to other mechanisms like AMD's SEV and
> > > > > POWER's PEF, which are controlled bythe "host-trust-limitation"
> > > > > machine option.  s390 is a slightly special case, because we already
> > > > > supported PV, simply by using a CPU model with the required feature
> > > > > (S390_FEAT_UNPACK).
> > > > > 
> > > > > To integrate this with the option used by other platforms, we
> > > > > implement the following compromise:
> > > > > 
> > > > >  - When the host-trust-limitation option is set, s390 will recognize
> > > > >    it, verify that the CPU can support PV (failing if not) and set
> > > > >    virtio default options necessary for encrypted or protected guests,
> > > > >    as on other platforms.  i.e. if host-trust-limitation is set, we
> > > > >    will either create a guest capable of entering PV mode, or fail
> > > > >    outright  
> > > > 
> > > > Shouldn't we also fail outright if the virtio features are not PV
> > > > compatible (invalid configuration)?
> > > > 
> > > > I would like to see something like follows as a part of this series.
> > > > ----------------------------8<--------------------------
> > > > From: Halil Pasic <pasic@linux.ibm.com>
> > > > Date: Mon, 7 Sep 2020 15:00:17 +0200
> > > > Subject: [PATCH] virtio: handle host trust limitation
> > > > 
> > > > If host_trust_limitation_enabled() returns true, then emulated virtio
> > > > devices must offer VIRTIO_F_ACCESS_PLATFORM, because the device is not
> > > > capable of accessing all of the guest memory. Otherwise we are in
> > > > violation of the virtio specification.
> > > > 
> > > > Let's fail realize if we detect that VIRTIO_F_ACCESS_PLATFORM feature is
> > > > obligatory but missing.
> > > > 
> > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > > ---
> > > >  hw/virtio/virtio.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index 5bd2a2f621..19b4b0a37a 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include "hw/virtio/virtio-access.h"
> > > >  #include "sysemu/dma.h"
> > > >  #include "sysemu/runstate.h"
> > > > +#include "exec/host-trust-limitation.h"
> > > >  
> > > >  /*
> > > >   * The alignment to use between consumer and producer parts of vring.
> > > > @@ -3618,6 +3619,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
> > > >      /* Devices should either use vmsd or the load/save methods */
> > > >      assert(!vdc->vmsd || !vdc->load);
> > > >  
> > > > +    if (host_trust_limitation_enabled(MACHINE(qdev_get_machine()))
> > > > +        && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> > > > +        error_setg(&err, "devices without VIRTIO_F_ACCESS_PLATFORM are not compatible with host trust imitation");
> > > > +        error_propagate(errp, err);
> > > > +        return;
> > > 
> > > How can we get here? I assume only if the user explicitly turned the
> > > feature off while turning HTL on, as otherwise patch 9 should have
> > > taken care of it?
> > > 
> > 
> > Yes, we can get here only if iommu_platform is explicitly turned off.
> 
> Right.. my assumption was that if you really want to specify
> contradictory options, you get to keep both pieces.  Or, more
> seriously, there might be some weird experimental cases where this
> combination could do something useful if you really know what you're
> doing, and explicitly telling qemu to do this implies you know what
> you're doing.
> 

I guess this deserves at least a warning for the case of someone that
doesn't really know what they're doing, eg. borrowing a complex QEMU
command line or libvirt XML from somewhere else ?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]