On Mon, Jan 6, 2020 at 12:37 PM Yuri Benditovich <
address@hidden> wrote:
On Mon, Jan 6, 2020 at 11:58 AM Michael S. Tsirkin <
address@hidden> 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 <address@hidden>
> wrote:
>
>
>
> On Sun, Jan 5, 2020 at 1:39 PM Michael S. Tsirkin <address@hidden> 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 <address@hidden>
> 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 <
> address@hidden> 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 <
> address@hidden>
> > > > > ---
> > > > > 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);
> > > > > }
> > > > >
> > > >
> >
> >
>
>