Re: [PATCH v4 1/1] KVM: s390: disable migration mode when dirty tracking is disabled

[Janosch Frank <frankja@linux.ibm.com>]

On 1/27/23 15:05, Nico Boehr wrote:
> Migration mode is a VM attribute which enables tracking of changes in
> storage attributes (PGSTE). It assumes dirty tracking is enabled on all
> memslots to keep a dirty bitmap of pages with changed storage attributes.
> 
> When enabling migration mode, we currently check that dirty tracking is
> enabled for all memslots. However, userspace can disable dirty tracking
> without disabling migration mode.
> 
> Since migration mode is pointless with dirty tracking disabled, disable
> migration mode whenever userspace disables dirty tracking on any slot.
> 
> Also update the documentation to clarify that dirty tracking must be
> enabled when enabling migration mode, which is already enforced by the
> code in kvm_s390_vm_start_migration().
> 
> Also highlight in the documentation for KVM_S390_GET_CMMA_BITS that it
> can now fail with -EINVAL when dirty tracking is disabled while
> migration mode is on. Move all the error codes to a table to this stays

s/to/so/

I'll fix that up when picking.

> readable.
> 
> To disable migration mode, slots_lock should be held, which is taken
> in kvm_set_memory_region() and thus held in
> kvm_arch_prepare_memory_region().
> 
> Restructure the prepare code a bit so all the sanity checking is done
> before disabling migration mode. This ensures migration mode isn't
> disabled when some sanity check fails.
> 
> Cc: stable@vger.kernel.org
> Fixes: 190df4a212a7 ("KVM: s390: CMMA tracking, ESSA emulation, migration mode")
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

Good find:
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> ---
>   Documentation/virt/kvm/api.rst        | 16 ++++++----
>   Documentation/virt/kvm/devices/vm.rst |  4 +++
>   arch/s390/kvm/kvm-s390.c              | 43 +++++++++++++++++++--------
>   3 files changed, 45 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 9807b05a1b57..2978acfcafc4 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -4537,11 +4537,17 @@ mask is unused.
>   
>   values points to the userspace buffer where the result will be stored.
>   
> -This ioctl can fail with -ENOMEM if not enough memory can be allocated to
> -complete the task, with -ENXIO if CMMA is not enabled, with -EINVAL if
> -KVM_S390_CMMA_PEEK is not set but migration mode was not enabled, with
> --EFAULT if the userspace address is invalid or if no page table is
> -present for the addresses (e.g. when using hugepages).
> +Errors:
> +
> +  ======     =============================================================
> +  ENOMEM     not enough memory can be allocated to complete the task
> +  ENXIO      if CMMA is not enabled
> +  EINVAL     if KVM_S390_CMMA_PEEK is not set but migration mode was not enabled
> +  EINVAL     if KVM_S390_CMMA_PEEK is not set but dirty tracking has been
> +             disabled (and thus migration mode was automatically disabled)
> +  EFAULT     if the userspace address is invalid or if no page table is
> +             present for the addresses (e.g. when using hugepages).
> +  ======     =============================================================

May I move this to the top?