Virtualization - Jun 2021 - [RFC v3 06/29] virtio-net: Honor VIRTIO_CONFIG_S_DEVICE_STOPPED

? 2021/6/1 ??3:13, Eugenio Perez Martin ??:
> On Wed, May 26, 2021 at 3:10 AM Jason Wang <jasowang at 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 at 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
>>>
>>>