On Mon, Jan 6, 2020 at 12:37 PM Yuri Benditovich < [email protected]> wrote:
> > On Mon, Jan 6, 2020 at 11:58 AM Michael S. Tsirkin <[email protected]> wrote: > >> I guess it somehow has to do with the following: >> >> if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) { >> proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : >> ON_OFF_AUTO_OFF; >> } >> >> so by default device on an express port does not have a legacy interface. >> >> Somehow having a legacy interface fixes things? >> > > No, transitional virtio-net on q35 behaves exactly as the modern one. > > >> Does enabling legacy on q35 without this patch fix things? >> > > No, legacy virtio-net on q35 has the same problem. There is also an additional problem with legacy device unplug on q35, but I think it is not in the scope of this discussion. > > >> And does disabling legacy on PIIX without this patch break thing? >> > > No, modern device on PIIX does hot unplug without problems. > > >> >> How can having a legacy interface fix things if it's not >> even active? I don't know. Is there a chance windows drivers use the >> legacy interface on a transitional device by mistake? >> > As far as I can see - no. The driver works with the device depending on VERSION_1 > I'll recheck it. I do not think we use legacy interface on modern device > even if it has one. > > >> >> On Mon, Jan 06, 2020 at 11:10:18AM +0200, Yuri Benditovich wrote: >> > 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 < >> [email protected]> >> > wrote: >> > >> > >> > >> > On Sun, Jan 5, 2020 at 1:39 PM Michael S. Tsirkin <[email protected]> >> 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 < >> [email protected]> >> > 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 < >> > [email protected]> 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 < >> > [email protected]> >> > > > > > --- >> > > > > > 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); >> > > > > > } >> > > > > > >> > > > > >> > > >> > > >> > >> > >> >>
