[PATCH v4 01/42] virtio: use u32, not bitmap for struct virtio_device's features

[PATCH v4 01/42] virtio: use u32, not bitmap for struct virtio_device's features

[Michael S. Tsirkin]

From: Rusty Russell <rusty@rustcorp.com.au>

It seemed like a good idea, but it's actually a pain when we get more
than 32 feature bits.  Just change it to a u32 for now.

Cc: Brian Swetland <swetland@google.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Acked-by: Pawel Moll <pawel.moll@arm.com>
Acked-by: Ohad Ben-Cohen <ohad@wizery.com>

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/virtio.h                 |  3 +--
 include/linux/virtio_config.h          |  2 +-
 tools/virtio/linux/virtio.h            | 22 +---------------------
 tools/virtio/linux/virtio_config.h     |  2 +-
 drivers/char/virtio_console.c          |  2 +-
 drivers/lguest/lguest_device.c         |  8 ++++----
 drivers/misc/mic/card/mic_virtio.c     |  2 +-
 drivers/remoteproc/remoteproc_virtio.c |  2 +-
 drivers/s390/kvm/kvm_virtio.c          |  2 +-
 drivers/s390/kvm/virtio_ccw.c          | 23 +++++++++--------------
 drivers/virtio/virtio.c                | 10 +++++-----
 drivers/virtio/virtio_mmio.c           |  8 ++------
 drivers/virtio/virtio_pci.c            |  3 +--
 drivers/virtio/virtio_ring.c           |  2 +-
 tools/virtio/virtio_test.c             |  5 ++---
 tools/virtio/vringh_test.c             | 16 ++++++++--------
 16 files changed, 40 insertions(+), 72 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 65261a7..7828a7f 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -101,8 +101,7 @@ struct virtio_device {
 	const struct virtio_config_ops *config;
 	const struct vringh_config_ops *vringh_config;
 	struct list_head vqs;
-	/* Note that this is a Linux set_bit-style bitmap. */
-	unsigned long features[1];
+	u32 features;
 	void *priv;
 };
 
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 7f4ef66..aa84d0e 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -93,7 +93,7 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev,
 	if (fbit < VIRTIO_TRANSPORT_F_START)
 		virtio_check_driver_offered_feature(vdev, fbit);
 
-	return test_bit(fbit, vdev->features);
+	return vdev->features & (1 << fbit);
 }
 
 static inline
diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h
index 5a2d1f0..72bff70 100644
--- a/tools/virtio/linux/virtio.h
+++ b/tools/virtio/linux/virtio.h
@@ -6,31 +6,11 @@
 /* TODO: empty stubs for now. Broken but enough for virtio_ring.c */
 #define list_add_tail(a, b) do {} while (0)
 #define list_del(a) do {} while (0)
-
-#define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
-#define BITS_PER_BYTE		8
-#define BITS_PER_LONG (sizeof(long) * BITS_PER_BYTE)
-#define BIT_MASK(nr)		(1UL << ((nr) % BITS_PER_LONG))
-
-/* TODO: Not atomic as it should be:
- * we don't use this for anything important. */
-static inline void clear_bit(int nr, volatile unsigned long *addr)
-{
-	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
-
-	*p &= ~mask;
-}
-
-static inline int test_bit(int nr, const volatile unsigned long *addr)
-{
-        return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
-}
 /* end of stubs */
 
 struct virtio_device {
 	void *dev;
-	unsigned long features[1];
+	u32 features;
 };
 
 struct virtqueue {
diff --git a/tools/virtio/linux/virtio_config.h b/tools/virtio/linux/virtio_config.h
index 5049967..1f1636b 100644
--- a/tools/virtio/linux/virtio_config.h
+++ b/tools/virtio/linux/virtio_config.h
@@ -2,5 +2,5 @@
 #define VIRTIO_TRANSPORT_F_END		32
 
 #define virtio_has_feature(dev, feature) \
-	test_bit((feature), (dev)->features)
+	((dev)->features & (1 << feature))
 
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cf7a561..0074f9b 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -355,7 +355,7 @@ static inline bool use_multiport(struct ports_device *portdev)
 	 */
 	if (!portdev->vdev)
 		return 0;
-	return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+	return portdev->vdev->features & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
 static DEFINE_SPINLOCK(dma_bufs_lock);
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index d0a1d8a..c831c47 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -137,14 +137,14 @@ static void lg_finalize_features(struct virtio_device *vdev)
 	vring_transport_features(vdev);
 
 	/*
-	 * The vdev->feature array is a Linux bitmask: this isn't the same as a
-	 * the simple array of bits used by lguest devices for features.  So we
-	 * do this slow, manual conversion which is completely general.
+	 * Since lguest is currently x86-only, we're little-endian.  That
+	 * means we could just memcpy.  But it's not time critical, and in
+	 * case someone copies this code, we do it the slow, obvious way.
 	 */
 	memset(out_features, 0, desc->feature_len);
 	bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8;
 	for (i = 0; i < bits; i++) {
-		if (test_bit(i, vdev->features))
+		if (vdev->features & (1 << i))
 			out_features[i / 8] |= (1 << (i % 8));
 	}
 
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index e647947..0acd564 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -101,7 +101,7 @@ static void mic_finalize_features(struct virtio_device *vdev)
 	bits = min_t(unsigned, feature_len,
 		sizeof(vdev->features)) * 8;
 	for (i = 0; i < bits; i++) {
-		if (test_bit(i, vdev->features))
+		if (vdev->features & BIT(bits))
 			iowrite8(ioread8(&out_features[i / 8]) | (1 << (i % 8)),
 				 &out_features[i / 8]);
 	}
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index a34b506..dafaf38 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -231,7 +231,7 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev)
 	 * Remember the finalized features of our vdev, and provide it
 	 * to the remote processor once it is powered on.
 	 */
-	rsc->gfeatures = vdev->features[0];
+	rsc->gfeatures = vdev->features;
 }
 
 static void rproc_virtio_get(struct virtio_device *vdev, unsigned offset,
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index 6431290..ac79a09 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -106,7 +106,7 @@ static void kvm_finalize_features(struct virtio_device *vdev)
 	memset(out_features, 0, desc->feature_len);
 	bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8;
 	for (i = 0; i < bits; i++) {
-		if (test_bit(i, vdev->features))
+		if (vdev->features & (1 << i))
 			out_features[i / 8] |= (1 << (i % 8));
 	}
 }
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index bda52f1..1dbee95 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -701,7 +701,6 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
 {
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
 	struct virtio_feature_desc *features;
-	int i;
 	struct ccw1 *ccw;
 
 	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
@@ -715,19 +714,15 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
 	/* Give virtio_ring a chance to accept features. */
 	vring_transport_features(vdev);
 
-	for (i = 0; i < sizeof(*vdev->features) / sizeof(features->features);
-	     i++) {
-		int highbits = i % 2 ? 32 : 0;
-		features->index = i;
-		features->features = cpu_to_le32(vdev->features[i / 2]
-						 >> highbits);
-		/* Write the feature bits to the host. */
-		ccw->cmd_code = CCW_CMD_WRITE_FEAT;
-		ccw->flags = 0;
-		ccw->count = sizeof(*features);
-		ccw->cda = (__u32)(unsigned long)features;
-		ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
-	}
+	features->index = 0;
+	features->features = cpu_to_le32(vdev->features);
+	/* Write the feature bits to the host. */
+	ccw->cmd_code = CCW_CMD_WRITE_FEAT;
+	ccw->flags = 0;
+	ccw->count = sizeof(*features);
+	ccw->cda = (__u32)(unsigned long)features;
+	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
+
 out_free:
 	kfree(features);
 	kfree(ccw);
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index df598dd..7814b1f 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -49,9 +49,9 @@ static ssize_t features_show(struct device *_d,
 
 	/* We actually represent this as a bitstring, as it could be
 	 * arbitrary length in future. */
-	for (i = 0; i < ARRAY_SIZE(dev->features)*BITS_PER_LONG; i++)
+	for (i = 0; i < sizeof(dev->features)*8; i++)
 		len += sprintf(buf+len, "%c",
-			       test_bit(i, dev->features) ? '1' : '0');
+			       dev->features & (1ULL << i) ? '1' : '0');
 	len += sprintf(buf+len, "\n");
 	return len;
 }
@@ -168,18 +168,18 @@ static int virtio_dev_probe(struct device *_d)
 	device_features = dev->config->get_features(dev);
 
 	/* Features supported by both device and driver into dev->features. */
