[PATCH v4 00/25] virtio: fix spec compliance issues

[Michael S. Tsirkin]

Changes from v4:
rename virtio_enable_vqs_early() to virtio_device_ready()
Note: Rusty requested we add a BUG_ON in the virtio_ring code.
This can be done by a separate patch on top.
Good for bisectability in case BUG_ON starts triggering :)

Rusty, please review this, and consider for this merge window.

This fixes the following virtio spec compliance issues:
1. on restore, drivers use device before setting ACKNOWLEDGE and DRIVER bits
2. on probe, drivers aren't prepared to handle config interrupts
arriving before probe returns
3. on probe, drivers use device before DRIVER_OK it set

Note that 1 is a clear violation of virtio spec 0.9 and 1.0,
so I am proposing the fix for stable. OTOH 2 is against 1.0 rules but
is a known documented bug in many drivers, so let's fix it going
forward, but it does not seem to be worth it to backport
the changes.

An error handling bugfix for virtio-net and a fix for a
theoretical race condition in virtio scsi is also included.

2 is merely a theoretical race condition, but it seems important
to address to make sure that changes to address 3 do not introduce
instability.

I also included patch to drop scan callback use from virtio
scsi: using this callback creates asymmetry between probe
and resume, and we actually had to add code comments so
people can figure out the flow. Code flow becomes clearer
with the new API.

After this change, the only user of the scan callback is virtio rng.
It's possible to modify it trivially and then drop scan callback
from core, but I'm rather inclined to look at ways to
make some rng core changes so that we don't need to
have so many variables tracking device state.
So this is deferred for now.

Michael S. Tsirkin (24):
virtio_pci: fix virtio spec compliance on restore
virtio: unify config_changed handling
virtio-pci: move freeze/restore to virtio core
virtio: defer config changed notifications
virtio_blk: drop config_enable
virtio-blk: drop config_mutex
virtio_net: drop config_enable
virtio-net: drop config_mutex
virtio_net: minor cleanup
virtio: add API to enable VQs early
virtio_net: enable VQs early
virtio_blk: enable VQs early
virtio_console: enable VQs early
9p/trans_virtio: enable VQs early
virtio_net: fix use after free on allocation failure
virtio_scsi: move kick event out from virtscsi_init
virtio_blk: enable VQs early on restore
virtio_scsi: enable VQs early on restore
virtio_console: enable VQs early on restore
virtio_net: enable VQs early on restore
virtio_scsi: fix race on device removal
virtio_balloon: enable VQs early on restore
virtio_scsi: drop scan callback
virtio-rng: refactor probe error handling

Paolo Bonzini (1):
virito_scsi: use freezable WQ for events

include/linux/virtio.h | 14 +++++
include/linux/virtio_config.h | 17 ++++++
drivers/block/virtio_blk.c | 40 ++++----------
drivers/char/hw_random/virtio-rng.c | 15 +++---
drivers/char/virtio_console.c | 4 ++
drivers/misc/mic/card/mic_virtio.c | 6 +--
drivers/net/virtio_net.c | 44 +++++-----------
drivers/s390/kvm/kvm_virtio.c | 9 +---
drivers/s390/kvm/virtio_ccw.c | 6 +--
drivers/scsi/virtio_scsi.c | 42 +++++++++------
drivers/virtio/virtio.c | 102 ++++++++++++++++++++++++++++++++++++
drivers/virtio/virtio_balloon.c | 2 +
drivers/virtio/virtio_mmio.c | 7 +--
drivers/virtio/virtio_pci.c | 33 ++----------
net/9p/trans_virtio.c | 2 +
15 files changed, 206 insertions(+), 137 deletions(-)

