[PATCH 0/2] Solve problem of hot removal of virtio-net-pci

[PATCH 0/2] Solve problem of hot removal of virtio-net-pci

[Yuri Benditovich]

There is regression of hotplug removal of virtio-net-pci on q35
introduced long time ago in 3.0.0 with commit:
 48564041a73adbbff52834f9edbe3806fceefab7
 exec: reintroduce MemoryRegion caching

virtio-net does not destroy some of caching resources upon
hot unplug and on further hot plug the device with the same
name can not be added.
These 2 patches solve the problem.

Yuri Benditovich (2):
  virtio: reset region cache when on queue deletion
  virtio-net: delete also control queue when TX/RX deleted

 hw/net/virtio-net.c | 3 ++-
 hw/virtio/virtio.c  | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/2] virtio: reset region cache when on queue deletion

[Yuri Benditovich]

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

[PATCH 2/2] virtio-net: delete also control queue when TX/RX deleted

[Yuri Benditovich]

https://bugzilla.redhat.com/show_bug.cgi?id=1708480
If the control queue is not deleted together with TX/RX, it
later will be ignored in freeing cache resources and hot
unplug will not be completed.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 hw/net/virtio-net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index db3d7c38e6..f325440d01 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3101,7 +3101,8 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp)
     for (i = 0; i < max_queues; i++) {
         virtio_net_del_queue(n, i);
     }
-
+    /* delete also control vq */
+    virtio_del_queue(vdev, max_queues * 2);
     qemu_announce_timer_del(&n->announce_timer, false);
     g_free(n->vqs);
     qemu_del_nic(n->nic);
-- 
2.17.1

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Jason Wang]

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);
>   }
>   

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Yuri Benditovich]

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);
> >   }
> >
>

Re: [PATCH 2/2] virtio-net: delete also control queue when TX/RX deleted

[Michael S. Tsirkin]

On Thu, Dec 26, 2019 at 06:36:49AM +0200, Yuri Benditovich wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1708480
> If the control queue is not deleted together with TX/RX, it
> later will be ignored in freeing cache resources and hot
> unplug will not be completed.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  hw/net/virtio-net.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index db3d7c38e6..f325440d01 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3101,7 +3101,8 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp)
>      for (i = 0; i < max_queues; i++) {
>          virtio_net_del_queue(n, i);
>      }
> -
> +    /* delete also control vq */
> +    virtio_del_queue(vdev, max_queues * 2);
>      qemu_announce_timer_del(&n->announce_timer, false);
>      g_free(n->vqs);
>      qemu_del_nic(n->nic);

Do we need to limit this to when ctrl vq exists?

> -- 
> 2.17.1

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Michael S. Tsirkin]

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);
> > >   }
> > >
> >

Re: [PATCH 2/2] virtio-net: delete also control queue when TX/RX deleted

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

On Thu, Jan 2, 2020 at 1:43 AM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Thu, Dec 26, 2019 at 06:36:49AM +0200, Yuri Benditovich wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1708480
> > If the control queue is not deleted together with TX/RX, it
> > later will be ignored in freeing cache resources and hot
> > unplug will not be completed.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > ---
> >  hw/net/virtio-net.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index db3d7c38e6..f325440d01 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -3101,7 +3101,8 @@ static void
> virtio_net_device_unrealize(DeviceState *dev, Error **errp)
> >      for (i = 0; i < max_queues; i++) {
> >          virtio_net_del_queue(n, i);
> >      }
> > -
> > +    /* delete also control vq */
> > +    virtio_del_queue(vdev, max_queues * 2);
> >      qemu_announce_timer_del(&n->announce_timer, false);
> >      g_free(n->vqs);
> >      qemu_del_nic(n->nic);
>
> Do we need to limit this to when ctrl vq exists?
>

ctrl vq always exists (we _add_ it unconditionally).
we may suggest respective feature or not, but the initialized queue
structure present.


>
> > --
> > 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 2175 bytes --]

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 2181 bytes --]

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);
> > > >   }
> > > >
> > >
>
>

[-- Attachment #2: Type: text/html, Size: 3443 bytes --]

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Michael S. Tsirkin]

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);
>     > > >   }
>     > > >
>     > >
> 
> 

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Michael S. Tsirkin]

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?

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 2382 bytes --]

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 3075 bytes --]

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);
> >     > > >   }
> >     > > >
> >     > >
> >
> >
>
>

[-- Attachment #2: Type: text/html, Size: 4762 bytes --]

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 3516 bytes --]

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);
>> >     > > >   }
>> >     > > >
>> >     > >
>> >
>> >
>>
>>

[-- Attachment #2: Type: text/html, Size: 5447 bytes --]

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Michael S. Tsirkin]

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);
>         >     > > >   }
>         >     > > >
>         >     > >
>         >
>         >
> 
> 

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 5410 bytes --]

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);
> >         >     > > >   }
> >         >     > > >
> >         >     > >
> >         >
> >         >
> >
> >
>
>

[-- Attachment #2: Type: text/html, Size: 8649 bytes --]

Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 6044 bytes --]

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);
>> >         >     > > >   }
>> >         >     > > >
>> >         >     > >
>> >         >
>> >         >
>> >
>> >
>>
>>

[-- Attachment #2: Type: text/html, Size: 10317 bytes --]