Re: [PATCH 05/10] KVM: Adds ability to signal userspace using a file-descriptor

["Gregory Haskins" <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>]

>>> On Sun, May 13, 2007 at  9:00 AM, in message <46470BF7.5080108-atKUWr5tajBWk0Htik3J/w@public.gmane.org>,
Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote: 
> Gregory Haskins wrote:
>> Signed- off- by: Gregory Haskins <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>
>>   
> 
> 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 <linux/file.h>
>>  #include <linux/fs.h>
>>  #include <linux/mount.h>
>> +#include <linux/poll.h>
>>  
>>  #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/