[Michael S. Tsirkin]

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: ***@vger.kernel.org
Cc: Amit Shah <***@redhat.com>
Signed-off-by: Michael S. Tsirkin <***@redhat.com>
---
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 3d1463c..add40d0 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;
}

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
include/linux/virtio.h | 2 ++
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 +-----
7 files changed, 17 insertions(+), 28 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index b46671e..3c19bd3 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).
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index f14b600..e647947 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 a134965..6431290 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 d2c0b44..6cbe6ef 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 fed0ce1..3980687 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 c600ccf..ef9a165 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 add40d0..f39f4e7 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;
}

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
include/linux/virtio.h | 6 +++++
drivers/virtio/virtio.c | 54 +++++++++++++++++++++++++++++++++++++++++++++
drivers/virtio/virtio_pci.c | 54 ++-------------------------------------------
3 files changed, 62 insertions(+), 52 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 3c19bd3..8df7ba8 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
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 3980687..8216b73 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 f39f4e7..d34ebfa 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 = {

[Paul Bolle]

Post by Michael S. Tsirkin
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.

This patch landed in today's linux-next (next-20141015).

Post by Michael S. Tsirkin
include/linux/virtio.h | 6 +++++
drivers/virtio/virtio.c | 54 +++++++++++++++++++++++++++++++++++++++++++++
drivers/virtio/virtio_pci.c | 54 ++-------------------------------------------
3 files changed, 62 insertions(+), 52 deletions(-)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 3c19bd3..8df7ba8 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

s/CONFIG_S_FAILED/VIRTIO_CONFIG_S_FAILED/?
Paul Bolle

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
include/linux/virtio.h | 6 ++++++
drivers/virtio/virtio.c | 57 +++++++++++++++++++++++++++++++++++++++++--------
2 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 8df7ba8..5636b11 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_changed: 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_changed;
+ spinlock_t config_lock;
struct device dev;
struct virtio_device_id id;
const struct virtio_config_ops *config;
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 8216b73..2536701 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -117,6 +117,42 @@ 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_changed = 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;
+ __virtio_config_changed(dev);
+ dev->config_changed = false;
+ spin_unlock_irq(&dev->config_lock);
+}
+
static int virtio_dev_probe(struct device *_d)
{
int err, i;
@@ -153,6 +189,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 +201,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 +251,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_changed = 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 +283,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 +334,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);

[Rusty Russell]

Post by Michael S. Tsirkin
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.

But AFAICT you never *read* dev->config_changed.

You unconditionally trigger a config_changed event in
virtio_config_enable(). That's a bit weird, but probably OK.

How about the following change (on top of your patch). I
think the renaming is clearer, and note the added if() test in
virtio_config_enable().

If you approve, I'll fold it in.

Cheers,
Rusty.

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 2536701b098b..df598dd8c5c8 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -122,7 +122,7 @@ 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_changed = true;
+ dev->config_change_pending = true;
else if (drv && drv->config_changed)
drv->config_changed(dev);
}
@@ -148,8 +148,9 @@ static void virtio_config_enable(struct virtio_device *dev)
{
spin_lock_irq(&dev->config_lock);
dev->config_enabled = true;
- __virtio_config_changed(dev);
- dev->config_changed = false;
+ if (dev->config_change_pending)
+ __virtio_config_changed(dev);
+ dev->config_change_pending = false;
spin_unlock_irq(&dev->config_lock);
}

@@ -253,7 +254,7 @@ int register_virtio_device(struct virtio_device *dev)