-	memset(dev->features, 0, sizeof(dev->features));
+	dev->features = 0;
 	for (i = 0; i < drv->feature_table_size; i++) {
 		unsigned int f = drv->feature_table[i];
 		BUG_ON(f >= 32);
 		if (device_features & (1 << f))
-			set_bit(f, dev->features);
+			dev->features |= (1 << f);
 	}
 
 	/* Transport features always preserved to pass to finalize_features. */
 	for (i = VIRTIO_TRANSPORT_F_START; i < VIRTIO_TRANSPORT_F_END; i++)
 		if (device_features & (1 << i))
-			set_bit(i, dev->features);
+			dev->features |= (1 << i);
 
 	dev->config->finalize_features(dev);
 
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index ef9a165..eb5b0e2 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -155,16 +155,12 @@ static u32 vm_get_features(struct virtio_device *vdev)
 static void vm_finalize_features(struct virtio_device *vdev)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
-	int i;
 
 	/* Give virtio_ring a chance to accept features. */
 	vring_transport_features(vdev);
 
-	for (i = 0; i < ARRAY_SIZE(vdev->features); i++) {
-		writel(i, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
-		writel(vdev->features[i],
-				vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
-	}
+	writel(0, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
+	writel(vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
 }
 
 static void vm_get(struct virtio_device *vdev, unsigned offset,
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index d34ebfa..ab95a4c 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -120,8 +120,7 @@ static void vp_finalize_features(struct virtio_device *vdev)
 	vring_transport_features(vdev);
 
 	/* We only support 32 feature bits. */
-	BUILD_BUG_ON(ARRAY_SIZE(vdev->features) != 1);
-	iowrite32(vdev->features[0], vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES);
+	iowrite32(vdev->features, vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES);
 }
 
 /* virtio config->get() implementation */
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 3b1f89b..15a8a05 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -781,7 +781,7 @@ void vring_transport_features(struct virtio_device *vdev)
 			break;
 		default:
 			/* We don't understand this bit. */
-			clear_bit(i, vdev->features);
+			vdev->features &= ~(1 << i);
 		}
 	}
 }
diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
index 00ea679..db3437c 100644
--- a/tools/virtio/virtio_test.c
+++ b/tools/virtio/virtio_test.c
@@ -60,7 +60,7 @@ void vhost_vq_setup(struct vdev_info *dev, struct vq_info *info)
 {
 	struct vhost_vring_state state = { .index = info->idx };
 	struct vhost_vring_file file = { .index = info->idx };
-	unsigned long long features = dev->vdev.features[0];
+	unsigned long long features = dev->vdev.features;
 	struct vhost_vring_addr addr = {
 		.index = info->idx,
 		.desc_user_addr = (uint64_t)(unsigned long)info->vring.desc,
@@ -113,8 +113,7 @@ static void vdev_info_init(struct vdev_info* dev, unsigned long long features)
 {
 	int r;
 	memset(dev, 0, sizeof *dev);
-	dev->vdev.features[0] = features;
-	dev->vdev.features[1] = features >> 32;
+	dev->vdev.features = features;
 	dev->buf_size = 1024;
 	dev->buf = malloc(dev->buf_size);
 	assert(dev->buf);
diff --git a/tools/virtio/vringh_test.c b/tools/virtio/vringh_test.c
index 14a4f4c..b6c9ee3 100644
--- a/tools/virtio/vringh_test.c
+++ b/tools/virtio/vringh_test.c
@@ -304,7 +304,7 @@ static int parallel_test(unsigned long features,
 		close(to_guest[1]);
 		close(to_host[0]);
 
-		gvdev.vdev.features[0] = features;
+		gvdev.vdev.features = features;
 		gvdev.to_host_fd = to_host[1];
 		gvdev.notifies = 0;
 
@@ -449,13 +449,13 @@ int main(int argc, char *argv[])
 	bool fast_vringh = false, parallel = false;
 
 	getrange = getrange_iov;
-	vdev.features[0] = 0;
+	vdev.features = 0;
 
 	while (argv[1]) {
 		if (strcmp(argv[1], "--indirect") == 0)
-			vdev.features[0] |= (1 << VIRTIO_RING_F_INDIRECT_DESC);
+			vdev.features |= (1 << VIRTIO_RING_F_INDIRECT_DESC);
 		else if (strcmp(argv[1], "--eventidx") == 0)
-			vdev.features[0] |= (1 << VIRTIO_RING_F_EVENT_IDX);
+			vdev.features |= (1 << VIRTIO_RING_F_EVENT_IDX);
 		else if (strcmp(argv[1], "--slow-range") == 0)
 			getrange = getrange_slow;
 		else if (strcmp(argv[1], "--fast-vringh") == 0)
@@ -468,7 +468,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (parallel)
-		return parallel_test(vdev.features[0], getrange, fast_vringh);
+		return parallel_test(vdev.features, getrange, fast_vringh);
 
 	if (posix_memalign(&__user_addr_min, PAGE_SIZE, USER_MEM) != 0)
 		abort();
@@ -483,7 +483,7 @@ int main(int argc, char *argv[])
 
 	/* Set up host side. */
 	vring_init(&vrh.vring, RINGSIZE, __user_addr_min, ALIGN);
-	vringh_init_user(&vrh, vdev.features[0], RINGSIZE, true,
+	vringh_init_user(&vrh, vdev.features, RINGSIZE, true,
 			 vrh.vring.desc, vrh.vring.avail, vrh.vring.used);
 
 	/* No descriptor to get yet... */
@@ -652,13 +652,13 @@ int main(int argc, char *argv[])
 	}
 
 	/* Test weird (but legal!) indirect. */
-	if (vdev.features[0] & (1 << VIRTIO_RING_F_INDIRECT_DESC)) {
+	if (vdev.features & (1 << VIRTIO_RING_F_INDIRECT_DESC)) {
 		char *data = __user_addr_max - USER_MEM/4;
 		struct vring_desc *d = __user_addr_max - USER_MEM/2;
 		struct vring vring;
 
 		/* Force creation of direct, which we modify. */
-		vdev.features[0] &= ~(1 << VIRTIO_RING_F_INDIRECT_DESC);
+		vdev.features &= ~(1 << VIRTIO_RING_F_INDIRECT_DESC);
 		vq = vring_new_virtqueue(0, RINGSIZE, ALIGN, &vdev, true,
 					 __user_addr_min,
 					 never_notify_host,
-- 
MST

[PATCH v4 02/42] virtio: add support for 64 bit features.

[Michael S. Tsirkin]

From: Rusty Russell <rusty@rustcorp.com.au>

Change the u32 to a u64, and make sure to use 1ULL everywhere!

Cc: Brian Swetland <swetland@google.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
[Thomas Huth: fix up virtio-ccw get_features]
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Acked-by: Pawel Moll <pawel.moll@arm.com>
Acked-by: Ohad Ben-Cohen <ohad@wizery.com>

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/virtio.h                 |  2 +-
 include/linux/virtio_config.h          |  8 ++++----
 tools/virtio/linux/virtio.h            |  2 +-
 tools/virtio/linux/virtio_config.h     |  2 +-
 drivers/char/virtio_console.c          |  2 +-
 drivers/lguest/lguest_device.c         | 10 +++++-----
 drivers/misc/mic/card/mic_virtio.c     | 10 +++++-----
 drivers/remoteproc/remoteproc_virtio.c |  5 ++++-
 drivers/s390/kvm/kvm_virtio.c          | 10 +++++-----
 drivers/s390/kvm/virtio_ccw.c          | 29 ++++++++++++++++++++++++-----
 drivers/virtio/virtio.c                | 12 ++++++------
 drivers/virtio/virtio_mmio.c           | 14 +++++++++-----
 drivers/virtio/virtio_pci.c            |  5 ++---
 drivers/virtio/virtio_ring.c           |  2 +-
 14 files changed, 69 insertions(+), 44 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 7828a7f..149284e 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -101,7 +101,7 @@ struct virtio_device {
 	const struct virtio_config_ops *config;
 	const struct vringh_config_ops *vringh_config;
 	struct list_head vqs;
-	u32 features;
+	u64 features;
 	void *priv;
 };
 
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index aa84d0e..022d904 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -66,7 +66,7 @@ struct virtio_config_ops {
 			vq_callback_t *callbacks[],
 			const char *names[]);
 	void (*del_vqs)(struct virtio_device *);
-	u32 (*get_features)(struct virtio_device *vdev);
+	u64 (*get_features)(struct virtio_device *vdev);
 	void (*finalize_features)(struct virtio_device *vdev);
 	const char *(*bus_name)(struct virtio_device *vdev);
 	int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
@@ -86,14 +86,14 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev,
 {
 	/* Did you forget to fix assumptions on max features? */
 	if (__builtin_constant_p(fbit))
-		BUILD_BUG_ON(fbit >= 32);
+		BUILD_BUG_ON(fbit >= 64);
 	else
-		BUG_ON(fbit >= 32);
+		BUG_ON(fbit >= 64);
 
 	if (fbit < VIRTIO_TRANSPORT_F_START)
 		virtio_check_driver_offered_feature(vdev, fbit);
 
-	return vdev->features & (1 << fbit);
+	return vdev->features & (1ULL << fbit);
 }
 
 static inline
diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h
index 72bff70..8eb6421 100644
--- a/tools/virtio/linux/virtio.h
+++ b/tools/virtio/linux/virtio.h
@@ -10,7 +10,7 @@
 
 struct virtio_device {
 	void *dev;
-	u32 features;
+	u64 features;
 };
 
 struct virtqueue {
diff --git a/tools/virtio/linux/virtio_config.h b/tools/virtio/linux/virtio_config.h
index 1f1636b..a254c2b 100644
--- a/tools/virtio/linux/virtio_config.h
+++ b/tools/virtio/linux/virtio_config.h
@@ -2,5 +2,5 @@
 #define VIRTIO_TRANSPORT_F_END		32
 
 #define virtio_has_feature(dev, feature) \
-	((dev)->features & (1 << feature))
+	((dev)->features & (1ULL << feature))
 
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 0074f9b..fda9a75 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -355,7 +355,7 @@ static inline bool use_multiport(struct ports_device *portdev)
 	 */
 	if (!portdev->vdev)
 		return 0;
-	return portdev->vdev->features & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+	return portdev->vdev->features & (1ULL << VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
 static DEFINE_SPINLOCK(dma_bufs_lock);
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index c831c47..4d29bcd 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -94,17 +94,17 @@ static unsigned desc_size(const struct lguest_device_desc *desc)
 }
 
 /* This gets the device's feature bits. */
-static u32 lg_get_features(struct virtio_device *vdev)
+static u64 lg_get_features(struct virtio_device *vdev)
 {
 	unsigned int i;
-	u32 features = 0;
+	u64 features = 0;
 	struct lguest_device_desc *desc = to_lgdev(vdev)->desc;
 	u8 *in_features = lg_features(desc);
 
 	/* We do this the slow but generic way. */
-	for (i = 0; i < min(desc->feature_len * 8, 32); i++)
+	for (i = 0; i < min(desc->feature_len * 8, 64); i++)
 		if (in_features[i / 8] & (1 << (i % 8)))
-			features |= (1 << i);
+			features |= (1ULL << i);
 
 	return features;
 }
@@ -144,7 +144,7 @@ static void lg_finalize_features(struct virtio_device *vdev)
 	memset(out_features, 0, desc->feature_len);
 	bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8;
 	for (i = 0; i < bits; i++) {
-		if (vdev->features & (1 << i))
+		if (vdev->features & (1ULL << i))
 			out_features[i / 8] |= (1 << (i % 8));
 	}
 
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index 0acd564..6d94f04 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -68,10 +68,10 @@ static inline struct device *mic_dev(struct mic_vdev *mvdev)
 }
 
 /* This gets the device's feature bits. */
-static u32 mic_get_features(struct virtio_device *vdev)
+static u64 mic_get_features(struct virtio_device *vdev)
 {
 	unsigned int i, bits;
-	u32 features = 0;
+	u64 features = 0;
 	struct mic_device_desc __iomem *desc = to_micvdev(vdev)->desc;
 	u8 __iomem *in_features = mic_vq_features(desc);
 	int feature_len = ioread8(&desc->feature_len);
@@ -79,8 +79,8 @@ static u32 mic_get_features(struct virtio_device *vdev)
 	bits = min_t(unsigned, feature_len,
 		sizeof(vdev->features)) * 8;
 	for (i = 0; i < bits; i++)
-		if (ioread8(&in_features[i / 8]) & (BIT(i % 8)))
-			features |= BIT(i);
+		if (ioread8(&in_features[i / 8]) & (BIT_ULL(i % 8)))
+			features |= BIT_ULL(i);
 
 	return features;
 }
@@ -101,7 +101,7 @@ static void mic_finalize_features(struct virtio_device *vdev)
 	bits = min_t(unsigned, feature_len,
 		sizeof(vdev->features)) * 8;
 	for (i = 0; i < bits; i++) {
-		if (vdev->features & BIT(bits))
+		if (vdev->features & BIT_ULL(bits))
 			iowrite8(ioread8(&out_features[i / 8]) | (1 << (i % 8)),
 				 &out_features[i / 8]);
 	}
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index dafaf38..627737e 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -207,7 +207,7 @@ static void rproc_virtio_reset(struct virtio_device *vdev)
 }
 
 /* provide the vdev features as retrieved from the firmware */
-static u32 rproc_virtio_get_features(struct virtio_device *vdev)
+static u64 rproc_virtio_get_features(struct virtio_device *vdev)
 {
 	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 	struct fw_rsc_vdev *rsc;
@@ -227,6 +227,9 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev)
 	/* Give virtio_ring a chance to accept features */
 	vring_transport_features(vdev);
 
+	/* Make sure we don't have any features > 32 bits! */
+	BUG_ON((u32)vdev->features != vdev->features);
+
 	/*
 	 * Remember the finalized features of our vdev, and provide it
 	 * to the remote processor once it is powered on.
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index ac79a09..c78251d 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -80,16 +80,16 @@ static unsigned desc_size(const struct kvm_device_desc *desc)
 }
 
 /* This gets the device's feature bits. */
-static u32 kvm_get_features(struct virtio_device *vdev)
+static u64 kvm_get_features(struct virtio_device *vdev)
 {
 	unsigned int i;
-	u32 features = 0;
+	u64 features = 0;
 	struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
 	u8 *in_features = kvm_vq_features(desc);
 
-	for (i = 0; i < min(desc->feature_len * 8, 32); i++)
+	for (i = 0; i < min(desc->feature_len * 8, 64); i++)
 		if (in_features[i / 8] & (1 << (i % 8)))
-			features |= (1 << i);
+			features |= (1ULL << i);
 	return features;
 }
 
@@ -106,7 +106,7 @@ static void kvm_finalize_features(struct virtio_device *vdev)
 	memset(out_features, 0, desc->feature_len);
 	bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8;
 	for (i = 0; i < bits; i++) {
-		if (vdev->features & (1 << i))
+		if (vdev->features & (1ULL << i))
 			out_features[i / 8] |= (1 << (i % 8));
 	}
 }
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 1dbee95..abba04d 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -660,11 +660,12 @@ static void virtio_ccw_reset(struct virtio_device *vdev)
 	kfree(ccw);
 }
 
-static u32 virtio_ccw_get_features(struct virtio_device *vdev)
+static u64 virtio_ccw_get_features(struct virtio_device *vdev)
 {
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
 	struct virtio_feature_desc *features;
-	int ret, rc;
+	int ret;
+	u64 rc;
 	struct ccw1 *ccw;
 
 	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
@@ -677,7 +678,6 @@ static u32 virtio_ccw_get_features(struct virtio_device *vdev)
 		goto out_free;
 	}
 	/* Read the feature bits from the host. */
-	/* TODO: Features > 32 bits */
 	features->index = 0;
 	ccw->cmd_code = CCW_CMD_READ_FEAT;
 	ccw->flags = 0;
@@ -691,6 +691,16 @@ static u32 virtio_ccw_get_features(struct virtio_device *vdev)
 
 	rc = le32_to_cpu(features->features);
 
+	/* Read second half feature bits from the host. */
+	features->index = 1;
+	ccw->cmd_code = CCW_CMD_READ_FEAT;
+	ccw->flags = 0;
+	ccw->count = sizeof(*features);
+	ccw->cda = (__u32)(unsigned long)features;
+	ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_READ_FEAT);
+	if (ret == 0)
+		rc |= (u64)le32_to_cpu(features->features) << 32;
+
 out_free:
 	kfree(features);
 	kfree(ccw);
@@ -715,8 +725,17 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
 	vring_transport_features(vdev);
 
 	features->index = 0;
-	features->features = cpu_to_le32(vdev->features);
-	/* Write the feature bits to the host. */
+	features->features = cpu_to_le32((u32)vdev->features);
+	/* Write the first half of the feature bits to the host. */
+	ccw->cmd_code = CCW_CMD_WRITE_FEAT;
+	ccw->flags = 0;
+	ccw->count = sizeof(*features);
+	ccw->cda = (__u32)(unsigned long)features;
+	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
+
+	features->index = 1;
+	features->features = cpu_to_le32(vdev->features >> 32);
+	/* Write the second half of the feature bits to the host. */
 	ccw->cmd_code = CCW_CMD_WRITE_FEAT;
 	ccw->flags = 0;
 	ccw->count = sizeof(*features);
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 7814b1f..d213567 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -159,7 +159,7 @@ static int virtio_dev_probe(struct device *_d)
 	int err, i;
 	struct virtio_device *dev = dev_to_virtio(_d);
 	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
-	u32 device_features;
+	u64 device_features;
 
 	/* We have a driver! */
 	add_status(dev, VIRTIO_CONFIG_S_DRIVER);
@@ -171,15 +171,15 @@ static int virtio_dev_probe(struct device *_d)
 	dev->features = 0;
 	for (i = 0; i < drv->feature_table_size; i++) {
 		unsigned int f = drv->feature_table[i];
-		BUG_ON(f >= 32);
-		if (device_features & (1 << f))
-			dev->features |= (1 << f);
+		BUG_ON(f >= 64);
+		if (device_features & (1ULL << f))
+			dev->features |= (1ULL << f);
 	}
 
 	/* Transport features always preserved to pass to finalize_features. */
 	for (i = VIRTIO_TRANSPORT_F_START; i < VIRTIO_TRANSPORT_F_END; i++)
-		if (device_features & (1 << i))
-			dev->features |= (1 << i);
+		if (device_features & (1ULL << i))
+			dev->features |= (1ULL << i);
 
 	dev->config->finalize_features(dev);
 
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index eb5b0e2..fd01c6d 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -142,14 +142,16 @@ struct virtio_mmio_vq_info {
 
 /* Configuration interface */
 
-static u32 vm_get_features(struct virtio_device *vdev)
+static u64 vm_get_features(struct virtio_device *vdev)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+	u64 features;
 
-	/* TODO: Features > 32 bits */
 	writel(0, vm_dev->base + VIRTIO_MMIO_HOST_FEATURES_SEL);
-
-	return readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
+	features = readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
+	writel(1, vm_dev->base + VIRTIO_MMIO_HOST_FEATURES_SEL);
+	features |= ((u64)readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES) << 32);
+	return features;
 }
 
 static void vm_finalize_features(struct virtio_device *vdev)
@@ -160,7 +162,9 @@ static void vm_finalize_features(struct virtio_device *vdev)
 	vring_transport_features(vdev);
 
 	writel(0, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
-	writel(vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
+	writel((u32)vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
+	writel(1, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
+	writel(vdev->features >> 32, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
 }
 
 static void vm_get(struct virtio_device *vdev, unsigned offset,
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index ab95a4c..68c0711 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -102,12 +102,11 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
 }
 
 /* virtio config->get_features() implementation */
-static u32 vp_get_features(struct virtio_device *vdev)
+static u64 vp_get_features(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 
-	/* When someone needs more than 32 feature bits, we'll need to
-	 * steal a bit to indicate that the rest are somewhere else. */
+	/* We only support 32 feature bits. */
 	return ioread32(vp_dev->ioaddr + VIRTIO_PCI_HOST_FEATURES);
 }
 
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 15a8a05..61a1fe1 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -781,7 +781,7 @@ void vring_transport_features(struct virtio_device *vdev)
 			break;
 		default:
 			/* We don't understand this bit. */
-			vdev->features &= ~(1 << i);
+			vdev->features &= ~(1ULL << i);
 		}
 	}
 }
-- 
MST

Re: [PATCH v4 02/42] virtio: add support for 64 bit features.

[Greg Kurz]

On Tue, 25 Nov 2014 18:41:22 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> From: Rusty Russell <rusty@rustcorp.com.au>
> 
> Change the u32 to a u64, and make sure to use 1ULL everywhere!
> 
> Cc: Brian Swetland <swetland@google.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> [Thomas Huth: fix up virtio-ccw get_features]
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Acked-by: Pawel Moll <pawel.moll@arm.com>
> Acked-by: Ohad Ben-Cohen <ohad@wizery.com>
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/linux/virtio.h                 |  2 +-
>  include/linux/virtio_config.h          |  8 ++++----
>  tools/virtio/linux/virtio.h            |  2 +-
>  tools/virtio/linux/virtio_config.h     |  2 +-
>  drivers/char/virtio_console.c          |  2 +-
>  drivers/lguest/lguest_device.c         | 10 +++++-----
>  drivers/misc/mic/card/mic_virtio.c     | 10 +++++-----
>  drivers/remoteproc/remoteproc_virtio.c |  5 ++++-
>  drivers/s390/kvm/kvm_virtio.c          | 10 +++++-----
>  drivers/s390/kvm/virtio_ccw.c          | 29 ++++++++++++++++++++++++-----
>  drivers/virtio/virtio.c                | 12 ++++++------
>  drivers/virtio/virtio_mmio.c           | 14 +++++++++-----
>  drivers/virtio/virtio_pci.c            |  5 ++---
>  drivers/virtio/virtio_ring.c           |  2 +-
>  14 files changed, 69 insertions(+), 44 deletions(-)
> 
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 7828a7f..149284e 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -101,7 +101,7 @@ struct virtio_device {
>  	const struct virtio_config_ops *config;
>  	const struct vringh_config_ops *vringh_config;
>  	struct list_head vqs;
> -	u32 features;
> +	u64 features;
>  	void *priv;
>  };
> 
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index aa84d0e..022d904 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -66,7 +66,7 @@ struct virtio_config_ops {
>  			vq_callback_t *callbacks[],
>  			const char *names[]);
>  	void (*del_vqs)(struct virtio_device *);
> -	u32 (*get_features)(struct virtio_device *vdev);
> +	u64 (*get_features)(struct virtio_device *vdev);
>  	void (*finalize_features)(struct virtio_device *vdev);
>  	const char *(*bus_name)(struct virtio_device *vdev);
>  	int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
> @@ -86,14 +86,14 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev,
>  {
>  	/* Did you forget to fix assumptions on max features? */
>  	if (__builtin_constant_p(fbit))
> -		BUILD_BUG_ON(fbit >= 32);
> +		BUILD_BUG_ON(fbit >= 64);


While you're here, maybe you could derive the max value from the .features field ?

-               BUILD_BUG_ON(fbit >= 64);
+               BUILD_BUG_ON(fbit >= (sizeof(vdev->features) << 3));

Two other locations below.

>  	else
> -		BUG_ON(fbit >= 32);
> +		BUG_ON(fbit >= 64);
> 

Here...

>  	if (fbit < VIRTIO_TRANSPORT_F_START)
>  		virtio_check_driver_offered_feature(vdev, fbit);
> 
> -	return vdev->features & (1 << fbit);
> +	return vdev->features & (1ULL << fbit);
>  }
> 
>  static inline
> diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h
> index 72bff70..8eb6421 100644
> --- a/tools/virtio/linux/virtio.h
> +++ b/tools/virtio/linux/virtio.h
> @@ -10,7 +10,7 @@
> 
>  struct virtio_device {
>  	void *dev;
> -	u32 features;
> +	u64 features;
>  };
> 
>  struct virtqueue {
> diff --git a/tools/virtio/linux/virtio_config.h b/tools/virtio/linux/virtio_config.h
> index 1f1636b..a254c2b 100644
> --- a/tools/virtio/linux/virtio_config.h
> +++ b/tools/virtio/linux/virtio_config.h
> @@ -2,5 +2,5 @@
>  #define VIRTIO_TRANSPORT_F_END		32
> 
>  #define virtio_has_feature(dev, feature) \
> -	((dev)->features & (1 << feature))
> +	((dev)->features & (1ULL << feature))
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 0074f9b..fda9a75 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -355,7 +355,7 @@ static inline bool use_multiport(struct ports_device *portdev)
>  	 */
>  	if (!portdev->vdev)
>  		return 0;
> -	return portdev->vdev->features & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
> +	return portdev->vdev->features & (1ULL << VIRTIO_CONSOLE_F_MULTIPORT);
>  }
> 
>  static DEFINE_SPINLOCK(dma_bufs_lock);
> diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
> index c831c47..4d29bcd 100644
> --- a/drivers/lguest/lguest_device.c
> +++ b/drivers/lguest/lguest_device.c
> @@ -94,17 +94,17 @@ static unsigned desc_size(const struct lguest_device_desc *desc)
>  }
> 
>  /* This gets the device's feature bits. */
> -static u32 lg_get_features(struct virtio_device *vdev)
> +static u64 lg_get_features(struct virtio_device *vdev)
>  {
>  	unsigned int i;
> -	u32 features = 0;
> +	u64 features = 0;
>  	struct lguest_device_desc *desc = to_lgdev(vdev)->desc;
>  	u8 *in_features = lg_features(desc);
> 
>  	/* We do this the slow but generic way. */
> -	for (i = 0; i < min(desc->feature_len * 8, 32); i++)
> +	for (i = 0; i < min(desc->feature_len * 8, 64); i++)
>  		if (in_features[i / 8] & (1 << (i % 8)))
> -			features |= (1 << i);
> +			features |= (1ULL << i);
> 
>  	return features;
>  }
> @@ -144,7 +144,7 @@ static void lg_finalize_features(struct virtio_device *vdev)
>  	memset(out_features, 0, desc->feature_len);
>  	bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8;
>  	for (i = 0; i < bits; i++) {
> -		if (vdev->features & (1 << i))
> +		if (vdev->features & (1ULL << i))
>  			out_features[i / 8] |= (1 << (i % 8));
>  	}
> 
> diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
> index 0acd564..6d94f04 100644
> --- a/drivers/misc/mic/card/mic_virtio.c
> +++ b/drivers/misc/mic/card/mic_virtio.c
> @@ -68,10 +68,10 @@ static inline struct device *mic_dev(struct mic_vdev *mvdev)
>  }
> 
>  /* This gets the device's feature bits. */
> -static u32 mic_get_features(struct virtio_device *vdev)
> +static u64 mic_get_features(struct virtio_device *vdev)
>  {
>  	unsigned int i, bits;
> -	u32 features = 0;
> +	u64 features = 0;
>  	struct mic_device_desc __iomem *desc = to_micvdev(vdev)->desc;
>  	u8 __iomem *in_features = mic_vq_features(desc);
>  	int feature_len = ioread8(&desc->feature_len);
> @@ -79,8 +79,8 @@ static u32 mic_get_features(struct virtio_device *vdev)
>  	bits = min_t(unsigned, feature_len,
>  		sizeof(vdev->features)) * 8;
>  	for (i = 0; i < bits; i++)
> -		if (ioread8(&in_features[i / 8]) & (BIT(i % 8)))
> -			features |= BIT(i);
> +		if (ioread8(&in_features[i / 8]) & (BIT_ULL(i % 8)))
> +			features |= BIT_ULL(i);
> 
>  	return features;
>  }
> @@ -101,7 +101,7 @@ static void mic_finalize_features(struct virtio_device *vdev)
>  	bits = min_t(unsigned, feature_len,
>  		sizeof(vdev->features)) * 8;
>  	for (i = 0; i < bits; i++) {
> -		if (vdev->features & BIT(bits))
> +		if (vdev->features & BIT_ULL(bits))
>  			iowrite8(ioread8(&out_features[i / 8]) | (1 << (i % 8)),
>  				 &out_features[i / 8]);
>  	}
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index dafaf38..627737e 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -207,7 +207,7 @@ static void rproc_virtio_reset(struct virtio_device *vdev)
>  }
> 
>  /* provide the vdev features as retrieved from the firmware */
> -static u32 rproc_virtio_get_features(struct virtio_device *vdev)
> +static u64 rproc_virtio_get_features(struct virtio_device *vdev)
>  {
>  	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
>  	struct fw_rsc_vdev *rsc;
> @@ -227,6 +227,9 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev)
>  	/* Give virtio_ring a chance to accept features */
>  	vring_transport_features(vdev);
> 
> +	/* Make sure we don't have any features > 32 bits! */
> +	BUG_ON((u32)vdev->features != vdev->features);
> +
>  	/*
>  	 * Remember the finalized features of our vdev, and provide it
>  	 * to the remote processor once it is powered on.
> diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
> index ac79a09..c78251d 100644
> --- a/drivers/s390/kvm/kvm_virtio.c
> +++ b/drivers/s390/kvm/kvm_virtio.c
> @@ -80,16 +80,16 @@ static unsigned desc_size(const struct kvm_device_desc *desc)
>  }
> 
>  /* This gets the device's feature bits. */
> -static u32 kvm_get_features(struct virtio_device *vdev)
> +static u64 kvm_get_features(struct virtio_device *vdev)
>  {
>  	unsigned int i;
> -	u32 features = 0;
> +	u64 features = 0;
>  	struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
>  	u8 *in_features = kvm_vq_features(desc);
> 
> -	for (i = 0; i < min(desc->feature_len * 8, 32); i++)
> +	for (i = 0; i < min(desc->feature_len * 8, 64); i++)
>  		if (in_features[i / 8] & (1 << (i % 8)))
> -			features |= (1 << i);
> +			features |= (1ULL << i);
>  	return features;
>  }
> 
> @@ -106,7 +106,7 @@ static void kvm_finalize_features(struct virtio_device *vdev)
>  	memset(out_features, 0, desc->feature_len);
>  	bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8;
>  	for (i = 0; i < bits; i++) {
> -		if (vdev->features & (1 << i))
> +		if (vdev->features & (1ULL << i))
>  			out_features[i / 8] |= (1 << (i % 8));
>  	}
>  }
> diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> index 1dbee95..abba04d 100644
> --- a/drivers/s390/kvm/virtio_ccw.c
> +++ b/drivers/s390/kvm/virtio_ccw.c
> @@ -660,11 +660,12 @@ static void virtio_ccw_reset(struct virtio_device *vdev)
>  	kfree(ccw);
>  }
> 
> -static u32 virtio_ccw_get_features(struct virtio_device *vdev)
> +static u64 virtio_ccw_get_features(struct virtio_device *vdev)
>  {
>  	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>  	struct virtio_feature_desc *features;
> -	int ret, rc;
> +	int ret;
> +	u64 rc;
>  	struct ccw1 *ccw;
> 
>  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> @@ -677,7 +678,6 @@ static u32 virtio_ccw_get_features(struct virtio_device *vdev)
>  		goto out_free;
>  	}
>  	/* Read the feature bits from the host. */
> -	/* TODO: Features > 32 bits */
>  	features->index = 0;
>  	ccw->cmd_code = CCW_CMD_READ_FEAT;
>  	ccw->flags = 0;
> @@ -691,6 +691,16 @@ static u32 virtio_ccw_get_features(struct virtio_device *vdev)
> 
>  	rc = le32_to_cpu(features->features);
> 
> +	/* Read second half feature bits from the host. */
> +	features->index = 1;
> +	ccw->cmd_code = CCW_CMD_READ_FEAT;
> +	ccw->flags = 0;
> +	ccw->count = sizeof(*features);
> +	ccw->cda = (__u32)(unsigned long)features;
> +	ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_READ_FEAT);
> +	if (ret == 0)
> +		rc |= (u64)le32_to_cpu(features->features) << 32;
> +
>  out_free:
>  	kfree(features);
>  	kfree(ccw);
> @@ -715,8 +725,17 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
>  	vring_transport_features(vdev);
> 
>  	features->index = 0;
> -	features->features = cpu_to_le32(vdev->features);
> -	/* Write the feature bits to the host. */
> +	features->features = cpu_to_le32((u32)vdev->features);
> +	/* Write the first half of the feature bits to the host. */
> +	ccw->cmd_code = CCW_CMD_WRITE_FEAT;
> +	ccw->flags = 0;
> +	ccw->count = sizeof(*features);
> +	ccw->cda = (__u32)(unsigned long)features;
> +	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
> +
> +	features->index = 1;
> +	features->features = cpu_to_le32(vdev->features >> 32);
> +	/* Write the second half of the feature bits to the host. */
>  	ccw->cmd_code = CCW_CMD_WRITE_FEAT;
>  	ccw->flags = 0;
>  	ccw->count = sizeof(*features);
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 7814b1f..d213567 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -159,7 +159,7 @@ static int virtio_dev_probe(struct device *_d)
>  	int err, i;
>  	struct virtio_device *dev = dev_to_virtio(_d);
>  	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> -	u32 device_features;
> +	u64 device_features;
> 
>  	/* We have a driver! */
>  	add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> @@ -171,15 +171,15 @@ static int virtio_dev_probe(struct device *_d)
>  	dev->features = 0;
>  	for (i = 0; i < drv->feature_table_size; i++) {
>  		unsigned int f = drv->feature_table[i];
> -		BUG_ON(f >= 32);
> -		if (device_features & (1 << f))
> -			dev->features |= (1 << f);
> +		BUG_ON(f >= 64);

and here.

> +		if (device_features & (1ULL << f))
> +			dev->features |= (1ULL << f);
>  	}
> 
>  	/* Transport features always preserved to pass to finalize_features. */
>  	for (i = VIRTIO_TRANSPORT_F_START; i < VIRTIO_TRANSPORT_F_END; i++)
> -		if (device_features & (1 << i))
> -			dev->features |= (1 << i);
> +		if (device_features & (1ULL << i))
> +			dev->features |= (1ULL << i);
> 
>  	dev->config->finalize_features(dev);
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index eb5b0e2..fd01c6d 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -142,14 +142,16 @@ struct virtio_mmio_vq_info {
> 
>  /* Configuration interface */
> 
> -static u32 vm_get_features(struct virtio_device *vdev)
> +static u64 vm_get_features(struct virtio_device *vdev)
>  {
>  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> +	u64 features;
> 
> -	/* TODO: Features > 32 bits */
>  	writel(0, vm_dev->base + VIRTIO_MMIO_HOST_FEATURES_SEL);
> -
> -	return readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
> +	features = readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
> +	writel(1, vm_dev->base + VIRTIO_MMIO_HOST_FEATURES_SEL);
> +	features |= ((u64)readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES) << 32);
> +	return features;
>  }
> 
>  static void vm_finalize_features(struct virtio_device *vdev)
> @@ -160,7 +162,9 @@ static void vm_finalize_features(struct virtio_device *vdev)
>  	vring_transport_features(vdev);
> 
>  	writel(0, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
> -	writel(vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
> +	writel((u32)vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
> +	writel(1, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
> +	writel(vdev->features >> 32, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
>  }
> 
>  static void vm_get(struct virtio_device *vdev, unsigned offset,
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index ab95a4c..68c0711 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -102,12 +102,11 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
>  }
> 
>  /* virtio config->get_features() implementation */
> -static u32 vp_get_features(struct virtio_device *vdev)
> +static u64 vp_get_features(struct virtio_device *vdev)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> 
> -	/* When someone needs more than 32 feature bits, we'll need to
> -	 * steal a bit to indicate that the rest are somewhere else. */
> +	/* We only support 32 feature bits. */
>  	return ioread32(vp_dev->ioaddr + VIRTIO_PCI_HOST_FEATURES);
>  }
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 15a8a05..61a1fe1 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -781,7 +781,7 @@ void vring_transport_features(struct virtio_device *vdev)
>  			break;
>  		default:
>  			/* We don't understand this bit. */
> -			vdev->features &= ~(1 << i);
> +			vdev->features &= ~(1ULL << i);
>  		}
>  	}
>  }

