Re: [PATCH v2 1/1] virtio-ccw: auto-manage VIRTIO_F_IOMMU_PLATFORM if PV

[Halil Pasic]

On Wed, 20 May 2020 12:23:24 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, May 15, 2020 at 12:11:55AM +0200, Halil Pasic wrote:
> > 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 VMs, as the device can access only memory addresses that are
> > in pages that are currently shared (only the guest can share/unsare its
> > pages).
> > 
> > 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. Since the correctness of the
> > paravirtualized virtio devices depends (and thus in a sense the
> > correctness of the hypervisor) it, then the hypervisor should have the
> > last word about whether VIRTIO_F_ACCESS_PLATFORM is to be presented or
> > not.
> 
> So, how about this: switch iommu to on/off/auto.  Add a property with a
> reasonable name "allow protected"?  If set allow switch to protected
> memory and also set iommu auto to on by default.  If not set then don't.
> 
> This will come handy for other things like migrating to hosts without
> protected memory support.
> 
> 
> Also, virtio now calls this PLATFORM_ACCESS, maybe we should rename
> the property (keeping old one around for compat)?
> I feel this will address lots of complaints ...
> 

I'm not sure I've entirely understood your proposal, but I tried to
do something in that direction. 

Short summary of the changes: 
* added new property "access_platform" as on/off/auto (default auto)
* added alias "iommu_platform" for compatibility
* rewrote this patch to on/off/auto so that we only do the override when
  user specified auto 

Let me list some pros and cons (compared to the previous patch):

PRO:
* Thanks to on/off/auto we don't override what the user specified. From 
user interface perspective preferable. I usually hate software that
thinks its than me and can do the opposite I tell it.

CON:
* It is more code: "4 files changed, 37 insertions(+), 2 deletions(-)"
  against "3 files changed, 17 insertions(+)"
* Unlike the previous one, this one is not fool-proof! The user can
  still specify access_platform=off to lets say a hotplug device, and
  bring down the guest. We could however fence such stuff with an error
  message. Would be even more code though.
* As far as I can tell 'auto' was used to pick a value on initialization
  time. This is a novel, and possibly dodgy use in a sense that the value
  of the property may change during the lifetime of the VM.
* We may need to do something about libvirt.

Further improvements are possible and probably necessary if we want
to go down this path. But I would like to verify that first.

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

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 VMs, as the device can access only memory addresses that are
in pages that are currently shared (only the guest can share/unsare its
pages).

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. Since the correctness of the
paravirtualized virtio devices depends (and thus in a sense the
correctness of the hypervisor) it, then the hypervisor should have the
last word about whether VIRTIO_F_ACCESS_PLATFORM is to be presented or
not.

Currently presenting a PV guest with a (paravirtualized) virtio-ccw
device has catastrophic consequences for the VM (after the hypervisors
access to protected memory). This is especially grave in case of device
hotplug (because in this case the guest is more likely to be in the
middle of something important).

Let us add the ability to manage the VIRTIO_F_ACCESS_PLATFORM virtio
feature automatically. This is accomplished  by turning the property
into an 'on/off/auto' property, and for virtio-ccw devices if auto
was specified forcing its value before  we start the protected VM. If
the VM should cease to be protected, the original value is restored.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c |  2 ++
 hw/s390x/virtio-ccw.c      | 14 ++++++++++++++
 hw/virtio/virtio.c         | 19 +++++++++++++++++++
 include/hw/virtio/virtio.h |  4 ++--
 4 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f660070d22..705e6b153a 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -330,6 +330,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
     migrate_del_blocker(pv_mig_blocker);
     error_free_or_abort(&pv_mig_blocker);
     qemu_balloon_inhibit(false);
+    subsystem_reset();
 }
 
 static int s390_machine_protect(S390CcwMachineState *ms)
@@ -382,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 64f928fc7d..2bb29b57aa 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -874,6 +874,20 @@ 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);
+    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+
+    /*
+     * An attempt to use a paravirt device without VIRTIO_F_IOMMU_PLATFORM
+     * in PV, has catastrophic consequences for the VM. Let's force
+     * VIRTIO_F_IOMMU_PLATFORM not already specified.
+     */
+    if (vdev->access_platform_auto && ms->pv) {
+        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
+        vdev->access_platform = ON_OFF_AUTO_ON;
+    } else if (vdev->access_platform_auto) {
+        virtio_clear_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
+        vdev->access_platform = ON_OFF_AUTO_OFF;
+    }
 
     virtio_ccw_reset_virtio(dev, vdev);
     if (vdc->parent_reset) {
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index b6c8ef5bc0..f6bd271f14 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3232,7 +3232,11 @@ void virtio_instance_init_common(Object *proxy_obj, void 
*data,
 
     object_initialize_child(proxy_obj, "virtio-backend", vdev, vdev_size,
                             vdev_name, &error_abort, NULL);
+    object_property_add_alias(OBJECT(vdev), "iommu_platform",
+                              OBJECT(vdev), "access_platform", &error_abort);
     qdev_alias_all_properties(vdev, proxy_obj);
+    object_property_add_alias(proxy_obj, "iommu_platform",
+                              OBJECT(vdev), "access_platform", &error_abort);
 }
 
 void virtio_init(VirtIODevice *vdev, const char *name,
@@ -3626,6 +3630,19 @@ static void virtio_device_realize(DeviceState *dev, 
Error **errp)
         return;
     }
 
+    switch (vdev->access_platform) {
+    case ON_OFF_AUTO_ON:
+        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
+        break;
+    case ON_OFF_AUTO_AUTO:
+        /* transport code can mange access_platform */
+        vdev->access_platform_auto = true;
+        break;
+    case ON_OFF_AUTO_OFF: /*fall through*/
+    default:
+        vdev->access_platform_auto = false;
+    }
+
     vdev->listener.commit = virtio_memory_listener_commit;
     memory_listener_register(&vdev->listener, vdev->dma_as);
 }
@@ -3681,6 +3698,8 @@ static Property virtio_properties[] = {
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
     DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
     DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, 
true),
+    DEFINE_PROP_ON_OFF_AUTO("access_platform", VirtIODevice, access_platform,
+                            ON_OFF_AUTO_AUTO),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b69d517496..b77e1545b4 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -110,6 +110,8 @@ struct VirtIODevice
     uint8_t device_endian;
     bool use_guest_notifier_mask;
     AddressSpace *dma_as;
+    OnOffAuto access_platform;
+    bool access_platform_auto;
     QLIST_HEAD(, VirtQueue) *vector_queues;
 };
 
@@ -289,8 +291,6 @@ typedef struct VirtIORNGConf VirtIORNGConf;
                       VIRTIO_F_NOTIFY_ON_EMPTY, true), \
     DEFINE_PROP_BIT64("any_layout", _state, _field, \
                       VIRTIO_F_ANY_LAYOUT, true), \
-    DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
-                      VIRTIO_F_IOMMU_PLATFORM, false), \
     DEFINE_PROP_BIT64("packed", _state, _field, \
                       VIRTIO_F_RING_PACKED, false)
 

base-commit: 0ffd3d64bd1bb8b84950e52159a0062fdab34628
-- 
2.17.1