[RFC PATCH] include/hw: attempt to document VirtIO feature variables (!DISCUSS!)

["Alex Bennée" <alex.bennee@linaro.org>]

We have a bunch of variables associated with the device and the vhost
backend which are used inconsistently throughout the code base. Lets
start trying to bring some order by agreeing what each variable is
for. Some cases to address (vho/vio renames to avoid ambiguous results
while grepping):

virtio->guest_features is mostly the live status of the features field
and read and written as such by the guest. It does get manipulated by
the various load state via virtio_set_features_nocheck(vdev, val) for
migration.

virtio->host_features is the result of vcd->get_features() most of the
time and for vhost-user devices eventually ends up down at the vhost
get features message:

  ./hw/virtio/virtio-bus.c:66:    vdev->host_features = vdc->get_features(vdev, vdev->host_features,

However virtio-net does a lot of direct modification of it:

  ./hw/net/virtio-net.c:3517:        n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
  ./hw/net/virtio-net.c:3529:        n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
  ./hw/net/virtio-net.c:3539:        n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
  ./hw/net/virtio-net.c:3548:        n->host_features |= (1ULL << VIRTIO_NET_F_STANDBY);
  ./hw/virtio/virtio.c:3438:    bool bad = (val & ~(vdev->host_features)) != 0;

And we have this case which propagates the global QMP values for the
device to the host features. This causes the resent regression of
vhost-user-sock due to 69e1c14aa2 (virtio: core: vq reset feature
negotation support) because the reset feature was rejected by the
vhost-user backend causing it to freeze:

  ./hw/virtio/virtio.c:4667:    status->host_features = qmp_decode_features(vdev->device_id,

virtio->backend_features is only used by virtio-net to stash the
vhost_net_get_features features for checking later:

    features = vhost_net_get_features(get_vhost_net(nc->peer), features);
    vdev->vio_backend_features = features;

and:

    if (n->mtu_bypass_backend &&
            !virtio_has_feature(vdev->vio_backend_features, VIRTIO_NET_F_MTU)) {
        features &= ~(1ULL << VIRTIO_NET_F_MTU);
    }

vhost_dev->acked_features seems to mostly reflect
virtio->guest_features (but where in the negotiation cycle?). Except
for vhost_net where is becomes vhost_dev->backend_features

  ./backends/vhost-user.c:87:    b->dev.vho_acked_features = b->vdev->guest_features;
  ./hw/block/vhost-user-blk.c:149:    s->dev.vho_acked_features = vdev->guest_features;
  ./hw/net/vhost_net.c:132:    net->dev.vho_acked_features = net->dev.vho_backend_features;
  ./hw/scsi/vhost-scsi-common.c:53:    vsc->dev.vho_acked_features = vdev->guest_features;
  ./hw/virtio/vhost-user-fs.c:77:    fs->vhost_dev.vho_acked_features = vdev->guest_features;
  ./hw/virtio/vhost-user-i2c.c:46:    i2c->vhost_dev.vho_acked_features = vdev->guest_features;
  ./hw/virtio/vhost-user-rng.c:44:    rng->vhost_dev.vho_acked_features = vdev->guest_features;
  ./hw/virtio/vhost-vsock-common.c:71:    vvc->vhost_dev.vho_acked_features = vdev->guest_features;
  ./hw/virtio/vhost.c:1631:            hdev->vho_acked_features |= bit_mask;

vhost_dev->backend_features has become overloaded with two use cases:

  ./hw/block/vhost-user-blk.c:336:    s->dev.vho_backend_features = 0;
  ./hw/net/vhost_net.c:180:        net->dev.vho_backend_features = qemu_has_vnet_hdr(options->net_backend)
  ./hw/net/vhost_net.c:185:        net->dev.vho_backend_features = 0;
  ./hw/scsi/vhost-scsi.c:220:    vsc->dev.vho_backend_features = 0;
  ./hw/scsi/vhost-user-scsi.c:121:    vsc->dev.vho_backend_features = 0;
  ./hw/virtio/vhost-user.c:2083:        dev->vho_backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
One use for saving the availability of a vhost-net feature and another
for ensuring we add the protocol feature negotiation bit when querying
a vhost backend. Maybe the places where this is queried should really
be bools that can be queried in the appropriate places?

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Stefano Garzarella <sgarzare@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@gmail.com>
---
 include/hw/virtio/vhost.h  | 18 +++++++++++++++---
 include/hw/virtio/virtio.h | 20 +++++++++++++++++++-
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 353252ac3e..502aa5677a 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -88,13 +88,25 @@ struct vhost_dev {
     int vq_index_end;
     /* if non-zero, minimum required value for max_queues */
     int num_queues;
+    /**
+     * vhost feature handling requires matching the feature set
+     * offered by a backend which may be a subset of the total
+     * features eventually offered to the guest.
+     *
+     * @features: available features provided by the backend
+     * @acked_features: final set of negotiated features with the
+     * front-end driver
+     * @backend_features: additional feature bits applied during negotiation
+     *
+     * Finally the @protocol_features is the final protocal feature
+     * set negotiated between QEMU and the backend (after
+     * VHOST_USER_F_PROTOCOL_FEATURES is negotiated)
+     */
     uint64_t features;
-    /** @acked_features: final set of negotiated features */
     uint64_t acked_features;
-    /** @backend_features: backend specific feature bits */
     uint64_t backend_features;
-    /** @protocol_features: final negotiated protocol features */
     uint64_t protocol_features;
+
     uint64_t max_queues;
     uint64_t backend_cap;
     /* @started: is the vhost device started? */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index a973811cbf..9939a0a632 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -93,6 +93,12 @@ enum virtio_device_endian {
     VIRTIO_DEVICE_ENDIAN_BIG,
 };
 
+/**
+ * struct VirtIODevice - common VirtIO structure
+ * @name: name of the device
+ * @status: VirtIO Device Status field
+ *
+ */
 struct VirtIODevice
 {
     DeviceState parent_obj;
@@ -100,9 +106,21 @@ struct VirtIODevice
     uint8_t status;
     uint8_t isr;
     uint16_t queue_sel;
-    uint64_t guest_features;
+    /**
+     * These fields represent a set of VirtIO features at various
+     * levels of the stack. @host_features indicates the complete
+     * feature set the VirtIO device can offer to the driver.
+     * @guest_features indicates which features the VirtIO driver can
+     * support. Finally @backend_features represents everything
+     * supported by the backend. This set might be split between stuff
+     * done by QEMU itself and stuff handled by an external backend
+     * (e.g. vhost). As a result some feature bits may be added or
+     * masked from the backend.
+     */
     uint64_t host_features;
+    uint64_t guest_features;
     uint64_t backend_features;
+
     size_t config_len;
     void *config;
     uint16_t config_vector;
-- 
2.34.1