Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode

[Halil Pasic]

On Thu, 27 Feb 2020 13:24:02 +0100
Halil Pasic <address@hidden> wrote:

> On Wed, 26 Feb 2020 16:11:03 +0100
> Janosch Frank <address@hidden> wrote:
> 
> > On 2/26/20 3:59 PM, David Hildenbrand wrote:
> > > On 26.02.20 13:20, Janosch Frank wrote:
> > >> Ballooning in protected VMs can only be done when the guest shares the
> > >> pages it gives to the host. Hence, until we have a solution for this
> > >> in the guest kernel, we inhibit ballooning when switching into
> > >> protected mode and reverse that once we move out of it.
> > > 
> > > I don't understand what you mean here, sorry. zapping a page will mean
> > > that a fresh one will be faulted in when accessed. And AFAIK, that means
> > > it will be encrypted again when needed.
> > 
> > Yes, as soon as the host alters non-shared memory we'll run into
> > integrity issues.
> > 
> > 
> > I've been talking to Halil after I sent this out and it looks like we'll
> > rather try to automatically enable the IOMMU for all devices when
> > switching into protected mode. He said that if the IOMMU is set the
> > balloon code will do an early exit on feature negotiation.
> > 
> 
> I have a discussion starter RFC for you.
> 
> --------------------------8<----------------------------------------------
> From: Halil Pasic <address@hidden>
> Date: Wed, 26 Feb 2020 16:48:21 +0100
> Subject: [RFC PATCH 1/1] virtio-ccw: auto-manage VIRTIO_F_IOMMU_PLATFORM
> 
> 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 VM, as the device can access only memory addresses that are in
> pages that are currently shared (only the guest can share/unsare its
> page).
> 
> 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. If the correctness of the
> paravirtualized virtio devices depends (and thus in a sense the
> correctness of the hypervisor), then the hypervisor should have the last
> word about whether VIRTIO_F_ACCESS_PLATFORM is to be presented or not.
> 
> Let's manage the VIRTIO_F_ACCESS_PLATFORM virtio feature automatically
> for virtio-ccw devices, so that we set it before we start the protected
> configuration, and unset it when our VM is not protected.
> 
> Signed-off-by: Halil Pasic <address@hidden>
> ---
> NOTES:
> * I wanted to have a discussion starter fast, there are multiple open
> questions.
> 
> * Doing more than one system_resets() is hackish.  We
> should look into this.
> 
> * The user interface implications of this patch are also an ugly can of
> worms. Needs to be discussed.
> 
> * We should consider keeping the original value if !pv and restoring it
> on pv --> !pv, instead of forcing to unset when leaving pv, and actually
> not forcing unset when !pv.
> 
> * It might make sense to do something like this in virtio core, but I
>   decided start the discussion with a ccw-only change.
> 
> * Maybe we need a machine option that enables this sort of behavior,
> especially if we decide not to do the conserving/restoring.
> 
> * Lightly tested.

ping

Any interest in this?

> ---
>  hw/s390x/s390-virtio-ccw.c |  4 ++--
>  hw/s390x/virtio-ccw.c      | 13 +++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 0f4455d1df..996124f152 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -337,7 +337,7 @@ static void s390_machine_unprotect(S390CcwMachineState 
> *ms)
>          ms->pv = false;
>      }
>      migrate_del_blocker(pv_mig_blocker);
> -    qemu_balloon_inhibit(false);
> +    subsystem_reset();
>  }
>  
>  static int s390_machine_protect(S390CcwMachineState *ms)
> @@ -346,7 +346,6 @@ static int s390_machine_protect(S390CcwMachineState *ms)
>      CPUState *t;
>      int rc;
>  
> -    qemu_balloon_inhibit(true);
>      if (!pv_mig_blocker) {
>          error_setg(&pv_mig_blocker,
>                     "protected VMs are currently not migrateable.");
> @@ -384,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 13f57e7b67..48bb54f68e 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -869,12 +869,24 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t 
> vector)
>      }
>  }
>  
> +static inline void virtio_ccw_pv_enforce_features(VirtIODevice *vdev)
> +{
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
> +
> +    if (ms->pv) {
> +        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> +    } else {
> +        virtio_clear_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> +    }
> +}
> +
>  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);
>  
> +    virtio_ccw_pv_enforce_features(vdev);
>      virtio_ccw_reset_virtio(dev, vdev);
>      if (vdc->parent_reset) {
>          vdc->parent_reset(d);
> @@ -1103,6 +1115,7 @@ static void virtio_ccw_pre_plugged(DeviceState *d, 
> Error **errp)
>      if (dev->max_rev >= 1) {
>          virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
>      }
> +    virtio_ccw_pv_enforce_features(vdev);
>  }
>  
>  /* This is called by virtio-bus just after the device is plugged. */
> 
> base-commit: 8665f2475f5999d4c9f33598f1360f0b0797c489

pgp0iuXv83AMv.pgp
Description: OpenPGP digital signature