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

[Michael S. Tsirkin]

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

> 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).
> 
> Let us manage the VIRTIO_F_ACCESS_PLATFORM virtio feature automatically
> for virtio-ccw devices, i.e. force it before we start the protected VM.
> If the VM should cease to be protected, the original value is restored.
> 
> Signed-off-by: Halil Pasic <address@hidden>


I don't really understand things fully but it looks like you are
changing features of a device.  If so this bothers me, resets
happen at random times while driver is active, and we never
expect features to change.


> ---
> 
> NOTES:
> 
> * Doing more system_resets() is a big hack.  We should look into this.
> * The user interface implications of this patch are also an ugly can of
> worms. We need to discuss them.
> 
> 
> v1 --> v2:
> * Use the default or user supplied iommu_on flag when when !PV
> * Use virtio functions for feature manipulation
> 
> Link to v1:
> https://www.mail-archive.com/address@hidden/msg683775.html
> 
> Unfortunately the v1 did not see much discussion because we had more
> pressing issues.
> 
> ---
>  hw/s390x/s390-virtio-ccw.c |  2 ++
>  hw/s390x/virtio-ccw.c      | 14 ++++++++++++++
>  hw/s390x/virtio-ccw.h      |  1 +
>  3 files changed, 17 insertions(+)
> 
> 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..67d5bc68ba 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 (ms->pv && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> +        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> +        dev->forced_iommu_platform = true;
> +    } else if (!ms->pv && dev->forced_iommu_platform) {
> +        virtio_clear_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> +        dev->forced_iommu_platform = false;
> +    }
>  
>      virtio_ccw_reset_virtio(dev, vdev);
>      if (vdc->parent_reset) {
> diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> index 3453aa1f98..34ff7b0b4e 100644
> --- a/hw/s390x/virtio-ccw.h
> +++ b/hw/s390x/virtio-ccw.h
> @@ -99,6 +99,7 @@ struct VirtioCcwDevice {
>      IndAddr *summary_indicator;
>      uint64_t ind_bit;
>      bool force_revision_1;
> +    bool forced_iommu_platform;
>  };
>  
>  /* The maximum virtio revision we support. */
> 

This seems unused. Why is it helpful?


> base-commit: 0ffd3d64bd1bb8b84950e52159a0062fdab34628
> -- 
> 2.17.1