rte_virtio: cleanup feature negotiation

Moved virtio backend specific code out
of common feature negotiation path.

virtio-pci features are stored in PCI
register. Before feature negotiation
they can be considered device (host)
features. They have to be negotiated
with local features and written back
into the very same register.

For virtio-user, the situation is a
bit different - vhost-user protocol
contains two feature-related commands
GET_FEATURES and SET_FEATURES. While
SET_ sends already negotiated features,
GET_ always returns device (host)
features.

Previously, we used to store native
device (host) features in local
variables to (ineptly) make the
negotiation path the same for
virtio-pci and virtio-user.

Instead of fixing this functionality,
it has been removed. Now, the
vtpci->get_features might be either
negotiated or not. This is now stated
in this function's doc.

This solution doesn't have any
drawbacks and simplifies the code.

Change-Id: I270e9cad8baaeae61b58992ee06e9069eac10aaf
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/381472
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>

lib/bdev/virtio/rte_virtio/virtio_dev.c

@@ -53,6 +53,7 @@
 #include <rte_eal.h>
 #include <rte_dev.h>
 
+#include "virtio_user/vhost.h"
 #include "virtio_dev.h"
 #include "virtio_pci.h"
 #include "virtio_logs.h"
@@ -242,37 +243,20 @@ virtio_alloc_queues(struct virtio_dev *dev)
 static int
 virtio_negotiate_features(struct virtio_dev *dev, uint64_t req_features)
 {
-	uint64_t host_features;
+	uint64_t host_features = vtpci_ops(dev)->get_features(dev);
+	int rc;
 
-	/* Prepare guest_features: feature that driver wants to support */
+	PMD_INIT_LOG(DEBUG, "guest features = %" PRIx64, req_features);
-	PMD_INIT_LOG(DEBUG, "guest_features before negotiate = %" PRIx64,
+	PMD_INIT_LOG(DEBUG, "device features = %" PRIx64, host_features);
-		req_features);
 
-	/* Read device(host) feature bits */
+	rc = vtpci_ops(dev)->set_features(dev, req_features & host_features);
-	host_features = vtpci_ops(dev)->get_features(dev);
+	if (rc != 0) {
-	PMD_INIT_LOG(DEBUG, "host_features before negotiate = %" PRIx64,
+		PMD_INIT_LOG(ERR, "failed to negotiate device features.");
-		host_features);
-
-	/*
-	 * Negotiate features: Subset of device feature bits are written back
-	 * guest feature bits.
-	 */
-	dev->req_guest_features = req_features;
-	dev->guest_features = vtpci_negotiate_features(dev, host_features);
-	PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64,
-		dev->guest_features);
-
-	if (!vtpci_with_feature(dev, VIRTIO_F_VERSION_1)) {
-		if (dev->modern) {
-			PMD_INIT_LOG(ERR,
-				     "VIRTIO_F_VERSION_1 features is not enabled.");
 		return -1;
 	}
 
-		return 0;
+	PMD_INIT_LOG(DEBUG, "negotiated features = %" PRIx64, dev->negotiated_features);
-	}
 
-	dev->modern = 1;
 	vtpci_set_status(dev, VIRTIO_CONFIG_STATUS_FEATURES_OK);
 	if (!(vtpci_get_status(dev) & VIRTIO_CONFIG_STATUS_FEATURES_OK)) {
 		PMD_INIT_LOG(ERR,

lib/bdev/virtio/rte_virtio/virtio_dev.h

@@ -47,8 +47,10 @@ struct virtio_dev {
 
 	/* Device index. */
 	uint32_t	id;
-	uint64_t	req_guest_features;
+
-	uint64_t	guest_features;
+	/* Common device & guest features. */
+	uint64_t	negotiated_features;
+
 	int		is_hw;
 
 	/** Modern/legacy virtio device flag. */

lib/bdev/virtio/rte_virtio/virtio_pci.c

@@ -200,16 +200,19 @@ legacy_get_features(struct virtio_dev *dev)
 	return dst;
 }
 
-static void
+static int
 legacy_set_features(struct virtio_dev *dev, uint64_t features)
 {
 	if ((features >> 32) != 0) {
 		PMD_DRV_LOG(ERR,
 			"only 32 bit features are allowed for legacy virtio!");
-		return;
+		return -1;
 	}
 	rte_pci_ioport_write(vtpci_io(dev), &features, 4,
 		VIRTIO_PCI_GUEST_FEATURES);
+	dev->negotiated_features = features;
+
+	return 0;
 }
 
 static uint8_t
@@ -373,11 +376,17 @@ modern_get_features(struct virtio_dev *dev)
 	return ((uint64_t)features_hi << 32) | features_lo;
 }
 
-static void
+static int
 modern_set_features(struct virtio_dev *dev, uint64_t features)
 {
 	struct virtio_hw *hw = virtio_dev_get_hw(dev);
 
+	if ((features & (1ULL << VIRTIO_F_VERSION_1)) == 0) {
+		PMD_INIT_LOG(ERR,
+			     "VIRTIO_F_VERSION_1 feature is not enabled.");
+		return -1;
+	}
+
 	rte_write32(0, &hw->common_cfg->guest_feature_select);
 	rte_write32(features & ((1ULL << 32) - 1),
 		    &hw->common_cfg->guest_feature);
@@ -385,6 +394,10 @@ modern_set_features(struct virtio_dev *dev, uint64_t features)
 	rte_write32(1, &hw->common_cfg->guest_feature_select);
 	rte_write32(features >> 32,
 		    &hw->common_cfg->guest_feature);
+
+	dev->negotiated_features = features;
+
+	return 0;
 }
 
 static uint8_t
@@ -535,21 +548,6 @@ vtpci_write_dev_config(struct virtio_dev *dev, size_t offset,
 	vtpci_ops(dev)->write_dev_cfg(dev, offset, src, length);
 }
 
-uint64_t
-vtpci_negotiate_features(struct virtio_dev *dev, uint64_t host_features)
-{
-	uint64_t features;
-
-	/*
-	 * Limit negotiated features to what the driver, virtqueue, and
-	 * host all support.
-	 */
-	features = host_features & dev->req_guest_features;
-	vtpci_ops(dev)->set_features(dev, features);
-
-	return features;
-}
-
 void
 vtpci_reset(struct virtio_dev *dev)
 {

lib/bdev/virtio/rte_virtio/virtio_pci.h

@@ -196,8 +196,18 @@ struct virtio_pci_ops {
 	uint8_t (*get_status)(struct virtio_dev *hw);
 	void    (*set_status)(struct virtio_dev *hw, uint8_t status);
 
-	uint64_t (*get_features)(struct virtio_dev *hw);
+	/**
-	void     (*set_features)(struct virtio_dev *hw, uint64_t features);
+	 * Get device features. The features might be already
+	 * negotiated with driver (guest) features.
+	 */
+	uint64_t (*get_features)(struct virtio_dev *vdev);
+
+	/**
+	 * Negotiate and set device features.
+	 * The negotiation can fail with return code -1.
+	 * This function should also set vdev->negotiated_features field.
+	 */
+	int     (*set_features)(struct virtio_dev *vdev, uint64_t features);
 
 	uint8_t (*get_isr)(struct virtio_dev *hw);
 
@@ -264,7 +274,7 @@ extern struct virtio_driver g_virtio_driver;
 static inline int
 vtpci_with_feature(struct virtio_dev *dev, uint64_t bit)
 {
-	return (dev->guest_features & (1ULL << bit)) != 0;
+	return (dev->negotiated_features & (1ULL << bit)) != 0;
 }
 
 /**

lib/bdev/virtio/rte_virtio/virtio_user.c

@@ -94,17 +94,31 @@ static uint64_t
 virtio_user_get_features(struct virtio_dev *vdev)
 {
 	struct virtio_user_dev *dev = virtio_dev_get_user_dev(vdev);
+	uint64_t features;
 
-	/* unmask feature bits defined in vhost user protocol */
+	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES, &features) < 0) {
-	return dev->device_features;
+		PMD_INIT_LOG(ERR, "get_features failed: %s", strerror(errno));
+		return 0;
 	}
 
-static void
+	return features;
+}
+
+static int
 virtio_user_set_features(struct virtio_dev *vdev, uint64_t features)
 {
 	struct virtio_user_dev *dev = virtio_dev_get_user_dev(vdev);
+	int ret;
 
-	dev->features = features & dev->device_features;
+	ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
+	if (ret < 0) {
+		return -1;
+	}
+
+	vdev->negotiated_features = features;
+	vdev->modern = vtpci_with_feature(vdev, VIRTIO_F_VERSION_1);
+
+	return 0;
 }
 
 static uint8_t

lib/bdev/virtio/rte_virtio/virtio_user/virtio_user_dev.c

@@ -118,36 +118,22 @@ virtio_user_queue_setup(struct virtio_user_dev *dev,
 int
 virtio_user_start_device(struct virtio_user_dev *dev)
 {
-	uint64_t features;
 	int ret;
 
-	/* Step 0: tell vhost to create queues */
+	/* tell vhost to create queues */
 	if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
-		goto error;
+		return -1;
 
-	/* Step 1: set features */
+	/* share memory regions */
-	features = dev->features;
-
-	printf("features = %jx\n", features);
-
-	ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
-	if (ret < 0)
-		goto error;
-	PMD_DRV_LOG(INFO, "set features: %" PRIx64, features);
-
-	/* Step 2: share memory regions */
 	ret = dev->ops->send_request(dev, VHOST_USER_SET_MEM_TABLE, NULL);
 	if (ret < 0)
-		goto error;
+		return -1;
 
-	/* Step 3: kick queues */
+	/* kick queues */
 	if (virtio_user_queue_setup(dev, virtio_user_kick_queue) < 0)
-		goto error;
+		return -1;
 
 	return 0;
-error:
-	/* TODO: free resource here or caller to check */
-	return -1;
 }
 
 int virtio_user_stop_device(struct virtio_user_dev *dev)
@@ -229,12 +215,6 @@ virtio_user_dev_init(char *path, int queue_size)
 		goto err;
 	}
 
-	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
-			    &dev->device_features) < 0) {
-		PMD_INIT_LOG(ERR, "get_features failed: %s", strerror(errno));
-		goto err;
-	}
-
 	TAILQ_INSERT_TAIL(&g_virtio_driver.init_ctrlrs, vdev, tailq);
 	return vdev;
 

lib/bdev/virtio/rte_virtio/virtio_user/virtio_user_dev.h

@@ -58,10 +58,7 @@ struct virtio_user_dev {
 	int		callfds[VIRTIO_MAX_VIRTQUEUES];
 	int		kickfds[VIRTIO_MAX_VIRTQUEUES];
 	uint32_t	queue_size;
-	uint64_t	features; /* the negotiated features with driver,
+
-				   * and will be sync with device
-				   */
-	uint64_t	device_features; /* supported features by device */
 	uint8_t		status;
 	uint8_t		port_id;
 	char		path[PATH_MAX];