On Thu, Jan 12, 2012 at 12:12:17PM +1030, Rusty Russell wrote:
> On Thu, 12 Jan 2012 00:02:33 +0200, "Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> > Look, we have a race currently. Let us not tie a bug fix to a huge
> > rewrite with unclear performance benefits, please.
>
> In theory, yes. In practice, we bandaid it.
>
> I think in the short term we change ->get to get the entire sequence
> twice, and check it's the same. Theoretically, still racy, but it does
> cut the window. And we haven't seen the bug yet, either.
I thought about this some more. Since we always get
an interrupt on config changes, it seems that a rather
robust method would be to just synchronize against that.
Something like the below (warning - completely untested).
Still need to think about memory barriers, overflow etc.
What do you think?
Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 03d1984..b5df385 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -57,6 +57,7 @@ struct virtio_pci_device
unsigned msix_used_vectors;
/* Whether we have vector per vq */
bool per_vq_vectors;
+ atomic_t config_changes;
};
/* Constants for MSI-X */
@@ -125,6 +126,19 @@ static void vp_finalize_features(struct virtio_device *vdev)
iowrite32(vdev->features[0], vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES);
}
+/* wait for pending irq handlers */
+static void vp_synchronize_vectors(struct virtio_device *vdev)
+{
+ struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+ int i;
+
+ if (vp_dev->intx_enabled)
+ synchronize_irq(vp_dev->pci_dev->irq);
+
+ for (i = 0; i < vp_dev->msix_vectors; ++i)
+ synchronize_irq(vp_dev->msix_entries[i].vector);
+}
+
/* virtio config->get() implementation */
static void vp_get(struct virtio_device *vdev, unsigned offset,
void *buf, unsigned len)
@@ -134,9 +148,20 @@ static void vp_get(struct virtio_device *vdev, unsigned offset,
VIRTIO_PCI_CONFIG(vp_dev) + offset;
u8 *ptr = buf;
int i;
-
- for (i = 0; i < len; i++)
- ptr[i] = ioread8(ioaddr + i);
+ int uninitialized_var(c);
+ c = atomic_read(&vp_dev->config_changes);
+ /* Make sure read is done before we get the first config byte */
+ rmb();
+ do {
+ for (i = 0; i < len; i++)
+ ptr[i] = ioread8(ioaddr + i);
+ /* Synchronize with config interrupt */
+ vp_synchronize_vectors(vdev);
+ /*
+ * For multi-byte fields, we might get a config change interrupt
+ * between byte reads. If this happens, retry the read.
+ */
+ } while (c != atomic_read(&vp_dev->config_changes))
}
/* the config->set() implementation. it's symmetric to the config->get()
@@ -169,19 +194,6 @@ static void vp_set_status(struct virtio_device *vdev, u8 status)
iowrite8(status, vp_dev->ioaddr + VIRTIO_PCI_STATUS);
}
-/* wait for pending irq handlers */
-static void vp_synchronize_vectors(struct virtio_device *vdev)
-{
- struct virtio_pci_device *vp_dev = to_vp_device(vdev);
- int i;
-
- if (vp_dev->intx_enabled)
- synchronize_irq(vp_dev->pci_dev->irq);
-
- for (i = 0; i < vp_dev->msix_vectors; ++i)
- synchronize_irq(vp_dev->msix_entries[i].vector);
-}
-
static void vp_reset(struct virtio_device *vdev)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
@@ -213,6 +225,8 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
drv = container_of(vp_dev->vdev.dev.driver,
struct virtio_driver, driver);
+ atomic_inc(&vp_dev->config_changes);
+
if (drv && drv->config_changed)
drv->config_changed(&vp_dev->vdev);
return IRQ_HANDLED;
@@ -646,6 +660,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
vp_dev->vdev.config = &virtio_pci_config_ops;
vp_dev->pci_dev = pci_dev;
INIT_LIST_HEAD(&vp_dev->virtqueues);
+ atomic_set(&vp_dev->config_changes, 0);
spin_lock_init(&vp_dev->lock);
/* Disable MSI/MSIX to bring device to a known good state. */
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