Re: [PATCH v4 02/42] virtio: add support for 64 bit features.

[Michael S. Tsirkin]

On Wed, Nov 26, 2014 at 05:48:09PM +0100, Greg Kurz wrote:
> On Tue, 25 Nov 2014 18:41:22 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > From: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > Change the u32 to a u64, and make sure to use 1ULL everywhere!
> > 
> > Cc: Brian Swetland <swetland@google.com>
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > [Thomas Huth: fix up virtio-ccw get_features]
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Acked-by: Pawel Moll <pawel.moll@arm.com>
> > Acked-by: Ohad Ben-Cohen <ohad@wizery.com>
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

...

> > @@ -86,14 +86,14 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev,
> >  {
> >  	/* Did you forget to fix assumptions on max features? */
> >  	if (__builtin_constant_p(fbit))
> > -		BUILD_BUG_ON(fbit >= 32);
> > +		BUILD_BUG_ON(fbit >= 64);
> 
> 
> While you're here, maybe you could derive the max value from the .features field ?
> 
> -               BUILD_BUG_ON(fbit >= 64);
> +               BUILD_BUG_ON(fbit >= (sizeof(vdev->features) << 3));

I don't see how that will help.
All that 1ULL math only works up to 64 bit.
So this only makes it look like we support any size,
but doesn't really.

No?

[PATCH v4 04/42] virtio: disable virtio 1.0 in transports

[Michael S. Tsirkin]

disable virtio 1.0 in transports that don't
support it yet.
We will gradually re-enable as support is added.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/lguest/lguest_device.c     | 3 ++-
 drivers/misc/mic/card/mic_virtio.c | 2 ++
 drivers/s390/kvm/virtio_ccw.c      | 3 ++-
 drivers/virtio/virtio_mmio.c       | 2 ++
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 4d29bcd..4deaf88 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -106,7 +106,8 @@ static u64 lg_get_features(struct virtio_device *vdev)
 		if (in_features[i / 8] & (1 << (i % 8)))
 			features |= (1ULL << i);
 
-	return features;
+	/* lguest is not in virtio 1.0 */
+	return features & ~BIT_ULL(VIRTIO_F_VERSION_1);
 }
 
 /*
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index 6d94f04..edc77f1 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -82,6 +82,8 @@ static u64 mic_get_features(struct virtio_device *vdev)
 		if (ioread8(&in_features[i / 8]) & (BIT_ULL(i % 8)))
 			features |= BIT_ULL(i);
 
+	/* MIC is not in virtio 1.0, disable it for now. */
+	features &= ~BIT_ULL(VIRTIO_F_VERSION_1);
 	return features;
 }
 
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index abba04d..08536f0 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -704,7 +704,8 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev)
 out_free:
 	kfree(features);
 	kfree(ccw);
