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

[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.

>
> 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.

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