https://bugzilla.redhat.com/show_bug.cgi?id=1708480
Fix leak of region reference that prevents complete
device deletion on hot unplug.
Signed-off-by: Yuri Benditovich <yuri.benditovich@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
+ */
+ virtio_virtqueue_reset_region_cache(&vdev->vq[n]);
g_free(vdev->vq[n].used_elems);
}
--
2.17.1
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? > > Signed-off-by: Yuri Benditovich <yuri.benditovich@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]); > g_free(vdev->vq[n].used_elems); > } >
On Thu, Dec 26, 2019 at 10:58 AM Jason Wang <jasowang@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.benditovich@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]); > > g_free(vdev->vq[n].used_elems); > > } > > >
On Thu, Dec 26, 2019 at 11:29:50AM +0200, Yuri Benditovich wrote: > On Thu, Dec 26, 2019 at 10:58 AM Jason Wang <jasowang@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.benditovich@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? > > > g_free(vdev->vq[n].used_elems); > > > } > > > > >
On Thu, Jan 2, 2020 at 1:50 AM Michael S. Tsirkin <mst@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 <jasowang@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.benditovich@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. > > > > g_free(vdev->vq[n].used_elems); > > > > } > > > > > > > > >
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 <mst@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 <jasowang@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.benditovich@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. > > > > > g_free(vdev->vq[n].used_elems); > > > > } > > > > > > > > >
On Sun, Jan 5, 2020 at 1:39 PM Michael S. Tsirkin <mst@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 <mst@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 <jasowang@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.benditovich@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); > > > > > } > > > > > > > > > > > > > > >
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.benditovich@daynix.com> wrote: > > > On Sun, Jan 5, 2020 at 1:39 PM Michael S. Tsirkin <mst@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 <mst@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 <jasowang@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.benditovich@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); >> > > > > } >> > > > > >> > > > >> > >> > >> >>
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?
Does enabling legacy on q35 without this patch fix things?
And does disabling legacy on PIIX without this patch break thing?
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?
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 <yuri.benditovich@daynix.com>
> wrote:
>
>
>
> On Sun, Jan 5, 2020 at 1:39 PM Michael S. Tsirkin <mst@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 <mst@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 <
> jasowang@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.benditovich@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);
> > > > > }
> > > > >
> > > >
> >
> >
>
>
On Mon, Jan 6, 2020 at 11:58 AM Michael S. Tsirkin <mst@redhat.com> 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 <
> yuri.benditovich@daynix.com>
> > wrote:
> >
> >
> >
> > On Sun, Jan 5, 2020 at 1:39 PM Michael S. Tsirkin <mst@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 <
> mst@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 <
> > jasowang@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.benditovich@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);
> > > > > > }
> > > > > >
> > > > >
> > >
> > >
> >
> >
>
>
On Mon, Jan 6, 2020 at 12:37 PM Yuri Benditovich <
yuri.benditovich@daynix.com> wrote:
>
> On Mon, Jan 6, 2020 at 11:58 AM Michael S. Tsirkin <mst@redhat.com> 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 <
>> yuri.benditovich@daynix.com>
>> > wrote:
>> >
>> >
>> >
>> > On Sun, Jan 5, 2020 at 1:39 PM Michael S. Tsirkin <mst@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 <
>> mst@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 <
>> > jasowang@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.benditovich@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);
>> > > > > > }
>> > > > > >
>> > > > >
>> > >
>> > >
>> >
>> >
>>
>>
On Thu, Dec 26, 2019 at 06:36:48AM +0200, 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.
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
I rebased this on top of my tree.
Got this:
commit f3dee6a062c1f4445768296ee39070bab9863372
Author: Yuri Benditovich <yuri.benditovich@daynix.com>
Date: Thu Dec 26 06:36:48 2019 +0200
virtio: reset region cache when on queue deletion
https://bugzilla.redhat.com/show_bug.cgi?id=1708480
Fix leak of region reference that prevents complete
device deletion on hot unplug.
Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
Message-Id: <20191226043649.14481-2-yuri.benditovich@daynix.com>
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 95d8ff8508..7b861e0ca0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2344,6 +2344,7 @@ void virtio_delete_queue(VirtQueue *vq)
vq->handle_aio_output = NULL;
g_free(vq->used_elems);
vq->used_elems = NULL;
+ virtio_virtqueue_reset_region_cache(vq);
}
void virtio_del_queue(VirtIODevice *vdev, int n)
Can you confirm pls?
On Sun, Jan 5, 2020 at 2:22 PM Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Dec 26, 2019 at 06:36:48AM +0200, 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. > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > > I rebased this on top of my tree. > > Got this: > > > commit f3dee6a062c1f4445768296ee39070bab9863372 > Author: Yuri Benditovich <yuri.benditovich@daynix.com> > Date: Thu Dec 26 06:36:48 2019 +0200 > > virtio: reset region cache when on queue deletion > > https://bugzilla.redhat.com/show_bug.cgi?id=1708480 > Fix leak of region reference that prevents complete > device deletion on hot unplug. > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > Message-Id: <20191226043649.14481-2-yuri.benditovich@daynix.com> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 95d8ff8508..7b861e0ca0 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2344,6 +2344,7 @@ void virtio_delete_queue(VirtQueue *vq) > vq->handle_aio_output = NULL; > g_free(vq->used_elems); > vq->used_elems = NULL; > + virtio_virtqueue_reset_region_cache(vq); > } > > void virtio_del_queue(VirtIODevice *vdev, int n) > > Can you confirm pls? > Yes, it is
© 2016 - 2026 Red Hat, Inc.