diff mbox series

[RFC,v9,09/23] vhost: Add SVQElement

Message ID 20220706184008.1649478-10-eperezma@redhat.com
State New
Headers show
Series Net Control VQ support in SVQ | expand

Commit Message

Eugenio Perez Martin July 6, 2022, 6:39 p.m. UTC 
This will allow SVQ to add metadata to the different queue elements. To
simplify changes, only store actual element at this patch.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.h |  8 ++++--
 hw/virtio/vhost-shadow-virtqueue.c | 41 ++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 16 deletions(-)

Comments

Jason Wang July 11, 2022, 9 a.m. UTC | #1 
在 2022/7/7 02:39, Eugenio Pérez 写道:
> This will allow SVQ to add metadata to the different queue elements. To
> simplify changes, only store actual element at this patch.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-shadow-virtqueue.h |  8 ++++--
>   hw/virtio/vhost-shadow-virtqueue.c | 41 ++++++++++++++++++++----------
>   2 files changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> index 0fbdd69153..e434dc63b0 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -15,6 +15,10 @@
>   #include "standard-headers/linux/vhost_types.h"
>   #include "hw/virtio/vhost-iova-tree.h"
>   
> +typedef struct SVQElement {
> +    VirtQueueElement *elem;
> +} SVQElement;
> +
>   typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
>   typedef int (*ShadowVirtQueueStart)(VhostShadowVirtqueue *svq,
>                                       void *opaque);
> @@ -55,8 +59,8 @@ typedef struct VhostShadowVirtqueue {
>       /* IOVA mapping */
>       VhostIOVATree *iova_tree;
>   
> -    /* Map for use the guest's descriptors */
> -    VirtQueueElement **ring_id_maps;
> +    /* Each element context */
> +    SVQElement *ring_id_maps;
>   
>       /* Next VirtQueue element that guest made available */
>       VirtQueueElement *next_guest_avail_elem;
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 46d3c1d74f..913bca8769 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -237,7 +237,7 @@ static bool vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
>           return false;
>       }
>   
> -    svq->ring_id_maps[qemu_head] = elem;
> +    svq->ring_id_maps[qemu_head].elem = elem;
>       return true;
>   }
>   
> @@ -385,15 +385,25 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
>       return i;
>   }
>   
> -static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> -                                           uint32_t *len)
> +static bool vhost_svq_is_empty_elem(SVQElement elem)
> +{
> +    return elem.elem == NULL;
> +}
> +
> +static SVQElement vhost_svq_empty_elem(void)
> +{
> +    return (SVQElement){};
> +}


I wonder what's the benefit of using this instead of passing pointer to 
SVQElement and using memset().


