On Mon, 8 Jun 2020 19:00:45 +0200 Halil Pasic <pa...@linux.ibm.com> wrote:
> > 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. > > > > I'm going to definitely have a good look at that. What I think special > about s390 is that F_ACCESS_PLATFORM is hurting us because all IO needs > to go through ZONE_DMA (this is a problem of the implementation that > stemming form a limitation of the DMA API, upstream didn't let me > fix it). My understanding is that power runs into similar issues, but I don't know much about power, so I might be entirely wrong :) > > > > > > > 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? > > > > > > Right, I'm missing the most important words. > > > > > > > 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. > > I totally agree. My previous patch didn't have the problem because there > we just forced what we need. > > > > > (Can we disallow transition to pv if it is off? Maybe with the machine > > property approach from the cross-arch patchset?) > > No we can't disallow transition because for hotplug that already > happened. I mean, can a virtio devices without IOMMU_PLATFORM act as a transition blocker (i.e. an attempt by a guest to move to pv would fail?) > > I can't say anything about the cross-arch patchset. Will come back to you > once I'm smarter. > > > > > > > > > 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. > > > > Right, this is more about the machine than the transport. I was thinking > of a machine hook, but decided to discuss the more basic stuff first > (i.e. is it OK to change the property after stuff is realized). > > This would already fix the most pressing issue which is virtio-ccw. I > didn't even bother checking if virtio-pci works with PV out of the box, > or if something needs to be done there. My bet is that it does not work. I agree, virtio-pci + pv is unlikely to work. But if at all possible, I'd prefer a general solution anyway, as other architectures care about virtio-pci.