spin_lock_init(&dev->config_lock);
dev->config_enabled = false;
- dev->config_changed = 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. */
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 5636b119dc25..65261a7244fc 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -80,7 +80,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
* @index: unique position on the virtio bus
* @failed: saved value for CONFIG_S_FAILED bit (for restore)
* @config_enabled: configuration change reporting enabled
- * @config_changed: configuration change reported while disabled
+ * @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).
@@ -94,7 +94,7 @@ struct virtio_device {
int index;
bool failed;
bool config_enabled;
- bool config_changed;
+ bool config_change_pending;
spinlock_t config_lock;
struct device dev;
struct virtio_device_id id;

[Michael S. Tsirkin]

Post by Rusty Russell

Post by Michael S. Tsirkin
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.

But AFAICT you never *read* dev->config_changed.
You unconditionally trigger a config_changed event in
virtio_config_enable(). That's a bit weird, but probably OK.
How about the following change (on top of your patch). I
think the renaming is clearer, and note the added if() test in
virtio_config_enable().
If you approve, I'll fold it in.
Cheers,
Rusty.

Hi Rusty,
I'm okay with both changes.
You can fold it in if you prefer, or just make it a patch on top.

Post by Rusty Russell
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 2536701b098b..df598dd8c5c8 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -122,7 +122,7 @@ 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_changed = true;
+ dev->config_change_pending = true;
else if (drv && drv->config_changed)
drv->config_changed(dev);
}
@@ -148,8 +148,9 @@ static void virtio_config_enable(struct virtio_device *dev)
{
spin_lock_irq(&dev->config_lock);
dev->config_enabled = true;
- __virtio_config_changed(dev);
- dev->config_changed = false;
+ if (dev->config_change_pending)
+ __virtio_config_changed(dev);
+ dev->config_change_pending = false;
spin_unlock_irq(&dev->config_lock);
}
@@ -253,7 +254,7 @@ int register_virtio_device(struct virtio_device *dev)
spin_lock_init(&dev->config_lock);
dev->config_enabled = false;
- dev->config_changed = 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. */
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 5636b119dc25..65261a7244fc 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -80,7 +80,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
@@ -94,7 +94,7 @@ struct virtio_device {
int index;
bool failed;
bool config_enabled;
- bool config_changed;
+ bool config_change_pending;
spinlock_t config_lock;
struct device dev;
struct virtio_device_id id;

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
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 0a58140..91272f1a 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);

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
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 91272f1a..89ba8d6 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);

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
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 59caa06..743fb04 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();

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
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 e8f8f71..e36403b 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)
{

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
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 743fb04..23e4a69 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. */

[Michael S. Tsirkin]

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 <***@redhat.com>
Acked-by: Cornelia Huck <***@de.ibm.com>
---
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 23e4a69..ef04d23 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)

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
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 89ba8d6..46b04bf 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)

