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

[Stefan Hajnoczi]

On Fri, Nov 12, 2010 at 9:25 AM, Michael S. Tsirkin <address@hidden> wrote:
> 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 <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.
>
> 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?

For v3 I have switched to a bindings callback.  I wish it wasn't
necessary but the only other ways I can think of catching status
writes are hacks which depend on side-effects too much.

Stefan