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