[Michael S. Tsirkin]

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 <***@redhat.com>
---
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 b585b47..6ebe8f6 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) {

[Thomas Graf]

Post by Michael S. Tsirkin
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.
---
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 b585b47..6ebe8f6 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) {

Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK

1.839658] kernel BUG at include/linux/virtio_config.h:125!
[ 1.839995] invalid opcode: 0000 [#1] SMP
[ 1.840169] Modules linked in: serio_raw virtio_balloon pcspkr virtio_net virtio_console soundcore parport_pc floppy pvpanic parport i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc qxl drm_kms_helper ttm drm virtio_blk i2c_core virtio_pci ata_generic virtio_ring virtio pata_acpi
[ 1.840169] CPU: 2 PID: 266 Comm: kworker/2:2 Not tainted 3.17.0+ #1
[ 1.840169] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 1.840169] Workqueue: events control_work_handler [virtio_console]
[ 1.840169] task: ffff8800364bc840 ti: ffff880078490000 task.ti: ffff880078490000
[ 1.840169] RIP: 0010:[<ffffffffa01d92c6>] [<ffffffffa01d92c6>] virtio_device_ready.part.12+0x4/0x6 [virtio_console]
[ 1.840169] RSP: 0018:ffff880078493c78 EFLAGS: 00010202
[ 1.840169] RAX: 0000000000000007 RBX: ffff880036406200 RCX: 0000000000006e39
[ 1.840169] RDX: 000000000000c0b2 RSI: 0000000000000000 RDI: 000000000001c0b2
[ 1.840169] RBP: ffff880078493c78 R08: ffffffff81d2c6f8 R09: 0000000000000000
[ 1.840169] R10: 0000000000000001 R11: ffff8800364bd000 R12: ffff880036618400
[ 1.840169] R13: 0000000000000001 R14: ffff8800368c6800 R15: ffff880036406220
[ 1.840169] FS: 0000000000000000(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000
[ 1.840169] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1.840169] CR2: 00007f1c31c90000 CR3: 0000000001c14000 CR4: 00000000000006e0
[ 1.840169] Stack:
[ 1.840169] ffff880078493ce8 ffffffffa01d886a ffff880000000001 ffffffff810e20cd
[ 1.840169] ffff880078493cb8 0000000000000282 0000000000000000 0000000087f90194
[ 1.840169] ffff880078493ce8 ffff88007ab1d4e0 ffff880036618498 ffff880036618410
[ 1.840169] Call Trace:
[ 1.840169] [<ffffffffa01d886a>] add_port+0x40a/0x440 [virtio_console]
[ 1.840169] [<ffffffff810e20cd>] ? trace_hardirqs_on+0xd/0x10
[ 1.840169] [<ffffffffa01d8c6a>] control_work_handler+0x3ca/0x420 [virtio_console]
[ 1.840169] [<ffffffff810b0e7b>] ? process_one_work+0x15b/0x530
[ 1.840169] [<ffffffff810b0ef4>] process_one_work+0x1d4/0x530
[ 1.840169] [<ffffffff810b0e7b>] ? process_one_work+0x15b/0x530
[ 1.840169] [<ffffffff810b136b>] worker_thread+0x11b/0x490
[ 1.840169] [<ffffffff810b1250>] ? process_one_work+0x530/0x530
[ 1.840169] [<ffffffff810b67c3>] kthread+0xf3/0x110
[ 1.840169] [<ffffffff81788f00>] ? _raw_spin_unlock_irq+0x30/0x50
[ 1.840169] [<ffffffff810b66d0>] ? kthread_create_on_node+0x240/0x240
[ 1.840169] [<ffffffff81789a7c>] ret_from_fork+0x7c/0xb0
[ 1.840169] [<ffffffff810b66d0>] ? kthread_create_on_node+0x240/0x240
[ 1.840169] Code: ff 49 89 c4 4d 85 e4 0f 8f 25 ff ff ff eb ad 48 c7 c0 f4 ff ff ff e9 17 ff ff ff e8 f5 cd eb e0 90 55 48 89 e5 0f 0b 55 48 89 e5 <0f> 0b 55 48 89 e5 0f 0b 55 48 89 e5 e8 99 e2 ff ff 48 c7 c7 c0
[ 1.840169] RIP [<ffffffffa01d92c6>] virtio_device_ready.part.12+0x4/0x6 [virtio_console]
[ 1.840169] RSP <ffff880078493c78>

[Cornelia Huck]

On Mon, 20 Oct 2014 13:07:50 +0100

Post by Thomas Graf

