On Wed, 26 Feb 2020 16:11:03 +0100 Janosch Frank 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 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 --- 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. --- 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 -- 2.17.1