[PATCH v3 0/6] virtio 1.0 enhancements

[PATCH v3 0/6] virtio 1.0 enhancements

[Michael S. Tsirkin]

These are minor robustness enhancements on top of v8 of the patchset
	[PATCH v8 00/50] linux: towards virtio-1 guest support
http://mid.gmane.org/1417449619-24896-1-git-send-email-mst@redhat.com

As that one seems stable and actually seems to work well for people,
I'm not respinning it anymore.

The main motivation is to prevent us accidentally supporting
bad configurations, such as legacy balloon on top of
virtio 1.0 transport.
The point is that if we happen to support them mistakenly, we'll have trouble
backing out in future.  This will also help make sure hypervisors don't do
silly things.

Changes from v2:
	Check finalize_features return status on resume.
		This shouldn't normally fail, but checking can't hurt.

Michael S. Tsirkin (6):
  virtio: add API to detect legacy devices
  virtio_ccw: legacy: don't negotiate rev 1/features
  virtio: allow finalize_features to fail
  virtio_ccw: rev 1 devices set VIRTIO_F_VERSION_1
  virtio_balloon: drop legacy_only driver flag
  virtio: drop legacy_only driver flag

 include/linux/virtio.h                 |  4 ++--
 include/linux/virtio_config.h          |  3 ++-
 drivers/lguest/lguest_device.c         |  4 +++-
 drivers/misc/mic/card/mic_virtio.c     |  4 +++-
 drivers/remoteproc/remoteproc_virtio.c |  4 +++-
 drivers/s390/kvm/kvm_virtio.c          |  4 +++-
 drivers/s390/kvm/virtio_ccw.c          | 29 ++++++++++++++++++++++++-----
 drivers/virtio/virtio.c                | 32 +++++++++++++++++++++-----------
 drivers/virtio/virtio_balloon.c        |  1 -
 drivers/virtio/virtio_mmio.c           |  4 +++-
 drivers/virtio/virtio_pci.c            |  4 +++-
 11 files changed, 67 insertions(+), 26 deletions(-)

-- 
MST

[PATCH v3 1/6] virtio: add API to detect legacy devices

[Michael S. Tsirkin]

transports need to be able to detect legacy-only
devices (ATM balloon only) to use legacy path
to drive them.

Add a core API to do just that.
The implementation just blacklists balloon:
not too pretty, but let's not over-engineer.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 include/linux/virtio.h  | 2 ++
 drivers/virtio/virtio.c | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 2bbf626..d666bcb 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -108,6 +108,8 @@ struct virtio_device {
 	void *priv;
 };
 