-	return rc;
+	/* TODO: enable virtio 1.0 */
+	return rc & ~BIT_ULL(VIRTIO_F_VERSION_1);;
 }
 
 static void virtio_ccw_finalize_features(struct virtio_device *vdev)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index fd01c6d..e1d38a9 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -151,6 +151,8 @@ static u64 vm_get_features(struct virtio_device *vdev)
 	features = readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
 	writel(1, vm_dev->base + VIRTIO_MMIO_HOST_FEATURES_SEL);
 	features |= ((u64)readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES) << 32);
+	/* TODO: enable virtio 1.0 support */
+	features &= ~BIT_ULL(VIRTIO_F_VERSION_1);
 	return features;
 }
 
-- 
MST

Re: [PATCH v4 04/42] virtio: disable virtio 1.0 in transports

[Cornelia Huck]

On Tue, 25 Nov 2014 18:41:35 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> disable virtio 1.0 in transports that don't
> support it yet.

I'd prefer if you disabled it for _every_ transport in this patch,
until the needed infrastructure is in place. Else this is a bit
confusing.

> We will gradually re-enable as support is added.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/lguest/lguest_device.c     | 3 ++-
>  drivers/misc/mic/card/mic_virtio.c | 2 ++
>  drivers/s390/kvm/virtio_ccw.c      | 3 ++-
>  drivers/virtio/virtio_mmio.c       | 2 ++
>  4 files changed, 8 insertions(+), 2 deletions(-)

