在 2023/8/17 22:05, Eugenio Perez Martin 写道:
> On Thu, Aug 17, 2023 at 2:42 PM Hawkins Jiawei <yin31...@gmail.com> wrote:
>>
>> On 2023/8/17 17:23, Eugenio Perez Martin wrote:
>>> On Fri, Jul 7, 2023 at 5:27 PM Hawkins Jiawei <yin31...@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 <yin31...@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.yin31...@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.
>>
>
> Ouch, I didn't realize that. No need to make it back again, I was just
> trying to reduce lines changed.
Ok, I got it. Regardless, thank you for your review!
>
>>>
>>>> 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.
>>
>
> Got it, I didn't realize it. Everything is ok then.
>
> Thanks!
>
>> 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 <epere...@redhat.com>
>>>
>>> Thanks!
>>>
>>
>