> +
> +static SVQElement vhost_svq_get_buf(VhostShadowVirtqueue *svq, uint32_t *len)
>   {
>       const vring_used_t *used = svq->vring.used;
>       vring_used_elem_t used_elem;
> +    SVQElement svq_elem = vhost_svq_empty_elem();
>       uint16_t last_used, last_used_chain, num;
>   
>       if (!vhost_svq_more_used(svq)) {
> -        return NULL;
> +        return svq_elem;
>       }
>   
>       /* Only get used array entries after they have been exposed by dev */
> @@ -406,24 +416,25 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
>       if (unlikely(used_elem.id >= svq->vring.num)) {
>           qemu_log_mask(LOG_GUEST_ERROR, "Device %s says index %u is used",
>                         svq->vdev->name, used_elem.id);
> -        return NULL;
> +        return svq_elem;
>       }
>   
> -    if (unlikely(!svq->ring_id_maps[used_elem.id])) {
> +    svq_elem = svq->ring_id_maps[used_elem.id];
> +    svq->ring_id_maps[used_elem.id] = vhost_svq_empty_elem();
> +    if (unlikely(vhost_svq_is_empty_elem(svq_elem))) {


Any reason we can't simply assign NULL to ring_id_maps[used_elem.id]?

Thanks


>           qemu_log_mask(LOG_GUEST_ERROR,
>               "Device %s says index %u is used, but it was not available",
>               svq->vdev->name, used_elem.id);
> -        return NULL;
> +        return svq_elem;
>       }
>   
> -    num = svq->ring_id_maps[used_elem.id]->in_num +
> -          svq->ring_id_maps[used_elem.id]->out_num;
> +    num = svq_elem.elem->in_num + svq_elem.elem->out_num;
>       last_used_chain = vhost_svq_last_desc_of_chain(svq, num, used_elem.id);
>       svq->desc_next[last_used_chain] = svq->free_head;
>       svq->free_head = used_elem.id;
>   
>       *len = used_elem.len;
> -    return g_steal_pointer(&svq->ring_id_maps[used_elem.id]);
> +    return svq_elem;
>   }
>   
>   /**
> @@ -454,6 +465,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
>           vhost_svq_disable_notification(svq);
>           while (true) {
>               uint32_t len;
> +            SVQElement svq_elem;
>               g_autofree VirtQueueElement *elem = NULL;
>   
>               if (unlikely(i >= svq->vring.num)) {
> @@ -464,11 +476,12 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
>                   return;
>               }
>   
> -            elem = vhost_svq_get_buf(svq, &len);
> -            if (!elem) {
> +            svq_elem = vhost_svq_get_buf(svq, &len);
> +            if (vhost_svq_is_empty_elem(svq_elem)) {
>                   break;
>               }
>   
> +            elem = g_steal_pointer(&svq_elem.elem);
>               virtqueue_fill(vq, elem, len, i++);
>           }
>   
> @@ -611,7 +624,7 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
>       memset(svq->vring.desc, 0, driver_size);
>       svq->vring.used = qemu_memalign(qemu_real_host_page_size(), device_size);
>       memset(svq->vring.used, 0, device_size);
> -    svq->ring_id_maps = g_new0(VirtQueueElement *, svq->vring.num);
> +    svq->ring_id_maps = g_new0(SVQElement, svq->vring.num);
>       svq->desc_next = g_new0(uint16_t, svq->vring.num);
>       for (unsigned i = 0; i < svq->vring.num - 1; i++) {
>           svq->desc_next[i] = cpu_to_le16(i + 1);
> @@ -636,7 +649,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 = g_steal_pointer(&svq->ring_id_maps[i].elem);
>           if (elem) {
>               virtqueue_detach_element(svq->vq, elem, 0);
>           }
Eugenio Perez Martin July 11, 2022, 9:33 a.m. UTC | #2 
On Mon, Jul 11, 2022 at 11:00 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/7/7 02:39, Eugenio Pérez 写道:
> > This will allow SVQ to add metadata to the different queue elements. To
> > simplify changes, only store actual element at this patch.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-shadow-virtqueue.h |  8 ++++--
> >   hw/virtio/vhost-shadow-virtqueue.c | 41 ++++++++++++++++++++----------
> >   2 files changed, 33 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> > index 0fbdd69153..e434dc63b0 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > @@ -15,6 +15,10 @@
> >   #include "standard-headers/linux/vhost_types.h"
> >   #include "hw/virtio/vhost-iova-tree.h"
> >
> > +typedef struct SVQElement {
> > +    VirtQueueElement *elem;
> > +} SVQElement;
> > +
> >   typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> >   typedef int (*ShadowVirtQueueStart)(VhostShadowVirtqueue *svq,
> >                                       void *opaque);
> > @@ -55,8 +59,8 @@ typedef struct VhostShadowVirtqueue {
> >       /* IOVA mapping */
> >       VhostIOVATree *iova_tree;
> >
> > -    /* Map for use the guest's descriptors */
> > -    VirtQueueElement **ring_id_maps;
> > +    /* Each element context */
> > +    SVQElement *ring_id_maps;
> >
> >       /* Next VirtQueue element that guest made available */
> >       VirtQueueElement *next_guest_avail_elem;
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > index 46d3c1d74f..913bca8769 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -237,7 +237,7 @@ static bool vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
> >           return false;
> >       }
> >
> > -    svq->ring_id_maps[qemu_head] = elem;
> > +    svq->ring_id_maps[qemu_head].elem = elem;
> >       return true;
> >   }
> >
> > @@ -385,15 +385,25 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> >       return i;
> >   }
> >
> > -static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> > -                                           uint32_t *len)
> > +static bool vhost_svq_is_empty_elem(SVQElement elem)
> > +{
> > +    return elem.elem == NULL;
> > +}
> > +
> > +static SVQElement vhost_svq_empty_elem(void)
> > +{
> > +    return (SVQElement){};
> > +}
>
>
> I wonder what's the benefit of using this instead of passing pointer to
> SVQElement and using memset().
>

