On 1/26/23 09:41, Nico Boehr wrote:
Quoting Janosch Frank (2023-01-25 14:55:59)
On 1/20/23 08:54, 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.
Will userspace be able to handle the sudden -EINVAL rcs on
KVM_S390_GET_CMMA_BITS and KVM_S390_SET_CMMA_BITS?
QEMU has proper error handling on the GET_CMMA_BITS code path and will not
attempt GET_CMMA_BITS after it disabled dirty tracking. So yes, userspace can
handle this fine. In addition, as mentioned in the commit, it was never allowed
to have migration mode without dirty tracking. It was checked when migration
mode is enabled, just wasn't enforced when dirty tracking went off. The
alternative would be to refuse disabling dirty tracking when migration mode is
on; and that would _really_ break userspace.
Or we just leave migration mode on and check on every emulation/ioctl that a
dirty bitmap is still there, which would change absolutely nothing about the
return value of GET_CMMA_BITS.
Or we allocate the dirty bitmap for storage attributes independent of the dirty
bitmap for pages, which increases memory usage and makes this patch quite a bit
more complex, risking that we break more than what is already broken.
This approach really seems like the sane option to me.
Jup, I'm just trying to consider all possibilities to find the best one
and that includes asking all the questions.
+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.
Do we also need to add a warning to the CMMA IOCTLs?
No, it is already documented there:
This ioctl can fail with [...] > -EINVAL if KVM_S390_CMMA_PEEK is not set
but migration mode was not enabled
That's fine but doesn't include the part where migration mode can
suddenly be disabled via a memory region change.
If we implicitly change migration mode disablement then we need to
document that as much as possible to cover all bases.
[...]
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e4890e04b210..4785f002cd93 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5628,28 +5628,43 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
enum kvm_mr_change change)
{
gpa_t size;
+ int rc;
Not sure why you added rc even though it doesn't need to be used.
You prefer a line which is 100 chars wide over a new variable? OK fine for me.
I'm not sure how you manage to get over 100 chars, did I miss something?
if (!kvm->arch.migration_mode)
return 0;
if ((old->flags & KVM_MEM_LOG_DIRTY_PAGES) &&
!(new->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
WARN(kvm_s390_vm_stop_migration(kvm),
"Migration mode could not be disabled");
return 0;
