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

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 8 Jun 2020 19:00:45 +0200
Halil Pasic <pasic@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 <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).  
> > 
> > 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 <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;
> > > +    }  
> > 
> > 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.