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 at 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
Michael S. Tsirkin
2014-Dec-08 13:05 UTC
[PATCH v3 1/6] virtio: add API to detect legacy devices
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 at redhat.com> Acked-by: Cornelia Huck <cornelia.huck at 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
Michael S. Tsirkin
2014-Dec-08 13:05 UTC
[PATCH v3 3/6] virtio: allow finalize_features to fail
This will make it easy for transports to validate features and return failure. Signed-off-by: Michael S. Tsirkin <mst at 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
Michael S. Tsirkin
2014-Dec-08 13:06 UTC
[PATCH v3 5/6] virtio_balloon: drop legacy_only driver flag
we have blacklisted balloon in core, no need for a driver flag. Signed-off-by: Michael S. Tsirkin <mst at 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
Michael S. Tsirkin
2014-Dec-08 13:06 UTC
[PATCH v3 6/6] virtio: drop legacy_only driver flag
legacy_only flag is now unused, drop it from core. Signed-off-by: Michael S. Tsirkin <mst at 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
Cornelia Huck
2014-Dec-09 10:46 UTC
[PATCH v3 3/6] virtio: allow finalize_features to fail
On Mon, 8 Dec 2014 15:05:58 +0200 "Michael S. Tsirkin" <mst at redhat.com> wrote:> This will make it easy for transports to validate features and return > failure. > > Signed-off-by: Michael S. Tsirkin <mst at 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,
Cornelia Huck
2014-Dec-09 11:24 UTC
[PATCH v3 5/6] virtio_balloon: drop legacy_only driver flag
On Mon, 8 Dec 2014 15:06:07 +0200 "Michael S. Tsirkin" <mst at redhat.com> wrote:> we have blacklisted balloon in core, no need > for a driver flag. > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > drivers/virtio/virtio_balloon.c | 1 - > 1 file changed, 1 deletion(-)Acked-by: Cornelia Huck <cornelia.huck at de.ibm.com>
On Mon, 8 Dec 2014 15:06:10 +0200 "Michael S. Tsirkin" <mst at redhat.com> wrote:> legacy_only flag is now unused, drop it from core. > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > include/linux/virtio.h | 2 -- > drivers/virtio/virtio.c | 4 ---- > 2 files changed, 6 deletions(-) >Acked-by: Cornelia Huck <cornelia.huck at de.ibm.com>
Apparently Analagous Threads
- [PATCH v8 48/50] virtio_balloon: add legacy_only flag
- [PATCH v8 48/50] virtio_balloon: add legacy_only flag
- [PATCH v3 5/6] virtio_balloon: drop legacy_only driver flag
- [PATCH v3 6/6] virtio: drop legacy_only driver flag
- [PATCH v3 6/6] virtio: drop legacy_only driver flag