[PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[pannengyuan]

From: Pan Nengyuan <pannengyuan@huawei.com>

Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Changes v2 to v1:
- add a new function virtio_delete_queue to cleanup vq through a vq pointer
---
 hw/virtio/virtio.c         | 16 +++++++++++-----
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 04716b5..6de3cfd 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
     return &vdev->vq[i];
 }
 
+void virtio_delete_queue(VirtQueue *vq)
+{
+    vq->vring.num = 0;
+    vq->vring.num_default = 0;
+    vq->handle_output = NULL;
+    vq->handle_aio_output = NULL;
+    g_free(vq->used_elems);
+    vq->used_elems = NULL;
+}
+
 void virtio_del_queue(VirtIODevice *vdev, int n)
 {
     if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
         abort();
     }
 
-    vdev->vq[n].vring.num = 0;
-    vdev->vq[n].vring.num_default = 0;
-    vdev->vq[n].handle_output = NULL;
-    vdev->vq[n].handle_aio_output = NULL;
-    g_free(vdev->vq[n].used_elems);
+    virtio_delete_queue(&vdev->vq[n]);
 }
 
 static void virtio_set_isr(VirtIODevice *vdev, int value)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index c32a815..e18756d 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
 
 void virtio_del_queue(VirtIODevice *vdev, int n);
 
+void virtio_delete_queue(VirtQueue *vq);
+
 void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len);
 void virtqueue_flush(VirtQueue *vq, unsigned int count);
-- 
2.7.2.windows.1

[PATCH v2 2/3] virtio-balloon: fix memory leak while attach virtio-balloon device

[pannengyuan]

From: Pan Nengyuan <pannengyuan@huawei.com>

ivq/dvq/svq/free_page_vq is forgot to cleanup in
virtio_balloon_device_unrealize, the memory leak stack is as follow:

Direct leak of 14336 byte(s) in 2 object(s) allocated from:
    #0 0x7f99fd9d8560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
    #1 0x7f99fcb20015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
    #2 0x557d90638437 in virtio_add_queue hw/virtio/virtio.c:2327
    #3 0x557d9064401d in virtio_balloon_device_realize hw/virtio/virtio-balloon.c:793
    #4 0x557d906356f7 in virtio_device_realize hw/virtio/virtio.c:3504
    #5 0x557d9073f081 in device_set_realized hw/core/qdev.c:876
    #6 0x557d908b1f4d in property_set_bool qom/object.c:2080
    #7 0x557d908b655e in object_property_set_qobject qom/qom-qobject.c:26

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Change v2 to v1:
- use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
  Michael S. Tsirkin)
---
 hw/virtio/virtio-balloon.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 40b04f5..57f3b9f 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -831,6 +831,13 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
     }
     balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
+
+    virtio_delete_queue(s->ivq);
+    virtio_delete_queue(s->dvq);
+    virtio_delete_queue(s->svq);
+    if (s->free_page_vq) {
+        virtio_delete_queue(s->free_page_vq);
+    }
     virtio_cleanup(vdev);
 }
 
-- 
2.7.2.windows.1

Re: [PATCH v2 2/3] virtio-balloon: fix memory leak while attach virtio-balloon device

[Laurent Vivier]

On 04/12/2019 08:31, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> ivq/dvq/svq/free_page_vq is forgot to cleanup in
> virtio_balloon_device_unrealize, the memory leak stack is as follow:
> 
> Direct leak of 14336 byte(s) in 2 object(s) allocated from:
>     #0 0x7f99fd9d8560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
>     #1 0x7f99fcb20015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
>     #2 0x557d90638437 in virtio_add_queue hw/virtio/virtio.c:2327
>     #3 0x557d9064401d in virtio_balloon_device_realize hw/virtio/virtio-balloon.c:793
>     #4 0x557d906356f7 in virtio_device_realize hw/virtio/virtio.c:3504
>     #5 0x557d9073f081 in device_set_realized hw/core/qdev.c:876
>     #6 0x557d908b1f4d in property_set_bool qom/object.c:2080
>     #7 0x557d908b655e in object_property_set_qobject qom/qom-qobject.c:26
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Change v2 to v1:
> - use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
>   Michael S. Tsirkin)
> ---
>  hw/virtio/virtio-balloon.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 40b04f5..57f3b9f 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -831,6 +831,13 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
>      }
>      balloon_stats_destroy_timer(s);
>      qemu_remove_balloon_handler(s);
> +
> +    virtio_delete_queue(s->ivq);
> +    virtio_delete_queue(s->dvq);
> +    virtio_delete_queue(s->svq);
> +    if (s->free_page_vq) {
> +        virtio_delete_queue(s->free_page_vq);
> +    }
>      virtio_cleanup(vdev);
>  }
>  
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

[PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[pannengyuan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 2195 bytes --]

From: Pan Nengyuan <pannengyuan@huawei.com>

ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
virtio_serial_device_unrealize, the memory leak stack is as bellow:

Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
    #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
    #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
    #2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
    #3 0x5650e02847b5 in virtio_serial_device_realize hw/char/virtio-serial-bus.c:1089
    #4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
    #5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
    #6 0x5650e0531efd in property_set_bool qom/object.c:2080
    #7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
    #8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
    #9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
Cc: Laurent Vivier <lvivier@redhat.com>
Cc: Amit Shah <amit@kernel.org>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
Changes v2 to v1:
- use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
  Michael S. Tsirkin)
---
 hw/char/virtio-serial-bus.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 3325904..e1cbce3 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -1126,9 +1126,17 @@ static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOSerial *vser = VIRTIO_SERIAL(dev);
+    int i;
 
     QLIST_REMOVE(vser, next);
 
+    virtio_delete_queue(vser->c_ivq);
+    virtio_delete_queue(vser->c_ovq);
+    for (i = 0; i < vser->bus.max_nr_ports; i++) {
+        virtio_delete_queue(vser->ivqs[i]);
+        virtio_delete_queue(vser->ovqs[i]);
+    }
+
     g_free(vser->ivqs);
     g_free(vser->ovqs);
     g_free(vser->ports_map);
-- 
2.7.2.windows.1

Re: [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[Laurent Vivier]

On 04/12/2019 08:31, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
> virtio_serial_device_unrealize, the memory leak stack is as bellow:
> 
> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
>     #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
>     #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
>     #2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
>     #3 0x5650e02847b5 in virtio_serial_device_realize hw/char/virtio-serial-bus.c:1089
>     #4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
>     #5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
>     #6 0x5650e0531efd in property_set_bool qom/object.c:2080
>     #7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
>     #8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
>     #9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> Cc: Laurent Vivier <lvivier@redhat.com>
> Cc: Amit Shah <amit@kernel.org>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
> Changes v2 to v1:
> - use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
>   Michael S. Tsirkin)
> ---
>  hw/char/virtio-serial-bus.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 3325904..e1cbce3 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1126,9 +1126,17 @@ static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOSerial *vser = VIRTIO_SERIAL(dev);
> +    int i;
>  
>      QLIST_REMOVE(vser, next);
>  
> +    virtio_delete_queue(vser->c_ivq);
> +    virtio_delete_queue(vser->c_ovq);
> +    for (i = 0; i < vser->bus.max_nr_ports; i++) {
> +        virtio_delete_queue(vser->ivqs[i]);
> +        virtio_delete_queue(vser->ovqs[i]);
> +    }
> +
>      g_free(vser->ivqs);
>      g_free(vser->ovqs);
>      g_free(vser->ports_map);
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[Eric Blake]

On 12/4/19 1:31 AM, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in

s/is //

> virtio_serial_device_unrealize, the memory leak stack is as bellow:

below

> 
> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
>      #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
>      #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
>      #2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
>      #3 0x5650e02847b5 in virtio_serial_device_realize hw/char/virtio-serial-bus.c:1089
>      #4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
>      #5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
>      #6 0x5650e0531efd in property_set_bool qom/object.c:2080
>      #7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
>      #8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
>      #9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> Cc: Laurent Vivier <lvivier@redhat.com>
> Cc: Amit Shah <amit@kernel.org>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[Michael S. Tsirkin]

Headers are wrong for this one: charset="y" confuses git am.