Why do you disable ccw but not pci? (Doesn't pci need any changes
transport-side?) And you missed the old s390 virtio transport in
drivers/s390/kvm/kvm_virtio.c :)

> diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> index abba04d..08536f0 100644
> --- a/drivers/s390/kvm/virtio_ccw.c
> +++ b/drivers/s390/kvm/virtio_ccw.c
> @@ -704,7 +704,8 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev)
>  out_free:
>  	kfree(features);
>  	kfree(ccw);
> -	return rc;
> +	/* TODO: enable virtio 1.0 */
> +	return rc & ~BIT_ULL(VIRTIO_F_VERSION_1);;

double ';'

FWIW, as negotiating a revision >= 1 is a pre-req for virtio 1.0
support on ccw, virtio 1.0 is already implicitly disabled.

>  }
> 
>  static void virtio_ccw_finalize_features(struct virtio_device *vdev)

Re: [PATCH v4 04/42] virtio: disable virtio 1.0 in transports

[Michael S. Tsirkin]

On Tue, Nov 25, 2014 at 06:29:42PM +0100, Cornelia Huck wrote:
> On Tue, 25 Nov 2014 18:41:35 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > disable virtio 1.0 in transports that don't
> > support it yet.
> 
> I'd prefer if you disabled it for _every_ transport in this patch,
> until the needed infrastructure is in place. Else this is a bit
> confusing.

Well the only transports left are pci and rpoc, and these only
read the low 32 bit of the features from the device -
so there's nothing to clear.

E.g. the following would be even more confusing, would it not:

	u32 features;
	....
	features &= ~BIT_ULL(VIRTIO_F_VERSION_1);

Agree?

> > We will gradually re-enable as support is added.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/lguest/lguest_device.c     | 3 ++-
> >  drivers/misc/mic/card/mic_virtio.c | 2 ++
> >  drivers/s390/kvm/virtio_ccw.c      | 3 ++-
> >  drivers/virtio/virtio_mmio.c       | 2 ++
> >  4 files changed, 8 insertions(+), 2 deletions(-)
> 
> Why do you disable ccw but not pci? (Doesn't pci need any changes
> transport-side?)

No because the register is 32 bit wide there.

> And you missed the old s390 virtio transport in
> drivers/s390/kvm/kvm_virtio.c :)

Good catch. I can fix that for v5, but see below.

> > diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> > index abba04d..08536f0 100644
> > --- a/drivers/s390/kvm/virtio_ccw.c
> > +++ b/drivers/s390/kvm/virtio_ccw.c
> > @@ -704,7 +704,8 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev)
> >  out_free:
> >  	kfree(features);
> >  	kfree(ccw);
> > -	return rc;
> > +	/* TODO: enable virtio 1.0 */
> > +	return rc & ~BIT_ULL(VIRTIO_F_VERSION_1);;
> 
> double ';'
> 
> FWIW, as negotiating a revision >= 1 is a pre-req for virtio 1.0
> support on ccw, virtio 1.0 is already implicitly disabled.

Ah, you mean device guarantees that VIRTIO_F_VERSION_1 isn't set
if guest sets revision to 0?
In that case it's probably best to drop this from both ccw
devices.

It is here temporarily anyway, only in order to avoid
using transitional devices incorrectly when bisecting.


> >  }
> > 
> >  static void virtio_ccw_finalize_features(struct virtio_device *vdev)

Re: [PATCH v4 04/42] virtio: disable virtio 1.0 in transports

[Cornelia Huck]

On Tue, 25 Nov 2014 23:20:11 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 25, 2014 at 06:29:42PM +0100, Cornelia Huck wrote:
> > On Tue, 25 Nov 2014 18:41:35 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > disable virtio 1.0 in transports that don't
> > > support it yet.
> > 
> > I'd prefer if you disabled it for _every_ transport in this patch,
> > until the needed infrastructure is in place. Else this is a bit
> > confusing.
> 
> Well the only transports left are pci and rpoc, and these only
> read the low 32 bit of the features from the device -
> so there's nothing to clear.
> 
> E.g. the following would be even more confusing, would it not:
> 
> 	u32 features;
> 	....
> 	features &= ~BIT_ULL(VIRTIO_F_VERSION_1);
> 
> Agree?

Maybe you should tweak the patch description a bit and mention that you
only disable virtio 1.0 for transports where it is actually needed?

(...)

> > FWIW, as negotiating a revision >= 1 is a pre-req for virtio 1.0
> > support on ccw, virtio 1.0 is already implicitly disabled.
> 
> Ah, you mean device guarantees that VIRTIO_F_VERSION_1 isn't set
> if guest sets revision to 0?

Yes, the bit will not be offered if the revision is 0 or has not been
set at all.

> In that case it's probably best to drop this from both ccw
> devices.

There's only one ccw transport :)

The old s390 virtio transport in kvm_virtio.c is not part of virtio 1.0.

Re: [PATCH v4 04/42] virtio: disable virtio 1.0 in transports

[Michael S. Tsirkin]

On Wed, Nov 26, 2014 at 10:09:54AM +0100, Cornelia Huck wrote:
> On Tue, 25 Nov 2014 23:20:11 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Nov 25, 2014 at 06:29:42PM +0100, Cornelia Huck wrote:
> > > On Tue, 25 Nov 2014 18:41:35 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > disable virtio 1.0 in transports that don't
> > > > support it yet.
> > > 
> > > I'd prefer if you disabled it for _every_ transport in this patch,
> > > until the needed infrastructure is in place. Else this is a bit
> > > confusing.
> > 
> > Well the only transports left are pci and rpoc, and these only
> > read the low 32 bit of the features from the device -
> > so there's nothing to clear.
> > 
> > E.g. the following would be even more confusing, would it not:
> > 
> > 	u32 features;
> > 	....
> > 	features &= ~BIT_ULL(VIRTIO_F_VERSION_1);
> > 
> > Agree?
> 
> Maybe you should tweak the patch description a bit and mention that you
> only disable virtio 1.0 for transports where it is actually needed?
> 
> (...)

Yes, I did that now.

> > > FWIW, as negotiating a revision >= 1 is a pre-req for virtio 1.0
> > > support on ccw, virtio 1.0 is already implicitly disabled.
> > 
> > Ah, you mean device guarantees that VIRTIO_F_VERSION_1 isn't set
> > if guest sets revision to 0?
> 
> Yes, the bit will not be offered if the revision is 0 or has not been
> set at all.
> 
> > In that case it's probably best to drop this from both ccw
> > devices.
> 
> There's only one ccw transport :)
> 
> The old s390 virtio transport in kvm_virtio.c is not part of virtio 1.0.

It might or might not be a good idea to add code in kvm_virtio.c
blacklisting VIRTIO_F_VERSION_1, just in case there's a buggy device
that sets it.
As correct devices won't set it, I don't think we need to
worry about it too much. We can make it a patch on top later
if we want.

-- 
MST

Re: [PATCH v4 04/42] virtio: disable virtio 1.0 in transports

[Cornelia Huck]

On Thu, 27 Nov 2014 12:54:34 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Nov 26, 2014 at 10:09:54AM +0100, Cornelia Huck wrote:

> > The old s390 virtio transport in kvm_virtio.c is not part of virtio 1.0.
> 
> It might or might not be a good idea to add code in kvm_virtio.c
> blacklisting VIRTIO_F_VERSION_1, just in case there's a buggy device
> that sets it.
> As correct devices won't set it, I don't think we need to
> worry about it too much. We can make it a patch on top later
> if we want.
> 

I'd want to blacklist it, just to make sure nothing weird happens. I
don't want to spend effort on the old transport beyond that :)

Re: [PATCH v4 04/42] virtio: disable virtio 1.0 in transports

[Michael S. Tsirkin]

On Thu, Nov 27, 2014 at 12:02:52PM +0100, Cornelia Huck wrote:
> On Thu, 27 Nov 2014 12:54:34 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Nov 26, 2014 at 10:09:54AM +0100, Cornelia Huck wrote:
> 
> > > The old s390 virtio transport in kvm_virtio.c is not part of virtio 1.0.
> > 
> > It might or might not be a good idea to add code in kvm_virtio.c
> > blacklisting VIRTIO_F_VERSION_1, just in case there's a buggy device
> > that sets it.
> > As correct devices won't set it, I don't think we need to
> > worry about it too much. We can make it a patch on top later
> > if we want.
> > 
> 
> I'd want to blacklist it, just to make sure nothing weird happens. I
> don't want to spend effort on the old transport beyond that :)

I have a better idea. you'll see it in v5.

[PATCH v4 14/42] KVM: s390: Set virtio-ccw transport revision

[Michael S. Tsirkin]

From: Thomas Huth <thuth@linux.vnet.ibm.com>

With the new SET-VIRTIO-REVISION command of the virtio 1.0 standard, we
can now negotiate the virtio-ccw revision after setting a channel online.

Note that we don't negotiate version 1 yet.

