On Sat, 6 Jun 2020 01:32:17 +0200 Halil Pasic <pa...@linux.ibm.com> wrote:
> 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. Agreed. > > 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. I think trying to hotplug such a device to a guest running in protected mode should simply fail (and not crash anything.) > * 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. You mean that we start the vm once with support for prot virt, and later without? > * We may need to do something about libvirt. I'm also not 100% sure about migration... would it make sense to discuss all of this in the context of the cross-arch patchset? It seems power has similar issues. > > 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). You mean for virtio-ccw devices that don't have iommu_on, right? > > 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; > + } If the consequences are so dire, we really should disallow adding a device of IOMMU_PLATFORM off if pv is on. (Can we disallow transition to pv if it is off? Maybe with the machine property approach from the cross-arch patchset?) > > 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; Can we really make that transport-specific? While ccw implies s390, pci might be a variety of architectures. > + 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), \ I'm wondering about migration compat. > DEFINE_PROP_BIT64("packed", _state, _field, \ > VIRTIO_F_RING_PACKED, false) > > > base-commit: 0ffd3d64bd1bb8b84950e52159a0062fdab34628