It was a more direct translation of the previous workflow but we can
use memset here for sure.

>
> > +
> > +static SVQElement vhost_svq_get_buf(VhostShadowVirtqueue *svq, uint32_t *len)
> >   {
> >       const vring_used_t *used = svq->vring.used;
> >       vring_used_elem_t used_elem;
> > +    SVQElement svq_elem = vhost_svq_empty_elem();
> >       uint16_t last_used, last_used_chain, num;
> >
> >       if (!vhost_svq_more_used(svq)) {
> > -        return NULL;
> > +        return svq_elem;
> >       }
> >
> >       /* Only get used array entries after they have been exposed by dev */
> > @@ -406,24 +416,25 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> >       if (unlikely(used_elem.id >= svq->vring.num)) {
> >           qemu_log_mask(LOG_GUEST_ERROR, "Device %s says index %u is used",
> >                         svq->vdev->name, used_elem.id);
> > -        return NULL;
> > +        return svq_elem;
> >       }
> >
> > -    if (unlikely(!svq->ring_id_maps[used_elem.id])) {
> > +    svq_elem = svq->ring_id_maps[used_elem.id];
> > +    svq->ring_id_maps[used_elem.id] = vhost_svq_empty_elem();
> > +    if (unlikely(vhost_svq_is_empty_elem(svq_elem))) {
>
>
> Any reason we can't simply assign NULL to ring_id_maps[used_elem.id]?
>

It simply avoids allocating more memory, so error code paths are
simplified, etc. In the kernel, vring_desc_state_split, desc_extra and
similar are not an array of pointers but an array of states, so we
apply the same here. Returning them by value it's not so common
though.

But we can allocate a state per in-flight descriptor for sure.

Thanks!


> Thanks
>
>
> >           qemu_log_mask(LOG_GUEST_ERROR,
> >               "Device %s says index %u is used, but it was not available",
> >               svq->vdev->name, used_elem.id);
> > -        return NULL;
> > +        return svq_elem;
> >       }
> >
> > -    num = svq->ring_id_maps[used_elem.id]->in_num +
> > -          svq->ring_id_maps[used_elem.id]->out_num;
> > +    num = svq_elem.elem->in_num + svq_elem.elem->out_num;
> >       last_used_chain = vhost_svq_last_desc_of_chain(svq, num, used_elem.id);
> >       svq->desc_next[last_used_chain] = svq->free_head;
> >       svq->free_head = used_elem.id;
> >
> >       *len = used_elem.len;
> > -    return g_steal_pointer(&svq->ring_id_maps[used_elem.id]);
> > +    return svq_elem;
> >   }
> >
> >   /**
> > @@ -454,6 +465,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> >           vhost_svq_disable_notification(svq);
> >           while (true) {
> >               uint32_t len;
> > +            SVQElement svq_elem;
> >               g_autofree VirtQueueElement *elem = NULL;
> >
> >               if (unlikely(i >= svq->vring.num)) {
> > @@ -464,11 +476,12 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> >                   return;
> >               }
> >
> > -            elem = vhost_svq_get_buf(svq, &len);
> > -            if (!elem) {
> > +            svq_elem = vhost_svq_get_buf(svq, &len);
> > +            if (vhost_svq_is_empty_elem(svq_elem)) {
> >                   break;
> >               }
> >
> > +            elem = g_steal_pointer(&svq_elem.elem);
> >               virtqueue_fill(vq, elem, len, i++);
> >           }
> >
> > @@ -611,7 +624,7 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
> >       memset(svq->vring.desc, 0, driver_size);
> >       svq->vring.used = qemu_memalign(qemu_real_host_page_size(), device_size);
> >       memset(svq->vring.used, 0, device_size);
> > -    svq->ring_id_maps = g_new0(VirtQueueElement *, svq->vring.num);
> > +    svq->ring_id_maps = g_new0(SVQElement, svq->vring.num);
> >       svq->desc_next = g_new0(uint16_t, svq->vring.num);
> >       for (unsigned i = 0; i < svq->vring.num - 1; i++) {
> >           svq->desc_next[i] = cpu_to_le16(i + 1);
> > @@ -636,7 +649,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 = g_steal_pointer(&svq->ring_id_maps[i].elem);
> >           if (elem) {
> >               virtqueue_detach_element(svq->vq, elem, 0);
> >           }
>
Jason Wang July 12, 2022, 7:49 a.m. UTC | #3 
在 2022/7/11 17:33, Eugenio Perez Martin 写道:
> On Mon, Jul 11, 2022 at 11:00 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 在 2022/7/7 02:39, Eugenio Pérez 写道:
>>> This will allow SVQ to add metadata to the different queue elements. To
>>> simplify changes, only store actual element at this patch.
>>>
>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>> ---
>>>    hw/virtio/vhost-shadow-virtqueue.h |  8 ++++--
>>>    hw/virtio/vhost-shadow-virtqueue.c | 41 ++++++++++++++++++++----------
>>>    2 files changed, 33 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
>>> index 0fbdd69153..e434dc63b0 100644
>>> --- a/hw/virtio/vhost-shadow-virtqueue.h
>>> +++ b/hw/virtio/vhost-shadow-virtqueue.h
>>> @@ -15,6 +15,10 @@
>>>    #include "standard-headers/linux/vhost_types.h"
>>>    #include "hw/virtio/vhost-iova-tree.h"
>>>
>>> +typedef struct SVQElement {
>>> +    VirtQueueElement *elem;
>>> +} SVQElement;
>>> +
>>>    typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
>>>    typedef int (*ShadowVirtQueueStart)(VhostShadowVirtqueue *svq,
>>>                                        void *opaque);
>>> @@ -55,8 +59,8 @@ typedef struct VhostShadowVirtqueue {
>>>        /* IOVA mapping */
>>>        VhostIOVATree *iova_tree;
>>>
>>> -    /* Map for use the guest's descriptors */
>>> -    VirtQueueElement **ring_id_maps;
>>> +    /* Each element context */
>>> +    SVQElement *ring_id_maps;
>>>
>>>        /* Next VirtQueue element that guest made available */
>>>        VirtQueueElement *next_guest_avail_elem;
>>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
>>> index 46d3c1d74f..913bca8769 100644
>>> --- a/hw/virtio/vhost-shadow-virtqueue.c
>>> +++ b/hw/virtio/vhost-shadow-virtqueue.c
>>> @@ -237,7 +237,7 @@ static bool vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
>>>            return false;
>>>        }
>>>
>>> -    svq->ring_id_maps[qemu_head] = elem;
>>> +    svq->ring_id_maps[qemu_head].elem = elem;
>>>        return true;
>>>    }
>>>
>>> @@ -385,15 +385,25 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
>>>        return i;
>>>    }
>>>
>>> -static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
>>> -                                           uint32_t *len)
>>> +static bool vhost_svq_is_empty_elem(SVQElement elem)
>>> +{
>>> +    return elem.elem == NULL;
>>> +}
>>> +
>>> +static SVQElement vhost_svq_empty_elem(void)
>>> +{
>>> +    return (SVQElement){};
>>> +}
>>
>> I wonder what's the benefit of using this instead of passing pointer to
>> SVQElement and using memset().
>>
> It was a more direct translation of the previous workflow but we can
> use memset here for sure.
>
>>> +
>>> +static SVQElement vhost_svq_get_buf(VhostShadowVirtqueue *svq, uint32_t *len)
>>>    {
>>>        const vring_used_t *used = svq->vring.used;
>>>        vring_used_elem_t used_elem;
>>> +    SVQElement svq_elem = vhost_svq_empty_elem();
>>>        uint16_t last_used, last_used_chain, num;
>>>
>>>        if (!vhost_svq_more_used(svq)) {
>>> -        return NULL;
>>> +        return svq_elem;
>>>        }
>>>
>>>        /* Only get used array entries after they have been exposed by dev */
>>> @@ -406,24 +416,25 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
>>>        if (unlikely(used_elem.id >= svq->vring.num)) {
>>>            qemu_log_mask(LOG_GUEST_ERROR, "Device %s says index %u is used",
>>>                          svq->vdev->name, used_elem.id);
>>> -        return NULL;
>>> +        return svq_elem;
>>>        }
>>>
>>> -    if (unlikely(!svq->ring_id_maps[used_elem.id])) {
>>> +    svq_elem = svq->ring_id_maps[used_elem.id];
>>> +    svq->ring_id_maps[used_elem.id] = vhost_svq_empty_elem();
>>> +    if (unlikely(vhost_svq_is_empty_elem(svq_elem))) {
>>
>> Any reason we can't simply assign NULL to ring_id_maps[used_elem.id]?
>>
> It simply avoids allocating more memory, so error code paths are
> simplified, etc. In the kernel, vring_desc_state_split, desc_extra and
> similar are not an array of pointers but an array of states, so we
> apply the same here. Returning them by value it's not so common
> though.


Yes, but kernel validate the used id through a pointer to data (token). 
This is the elem we used before this patch.

The code here looks more like a workaround of adding indirection level 
for elem. We'd better try to avoid that.

Thanks


>
> But we can allocate a state per in-flight descriptor for sure.
>
> Thanks!
>
>
>> Thanks
>>
>>
>>>            qemu_log_mask(LOG_GUEST_ERROR,
>>>                "Device %s says index %u is used, but it was not available",
>>>                svq->vdev->name, used_elem.id);
>>> -        return NULL;
>>> +        return svq_elem;
>>>        }
>>>
>>> -    num = svq->ring_id_maps[used_elem.id]->in_num +
>>> -          svq->ring_id_maps[used_elem.id]->out_num;
>>> +    num = svq_elem.elem->in_num + svq_elem.elem->out_num;
>>>        last_used_chain = vhost_svq_last_desc_of_chain(svq, num, used_elem.id);
>>>        svq->desc_next[last_used_chain] = svq->free_head;
>>>        svq->free_head = used_elem.id;
>>>
>>>        *len = used_elem.len;
>>> -    return g_steal_pointer(&svq->ring_id_maps[used_elem.id]);
>>> +    return svq_elem;
>>>    }
>>>
>>>    /**
>>> @@ -454,6 +465,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
>>>            vhost_svq_disable_notification(svq);
>>>            while (true) {
>>>                uint32_t len;
>>> +            SVQElement svq_elem;
>>>                g_autofree VirtQueueElement *elem = NULL;
>>>
>>>                if (unlikely(i >= svq->vring.num)) {
>>> @@ -464,11 +476,12 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
>>>                    return;
>>>                }
>>>
>>> -            elem = vhost_svq_get_buf(svq, &len);
>>> -            if (!elem) {
>>> +            svq_elem = vhost_svq_get_buf(svq, &len);
>>> +            if (vhost_svq_is_empty_elem(svq_elem)) {
>>>                    break;
>>>                }
>>>
>>> +            elem = g_steal_pointer(&svq_elem.elem);
>>>                virtqueue_fill(vq, elem, len, i++);
>>>            }
>>>
>>> @@ -611,7 +624,7 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
>>>        memset(svq->vring.desc, 0, driver_size);
>>>        svq->vring.used = qemu_memalign(qemu_real_host_page_size(), device_size);
>>>        memset(svq->vring.used, 0, device_size);
>>> -    svq->ring_id_maps = g_new0(VirtQueueElement *, svq->vring.num);
>>> +    svq->ring_id_maps = g_new0(SVQElement, svq->vring.num);
>>>        svq->desc_next = g_new0(uint16_t, svq->vring.num);
>>>        for (unsigned i = 0; i < svq->vring.num - 1; i++) {
>>>            svq->desc_next[i] = cpu_to_le16(i + 1);
>>> @@ -636,7 +649,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 = g_steal_pointer(&svq->ring_id_maps[i].elem);
>>>            if (elem) {
>>>                virtqueue_detach_element(svq->vq, elem, 0);
>>>            }
diff mbox series

Patch

diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
index 0fbdd69153..e434dc63b0 100644
--- a/hw/virtio/vhost-shadow-virtqueue.h
+++ b/hw/virtio/vhost-shadow-virtqueue.h
@@ -15,6 +15,10 @@ 
 #include "standard-headers/linux/vhost_types.h"
 #include "hw/virtio/vhost-iova-tree.h"
 
+typedef struct SVQElement {
+    VirtQueueElement *elem;
+} SVQElement;
+
 typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
 typedef int (*ShadowVirtQueueStart)(VhostShadowVirtqueue *svq,
                                     void *opaque);
@@ -55,8 +59,8 @@  typedef struct VhostShadowVirtqueue {
     /* IOVA mapping */
     VhostIOVATree *iova_tree;
 
-    /* Map for use the guest's descriptors */
-    VirtQueueElement **ring_id_maps;
+    /* Each element context */
+    SVQElement *ring_id_maps;
 
     /* Next VirtQueue element that guest made available */
     VirtQueueElement *next_guest_avail_elem;
diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 46d3c1d74f..913bca8769 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -237,7 +237,7 @@  static bool vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
         return false;
     }
 
