On Thu, 27 Feb 2020 13:24:02 +0100 Halil Pasic <pa...@linux.ibm.com> wrote:
> On Wed, 26 Feb 2020 16:11:03 +0100
> Janosch Frank <fran...@linux.ibm.com> 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 <pa...@linux.ibm.com>
> 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 <pa...@linux.ibm.com>
> ---
> 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
pgp496Fzdzm1u.pgp
Description: OpenPGP digital signature
