Michael, Can you please comment on Jason's question: why we have a problem only with q35 and not with legacy pc? If you have a simple answer, it will help us in further work with other hot plug/unplug problems.
Thanks, Yuri Benditovich On Sun, Jan 5, 2020 at 6:21 PM Yuri Benditovich <yuri.benditov...@daynix.com> wrote: > > > On Sun, Jan 5, 2020 at 1:39 PM Michael S. Tsirkin <m...@redhat.com> wrote: > >> On Thu, Jan 02, 2020 at 09:09:04AM +0200, Yuri Benditovich wrote: >> > >> > >> > On Thu, Jan 2, 2020 at 1:50 AM Michael S. Tsirkin <m...@redhat.com> >> wrote: >> > >> > On Thu, Dec 26, 2019 at 11:29:50AM +0200, Yuri Benditovich wrote: >> > > On Thu, Dec 26, 2019 at 10:58 AM Jason Wang <jasow...@redhat.com> >> wrote: >> > > > >> > > > >> > > > On 2019/12/26 下午12:36, Yuri Benditovich wrote: >> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1708480 >> > > > > Fix leak of region reference that prevents complete >> > > > > device deletion on hot unplug. >> > > > >> > > > >> > > > More information is needed here, the bug said only q35 can meet >> this >> > > > issue. What makes q35 different here? >> > > > >> > > >> > > I do not have any ready answer, I did not dig into it too much. >> > > Probably Michael Tsirkin or Paolo Bonzini can answer without >> digging. >> > >> > >> > >> > > > >> > > > > >> > > > > Signed-off-by: Yuri Benditovich <yuri.benditov...@daynix.com> >> > > > > --- >> > > > > hw/virtio/virtio.c | 5 +++++ >> > > > > 1 file changed, 5 insertions(+) >> > > > > >> > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> > > > > index 04716b5f6c..baadec8abc 100644 >> > > > > --- a/hw/virtio/virtio.c >> > > > > +++ b/hw/virtio/virtio.c >> > > > > @@ -2340,6 +2340,11 @@ void virtio_del_queue(VirtIODevice >> *vdev, int >> > n) >> > > > > vdev->vq[n].vring.num_default = 0; >> > > > > vdev->vq[n].handle_output = NULL; >> > > > > vdev->vq[n].handle_aio_output = NULL; >> > > > > + /* >> > > > > + * with vring.num = 0 the queue will be ignored >> > > > > + * in later loops of region cache reset >> > > > > + */ >> > > > >> > > > >> > > > I can't get the meaning of this comment. >> > > > >> > > > Thanks >> > > > >> > > > >> > > > > + virtio_virtqueue_reset_region_cache(&vdev->vq[n]); >> > >> > >> > Do we need to drop this from virtio_device_free_virtqueues then? >> > >> > >> > >> > Not mandatory. Repetitive virtio_virtqueue_reset_region_cache does not >> do >> > anything bad. >> > Some of virtio devices do not do 'virtio_del_queue' at all. Currently >> > virtio_device_free_virtqueues resets region cache for them. >> > IMO, not calling 'virtio_del_queue' is a bug, but not in the scope of >> current >> > series, I'll take care of that later. >> >> Maybe we should just del all queues in virtio_device_unrealize? >> Will allow us to drop some logic tracking which vqs were created. >> >> > Yes, this is also possible with some rework of > virtio_device_free_virtqueues. > virtio-net has some additional operations around queue deletion, it > deletes queues when switches from single queue to multiple. > > >> >> > >> > > > > g_free(vdev->vq[n].used_elems); >> > > > > } >> > > > > >> > > > >> > >> > >> >>
