Re: [PATCH v1 3/6] kvm: Synchronize the backup bitmap in the last stage

[Gavin Shan <gshan@redhat.com>]

On 2/22/23 4:46 AM, Peter Xu wrote:
> On Mon, Feb 13, 2023 at 08:39:22AM +0800, Gavin Shan wrote:
>> In the last stage of live migration or memory slot removal, the
>> backup bitmap needs to be synchronized when it has been enabled.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   accel/kvm/kvm-all.c      | 11 +++++++++++
>>   include/sysemu/kvm_int.h |  1 +
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 01a6a026af..b5e12de522 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -1352,6 +1352,10 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
>>                    */
>>                   if (kvm_state->kvm_dirty_ring_size) {
>>                       kvm_dirty_ring_reap_locked(kvm_state, NULL);
>> +                    if (kvm_state->kvm_dirty_ring_with_bitmap) {
>> +                        kvm_slot_sync_dirty_pages(mem);
>> +                        kvm_slot_get_dirty_log(kvm_state, mem);
>> +                    }
>>                   } else {
>>                       kvm_slot_get_dirty_log(kvm_state, mem);
>>                   }
> 
> IIUC after the memory atomic update changes lands QEMU, we may not need
> this sync at all.
> 
> My understanding is that we sync dirty log here only because of non-atomic
> updates happening in the past and we may lose dirty bits unexpectedly.
> Maybe Paolo knows.
> 
> But that needs some more justification and history digging, so definitely
> more suitable to leave it for later and separate discussion.
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 

Peter, could you please give some hints for me to understand the atomic
and non-atomic update here? Ok, I will drop this part of changes in next
revision with the assumption that we have atomic update supported for
ARM64.

Thanks,
Gavin

> 
>> @@ -1573,6 +1577,12 @@ static void kvm_log_sync_global(MemoryListener *l, bool last_stage)
>>           mem = &kml->slots[i];
>>           if (mem->memory_size && mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
>>               kvm_slot_sync_dirty_pages(mem);
>> +
>> +            if (s->kvm_dirty_ring_with_bitmap && last_stage &&
>> +                kvm_slot_get_dirty_log(s, mem)) {
>> +                kvm_slot_sync_dirty_pages(mem);
>> +            }
>> +
>>               /*
>>                * This is not needed by KVM_GET_DIRTY_LOG because the
>>                * ioctl will unconditionally overwrite the whole region.
>> @@ -3701,6 +3711,7 @@ static void kvm_accel_instance_init(Object *obj)
>>       s->kernel_irqchip_split = ON_OFF_AUTO_AUTO;
>>       /* KVM dirty ring is by default off */
>>       s->kvm_dirty_ring_size = 0;
>> +    s->kvm_dirty_ring_with_bitmap = false;
>>       s->notify_vmexit = NOTIFY_VMEXIT_OPTION_RUN;
>>       s->notify_window = 0;
>>   }
>> diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
>> index 60b520a13e..fdd5b1bde0 100644
>> --- a/include/sysemu/kvm_int.h
>> +++ b/include/sysemu/kvm_int.h
>> @@ -115,6 +115,7 @@ struct KVMState
>>       } *as;
>>       uint64_t kvm_dirty_ring_bytes;  /* Size of the per-vcpu dirty ring */
>>       uint32_t kvm_dirty_ring_size;   /* Number of dirty GFNs per ring */
>> +    bool kvm_dirty_ring_with_bitmap;
>>       struct KVMDirtyRingReaper reaper;
>>       NotifyVmexitOption notify_vmexit;
>>       uint32_t notify_window;
>> -- 
>> 2.23.0
>>
>