Re: [PATCH RFC 1/2] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET

On Tue, Aug 29, 2017 at 4:20 PM, Michael S. Tsirkin [off-list ref] wrote:

On Tue, Aug 29, 2017 at 04:07:58PM -0400, Willem de Bruijn wrote:

quoted

From: Willem de Bruijn <willemb@google.com>

Implement the reset communication request defined in the VIRTIO 1.0
specification and introduces in Linux in commit c00bbcf862896 ("virtio:
add VIRTIO_CONFIG_S_NEEDS_RESET device status bit").

Since that patch, the virtio-net driver has added a virtnet_reset
function that implements the requested behavior through calls to the
power management freeze and restore functions.

That function has recently been reverted when its sole caller was
updated. Bring it back and listen for the request from the host on
the config channel.

Implement the feature analogous to other config requests. In
particular, acknowledge the request in the same manner as the
NET_S_ANNOUNCE link announce request, by responding with a new
VIRTIO_NET_CTRL_${TYPE} command. On reception, the host must check
the config status register for success or failure.

Pls make it clearer why do you need these interface extensions.

quoted

The existing freeze handler verifies that no config changes are
running concurrently. Elide this check for reset. The request is
always handled from the config workqueue. No other config requests
can be active or scheduled concurrently on vi->config.

You need to prevent packet TX from being in progress.

I had another look at this.

As of commit 713a98d90c5e ("virtio-net: serialize tx routine during reset")
virtnet_freeze_down calls synchronize_net() after stopping the queues
to quiesce the device before any further actions. This should suffice for
virtnet_reset, too.

The control path can indeed be much simpler than my initial patchset.
The host can read vdev->status to observe when the reset went through.
It adds flag VIRTIO_CONFIG_S_NEEDS_RESET to request the reset.
This flag is cleared by the operation itself.

quoted

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/virtio_net.c        | 69 +++++++++++++++++++++++++++++++++++------
 include/uapi/linux/virtio_net.h |  4 +++

virtio dev or another virtio TC list must be Cc'd on any proposed API changes.

quoted

 2 files changed, 64 insertions(+), 9 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 52ae78ca3d38..5e349226f7c1 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -1458,12 +1458,11 @@ static void virtnet_netpoll(struct net_device *dev)
 }
 #endif

-static void virtnet_ack_link_announce(struct virtnet_info *vi)
+static void virtnet_ack(struct virtnet_info *vi, u8 class, u8 cmd)
 {
      rtnl_lock();
-     if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
-                               VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL))
-             dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
+     if (!virtnet_send_command(vi, class, cmd, NULL))
+             dev_warn(&vi->dev->dev, "Failed to ack %u.%u\n", class, cmd);
      rtnl_unlock();
 }

@@ -1857,13 +1856,16 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
      .set_link_ksettings = virtnet_set_link_ksettings,
 };

-static void virtnet_freeze_down(struct virtio_device *vdev)
+static void virtnet_freeze_down(struct virtio_device *vdev, bool in_reset)
 {
      struct virtnet_info *vi = vdev->priv;
      int i;

-     /* Make sure no work handler is accessing the device */
-     flush_work(&vi->config_work);
+     /* Make sure no work handler is accessing the device,
+      * unless this call is made from the reset work handler itself.
+      */
+     if (!in_reset)
+             flush_work(&vi->config_work);

      netif_device_detach(vi->dev);
      netif_tx_disable(vi->dev);

@@ -1878,6 +1880,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
 }

 static int init_vqs(struct virtnet_info *vi);
+static void remove_vq_common(struct virtnet_info *vi);

 static int virtnet_restore_up(struct virtio_device *vdev)
 {

@@ -1906,6 +1909,45 @@ static int virtnet_restore_up(struct virtio_device *vdev)
      return err;
 }

+static int virtnet_reset(struct virtnet_info *vi)
+{
+     struct virtio_device *dev = vi->vdev;
+     bool failed;
+     int ret;
+
+     virtio_config_disable(dev);
+     failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
+     virtnet_freeze_down(dev, true);
+     remove_vq_common(vi);
+
+     virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
+     virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
+
+     /* Restore the failed status (see virtio_device_restore). */
+     if (failed)
+             virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
+
+     ret = virtio_finalize_features(dev);
+     if (ret)
+             goto err;
+
+     ret = virtnet_restore_up(dev);
+     if (ret)
+             goto err;
+
+     ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
+     if (ret)
+             goto err;
+
+     virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+     virtio_config_enable(dev);
+     return 0;
+
+err:
+     virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
+     return ret;
+}
+
 static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
 {
      struct scatterlist sg;

@@ -2085,7 +2127,16 @@ static void virtnet_config_changed_work(struct work_struct *work)

      if (v & VIRTIO_NET_S_ANNOUNCE) {
              netdev_notify_peers(vi->dev);
-             virtnet_ack_link_announce(vi);
+             virtnet_ack(vi, VIRTIO_NET_CTRL_ANNOUNCE,
+                         VIRTIO_NET_CTRL_ANNOUNCE_ACK);
+     }
+
+     if (vi->vdev->config->get_status(vi->vdev) &
+         VIRTIO_CONFIG_S_NEEDS_RESET) {
+             virtnet_reset(vi);
+             virtnet_ack(vi, VIRTIO_NET_CTRL_RESET,
+                         VIRTIO_NET_CTRL_RESET_ACK);
+
      }

      /* Ignore unknown (future) status bits */

@@ -2708,7 +2759,7 @@ static __maybe_unused int virtnet_freeze(struct virtio_device *vdev)
      struct virtnet_info *vi = vdev->priv;

      virtnet_cpu_notif_remove(vi);
-     virtnet_freeze_down(vdev);
+     virtnet_freeze_down(vdev, false);
      remove_vq_common(vi);

      return 0;

diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index fc353b518288..188fdc528f13 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h

@@ -245,4 +245,8 @@ struct virtio_net_ctrl_mq {
 #define VIRTIO_NET_CTRL_GUEST_OFFLOADS   5
 #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET        0

+/* Signal that the driver received and executed the reset command. */
+#define VIRTIO_NET_CTRL_RESET                        6
+#define VIRTIO_NET_CTRL_RESET_ACK            0
+
 #endif /* _UAPI_LINUX_VIRTIO_NET_H */
--

2.14.1.342.g6490525c54-goog