On Wed, 20 May 2020 12:23:24 -0400 "Michael S. Tsirkin" <m...@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 <pa...@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 <pa...@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