| Message ID | 20230127140532.230651-2-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: disable migration mode when dirty tracking is disabled | expand |
On Fri, 27 Jan 2023 15:05:32 +0100 Nico Boehr <nrb@linux.ibm.com> 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@vger.kernel.org > Fixes: 190df4a212a7 ("KVM: s390: CMMA tracking, ESSA emulation, migration mode") > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> Reviewed-by: Claudio Imbrenda <imbrenda@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). > + ====== ============================================================= > > 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; > }
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?
Quoting Janosch Frank (2023-01-30 10:53:23) [...] > > 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? Sure, please go ahead. >
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; }
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@vger.kernel.org Fixes: 190df4a212a7 ("KVM: s390: CMMA tracking, ESSA emulation, migration mode") Signed-off-by: Nico Boehr <nrb@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(-)