On Wed, Dec 04, 2019 at 03:31:56PM +0800, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
> virtio_serial_device_unrealize, the memory leak stack is as bellow:
> 
> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
>     #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
>     #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
>     #2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
>     #3 0x5650e02847b5 in virtio_serial_device_realize hw/char/virtio-serial-bus.c:1089
>     #4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
>     #5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
>     #6 0x5650e0531efd in property_set_bool qom/object.c:2080
>     #7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
>     #8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
>     #9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> Cc: Laurent Vivier <lvivier@redhat.com>
> Cc: Amit Shah <amit@kernel.org>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
> Changes v2 to v1:
> - use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
>   Michael S. Tsirkin)
> ---
>  hw/char/virtio-serial-bus.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 3325904..e1cbce3 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1126,9 +1126,17 @@ static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOSerial *vser = VIRTIO_SERIAL(dev);
> +    int i;
>  
>      QLIST_REMOVE(vser, next);
>  
> +    virtio_delete_queue(vser->c_ivq);
> +    virtio_delete_queue(vser->c_ovq);
> +    for (i = 0; i < vser->bus.max_nr_ports; i++) {
> +        virtio_delete_queue(vser->ivqs[i]);
> +        virtio_delete_queue(vser->ovqs[i]);
> +    }
> +
>      g_free(vser->ivqs);
>      g_free(vser->ovqs);
>      g_free(vser->ports_map);
> -- 
> 2.7.2.windows.1
> 

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Pankaj Gupta]

> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Devices tend to maintain vq pointers, allow deleting them trough a vq
> pointer.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> ---
>  hw/virtio/virtio.c         | 16 +++++++++++-----
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> queue_size,
>      return &vdev->vq[i];
>  }
>  
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> +    vq->vring.num = 0;
> +    vq->vring.num_default = 0;
> +    vq->handle_output = NULL;
> +    vq->handle_aio_output = NULL;
> +    g_free(vq->used_elems);
> +    vq->used_elems = NULL;
> +}
> +
>  void virtio_del_queue(VirtIODevice *vdev, int n)
>  {
>      if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>          abort();
>      }
>  
> -    vdev->vq[n].vring.num = 0;
> -    vdev->vq[n].vring.num_default = 0;
> -    vdev->vq[n].handle_output = NULL;
> -    vdev->vq[n].handle_aio_output = NULL;
> -    g_free(vdev->vq[n].used_elems);
> +    virtio_delete_queue(&vdev->vq[n]);
>  }
>  
>  static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> queue_size,
>  
>  void virtio_del_queue(VirtIODevice *vdev, int n);
>  
> +void virtio_delete_queue(VirtQueue *vq);
> +
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>                      unsigned int len);
>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> --
> 2.7.2.windows.1
> 
> 
Overall it ooks good to me.

Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function
We are doing :     virtio_del_queue(vdev, 0);

One can directly call "virtio_delete_queue". It can become confusing
to call multiple functions for same purpose. Instead, Can we make 
"virtio_delete_queue" static inline?

Other than that:
Reviewed-by: Pankaj Gupta <pagupta@redhat.com>

> 
> 

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Pan Nengyuan]

On 2019/12/4 16:33, Pankaj Gupta wrote:
> 
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>
>> Devices tend to maintain vq pointers, allow deleting them trough a vq
>> pointer.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Changes v2 to v1:
>> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
>> ---
>>  hw/virtio/virtio.c         | 16 +++++++++++-----
>>  include/hw/virtio/virtio.h |  2 ++
>>  2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 04716b5..6de3cfd 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
>> queue_size,
>>      return &vdev->vq[i];
>>  }
>>  
>> +void virtio_delete_queue(VirtQueue *vq)
>> +{
>> +    vq->vring.num = 0;
>> +    vq->vring.num_default = 0;
>> +    vq->handle_output = NULL;
>> +    vq->handle_aio_output = NULL;
>> +    g_free(vq->used_elems);
>> +    vq->used_elems = NULL;
>> +}
>> +
>>  void virtio_del_queue(VirtIODevice *vdev, int n)
>>  {
>>      if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>>          abort();
>>      }
>>  
>> -    vdev->vq[n].vring.num = 0;
>> -    vdev->vq[n].vring.num_default = 0;
>> -    vdev->vq[n].handle_output = NULL;
>> -    vdev->vq[n].handle_aio_output = NULL;
>> -    g_free(vdev->vq[n].used_elems);
>> +    virtio_delete_queue(&vdev->vq[n]);
>>  }
>>  
>>  static void virtio_set_isr(VirtIODevice *vdev, int value)
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index c32a815..e18756d 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
>> queue_size,
>>  
>>  void virtio_del_queue(VirtIODevice *vdev, int n);
>>  
>> +void virtio_delete_queue(VirtQueue *vq);
>> +
>>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>>                      unsigned int len);
>>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
>> --
>> 2.7.2.windows.1
>>
>>
> Overall it ooks good to me.
> 
> Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function
> We are doing :     virtio_del_queue(vdev, 0);
> 
> One can directly call "virtio_delete_queue". It can become confusing
> to call multiple functions for same purpose. Instead, Can we make 
> "virtio_delete_queue" static inline?
> 
yes, It will be a little confused, but I think it will have the same
problem if we make "virtio_delete_queue" static inline. We can directly
call it aslo. （e.g virtio-serial-bus.c virtio-balloon.c).

