Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On 08/22/2012 03:41 AM, Michael S. Tsirkin wrote: I assumed you were pointing out the level vs edge interaction. If we call that a userspace bug, I can just drop this. Thanks, Alex level is userspace bug I think :) I don't see how it's a bug. Suppose we have a vfio device that shares a gsi with an emulated device. The emulated device naturally uses KVM_IRQ_LINE (it has no need to re-sample on ADN), while vfio naturally has to use irqfd. Note one would expect that each irqfd gets its own irq source id, since they are all independent level sources. The reason they don't is that we shut them down anyway and let the sources re-trigger (it is more accurate to say that they have no irq source id, but that would just muddle the implementation). Alex, if the conclusion is that we do need this patch, then please add a comment explaining why we can share the source id among all irqfd users. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On 08/21/2012 10:29 PM, Alex Williamson wrote: KVM_IRQFD currently uses the reserved KVM_USERSPACE_IRQ_SOURCE_ID which is also shared with userspace injection methods like KVM_IRQ_LINE. This can cause a conflict if an irqfd triggers on a GSI asserted through KVM_IRQ_LINE. Move irqfd to it's own reserved IRQ source ID. Add a capability for userspace to test for this fix. I don't think we need a cap, rather a backport if we identify real cases where an edge gsi is shared among several devices. Otherwise it is just a theoretical bug before level irqfd is introduced. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Wed, Sep 05, 2012 at 05:35:43PM +0300, Avi Kivity wrote: On 08/22/2012 03:41 AM, Michael S. Tsirkin wrote: I assumed you were pointing out the level vs edge interaction. If we call that a userspace bug, I can just drop this. Thanks, Alex level is userspace bug I think :) I don't see how it's a bug. Suppose we have a vfio device that shares a gsi with an emulated device. The emulated device naturally uses KVM_IRQ_LINE (it has no need to re-sample on ADN), while vfio naturally has to use irqfd. Absolutely. But vfio needs to use irqfd with the new flag. Using existing irqfd for level is a bug. Note one would expect that each irqfd gets its own irq source id, since they are all independent level sources. The reason they don't is that we shut them down anyway and let the sources re-trigger (it is more accurate to say that they have no irq source id, but that would just muddle the implementation). Alex, if the conclusion is that we do need this patch, then please add a comment explaining why we can share the source id among all irqfd users. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On 09/05/2012 05:51 PM, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:35:43PM +0300, Avi Kivity wrote: On 08/22/2012 03:41 AM, Michael S. Tsirkin wrote: I assumed you were pointing out the level vs edge interaction. If we call that a userspace bug, I can just drop this. Thanks, Alex level is userspace bug I think :) I don't see how it's a bug. Suppose we have a vfio device that shares a gsi with an emulated device. The emulated device naturally uses KVM_IRQ_LINE (it has no need to re-sample on ADN), while vfio naturally has to use irqfd. Absolutely. But vfio needs to use irqfd with the new flag. Using existing irqfd for level is a bug. I see we're not reusing this irq source id for level irqfd. But I think we should, there's no need for per-gsi irq source id. Plus I'd like to fix the theoretical bug even if it doesn't bite in practice. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Wed, Sep 05, 2012 at 05:46:17PM +0300, Avi Kivity wrote: On 08/21/2012 10:29 PM, Alex Williamson wrote: KVM_IRQFD currently uses the reserved KVM_USERSPACE_IRQ_SOURCE_ID which is also shared with userspace injection methods like KVM_IRQ_LINE. This can cause a conflict if an irqfd triggers on a GSI asserted through KVM_IRQ_LINE. Move irqfd to it's own reserved IRQ source ID. Add a capability for userspace to test for this fix. I don't think we need a cap, rather a backport if we identify real cases where an edge gsi is shared among several devices. Otherwise it is just a theoretical bug before level irqfd is introduced. In that case, I think it's safer to preserve the bug as is: we are changing userspace-visible behaviour for edge interrupts otherwise. For example if userspace uses kvm_irq_line for an edge interrupt, set it to 1, previously it could then continue to send any number of interrupts with irqfd, now it can't. Basically the logical OR functionality of source IDs does not make sense for edge. How about we do if (flagsRESAMPLE) source_id = USERSPACE else source_id = IRQFD -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Wed, Sep 05, 2012 at 05:51:53PM +0300, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:35:43PM +0300, Avi Kivity wrote: On 08/22/2012 03:41 AM, Michael S. Tsirkin wrote: I assumed you were pointing out the level vs edge interaction. If we call that a userspace bug, I can just drop this. Thanks, Alex level is userspace bug I think :) I don't see how it's a bug. Suppose we have a vfio device that shares a gsi with an emulated device. The emulated device naturally uses KVM_IRQ_LINE (it has no need to re-sample on ADN), while vfio naturally has to use irqfd. Absolutely. But vfio needs to use irqfd with the new flag. Using existing irqfd for level is a bug. Note one would expect that each irqfd gets its own irq source id, since they are all independent level sources. The reason they don't is that we shut them down anyway and let the sources re-trigger (it is more accurate to say that they have no irq source id, but that would just muddle the implementation). Alex, if the conclusion is that we do need this patch, then please add a comment explaining why we can share the source id among all irqfd users. Something along the lines of /* For resample irqfds, level is a logical OR of all inputs; to support this, track state for RESAMPLE irqfds separately from userspace. We do not need to track state for each input since they are all deasserted at the same time, before resampling. */ ? -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On 09/05/2012 06:09 PM, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:51:53PM +0300, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:35:43PM +0300, Avi Kivity wrote: On 08/22/2012 03:41 AM, Michael S. Tsirkin wrote: I assumed you were pointing out the level vs edge interaction. If we call that a userspace bug, I can just drop this. Thanks, Alex level is userspace bug I think :) I don't see how it's a bug. Suppose we have a vfio device that shares a gsi with an emulated device. The emulated device naturally uses KVM_IRQ_LINE (it has no need to re-sample on ADN), while vfio naturally has to use irqfd. Absolutely. But vfio needs to use irqfd with the new flag. Using existing irqfd for level is a bug. Note one would expect that each irqfd gets its own irq source id, since they are all independent level sources. The reason they don't is that we shut them down anyway and let the sources re-trigger (it is more accurate to say that they have no irq source id, but that would just muddle the implementation). Alex, if the conclusion is that we do need this patch, then please add a comment explaining why we can share the source id among all irqfd users. Something along the lines of /* For resample irqfds, level is a logical OR of all inputs; to support this, track state for RESAMPLE irqfds separately from userspace. We do not need to track state for each input since they are all deasserted at the same time, before resampling. */ Well the comment style is wrong. To expand a little more, irqfd only sends assert events, so assigning the level is equivalent to an OR. Clearing an resampling simply builds the state again. btw, there can be other irq source IDs if the lines are shared with the PIT or kvm assigned devices. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Wed, Sep 05, 2012 at 05:59:46PM +0300, Avi Kivity wrote: On 09/05/2012 05:51 PM, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:35:43PM +0300, Avi Kivity wrote: On 08/22/2012 03:41 AM, Michael S. Tsirkin wrote: I assumed you were pointing out the level vs edge interaction. If we call that a userspace bug, I can just drop this. Thanks, Alex level is userspace bug I think :) I don't see how it's a bug. Suppose we have a vfio device that shares a gsi with an emulated device. The emulated device naturally uses KVM_IRQ_LINE (it has no need to re-sample on ADN), while vfio naturally has to use irqfd. Absolutely. But vfio needs to use irqfd with the new flag. Using existing irqfd for level is a bug. I see we're not reusing this irq source id for level irqfd. But I think we should, there's no need for per-gsi irq source id. I agree. All resample irqfds are deasserted at the same time, tracking them separately gets us nothing. Plus I'd like to fix the theoretical bug even if it doesn't bite in practice. I'm not sure what the bug is, for edge, and how a separate ID fixes it. Could you clarify? -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Wed, Sep 05, 2012 at 06:12:04PM +0300, Avi Kivity wrote: On 09/05/2012 06:09 PM, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:51:53PM +0300, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:35:43PM +0300, Avi Kivity wrote: On 08/22/2012 03:41 AM, Michael S. Tsirkin wrote: I assumed you were pointing out the level vs edge interaction. If we call that a userspace bug, I can just drop this. Thanks, Alex level is userspace bug I think :) I don't see how it's a bug. Suppose we have a vfio device that shares a gsi with an emulated device. The emulated device naturally uses KVM_IRQ_LINE (it has no need to re-sample on ADN), while vfio naturally has to use irqfd. Absolutely. But vfio needs to use irqfd with the new flag. Using existing irqfd for level is a bug. Note one would expect that each irqfd gets its own irq source id, since they are all independent level sources. The reason they don't is that we shut them down anyway and let the sources re-trigger (it is more accurate to say that they have no irq source id, but that would just muddle the implementation). Alex, if the conclusion is that we do need this patch, then please add a comment explaining why we can share the source id among all irqfd users. Something along the lines of /* For resample irqfds, level is a logical OR of all inputs; to support this, track state for RESAMPLE irqfds separately from userspace. We do not need to track state for each input since they are all deasserted at the same time, before resampling. */ Well the comment style is wrong. Ouch. To expand a little more, irqfd only sends assert events, so assigning the level is equivalent to an OR. Clearing an resampling simply builds the state again. btw, there can be other irq source IDs if the lines are shared with the PIT or kvm assigned devices. Nod. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On 09/05/2012 06:07 PM, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:46:17PM +0300, Avi Kivity wrote: On 08/21/2012 10:29 PM, Alex Williamson wrote: KVM_IRQFD currently uses the reserved KVM_USERSPACE_IRQ_SOURCE_ID which is also shared with userspace injection methods like KVM_IRQ_LINE. This can cause a conflict if an irqfd triggers on a GSI asserted through KVM_IRQ_LINE. Move irqfd to it's own reserved IRQ source ID. Add a capability for userspace to test for this fix. I don't think we need a cap, rather a backport if we identify real cases where an edge gsi is shared among several devices. Otherwise it is just a theoretical bug before level irqfd is introduced. In that case, I think it's safer to preserve the bug as is: we are changing userspace-visible behaviour for edge interrupts otherwise. For example if userspace uses kvm_irq_line for an edge interrupt, set it to 1, previously it could then continue to send any number of interrupts with irqfd, now it can't. If anyone did that, they should have reported a bug, since they surely didn't expect edges if the line was held high. Basically the logical OR functionality of source IDs does not make sense for edge. Edge is only interpreted at the ioapic or pic input; the line is just a line (an open collector line that ORs anything connected to it, or an equivalent). How about we do if (flagsRESAMPLE) source_id = USERSPACE else source_id = IRQFD Okay if we identify something that depends on the bug, otherwise not. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On 09/05/2012 06:13 PM, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:59:46PM +0300, Avi Kivity wrote: On 09/05/2012 05:51 PM, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:35:43PM +0300, Avi Kivity wrote: On 08/22/2012 03:41 AM, Michael S. Tsirkin wrote: I assumed you were pointing out the level vs edge interaction. If we call that a userspace bug, I can just drop this. Thanks, Alex level is userspace bug I think :) I don't see how it's a bug. Suppose we have a vfio device that shares a gsi with an emulated device. The emulated device naturally uses KVM_IRQ_LINE (it has no need to re-sample on ADN), while vfio naturally has to use irqfd. Absolutely. But vfio needs to use irqfd with the new flag. Using existing irqfd for level is a bug. I see we're not reusing this irq source id for level irqfd. But I think we should, there's no need for per-gsi irq source id. I agree. All resample irqfds are deasserted at the same time, tracking them separately gets us nothing. That's not the reason. Separate irq source ids only have meanings within a gsi. We could have two lines (gsi 3 isid 4) and (gsi 4 isid 4) that can be toggled independently with no effect on the other gsi. Within a gsi we do need a separate irq source id usually, but as 2/2 recognizes, AODNs are a special case since we clear all inputs anyway. The end result is that all AODNs can share a single isid. Plus I'd like to fix the theoretical bug even if it doesn't bite in practice. I'm not sure what the bug is, for edge, and how a separate ID fixes it. Could you clarify? gsi 3 is configured as edge in the ioapic. It has (unusually) two inputs: one driven by userspace, the other by irqfd. cpu 0cpu 1 - irqfd: set to 1 ioapic: recognize edge inject irq EOI KVM_IRQ_LINE: set to 1 ioapic: ignore KVM_IRQ_LINE: set to 0 irqfd: set to 0 We had two edges with an EOI between them, but injected just on interrupt. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Wed, Sep 05, 2012 at 06:22:57PM +0300, Avi Kivity wrote: On 09/05/2012 06:13 PM, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:59:46PM +0300, Avi Kivity wrote: On 09/05/2012 05:51 PM, Michael S. Tsirkin wrote: On Wed, Sep 05, 2012 at 05:35:43PM +0300, Avi Kivity wrote: On 08/22/2012 03:41 AM, Michael S. Tsirkin wrote: I assumed you were pointing out the level vs edge interaction. If we call that a userspace bug, I can just drop this. Thanks, Alex level is userspace bug I think :) I don't see how it's a bug. Suppose we have a vfio device that shares a gsi with an emulated device. The emulated device naturally uses KVM_IRQ_LINE (it has no need to re-sample on ADN), while vfio naturally has to use irqfd. Absolutely. But vfio needs to use irqfd with the new flag. Using existing irqfd for level is a bug. I see we're not reusing this irq source id for level irqfd. But I think we should, there's no need for per-gsi irq source id. I agree. All resample irqfds are deasserted at the same time, tracking them separately gets us nothing. That's not the reason. Separate irq source ids only have meanings within a gsi. We could have two lines (gsi 3 isid 4) and (gsi 4 isid 4) that can be toggled independently with no effect on the other gsi. Within a gsi we do need a separate irq source id usually, but as 2/2 recognizes, AODNs are a special case since we clear all inputs anyway. The end result is that all AODNs can share a single isid. Plus I'd like to fix the theoretical bug even if it doesn't bite in practice. I'm not sure what the bug is, for edge, and how a separate ID fixes it. Could you clarify? gsi 3 is configured as edge in the ioapic. It has (unusually) two inputs: one driven by userspace, the other by irqfd. cpu 0cpu 1 - irqfd: set to 1 ioapic: recognize edge inject irq EOI KVM_IRQ_LINE: set to 1 ioapic: ignore KVM_IRQ_LINE: set to 0 irqfd: set to 0 We had two edges with an EOI between them, but injected just on interrupt. I see. Makes sense, ACK this patch. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On 09/05/2012 06:28 PM, Michael S. Tsirkin wrote: gsi 3 is configured as edge in the ioapic. It has (unusually) two inputs: one driven by userspace, the other by irqfd. cpu 0cpu 1 - irqfd: set to 1 ioapic: recognize edge inject irq EOI KVM_IRQ_LINE: set to 1 ioapic: ignore KVM_IRQ_LINE: set to 0 irqfd: set to 0 We had two edges with an EOI between them, but injected just on interrupt. I see. Makes sense, ACK this patch. Actually it's wrong. The two sources are not synchronized, so there is no way for them to know the two edges did not coalesce. On real hardware, after all, edge interrupts have a non-zero pulse width, and kvm faithfully emulates this. But this patch makes sense for level irqfd, so we might as well keep it with a different description. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
KVM_IRQFD currently uses the reserved KVM_USERSPACE_IRQ_SOURCE_ID
which is also shared with userspace injection methods like
KVM_IRQ_LINE. This can cause a conflict if an irqfd triggers on
a GSI asserted through KVM_IRQ_LINE. Move irqfd to it's own
reserved IRQ source ID. Add a capability for userspace to test
for this fix.
Signed-off-by: Alex Williamson alex.william...@redhat.com
---
arch/x86/kvm/x86.c |3 +++
include/linux/kvm.h |1 +
include/linux/kvm_host.h |1 +
virt/kvm/eventfd.c |6 +++---
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 42bce48..cd98673 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2174,6 +2174,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_GET_TSC_KHZ:
case KVM_CAP_PCI_2_3:
case KVM_CAP_KVMCLOCK_CTRL:
+ case KVM_CAP_IRQFD_IRQ_SOURCE_ID:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -6258,6 +6259,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
set_bit(KVM_USERSPACE_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
+ /* Reserve bit 1 of irq_sources_bitmap for irqfd irq source */
+ set_bit(KVM_IRQFD_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
raw_spin_lock_init(kvm-arch.tsc_write_lock);
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 2ce09aa..ae66b9c 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -618,6 +618,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_PPC_GET_SMMU_INFO 78
#define KVM_CAP_S390_COW 79
#define KVM_CAP_PPC_ALLOC_HTAB 80
+#define KVM_CAP_IRQFD_IRQ_SOURCE_ID 81
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b70b48b..b763230 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -71,6 +71,7 @@
#define KVM_REQ_PMI 17
#define KVM_USERSPACE_IRQ_SOURCE_ID0
+#define KVM_IRQFD_IRQ_SOURCE_ID1
struct kvm;
struct kvm_vcpu;
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 7d7e2aa..2245cfa 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -67,8 +67,8 @@ irqfd_inject(struct work_struct *work)
struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
struct kvm *kvm = irqfd-kvm;
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 1);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 0);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 1);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 0);
}
/*
@@ -138,7 +138,7 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync,
void *key)
irq = rcu_dereference(irqfd-irq_entry);
/* An event has been signaled, inject an interrupt */
if (irq)
- kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+ kvm_set_msi(irq, kvm, KVM_IRQFD_IRQ_SOURCE_ID, 1);
else
schedule_work(irqfd-inject);
rcu_read_unlock();
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Tue, Aug 21, 2012 at 01:29:06PM -0600, Alex Williamson wrote:
KVM_IRQFD currently uses the reserved KVM_USERSPACE_IRQ_SOURCE_ID
which is also shared with userspace injection methods like
KVM_IRQ_LINE. This can cause a conflict if an irqfd triggers on
a GSI asserted through KVM_IRQ_LINE.
What kind of conflict do you envision? Pls note level interrupts are
unsupported ATM.
Move irqfd to it's own reserved IRQ source ID. Add a capability for
userspace to test for this fix.
Signed-off-by: Alex Williamson alex.william...@redhat.com
---
arch/x86/kvm/x86.c |3 +++
include/linux/kvm.h |1 +
include/linux/kvm_host.h |1 +
virt/kvm/eventfd.c |6 +++---
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 42bce48..cd98673 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2174,6 +2174,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_GET_TSC_KHZ:
case KVM_CAP_PCI_2_3:
case KVM_CAP_KVMCLOCK_CTRL:
+ case KVM_CAP_IRQFD_IRQ_SOURCE_ID:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -6258,6 +6259,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long
type)
/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
set_bit(KVM_USERSPACE_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
+ /* Reserve bit 1 of irq_sources_bitmap for irqfd irq source */
+ set_bit(KVM_IRQFD_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
raw_spin_lock_init(kvm-arch.tsc_write_lock);
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 2ce09aa..ae66b9c 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -618,6 +618,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_PPC_GET_SMMU_INFO 78
#define KVM_CAP_S390_COW 79
#define KVM_CAP_PPC_ALLOC_HTAB 80
+#define KVM_CAP_IRQFD_IRQ_SOURCE_ID 81
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b70b48b..b763230 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -71,6 +71,7 @@
#define KVM_REQ_PMI 17
#define KVM_USERSPACE_IRQ_SOURCE_ID 0
+#define KVM_IRQFD_IRQ_SOURCE_ID 1
struct kvm;
struct kvm_vcpu;
Above looks fine but I'm not sure why is the below needed.
This changes irqfd behaviour for edge GSIs slightly
in a userspace-visible way. Maybe make it a separate patch
so it can be considered on merits?
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 7d7e2aa..2245cfa 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -67,8 +67,8 @@ irqfd_inject(struct work_struct *work)
struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
struct kvm *kvm = irqfd-kvm;
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 1);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 0);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 1);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 0);
}
/*
@@ -138,7 +138,7 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync,
void *key)
irq = rcu_dereference(irqfd-irq_entry);
/* An event has been signaled, inject an interrupt */
if (irq)
- kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+ kvm_set_msi(irq, kvm, KVM_IRQFD_IRQ_SOURCE_ID, 1);
else
schedule_work(irqfd-inject);
rcu_read_unlock();
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Tue, 2012-08-21 at 22:58 +0300, Michael S. Tsirkin wrote:
On Tue, Aug 21, 2012 at 01:29:06PM -0600, Alex Williamson wrote:
KVM_IRQFD currently uses the reserved KVM_USERSPACE_IRQ_SOURCE_ID
which is also shared with userspace injection methods like
KVM_IRQ_LINE. This can cause a conflict if an irqfd triggers on
a GSI asserted through KVM_IRQ_LINE.
What kind of conflict do you envision? Pls note level interrupts are
unsupported ATM.
If KVM_IRQ_LINE asserts a level interrupt and KVM_IRQFD triggers on the
same GSI then the pin is no longer asserted as userspace thinks it is.
Do we just chalk this up to userspace error?
Move irqfd to it's own reserved IRQ source ID. Add a capability for
userspace to test for this fix.
Signed-off-by: Alex Williamson alex.william...@redhat.com
---
arch/x86/kvm/x86.c |3 +++
include/linux/kvm.h |1 +
include/linux/kvm_host.h |1 +
virt/kvm/eventfd.c |6 +++---
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 42bce48..cd98673 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2174,6 +2174,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_GET_TSC_KHZ:
case KVM_CAP_PCI_2_3:
case KVM_CAP_KVMCLOCK_CTRL:
+ case KVM_CAP_IRQFD_IRQ_SOURCE_ID:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -6258,6 +6259,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long
type)
/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
set_bit(KVM_USERSPACE_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
+ /* Reserve bit 1 of irq_sources_bitmap for irqfd irq source */
+ set_bit(KVM_IRQFD_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
raw_spin_lock_init(kvm-arch.tsc_write_lock);
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 2ce09aa..ae66b9c 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -618,6 +618,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_PPC_GET_SMMU_INFO 78
#define KVM_CAP_S390_COW 79
#define KVM_CAP_PPC_ALLOC_HTAB 80
+#define KVM_CAP_IRQFD_IRQ_SOURCE_ID 81
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b70b48b..b763230 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -71,6 +71,7 @@
#define KVM_REQ_PMI 17
#define KVM_USERSPACE_IRQ_SOURCE_ID0
+#define KVM_IRQFD_IRQ_SOURCE_ID1
struct kvm;
struct kvm_vcpu;
Above looks fine but I'm not sure why is the below needed.
This changes irqfd behaviour for edge GSIs slightly
in a userspace-visible way. Maybe make it a separate patch
so it can be considered on merits?
Hmm, the above does nothing without the below. I thought I was just
implementing your idea that IRQFDs should all share a single IRQ source
ID... why is that no longer a good idea? Thanks,
Alex
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 7d7e2aa..2245cfa 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -67,8 +67,8 @@ irqfd_inject(struct work_struct *work)
struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
struct kvm *kvm = irqfd-kvm;
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 1);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 0);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 1);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 0);
}
/*
@@ -138,7 +138,7 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int
sync, void *key)
irq = rcu_dereference(irqfd-irq_entry);
/* An event has been signaled, inject an interrupt */
if (irq)
- kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+ kvm_set_msi(irq, kvm, KVM_IRQFD_IRQ_SOURCE_ID, 1);
else
schedule_work(irqfd-inject);
rcu_read_unlock();
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Tue, Aug 21, 2012 at 02:06:19PM -0600, Alex Williamson wrote:
On Tue, 2012-08-21 at 22:58 +0300, Michael S. Tsirkin wrote:
On Tue, Aug 21, 2012 at 01:29:06PM -0600, Alex Williamson wrote:
KVM_IRQFD currently uses the reserved KVM_USERSPACE_IRQ_SOURCE_ID
which is also shared with userspace injection methods like
KVM_IRQ_LINE. This can cause a conflict if an irqfd triggers on
a GSI asserted through KVM_IRQ_LINE.
What kind of conflict do you envision? Pls note level interrupts are
unsupported ATM.
If KVM_IRQ_LINE asserts a level interrupt and KVM_IRQFD triggers on the
same GSI then the pin is no longer asserted as userspace thinks it is.
Do we just chalk this up to userspace error?
Yes: using a level GSI with current irqfd is a userspace error
because you can lose interrupts anyway.
Are edge GSIs affected?
Move irqfd to it's own reserved IRQ source ID. Add a capability for
userspace to test for this fix.
Signed-off-by: Alex Williamson alex.william...@redhat.com
---
arch/x86/kvm/x86.c |3 +++
include/linux/kvm.h |1 +
include/linux/kvm_host.h |1 +
virt/kvm/eventfd.c |6 +++---
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 42bce48..cd98673 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2174,6 +2174,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_GET_TSC_KHZ:
case KVM_CAP_PCI_2_3:
case KVM_CAP_KVMCLOCK_CTRL:
+ case KVM_CAP_IRQFD_IRQ_SOURCE_ID:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -6258,6 +6259,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long
type)
/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
set_bit(KVM_USERSPACE_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
+ /* Reserve bit 1 of irq_sources_bitmap for irqfd irq source */
+ set_bit(KVM_IRQFD_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
raw_spin_lock_init(kvm-arch.tsc_write_lock);
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 2ce09aa..ae66b9c 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -618,6 +618,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_PPC_GET_SMMU_INFO 78
#define KVM_CAP_S390_COW 79
#define KVM_CAP_PPC_ALLOC_HTAB 80
+#define KVM_CAP_IRQFD_IRQ_SOURCE_ID 81
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b70b48b..b763230 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -71,6 +71,7 @@
#define KVM_REQ_PMI 17
#define KVM_USERSPACE_IRQ_SOURCE_ID 0
+#define KVM_IRQFD_IRQ_SOURCE_ID 1
struct kvm;
struct kvm_vcpu;
Above looks fine but I'm not sure why is the below needed.
This changes irqfd behaviour for edge GSIs slightly
in a userspace-visible way. Maybe make it a separate patch
so it can be considered on merits?
Hmm, the above does nothing without the below.
Yes. But you can use the above with the new irqfds you are adding.
I thought I was just
implementing your idea that IRQFDs should all share a single IRQ source
ID...
Sorry I only meant for level irqfds. You are changing edge here.
why is that no longer a good idea? Thanks,
Alex
Maybe it is a good idea. I am just asking for the motivation.
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 7d7e2aa..2245cfa 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -67,8 +67,8 @@ irqfd_inject(struct work_struct *work)
struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
struct kvm *kvm = irqfd-kvm;
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 1);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 0);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 1);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 0);
}
/*
@@ -138,7 +138,7 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int
sync, void *key)
irq = rcu_dereference(irqfd-irq_entry);
/* An event has been signaled, inject an interrupt */
if (irq)
- kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+ kvm_set_msi(irq, kvm, KVM_IRQFD_IRQ_SOURCE_ID, 1);
else
schedule_work(irqfd-inject);
rcu_read_unlock();
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Tue, 2012-08-21 at 23:41 +0300, Michael S. Tsirkin wrote:
On Tue, Aug 21, 2012 at 02:06:19PM -0600, Alex Williamson wrote:
On Tue, 2012-08-21 at 22:58 +0300, Michael S. Tsirkin wrote:
On Tue, Aug 21, 2012 at 01:29:06PM -0600, Alex Williamson wrote:
KVM_IRQFD currently uses the reserved KVM_USERSPACE_IRQ_SOURCE_ID
which is also shared with userspace injection methods like
KVM_IRQ_LINE. This can cause a conflict if an irqfd triggers on
a GSI asserted through KVM_IRQ_LINE.
What kind of conflict do you envision? Pls note level interrupts are
unsupported ATM.
If KVM_IRQ_LINE asserts a level interrupt and KVM_IRQFD triggers on the
same GSI then the pin is no longer asserted as userspace thinks it is.
Do we just chalk this up to userspace error?
Yes: using a level GSI with current irqfd is a userspace error
because you can lose interrupts anyway.
Are edge GSIs affected?
I wouldn't think so.
Move irqfd to it's own reserved IRQ source ID. Add a capability for
userspace to test for this fix.
Signed-off-by: Alex Williamson alex.william...@redhat.com
---
arch/x86/kvm/x86.c |3 +++
include/linux/kvm.h |1 +
include/linux/kvm_host.h |1 +
virt/kvm/eventfd.c |6 +++---
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 42bce48..cd98673 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2174,6 +2174,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_GET_TSC_KHZ:
case KVM_CAP_PCI_2_3:
case KVM_CAP_KVMCLOCK_CTRL:
+ case KVM_CAP_IRQFD_IRQ_SOURCE_ID:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -6258,6 +6259,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned
long type)
/* Reserve bit 0 of irq_sources_bitmap for userspace irq source
*/
set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,
kvm-arch.irq_sources_bitmap);
+ /* Reserve bit 1 of irq_sources_bitmap for irqfd irq source */
+ set_bit(KVM_IRQFD_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
raw_spin_lock_init(kvm-arch.tsc_write_lock);
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 2ce09aa..ae66b9c 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -618,6 +618,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_PPC_GET_SMMU_INFO 78
#define KVM_CAP_S390_COW 79
#define KVM_CAP_PPC_ALLOC_HTAB 80
+#define KVM_CAP_IRQFD_IRQ_SOURCE_ID 81
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b70b48b..b763230 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -71,6 +71,7 @@
#define KVM_REQ_PMI 17
#define KVM_USERSPACE_IRQ_SOURCE_ID0
+#define KVM_IRQFD_IRQ_SOURCE_ID1
struct kvm;
struct kvm_vcpu;
Above looks fine but I'm not sure why is the below needed.
This changes irqfd behaviour for edge GSIs slightly
in a userspace-visible way. Maybe make it a separate patch
so it can be considered on merits?
Hmm, the above does nothing without the below.
Yes. But you can use the above with the new irqfds you are adding.
Nope, racy.
I thought I was just
implementing your idea that IRQFDs should all share a single IRQ source
ID...
Sorry I only meant for level irqfds. You are changing edge here.
Ok, I misunderstood then.
why is that no longer a good idea? Thanks,
Alex
Maybe it is a good idea. I am just asking for the motivation.
I assumed you were pointing out the level vs edge interaction. If we
call that a userspace bug, I can just drop this. Thanks,
Alex
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 7d7e2aa..2245cfa 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -67,8 +67,8 @@ irqfd_inject(struct work_struct *work)
struct _irqfd *irqfd = container_of(work, struct _irqfd,
inject);
struct kvm *kvm = irqfd-kvm;
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 1);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 0);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 1);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 0);
}
/*
@@ -138,7 +138,7 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int
sync, void *key)
irq = rcu_dereference(irqfd-irq_entry);
/* An event has been signaled, inject an interrupt */
if (irq)
- kvm_set_msi(irq, kvm,
KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+ kvm_set_msi(irq, kvm, KVM_IRQFD_IRQ_SOURCE_ID,
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Tue, Aug 21, 2012 at 03:14:54PM -0600, Alex Williamson wrote:
On Tue, 2012-08-21 at 23:41 +0300, Michael S. Tsirkin wrote:
On Tue, Aug 21, 2012 at 02:06:19PM -0600, Alex Williamson wrote:
On Tue, 2012-08-21 at 22:58 +0300, Michael S. Tsirkin wrote:
On Tue, Aug 21, 2012 at 01:29:06PM -0600, Alex Williamson wrote:
KVM_IRQFD currently uses the reserved KVM_USERSPACE_IRQ_SOURCE_ID
which is also shared with userspace injection methods like
KVM_IRQ_LINE. This can cause a conflict if an irqfd triggers on
a GSI asserted through KVM_IRQ_LINE.
What kind of conflict do you envision? Pls note level interrupts are
unsupported ATM.
If KVM_IRQ_LINE asserts a level interrupt and KVM_IRQFD triggers on the
same GSI then the pin is no longer asserted as userspace thinks it is.
Do we just chalk this up to userspace error?
Yes: using a level GSI with current irqfd is a userspace error
because you can lose interrupts anyway.
Are edge GSIs affected?
I wouldn't think so.
No? If userspace does
. set line to 1
. trigger irqfd
. set line to 1
. trigger irqfd
. set line to 1
. trigger irqfd
. set line to 1
it gets 4 interrupts now
With your patch it will get 1, right?
Move irqfd to it's own reserved IRQ source ID. Add a capability for
userspace to test for this fix.
Signed-off-by: Alex Williamson alex.william...@redhat.com
---
arch/x86/kvm/x86.c |3 +++
include/linux/kvm.h |1 +
include/linux/kvm_host.h |1 +
virt/kvm/eventfd.c |6 +++---
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 42bce48..cd98673 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2174,6 +2174,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_GET_TSC_KHZ:
case KVM_CAP_PCI_2_3:
case KVM_CAP_KVMCLOCK_CTRL:
+ case KVM_CAP_IRQFD_IRQ_SOURCE_ID:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -6258,6 +6259,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned
long type)
/* Reserve bit 0 of irq_sources_bitmap for userspace irq source
*/
set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,
kvm-arch.irq_sources_bitmap);
+ /* Reserve bit 1 of irq_sources_bitmap for irqfd irq source */
+ set_bit(KVM_IRQFD_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
raw_spin_lock_init(kvm-arch.tsc_write_lock);
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 2ce09aa..ae66b9c 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -618,6 +618,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_PPC_GET_SMMU_INFO 78
#define KVM_CAP_S390_COW 79
#define KVM_CAP_PPC_ALLOC_HTAB 80
+#define KVM_CAP_IRQFD_IRQ_SOURCE_ID 81
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b70b48b..b763230 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -71,6 +71,7 @@
#define KVM_REQ_PMI 17
#define KVM_USERSPACE_IRQ_SOURCE_ID 0
+#define KVM_IRQFD_IRQ_SOURCE_ID 1
struct kvm;
struct kvm_vcpu;
Above looks fine but I'm not sure why is the below needed.
This changes irqfd behaviour for edge GSIs slightly
in a userspace-visible way. Maybe make it a separate patch
so it can be considered on merits?
Hmm, the above does nothing without the below.
Yes. But you can use the above with the new irqfds you are adding.
Nope, racy.
I thought I was just
implementing your idea that IRQFDs should all share a single IRQ source
ID...
Sorry I only meant for level irqfds. You are changing edge here.
Ok, I misunderstood then.
why is that no longer a good idea? Thanks,
Alex
Maybe it is a good idea. I am just asking for the motivation.
I assumed you were pointing out the level vs edge interaction. If we
call that a userspace bug, I can just drop this. Thanks,
Alex
level is userspace bug I think :)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 7d7e2aa..2245cfa 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -67,8 +67,8 @@ irqfd_inject(struct work_struct *work)
struct _irqfd *irqfd = container_of(work, struct _irqfd,
inject);
struct kvm *kvm = irqfd-kvm;
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 1);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd-gsi, 0);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 1);
+ kvm_set_irq(kvm, KVM_IRQFD_IRQ_SOURCE_ID, irqfd-gsi, 0);
}
/*
@@ -138,7 +138,7 @@
Re: [PATCH v9 1/2] kvm: Use a reserved IRQ source ID for irqfd
On Wed, 2012-08-22 at 03:41 +0300, Michael S. Tsirkin wrote:
On Tue, Aug 21, 2012 at 03:14:54PM -0600, Alex Williamson wrote:
On Tue, 2012-08-21 at 23:41 +0300, Michael S. Tsirkin wrote:
On Tue, Aug 21, 2012 at 02:06:19PM -0600, Alex Williamson wrote:
On Tue, 2012-08-21 at 22:58 +0300, Michael S. Tsirkin wrote:
On Tue, Aug 21, 2012 at 01:29:06PM -0600, Alex Williamson wrote:
KVM_IRQFD currently uses the reserved KVM_USERSPACE_IRQ_SOURCE_ID
which is also shared with userspace injection methods like
KVM_IRQ_LINE. This can cause a conflict if an irqfd triggers on
a GSI asserted through KVM_IRQ_LINE.
What kind of conflict do you envision? Pls note level interrupts are
unsupported ATM.
If KVM_IRQ_LINE asserts a level interrupt and KVM_IRQFD triggers on the
same GSI then the pin is no longer asserted as userspace thinks it is.
Do we just chalk this up to userspace error?
Yes: using a level GSI with current irqfd is a userspace error
because you can lose interrupts anyway.
Are edge GSIs affected?
I wouldn't think so.
No? If userspace does
. set line to 1
. trigger irqfd
. set line to 1
. trigger irqfd
. set line to 1
. trigger irqfd
. set line to 1
it gets 4 interrupts now
With your patch it will get 1, right?
Move irqfd to it's own reserved IRQ source ID. Add a capability for
userspace to test for this fix.
Signed-off-by: Alex Williamson alex.william...@redhat.com
---
arch/x86/kvm/x86.c |3 +++
include/linux/kvm.h |1 +
include/linux/kvm_host.h |1 +
virt/kvm/eventfd.c |6 +++---
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 42bce48..cd98673 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2174,6 +2174,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_GET_TSC_KHZ:
case KVM_CAP_PCI_2_3:
case KVM_CAP_KVMCLOCK_CTRL:
+ case KVM_CAP_IRQFD_IRQ_SOURCE_ID:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -6258,6 +6259,8 @@ int kvm_arch_init_vm(struct kvm *kvm,
unsigned long type)
/* Reserve bit 0 of irq_sources_bitmap for userspace irq source
*/
set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,
kvm-arch.irq_sources_bitmap);
+ /* Reserve bit 1 of irq_sources_bitmap for irqfd irq source */
+ set_bit(KVM_IRQFD_IRQ_SOURCE_ID, kvm-arch.irq_sources_bitmap);
raw_spin_lock_init(kvm-arch.tsc_write_lock);
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 2ce09aa..ae66b9c 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -618,6 +618,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_PPC_GET_SMMU_INFO 78
#define KVM_CAP_S390_COW 79
#define KVM_CAP_PPC_ALLOC_HTAB 80
+#define KVM_CAP_IRQFD_IRQ_SOURCE_ID 81
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b70b48b..b763230 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -71,6 +71,7 @@
#define KVM_REQ_PMI 17
#define KVM_USERSPACE_IRQ_SOURCE_ID0
+#define KVM_IRQFD_IRQ_SOURCE_ID1
struct kvm;
struct kvm_vcpu;
Above looks fine but I'm not sure why is the below needed.
This changes irqfd behaviour for edge GSIs slightly
in a userspace-visible way. Maybe make it a separate patch
so it can be considered on merits?
Hmm, the above does nothing without the below.
Yes. But you can use the above with the new irqfds you are adding.
Nope, racy.
I thought I was just
implementing your idea that IRQFDs should all share a single IRQ source
ID...
Sorry I only meant for level irqfds. You are changing edge here.
Ok, I misunderstood then.
why is that no longer a good idea? Thanks,
Alex
Maybe it is a good idea. I am just asking for the motivation.
I assumed you were pointing out the level vs edge interaction. If we
call that a userspace bug, I can just drop this. Thanks,
Alex
level is userspace bug I think :)
Dropped. Thanks,
Alex
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
