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

[Halil Pasic]

[..]
> > 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.)

Yes, if on/off/auto with a similar semantics like here is the path
we are going to walk, I will definitely have to throw in some code that
fails the hotplug of such devices.

> 
> > * 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?

No, this patch does not care if VM supports prot virt or not, it only
cares about the mode we are running in (prot virt or not). That is, I
still might add F_ACCESS_PLATFORM when the VM gets transitioned to a
prot virt VM. And this is something other uses of OnOffAuto don't seem
to do. 

> 
> > * 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.
> 

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). 

> > 
> > 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 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.

> > +        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.

Should be fine, I have the alias for that. But if this is the
path we are taking I will definitely test it.

Thanks for having a look and for all the good questions!

Regards,
Halil 


> 
> >      DEFINE_PROP_BIT64("packed", _state, _field, \
> >                        VIRTIO_F_RING_PACKED, false)
> >  
> > 
> > base-commit: 0ffd3d64bd1bb8b84950e52159a0062fdab34628
>