How about replacing the function name to make it more clear (e.g
virtio_delete_queue -> virtio_queue_cleanup) ? It's too similar between
"virtio_del_queue" and "virtio_delete_queue".

> Other than that:
> Reviewed-by: Pankaj Gupta <pagupta@redhat.com>
> 
>>
>>
> 
> 
> .
> 

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Pankaj Gupta]

> 
> On 2019/12/4 16:33, Pankaj Gupta wrote:
> > 
> >> From: Pan Nengyuan <pannengyuan@huawei.com>
> >>
> >> Devices tend to maintain vq pointers, allow deleting them trough a vq
> >> pointer.
> >>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> >> ---
> >> Changes v2 to v1:
> >> - add a new function virtio_delete_queue to cleanup vq through a vq
> >> pointer
> >> ---
> >>  hw/virtio/virtio.c         | 16 +++++++++++-----
> >>  include/hw/virtio/virtio.h |  2 ++
> >>  2 files changed, 13 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 04716b5..6de3cfd 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev,
> >> int
> >> queue_size,
> >>      return &vdev->vq[i];
> >>  }
> >>  
> >> +void virtio_delete_queue(VirtQueue *vq)
> >> +{
> >> +    vq->vring.num = 0;
> >> +    vq->vring.num_default = 0;
> >> +    vq->handle_output = NULL;
> >> +    vq->handle_aio_output = NULL;
> >> +    g_free(vq->used_elems);
> >> +    vq->used_elems = NULL;
> >> +}
> >> +
> >>  void virtio_del_queue(VirtIODevice *vdev, int n)
> >>  {
> >>      if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
> >>          abort();
> >>      }
> >>  
> >> -    vdev->vq[n].vring.num = 0;
> >> -    vdev->vq[n].vring.num_default = 0;
> >> -    vdev->vq[n].handle_output = NULL;
> >> -    vdev->vq[n].handle_aio_output = NULL;
> >> -    g_free(vdev->vq[n].used_elems);
> >> +    virtio_delete_queue(&vdev->vq[n]);
> >>  }
> >>  
> >>  static void virtio_set_isr(VirtIODevice *vdev, int value)
> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >> index c32a815..e18756d 100644
> >> --- a/include/hw/virtio/virtio.h
> >> +++ b/include/hw/virtio/virtio.h
> >> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> >> queue_size,
> >>  
> >>  void virtio_del_queue(VirtIODevice *vdev, int n);
> >>  
> >> +void virtio_delete_queue(VirtQueue *vq);
> >> +
> >>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> >>                      unsigned int len);
> >>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> >> --
> >> 2.7.2.windows.1
> >>
> >>
> > Overall it ooks good to me.
> > 
> > Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function
> > We are doing :     virtio_del_queue(vdev, 0);
> > 
> > One can directly call "virtio_delete_queue". It can become confusing
> > to call multiple functions for same purpose. Instead, Can we make
> > "virtio_delete_queue" static inline?
> > 
> yes, It will be a little confused, but I think it will have the same
> problem if we make "virtio_delete_queue" static inline. We can directly
> call it aslo. （e.g virtio-serial-bus.c virtio-balloon.c).
> 
> How about replacing the function name to make it more clear (e.g
> virtio_delete_queue -> virtio_queue_cleanup) ? It's too similar between
> "virtio_del_queue" and "virtio_delete_queue".

I am just thinking if we need these two separate functions.

Yes, changing name of virtio_delete_queue -> virtio_queue_cleanup
should be good enough.

Thanks,
Pankaj

