Re: [RFC PATCH v9 08/23] vhost: Decouple vhost_svq_add_split from VirtQueueElement

[Jason Wang <jasowang@redhat.com>]

在 2022/7/11 16:27, Eugenio Perez Martin 写道:
> On Mon, Jul 11, 2022 at 10:00 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 在 2022/7/7 02:39, Eugenio Pérez 写道:
>>> VirtQueueElement comes from the guest, but we're heading SVQ to be able
>>> to inject element without the guest's knowledge.
>>>
>>> To do so, make this accept sg buffers directly, instead of using
>>> VirtQueueElement.
>>>
>>> Add vhost_svq_add_element to maintain element convenience
>>>
>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>> ---
>>>    hw/virtio/vhost-shadow-virtqueue.c | 38 +++++++++++++++++++++---------
>>>    1 file changed, 27 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
>>> index 2fc5789b73..46d3c1d74f 100644
>>> --- a/hw/virtio/vhost-shadow-virtqueue.c
>>> +++ b/hw/virtio/vhost-shadow-virtqueue.c
>>> @@ -172,30 +172,32 @@ static bool vhost_svq_vring_write_descs(VhostShadowVirtqueue *svq, hwaddr *sg,
>>>    }
>>>
>>>    static bool vhost_svq_add_split(VhostShadowVirtqueue *svq,
>>> -                                VirtQueueElement *elem, unsigned *head)
>>> +                                const struct iovec *out_sg, size_t out_num,
>>> +                                const struct iovec *in_sg, size_t in_num,
>>> +                                unsigned *head)
>>>    {
>>>        unsigned avail_idx;
>>>        vring_avail_t *avail = svq->vring.avail;
>>>        bool ok;
>>> -    g_autofree hwaddr *sgs = g_new(hwaddr, MAX(elem->out_num, elem->in_num));
>>> +    g_autofree hwaddr *sgs = NULL;
>>
>> Is this change a must for this patch? (looks not related to the
>> decoupling anyhow)
>>
> Right, the delay on the variable assignment is an artifact I missed in
> the cleaning. I can revert for the next version if any.
>
> With that reverted, can I add the acked-by tag from you?


Yes.

Thanks


>
> Thanks!
>
>> Other looks good.
>>
>> Thanks
>>
>>
>>>        *head = svq->free_head;
>>>
>>>        /* We need some descriptors here */
>>> -    if (unlikely(!elem->out_num && !elem->in_num)) {
>>> +    if (unlikely(!out_num && !in_num)) {
>>>            qemu_log_mask(LOG_GUEST_ERROR,
>>>                          "Guest provided element with no descriptors");
>>>            return false;
>>>        }
>>>
>>> -    ok = vhost_svq_vring_write_descs(svq, sgs, elem->out_sg, elem->out_num,
>>> -                                     elem->in_num > 0, false);
>>> +    sgs = g_new(hwaddr, MAX(out_num, in_num));
>>> +    ok = vhost_svq_vring_write_descs(svq, sgs, out_sg, out_num, in_num > 0,
>>> +                                     false);
>>>        if (unlikely(!ok)) {
>>>            return false;
>>>        }
>>>
>>> -    ok = vhost_svq_vring_write_descs(svq, sgs, elem->in_sg, elem->in_num, false,
>>> -                                     true);
>>> +    ok = vhost_svq_vring_write_descs(svq, sgs, in_sg, in_num, false, true);
>>>        if (unlikely(!ok)) {
>>>            /* TODO unwind out_sg */
>>>            return false;
>>> @@ -223,10 +225,13 @@ static bool vhost_svq_add_split(VhostShadowVirtqueue *svq,
>>>     * takes ownership of the element: In case of failure, it is free and the SVQ
>>>     * is considered broken.
>>>     */
>>> -static bool vhost_svq_add(VhostShadowVirtqueue *svq, VirtQueueElement *elem)
>>> +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)
>>>    {
>>>        unsigned qemu_head;
>>> -    bool ok = vhost_svq_add_split(svq, elem, &qemu_head);
>>> +    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;
>>> @@ -250,6 +255,18 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
>>>        event_notifier_set(&svq->hdev_kick);
>>>    }
>>>
>>> +static bool vhost_svq_add_element(VhostShadowVirtqueue *svq,
>>> +                                  VirtQueueElement *elem)
>>> +{
>>> +    bool ok = vhost_svq_add(svq, elem->out_sg, elem->out_num, elem->in_sg,
>>> +                            elem->in_num, elem);
>>> +    if (ok) {
>>> +        vhost_svq_kick(svq);
>>> +    }
>>> +
>>> +    return ok;
>>> +}
>>> +
>>>    /**
>>>     * Forward available buffers.
>>>     *
>>> @@ -302,12 +319,11 @@ static void vhost_handle_guest_kick(VhostShadowVirtqueue *svq)
>>>                    return;
>>>                }
>>>
>>> -            ok = vhost_svq_add(svq, elem);
>>> +            ok = vhost_svq_add_element(svq, g_steal_pointer(&elem));
>>>                if (unlikely(!ok)) {
>>>                    /* VQ is broken, just return and ignore any other kicks */
>>>                    return;
>>>                }
>>> -            vhost_svq_kick(svq);
>>>            }
>>>
>>>            virtio_queue_set_notification(svq->vq, true);