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