-    svq->ring_id_maps[qemu_head] = elem;
+    svq->ring_id_maps[qemu_head].elem = elem;
     return true;
 }
 
@@ -385,15 +385,25 @@  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
     return i;
 }
 
-static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
-                                           uint32_t *len)
+static bool vhost_svq_is_empty_elem(SVQElement elem)
+{
+    return elem.elem == NULL;
+}
+
+static SVQElement vhost_svq_empty_elem(void)
+{
+    return (SVQElement){};
+}
+
+static SVQElement vhost_svq_get_buf(VhostShadowVirtqueue *svq, uint32_t *len)
 {
     const vring_used_t *used = svq->vring.used;
     vring_used_elem_t used_elem;
+    SVQElement svq_elem = vhost_svq_empty_elem();
     uint16_t last_used, last_used_chain, num;
 
     if (!vhost_svq_more_used(svq)) {
-        return NULL;
+        return svq_elem;
     }
 
     /* Only get used array entries after they have been exposed by dev */
@@ -406,24 +416,25 @@  static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
     if (unlikely(used_elem.id >= svq->vring.num)) {
         qemu_log_mask(LOG_GUEST_ERROR, "Device %s says index %u is used",
                       svq->vdev->name, used_elem.id);
-        return NULL;
+        return svq_elem;
     }
 
-    if (unlikely(!svq->ring_id_maps[used_elem.id])) {
+    svq_elem = svq->ring_id_maps[used_elem.id];
+    svq->ring_id_maps[used_elem.id] = vhost_svq_empty_elem();
+    if (unlikely(vhost_svq_is_empty_elem(svq_elem))) {
         qemu_log_mask(LOG_GUEST_ERROR,
             "Device %s says index %u is used, but it was not available",
             svq->vdev->name, used_elem.id);
-        return NULL;
+        return svq_elem;
     }
 
-    num = svq->ring_id_maps[used_elem.id]->in_num +
-          svq->ring_id_maps[used_elem.id]->out_num;
+    num = svq_elem.elem->in_num + svq_elem.elem->out_num;
     last_used_chain = vhost_svq_last_desc_of_chain(svq, num, used_elem.id);
     svq->desc_next[last_used_chain] = svq->free_head;
     svq->free_head = used_elem.id;
 
     *len = used_elem.len;
-    return g_steal_pointer(&svq->ring_id_maps[used_elem.id]);
+    return svq_elem;
 }
 
 /**
@@ -454,6 +465,7 @@  static void vhost_svq_flush(VhostShadowVirtqueue *svq,
         vhost_svq_disable_notification(svq);
         while (true) {
             uint32_t len;
+            SVQElement svq_elem;
             g_autofree VirtQueueElement *elem = NULL;
 
             if (unlikely(i >= svq->vring.num)) {
@@ -464,11 +476,12 @@  static void vhost_svq_flush(VhostShadowVirtqueue *svq,
                 return;
             }
 
-            elem = vhost_svq_get_buf(svq, &len);
-            if (!elem) {
+            svq_elem = vhost_svq_get_buf(svq, &len);
+            if (vhost_svq_is_empty_elem(svq_elem)) {
                 break;
             }
 
+            elem = g_steal_pointer(&svq_elem.elem);
             virtqueue_fill(vq, elem, len, i++);
         }
 
@@ -611,7 +624,7 @@  void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
     memset(svq->vring.desc, 0, driver_size);
     svq->vring.used = qemu_memalign(qemu_real_host_page_size(), device_size);
     memset(svq->vring.used, 0, device_size);
-    svq->ring_id_maps = g_new0(VirtQueueElement *, svq->vring.num);
+    svq->ring_id_maps = g_new0(SVQElement, svq->vring.num);
     svq->desc_next = g_new0(uint16_t, svq->vring.num);
     for (unsigned i = 0; i < svq->vring.num - 1; i++) {
         svq->desc_next[i] = cpu_to_le16(i + 1);
@@ -636,7 +649,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 = g_steal_pointer(&svq->ring_id_maps[i].elem);
         if (elem) {
             virtqueue_detach_element(svq->vq, elem, 0);
         }