From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] ioeventfd: Introduce KVM_IOEVENTFD_FLAG_PIPE Date: Sun, 03 Jul 2011 20:16:22 +0300 Message-ID: <4E10A3E6.1070606@redhat.com> References: <1309712689-4290-1-git-send-email-levinsasha928@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Ingo Molnar , Marcelo Tosatti , "Michael S. Tsirkin" , Pekka Enberg To: Sasha Levin Return-path: Received: from mx1.redhat.com ([209.132.183.28]:34778 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753104Ab1GCRQe (ORCPT ); Sun, 3 Jul 2011 13:16:34 -0400 In-Reply-To: <1309712689-4290-1-git-send-email-levinsasha928@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: On 07/03/2011 08:04 PM, Sasha Levin wrote: > The new flag allows passing a write side of a pipe instead of an > eventfd to be notified of writes to the specified memory region. > > Instead of signaling an event, the value written to the memory region > is written to the pipe. > > Using a pipe instead of an eventfd is usefull when any value can be > written to the memory region but we're interested in recieving the > actual value instead of just a notification. > > A simple example for practical use is the serial port. we are not > interested in an exit every time a char is written to the port, but > we do need to know what was written so we could handle it on the guest. > --- > include/linux/kvm.h | 2 + > virt/kvm/eventfd.c | 65 +++++++++++++++++++++++++++++++++++--------------- Documentation/virtua/kvm/api.txt +++++++++++++++++ > > @@ -424,6 +425,7 @@ struct _ioeventfd { > struct list_head list; > u64 addr; > int length; > + struct file *pipe; > struct eventfd_ctx *eventfd; In a union with eventfd please. > @@ -481,6 +487,21 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val) > return _val == p->datamatch ? true : false; > } > > +static ssize_t kernel_write(struct file *file, const char *buf, size_t count, > + loff_t pos) > +{ > + mm_segment_t old_fs; > + ssize_t res; > + > + old_fs = get_fs(); > + set_fs(get_ds()); > + /* The cast to a user pointer is valid due to the set_fs() */ > + res = vfs_write(file, (const char __user *)buf, count,&pos); > + set_fs(old_fs); > + > + return res; > +} > + Is there no generic helper for this? Should there be? > /* MMIO/PIO writes trigger an event if the addr/val match */ > static int > ioeventfd_write(struct kvm_io_device *this, gpa_t addr, int len, > @@ -491,7 +512,11 @@ ioeventfd_write(struct kvm_io_device *this, gpa_t addr, int len, > if (!ioeventfd_in_range(p, addr, len, val)) > return -EOPNOTSUPP; > > - eventfd_signal(p->eventfd, 1); > + if (p->pipe) > + kernel_write(p->pipe, val, len, 0); You're writing potentially variable length data. We need a protocol containing address, data, length, and supporting read accesses as well. Is the write guaranteed atomic? We probably need serialization here. > + else > + eventfd_signal(p->eventfd, 1); > + > return 0; > } > > @@ -555,9 +580,11 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) > if (args->flags& ~KVM_IOEVENTFD_VALID_FLAG_MASK) > return -EINVAL; > > - eventfd = eventfd_ctx_fdget(args->fd); > - if (IS_ERR(eventfd)) > - return PTR_ERR(eventfd); > + if (!(args->flags& KVM_IOEVENTFD_FLAG_PIPE)) { > + eventfd = eventfd_ctx_fdget(args->fd); > + if (IS_ERR(eventfd)) > + return PTR_ERR(eventfd); > + } > > p = kzalloc(sizeof(*p), GFP_KERNEL); > if (!p) { > @@ -568,7 +595,11 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) > INIT_LIST_HEAD(&p->list); > p->addr = args->addr; > p->length = args->len; > - p->eventfd = eventfd; > + > + if (args->flags& KVM_IOEVENTFD_FLAG_PIPE) > + p->pipe = fget(args->fd); > + else > + p->eventfd = eventfd; The split logic with the previous hunk isn't nice. Suggest moving the 'else' there, and assigning the whole union here. > list_for_each_entry_safe(p, tmp,&kvm->ioeventfds, list) { > bool wildcard = !(args->flags& KVM_IOEVENTFD_FLAG_DATAMATCH); > > - if (p->eventfd != eventfd || > - p->addr != args->addr || > + if (p->addr != args->addr || > p->length != args->len || > p->wildcard != wildcard) > continue; Why? -- error compiling committee.c: too many arguments to function