+bool virtio_device_is_legacy_only(struct virtio_device_id id);
+
 static inline struct virtio_device *dev_to_virtio(struct device *_dev)
 {
 	return container_of(_dev, struct virtio_device, dev);
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index fa6b75d..224f854 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -3,6 +3,7 @@
 #include <linux/virtio_config.h>
 #include <linux/module.h>
 #include <linux/idr.h>
+#include <uapi/linux/virtio_ids.h>
 
 /* Unique numbering for virtio devices. */
 static DEFINE_IDA(virtio_index_ida);
@@ -267,6 +268,12 @@ static struct bus_type virtio_bus = {
 	.remove = virtio_dev_remove,
 };
 
+bool virtio_device_is_legacy_only(struct virtio_device_id id)
+{
+	return id.device == VIRTIO_ID_BALLOON;
+}
+EXPORT_SYMBOL_GPL(virtio_device_is_legacy_only);
+
 int register_virtio_driver(struct virtio_driver *driver)
 {
 	/* Catch this early. */
-- 
MST

[PATCH v3 2/6] virtio_ccw: legacy: don't negotiate rev 1/features

[Michael S. Tsirkin]

Legacy balloon device doesn't pretend to support revision 1 or 64 bit
features.

But just in case someone implements a broken one that does, let's not
even try to drive legacy only devices using revision 1, and let's not
give them a chance to say they support VIRTIO_F_VERSION_1 by not reading
high feature bits.

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

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 4a3e6e5..c792b5f 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -733,6 +733,9 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev)
 
 	rc = le32_to_cpu(features->features);
 
+	if (vcdev->revision == 0)
+		goto out_free;
+
 	/* Read second half of the feature bits from the host. */
 	features->index = 1;
 	ccw->cmd_code = CCW_CMD_READ_FEAT;
@@ -775,6 +778,9 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
 	ccw->cda = (__u32)(unsigned long)features;
 	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
 
+	if (vcdev->revision == 0)
+		goto out_free;
+
 	features->index = 1;
 	features->features = cpu_to_le32(vdev->features >> 32);
 	/* Write the second half of the feature bits to the host. */
@@ -1182,9 +1188,13 @@ static int virtio_ccw_online(struct ccw_device *cdev)
 	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;
+	if (virtio_device_is_legacy_only(vcdev->vdev.id)) {
+		vcdev->revision = 0;
+	} else {
+		ret = virtio_ccw_set_transport_rev(vcdev);
+		if (ret)
+			goto out_free;
+	}
 
 	ret = register_virtio_device(&vcdev->vdev);
 	if (ret) {
-- 
MST

Re: [PATCH v3 2/6] virtio_ccw: legacy: don't negotiate rev 1/features

[Cornelia Huck]

On Mon, 8 Dec 2014 15:05:54 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Legacy balloon device doesn't pretend to support revision 1 or 64 bit
> features.
> 
> But just in case someone implements a broken one that does, let's not
> even try to drive legacy only devices using revision 1, and let's not
> give them a chance to say they support VIRTIO_F_VERSION_1 by not reading

s/reading/reading or writing/

> high feature bits.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/s390/kvm/virtio_ccw.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)

Otherwise, looks good.

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

[PATCH v3 3/6] virtio: allow finalize_features to fail

[Michael S. Tsirkin]

This will make it easy for transports to validate features and return
failure.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/virtio_config.h          |  3 ++-
 drivers/lguest/lguest_device.c         |  4 +++-
 drivers/misc/mic/card/mic_virtio.c     |  4 +++-
 drivers/remoteproc/remoteproc_virtio.c |  4 +++-
 drivers/s390/kvm/kvm_virtio.c          |  4 +++-
 drivers/s390/kvm/virtio_ccw.c          |  6 ++++--
 drivers/virtio/virtio.c                | 21 ++++++++++++++-------
 drivers/virtio/virtio_mmio.c           |  4 +++-
 drivers/virtio/virtio_pci.c            |  4 +++-
 9 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 1fa5faa..7979f85 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -47,6 +47,7 @@
  *	vdev: the virtio_device
  *	This gives the final feature bits for the device: it can change
  *	the dev->feature bits if it wants.
+ *	Returns 0 on success or error status
  * @bus_name: return the bus name associated with the device
  *	vdev: the virtio_device
  *      This returns a pointer to the bus name a la pci_name from which
@@ -68,7 +69,7 @@ struct virtio_config_ops {
 			const char *names[]);
 	void (*del_vqs)(struct virtio_device *);
 	u64 (*get_features)(struct virtio_device *vdev);
-	void (*finalize_features)(struct virtio_device *vdev);
+	int (*finalize_features)(struct virtio_device *vdev);
 	const char *(*bus_name)(struct virtio_device *vdev);
 	int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
 };
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 9b77b66..89088d6 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -126,7 +126,7 @@ static void status_notify(struct virtio_device *vdev)
  * sorted out, this routine is called so we can tell the Host which features we
  * understand and accept.
  */
-static void lg_finalize_features(struct virtio_device *vdev)
+static int lg_finalize_features(struct virtio_device *vdev)
 {
 	unsigned int i, bits;
 	struct lguest_device_desc *desc = to_lgdev(vdev)->desc;
@@ -153,6 +153,8 @@ static void lg_finalize_features(struct virtio_device *vdev)
 
 	/* Tell Host we've finished with this device's feature negotiation */
 	status_notify(vdev);
+
+	return 0;
 }
 
 /* Once they've found a field, getting a copy of it is easy. */
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index d027d29..e486a0c 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -84,7 +84,7 @@ static u64 mic_get_features(struct virtio_device *vdev)
 	return features;
 }
 
-static void mic_finalize_features(struct virtio_device *vdev)
+static int mic_finalize_features(struct virtio_device *vdev)
 {
 	unsigned int i, bits;
 	struct mic_device_desc __iomem *desc = to_micvdev(vdev)->desc;
@@ -107,6 +107,8 @@ static void mic_finalize_features(struct virtio_device *vdev)
 			iowrite8(ioread8(&out_features[i / 8]) | (1 << (i % 8)),
 				 &out_features[i / 8]);
 	}
+
+	return 0;
 }
 
 /*
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 627737e..e1a1023 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -217,7 +217,7 @@ static u64 rproc_virtio_get_features(struct virtio_device *vdev)
 	return rsc->dfeatures;
 }
 
-static void rproc_virtio_finalize_features(struct virtio_device *vdev)
+static int rproc_virtio_finalize_features(struct virtio_device *vdev)
 {
 	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 	struct fw_rsc_vdev *rsc;
@@ -235,6 +235,8 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev)
 	 * to the remote processor once it is powered on.
 	 */
 	rsc->gfeatures = vdev->features;
+
+	return 0;
 }
 
 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 f5575cc..dd65c8b 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -93,7 +93,7 @@ static u64 kvm_get_features(struct virtio_device *vdev)
 	return features;
 }
 
