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

[Cornelia Huck]

On Sat, 6 Jun 2020 01:32:17 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

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


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

(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