> 
> > Other than that:
> > Reviewed-by: Pankaj Gupta <pagupta@redhat.com>
> > 
> >>
> >>
> > 
> > 
> > .
> > 
> 
> 
> 

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Michael S. Tsirkin]

On Wed, Dec 04, 2019 at 03:33:07AM -0500, Pankaj Gupta wrote:
> 
> > From: Pan Nengyuan <pannengyuan@huawei.com>
> > 
> > Devices tend to maintain vq pointers, allow deleting them trough a vq
> > pointer.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> > ---
> > Changes v2 to v1:
> > - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> > ---
> >  hw/virtio/virtio.c         | 16 +++++++++++-----
> >  include/hw/virtio/virtio.h |  2 ++
> >  2 files changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 04716b5..6de3cfd 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> > queue_size,
> >      return &vdev->vq[i];
> >  }
> >  
> > +void virtio_delete_queue(VirtQueue *vq)
> > +{
> > +    vq->vring.num = 0;
> > +    vq->vring.num_default = 0;
> > +    vq->handle_output = NULL;
> > +    vq->handle_aio_output = NULL;
> > +    g_free(vq->used_elems);
> > +    vq->used_elems = NULL;
> > +}
> > +
> >  void virtio_del_queue(VirtIODevice *vdev, int n)
> >  {
> >      if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
> >          abort();
> >      }
> >  
> > -    vdev->vq[n].vring.num = 0;
> > -    vdev->vq[n].vring.num_default = 0;
> > -    vdev->vq[n].handle_output = NULL;
> > -    vdev->vq[n].handle_aio_output = NULL;
> > -    g_free(vdev->vq[n].used_elems);
> > +    virtio_delete_queue(&vdev->vq[n]);
> >  }
> >  
> >  static void virtio_set_isr(VirtIODevice *vdev, int value)
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index c32a815..e18756d 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> > queue_size,
> >  
> >  void virtio_del_queue(VirtIODevice *vdev, int n);
> >  
> > +void virtio_delete_queue(VirtQueue *vq);
> > +
> >  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> >                      unsigned int len);
> >  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> > --
> > 2.7.2.windows.1
> > 
> > 
> Overall it ooks good to me.
> 
> Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function
> We are doing :     virtio_del_queue(vdev, 0);

Yea. Let's just bite the bullet and convert all callers.
Not so many of them.

> One can directly call "virtio_delete_queue". It can become confusing
> to call multiple functions for same purpose. Instead, Can we make 
> "virtio_delete_queue" static inline?

We can't really.


> Other than that:
> Reviewed-by: Pankaj Gupta <pagupta@redhat.com>
> 
> > 
> > 

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Laurent Vivier]

On 04/12/2019 08:31, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> ---
>  hw/virtio/virtio.c         | 16 +++++++++++-----
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>      return &vdev->vq[i];
>  }
>  
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> +    vq->vring.num = 0;
> +    vq->vring.num_default = 0;
> +    vq->handle_output = NULL;
> +    vq->handle_aio_output = NULL;
> +    g_free(vq->used_elems);
> +    vq->used_elems = NULL;
> +}
> +
>  void virtio_del_queue(VirtIODevice *vdev, int n)
>  {
>      if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>          abort();
>      }
>  
> -    vdev->vq[n].vring.num = 0;
> -    vdev->vq[n].vring.num_default = 0;
> -    vdev->vq[n].handle_output = NULL;
> -    vdev->vq[n].handle_aio_output = NULL;
> -    g_free(vdev->vq[n].used_elems);
> +    virtio_delete_queue(&vdev->vq[n]);
>  }
>  
>  static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>  
>  void virtio_del_queue(VirtIODevice *vdev, int n);
>  
> +void virtio_delete_queue(VirtQueue *vq);
> +
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>                      unsigned int len);
>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Eric Blake]

On 12/4/19 1:31 AM, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.

through

> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---

Also, don't forget to send a 0/3 cover letter (any series longer than 
one patch should have a cover letter; it is possible to configure git to 
do this automatically: https://wiki.qemu.org/Contribute/SubmitAPatch has 
this tip and others)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Pan Nengyuan]

On 2019/12/4 22:40, Eric Blake wrote:
> On 12/4/19 1:31 AM, pannengyuan@huawei.com wrote:
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>
>> Devices tend to maintain vq pointers, allow deleting them trough a vq
>> pointer.
> 
> through

