kvm: fix spurious interrupt with irqfd
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/2 kvm: fix spurious interrupt with irqfd
kvm didn't clear irqfd counter on deassign, as a result we could get a spurious interrupt when irqfd is assigned back. this leads to poor performance and, in theory, guest crash.
--- virt/kvm/eventfd.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
virt/kvm/eventfd.c +2 -1
@@ -72,12 +72,13 @@ static void irqfd_shutdown(struct work_struct *work) { struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown); +u64 cnt;/* * Synchronize with the wait-queue and unhook ourselves to prevent * further events. */ -remove_wait_queue (irqfd->wqh, &irqfd->wait); +eventfd_ctx_remove_wait_queue (irqfd->eventfd, &irqfd->wait, &cnt);/* * We know no new events will be scheduled at this point, so block
Signature · 1 line
1.6.6.144.g5c3af
Quoted 18 lines · Michael S. Tsirkin
kvm didn't clear irqfd counter on deassign, as a result we could get a spurious interrupt when irqfd is assigned back. this leads to poor performance and, in theory, guest crash.
Signed-off-by Michael S. Tsirkin <[email protected]>--- virt/kvm/eventfd.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)virt/kvm/eventfd.c +1
-0@@ -72,12 +72,13 @@ static void irqfd_shutdown(struct work_struct *work) { struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown); +u64 cnt;
virt/kvm/eventfd.c static void · line 84
Quoted 10 lines
virt/kvm/eventfd.c +1
-1/* * Synchronize with the wait-queue and unhook ourselves to prevent * further events. */ -remove_wait_queue (irqfd->wqh, &irqfd->wait); +eventfd_ctx_remove_wait_queue (irqfd->eventfd, &irqfd->wait, &cnt);/* * We know no new events will be scheduled at this point, so block
For kvm-kmod, I'm fighting with compat support for eventfd_ctx_remove_wait_queue. I basically have a solution for kernels with CONFIG_KPROBES enabled (I need to look up unexported __wake_up_locked[_key]), but there will also be target kernels that do not have this. So there are three options for that case:
- Warn the user and fall back to the old racy approach
- (Somehow) disable KVM subsystems that use eventfd
- Refuse to start KVM
As far as I understood, irqfd is interesting for device assignment and now also for vhost, right? What about ioeventfd? I just wonder how broad the impact of a broken or non-existent eventfd subsystem for kvm-kmod is. Any thoughts welcome.
Jan
PS: If anyone forgot why Avi handed over this job, you should now remember why. :)
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 36 lines · Jan Kiszka
Quoted 18 lines · Michael S. Tsirkin
kvm didn't clear irqfd counter on deassign, as a result we could get a spurious interrupt when irqfd is assigned back. this leads to poor performance and, in theory, guest crash.
Signed-off-by Michael S. Tsirkin <[email protected]>--- virt/kvm/eventfd.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)virt/kvm/eventfd.c +1
-0@@ -72,12 +72,13 @@ static void irqfd_shutdown(struct work_struct *work) { struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown); +u64 cnt;virt/kvm/eventfd.c static void · line 84
Quoted 10 lines
virt/kvm/eventfd.c +1
-1/* * Synchronize with the wait-queue and unhook ourselves to prevent * further events. */ -remove_wait_queue (irqfd->wqh, &irqfd->wait); +eventfd_ctx_remove_wait_queue (irqfd->eventfd, &irqfd->wait, &cnt);/* * We know no new events will be scheduled at this point, so block For kvm-kmod, I'm fighting with compat support for
eventfd_ctx_remove_wait_queue. I basically have a solution for kernels withCONFIG_KPROBESenabled (I need to look up unexported __wake_up_locked[_key]), but there will also be target kernels that do not have this. So there are three options for that case:
Quoted 5 lines
- Warn the user and fall back to the old racy approach
- (Somehow) disable KVM subsystems that use eventfd
- Refuse to start KVM
As far as I understood, irqfd is interesting for device assignment and now also for vhost, right?
At the moment, only vhost.
Quoted 1 line
What about ioeventfd?
Same thing.
Quoted 3 lines
I just wonder how broad
the impact of a broken or non-existent eventfd subsystem for kvm-kmod is. Any thoughts welcome.
How do you handle kernels that don't export eventfd_ctx_fileget?
Quoted 4 lines
Jan
PS: If anyone forgot why Avi handed over this job, you should now remember why. :)
Heh, I did the same kind of thing for infiniband for several years. It's hard to forget.
Quoted 3 lines
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 42 lines · Michael S. Tsirkin
Quoted 35 lines · Jan Kiszka
Quoted 18 lines · Michael S. Tsirkin
kvm didn't clear irqfd counter on deassign, as a result we could get a spurious interrupt when irqfd is assigned back. this leads to poor performance and, in theory, guest crash.
Signed-off-by Michael S. Tsirkin <[email protected]>--- virt/kvm/eventfd.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)virt/kvm/eventfd.c +1
-0@@ -72,12 +72,13 @@ static void irqfd_shutdown(struct work_struct *work) { struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown); +u64 cnt;virt/kvm/eventfd.c static void · line 84
Quoted 10 lines
virt/kvm/eventfd.c +1
-1/* * Synchronize with the wait-queue and unhook ourselves to prevent * further events. */ -remove_wait_queue (irqfd->wqh, &irqfd->wait); +eventfd_ctx_remove_wait_queue (irqfd->eventfd, &irqfd->wait, &cnt);/* * We know no new events will be scheduled at this point, so block For kvm-kmod, I'm fighting with compat support for
eventfd_ctx_remove_wait_queue. I basically have a solution for kernels withCONFIG_KPROBESenabled (I need to look up unexported __wake_up_locked[_key]), but there will also be target kernels that do not have this. So there are three options for that case:Quoted 5 lines
- Warn the user and fall back to the old racy approach
- (Somehow) disable KVM subsystems that use eventfd
- Refuse to start KVM
As far as I understood, irqfd is interesting for device assignment and now also for vhost, right?
Quoted 6 lines
At the moment, only vhost.
Quoted 1 line
What about ioeventfd?
Same thing.
OK...
Quoted 5 lines
Quoted 3 lines
I just wonder how broad
the impact of a broken or non-existent eventfd subsystem for kvm-kmod is. Any thoughts welcome.How do you handle kernels that don't export
eventfd_ctx_fileget?
Now that you mention it: not yet properly. So far we pass the file struct as pseudo eventfd_ctx around on < 2.6.31. But now that I peek into the struct in kvm_eventfd_ctx_remove_wait_queue, this should should crash. Guess I need to look up that module the same way as I acquire __wake_up_locked[_key].
Quoted 9 lines
Quoted 5 lines
Jan
PS: If anyone forgot why Avi handed over this job, you should now remember why. :)
Heh, I did the same kind of thing for infiniband for several years. It's hard to forget.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 56 lines · Jan Kiszka
Quoted 42 lines · Michael S. Tsirkin
Quoted 35 lines · Jan Kiszka
Quoted 18 lines · Michael S. Tsirkin
kvm didn't clear irqfd counter on deassign, as a result we could get a spurious interrupt when irqfd is assigned back. this leads to poor performance and, in theory, guest crash.
Signed-off-by Michael S. Tsirkin <[email protected]>--- virt/kvm/eventfd.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)virt/kvm/eventfd.c +1
-0@@ -72,12 +72,13 @@ static void irqfd_shutdown(struct work_struct *work) { struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown); +u64 cnt;virt/kvm/eventfd.c static void · line 84
Quoted 10 lines
virt/kvm/eventfd.c +1
-1/* * Synchronize with the wait-queue and unhook ourselves to prevent * further events. */ -remove_wait_queue (irqfd->wqh, &irqfd->wait); +eventfd_ctx_remove_wait_queue (irqfd->eventfd, &irqfd->wait, &cnt);/* * We know no new events will be scheduled at this point, so block For kvm-kmod, I'm fighting with compat support for
eventfd_ctx_remove_wait_queue. I basically have a solution for kernels withCONFIG_KPROBESenabled (I need to look up unexported __wake_up_locked[_key]), but there will also be target kernels that do not have this. So there are three options for that case:Quoted 5 lines
- Warn the user and fall back to the old racy approach
- (Somehow) disable KVM subsystems that use eventfd
- Refuse to start KVM
As far as I understood, irqfd is interesting for device assignment and now also for vhost, right?Quoted 6 lines
At the moment, only vhost.
Quoted 1 line
What about ioeventfd?
Same thing.
OK...
Quoted 4 lines
Quoted 3 lines
I just wonder how broad
the impact of a broken or non-existent eventfd subsystem for kvm-kmod is. Any thoughts welcome.
Quoted 7 lines
Quoted 1 line
How do you handle kernels that don't export
eventfd_ctx_fileget?Now that you mention it: not yet properly. So far we pass the file struct as pseudo
eventfd_ctxaround on < 2.6.31. But now that I peek into the struct inkvm_eventfd_ctx_remove_wait_queue, this should should crash. Guess I need to look up that module the same way as I acquire __wake_up_locked[_key].
This won't work that well: eventfd in upstream sends us POLLHUP so we can close the structure, in old kernels it doesn't so kernel will crash when we try to reference the structure later.
Quoted 15 lines
Quoted 9 lines
Quoted 5 lines
Jan
PS: If anyone forgot why Avi handed over this job, you should now remember why. :)
Heh, I did the same kind of thing for infiniband for several years. It's hard to forget.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines
Quoted 4 lines
Quoted 3 lines
I just wonder how broad
the impact of a broken or non-existent eventfd subsystem for kvm-kmod is. Any thoughts welcome.How do you handle kernels that don't export
eventfd_ctx_fileget?Now that you mention it: not yet properly. So far we pass the file struct as pseudo
eventfd_ctxaround on < 2.6.31. But now that I peek into the struct inkvm_eventfd_ctx_remove_wait_queue, this should should crash. Guess I need to look up that module the same way as I acquire __wake_up_locked[_key].
Quoted 5 lines
This won't work that well: eventfd in upstream sends us POLLHUP so we can close the structure, in old kernels it doesn't so kernel will crash when we try to reference the structure later.
OK, so any host kernel < 2.6.31 will never work for us. Mmh, then I could only close the gap 2.6.31..2.6.33. vhost will show up in 33... Will that version already be worth any eventfd wrapping?
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 17 lines · Jan Kiszka
Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines
Quoted 4 lines
Quoted 3 lines
I just wonder how broad
the impact of a broken or non-existent eventfd subsystem for kvm-kmod is. Any thoughts welcome.How do you handle kernels that don't export
eventfd_ctx_fileget?Now that you mention it: not yet properly. So far we pass the file struct as pseudo
eventfd_ctxaround on < 2.6.31. But now that I peek into the struct inkvm_eventfd_ctx_remove_wait_queue, this should should crash. Guess I need to look up that module the same way as I acquire __wake_up_locked[_key].Quoted 5 lines
This won't work that well: eventfd in upstream sends us POLLHUP so we can close the structure, in old kernels it doesn't so kernel will crash when we try to reference the structure later.
Quoted 5 lines
OK, so any host kernel < 2.6.31 will never work for us. Mmh, then I could only close the gap 2.6.31..2.6.33. vhost will show up in 33... Will that version already be worth any eventfd wrapping?
Jan
I asked Avi to send a patch upstream into 2.6.32.X fixing spurious interrupts there. 2.6.31-stable is closed unfortunately, so we won't be able to support it. Disable eventfd there?
Quoted 3 lines
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 7 lines · Jan Kiszka
For kvm-kmod, I'm fighting with compat support for
eventfd_ctx_remove_wait_queue. I basically have a solution for kernels withCONFIG_KPROBESenabled (I need to look up unexported __wake_up_locked[_key]), but there will also be target kernels that do not have this. So there are three options for that case:
Quoted 10 lines
- Warn the user and fall back to the old racy approach
- (Somehow) disable KVM subsystems that use eventfd
- Refuse to start KVMAs far as I understood, irqfd is interesting for device assignment and now also for vhost, right? What about ioeventfd? I just wonder how broad the impact of a broken or non-existent eventfd subsystem for kvm-kmod is. Any thoughts welcome.
Since vhost is only a performance option (and there isn't a vhost-kmod) and device assignment wants a new kernel anyway (and only applies to a small subset of users), I think it's okay to drop it from kvm-kmod. It should be sufficient to return 0 from KVM_CHECK_EXCEPTION and substitute some stubs for the functions.
ioeventfd/irqfd are useful for inter-guest wakeups, but there isn't any public code for that that I'm aware of.
Quoted 3 lines
PS: If anyone forgot why Avi handed over this job, you should now remember why. :)
It's only going to get more difficult, I'm afraid. The list of old kernels keeps growing and we're going to depend on core kernel functionality more and more.
Luckily for me you accepted in time...
Signature · 1 line
error compiling committee.c: too many arguments to function
PATCH 2/2 kvm: do not store wqh in irqfd
In reply to a message not in the archivewqh is unused, so we do not need to store it in irqfd anymore
--- virt/kvm/eventfd.c | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-)
virt/kvm/eventfd.c +0 -3
@@ -47,7 +47,6 @@ struct _irqfd {int gsi;struct list_head list;poll_table pt; -wait_queue_head_t *wqh;wait_queue_t wait;struct work_struct inject;struct work_struct shutdown;
@@ -159,8 +158,6 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh, poll_table *pt) { struct _irqfd *irqfd = container_of(pt, struct _irqfd, pt); - - irqfd->wqh = wqh;add_wait_queue (wqh, &irqfd->wait); }
Signature · 1 line
1.6.6.144.g5c3af