Re: [RFC v3 06/29] virtio-net: Honor VIRTIO_CONFIG_S_DEVICE_STOPPED

[Jason Wang <jasowang@redhat.com>]

在 2021/6/1 下午3:13, Eugenio Perez Martin 写道:
> On Wed, May 26, 2021 at 3:10 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 在 2021/5/26 上午9:06, Jason Wang 写道:
>>> 在 2021/5/20 上午12:28, Eugenio Pérez 写道:
>>>> So the guest can stop and start net device. It implements the RFC
>>>> https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html
>>>>
>>>>
>>>> To stop (as "pause") the device is required to migrate status and vring
>>>> addresses between device and SVQ.
>>>>
>>>> This is a WIP commit: as with VIRTIO_F_QUEUE_STATE, is introduced in
>>>> virtio_config.h before of even proposing for the kernel, with no feature
>>>> flag, and, with no checking in the device. It also needs a modified
>>>> vp_vdpa driver that supports to set and retrieve status.
>>>>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> ---
>>>>    include/standard-headers/linux/virtio_config.h | 2 ++
>>>>    hw/net/virtio-net.c                            | 4 +++-
>>>>    2 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/standard-headers/linux/virtio_config.h
>>>> b/include/standard-headers/linux/virtio_config.h
>>>> index 59fad3eb45..b3f6b1365d 100644
>>>> --- a/include/standard-headers/linux/virtio_config.h
>>>> +++ b/include/standard-headers/linux/virtio_config.h
>>>> @@ -40,6 +40,8 @@
>>>>    #define VIRTIO_CONFIG_S_DRIVER_OK    4
>>>>    /* Driver has finished configuring features */
>>>>    #define VIRTIO_CONFIG_S_FEATURES_OK    8
>>>> +/* Device is stopped */
>>>> +#define VIRTIO_CONFIG_S_DEVICE_STOPPED 32
>>>>    /* Device entered invalid state, driver must reset it */
>>>>    #define VIRTIO_CONFIG_S_NEEDS_RESET    0x40
>>>>    /* We've given up on this device. */
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 96a3cc8357..2d3caea289 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -198,7 +198,9 @@ static bool virtio_net_started(VirtIONet *n,
>>>> uint8_t status)
>>>>    {
>>>>        VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>>        return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>> -        (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
>>>> +        (!(n->status & VIRTIO_CONFIG_S_DEVICE_STOPPED)) &&
>>>> +        (n->status & VIRTIO_NET_S_LINK_UP) &&
>>>> +        vdev->vm_running;
>>>>    }
>>>>      static void virtio_net_announce_notify(VirtIONet *net)
>>>
>>> It looks to me this is only the part of pause.
> For SVQ we need to switch vring addresses, and a full reset of the
> device is required for that. Actually, the pause is just used to
> recover
>
> If you prefer this could be sent as a separate series where the full
> pause/resume cycle is implemented, and then SVQ uses the pause part.
> However there are no use for the resume part at the moment.


That would be fine if you can send it in another series.


>
>> And even for pause, I don't see anything that prevents rx/tx from being
>> executed? (E.g virtio_net_handle_tx_bh or virtio_net_handle_rx).
>>
> virtio_net_started is called from virtio_net_set_status. If
> _S_DEVICE_STOPPED is true, the former return false, and variable
> queue_started is false in the latter:
>    queue_started =
>              virtio_net_started(n, queue_status) && !n->vhost_started;
>
> After that, it should work like a regular device reset or link down if
> I'm not wrong, and the last part of virtio_net_set_status should
> delete timer or cancel bh.


You are right.

Thanks


>
>> Thanks
>>
>>
>>> We still need the resume?
>>>
>>> Thanks
>>>
>>>