Thanks. I'm sorry for my carelessness.

> 
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
> 
> Also, don't forget to send a 0/3 cover letter (any series longer than
> one patch should have a cover letter; it is possible to configure git to
> do this automatically: https://wiki.qemu.org/Contribute/SubmitAPatch has
> this tip and others)

ok, thanks.

> 

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Amit Shah]

On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>

Shouldn't this be From: mst?

I didn't find a ref to the original patch to confirm if you had to
adapt it in any way, though.

> Devices tend to maintain vq pointers, allow deleting them trough a vq
> pointer.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq
> pointer
> ---
>  hw/virtio/virtio.c         | 16 +++++++++++-----
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice
> *vdev, int queue_size,
>      return &vdev->vq[i];
>  }
>  
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> +    vq->vring.num = 0;
> +    vq->vring.num_default = 0;
> +    vq->handle_output = NULL;
> +    vq->handle_aio_output = NULL;
> +    g_free(vq->used_elems);
> +    vq->used_elems = NULL;
> +}
> +
>  void virtio_del_queue(VirtIODevice *vdev, int n)
>  {
>      if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>          abort();
>      }
>  
> -    vdev->vq[n].vring.num = 0;
> -    vdev->vq[n].vring.num_default = 0;
> -    vdev->vq[n].handle_output = NULL;
> -    vdev->vq[n].handle_aio_output = NULL;
> -    g_free(vdev->vq[n].used_elems);
> +    virtio_delete_queue(&vdev->vq[n]);
>  }
>  
>  static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev,
> int queue_size,
>  
>  void virtio_del_queue(VirtIODevice *vdev, int n);
>  
> +void virtio_delete_queue(VirtQueue *vq);
> +
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>                      unsigned int len);
>  void virtqueue_flush(VirtQueue *vq, unsigned int count);

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Pan Nengyuan]

On 2019/12/6 0:45, Amit Shah wrote:
> On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote:
>> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Shouldn't this be From: mst?
> 
> I didn't find a ref to the original patch to confirm if you had to
> adapt it in any way, though.
> 

Here is the original
patch:　https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00402.html

I just change one line(set used_elems to NULL). In next version, I will
change function name from virtio_delete_queue to virtio_queue_cleanup
(It's too similar between "virtio_del_queue" and "virtio_delete_queue"):
https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00877.html.

According to these, should I change it in next version?

Thanks.

>> Devices tend to maintain vq pointers, allow deleting them trough a vq
>> pointer.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Changes v2 to v1:
>> - add a new function virtio_delete_queue to cleanup vq through a vq
>> pointer
>> ---
>>  hw/virtio/virtio.c         | 16 +++++++++++-----
>>  include/hw/virtio/virtio.h |  2 ++
>>  2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 04716b5..6de3cfd 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice
>> *vdev, int queue_size,
>>      return &vdev->vq[i];
>>  }
>>  
>> +void virtio_delete_queue(VirtQueue *vq)
>> +{
>> +    vq->vring.num = 0;
>> +    vq->vring.num_default = 0;
>> +    vq->handle_output = NULL;
>> +    vq->handle_aio_output = NULL;
>> +    g_free(vq->used_elems);
>> +    vq->used_elems = NULL;
>> +}
>> +
>>  void virtio_del_queue(VirtIODevice *vdev, int n)
>>  {
>>      if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>>          abort();
>>      }
>>  
>> -    vdev->vq[n].vring.num = 0;
>> -    vdev->vq[n].vring.num_default = 0;
>> -    vdev->vq[n].handle_output = NULL;
>> -    vdev->vq[n].handle_aio_output = NULL;
>> -    g_free(vdev->vq[n].used_elems);
>> +    virtio_delete_queue(&vdev->vq[n]);
>>  }
>>  
>>  static void virtio_set_isr(VirtIODevice *vdev, int value)
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index c32a815..e18756d 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev,
>> int queue_size,
>>  
>>  void virtio_del_queue(VirtIODevice *vdev, int n);
>>  
>> +void virtio_delete_queue(VirtQueue *vq);
>> +
>>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>>                      unsigned int len);
>>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> 
> 
> .
> 

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Amit Shah]

