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?
>
I'll check it.
> Does enabling legacy on q35 without this patch fix things?
>
I'll check it also.
> And does disabling legacy on PIIX without this patch break thing?
>
I'll check it also.
>
> 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?
>
>
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);
> > > > > > }
> > > > > >
> > > > >
> > >
> > >
> >
> >
>
>
