Re: [Qemu-devel] [PATCH] virtio: Implement userspace forwarding for host notifiers

[Michael S. Tsirkin]

On Thu, Nov 19, 2015 at 02:44:37PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > OK, that's better, thanks!
> 
>  Yes, indeed this was simple.
> 
> > Why do we need
> >         if (kvm_eventfds_enabled()) {
> >             memory_region_add_eventfd(&proxy->iomem, 
> > VIRTIO_MMIO_QUEUENOTIFY, 4,
> >                                     true, n, notifier);
> >         } else if (!set_handler) {
> >            virtio_queue_set_host_notifier_forwarding(vq);
> >         }
> > everywhere though?
> > 
> > Can't memory_region_add_eventfd DTRT depending on kvm etc?
> 
>  You know... I took a look at this, and yes, i could simply hook up emulation 
> into memory region handlers. And everything that
> expects KVM eventfd binding will magically start working, probably rendering 
> some bypass code obsolete.
>  I have only one concern against this. qemu is a large piece of software, 
> consisting of lots of components. I cannot test absolutely
> everything in every configuration. I suggest, old code was written with the 
> assumption that if memory_region_add_eventfd() works, we
> are really using KVM acceleration. If we break this assumption, how much code 
> will mysteriously misbehave instead of throwing

First of all, memory_region_add_eventfd simply exits on failure.
It seems unlikely you will break something which isn't already
broken.

Further:

$ git grep memory_region_add_eventfd
hw/misc/ivshmem.c:    memory_region_add_eventfd(&s->ivshmem_mmio,
hw/misc/pci-testdev.c:    memory_region_add_eventfd(test->mr,
hw/virtio/virtio-mmio.c:        memory_region_add_eventfd(&proxy->iomem, 
VIRTIO_MMIO_QUEUENOTIFY, 4,
hw/virtio/virtio-pci.c:                memory_region_add_eventfd(modern_mr, 
modern_addr, 0,
hw/virtio/virtio-pci.c:                memory_region_add_eventfd(modern_mr, 
modern_addr, 2,
hw/virtio/virtio-pci.c:                
memory_region_add_eventfd(modern_notify_mr, 0, 2,
hw/virtio/virtio-pci.c:            memory_region_add_eventfd(legacy_mr, 
legacy_addr, 2,
include/exec/memory.h: * memory_region_add_eventfd: Request an eventfd to be 
triggered when a word
include/exec/memory.h:void memory_region_add_eventfd(MemoryRegion *mr,
include/exec/memory.h: * memory_region_add_eventfd() call.
memory.c:void memory_region_add_eventfd(MemoryRegion *mr,

Not such a bit deal to audit all call sites, is it?

Cc memory API maintainer for an opinion.

Paolo, do you see anything wrong with making
memory_region_add_eventfd work (slowly) without kvm ioeventfd support?

> "function not supported" error, which is quite easy to trace down and fix?

That's a problem with switching from vhost to virtio too,
apparently you decided it's not a big deal there,
why a change of heart?

>  What i would refactor, perhaps, is to add a return code to 
> memory_region_add_eventfd(), so that it can signal failure instead of a
> critical abort, this would allow to get rid of kvm_eventfds_enabled() 
> accompanying checks.
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia