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

[Gavin Shan]

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