Re: [PATCH 2/3] virtio-pci: Use ioeventfd for virtqueue notify

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Nov 12, 2010 at 09:18:48AM +0000, Stefan Hajnoczi wrote:
> On Thu, Nov 11, 2010 at 3:53 PM, Michael S. Tsirkin <mst@redhat.com> 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.

Oh, likely not worth it. Mabe put the above explanation in the comment.
Will this go away now that you move to set notifiers on status write?

> >> @@ -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