-static void kvm_finalize_features(struct virtio_device *vdev)
+static int kvm_finalize_features(struct virtio_device *vdev)
 {
 	unsigned int i, bits;
 	struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
@@ -112,6 +112,8 @@ static void kvm_finalize_features(struct virtio_device *vdev)
 		if (__virtio_test_bit(vdev, i))
 			out_features[i / 8] |= (1 << (i % 8));
 	}
+
+	return 0;
 }
 
 /*
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index c792b5f..789275f 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -752,7 +752,7 @@ out_free:
 	return rc;
 }
 
-static void virtio_ccw_finalize_features(struct virtio_device *vdev)
+static int virtio_ccw_finalize_features(struct virtio_device *vdev)
 {
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
 	struct virtio_feature_desc *features;
@@ -760,7 +760,7 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
 
 	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
 	if (!ccw)
-		return;
+		return 0;
 
 	features = kzalloc(sizeof(*features), GFP_DMA | GFP_KERNEL);
 	if (!features)
@@ -793,6 +793,8 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
 out_free:
 	kfree(features);
 	kfree(ccw);
+
+	return 0;
 }
 
 static void virtio_ccw_get_config(struct virtio_device *vdev,
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 224f854..e1673a5 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -212,7 +212,9 @@ static int virtio_dev_probe(struct device *_d)
 		if (device_features & (1ULL << i))
 			__virtio_set_bit(dev, i);
 
-	dev->config->finalize_features(dev);
+	err = dev->config->finalize_features(dev);
+	if (err)
+		goto err;
 
 	if (virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
 		add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
@@ -354,6 +356,7 @@ EXPORT_SYMBOL_GPL(virtio_device_freeze);
 int virtio_device_restore(struct virtio_device *dev)
 {
 	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
+	int ret;
 
 	/* We always start by resetting the device, in case a previous
 	 * driver messed it up. */
@@ -373,14 +376,14 @@ int virtio_device_restore(struct virtio_device *dev)
 	/* We have a driver! */
 	add_status(dev, VIRTIO_CONFIG_S_DRIVER);
 
-	dev->config->finalize_features(dev);
+	ret = dev->config->finalize_features(dev);
+	if (ret)
+		goto err;
 
 	if (drv->restore) {
-		int ret = drv->restore(dev);
-		if (ret) {
-			add_status(dev, VIRTIO_CONFIG_S_FAILED);
-			return ret;
-		}
+		ret = drv->restore(dev);
+		if (ret)
+			goto err;
 	}
 
 	/* Finally, tell the device we're all set */
@@ -389,6 +392,10 @@ int virtio_device_restore(struct virtio_device *dev)
 	virtio_config_enable(dev);
 
 	return 0;
+
+err:
+	add_status(dev, VIRTIO_CONFIG_S_FAILED);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(virtio_device_restore);
 #endif
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index aec1dae..5219210 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -152,7 +152,7 @@ static u64 vm_get_features(struct virtio_device *vdev)
 	return readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
 }
 
-static void vm_finalize_features(struct virtio_device *vdev)
+static int vm_finalize_features(struct virtio_device *vdev)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
 
@@ -164,6 +164,8 @@ static void vm_finalize_features(struct virtio_device *vdev)
 
 	writel(0, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
 	writel(vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
+
+	return 0;
 }
 
 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 dd6df97..9be59d9 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -112,7 +112,7 @@ static u64 vp_get_features(struct virtio_device *vdev)
 }
 
 /* virtio config->finalize_features() implementation */
-static void vp_finalize_features(struct virtio_device *vdev)
+static int vp_finalize_features(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 
@@ -124,6 +124,8 @@ static void vp_finalize_features(struct virtio_device *vdev)
 
 	/* We only support 32 feature bits. */
 	iowrite32(vdev->features, vp_dev->ioaddr + VIRTIO_PCI_GUEST_FEATURES);
+
+	return 0;
 }
 
 /* virtio config->get() implementation */
-- 
MST

Re: [PATCH v3 3/6] virtio: allow finalize_features to fail

[Cornelia Huck]

On Mon, 8 Dec 2014 15:05:58 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> This will make it easy for transports to validate features and return
> failure.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/linux/virtio_config.h          |  3 ++-
>  drivers/lguest/lguest_device.c         |  4 +++-
>  drivers/misc/mic/card/mic_virtio.c     |  4 +++-
>  drivers/remoteproc/remoteproc_virtio.c |  4 +++-
>  drivers/s390/kvm/kvm_virtio.c          |  4 +++-
>  drivers/s390/kvm/virtio_ccw.c          |  6 ++++--
>  drivers/virtio/virtio.c                | 21 ++++++++++++++-------
>  drivers/virtio/virtio_mmio.c           |  4 +++-
>  drivers/virtio/virtio_pci.c            |  4 +++-
>  9 files changed, 38 insertions(+), 16 deletions(-)
> 

> diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> index c792b5f..789275f 100644
> --- a/drivers/s390/kvm/virtio_ccw.c
> +++ b/drivers/s390/kvm/virtio_ccw.c
> @@ -752,7 +752,7 @@ out_free:
>  	return rc;
>  }
> 
> -static void virtio_ccw_finalize_features(struct virtio_device *vdev)
> +static int virtio_ccw_finalize_features(struct virtio_device *vdev)
>  {
>  	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>  	struct virtio_feature_desc *features;
> @@ -760,7 +760,7 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
> 
>  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
>  	if (!ccw)
> -		return;
> +		return 0;

I think we'll want to return an error in this case as well to fail
probing. Also for the other places where something can go wrong.
Something like this:

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 789275f..bc6e671 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -757,15 +757,17 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
 	struct virtio_feature_desc *features;
 	struct ccw1 *ccw;
+	int ret;
 
 	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
 	if (!ccw)
-		return 0;
+		return -ENOMEM;
 
 	features = kzalloc(sizeof(*features), GFP_DMA | GFP_KERNEL);
-	if (!features)
+	if (!features) {
+		ret = -ENOMEM;
 		goto out_free;
-
+	}
 	/* Give virtio_ring a chance to accept features. */
 	vring_transport_features(vdev);
 
@@ -776,7 +778,9 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
 	ccw->flags = 0;
 	ccw->count = sizeof(*features);
 	ccw->cda = (__u32)(unsigned long)features;
-	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
+	ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
+	if (ret)
+		goto out_free;
 
 	if (vcdev->revision == 0)
 		goto out_free;
@@ -788,13 +792,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
 	ccw->flags = 0;
 	ccw->count = sizeof(*features);
 	ccw->cda = (__u32)(unsigned long)features;
-	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
+	ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
 
 out_free:
 	kfree(features);
 	kfree(ccw);
 
-	return 0;
+	return ret;
 }
 
 static void virtio_ccw_get_config(struct virtio_device *vdev,

Re: [PATCH v3 3/6] virtio: allow finalize_features to fail

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 11:46:59AM +0100, Cornelia Huck wrote:
> On Mon, 8 Dec 2014 15:05:58 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > This will make it easy for transports to validate features and return
> > failure.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/linux/virtio_config.h          |  3 ++-
> >  drivers/lguest/lguest_device.c         |  4 +++-
> >  drivers/misc/mic/card/mic_virtio.c     |  4 +++-
> >  drivers/remoteproc/remoteproc_virtio.c |  4 +++-
> >  drivers/s390/kvm/kvm_virtio.c          |  4 +++-
> >  drivers/s390/kvm/virtio_ccw.c          |  6 ++++--
> >  drivers/virtio/virtio.c                | 21 ++++++++++++++-------
> >  drivers/virtio/virtio_mmio.c           |  4 +++-
> >  drivers/virtio/virtio_pci.c            |  4 +++-
> >  9 files changed, 38 insertions(+), 16 deletions(-)
> > 
> 
> > diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> > index c792b5f..789275f 100644
> > --- a/drivers/s390/kvm/virtio_ccw.c
> > +++ b/drivers/s390/kvm/virtio_ccw.c
> > @@ -752,7 +752,7 @@ out_free:
> >  	return rc;
> >  }
> > 
> > -static void virtio_ccw_finalize_features(struct virtio_device *vdev)
> > +static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> >  {
> >  	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> >  	struct virtio_feature_desc *features;
> > @@ -760,7 +760,7 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
> > 
> >  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> >  	if (!ccw)
> > -		return;
> > +		return 0;
> 
> I think we'll want to return an error in this case as well to fail
> probing. Also for the other places where something can go wrong.
> Something like this:


I guess the fact this previously failed silently is a bug in original
code?

Can you please send your S.O.B so I can add this patch on top?

We might also need to Cc stable.


> diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> index 789275f..bc6e671 100644
> --- a/drivers/s390/kvm/virtio_ccw.c
> +++ b/drivers/s390/kvm/virtio_ccw.c
> @@ -757,15 +757,17 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
>  	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>  	struct virtio_feature_desc *features;
>  	struct ccw1 *ccw;
> +	int ret;
>  
>  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
>  	if (!ccw)
> -		return 0;
> +		return -ENOMEM;
>  
>  	features = kzalloc(sizeof(*features), GFP_DMA | GFP_KERNEL);
> -	if (!features)
> +	if (!features) {
> +		ret = -ENOMEM;
>  		goto out_free;
> -
> +	}
>  	/* Give virtio_ring a chance to accept features. */
>  	vring_transport_features(vdev);
>  
> @@ -776,7 +778,9 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
>  	ccw->flags = 0;
>  	ccw->count = sizeof(*features);
>  	ccw->cda = (__u32)(unsigned long)features;
> -	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
> +	ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
> +	if (ret)
> +		goto out_free;
>  
>  	if (vcdev->revision == 0)
>  		goto out_free;
> @@ -788,13 +792,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
>  	ccw->flags = 0;
>  	ccw->count = sizeof(*features);
>  	ccw->cda = (__u32)(unsigned long)features;
> -	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
> +	ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
>  
>  out_free:
>  	kfree(features);
>  	kfree(ccw);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void virtio_ccw_get_config(struct virtio_device *vdev,

Re: [PATCH v3 3/6] virtio: allow finalize_features to fail

[Cornelia Huck]

On Tue, 9 Dec 2014 14:07:54 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 09, 2014 at 11:46:59AM +0100, Cornelia Huck wrote:
> > On Mon, 8 Dec 2014 15:05:58 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > This will make it easy for transports to validate features and return
> > > failure.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  include/linux/virtio_config.h          |  3 ++-
> > >  drivers/lguest/lguest_device.c         |  4 +++-
> > >  drivers/misc/mic/card/mic_virtio.c     |  4 +++-
> > >  drivers/remoteproc/remoteproc_virtio.c |  4 +++-
> > >  drivers/s390/kvm/kvm_virtio.c          |  4 +++-
> > >  drivers/s390/kvm/virtio_ccw.c          |  6 ++++--
> > >  drivers/virtio/virtio.c                | 21 ++++++++++++++-------
> > >  drivers/virtio/virtio_mmio.c           |  4 +++-
> > >  drivers/virtio/virtio_pci.c            |  4 +++-
> > >  9 files changed, 38 insertions(+), 16 deletions(-)
> > > 
> > 
> > > diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> > > index c792b5f..789275f 100644
> > > --- a/drivers/s390/kvm/virtio_ccw.c
> > > +++ b/drivers/s390/kvm/virtio_ccw.c
> > > @@ -752,7 +752,7 @@ out_free:
> > >  	return rc;
> > >  }
> > > 
> > > -static void virtio_ccw_finalize_features(struct virtio_device *vdev)
> > > +static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> > >  {
> > >  	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> > >  	struct virtio_feature_desc *features;
> > > @@ -760,7 +760,7 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
> > > 
> > >  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> > >  	if (!ccw)
> > > -		return;
> > > +		return 0;
> > 
> > I think we'll want to return an error in this case as well to fail
> > probing. Also for the other places where something can go wrong.
> > Something like this:
> 
> 
> I guess the fact this previously failed silently is a bug in original
> code?

Not really a bug; it simply was not possible to fail it.

> 
> Can you please send your S.O.B so I can add this patch on top?

Sure. Code below is

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

> 
> We might also need to Cc stable.

Not sure whether this is stable-worthy, as these are largely
pathological cases (and for the one non-pathological case, device going
away, we're already fine).

> 
> 
> > diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> > index 789275f..bc6e671 100644
> > --- a/drivers/s390/kvm/virtio_ccw.c
> > +++ b/drivers/s390/kvm/virtio_ccw.c
> > @@ -757,15 +757,17 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> >  	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> >  	struct virtio_feature_desc *features;
> >  	struct ccw1 *ccw;
> > +	int ret;
> >  
> >  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> >  	if (!ccw)
> > -		return 0;
> > +		return -ENOMEM;
> >  
> >  	features = kzalloc(sizeof(*features), GFP_DMA | GFP_KERNEL);
> > -	if (!features)
> > +	if (!features) {
> > +		ret = -ENOMEM;
> >  		goto out_free;
> > -
> > +	}
> >  	/* Give virtio_ring a chance to accept features. */
> >  	vring_transport_features(vdev);
> >  
> > @@ -776,7 +778,9 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> >  	ccw->flags = 0;
> >  	ccw->count = sizeof(*features);
> >  	ccw->cda = (__u32)(unsigned long)features;
> > -	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
> > +	ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
> > +	if (ret)
> > +		goto out_free;
> >  
> >  	if (vcdev->revision == 0)
> >  		goto out_free;
> > @@ -788,13 +792,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> >  	ccw->flags = 0;
> >  	ccw->count = sizeof(*features);
> >  	ccw->cda = (__u32)(unsigned long)features;
> > -	ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
> > +	ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
> >  
> >  out_free:
> >  	kfree(features);
> >  	kfree(ccw);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static void virtio_ccw_get_config(struct virtio_device *vdev,
> 

[PATCH v3 4/6] virtio_ccw: rev 1 devices set VIRTIO_F_VERSION_1

[Michael S. Tsirkin]

What does it mean if rev 1 device does not set
VIRTIO_F_VERSION_1? E.g. is it native endian?

Let's not even try to drive such devices:
fail attempts to finalize features.
virtio core will detect this and bail out.

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

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 789275f..f9f87ba 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -758,6 +758,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
 	struct virtio_feature_desc *features;
 	struct ccw1 *ccw;
 
+	if (vcdev->revision == 1 &&
+	    !__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
+		dev_err(&vdev->dev, "virtio: device uses revision 1 "
+			"but does not have VIRTIO_F_VERSION_1\n");
+		return -EINVAL;
+	}
+
 	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
 	if (!ccw)
 		return 0;
-- 
MST

Re: [PATCH v3 4/6] virtio_ccw: rev 1 devices set VIRTIO_F_VERSION_1

[Cornelia Huck]

On Mon, 8 Dec 2014 15:06:03 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> What does it mean if rev 1 device does not set
> VIRTIO_F_VERSION_1? E.g. is it native endian?

My understanding is that revision only determines the set of channel
commands supported by the device, and their payload. IOW, it just
governs the transport-specific way to communicate; things like
endianness are independent of that and only governed by the VERSION_1
bit which has rev 1 as a pre-req.
> 
> Let's not even try to drive such devices:
> fail attempts to finalize features.
> virtio core will detect this and bail out.

Of course, we can still make the decision to refuse non-VERSION_1
devices if rev 1 has been negotiated, but I'm still not quite sure what
this buys us.

> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/s390/kvm/virtio_ccw.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> index 789275f..f9f87ba 100644
> --- a/drivers/s390/kvm/virtio_ccw.c
> +++ b/drivers/s390/kvm/virtio_ccw.c
> @@ -758,6 +758,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
>  	struct virtio_feature_desc *features;
>  	struct ccw1 *ccw;
> 
> +	if (vcdev->revision == 1 &&

If we decide to keep this check, it should be for rev >= 1, though.

> +	    !__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> +		dev_err(&vdev->dev, "virtio: device uses revision 1 "
> +			"but does not have VIRTIO_F_VERSION_1\n");
> +		return -EINVAL;
> +	}
> +
>  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
>  	if (!ccw)
>  		return 0;

I'm still not convinced by this change: I'd prefer to allow rev 1
without VERSION_1, especially as the core makes all its decisions based
upon VERSION_1. Unless someone else has a good argument in favour of
this change.

Re: [PATCH v3 4/6] virtio_ccw: rev 1 devices set VIRTIO_F_VERSION_1

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 12:01:23PM +0100, Cornelia Huck wrote:
> On Mon, 8 Dec 2014 15:06:03 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > What does it mean if rev 1 device does not set
> > VIRTIO_F_VERSION_1? E.g. is it native endian?
> 
> My understanding is that revision only determines the set of channel
> commands supported by the device, and their payload. IOW, it just
> governs the transport-specific way to communicate; things like
> endianness are independent of that and only governed by the VERSION_1
> bit which has rev 1 as a pre-req.

Well, that's a valid interpretation but it says so nowhere.

virtio 1.0 spec explicitly says VERSION_1 must be set.
virtio legacy explicitly said revision is 0.

And that is all.

We can go ahead and extend spec to support these configurations
in virtio 1.1.

Meanwhile, implementing random behaviour in guests will make
us support it forever.


> > 
> > Let's not even try to drive such devices:
> > fail attempts to finalize features.
> > virtio core will detect this and bail out.
> 
> Of course, we can still make the decision to refuse non-VERSION_1
> devices if rev 1 has been negotiated, but I'm still not quite sure what
> this buys us.

less configurations to support.


> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/s390/kvm/virtio_ccw.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> > index 789275f..f9f87ba 100644
> > --- a/drivers/s390/kvm/virtio_ccw.c
> > +++ b/drivers/s390/kvm/virtio_ccw.c
> > @@ -758,6 +758,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> >  	struct virtio_feature_desc *features;
> >  	struct ccw1 *ccw;
> > 
> > +	if (vcdev->revision == 1 &&
> 
> If we decide to keep this check, it should be for rev >= 1, though.

Fine, though this is theoretical, right?
Ican change this with a patch on top.

> > +	    !__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> > +		dev_err(&vdev->dev, "virtio: device uses revision 1 "
> > +			"but does not have VIRTIO_F_VERSION_1\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> >  	if (!ccw)
> >  		return 0;
> 
> I'm still not convinced by this change: I'd prefer to allow rev 1
> without VERSION_1, especially as the core makes all its decisions based
> upon VERSION_1.

At the moment, but this is an implementation detail.
This is exactly why I want this hard requirement in code.


> Unless someone else has a good argument in favour of
> this change.


Let's not commit to something we are not sure we
can support.

We can always remove this code, but once we release
guest we won't be able to drop it.



-- 
MST

Re: [PATCH v3 4/6] virtio_ccw: rev 1 devices set VIRTIO_F_VERSION_1

[Cornelia Huck]

On Tue, 9 Dec 2014 14:21:18 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 09, 2014 at 12:01:23PM +0100, Cornelia Huck wrote:
> > On Mon, 8 Dec 2014 15:06:03 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:

> > > diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> > > index 789275f..f9f87ba 100644
> > > --- a/drivers/s390/kvm/virtio_ccw.c
> > > +++ b/drivers/s390/kvm/virtio_ccw.c
> > > @@ -758,6 +758,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> > >  	struct virtio_feature_desc *features;
> > >  	struct ccw1 *ccw;

This needs

+	struct virtio_device *vdev = &vcdev->vdev;

to make it compile :)

> > > 
> > > +	if (vcdev->revision == 1 &&
> > 
> > If we decide to keep this check, it should be for rev >= 1, though.
> 
> Fine, though this is theoretical, right?
> Ican change this with a patch on top.
> 
> > > +	    !__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> > > +		dev_err(&vdev->dev, "virtio: device uses revision 1 "
> > > +			"but does not have VIRTIO_F_VERSION_1\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> > >  	if (!ccw)
> > >  		return 0;
> > 
> > I'm still not convinced by this change: I'd prefer to allow rev 1
> > without VERSION_1, especially as the core makes all its decisions based
> > upon VERSION_1.
> 
> At the moment, but this is an implementation detail.
> This is exactly why I want this hard requirement in code.
> 
> 
> > Unless someone else has a good argument in favour of
> > this change.
> 
> 
> Let's not commit to something we are not sure we
> can support.
> 
> We can always remove this code, but once we release
> guest we won't be able to drop it.
> 

OK, with your qemu patch on the host side this seems to be fine. No
further objections from me for now.

Re: [PATCH v3 4/6] virtio_ccw: rev 1 devices set VIRTIO_F_VERSION_1

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 06:23:19PM +0100, Cornelia Huck wrote:
> On Tue, 9 Dec 2014 14:21:18 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Dec 09, 2014 at 12:01:23PM +0100, Cornelia Huck wrote:
> > > On Mon, 8 Dec 2014 15:06:03 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > > diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> > > > index 789275f..f9f87ba 100644
> > > > --- a/drivers/s390/kvm/virtio_ccw.c
> > > > +++ b/drivers/s390/kvm/virtio_ccw.c
> > > > @@ -758,6 +758,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> > > >  	struct virtio_feature_desc *features;
> > > >  	struct ccw1 *ccw;
> 
> This needs
> 
> +	struct virtio_device *vdev = &vcdev->vdev;
> 
> to make it compile :)
> 
> > > > 
> > > > +	if (vcdev->revision == 1 &&
> > > 
> > > If we decide to keep this check, it should be for rev >= 1, though.
> > 
> > Fine, though this is theoretical, right?
> > Ican change this with a patch on top.
> > 
> > > > +	    !__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> > > > +		dev_err(&vdev->dev, "virtio: device uses revision 1 "
> > > > +			"but does not have VIRTIO_F_VERSION_1\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > >  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> > > >  	if (!ccw)
> > > >  		return 0;
> > > 
> > > I'm still not convinced by this change: I'd prefer to allow rev 1
> > > without VERSION_1, especially as the core makes all its decisions based
> > > upon VERSION_1.
> > 
> > At the moment, but this is an implementation detail.
> > This is exactly why I want this hard requirement in code.
> > 
> > 
> > > Unless someone else has a good argument in favour of
> > > this change.
> > 
> > 
> > Let's not commit to something we are not sure we
> > can support.
> > 
> > We can always remove this code, but once we release
> > guest we won't be able to drop it.
> > 
> 
> OK, with your qemu patch on the host side this seems to be fine. No
> further objections from me for now.


I fixed up my tree and pushed, so it should be good for testing now.

-- 
MST

Re: [PATCH v3 4/6] virtio_ccw: rev 1 devices set VIRTIO_F_VERSION_1

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 06:23:19PM +0100, Cornelia Huck wrote:
> On Tue, 9 Dec 2014 14:21:18 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Dec 09, 2014 at 12:01:23PM +0100, Cornelia Huck wrote:
> > > On Mon, 8 Dec 2014 15:06:03 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > > diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> > > > index 789275f..f9f87ba 100644
> > > > --- a/drivers/s390/kvm/virtio_ccw.c
> > > > +++ b/drivers/s390/kvm/virtio_ccw.c
> > > > @@ -758,6 +758,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> > > >  	struct virtio_feature_desc *features;
> > > >  	struct ccw1 *ccw;
> 
> This needs
> 
> +	struct virtio_device *vdev = &vcdev->vdev;
> 
> to make it compile :)

In fact why does it?
vdev is a parameter.

> > > > 
> > > > +	if (vcdev->revision == 1 &&
> > > 
> > > If we decide to keep this check, it should be for rev >= 1, though.
> > 
> > Fine, though this is theoretical, right?
> > Ican change this with a patch on top.
> > 
> > > > +	    !__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> > > > +		dev_err(&vdev->dev, "virtio: device uses revision 1 "
> > > > +			"but does not have VIRTIO_F_VERSION_1\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > >  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> > > >  	if (!ccw)
> > > >  		return 0;
> > > 
> > > I'm still not convinced by this change: I'd prefer to allow rev 1
> > > without VERSION_1, especially as the core makes all its decisions based
> > > upon VERSION_1.
> > 
> > At the moment, but this is an implementation detail.
> > This is exactly why I want this hard requirement in code.
> > 
> > 
> > > Unless someone else has a good argument in favour of
> > > this change.
> > 
> > 
> > Let's not commit to something we are not sure we
> > can support.
> > 
> > We can always remove this code, but once we release
> > guest we won't be able to drop it.
> > 
> 
> OK, with your qemu patch on the host side this seems to be fine. No
> further objections from me for now.

Re: [PATCH v3 4/6] virtio_ccw: rev 1 devices set VIRTIO_F_VERSION_1

[Cornelia Huck]

On Tue, 9 Dec 2014 21:40:52 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 09, 2014 at 06:23:19PM +0100, Cornelia Huck wrote:
> > On Tue, 9 Dec 2014 14:21:18 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Dec 09, 2014 at 12:01:23PM +0100, Cornelia Huck wrote:
> > > > On Mon, 8 Dec 2014 15:06:03 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > > > diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> > > > > index 789275f..f9f87ba 100644
> > > > > --- a/drivers/s390/kvm/virtio_ccw.c
> > > > > +++ b/drivers/s390/kvm/virtio_ccw.c
> > > > > @@ -758,6 +758,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> > > > >  	struct virtio_feature_desc *features;
> > > > >  	struct ccw1 *ccw;
> > 
> > This needs
> > 
> > +	struct virtio_device *vdev = &vcdev->vdev;
> > 
> > to make it compile :)
> 
> In fact why does it?
> vdev is a parameter.

Never mind, messed up tree on my side.

[PATCH v3 5/6] virtio_balloon: drop legacy_only driver flag

[Michael S. Tsirkin]

we have blacklisted balloon in core, no need
for a driver flag.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_balloon.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4497def..c9703d4 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -518,7 +518,6 @@ static unsigned int features[] = {
 };
 
 static struct virtio_driver virtio_balloon_driver = {
-	.legacy_only = true,
 	.feature_table = features,
 	.feature_table_size = ARRAY_SIZE(features),
 	.driver.name =	KBUILD_MODNAME,
-- 
MST

Re: [PATCH v3 5/6] virtio_balloon: drop legacy_only driver flag

[Cornelia Huck]

On Mon, 8 Dec 2014 15:06:07 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> we have blacklisted balloon in core, no need
> for a driver flag.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/virtio/virtio_balloon.c | 1 -
>  1 file changed, 1 deletion(-)

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

[PATCH v3 6/6] virtio: drop legacy_only driver flag

[Michael S. Tsirkin]

legacy_only flag is now unused, drop it from core.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/virtio.h  | 2 --
 drivers/virtio/virtio.c | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index d666bcb..d09e093 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -134,7 +134,6 @@ int virtio_device_restore(struct virtio_device *dev);
  * @feature_table_size: number of entries in the feature table array.
  * @feature_table_legacy: same as feature_table but when working in legacy mode.
  * @feature_table_size_legacy: number of entries in feature table legacy array.
- * @legacy_only: driver does not support virtio 1.0.
  * @probe: the function to call when a device is found.  Returns 0 or -errno.
  * @remove: the function to call when a device is removed.
  * @config_changed: optional function to call when the device configuration
@@ -147,7 +146,6 @@ struct virtio_driver {
 	unsigned int feature_table_size;
 	const unsigned int *feature_table_legacy;
 	unsigned int feature_table_size_legacy;
-	bool legacy_only;
 	int (*probe)(struct virtio_device *dev);
 	void (*scan)(struct virtio_device *dev);
 	void (*remove)(struct virtio_device *dev);
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index e1673a5..f226658 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -198,10 +198,6 @@ static int virtio_dev_probe(struct device *_d)
 		driver_features_legacy = driver_features;
 	}
 
-	/* Detect legacy-only drivers and disable VIRTIO_F_VERSION_1. */
-	if (drv->legacy_only)
-		device_features &= ~(1ULL << VIRTIO_F_VERSION_1);
-
 	if (device_features & (1ULL << VIRTIO_F_VERSION_1))
 		dev->features = driver_features & device_features;
 	else
-- 
MST

Re: [PATCH v3 6/6] virtio: drop legacy_only driver flag

[Cornelia Huck]

On Mon, 8 Dec 2014 15:06:10 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> legacy_only flag is now unused, drop it from core.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/linux/virtio.h  | 2 --
>  drivers/virtio/virtio.c | 4 ----
>  2 files changed, 6 deletions(-)
> 
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>