Re: [PATCH v1] KVM: s390: disable migration mode when dirty tracking is disabled

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

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;