On Fri, 2019-12-06 at 10:17 +0800, Pan Nengyuan wrote:
> On 2019/12/6 0:45, Amit Shah wrote:
> > On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote:
> > > From: Pan Nengyuan <pannengyuan@huawei.com>
> > 
> > Shouldn't this be From: mst?
> > 
> > I didn't find a ref to the original patch to confirm if you had to
> > adapt it in any way, though.
> > 
> 
> Here is the original
> patch:　
> https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00402.html
> 
> I just change one line(set used_elems to NULL). In next version, I
> will
> change function name from virtio_delete_queue to virtio_queue_cleanup
> (It's too similar between "virtio_del_queue" and
> "virtio_delete_queue"):
> 
https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00877.html
> .
> 
> According to these, should I change it in next version?

Sure, please change it.  Please ensure 'From:' is Michael, but in the
sign-off area, you can mention how you changed the original patch, e.g.
see the "[PMM: ...]" in



https://lore.kernel.org/qemu-devel/20191126141239.8219-5-peter.maydell@linaro.org/

Also, please CC me on the entire series.

Thanks,

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Pan Nengyuan]

On 2019/12/6 16:56, Amit Shah wrote:
> On Fri, 2019-12-06 at 10:17 +0800, Pan Nengyuan wrote:
>> On 2019/12/6 0:45, Amit Shah wrote:
>>> On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote:
>>>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>>
>>> Shouldn't this be From: mst?
>>>
>>> I didn't find a ref to the original patch to confirm if you had to
>>> adapt it in any way, though.
>>>
>>
>> Here is the original
>> patch:　
>> https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00402.html
>>
>> I just change one line(set used_elems to NULL). In next version, I
>> will
>> change function name from virtio_delete_queue to virtio_queue_cleanup
>> (It's too similar between "virtio_del_queue" and
>> "virtio_delete_queue"):
>>
> https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00877.html
>> .
>>
>> According to these, should I change it in next version?
> 
> Sure, please change it.  Please ensure 'From:' is Michael, but in the
> sign-off area, you can mention how you changed the original patch, e.g.
> see the "[PMM: ...]" in
> 
> 
> 
> https://lore.kernel.org/qemu-devel/20191126141239.8219-5-peter.maydell@linaro.org/
> 
> Also, please CC me on the entire series.
> 
> Thanks,
> 
> 
OK, I will change it.

Thanks.

> 
> .
> 

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Michael S. Tsirkin]

On Wed, Dec 04, 2019 at 03:31:54PM +0800, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.

You want to also mention something about clearing
.used_elems to avoid chances of double free.

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>


So let's just name the new one virtio_del_queue then,
and drop the old one.


> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> ---
>  hw/virtio/virtio.c         | 16 +++++++++++-----
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>      return &vdev->vq[i];
>  }
>  
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> +    vq->vring.num = 0;
> +    vq->vring.num_default = 0;
> +    vq->handle_output = NULL;
> +    vq->handle_aio_output = NULL;
> +    g_free(vq->used_elems);
> +    vq->used_elems = NULL;
> +}
> +
>  void virtio_del_queue(VirtIODevice *vdev, int n)
>  {
>      if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>          abort();
>      }
>  
> -    vdev->vq[n].vring.num = 0;
> -    vdev->vq[n].vring.num_default = 0;
> -    vdev->vq[n].handle_output = NULL;
> -    vdev->vq[n].handle_aio_output = NULL;
> -    g_free(vdev->vq[n].used_elems);
> +    virtio_delete_queue(&vdev->vq[n]);
>  }
>  
>  static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>  
>  void virtio_del_queue(VirtIODevice *vdev, int n);
>  
> +void virtio_delete_queue(VirtQueue *vq);
> +
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>                      unsigned int len);
>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> -- 
> 2.7.2.windows.1
> 

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Michael S. Tsirkin]

On Mon, Dec 09, 2019 at 11:43:20AM -0500, Michael S. Tsirkin wrote:
> On Wed, Dec 04, 2019 at 03:31:54PM +0800, pannengyuan@huawei.com wrote:
> > From: Pan Nengyuan <pannengyuan@huawei.com>
> > 
> > Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.
> 
> You want to also mention something about clearing
> .used_elems to avoid chances of double free.
> 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> 
> 
> So let's just name the new one virtio_del_queue then,
> and drop the old one.

