Re: [KVM PATCH v2 2/2] kvm: add support for irqfd via eventfd-notificationinterface

[Gregory Haskins]

Avi Kivity wrote:
> Gregory Haskins wrote:
>> This allows an eventfd to be registered as an irq source with a
>> guest.  Any
>> signaling operation on the eventfd (via userspace or kernel) will inject
>> the registered GSI at the next available window.
>>
>>  
>> +struct kvm_irqfd {
>> +    __u32 fd;
>> +    __u32 gsi;
>> +};
>> +
>>   
>
> I think it's better to have ioctl create and return the fd.  This way
> we aren't tied to eventfd (though it makes a lot of sense to use it).

I dont mind either way, but I am not sure it buys us much as the one
driving the fd would need to understand if the interface is
eventfd-esque or something else anyway.  Let me know if you still want
to see this changed.

>
> Also, please add a flags field and some padding so we can extend it
> later.
>

Good idea.  Will do.

>> +
>> +#include <linux/kvm_host.h>
>> +#include <linux/eventfd.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/wait.h>
>> +#include <linux/poll.h>
>> +#include <linux/file.h>
>> +#include <linux/list.h>
>> +
>> +struct _irqfd {
>> +    struct kvm               *kvm;
>> +    int                       gsi;
>> +    struct file              *file;
>> +    struct list_head          list;
>> +    poll_table                pt;
>> +    wait_queue_head_t        *wqh;
>> +    wait_queue_t              wait;
>> +    struct work_struct        work;
>> +};
>> +
>> +static void
>> +irqfd_inject(struct work_struct *work)
>> +{
>> +    struct _irqfd *irqfd = container_of(work, struct _irqfd, work);
>> +    struct kvm *kvm = irqfd->kvm;
>> +
>> +    mutex_lock(&kvm->lock);
>> +    kvm_set_irq(kvm, kvm->irqfd.src, irqfd->gsi, 1);
>>   
>
> Need to lower the irq too (though irqfd only supports edge triggered
> interrupts).
>
Should I just do back-to-back 1+0 inside the same lock?

>> +    mutex_unlock(&kvm->lock);
>> +}
>> +
>> +static int
>> +irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
>> +{
>> +    struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
>> +
>> +    /*
>> +     * The eventfd calls its wake_up with interrupts disabled,
>> +     * so we need to defer the IRQ injection until later since we need
>> +     * to acquire the kvm->lock to do so.
>> +     */
>> +    schedule_work(&irqfd->work);
>> +
>> +    return 0;
>> +}
>>   
>
> One day we'll have lockless injection and we'll want to drop this.  I
> guess if we create the fd ourselves we can make it work, but I don't
> see how we can do this with eventfd.
>

Hmm...this is a good point.  There probably is no way to use eventfd
"off the shelf" in a way that doesn't cause this callback to be in a
critical section.  Should we just worry about switching away from
eventfd when this occurs, or should I implement a custom anon-fd now?


>> +int
>> +kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
>> +{
>> +    struct _irqfd *irqfd;
>> +    struct file *file;
>> +    int ret;
>> +
>> +    irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
>> +    if (!irqfd)
>> +        return -ENOMEM;
>> +
>> +    irqfd->kvm = kvm;
>> +    irqfd->gsi = gsi;
>> +    INIT_LIST_HEAD(&irqfd->list);
>> +    init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
>> +    init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
>> +    INIT_WORK(&irqfd->work, irqfd_inject);
>> +
>> +    file = eventfd_fget(fd);
>> +    if (IS_ERR(file)) {
>> +        ret = PTR_ERR(file);
>> +        goto fail;
>> +    }
>> +
>> +    ret = file->f_op->poll(file, &irqfd->pt);
>> +    /* do we need to look for errors in ret? */
>>   
>
> Do we?

Probably.  Will fix in v3.

>
>> +
>> +    irqfd->file = file;
>> +
>> +    mutex_lock(&kvm->lock);
>> +    if (kvm->irqfd.src == -1) {
>> +        ret = kvm_request_irq_source_id(kvm);
>> +        BUG_ON(ret < 0);
>>   
>
> I think you can reuse the userspace irq source (since it's just
> another way for userspace to inject an interrupt).  It isn't really
> needed since the irq source stuff is only needed to support level
> triggered interrupts.
>
Ack, will do.

Thanks Avi,
-Greg

Attachment: signature.asc
Description: OpenPGP digital signature