From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60115) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzNub-0000Uh-4R for qemu-devel@nongnu.org; Thu, 19 Nov 2015 07:02:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZzNuV-0002AN-IG for qemu-devel@nongnu.org; Thu, 19 Nov 2015 07:02:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34832) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzNuV-00029s-BO for qemu-devel@nongnu.org; Thu, 19 Nov 2015 07:01:55 -0500 Date: Thu, 19 Nov 2015 14:01:50 +0200 From: "Michael S. Tsirkin" Message-ID: <20151119135725-mutt-send-email-mst@redhat.com> References: <011b01d122af$c60f7ce0$522e76a0$@samsung.com> <20151119125348-mutt-send-email-mst@redhat.com> <014b01d122bf$ab73dc90$025b95b0$@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <014b01d122bf$ab73dc90$025b95b0$@samsung.com> Subject: Re: [Qemu-devel] [PATCH] virtio: Implement userspace forwarding for host notifiers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Fedin Cc: 'Christian Borntraeger' , qemu-devel@nongnu.org, pbonzini@redhat.com 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