Re: [PATCH v4 4/8] vdpa: Avoid using vhost_vdpa_net_load_*() outside vhost_vdpa_net_load()

[Hawkins Jiawei]

在 2023/10/4 01:48, Eugenio Perez Martin 写道:
> On Tue, Aug 29, 2023 at 7:55 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> Next patches in this series will refactor vhost_vdpa_net_load_cmd()
>> to iterate through the control commands shadow buffers, allowing QEMU
>> to send CVQ state load commands in parallel at device startup.
>>
>> Considering that QEMU always forwards the CVQ command serialized
>> outside of vhost_vdpa_net_load(), it is more elegant to send the
>> CVQ commands directly without invoking vhost_vdpa_net_load_*() helpers.
>>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> ---
>> v4:
>>    - pack CVQ command by iov_from_buf() instead of accessing
>> `out` directly suggested by Eugenio
>>
>> v3: 
>> https://lore.kernel.org/all/428a8fac2a29b37757fa15ca747be93c0226cb1f.1689748694.git.yin31149@gmail.com/
>>
>>   net/vhost-vdpa.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index e6342b213f..7c67063469 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -1097,12 +1097,14 @@ static NetClientInfo net_vhost_vdpa_cvq_info = {
>>    */
>>   static int vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
>>                                                          VirtQueueElement 
>> *elem,
>> -                                                       struct iovec *out)
>> +                                                       struct iovec *out,
>> +                                                       const struct iovec 
>> *in)
>>   {
>>       struct virtio_net_ctrl_mac mac_data, *mac_ptr;
>>       struct virtio_net_ctrl_hdr *hdr_ptr;
>>       uint32_t cursor;
>>       ssize_t r;
>> +    uint8_t on = 1;
>>
>>       /* parse the non-multicast MAC address entries from CVQ command */
>>       cursor = sizeof(*hdr_ptr);
>> @@ -1150,7 +1152,15 @@ static int 
>> vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
>>        * filter table to the vdpa device, it should send the
>>        * VIRTIO_NET_CTRL_RX_PROMISC CVQ command to enable promiscuous mode
>>        */
>> -    r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, 1);
>> +    cursor = 0;
>
> I think this is redundant, as "cursor" is not used by the new code and
> it is already set to 0 before its usage, isn't it?
>

You are right, I will remove this code in the v5 patch.

>> +    hdr_ptr = out->iov_base;
>> +    out->iov_len = sizeof(*hdr_ptr) + sizeof(on);
>> +    assert(out->iov_len < vhost_vdpa_net_cvq_cmd_page_len());
>> +
>
> I think we can assume this assertion is never hit, as out->iov_len is
> controlled by this function.
>

Thanks for your suggestion, I will remove this assertion.

Thanks!


> Apart from these nits,
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
>> +    hdr_ptr->class = VIRTIO_NET_CTRL_RX;
>> +    hdr_ptr->cmd = VIRTIO_NET_CTRL_RX_PROMISC;
>> +    iov_from_buf(out, 1, sizeof(*hdr_ptr), &on, sizeof(on));
>> +    r = vhost_vdpa_net_cvq_add(s, out, 1, in, 1);
>>       if (unlikely(r < 0)) {
>>           return r;
>>       }
>> @@ -1268,7 +1278,7 @@ static int 
>> vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>>            * the CVQ command direclty.
>>            */
>>           dev_written = vhost_vdpa_net_excessive_mac_filter_cvq_add(s, elem,
>> -                                                                  &out);
>> +                                                            &out, &vdpa_in);
>>           if (unlikely(dev_written < 0)) {
>>               goto out;
>>           }
>> --
>> 2.25.1
>>
>