On 2023/8/18 23:23, Eugenio Perez Martin wrote:
> On Wed, Jul 19, 2023 at 9:54 AM Hawkins Jiawei <yin31...@gmail.com> wrote:
>>
>> Next patches in this series will no longer perform an
>> immediate poll and check of the device's used buffers
>> for each CVQ state load command. Consequently, there
>> will be multiple pending buffers in the shadow VirtQueue,
>> making it a must for every control command to have its
>> own buffer.
>>
>> To achieve this, this patch refactor vhost_vdpa_net_cvq_add()
>> to accept `struct iovec`, which eliminates the coupling of
>> control commands to `s->cvq_cmd_out_buffer` and `s->status`,
>> allowing them to use their own buffer.
>>
>> Signed-off-by: Hawkins Jiawei <yin31...@gmail.com>
>> ---
>> net/vhost-vdpa.c | 38 ++++++++++++++++++++------------------
>> 1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index d1dd140bf6..6b16c8ece0 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -596,22 +596,14 @@ static void vhost_vdpa_net_cvq_stop(NetClientState *nc)
>> vhost_vdpa_net_client_stop(nc);
>> }
>>
>> -static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, size_t out_len,
>> - size_t in_len)
>> +static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s,
>> + struct iovec *out_sg, size_t out_num,
>> + struct iovec *in_sg, size_t in_num)
>> {
>> - /* Buffers for the device */
>> - const struct iovec out = {
>> - .iov_base = s->cvq_cmd_out_buffer,
>> - .iov_len = out_len,
>> - };
>> - const struct iovec in = {
>> - .iov_base = s->status,
>> - .iov_len = sizeof(virtio_net_ctrl_ack),
>> - };
>> VhostShadowVirtqueue *svq =
>> g_ptr_array_index(s->vhost_vdpa.shadow_vqs, 0);
>> int r;
>>
>> - r = vhost_svq_add(svq, &out, 1, &in, 1, NULL);
>> + r = vhost_svq_add(svq, out_sg, out_num, in_sg, in_num, NULL);
>> if (unlikely(r != 0)) {
>> if (unlikely(r == -ENOSPC)) {
>> qemu_log_mask(LOG_GUEST_ERROR, "%s: No space on device
>> queue\n",
>> @@ -637,6 +629,15 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState
>> *s, uint8_t class,
>> .cmd = cmd,
>> };
>> size_t data_size = iov_size(data_sg, data_num);
>> + /* Buffers for the device */
>> + struct iovec out = {
>> + .iov_base = s->cvq_cmd_out_buffer,
>> + .iov_len = sizeof(ctrl) + data_size,
>> + };
>> + struct iovec in = {
>> + .iov_base = s->status,
>> + .iov_len = sizeof(*s->status),
>> + };
>>
>> assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>>
>> @@ -647,8 +648,7 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState
>> *s, uint8_t class,
>> iov_to_buf(data_sg, data_num, 0,
>> s->cvq_cmd_out_buffer + sizeof(ctrl), data_size);
>>
>> - return vhost_vdpa_net_cvq_add(s, data_size + sizeof(ctrl),
>> - sizeof(virtio_net_ctrl_ack));
>> + return vhost_vdpa_net_cvq_add(s, &out, 1, &in, 1);
>> }
>>
>> static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>> @@ -1222,9 +1222,7 @@ static int
>> vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>> struct iovec out = {
>> .iov_base = s->cvq_cmd_out_buffer,
>> };
>> - /* in buffer used for device model */
>> - const struct iovec in = {
>> - .iov_base = &status,
>> + struct iovec in = {
>
> What if instead of reusing "in" we declare a new struct iovec in the
> condition that calls vhost_vdpa_net_cvq_add? Something in the line of
> "device_in".
>
> I'm also ok with this code, but splitting them would reduce the
> possibility of sending the wrong one to the device / virtio device
> model by mistake.
Hi Eugenio,
Ok, I will refactor this part of code according to your suggestion in
the v4 patch.
Thanks!
>
> Thanks!
>
>> .iov_len = sizeof(status),
>> };
>> ssize_t dev_written = -EINVAL;
>> @@ -1232,6 +1230,8 @@ static int
>> vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>> out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
>> s->cvq_cmd_out_buffer,
>> vhost_vdpa_net_cvq_cmd_page_len());
>> + /* In buffer used for the vdpa device */
>> + in.iov_base = s->status;
>>
>> ctrl = s->cvq_cmd_out_buffer;
>> if (ctrl->class == VIRTIO_NET_CTRL_ANNOUNCE) {
>> @@ -1260,7 +1260,7 @@ static int
>> vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>> goto out;
>> }
>> } else {
>> - dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len,
>> sizeof(status));
>> + dev_written = vhost_vdpa_net_cvq_add(s, &out, 1, &in, 1);
>> if (unlikely(dev_written < 0)) {
>> goto out;
>> }
>> @@ -1276,6 +1276,8 @@ static int
>> vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>> }
>>
>> status = VIRTIO_NET_ERR;
>> + /* In buffer used for the device model */
>> + in.iov_base = &status;
>> virtio_net_handle_ctrl_iov(svq->vdev, &in, 1, &out, 1);
>> if (status != VIRTIO_NET_OK) {
>> error_report("Bad CVQ processing in model");
>> --
>> 2.25.1
>>
>
