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

[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.