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

[Stefan Hajnoczi]

On Thu, Nov 11, 2010 at 3:53 PM, Michael S. Tsirkin <address@hidden> 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