Re: [PATCH v13 1/6] KVM: s390: pv: asynchronous destroy for reboot

[Nico Boehr]

Quoting Claudio Imbrenda (2022-08-10 14:56:20)
> 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_CLEANUP_PREPARE: prepares the current protected VM for
> asynchronous teardown. 

I would add:

A protected VM which has been prepared for asynchronous teardown is called a *set aside VM*. A set a side VM never has any CPUs associated with it, but is still registered with the Ultravisor and may have memory.

> 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. There can be at
> most one prepared VM at any time.
> 
> KVM_PV_ASYNC_CLEANUP_PERFORM: 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. 

I would add:

A set aside VM where cleanup started but was interrupted is called a *leftover VM*. There can be multiple leftovers per VM, which are tracked in the need_cleanup list.

I would like a more consistent language. We have three different names for the leftover in the patch below:
- leftover,
- the struct describing a leftover is called pv_vm_to_be_destroyed,
- the list of leftover vms is called need_cleanup.

It would be nice if this were a little more consistent, e.g. by renaming pv_vm_to_be_destroyed to pvm_vm_leftover and maybe need_cleanup to leftovers.

Otherwise, this looks good. I would be glad if you pick up my naming suggestions, but feel free to add:

Reviewed-by: Nico Boehr <nrb@xxxxxxxxxxxxx>