From 6fbc198cf623944ab60a1db6d306a4d55cdd820d Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 14 Oct 2014 10:40:29 +1030 Subject: [PATCH 01/25] virtio_pci: fix virtio spec compliance on restore On restore, virtio pci does the following: + set features + init vqs etc - device can be used at this point! + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits This is in violation of the virtio spec, which requires the following order: - ACKNOWLEDGE - DRIVER - init vqs - DRIVER_OK This behaviour will break with hypervisors that assume spec compliant behaviour. It seems like a good idea to have this patch applied to stable branches to reduce the support butden for the hypervisors. Cc: stable@vger.kernel.org Cc: Amit Shah Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/virtio/virtio_pci.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index 3d1463c6b120..add40d00dcdb 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -789,6 +789,7 @@ static int virtio_pci_restore(struct device *dev) struct pci_dev *pci_dev = to_pci_dev(dev); struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); struct virtio_driver *drv; + unsigned status = 0; int ret; drv = container_of(vp_dev->vdev.dev.driver, @@ -799,14 +800,40 @@ static int virtio_pci_restore(struct device *dev) return ret; pci_set_master(pci_dev); + /* We always start by resetting the device, in case a previous + * driver messed it up. */ + vp_reset(&vp_dev->vdev); + + /* Acknowledge that we've seen the device. */ + status |= VIRTIO_CONFIG_S_ACKNOWLEDGE; + vp_set_status(&vp_dev->vdev, status); + + /* Maybe driver failed before freeze. + * Restore the failed status, for debugging. */ + status |= vp_dev->saved_status & VIRTIO_CONFIG_S_FAILED; + vp_set_status(&vp_dev->vdev, status); + + if (!drv) + return 0; + + /* We have a driver! */ + status |= VIRTIO_CONFIG_S_DRIVER; + vp_set_status(&vp_dev->vdev, status); + vp_finalize_features(&vp_dev->vdev); - if (drv && drv->restore) + if (drv->restore) { ret = drv->restore(&vp_dev->vdev); + if (ret) { + status |= VIRTIO_CONFIG_S_FAILED; + vp_set_status(&vp_dev->vdev, status); + return ret; + } + } /* Finally, tell the device we're all set */ - if (!ret) - vp_set_status(&vp_dev->vdev, vp_dev->saved_status); + status |= VIRTIO_CONFIG_S_DRIVER_OK; + vp_set_status(&vp_dev->vdev, status); return ret; } From 016c98c6fe0c914d12e2e242b2bccde6d6dea54b Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 14 Oct 2014 10:40:34 +1030 Subject: [PATCH 02/25] virtio: unify config_changed handling Replace duplicated code in all transports with a single wrapper in virtio.c. The only functional change is in virtio_mmio.c: if a buggy device sends us an interrupt before driver is set, we previously returned IRQ_NONE, now we return IRQ_HANDLED. As this must not happen in practice, this does not look like a big deal. See also commit 3fff0179e33cd7d0a688dab65700c46ad089e934 virtio-pci: do not oops on config change if driver not loaded. for the original motivation behind the driver check. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/misc/mic/card/mic_virtio.c | 6 +----- drivers/s390/kvm/kvm_virtio.c | 9 +-------- drivers/s390/kvm/virtio_ccw.c | 6 +----- drivers/virtio/virtio.c | 9 +++++++++ drivers/virtio/virtio_mmio.c | 7 ++----- drivers/virtio/virtio_pci.c | 6 +----- include/linux/virtio.h | 2 ++ 7 files changed, 17 insertions(+), 28 deletions(-) diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c index f14b60080c21..e64794730e21 100644 --- a/drivers/misc/mic/card/mic_virtio.c +++ b/drivers/misc/mic/card/mic_virtio.c @@ -462,16 +462,12 @@ static void mic_handle_config_change(struct mic_device_desc __iomem *d, struct mic_device_ctrl __iomem *dc = (void __iomem *)d + mic_aligned_desc_size(d); struct mic_vdev *mvdev = (struct mic_vdev *)ioread64(&dc->vdev); - struct virtio_driver *drv; if (ioread8(&dc->config_change) != MIC_VIRTIO_PARAM_CONFIG_CHANGED) return; dev_dbg(mdrv->dev, "%s %d\n", __func__, __LINE__); - drv = container_of(mvdev->vdev.dev.driver, - struct virtio_driver, driver); - if (drv->config_changed) - drv->config_changed(&mvdev->vdev); + virtio_config_changed(&mvdev->vdev); iowrite8(1, &dc->guest_ack); } diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c index a1349653c6d9..643129070c51 100644 --- a/drivers/s390/kvm/kvm_virtio.c +++ b/drivers/s390/kvm/kvm_virtio.c @@ -406,15 +406,8 @@ static void kvm_extint_handler(struct ext_code ext_code, switch (param) { case VIRTIO_PARAM_CONFIG_CHANGED: - { - struct virtio_driver *drv; - drv = container_of(vq->vdev->dev.driver, - struct virtio_driver, driver); - if (drv->config_changed) - drv->config_changed(vq->vdev); - + virtio_config_changed(vq->vdev); break; - } case VIRTIO_PARAM_DEV_ADD: schedule_work(&hotplug_work); break; diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c index d2c0b442bce5..6cbe6ef3c889 100644 --- a/drivers/s390/kvm/virtio_ccw.c +++ b/drivers/s390/kvm/virtio_ccw.c @@ -940,11 +940,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev, vring_interrupt(0, vq); } if (test_bit(0, &vcdev->indicators2)) { - drv = container_of(vcdev->vdev.dev.driver, - struct virtio_driver, driver); - - if (drv && drv->config_changed) - drv->config_changed(&vcdev->vdev); + virtio_config_changed(&vcdev->vdev); clear_bit(0, &vcdev->indicators2); } } diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index fed0ce198ae3..3980687401f6 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -239,6 +239,15 @@ void unregister_virtio_device(struct virtio_device *dev) } EXPORT_SYMBOL_GPL(unregister_virtio_device); +void virtio_config_changed(struct virtio_device *dev) +{ + struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); + + if (drv && drv->config_changed) + drv->config_changed(dev); +} +EXPORT_SYMBOL_GPL(virtio_config_changed); + static int virtio_init(void) { if (bus_register(&virtio_bus) != 0) diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c index c600ccfd6922..ef9a1650bb80 100644 --- a/drivers/virtio/virtio_mmio.c +++ b/drivers/virtio/virtio_mmio.c @@ -234,8 +234,6 @@ static irqreturn_t vm_interrupt(int irq, void *opaque) { struct virtio_mmio_device *vm_dev = opaque; struct virtio_mmio_vq_info *info; - struct virtio_driver *vdrv = container_of(vm_dev->vdev.dev.driver, - struct virtio_driver, driver); unsigned long status; unsigned long flags; irqreturn_t ret = IRQ_NONE; @@ -244,9 +242,8 @@ static irqreturn_t vm_interrupt(int irq, void *opaque) status = readl(vm_dev->base + VIRTIO_MMIO_INTERRUPT_STATUS); writel(status, vm_dev->base + VIRTIO_MMIO_INTERRUPT_ACK); - if (unlikely(status & VIRTIO_MMIO_INT_CONFIG) - && vdrv && vdrv->config_changed) { - vdrv->config_changed(&vm_dev->vdev); + if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)) { + virtio_config_changed(&vm_dev->vdev); ret = IRQ_HANDLED; } diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index add40d00dcdb..f39f4e772e6a 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -211,12 +211,8 @@ static bool vp_notify(struct virtqueue *vq) static irqreturn_t vp_config_changed(int irq, void *opaque) { struct virtio_pci_device *vp_dev = opaque; - struct virtio_driver *drv; - drv = container_of(vp_dev->vdev.dev.driver, - struct virtio_driver, driver); - if (drv && drv->config_changed) - drv->config_changed(&vp_dev->vdev); + virtio_config_changed(&vp_dev->vdev); return IRQ_HANDLED; } diff --git a/include/linux/virtio.h b/include/linux/virtio.h index b46671e28de2..3c19bd3189cb 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -108,6 +108,8 @@ void unregister_virtio_device(struct virtio_device *dev); void virtio_break_device(struct virtio_device *dev); +void virtio_config_changed(struct virtio_device *dev); + /** * virtio_driver - operations for a virtio I/O driver * @driver: underlying device driver (populate name and owner). From c6716bae52f97347e25166c6270aa98693d9212c Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 14 Oct 2014 10:40:35 +1030 Subject: [PATCH 03/25] virtio-pci: move freeze/restore to virtio core This is in preparation to extending config changed event handling in core. Wrapping these in an API also seems to make for a cleaner code. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/virtio/virtio.c | 54 +++++++++++++++++++++++++++++++++++++ drivers/virtio/virtio_pci.c | 54 ++----------------------------------- include/linux/virtio.h | 6 +++++ 3 files changed, 62 insertions(+), 52 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 3980687401f6..8216b7311092 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -248,6 +248,60 @@ void virtio_config_changed(struct virtio_device *dev) } EXPORT_SYMBOL_GPL(virtio_config_changed); +#ifdef CONFIG_PM_SLEEP +int virtio_device_freeze(struct virtio_device *dev) +{ + struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); + + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED; + + if (drv && drv->freeze) + return drv->freeze(dev); + + return 0; +} +EXPORT_SYMBOL_GPL(virtio_device_freeze); + +int virtio_device_restore(struct virtio_device *dev) +{ + struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); + + /* We always start by resetting the device, in case a previous + * driver messed it up. */ + dev->config->reset(dev); + + /* Acknowledge that we've seen the device. */ + add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); + + /* Maybe driver failed before freeze. + * Restore the failed status, for debugging. */ + if (dev->failed) + add_status(dev, VIRTIO_CONFIG_S_FAILED); + + if (!drv) + return 0; + + /* We have a driver! */ + add_status(dev, VIRTIO_CONFIG_S_DRIVER); + + dev->config->finalize_features(dev); + + if (drv->restore) { + int ret = drv->restore(dev); + if (ret) { + add_status(dev, VIRTIO_CONFIG_S_FAILED); + return ret; + } + } + + /* Finally, tell the device we're all set */ + add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); + + return 0; +} +EXPORT_SYMBOL_GPL(virtio_device_restore); +#endif + static int virtio_init(void) { if (bus_register(&virtio_bus) != 0) diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index f39f4e772e6a..d34ebfa604f3 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -57,9 +57,6 @@ struct virtio_pci_device /* Vectors allocated, excluding per-vq vectors if any */ unsigned msix_used_vectors; - /* Status saved during hibernate/restore */ - u8 saved_status; - /* Whether we have vector per vq */ bool per_vq_vectors; }; @@ -764,16 +761,9 @@ static int virtio_pci_freeze(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); - struct virtio_driver *drv; int ret; - drv = container_of(vp_dev->vdev.dev.driver, - struct virtio_driver, driver); - - ret = 0; - vp_dev->saved_status = vp_get_status(&vp_dev->vdev); - if (drv && drv->freeze) - ret = drv->freeze(&vp_dev->vdev); + ret = virtio_device_freeze(&vp_dev->vdev); if (!ret) pci_disable_device(pci_dev); @@ -784,54 +774,14 @@ static int virtio_pci_restore(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); - struct virtio_driver *drv; - unsigned status = 0; int ret; - drv = container_of(vp_dev->vdev.dev.driver, - struct virtio_driver, driver); - ret = pci_enable_device(pci_dev); if (ret) return ret; pci_set_master(pci_dev); - /* We always start by resetting the device, in case a previous - * driver messed it up. */ - vp_reset(&vp_dev->vdev); - - /* Acknowledge that we've seen the device. */ - status |= VIRTIO_CONFIG_S_ACKNOWLEDGE; - vp_set_status(&vp_dev->vdev, status); - - /* Maybe driver failed before freeze. - * Restore the failed status, for debugging. */ - status |= vp_dev->saved_status & VIRTIO_CONFIG_S_FAILED; - vp_set_status(&vp_dev->vdev, status); - - if (!drv) - return 0; - - /* We have a driver! */ - status |= VIRTIO_CONFIG_S_DRIVER; - vp_set_status(&vp_dev->vdev, status); - - vp_finalize_features(&vp_dev->vdev); - - if (drv->restore) { - ret = drv->restore(&vp_dev->vdev); - if (ret) { - status |= VIRTIO_CONFIG_S_FAILED; - vp_set_status(&vp_dev->vdev, status); - return ret; - } - } - - /* Finally, tell the device we're all set */ - status |= VIRTIO_CONFIG_S_DRIVER_OK; - vp_set_status(&vp_dev->vdev, status); - - return ret; + return virtio_device_restore(&vp_dev->vdev); } static const struct dev_pm_ops virtio_pci_pm_ops = { diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 3c19bd3189cb..8df7ba81e5c7 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq); /** * virtio_device - representation of a device using virtio * @index: unique position on the virtio bus + * @failed: saved value for CONFIG_S_FAILED bit (for restore) * @dev: underlying device. * @id: the device type identification (used to match it with a driver). * @config: the configuration ops for this device. @@ -88,6 +89,7 @@ bool virtqueue_is_broken(struct virtqueue *vq); */ struct virtio_device { int index; + bool failed; struct device dev; struct virtio_device_id id; const struct virtio_config_ops *config; @@ -109,6 +111,10 @@ void unregister_virtio_device(struct virtio_device *dev); void virtio_break_device(struct virtio_device *dev); void virtio_config_changed(struct virtio_device *dev); +#ifdef CONFIG_PM_SLEEP +int virtio_device_freeze(struct virtio_device *dev); +int virtio_device_restore(struct virtio_device *dev); +#endif /** * virtio_driver - operations for a virtio I/O driver From 22b7050a024d7deb0c9ef1e14ed73e3b1e369f24 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:21:55 +1030 Subject: [PATCH 04/25] virtio: defer config changed notifications Defer config changed notifications that arrive during probe/scan/freeze/restore. This will allow drivers to set DRIVER_OK earlier, without worrying about racing with config change interrupts. This change will also benefit old hypervisors (before 2009) that send interrupts without checking DRIVER_OK: previously, the callback could race with driver-specific initialization. This will also help simplify drivers. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell (cosmetic changes) --- drivers/virtio/virtio.c | 58 ++++++++++++++++++++++++++++++++++------- include/linux/virtio.h | 6 +++++ 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 8216b7311092..df598dd8c5c8 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -117,6 +117,43 @@ void virtio_check_driver_offered_feature(const struct virtio_device *vdev, } EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature); +static void __virtio_config_changed(struct virtio_device *dev) +{ + struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); + + if (!dev->config_enabled) + dev->config_change_pending = true; + else if (drv && drv->config_changed) + drv->config_changed(dev); +} + +void virtio_config_changed(struct virtio_device *dev) +{ + unsigned long flags; + + spin_lock_irqsave(&dev->config_lock, flags); + __virtio_config_changed(dev); + spin_unlock_irqrestore(&dev->config_lock, flags); +} +EXPORT_SYMBOL_GPL(virtio_config_changed); + +static void virtio_config_disable(struct virtio_device *dev) +{ + spin_lock_irq(&dev->config_lock); + dev->config_enabled = false; + spin_unlock_irq(&dev->config_lock); +} + +static void virtio_config_enable(struct virtio_device *dev) +{ + spin_lock_irq(&dev->config_lock); + dev->config_enabled = true; + if (dev->config_change_pending) + __virtio_config_changed(dev); + dev->config_change_pending = false; + spin_unlock_irq(&dev->config_lock); +} + static int virtio_dev_probe(struct device *_d) { int err, i; @@ -153,6 +190,8 @@ static int virtio_dev_probe(struct device *_d) add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); if (drv->scan) drv->scan(dev); + + virtio_config_enable(dev); } return err; @@ -163,6 +202,8 @@ static int virtio_dev_remove(struct device *_d) struct virtio_device *dev = dev_to_virtio(_d); struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); + virtio_config_disable(dev); + drv->remove(dev); /* Driver should have reset device. */ @@ -211,6 +252,10 @@ int register_virtio_device(struct virtio_device *dev) dev->index = err; dev_set_name(&dev->dev, "virtio%u", dev->index); + spin_lock_init(&dev->config_lock); + dev->config_enabled = false; + dev->config_change_pending = false; + /* We always start by resetting the device, in case a previous * driver messed it up. This also tests that code path a little. */ dev->config->reset(dev); @@ -239,20 +284,13 @@ void unregister_virtio_device(struct virtio_device *dev) } EXPORT_SYMBOL_GPL(unregister_virtio_device); -void virtio_config_changed(struct virtio_device *dev) -{ - struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); - - if (drv && drv->config_changed) - drv->config_changed(dev); -} -EXPORT_SYMBOL_GPL(virtio_config_changed); - #ifdef CONFIG_PM_SLEEP int virtio_device_freeze(struct virtio_device *dev) { struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); + virtio_config_disable(dev); + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED; if (drv && drv->freeze) @@ -297,6 +335,8 @@ int virtio_device_restore(struct virtio_device *dev) /* Finally, tell the device we're all set */ add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); + virtio_config_enable(dev); + return 0; } EXPORT_SYMBOL_GPL(virtio_device_restore); diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 8df7ba81e5c7..65261a7244fc 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -79,6 +79,9 @@ bool virtqueue_is_broken(struct virtqueue *vq); * virtio_device - representation of a device using virtio * @index: unique position on the virtio bus * @failed: saved value for CONFIG_S_FAILED bit (for restore) + * @config_enabled: configuration change reporting enabled + * @config_change_pending: configuration change reported while disabled + * @config_lock: protects configuration change reporting * @dev: underlying device. * @id: the device type identification (used to match it with a driver). * @config: the configuration ops for this device. @@ -90,6 +93,9 @@ bool virtqueue_is_broken(struct virtqueue *vq); struct virtio_device { int index; bool failed; + bool config_enabled; + bool config_change_pending; + spinlock_t config_lock; struct device dev; struct virtio_device_id id; const struct virtio_config_ops *config; From cc74f71934da13fa979669467c04f0d2e5563112 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:26 +1030 Subject: [PATCH 05/25] virtio_blk: drop config_enable Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/block/virtio_blk.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a581400de0f..91272f1a7dad 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -44,9 +44,6 @@ struct virtio_blk /* Lock for config space updates */ struct mutex config_lock; - /* enable config space updates */ - bool config_enable; - /* What host tells us, plus 2 for header & tailer. */ unsigned int sg_elems; @@ -348,8 +345,6 @@ static void virtblk_config_changed_work(struct work_struct *work) u64 capacity, size; mutex_lock(&vblk->config_lock); - if (!vblk->config_enable) - goto done; /* Host must always specify the capacity. */ virtio_cread(vdev, struct virtio_blk_config, capacity, &capacity); @@ -374,7 +369,7 @@ static void virtblk_config_changed_work(struct work_struct *work) set_capacity(vblk->disk, capacity); revalidate_disk(vblk->disk); kobject_uevent_env(&disk_to_dev(vblk->disk)->kobj, KOBJ_CHANGE, envp); -done: + mutex_unlock(&vblk->config_lock); } @@ -609,7 +604,6 @@ static int virtblk_probe(struct virtio_device *vdev) mutex_init(&vblk->config_lock); INIT_WORK(&vblk->config_work, virtblk_config_changed_work); - vblk->config_enable = true; err = init_vq(vblk); if (err) @@ -771,10 +765,8 @@ static void virtblk_remove(struct virtio_device *vdev) int index = vblk->index; int refc; - /* Prevent config work handler from accessing the device. */ - mutex_lock(&vblk->config_lock); - vblk->config_enable = false; - mutex_unlock(&vblk->config_lock); + /* Make sure no work handler is accessing the device. */ + flush_work(&vblk->config_work); del_gendisk(vblk->disk); blk_cleanup_queue(vblk->disk->queue); @@ -784,8 +776,6 @@ static void virtblk_remove(struct virtio_device *vdev) /* Stop all the virtqueues. */ vdev->config->reset(vdev); - flush_work(&vblk->config_work); - refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount); put_disk(vblk->disk); vdev->config->del_vqs(vdev); @@ -805,11 +795,7 @@ static int virtblk_freeze(struct virtio_device *vdev) /* Ensure we don't receive any more interrupts */ vdev->config->reset(vdev); - /* Prevent config work handler from accessing the device. */ - mutex_lock(&vblk->config_lock); - vblk->config_enable = false; - mutex_unlock(&vblk->config_lock); - + /* Make sure no work handler is accessing the device. */ flush_work(&vblk->config_work); blk_mq_stop_hw_queues(vblk->disk->queue); @@ -823,7 +809,6 @@ static int virtblk_restore(struct virtio_device *vdev) struct virtio_blk *vblk = vdev->priv; int ret; - vblk->config_enable = true; ret = init_vq(vdev->priv); if (!ret) blk_mq_start_stopped_hw_queues(vblk->disk->queue, true); From 1f54b0c055b9322f4e7acb49c492edc5accd15ae Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:29 +1030 Subject: [PATCH 06/25] virtio-blk: drop config_mutex config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/block/virtio_blk.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 91272f1a7dad..89ba8d6baa94 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -41,9 +41,6 @@ struct virtio_blk /* Process context for config space updates */ struct work_struct config_work; - /* Lock for config space updates */ - struct mutex config_lock; - /* What host tells us, plus 2 for header & tailer. */ unsigned int sg_elems; @@ -344,8 +341,6 @@ static void virtblk_config_changed_work(struct work_struct *work) char *envp[] = { "RESIZE=1", NULL }; u64 capacity, size; - mutex_lock(&vblk->config_lock); - /* Host must always specify the capacity. */ virtio_cread(vdev, struct virtio_blk_config, capacity, &capacity); @@ -369,8 +364,6 @@ static void virtblk_config_changed_work(struct work_struct *work) set_capacity(vblk->disk, capacity); revalidate_disk(vblk->disk); kobject_uevent_env(&disk_to_dev(vblk->disk)->kobj, KOBJ_CHANGE, envp); - - mutex_unlock(&vblk->config_lock); } static void virtblk_config_changed(struct virtio_device *vdev) @@ -601,7 +594,6 @@ static int virtblk_probe(struct virtio_device *vdev) vblk->vdev = vdev; vblk->sg_elems = sg_elems; - mutex_init(&vblk->config_lock); INIT_WORK(&vblk->config_work, virtblk_config_changed_work); From 102a2786c9df756cffdbcfd11096124e4dc6c311 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:29 +1030 Subject: [PATCH 07/25] virtio_net: drop config_enable Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio net. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 59caa06f34a6..743fb04a4df9 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -123,9 +123,6 @@ struct virtnet_info { /* Host can handle any s/g split between our header and packet data */ bool any_header_sg; - /* enable config space updates */ - bool config_enable; - /* Active statistics */ struct virtnet_stats __percpu *stats; @@ -1408,9 +1405,6 @@ static void virtnet_config_changed_work(struct work_struct *work) u16 v; mutex_lock(&vi->config_lock); - if (!vi->config_enable) - goto done; - if (virtio_cread_feature(vi->vdev, VIRTIO_NET_F_STATUS, struct virtio_net_config, status, &v) < 0) goto done; @@ -1758,7 +1752,6 @@ static int virtnet_probe(struct virtio_device *vdev) } mutex_init(&vi->config_lock); - vi->config_enable = true; INIT_WORK(&vi->config_work, virtnet_config_changed_work); /* If we can receive ANY GSO packets, we must allocate large ones. */ @@ -1875,17 +1868,13 @@ static void virtnet_remove(struct virtio_device *vdev) unregister_hotcpu_notifier(&vi->nb); - /* Prevent config work handler from accessing the device. */ - mutex_lock(&vi->config_lock); - vi->config_enable = false; - mutex_unlock(&vi->config_lock); + /* Make sure no work handler is accessing the device. */ + flush_work(&vi->config_work); unregister_netdev(vi->dev); remove_vq_common(vi); - flush_work(&vi->config_work); - free_percpu(vi->stats); free_netdev(vi->dev); } @@ -1898,10 +1887,8 @@ static int virtnet_freeze(struct virtio_device *vdev) unregister_hotcpu_notifier(&vi->nb); - /* Prevent config work handler from accessing the device */ - mutex_lock(&vi->config_lock); - vi->config_enable = false; - mutex_unlock(&vi->config_lock); + /* Make sure no work handler is accessing the device */ + flush_work(&vi->config_work); netif_device_detach(vi->dev); cancel_delayed_work_sync(&vi->refill); @@ -1916,8 +1903,6 @@ static int virtnet_freeze(struct virtio_device *vdev) remove_vq_common(vi); - flush_work(&vi->config_work); - return 0; } @@ -1941,10 +1926,6 @@ static int virtnet_restore(struct virtio_device *vdev) netif_device_attach(vi->dev); - mutex_lock(&vi->config_lock); - vi->config_enable = true; - mutex_unlock(&vi->config_lock); - rtnl_lock(); virtnet_set_queues(vi, vi->curr_queue_pairs); rtnl_unlock(); From 080c637373904258ecc20cedc552b2472ab03d10 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:29 +1030 Subject: [PATCH 08/25] virtio-net: drop config_mutex config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 743fb04a4df9..23e4a69d92de 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -132,9 +132,6 @@ struct virtnet_info { /* Work struct for config space updates */ struct work_struct config_work; - /* Lock for config space updates */ - struct mutex config_lock; - /* Does the affinity hint is set for virtqueues? */ bool affinity_hint_set; @@ -1404,7 +1401,6 @@ static void virtnet_config_changed_work(struct work_struct *work) container_of(work, struct virtnet_info, config_work); u16 v; - mutex_lock(&vi->config_lock); if (virtio_cread_feature(vi->vdev, VIRTIO_NET_F_STATUS, struct virtio_net_config, status, &v) < 0) goto done; @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_tx_stop_all_queues(vi->dev); } done: - mutex_unlock(&vi->config_lock); + return; } static void virtnet_config_changed(struct virtio_device *vdev) @@ -1751,7 +1747,6 @@ static int virtnet_probe(struct virtio_device *vdev) u64_stats_init(&virtnet_stats->rx_syncp); } - mutex_init(&vi->config_lock); INIT_WORK(&vi->config_work, virtnet_config_changed_work); /* If we can receive ANY GSO packets, we must allocate large ones. */ From 507613bf31f4bc0a344a1dfc1bc9074fed6eab8f Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:30 +1030 Subject: [PATCH 09/25] virtio_net: minor cleanup goto done; done: return; is ugly, it was put there to make diff review easier. replace by open-coded return. Signed-off-by: Michael S. Tsirkin Acked-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 23e4a69d92de..ef04d2394282 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1403,7 +1403,7 @@ static void virtnet_config_changed_work(struct work_struct *work) if (virtio_cread_feature(vi->vdev, VIRTIO_NET_F_STATUS, struct virtio_net_config, status, &v) < 0) - goto done; + return; if (v & VIRTIO_NET_S_ANNOUNCE) { netdev_notify_peers(vi->dev); @@ -1414,7 +1414,7 @@ static void virtnet_config_changed_work(struct work_struct *work) v &= VIRTIO_NET_S_LINK_UP; if (vi->status == v) - goto done; + return; vi->status = v; @@ -1425,8 +1425,6 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_carrier_off(vi->dev); netif_tx_stop_all_queues(vi->dev); } -done: - return; } static void virtnet_config_changed(struct virtio_device *vdev) From 3569db593081fd88bbd6df21b9b0531873f2042c Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:30 +1030 Subject: [PATCH 10/25] virtio: add API to enable VQs early virtio spec 0.9.X requires DRIVER_OK to be set before VQs are used, but some drivers use VQs before probe function returns. Since DRIVER_OK is set after probe, this violates the spec. Even though under virtio 1.0 transitional devices support this behaviour, we want to make it possible for those early callers to become spec compliant and eventually support non-transitional devices. Add API for drivers to call before using VQs. Sets DRIVER_OK internally. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- include/linux/virtio_config.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index e8f8f71e843c..7f4ef66873ef 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -109,6 +109,23 @@ struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev, return vq; } +/** + * virtio_device_ready - enable vq use in probe function + * @vdev: the device + * + * Driver must call this to use vqs in the probe function. + * + * Note: vqs are enabled automatically after probe returns. + */ +static inline +void virtio_device_ready(struct virtio_device *dev) +{ + unsigned status = dev->config->get_status(dev); + + BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); + dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK); +} + static inline const char *virtio_bus_name(struct virtio_device *vdev) { From 4baf1e33d0842c9673fef4af207d4b74da8d0126 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:30 +1030 Subject: [PATCH 11/25] virtio_net: enable VQs early virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after probe returns, virtio net violated this rule by using receive VQs within probe. To fix, call virtio_device_ready before using VQs. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index ef04d2394282..aba7b93286b3 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1792,6 +1792,8 @@ static int virtnet_probe(struct virtio_device *vdev) goto free_vqs; } + virtio_device_ready(vdev); + /* Last of all, set up some receive buffers. */ for (i = 0; i < vi->curr_queue_pairs; i++) { try_fill_recv(&vi->rq[i], GFP_KERNEL); From 7a11370e5e6c26566904bb7f08281093a3002ff2 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:30 +1030 Subject: [PATCH 12/25] virtio_blk: enable VQs early virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after probe returns, virtio block violated this rule by calling add_disk, which causes the VQ to be used directly within probe. To fix, call virtio_device_ready before using VQs. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/block/virtio_blk.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 89ba8d6baa94..34ec273b1d46 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -719,6 +719,8 @@ static int virtblk_probe(struct virtio_device *vdev) if (!err && opt_io_size) blk_queue_io_opt(q, blk_size * opt_io_size); + virtio_device_ready(vdev); + add_disk(vblk->disk); err = device_create_file(disk_to_dev(vblk->disk), &dev_attr_serial); if (err) From f5866db64f341776c2d9ed48080f82459fea6a55 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:31 +1030 Subject: [PATCH 13/25] virtio_console: enable VQs early virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after probe returns, virtio console violated this rule by adding inbufs, which causes the VQ to be used directly within probe. To fix, call virtio_device_ready before using VQs. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/char/virtio_console.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index b585b4789822..d0f25bdc7ba5 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 id) spin_lock_init(&port->outvq_lock); init_waitqueue_head(&port->waitqueue); + virtio_device_ready(portdev->vdev); + /* Fill the in_vq with buffers so the host can send us data. */ nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock); if (!nr_added_bufs) { From 64b4cc3911fe8284dfb3cfdb8065c100b818bab8 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:31 +1030 Subject: [PATCH 14/25] 9p/trans_virtio: enable VQs early virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after probe returns, but virtio 9p device adds self to channel list within probe, at which point VQ can be used in violation of the spec. To fix, call virtio_device_ready before using VQs. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- net/9p/trans_virtio.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index 6940d8fe8971..daa749c8b3fb 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -575,6 +575,8 @@ static int p9_virtio_probe(struct virtio_device *vdev) /* Ceiling limit to avoid denial of service attacks */ chan->p9_max_pages = nr_free_buffer_pages()/4; + virtio_device_ready(vdev); + mutex_lock(&virtio_9p_lock); list_add_tail(&chan->chan_list, &virtio_chan_list); mutex_unlock(&virtio_9p_lock); From 024655555021e971203c519770609509e0af4468 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:31 +1030 Subject: [PATCH 15/25] virtio_net: fix use after free on allocation failure In the extremely unlikely event that driver initialization fails after RX buffers are added, virtio net frees RX buffers while VQs are still active, potentially causing device to use a freed buffer. To fix, reset device first - same as we do on device removal. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index aba7b93286b3..53031e58a5fc 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1830,6 +1830,8 @@ static int virtnet_probe(struct virtio_device *vdev) return 0; free_recv_bufs: + vi->vdev->config->reset(vdev); + free_receive_bufs(vi); unregister_netdev(dev); free_vqs: From cd679048958011418f14a8fc7dfdb64ab72ca315 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:31 +1030 Subject: [PATCH 16/25] virtio_scsi: move kick event out from virtscsi_init We currently kick event within virtscsi_init, before host is fully initialized. This can in theory confuse guest if device consumes the buffers immediately. To fix, move virtscsi_kick_event_all out to scan/restore. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/scsi/virtio_scsi.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c index eee1bc0b506e..0642ce387c75 100644 --- a/drivers/scsi/virtio_scsi.c +++ b/drivers/scsi/virtio_scsi.c @@ -853,7 +853,11 @@ static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq, static void virtscsi_scan(struct virtio_device *vdev) { - struct Scsi_Host *shost = (struct Scsi_Host *)vdev->priv; + struct Scsi_Host *shost = virtio_scsi_host(vdev); + struct virtio_scsi *vscsi = shost_priv(shost); + + if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) + virtscsi_kick_event_all(vscsi); scsi_scan_host(shost); } @@ -916,9 +920,6 @@ static int virtscsi_init(struct virtio_device *vdev, virtscsi_config_set(vdev, cdb_size, VIRTIO_SCSI_CDB_SIZE); virtscsi_config_set(vdev, sense_size, VIRTIO_SCSI_SENSE_SIZE); - if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) - virtscsi_kick_event_all(vscsi); - err = 0; out: @@ -1048,8 +1049,13 @@ static int virtscsi_restore(struct virtio_device *vdev) return err; err = register_hotcpu_notifier(&vscsi->nb); - if (err) + if (err) { vdev->config->del_vqs(vdev); + return err; + } + + if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) + virtscsi_kick_event_all(vscsi); return err; } From 6d62c37f1991aafc872f8d8be8ac60e57ede8605 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:32 +1030 Subject: [PATCH 17/25] virtio_blk: enable VQs early on restore virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after restore returns, virtio block violated this rule on restore by restarting queues, which might in theory cause the VQ to be used directly within restore. To fix, call virtio_device_ready before using starting queues. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/block/virtio_blk.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 34ec273b1d46..930fee886917 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -804,10 +804,13 @@ static int virtblk_restore(struct virtio_device *vdev) int ret; ret = init_vq(vdev->priv); - if (!ret) - blk_mq_start_stopped_hw_queues(vblk->disk->queue, true); + if (ret) + return ret; - return ret; + virtio_device_ready(vdev); + + blk_mq_start_stopped_hw_queues(vblk->disk->queue, true); + return 0; } #endif From 52c9cf1ac3d315995e9a65b900bc25e1d8a538b3 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:32 +1030 Subject: [PATCH 18/25] virtio_scsi: enable VQs early on restore virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after restore returns, virtio scsi violated this rule on restore by kicking event vq within restore. To fix, call virtio_device_ready before using event queue. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/scsi/virtio_scsi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c index 0642ce387c75..6a39896b30dc 100644 --- a/drivers/scsi/virtio_scsi.c +++ b/drivers/scsi/virtio_scsi.c @@ -1054,6 +1054,8 @@ static int virtscsi_restore(struct virtio_device *vdev) return err; } + virtio_device_ready(vdev); + if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) virtscsi_kick_event_all(vscsi); From 401bbdc901b268113d7c562616feb7fc37492aca Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:32 +1030 Subject: [PATCH 19/25] virtio_console: enable VQs early on restore virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after resume returns, virtio console violated this rule by adding inbufs, which causes the VQ to be used directly within restore. To fix, call virtio_device_ready before using VQs. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/char/virtio_console.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index d0f25bdc7ba5..bfa640023e64 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -2184,6 +2184,8 @@ static int virtcons_restore(struct virtio_device *vdev) if (ret) return ret; + virtio_device_ready(portdev->vdev); + if (use_multiport(portdev)) fill_queue(portdev->c_ivq, &portdev->c_ivq_lock); From e53fbd11e983e896adaabef2d2f1695d6e0af829 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:32 +1030 Subject: [PATCH 20/25] virtio_net: enable VQs early on restore virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after restore returns, virtio net violated this rule by using receive VQs within restore. To fix, call virtio_device_ready before using VQs. Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck Signed-off-by: Rusty Russell --- drivers/net/virtio_net.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 53031e58a5fc..4e0cbbcd60a7 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1912,6 +1912,8 @@ static int virtnet_restore(struct virtio_device *vdev) if (err) return err; + virtio_device_ready(vdev); + if (netif_running(vi->dev)) { for (i = 0; i < vi->curr_queue_pairs; i++) if (!try_fill_recv(&vi->rq[i], GFP_KERNEL)) From 1fa5b2a784dc52d929432bcc963a0bfb3a74608f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 15 Oct 2014 10:22:33 +1030 Subject: [PATCH 21/25] virito_scsi: use freezable WQ for events Michael S. Tsirkin noticed a race condition: we reset device on freeze, but system WQ is still running so it might try adding bufs to a VQ meanwhile. To fix, switch to handling events from the freezable WQ. Reported-by: Michael S. Tsirkin Signed-off-by: Paolo Bonzini Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/scsi/virtio_scsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c index 6a39896b30dc..29fd44a5d4dd 100644 --- a/drivers/scsi/virtio_scsi.c +++ b/drivers/scsi/virtio_scsi.c @@ -390,7 +390,7 @@ static void virtscsi_complete_event(struct virtio_scsi *vscsi, void *buf) { struct virtio_scsi_event_node *event_node = buf; - schedule_work(&event_node->work); + queue_work(system_freezable_wq, &event_node->work); } static void virtscsi_event_done(struct virtqueue *vq) From e67423c7b4f20c327de533b068907aab33720482 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:33 +1030 Subject: [PATCH 22/25] virtio_scsi: fix race on device removal We cancel event work on device removal, but an interrupt could trigger immediately after this, and queue it again. To fix, set a flag. Loosely based on patch by Paolo Bonzini Signed-off-by: Paolo Bonzini Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/scsi/virtio_scsi.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c index 29fd44a5d4dd..0227d39f45f0 100644 --- a/drivers/scsi/virtio_scsi.c +++ b/drivers/scsi/virtio_scsi.c @@ -110,6 +110,9 @@ struct virtio_scsi { /* CPU hotplug notifier */ struct notifier_block nb; + /* Protected by event_vq lock */ + bool stop_events; + struct virtio_scsi_vq ctrl_vq; struct virtio_scsi_vq event_vq; struct virtio_scsi_vq req_vqs[]; @@ -303,6 +306,11 @@ static void virtscsi_cancel_event_work(struct virtio_scsi *vscsi) { int i; + /* Stop scheduling work before calling cancel_work_sync. */ + spin_lock_irq(&vscsi->event_vq.vq_lock); + vscsi->stop_events = true; + spin_unlock_irq(&vscsi->event_vq.vq_lock); + for (i = 0; i < VIRTIO_SCSI_EVENT_LEN; i++) cancel_work_sync(&vscsi->event_list[i].work); } @@ -390,7 +398,8 @@ static void virtscsi_complete_event(struct virtio_scsi *vscsi, void *buf) { struct virtio_scsi_event_node *event_node = buf; - queue_work(system_freezable_wq, &event_node->work); + if (!vscsi->stop_events) + queue_work(system_freezable_wq, &event_node->work); } static void virtscsi_event_done(struct virtqueue *vq) From 486d2e632ca157558a738626c092973f309f3b45 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:33 +1030 Subject: [PATCH 23/25] virtio_balloon: enable VQs early on restore virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after resume returns, virtio balloon violated this rule by adding bufs, which causes the VQ to be used directly within restore. To fix, call virtio_device_ready before using VQ. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/virtio/virtio_balloon.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 25ebe8eecdb7..f4a28af4865e 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -538,6 +538,8 @@ static int virtballoon_restore(struct virtio_device *vdev) if (ret) return ret; + virtio_device_ready(vdev); + fill_balloon(vb, towards_target(vb)); update_balloon_size(vb); return 0; From 5d8f16d08ba42937ae8c4152d218a77671be4b8f Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:33 +1030 Subject: [PATCH 24/25] virtio_scsi: drop scan callback Enable VQs early like we do for restore. This makes it possible to drop the scan callback, moving scanning into the probe function, and making code simpler. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/scsi/virtio_scsi.c | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c index 0227d39f45f0..b83846fc7859 100644 --- a/drivers/scsi/virtio_scsi.c +++ b/drivers/scsi/virtio_scsi.c @@ -860,17 +860,6 @@ static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq, virtscsi_vq->vq = vq; } -static void virtscsi_scan(struct virtio_device *vdev) -{ - struct Scsi_Host *shost = virtio_scsi_host(vdev); - struct virtio_scsi *vscsi = shost_priv(shost); - - if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) - virtscsi_kick_event_all(vscsi); - - scsi_scan_host(shost); -} - static void virtscsi_remove_vqs(struct virtio_device *vdev) { struct Scsi_Host *sh = virtio_scsi_host(vdev); @@ -1007,10 +996,13 @@ static int virtscsi_probe(struct virtio_device *vdev) err = scsi_add_host(shost, &vdev->dev); if (err) goto scsi_add_host_failed; - /* - * scsi_scan_host() happens in virtscsi_scan() via virtio_driver->scan() - * after VIRTIO_CONFIG_S_DRIVER_OK has been set.. - */ + + virtio_device_ready(vdev); + + if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) + virtscsi_kick_event_all(vscsi); + + scsi_scan_host(shost); return 0; scsi_add_host_failed: @@ -1090,7 +1082,6 @@ static struct virtio_driver virtio_scsi_driver = { .driver.owner = THIS_MODULE, .id_table = id_table, .probe = virtscsi_probe, - .scan = virtscsi_scan, #ifdef CONFIG_PM_SLEEP .freeze = virtscsi_freeze, .restore = virtscsi_restore, From 1bbc26062754b012656d34103215f7552e02b999 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 15 Oct 2014 10:22:33 +1030 Subject: [PATCH 25/25] virtio-rng: refactor probe error handling Code like vi->vq = NULL; kfree(vi) does not make sense. Clean it up, use goto error labels for cleanup. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/char/hw_random/virtio-rng.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index 2e3139eda93b..14e351d094ae 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -105,8 +105,8 @@ static int probe_common(struct virtio_device *vdev) vi->index = index = ida_simple_get(&rng_index_ida, 0, 0, GFP_KERNEL); if (index < 0) { - kfree(vi); - return index; + err = index; + goto err_ida; } sprintf(vi->name, "virtio_rng.%d", index); init_completion(&vi->have_data); @@ -124,13 +124,16 @@ static int probe_common(struct virtio_device *vdev) vi->vq = virtio_find_single_vq(vdev, random_recv_done, "input"); if (IS_ERR(vi->vq)) { err = PTR_ERR(vi->vq); - vi->vq = NULL; - kfree(vi); - ida_simple_remove(&rng_index_ida, index); - return err; + goto err_find; } return 0; + +err_find: + ida_simple_remove(&rng_index_ida, index); +err_ida: + kfree(vi); + return err; } static void remove_common(struct virtio_device *vdev)