I tried but it seems like too much work.

> 
> > ---
> > Changes v2 to v1:
> > - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> > ---
> >  hw/virtio/virtio.c         | 16 +++++++++++-----
> >  include/hw/virtio/virtio.h |  2 ++
> >  2 files changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 04716b5..6de3cfd 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
> >      return &vdev->vq[i];
> >  }
> >  
> > +void virtio_delete_queue(VirtQueue *vq)
> > +{
> > +    vq->vring.num = 0;
> > +    vq->vring.num_default = 0;
> > +    vq->handle_output = NULL;
> > +    vq->handle_aio_output = NULL;
> > +    g_free(vq->used_elems);
> > +    vq->used_elems = NULL;
> > +}
> > +
> >  void virtio_del_queue(VirtIODevice *vdev, int n)
> >  {
> >      if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
> >          abort();
> >      }
> >  
> > -    vdev->vq[n].vring.num = 0;
> > -    vdev->vq[n].vring.num_default = 0;
> > -    vdev->vq[n].handle_output = NULL;
> > -    vdev->vq[n].handle_aio_output = NULL;
> > -    g_free(vdev->vq[n].used_elems);
> > +    virtio_delete_queue(&vdev->vq[n]);
> >  }
> >  
> >  static void virtio_set_isr(VirtIODevice *vdev, int value)
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index c32a815..e18756d 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
> >  
> >  void virtio_del_queue(VirtIODevice *vdev, int n);
> >  
> > +void virtio_delete_queue(VirtQueue *vq);
> > +
> >  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> >                      unsigned int len);
> >  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> > -- 
> > 2.7.2.windows.1
> > 

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Pan Nengyuan]

On 2019/12/10 0:58, Michael S. Tsirkin wrote:
> On Mon, Dec 09, 2019 at 11:43:20AM -0500, Michael S. Tsirkin wrote:
>> On Wed, Dec 04, 2019 at 03:31:54PM +0800, pannengyuan@huawei.com wrote:
>>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>>
>>> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.
>>
>> You want to also mention something about clearing
>> .used_elems to avoid chances of double free.
>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>
>>
>> So let's just name the new one virtio_del_queue then,
>> and drop the old one.
> 
> I tried but it seems like too much work.

Yes, some of them do not maintain the vq pointer, so can we just rename
the virtio_delete_queue to avoid confusion?

I have sent a new version before your reply, can you check whether it's
appropriate or not?

> 
>>
>>> ---
>>> Changes v2 to v1:
>>> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
>>> ---
>>>  hw/virtio/virtio.c         | 16 +++++++++++-----
>>>  include/hw/virtio/virtio.h |  2 ++
>>>  2 files changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 04716b5..6de3cfd 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>>>      return &vdev->vq[i];
>>>  }
>>>  
>>> +void virtio_delete_queue(VirtQueue *vq)
>>> +{
>>> +    vq->vring.num = 0;
>>> +    vq->vring.num_default = 0;
>>> +    vq->handle_output = NULL;
>>> +    vq->handle_aio_output = NULL;
>>> +    g_free(vq->used_elems);
>>> +    vq->used_elems = NULL;
>>> +}
>>> +
>>>  void virtio_del_queue(VirtIODevice *vdev, int n)
>>>  {
>>>      if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>>>          abort();
>>>      }
>>>  
>>> -    vdev->vq[n].vring.num = 0;
>>> -    vdev->vq[n].vring.num_default = 0;
>>> -    vdev->vq[n].handle_output = NULL;
>>> -    vdev->vq[n].handle_aio_output = NULL;
>>> -    g_free(vdev->vq[n].used_elems);
>>> +    virtio_delete_queue(&vdev->vq[n]);
>>>  }
>>>  
>>>  static void virtio_set_isr(VirtIODevice *vdev, int value)
>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>> index c32a815..e18756d 100644
>>> --- a/include/hw/virtio/virtio.h
>>> +++ b/include/hw/virtio/virtio.h
>>> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>>>  
>>>  void virtio_del_queue(VirtIODevice *vdev, int n);
>>>  
>>> +void virtio_delete_queue(VirtQueue *vq);
>>> +
>>>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>>>                      unsigned int len);
>>>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
>>> -- 
>>> 2.7.2.windows.1
>>>
> 
> 
> .
>