Re: [PATCH v3 1/7] vdpa: Use iovec for vhost_vdpa_net_load_cmd()

[Hawkins Jiawei]

On 2023/8/17 17:23, Eugenio Perez Martin wrote:
> On Fri, Jul 7, 2023 at 5:27 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> According to VirtIO standard, "The driver MUST follow
>> the VIRTIO_NET_CTRL_MAC_TABLE_SET command by a le32 number,
>> followed by that number of non-multicast MAC addresses,
>> followed by another le32 number, followed by that number
>> of multicast addresses."
>>
>> Considering that these data is not stored in contiguous memory,
>> this patch refactors vhost_vdpa_net_load_cmd() to accept
>> scattered data, eliminating the need for an addtional data copy or
>> packing the data into s->cvq_cmd_out_buffer outside of
>> vhost_vdpa_net_load_cmd().
>>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> ---
>> v3:
>>    - rename argument name to `data_sg` and `data_num`
>>    - use iov_to_buf() suggested by Eugenio
>>
>> v2: 
>> https://lore.kernel.org/all/6d3dc0fc076564a03501e222ef1102a6a7a643af.1688051252.git.yin31149@gmail.com/
>>    - refactor vhost_vdpa_load_cmd() to accept iovec suggested by
>> Eugenio
>>
>>   net/vhost-vdpa.c | 33 +++++++++++++++++++++++++--------
>>   1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 373609216f..31ef6ad6ec 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -620,29 +620,38 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState 
>> *s, size_t out_len,
>>   }
>>
>>   static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
>> -                                       uint8_t cmd, const void *data,
>> -                                       size_t data_size)
>> +                                       uint8_t cmd, const struct iovec 
>> *data_sg,
>> +                                       size_t data_num)
>>   {
>>       const struct virtio_net_ctrl_hdr ctrl = {
>>           .class = class,
>>           .cmd = cmd,
>>       };
>> +    size_t data_size = iov_size(data_sg, data_num);
>>
>>       assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>>
>> +    /* pack the CVQ command header */
>>       memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
>> -    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
>>
>> -    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
>> +    /* pack the CVQ command command-specific-data */
>> +    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),
>
> Nit, any reason for changing the order of the addends? sizeof(ctrl) +
> data_size ?

Hi Eugenio,

Here the code should be changed to `sizeof(ctrl) + data_size` as you
point out.

Since this patch series has already been merged into master, I will
submit a separate patch to correct this problem.

>
>>                                     sizeof(virtio_net_ctrl_ack));
>>   }
>>
>>   static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>>   {
>>       if (virtio_vdev_has_feature(&n->parent_obj, 
>> VIRTIO_NET_F_CTRL_MAC_ADDR)) {
>> +        const struct iovec data = {
>> +            .iov_base = (void *)n->mac,
>
> Assign to void should always be valid, no need for casting here.

Yes, assign to void should be valid for normal pointers.

However, `n->mac` is an array and is treated as a const pointer. It will
trigger the warning "error: initialization discards ‘const’ qualifier
from pointer target type" if we don't add this cast.

Thanks!


>
>> +            .iov_len = sizeof(n->mac),
>> +        };
>>           ssize_t dev_written = vhost_vdpa_net_load_cmd(s, 
>> VIRTIO_NET_CTRL_MAC,
>>                                                     
>> VIRTIO_NET_CTRL_MAC_ADDR_SET,
>> -                                                  n->mac, sizeof(n->mac));
>> +                                                  &data, 1);
>>           if (unlikely(dev_written < 0)) {
>>               return dev_written;
>>           }
>> @@ -665,9 +674,13 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>>       }
>>
>>       mq.virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs);
>> +    const struct iovec data = {
>> +        .iov_base = &mq,
>> +        .iov_len = sizeof(mq),
>> +    };
>>       dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MQ,
>> -                                          VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, 
>> &mq,
>> -                                          sizeof(mq));
>> +                                          VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
>> +                                          &data, 1);
>>       if (unlikely(dev_written < 0)) {
>>           return dev_written;
>>       }
>> @@ -706,9 +719,13 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState 
>> *s,
>>       }
>>
>>       offloads = cpu_to_le64(n->curr_guest_offloads);
>> +    const struct iovec data = {
>> +        .iov_base = &offloads,
>> +        .iov_len = sizeof(offloads),
>> +    };
>>       dev_written = vhost_vdpa_net_load_cmd(s, 
>> VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>>                                             
>> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
>> -                                          &offloads, sizeof(offloads));
>> +                                          &data, 1);
>>       if (unlikely(dev_written < 0)) {
>>           return dev_written;
>>       }
>
> Apart from that:
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
> Thanks!
>