On Fri, Jan 13, 2012 at 01:32:42PM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2012-01-13 at 04:19 +0200, Michael S. Tsirkin wrote:
> > 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?
>
> Your interrupt may take an unpredictable amount of time to arrive, I
> don't see how you can use that as a guarantee.
>
> Cheers,
> Ben.
In theory yes.
Practically, under current KVM, what would delay it?
> > 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
