Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode

[Halil Pasic <pasic@linux.ibm.com>]

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

On Wed, 26 Feb 2020 16:11:03 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 2/26/20 3:59 PM, David Hildenbrand wrote:
> > On 26.02.20 13:20, Janosch Frank wrote:
> >> Ballooning in protected VMs can only be done when the guest shares the
> >> pages it gives to the host. Hence, until we have a solution for this
> >> in the guest kernel, we inhibit ballooning when switching into
> >> protected mode and reverse that once we move out of it.
> > 
> > I don't understand what you mean here, sorry. zapping a page will mean
> > that a fresh one will be faulted in when accessed. And AFAIK, that means
> > it will be encrypted again when needed.
> 
> Yes, as soon as the host alters non-shared memory we'll run into
> integrity issues.
> 
> 
> I've been talking to Halil after I sent this out and it looks like we'll
> rather try to automatically enable the IOMMU for all devices when
> switching into protected mode. He said that if the IOMMU is set the
> balloon code will do an early exit on feature negotiation.
> 

I have a discussion starter RFC for you.

--------------------------8<----------------------------------------------
From: Halil Pasic <pasic@linux.ibm.com>
Date: Wed, 26 Feb 2020 16:48:21 +0100
Subject: [RFC PATCH 1/1] virtio-ccw: auto-manage VIRTIO_F_IOMMU_PLATFORM

The virtio specification tells that the device is to present
VIRTIO_F_ACCESS_PLATFORM (a.k.a. VIRTIO_F_IOMMU_PLATFORM) when the
device "can only access certain memory addresses with said access
specified and/or granted by the platform". This is the case for a
protected VM, as the device can access only memory addresses that are in
pages that are currently shared (only the guest can share/unsare its
page).

No VM however starts out as a protected VM, but some VMs may be
converted to protected VMs if the guest decides so.

Making the end user explicitly manage the VIRTIO_F_ACCESS_PLATFORM via
the property iommu_on is a minor disaster. If the correctness of the
paravirtualized virtio devices depends (and thus in a sense the
correctness of the hypervisor), then the hypervisor should have the last
word about whether VIRTIO_F_ACCESS_PLATFORM is to be presented or not.

Let's manage the VIRTIO_F_ACCESS_PLATFORM virtio feature automatically
for virtio-ccw devices, so that we set it before we start the protected
configuration, and unset it when our VM is not protected.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
NOTES:
* I wanted to have a discussion starter fast, there are multiple open
questions.

* Doing more than one system_resets() is hackish.  We
should look into this.

* The user interface implications of this patch are also an ugly can of
worms. Needs to be discussed.

* We should consider keeping the original value if !pv and restoring it
on pv --> !pv, instead of forcing to unset when leaving pv, and actually
not forcing unset when !pv.

* It might make sense to do something like this in virtio core, but I
  decided start the discussion with a ccw-only change.

* Maybe we need a machine option that enables this sort of behavior,
especially if we decide not to do the conserving/restoring.

* Lightly tested.
---
 hw/s390x/s390-virtio-ccw.c |  4 ++--
 hw/s390x/virtio-ccw.c      | 13 +++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 0f4455d1df..996124f152 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -337,7 +337,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
         ms->pv = false;
     }
     migrate_del_blocker(pv_mig_blocker);
-    qemu_balloon_inhibit(false);
+    subsystem_reset();
 }
 
 static int s390_machine_protect(S390CcwMachineState *ms)
@@ -346,7 +346,6 @@ static int s390_machine_protect(S390CcwMachineState *ms)
     CPUState *t;
     int rc;
 
-    qemu_balloon_inhibit(true);
     if (!pv_mig_blocker) {
         error_setg(&pv_mig_blocker,
                    "protected VMs are currently not migrateable.");
@@ -384,6 +383,7 @@ static int s390_machine_protect(S390CcwMachineState *ms)
     if (rc) {
         goto out_err;
     }
+    subsystem_reset();
     return rc;
 
 out_err:
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 13f57e7b67..48bb54f68e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -869,12 +869,24 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
     }
 }
 
+static inline void virtio_ccw_pv_enforce_features(VirtIODevice *vdev)
+{
+    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+
+    if (ms->pv) {
+        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
+    } else {
+        virtio_clear_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
+    }
+}
+
 static void virtio_ccw_reset(DeviceState *d)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
     VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
     VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
 
+    virtio_ccw_pv_enforce_features(vdev);
     virtio_ccw_reset_virtio(dev, vdev);
     if (vdc->parent_reset) {
         vdc->parent_reset(d);
@@ -1103,6 +1115,7 @@ static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
     if (dev->max_rev >= 1) {
         virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
     }
+    virtio_ccw_pv_enforce_features(vdev);
 }
 
 /* This is called by virtio-bus just after the device is plugged. */

base-commit: 8665f2475f5999d4c9f33598f1360f0b0797c489
-- 
2.17.1


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