Post by Michael S. Tsirkin
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.
---
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 b585b47..6ebe8f6 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) {

Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK

I think we need to set this in the probe function instead, otherwise we
fail for multiqueue (which also wants to use the control queue early).

Completely untested patch below; I can send this with proper s-o-b if
it helps.

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index bfa6400..cf7a561 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1449,8 +1449,6 @@ 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) {
@@ -2026,6 +2024,8 @@ static int virtcons_probe(struct virtio_device *vdev)
spin_lock_init(&portdev->ports_lock);
INIT_LIST_HEAD(&portdev->ports);

+ virtio_device_ready(portdev->vdev);
+
if (multiport) {
unsigned int nr_added_bufs;

[Thomas Graf]

Post by Cornelia Huck
On Mon, 20 Oct 2014 13:07:50 +0100

Post by Thomas Graf

Post by Michael S. Tsirkin
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.
---
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 b585b47..6ebe8f6 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) {

Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK

I think we need to set this in the probe function instead, otherwise we
fail for multiqueue (which also wants to use the control queue early).
Completely untested patch below; I can send this with proper s-o-b if
it helps.

virtio_dev_probe() already sets DRIVER_OK if ->probe() returned
without an error. I assume Michael added it to add_port() because
probing doesn't always occur first. Can we just remove the BUG_ON()?
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Michael S. Tsirkin]

Post by Cornelia Huck
On Mon, 20 Oct 2014 13:07:50 +0100

Post by Thomas Graf

Post by Michael S. Tsirkin
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.
---
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 b585b47..6ebe8f6 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) {

Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK

I think we need to set this in the probe function instead, otherwise we
fail for multiqueue (which also wants to use the control queue early).
Completely untested patch below; I can send this with proper s-o-b if
it helps.
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index bfa6400..cf7a561 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1449,8 +1449,6 @@ 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) {
@@ -2026,6 +2024,8 @@ static int virtcons_probe(struct virtio_device *vdev)
spin_lock_init(&portdev->ports_lock);
INIT_LIST_HEAD(&portdev->ports);
+ virtio_device_ready(portdev->vdev);
+
if (multiport) {
unsigned int nr_added_bufs;

I wanted to set DRIVER_OK as late as possible, to both reduce
the chance it can fail after DRIVER_OK and to reduce the risk of
introducing a regression since old qemu might only start sending
interrupts after DRIVER_OK is set.

So I wanted everything completely initialized before DRIVER_OK.

You patch makes sense to me since nothing can fail,
and we won't get interrupts before we add inbufs.

Reviewed-by: Michael S. Tsirkin <***@redhat.com>

Testig will report shortly, pls send with sob line.

[Michael S. Tsirkin]

Post by Michael S. Tsirkin

Post by Cornelia Huck
On Mon, 20 Oct 2014 13:07:50 +0100

Post by Thomas Graf

Post by Michael S. Tsirkin
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.
---
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 b585b47..6ebe8f6 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) {

Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK

I think we need to set this in the probe function instead, otherwise we
fail for multiqueue (which also wants to use the control queue early).
Completely untested patch below; I can send this with proper s-o-b if
it helps.
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index bfa6400..cf7a561 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1449,8 +1449,6 @@ 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) {
@@ -2026,6 +2024,8 @@ static int virtcons_probe(struct virtio_device *vdev)
spin_lock_init(&portdev->ports_lock);
INIT_LIST_HEAD(&portdev->ports);
+ virtio_device_ready(portdev->vdev);
+
if (multiport) {
unsigned int nr_added_bufs;

I wanted to set DRIVER_OK as late as possible, to both reduce
the chance it can fail after DRIVER_OK and to reduce the risk of
introducing a regression since old qemu might only start sending
interrupts after DRIVER_OK is set.
So I wanted everything completely initialized before DRIVER_OK.
You patch makes sense to me since nothing can fail,
and we won't get interrupts before we add inbufs.
Testig will report shortly, pls send with sob line.

Sure enough, this helps:

Tested-by: Michael S. Tsirkin <***@redhat.com>
Acked-by: Michael S. Tsirkin <***@redhat.com>

Pls repost as a top-level patch.

Post by Michael S. Tsirkin
--
MST

[Thomas Graf]

Post by Michael S. Tsirkin
Pls repost as a top-level patch.

Thanks for fixing this so promptly.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Michael S. Tsirkin]

Post by Thomas Graf

Post by Michael S. Tsirkin
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.
---
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 b585b47..6ebe8f6 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) {

I see Cornelia sent a patch already.
I'd like to reproduce this though - could you send me
the command line please?

Post by Thomas Graf
Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK
1.839658] kernel BUG at include/linux/virtio_config.h:125!
[ 1.839995] invalid opcode: 0000 [#1] SMP
[ 1.840169] Modules linked in: serio_raw virtio_balloon pcspkr virtio_net virtio_console soundcore parport_pc floppy pvpanic parport i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc qxl drm_kms_helper ttm drm virtio_blk i2c_core virtio_pci ata_generic virtio_ring virtio pata_acpi
[ 1.840169] CPU: 2 PID: 266 Comm: kworker/2:2 Not tainted 3.17.0+ #1
[ 1.840169] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 1.840169] Workqueue: events control_work_handler [virtio_console]
[ 1.840169] task: ffff8800364bc840 ti: ffff880078490000 task.ti: ffff880078490000
[ 1.840169] RIP: 0010:[<ffffffffa01d92c6>] [<ffffffffa01d92c6>] virtio_device_ready.part.12+0x4/0x6 [virtio_console]
[ 1.840169] RSP: 0018:ffff880078493c78 EFLAGS: 00010202
[ 1.840169] RAX: 0000000000000007 RBX: ffff880036406200 RCX: 0000000000006e39
[ 1.840169] RDX: 000000000000c0b2 RSI: 0000000000000000 RDI: 000000000001c0b2
[ 1.840169] RBP: ffff880078493c78 R08: ffffffff81d2c6f8 R09: 0000000000000000
[ 1.840169] R10: 0000000000000001 R11: ffff8800364bd000 R12: ffff880036618400
[ 1.840169] R13: 0000000000000001 R14: ffff8800368c6800 R15: ffff880036406220
[ 1.840169] FS: 0000000000000000(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000
[ 1.840169] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1.840169] CR2: 00007f1c31c90000 CR3: 0000000001c14000 CR4: 00000000000006e0
[ 1.840169] ffff880078493ce8 ffffffffa01d886a ffff880000000001 ffffffff810e20cd
[ 1.840169] ffff880078493cb8 0000000000000282 0000000000000000 0000000087f90194
[ 1.840169] ffff880078493ce8 ffff88007ab1d4e0 ffff880036618498 ffff880036618410
[ 1.840169] [<ffffffffa01d886a>] add_port+0x40a/0x440 [virtio_console]
[ 1.840169] [<ffffffff810e20cd>] ? trace_hardirqs_on+0xd/0x10
[ 1.840169] [<ffffffffa01d8c6a>] control_work_handler+0x3ca/0x420 [virtio_console]
[ 1.840169] [<ffffffff810b0e7b>] ? process_one_work+0x15b/0x530
[ 1.840169] [<ffffffff810b0ef4>] process_one_work+0x1d4/0x530
[ 1.840169] [<ffffffff810b0e7b>] ? process_one_work+0x15b/0x530
[ 1.840169] [<ffffffff810b136b>] worker_thread+0x11b/0x490
[ 1.840169] [<ffffffff810b1250>] ? process_one_work+0x530/0x530
[ 1.840169] [<ffffffff810b67c3>] kthread+0xf3/0x110
[ 1.840169] [<ffffffff81788f00>] ? _raw_spin_unlock_irq+0x30/0x50
[ 1.840169] [<ffffffff810b66d0>] ? kthread_create_on_node+0x240/0x240
[ 1.840169] [<ffffffff81789a7c>] ret_from_fork+0x7c/0xb0
[ 1.840169] [<ffffffff810b66d0>] ? kthread_create_on_node+0x240/0x240
[ 1.840169] Code: ff 49 89 c4 4d 85 e4 0f 8f 25 ff ff ff eb ad 48 c7 c0 f4 ff ff ff e9 17 ff ff ff e8 f5 cd eb e0 90 55 48 89 e5 0f 0b 55 48 89 e5 <0f> 0b 55 48 89 e5 0f 0b 55 48 89 e5 e8 99 e2 ff ff 48 c7 c7 c0
[ 1.840169] RIP [<ffffffffa01d92c6>] virtio_device_ready.part.12+0x4/0x6 [virtio_console]
[ 1.840169] RSP <ffff880078493c78>

[Michael S. Tsirkin]

Post by Michael S. Tsirkin

Post by Michael S. Tsirkin
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.
---
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 b585b47..6ebe8f6 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) {

I see Cornelia sent a patch already.
I'd like to reproduce this though - could you send me
the command line please?

Nevermind, the trick is to add a port it seems:

-device virtio-serial -chardev socket,path=/tmp/c1,server,nowait,id=foo
-device virtserialport,chardev=foo,name=org.fedoraproject.port.0

works fine without -device virtserialport.

[Thomas Graf]

Post by Michael S. Tsirkin

Post by Michael S. Tsirkin
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.
---
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 b585b47..6ebe8f6 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) {

I see Cornelia sent a patch already.
I'd like to reproduce this though - could you send me
the command line please?

1. Invoke qemu:

/usr/bin/qemu-system-x86_64 -machine accel=kvm -name f20-2 -S -machine
pc-i440fx-1.4,accel=kvm,usb=off -m 2048 -realtime mlock=off -smp
4,sockets=4,cores=1,threads=1 -uuid
dd45ec4c-7c26-4b73-9b6b-81f2912c5235 -no-user-config -nodefaults
-chardev
socket,id=charmonitor,path=/var/lib/libvirt/qemu/f20-2.monitor,server,nowait
-mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc
-no-shutdown -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2
-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive
file=/virt/f20n2-clone,if=none,id=drive-virtio-disk0,format=raw
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
-netdev tap,fd=24,id=hostnet0,vhost=on,vhostfd=25 -device
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:ef:72:4e,bus=pci.0,addr=0x3
-chardev pty,id=charserial0 -device
isa-serial,chardev=charserial0,id=serial0 -chardev
spicevmc,id=charchannel0,name=vdagent -device
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
-device usb-tablet,id=input0 -spice
port=5900,addr=127.0.0.1,disable-ticketing,seamless-migration=on
-device
qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,addr=0x2
-device intel-hda,id=sound0,bus=pci.0,addr=0x4 -device
hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -device
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7

2. Attach console right away: virsh console f20-2
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Michael S. Tsirkin]

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 <***@redhat.com>
---
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 eee1bc0..0642ce3 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;
}