[Cornelia Huck: reworked revision loop a bit]
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/s390/kvm/virtio_ccw.c | 63 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 08536f0..68709fa 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -55,6 +55,7 @@ struct virtio_ccw_device {
 	struct ccw_device *cdev;
 	__u32 curr_io;
 	int err;
+	unsigned int revision; /* Transport revision */
 	wait_queue_head_t wait_q;
 	spinlock_t lock;
 	struct list_head virtqueues;
@@ -86,6 +87,15 @@ struct virtio_thinint_area {
 	u8 isc;
 } __packed;
 
+struct virtio_rev_info {
+	__u16 revision;
+	__u16 length;
+	__u8 data[];
+};
+
+/* the highest virtio-ccw revision we support */
+#define VIRTIO_CCW_REV_MAX 0
+
 struct virtio_ccw_vq_info {
 	struct virtqueue *vq;
 	int num;
@@ -122,6 +132,7 @@ static struct airq_info *airq_areas[MAX_AIRQ_AREAS];
 #define CCW_CMD_WRITE_STATUS 0x31
 #define CCW_CMD_READ_VQ_CONF 0x32
 #define CCW_CMD_SET_IND_ADAPTER 0x73
+#define CCW_CMD_SET_VIRTIO_REV 0x83
 
 #define VIRTIO_CCW_DOING_SET_VQ 0x00010000
 #define VIRTIO_CCW_DOING_RESET 0x00040000
@@ -134,6 +145,7 @@ static struct airq_info *airq_areas[MAX_AIRQ_AREAS];
 #define VIRTIO_CCW_DOING_READ_VQ_CONF 0x02000000
 #define VIRTIO_CCW_DOING_SET_CONF_IND 0x04000000
 #define VIRTIO_CCW_DOING_SET_IND_ADAPTER 0x08000000
+#define VIRTIO_CCW_DOING_SET_VIRTIO_REV 0x10000000
 #define VIRTIO_CCW_INTPARM_MASK 0xffff0000
 
 static struct virtio_ccw_device *to_vc_device(struct virtio_device *vdev)
@@ -934,6 +946,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
 		case VIRTIO_CCW_DOING_RESET:
 		case VIRTIO_CCW_DOING_READ_VQ_CONF:
 		case VIRTIO_CCW_DOING_SET_IND_ADAPTER:
+		case VIRTIO_CCW_DOING_SET_VIRTIO_REV:
 			vcdev->curr_io &= ~activity;
 			wake_up(&vcdev->wait_q);
 			break;
@@ -1049,6 +1062,51 @@ static int virtio_ccw_offline(struct ccw_device *cdev)
 	return 0;
 }
 
+static int virtio_ccw_set_transport_rev(struct virtio_ccw_device *vcdev)
+{
+	struct virtio_rev_info *rev;
+	struct ccw1 *ccw;
+	int ret;
+
+	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
+	if (!ccw)
+		return -ENOMEM;
+	rev = kzalloc(sizeof(*rev), GFP_DMA | GFP_KERNEL);
+	if (!rev) {
+		kfree(ccw);
+		return -ENOMEM;
+	}
+
+	/* Set transport revision */
+	ccw->cmd_code = CCW_CMD_SET_VIRTIO_REV;
+	ccw->flags = 0;
+	ccw->count = sizeof(*rev);
+	ccw->cda = (__u32)(unsigned long)rev;
+
+	vcdev->revision = VIRTIO_CCW_REV_MAX;
+	do {
+		rev->revision = vcdev->revision;
+		/* none of our supported revisions carry payload */
+		rev->length = 0;
+		ret = ccw_io_helper(vcdev, ccw,
+				    VIRTIO_CCW_DOING_SET_VIRTIO_REV);
+		if (ret == -EOPNOTSUPP) {
+			if (vcdev->revision == 0)
+				/*
+				 * The host device does not support setting
+				 * the revision: let's operate it in legacy
+				 * mode.
+				 */
+				ret = 0;
+			else
+				vcdev->revision--;
+		}
+	} while (ret == -EOPNOTSUPP);
+
+	kfree(ccw);
+	kfree(rev);
+	return ret;
+}
 
 static int virtio_ccw_online(struct ccw_device *cdev)
 {
@@ -1089,6 +1147,11 @@ static int virtio_ccw_online(struct ccw_device *cdev)
 	spin_unlock_irqrestore(get_ccwdev_lock(cdev), flags);
 	vcdev->vdev.id.vendor = cdev->id.cu_type;
 	vcdev->vdev.id.device = cdev->id.cu_model;
+
+	ret = virtio_ccw_set_transport_rev(vcdev);
+	if (ret)
+		goto out_free;
+
 	ret = register_virtio_device(&vcdev->vdev);
 	if (ret) {
 		dev_warn(&cdev->dev, "Failed to register virtio device: %d\n",
-- 
MST

[PATCH v4 15/42] KVM: s390: virtio-ccw revision 1 SET_VQ

[Michael S. Tsirkin]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

The CCW_CMD_SET_VQ command has a different format for revision 1+
devices, allowing to specify a more complex virtqueue layout. For
now, we stay however with the old layout and simply use the new
command format for virtio-1 devices.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/s390/kvm/virtio_ccw.c | 54 +++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 68709fa..9330065 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -68,13 +68,22 @@ struct virtio_ccw_device {
 	void *airq_info;
 };
 
-struct vq_info_block {
+struct vq_info_block_legacy {
 	__u64 queue;
 	__u32 align;
 	__u16 index;
 	__u16 num;
 } __packed;
 
+struct vq_info_block {
+	__u64 desc;
+	__u32 res0;
+	__u16 index;
+	__u16 num;
+	__u64 avail;
+	__u64 used;
+} __packed;
+
 struct virtio_feature_desc {
 	__u32 features;
 	__u8 index;
@@ -100,7 +109,10 @@ struct virtio_ccw_vq_info {
 	struct virtqueue *vq;
 	int num;
 	void *queue;
-	struct vq_info_block *info_block;
+	union {
+		struct vq_info_block s;
+		struct vq_info_block_legacy l;
+	} *info_block;
 	int bit_nr;
 	struct list_head node;
 	long cookie;
@@ -411,13 +423,22 @@ static void virtio_ccw_del_vq(struct virtqueue *vq, struct ccw1 *ccw)
 	spin_unlock_irqrestore(&vcdev->lock, flags);
 
 	/* Release from host. */
-	info->info_block->queue = 0;
-	info->info_block->align = 0;
-	info->info_block->index = index;
-	info->info_block->num = 0;
+	if (vcdev->revision == 0) {
+		info->info_block->l.queue = 0;
+		info->info_block->l.align = 0;
+		info->info_block->l.index = index;
+		info->info_block->l.num = 0;
+		ccw->count = sizeof(info->info_block->l);
+	} else {
+		info->info_block->s.desc = 0;
+		info->info_block->s.index = index;
+		info->info_block->s.num = 0;
+		info->info_block->s.avail = 0;
+		info->info_block->s.used = 0;
+		ccw->count = sizeof(info->info_block->s);
+	}
 	ccw->cmd_code = CCW_CMD_SET_VQ;
 	ccw->flags = 0;
-	ccw->count = sizeof(*info->info_block);
 	ccw->cda = (__u32)(unsigned long)(info->info_block);
 	ret = ccw_io_helper(vcdev, ccw,
 			    VIRTIO_CCW_DOING_SET_VQ | index);
@@ -500,13 +521,22 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
 	}
 
 	/* Register it with the host. */
-	info->info_block->queue = (__u64)info->queue;
-	info->info_block->align = KVM_VIRTIO_CCW_RING_ALIGN;
-	info->info_block->index = i;
-	info->info_block->num = info->num;
+	if (vcdev->revision == 0) {
+		info->info_block->l.queue = (__u64)info->queue;
+		info->info_block->l.align = KVM_VIRTIO_CCW_RING_ALIGN;
+		info->info_block->l.index = i;
+		info->info_block->l.num = info->num;
+		ccw->count = sizeof(info->info_block->l);
+	} else {
+		info->info_block->s.desc = (__u64)info->queue;
+		info->info_block->s.index = i;
+		info->info_block->s.num = info->num;
+		info->info_block->s.avail = (__u64)virtqueue_get_avail(vq);
+		info->info_block->s.used = (__u64)virtqueue_get_used(vq);
+		ccw->count = sizeof(info->info_block->s);
+	}
 	ccw->cmd_code = CCW_CMD_SET_VQ;
 	ccw->flags = 0;
-	ccw->count = sizeof(*info->info_block);
 	ccw->cda = (__u32)(unsigned long)(info->info_block);
 	err = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_SET_VQ | i);
 	if (err) {
-- 
MST

[PATCH v4 16/42] KVM: s390 allow virtio_ccw status writes to fail

[Michael S. Tsirkin]

Gracefully handle failure to write device status.
We really should handle other errors as well, but this one is needed for
virtio 1.0 compliance.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/s390/kvm/virtio_ccw.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 9330065..50306b2 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -863,7 +863,9 @@ static u8 virtio_ccw_get_status(struct virtio_device *vdev)
 static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status)
 {
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
+	u8 old_status = *vcdev->status;
 	struct ccw1 *ccw;
+	int ret;
 
 	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
 	if (!ccw)
@@ -875,7 +877,10 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status)
 	ccw->flags = 0;
 	ccw->count = sizeof(status);
 	ccw->cda = (__u32)(unsigned long)vcdev->status;
-	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_STATUS);
+	ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_STATUS);
+	/* Write failed? We assume status is unchanged. */
+	if (ret)
+		*vcdev->status = old_status;
 	kfree(ccw);
 }
 
-- 
MST

Re: [PATCH v4 16/42] KVM: s390 allow virtio_ccw status writes to fail

[Cornelia Huck]

On Tue, 25 Nov 2014 18:42:32 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

missing colon after "s390" in subject

> Gracefully handle failure to write device status.
> We really should handle other errors as well, but this one is needed for
> virtio 1.0 compliance.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/s390/kvm/virtio_ccw.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[PATCH v4 17/42] KVM: s390: enable virtio-ccw revision 1

[Michael S. Tsirkin]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

Now that virtio-ccw has everything needed to support virtio 1.0 in
place, try to enable it if the host supports it.

MST: enable virtio 1.0 feature bit

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/s390/kvm/virtio_ccw.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 50306b2..8ec68dd 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -103,7 +103,7 @@ struct virtio_rev_info {
 };
 
 /* the highest virtio-ccw revision we support */
-#define VIRTIO_CCW_REV_MAX 0
+#define VIRTIO_CCW_REV_MAX 1
 
 struct virtio_ccw_vq_info {
 	struct virtqueue *vq;
@@ -746,8 +746,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev)
 out_free:
 	kfree(features);
 	kfree(ccw);
-	/* TODO: enable virtio 1.0 */
-	return rc & ~BIT_ULL(VIRTIO_F_VERSION_1);;
+	return rc;
 }
 
 static void virtio_ccw_finalize_features(struct virtio_device *vdev)
-- 
MST