Re: [Qemu-devel] [RFC 0/2] memory/vfio: notify region_del() when unregister listeners

[Peter Xu <peterx@redhat.com>]

On Fri, Jan 19, 2018 at 12:41:01PM +0100, Paolo Bonzini wrote:
> On 19/01/2018 09:42, Peter Xu wrote:
> > I encountered an event loss problem during unplugging vfio devices:
> > 
> >   https://bugzilla.redhat.com/show_bug.cgi?id=1531393
> > 
> > I thought it should be a simple VT-d issue but I was wrong.  The whole
> > debugging leads me to these patches.
> > 
> > Basically I think what we missed is that when unregistering memory
> > listeners, we don't really call region_del() at all.  Instead we just
> > remove ourselves from the listener list.  IMHO that's not enough.  A
> > clean unregister should undo all possible changes that have done
> > during region_add().  That's patch 1.
> > 
> > Patch 2 fixes a vfio issue when patch 1 is applied.
> 
> It makes sense, though of course patch 1 must come second for
> bisectability.  Of the other listeners, most do not implement
> region_del, but commit must be audited as well.  What matters is whether
> the listener is unregistered, and only few are:
> 
> - kvm_arm_machine_init_done must unregister the listener after the
> QSLIST_FOREACH_SAFE loop.

I'll add another patch for this.  I noticed that we haven't really do
the list cleanup on kvm_devices_head.  Say, the items are still on the
list even after they are freed in kvm_arm_machine_init_done():

    QSLIST_FOREACH_SAFE(kd, &kvm_devices_head, entries, tkd) {
        if (kd->kda.addr != -1) {
            kvm_arm_set_device_addr(kd);
        }
        memory_region_unref(kd->mr);
        g_free(kd);
    }

I think it's pretty safe as long as we don't access kvm_devices_head
any more (that's what we did), but would it be better to clean the
list head too?  (CC PeterM too)

Anyway, I'll ignore this for my series and only do what's needed for
the memory listener fix.

> 
> - Xen seems okay
> 
> - vhost needs to remove the memory_region_unref loop after unregistering
> the listener - and this must be done at the same time as patch 1, not before

Noted.  I'll also fix the begin() missing problem you pointed out in
the other comment.  Thanks!

> 
> - virtio seems okay
> 
> Thanks,
> 
> Paolo

-- 
Peter Xu