Re: [RFC PATCH v9 12/23] vhost: Add opaque member to SVQElement

[Jason Wang]

在 2022/7/7 02:39, Eugenio Pérez 写道:
> When qemu injects buffers to the vdpa device it will be used to maintain
> contextual data. If SVQ has no operation, it will be used to maintain
> the VirtQueueElement pointer.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-shadow-virtqueue.h |  3 ++-
>   hw/virtio/vhost-shadow-virtqueue.c | 13 +++++++------
>   2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> b/hw/virtio/vhost-shadow-virtqueue.h
> index 0e434e9fd0..a811f90e01 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -16,7 +16,8 @@
>   #include "hw/virtio/vhost-iova-tree.h"
>   
>   typedef struct SVQElement {
> -    VirtQueueElement *elem;
> +    /* Opaque data */
> +    void *opaque;


So I wonder if we can simply:

1) introduce a opaque to VirtQueueElement
2) store pointers to ring_id_maps

Since

1) VirtQueueElement's member looks general
2) help to reduce the tricky codes like vhost_svq_empty_elem() and 
vhost_svq_empty_elem().

Thanks


>   
>       /* Last descriptor of the chain */
>       uint32_t last_chain_id;
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> b/hw/virtio/vhost-shadow-virtqueue.c
> index c5e49e51c5..492bb12b5f 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -237,7 +237,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const 
> VhostShadowVirtqueue *svq,
>    */
>   static bool vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec 
> *out_sg,
>                             size_t out_num, const struct iovec *in_sg,
> -                          size_t in_num, VirtQueueElement *elem)
> +                          size_t in_num, void *opaque)
>   {
>       SVQElement *svq_elem;
>       unsigned qemu_head;
> @@ -245,13 +245,12 @@ static bool vhost_svq_add(VhostShadowVirtqueue *svq, 
> const struct iovec *out_sg,
>       bool ok = vhost_svq_add_split(svq, out_sg, out_num, in_sg, in_num,
>                                     &qemu_head);
>       if (unlikely(!ok)) {
> -        g_free(elem);
>           return false;
>       }
>   
>       n = out_num + in_num;
>       svq_elem = &svq->ring_id_maps[qemu_head];
> -    svq_elem->elem = elem;
> +    svq_elem->opaque = opaque;
>       svq_elem->last_chain_id = vhost_svq_last_desc_of_chain(svq, n, qemu_head);
>       return true;
>   }
> @@ -277,6 +276,8 @@ static bool vhost_svq_add_element(VhostShadowVirtqueue *svq,
>                               elem->in_num, elem);
>       if (ok) {
>           vhost_svq_kick(svq);
> +    } else {
> +        g_free(elem);
>       }
>   
>       return ok;
> @@ -392,7 +393,7 @@ static void 
> vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
>   
>   static bool vhost_svq_is_empty_elem(SVQElement elem)
>   {
> -    return elem.elem == NULL;
> +    return elem.opaque == NULL;
>   }
>   
>   static SVQElement vhost_svq_empty_elem(void)
> @@ -483,7 +484,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
>                   break;
>               }
>   
> -            elem = g_steal_pointer(&svq_elem.elem);
> +            elem = g_steal_pointer(&svq_elem.opaque);
>               virtqueue_fill(vq, elem, len, i++);
>           }
>   
> @@ -651,7 +652,7 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
>   
>       for (unsigned i = 0; i < svq->vring.num; ++i) {
>           g_autofree VirtQueueElement *elem = NULL;
> -        elem = g_steal_pointer(&svq->ring_id_maps[i].elem);
> +        elem = g_steal_pointer(&svq->ring_id_maps[i].opaque);
>           if (elem) {
>               virtqueue_detach_element(svq->vq, elem, 0);
>           }