From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Gregory Haskins" Subject: Re: [PATCH 05/10] KVM: Adds ability to signal userspace using a file-descriptor Date: Mon, 14 May 2007 08:15:03 -0400 Message-ID: <46481A61.BA47.005A.0@novell.com> References: <20070510123831.10200.4769.stgit@novell1.haskins.net> <20070510124706.10200.68571.stgit@novell1.haskins.net> <46470BF7.5080108@qumranet.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: "Avi Kivity" Return-path: In-Reply-To: <46470BF7.5080108-atKUWr5tajBWk0Htik3J/w@public.gmane.org> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org >>> On Sun, May 13, 2007 at 9:00 AM, in message <46470BF7.5080108-atKUWr5tajBWk0Htik3J/w@public.gmane.org>, Avi Kivity wrote: > Gregory Haskins wrote: >> Signed- off- by: Gregory Haskins >> > > Please include patch descriptions. Ack. On that topic: Does anyone know how to retroactively change the patch comment in StGIT? > >> --- >> >> drivers/kvm/kvm.h | 2 + >> drivers/kvm/kvm_main.c | 82 > ++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 84 insertions(+), 0 deletions(- ) >> >> diff -- git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h >> index 7b5d5e6..1c46830 100644 >> --- a/drivers/kvm/kvm.h >> +++ b/drivers/kvm/kvm.h >> @@ - 333,6 +333,8 @@ struct kvm_vcpu_irq { >> int deferred; >> struct task_struct *task; >> int guest_mode; >> + wait_queue_head_t wq; >> + int usignal; >> }; >> >> struct kvm_vcpu { >> diff -- git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c >> index 3304cce..9a6d2c5 100644 >> --- a/drivers/kvm/kvm_main.c >> +++ b/drivers/kvm/kvm_main.c >> @@ - 40,6 +40,7 @@ >> #include >> #include >> #include >> +#include >> >> #include "x86_emulate.h" >> #include "segment_descriptor.h" >> @@ - 326,6 +327,7 @@ static struct kvm *kvm_create_vm(void) >> memset(&vcpu- >irq, 0, sizeof(vcpu- >irq)); >> spin_lock_init(&vcpu- >irq.lock); >> vcpu- >irq.deferred = - 1; >> + init_waitqueue_head(&vcpu- >irq.wq); >> >> vcpu- >cpu = - 1; >> vcpu- >kvm = kvm; >> @@ - 2288,11 +2290,78 @@ static int kvm_vcpu_release(struct inode *inode, > struct file *filp) >> return 0; >> } >> >> +static unsigned int kvm_vcpu_poll(struct file *filp, poll_table *wait) >> +{ >> + struct kvm_vcpu *vcpu = filp- >private_data; >> + unsigned int events = 0; >> + unsigned long flags; >> + >> + poll_wait(filp, &vcpu- >irq.wq, wait); >> + >> + spin_lock_irqsave(&vcpu- >irq.lock, flags); >> + if (vcpu- >irq.usignal) >> + events |= POLLIN; >> + spin_unlock_irqrestore(&vcpu- >irq.lock, flags); >> + >> + return events; >> +} >> + >> +static ssize_t kvm_vcpu_read(struct file *filp, char __user *buf, size_t > count, >> + loff_t *ppos) >> +{ >> > > Is having a read() (or a write()) actually necessary? Based on what I know: yes. It could be a case of ignorance, however ;) Heres why I think its necessary: You need poll to simply tell you when something is pending. You can't clear the pending status in poll because you cannot predict the internal access pattern (e.g. I assume it could be polled multiple times by the kernel without returning immediately to userspace). Therefore, you need a second method to actually clear the pending "signal", which I use the read() method for. I can be convinced otherwise, but that was my original thinking. > >> + >> + if (indirect_sig && waitqueue_active(&vcpu- >irq.wq)) >> + wake_up(&vcpu- >irq.wq); >> } >> >> > > Did you check that we can actually deliver signals with this? I think a > fasync_struct or something like that is necessary, but not sure. Actually, my signals *didn't* seem to be working, but they werent working with "send_sig()" either so I just assumed I had a userspace coding problem. Based on what I read, it seemed like what I did should work if you do a fcntl(F_SETSIG), etc. But again, it could be ignorance. I am not familiar with fasync_struct. If you have any pointers, please forward. > > Another implementation option (which I've only thought of now, sorry) is > to have an ioctl which returns a real eventfd, reducing some code > duplication. So based on this, I assume eventfd must be in the kernel already? Cool. Even if its not, I like this idea much better than what I did. There was still an unresolved problem regarding how I was going to expose the signaling mechanism to QEMU without giving away the vcpu_fd from the kvmctl library that this solves nicely. With this methodology, I can simply provide a function like "kvm_vcpu_get_eventfd()" in the library, and return the eventfd directly to the QEMU process. Then we dont have to worry about layering violations. Nice! ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/