[Michael S. Tsirkin]

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 <***@redhat.com>
---
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 46b04bf..1c95af5 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;
+
+ virtio_device_ready(vdev);

- return ret;
+ blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
+ return 0;
}
#endif

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
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 ef04d23..430f3ae 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);

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
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 3551417..6b6e136 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))

[Michael S. Tsirkin]

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 <***@redhat.com>
---
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 0642ce3..2b565b3 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);

[Michael S. Tsirkin]

From: Paolo Bonzini <***@redhat.com>

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 <***@redhat.com>
Signed-off-by: Paolo Bonzini <***@redhat.com>
Signed-off-by: Michael S. Tsirkin <***@redhat.com>
---
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 2b565b3..501838d 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)

[Michael S. Tsirkin]

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 <***@redhat.com>
---
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 25ebe8e..9629fad 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;

[Michael S. Tsirkin]

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 <***@redhat.com>
---
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 6940d8f..766ba48 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);

[Michael S. Tsirkin]

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 <***@redhat.com>
Reviewed-by: Cornelia Huck <***@de.ibm.com>
---
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 430f3ae..3551417 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:

[Michael S. Tsirkin]

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 <***@redhat.com>
---
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 327eba0..5f022ff 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,

[Michael S. Tsirkin]

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 <***@redhat.com>
---
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 132c9cc..72295ea 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -109,8 +109,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);
@@ -128,13 +128,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)

[Michael S. Tsirkin]

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 <***@redhat.com>
---
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 6ebe8f6..2ae843f 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);

[Michael S. Tsirkin]

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 <***@redhat.com>
Signed-off-by: Michael S. Tsirkin <***@redhat.com>
---
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 501838d..327eba0 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)