On Fri, 27 Jan 2023 15:05:32 +0100
Nico Boehr <nrb@xxxxxxxxxxxxx> 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
> 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@xxxxxxxxxxxxxxx
> Fixes: 190df4a212a7 ("KVM: s390: CMMA tracking, ESSA emulation, migration mode")
> Signed-off-by: Nico Boehr <nrb@xxxxxxxxxxxxx>
Reviewed-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> ---
> 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).
> + ====== =============================================================
>
> 4.108 KVM_S390_SET_CMMA_BITS
> ----------------------------
> diff --git a/Documentation/virt/kvm/devices/vm.rst b/Documentation/virt/kvm/devices/vm.rst
> index 60acc39e0e93..147efec626e5 100644
> --- a/Documentation/virt/kvm/devices/vm.rst
> +++ b/Documentation/virt/kvm/devices/vm.rst
> @@ -302,6 +302,10 @@ Allows userspace to start migration mode, needed for PGSTE migration.
> Setting this attribute when migration mode is already active will have
> no effects.
>
> +Dirty tracking must be enabled on all memslots, else -EINVAL is returned. When
> +dirty tracking is disabled on any memslot, migration mode is automatically
> +stopped.
> +
> :Parameters: none
> :Returns: -ENOMEM if there is not enough free memory to start migration mode;
> -EINVAL if the state of the VM is invalid (e.g. no memory defined);
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index e4890e04b210..cb72f9a09fb3 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -5633,23 +5633,40 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
> if (kvm_s390_pv_get_handle(kvm))
> return -EINVAL;
>
> - if (change == KVM_MR_DELETE || change == KVM_MR_FLAGS_ONLY)
> - return 0;
> + if (change != KVM_MR_DELETE && change != KVM_MR_FLAGS_ONLY) {
> + /*
> + * A few sanity checks. We can have memory slots which have to be
> + * located/ended at a segment boundary (1MB). The memory in userland is
> + * ok to be fragmented into various different vmas. It is okay to mmap()
> + * and munmap() stuff in this slot after doing this call at any time
> + */
>
> - /* A few sanity checks. We can have memory slots which have to be
> - located/ended at a segment boundary (1MB). The memory in userland is
> - ok to be fragmented into various different vmas. It is okay to mmap()
> - and munmap() stuff in this slot after doing this call at any time */
> + if (new->userspace_addr & 0xffffful)
> + return -EINVAL;
>
> - if (new->userspace_addr & 0xffffful)
> - return -EINVAL;
> + size = new->npages * PAGE_SIZE;
> + if (size & 0xffffful)
> + return -EINVAL;
>
> - size = new->npages * PAGE_SIZE;
> - if (size & 0xffffful)
> - return -EINVAL;
> + if ((new->base_gfn * PAGE_SIZE) + size > kvm->arch.mem_limit)
> + return -EINVAL;
> + }
>
> - if ((new->base_gfn * PAGE_SIZE) + size > kvm->arch.mem_limit)
> - return -EINVAL;
> + if (!kvm->arch.migration_mode)
> + return 0;
> +
> + /*
> + * Turn off migration mode when:
> + * - userspace creates a new memslot with dirty logging off,
> + * - userspace modifies an existing memslot (MOVE or FLAGS_ONLY) and
> + * dirty logging is turned off.
> + * Migration mode expects dirty page logging being enabled to store
> + * its dirty bitmap.
> + */
> + if (change != KVM_MR_DELETE &&
> + !(new->flags & KVM_MEM_LOG_DIRTY_PAGES))
> + WARN(kvm_s390_vm_stop_migration(kvm),
> + "Failed to stop migration mode");
>
> return 0;
> }
