Messages in this thread | | | Date | Mon, 20 Jun 2022 11:41:27 +0200 | From | Janosch Frank <> | Subject | Re: [PATCH v11 15/19] KVM: s390: pv: asynchronous destroy for reboot |
| |
On 6/3/22 08:56, Claudio Imbrenda wrote: > Until now, destroying a protected guest was an entirely synchronous > operation that could potentially take a very long time, depending on > the size of the guest, due to the time needed to clean up the address > space from protected pages. > > This patch implements an asynchronous destroy mechanism, that allows a > protected guest to reboot significantly faster than previously. > > This is achieved by clearing the pages of the old guest in background. > In case of reboot, the new guest will be able to run in the same > address space almost immediately. > > The old protected guest is then only destroyed when all of its memory has > been destroyed or otherwise made non protected. > > Two new PV commands are added for the KVM_S390_PV_COMMAND ioctl: > > KVM_PV_ASYNC_DISABLE_PREPARE: prepares the current protected VM for > asynchronous teardown. The current VM will then continue immediately > as non-protected. If a protected VM had already been set aside without > starting the teardown process, this call will fail. > > KVM_PV_ASYNC_DISABLE: tears down the protected VM previously set aside > for asynchronous teardown. This PV command should ideally be issued by > userspace from a separate thread. If a fatal signal is received (or the > process terminates naturally), the command will terminate immediately > without completing. > > Leftover protected VMs are cleaned up when a KVM VM is torn down > normally (either via IOCTL or when the process terminates); this > cleanup has been implemented in a previous patch. > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 34 +++++++++- > arch/s390/kvm/kvm-s390.h | 2 + > arch/s390/kvm/pv.c | 131 +++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/kvm.h | 2 + > 4 files changed, 166 insertions(+), 3 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 369de8377116..842419092c0c 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2256,9 +2256,13 @@ static int kvm_s390_cpus_to_pv(struct kvm *kvm, u16 *rc, u16 *rrc) > > static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd) > { > + const bool needslock = (cmd->cmd != KVM_PV_ASYNC_DISABLE); > + void __user *argp = (void __user *)cmd->data; > int r = 0; > u16 dummy; > - void __user *argp = (void __user *)cmd->data; > + > + if (needslock) > + mutex_lock(&kvm->lock); > > switch (cmd->cmd) { > case KVM_PV_ENABLE: { > @@ -2292,6 +2296,28 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd) > set_bit(IRQ_PEND_EXT_SERVICE, &kvm->arch.float_int.masked_irqs); > break; > } > + case KVM_PV_ASYNC_DISABLE_PREPARE: > + r = -EINVAL; > + if (!kvm_s390_pv_is_protected(kvm) || !async_destroy) > + break; > + > + r = kvm_s390_cpus_from_pv(kvm, &cmd->rc, &cmd->rrc); > + /* > + * If a CPU could not be destroyed, destroy VM will also fail. > + * There is no point in trying to destroy it. Instead return > + * the rc and rrc from the first CPU that failed destroying. > + */ > + if (r) > + break; > + r = kvm_s390_pv_deinit_vm_async_prepare(kvm, &cmd->rc, &cmd->rrc); > + > + /* no need to block service interrupts any more */ > + clear_bit(IRQ_PEND_EXT_SERVICE, &kvm->arch.float_int.masked_irqs); > + break; > + case KVM_PV_ASYNC_DISABLE:
I'd like to have the ASYNC and DISABLE swapped so we're in line with the normal disable. Also renaming KVM_PV_ASYNC_DISABLE to KVM_PV_ASYNC_DISABLE_EXECUTE or something similar would make sense to me.
> + /* This must not be called while holding kvm->lock */ > + r = kvm_s390_pv_deinit_vm_async(kvm, &cmd->rc, &cmd->rrc); > + break; > case KVM_PV_DISABLE: { > r = -EINVAL; > if (!kvm_s390_pv_is_protected(kvm)) > @@ -2393,6 +2419,9 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd) > default: > r = -ENOTTY; > } > + if (needslock) > + mutex_unlock(&kvm->lock); > + > return r; > } > > @@ -2597,9 +2626,8 @@ long kvm_arch_vm_ioctl(struct file *filp, > r = -EINVAL; > break; > } > - mutex_lock(&kvm->lock); > + /* must be called without kvm->lock */ > r = kvm_s390_handle_pv(kvm, &args); > - mutex_unlock(&kvm->lock); > if (copy_to_user(argp, &args, sizeof(args))) { > r = -EFAULT; > break; > diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h > index d3abedafa7a8..d296afb6041c 100644 > --- a/arch/s390/kvm/kvm-s390.h > +++ b/arch/s390/kvm/kvm-s390.h > @@ -243,6 +243,8 @@ static inline u32 kvm_s390_get_gisa_desc(struct kvm *kvm) > /* implemented in pv.c */ > int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc); > int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc); > +int kvm_s390_pv_deinit_vm_async_prepare(struct kvm *kvm, u16 *rc, u16 *rrc); > +int kvm_s390_pv_deinit_vm_async(struct kvm *kvm, u16 *rc, u16 *rrc); > int kvm_s390_pv_deinit_vm(struct kvm *kvm, u16 *rc, u16 *rrc); > int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc); > int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length, u16 *rc, > diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c > index 8471c17d538c..ab06fa366e49 100644 > --- a/arch/s390/kvm/pv.c > +++ b/arch/s390/kvm/pv.c > @@ -279,6 +279,137 @@ int kvm_s390_pv_deinit_vm(struct kvm *kvm, u16 *rc, u16 *rrc) > return cc ? -EIO : 0; > } > > +/** > + * kvm_s390_destroy_lower_2g - Destroy the first 2GB of protected guest memory. > + * @kvm the VM whose memory is to be cleared. > + * Destroy the first 2GB of guest memory, to avoid prefix issues after reboot.
Please add a line stating that destroying is only possible if the configuration that owns the storage has no PV cpus registered with the UV.
> + */ > +static void kvm_s390_destroy_lower_2g(struct kvm *kvm) > +{ > + struct kvm_memory_slot *slot; > + unsigned long lim; > + int srcu_idx; > + > + srcu_idx = srcu_read_lock(&kvm->srcu); > + > + /* Take the memslot containing guest absolute address 0 */ > + slot = gfn_to_memslot(kvm, 0); > + /* Clear all slots that are completely below 2GB */ > + while (slot && slot->base_gfn + slot->npages < SZ_2G / PAGE_SIZE) { > + lim = slot->userspace_addr + slot->npages * PAGE_SIZE; > + s390_uv_destroy_range(kvm->mm, slot->userspace_addr, lim); > + /* Take the next memslot */ > + slot = gfn_to_memslot(kvm, slot->base_gfn + slot->npages); > + } > + /* Last slot crosses the 2G boundary, clear only up to 2GB */ > + if (slot && slot->base_gfn < SZ_2G / PAGE_SIZE) { > + lim = slot->userspace_addr + SZ_2G - slot->base_gfn * PAGE_SIZE; > + s390_uv_destroy_range(kvm->mm, slot->userspace_addr, lim); > + } > + > + srcu_read_unlock(&kvm->srcu, srcu_idx); > +} > + > +/** > + * kvm_s390_pv_deinit_vm_async_prepare - Prepare a protected VM for > + * asynchronous teardown. > + * @kvm the VM > + * @rc return value for the RC field of the UVCB > + * @rrc return value for the RRC field of the UVCB > + * > + * Prepare the protected VM for asynchronous teardown. The VM will be able > + * to continue immediately as a non-secure VM, and the information needed to > + * properly tear down the protected VM is set aside. If another protected VM > + * was already set aside without starting a teardown, the function will > + * fail. > + * > + * Context: kvm->lock needs to be held
You're asserting that via lockdep in the function below, why not here?
Also add something like: All PV cpus need to be destroyed by the caller before calling this function.
I'm considering that we add a cpu counter to the kvm arch pv struct which counts the currently created PV cpus so we can do a proper WARN_ONCE(count) here.
> + * > + * Return: 0 in case of success, -EINVAL if another protected VM was already set > + * aside, -ENOMEM if the system ran out of memory. > + */ > +int kvm_s390_pv_deinit_vm_async_prepare(struct kvm *kvm, u16 *rc, u16 *rrc) > +{ > + struct leftover_pv_vm *priv; > + > + /* > + * If an asynchronous deinitialization is already pending, refuse. > + * A synchronous deinitialization has to be performed instead. > + */ > + if (READ_ONCE(kvm->arch.pv.prepared_for_async_deinit)) > + return -EINVAL; > + priv = kmalloc(sizeof(*priv), GFP_KERNEL | __GFP_ZERO); > + if (!priv) > + return -ENOMEM; > + > + priv->stor_var = kvm->arch.pv.stor_var; > + priv->stor_base = kvm->arch.pv.stor_base; > + priv->handle = kvm_s390_pv_get_handle(kvm); > + priv->old_gmap_table = (unsigned long)kvm->arch.gmap->table; > + WRITE_ONCE(kvm->arch.gmap->guest_handle, 0); > + if (s390_replace_asce(kvm->arch.gmap)) { > + kfree(priv); > + return -ENOMEM; > + } > + > + kvm_s390_destroy_lower_2g(kvm); > + kvm_s390_clear_pv_state(kvm); > + WRITE_ONCE(kvm->arch.pv.prepared_for_async_deinit, priv); > + > + *rc = 1; > + *rrc = 42; > + return 0; > +} > +
| |