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/[email protected]
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
This will make it easy for transports to validate features and return
failure.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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
legacy_only flag is now unused, drop it from core.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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
we have blacklisted balloon in core, no need
for a driver flag.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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
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 <[email protected]>
Acked-by: Cornelia Huck <[email protected]>
---
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
On Mon, 8 Dec 2014 15:05:54 +0200
"Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> ---
> drivers/s390/kvm/virtio_ccw.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
Otherwise, looks good.
Reviewed-by: Cornelia Huck <[email protected]>
On Mon, 8 Dec 2014 15:05:58 +0200
"Michael S. Tsirkin" <[email protected]> wrote:
> This will make it easy for transports to validate features and return
> failure.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> 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,
On Mon, 8 Dec 2014 15:06:03 +0200
"Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> ---
> 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.
On Mon, 8 Dec 2014 15:06:07 +0200
"Michael S. Tsirkin" <[email protected]> wrote:
> we have blacklisted balloon in core, no need
> for a driver flag.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/virtio/virtio_balloon.c | 1 -
> 1 file changed, 1 deletion(-)
Acked-by: Cornelia Huck <[email protected]>
On Mon, 8 Dec 2014 15:06:10 +0200
"Michael S. Tsirkin" <[email protected]> wrote:
> legacy_only flag is now unused, drop it from core.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> include/linux/virtio.h | 2 --
> drivers/virtio/virtio.c | 4 ----
> 2 files changed, 6 deletions(-)
>
Acked-by: Cornelia Huck <[email protected]>
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" <[email protected]> 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 <[email protected]>
> > ---
> > 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
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" <[email protected]> wrote:
>
> > This will make it easy for transports to validate features and return
> > failure.
> >
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > 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,
On Tue, 9 Dec 2014 14:07:54 +0200
"Michael S. Tsirkin" <[email protected]> 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" <[email protected]> wrote:
> >
> > > This will make it easy for transports to validate features and return
> > > failure.
> > >
> > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > ---
> > > 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 <[email protected]>
>
> 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,
>
On Tue, 9 Dec 2014 14:21:18 +0200
"Michael S. Tsirkin" <[email protected]> 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" <[email protected]> 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.
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" <[email protected]> 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" <[email protected]> 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
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" <[email protected]> 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" <[email protected]> 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.
On Tue, 9 Dec 2014 21:40:52 +0200
"Michael S. Tsirkin" <[email protected]> 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" <[email protected]> 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" <[email protected]> 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.