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

[Jason Wang]

在 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