On Thu, Nov 11, 2010 at 3:53 PM, Michael S. Tsirkin <mst@xxxxxxxxxx> wrote: > On Thu, Nov 11, 2010 at 01:47:21PM +0000, Stefan Hajnoczi wrote: >> Care must be taken not to interfere with vhost-net, which already uses >> ioeventfd host notifiers. The following list shows the behavior implemented in >> this patch and is designed to take vhost-net into account: >> >> * VIRTIO_CONFIG_S_DRIVER_OK -> assign host notifiers, qemu_set_fd_handler(virtio_pci_host_notifier_read) > > we should also deassign when VIRTIO_CONFIG_S_DRIVER_OK is cleared > by io write or bus master bit? You're right, I'll fix the lifecycle to trigger symmetrically on status bit changes rather than VIRTIO_CONFIG_S_DRIVER_OK/reset. >> +static void virtio_pci_reset_vdev(VirtIOPCIProxy *proxy) >> +{ >> + /* Poke virtio device so it deassigns its host notifiers (if any) */ >> + virtio_set_status(proxy->vdev, 0); > > Hmm. virtio_reset already sets status to 0. > I guess it should just be fixed to call virtio_set_status? This part is ugly. The problem is that virtio_reset() calls virtio_set_status(vdev, 0) but doesn't give the transport binding a chance clean up after the virtio device has cleaned up. Since virtio-net will spot status=0 and deassign its host notifier, we need to perform our own clean up after vhost. What makes this slightly less of a hack is the fact that virtio-pci.c was already causing virtio_set_status(vdev, 0) to be invoked twice during reset. When 0 is written to the VIRTIO_PCI_STATUS register, we do virtio_set_status(proxy->vdev, val & 0xFF) and then virtio_reset(proxy->vdev). So the status byte callback already gets invoked twice today. I've just split this out into virtio_pci_reset_vdev() and (ab)used it to correctly clean up virtqueue ioeventfd. The alternative is to add another callback from virtio.c so we are notified after the vdev's reset callback has finished. >> @@ -223,10 +322,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) >> virtio_queue_notify(vdev, val); >> break; >> case VIRTIO_PCI_STATUS: >> - virtio_set_status(vdev, val & 0xFF); >> - if (vdev->status == 0) { >> - virtio_reset(proxy->vdev); >> - msix_unuse_all_vectors(&proxy->pci_dev); >> + if ((val & VIRTIO_CONFIG_S_DRIVER_OK) && >> + !(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && >> + (proxy->flags & VIRTIO_PCI_FLAG_USE_IOEVENTFD)) { >> + virtio_pci_set_host_notifiers(proxy, true); >> + } > > So we set host notifiers to true from here, but to false > only on reset? This seems strange. Should not we disable > notifiers when driver clears OK status? > How about on bus master disable? You're right, this needs to be fixed. >> @@ -714,6 +803,8 @@ static PCIDeviceInfo virtio_info[] = { >> .exit = virtio_net_exit_pci, >> .romfile = "pxe-virtio.bin", >> .qdev.props = (Property[]) { >> + DEFINE_PROP_UINT32("flags", VirtIOPCIProxy, flags, >> + VIRTIO_PCI_FLAG_USE_IOEVENTFD), >> DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3), >> DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features), >> DEFINE_NIC_PROPERTIES(VirtIOPCIProxy, nic), > > This ties interface to an internal macro value. Further, user gets to > tweak other fields in this integer which we don't want. Finally, the > interface is extremely unfriendly. > Please use a bit property instead: DEFINE_PROP_BIT. Will fix in v3. >> diff --git a/hw/virtio.c b/hw/virtio.c >> index a2a657e..f588e29 100644 >> --- a/hw/virtio.c >> +++ b/hw/virtio.c >> @@ -582,6 +582,11 @@ void virtio_queue_notify(VirtIODevice *vdev, int n) >> } >> } >> >> +void virtio_queue_notify_vq(VirtQueue *vq) >> +{ >> + virtio_queue_notify(vq->vdev, vq - vq->vdev->vq); > > Let's implement virtio_queue_notify in terms of virtio_queue_notify_vq. > Not the other way around. Will fix in v3. Stefan -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html