Re: [PATCH v3 13/15] virtio-net: support queue reset

[Kangjie Xu <kangjie.xu@linux.alibaba.com>]

在 2022/9/5 16:30, Jason Wang 写道:
>
> 在 2022/8/25 16:08, Kangjie Xu 写道:
>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>
>> virtio-net and vhost-kernel implement queue reset.
>> Queued packets in the corresponding queue pair are flushed
>> or purged.
>>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>> ---
>>   hw/net/virtio-net.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 27b59c0ad6..d774a3e652 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -540,6 +540,23 @@ static RxFilterInfo 
>> *virtio_net_query_rxfilter(NetClientState *nc)
>>       return info;
>>   }
>>   +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t 
>> queue_index)
>> +{
>> +    VirtIONet *n = VIRTIO_NET(vdev);
>> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
>> +
>> +    if (!nc->peer) {
>> +        return;
>> +    }
>> +
>> +    if (get_vhost_net(nc->peer) &&
>> +        nc->peer->info->type == NET_CLIENT_DRIVER_TAP) {
>> +        vhost_net_virtqueue_reset(vdev, nc, queue_index);
>> +    }
>> +
>> +    flush_or_purge_queued_packets(nc);
>
>
> But the codes doesn't prevent the usersapce datapath from being used? 
> (e.g vhost=off)
>
> E.g vhost_net_start_one() had:
>
>     if (net->nc->info->poll) {
>         net->nc->info->poll(net->nc, false);
>     }
I review this part in vhost_net_start/stop_one().

I didn't take the usersapce datapath into consideration. If we don't 
prevent it, the userspace datapath may access it and causes some 
problems. From this point, we should disable it.

But if we add net->nc->info->poll in vq reset, it can only operate at 
queue-pair level, which obeys "per-virtqueue".

What's your opinion on this point? I think it's ok to add it in vq reset.

>
> And I will wonder if it's better to consider to:
>
> 1) factor out the per virtqueue start/stop from 
> vhost_net_start/stop_one()
>
> 2) simply use the helper factored out via step 1)
>
> Thanks
>
I have a different idea about it, vhost_virtqueue_start/stop()(in 
hw/virtio/vhost.c) are already good abstractions of per virtqueue 
start/stop.

These two functions are used in vhost_dev_start. It's just because 
vhost-net needs some configuration, so we add net->nc->info->poll and 
set_backend for it. But for other devices using vhost_dev_start, 
set_backend and net->nc->info->poll may be not necessary.

I think your apporach to abstract per virtqueue start/stop from 
vhost_net_start/stop_one() will break the generality of 
vhost_dev_start(), which is a common interface for different devices.

To conclude my opinions

1. I think we need to add net->nc->info->poll in our 
vhost_net_virtqueue_reset() and vhost_net_virtqueue_restart()

2. We still need vhost_net_virtqueue_reset() and 
vhost_net_virtqueue_restart().

     a. vhost_net_virtqueue_reset() is a wrapper for vhost_virtqueue_stop().

     b. vhost_net_virtqueue_restart() is a wrapper for 
vhost_virtqueue_start().

Thanks

>
>> +}
>> +
>>   static void virtio_net_reset(VirtIODevice *vdev)
>>   {
>>       VirtIONet *n = VIRTIO_NET(vdev);
>> @@ -3784,6 +3801,7 @@ static void virtio_net_class_init(ObjectClass 
>> *klass, void *data)
>>       vdc->set_features = virtio_net_set_features;
>>       vdc->bad_features = virtio_net_bad_features;
>>       vdc->reset = virtio_net_reset;
>> +    vdc->queue_reset = virtio_net_queue_reset;
>>       vdc->set_status = virtio_net_set_status;
>>       vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
>>       vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;