Re: [PATCH v2] report IRQ injection status to userspace.
On Mon, Feb 02, 2009 at 04:23:40PM +0200, Avi Kivity wrote:
> Gleb Natapov wrote:
>> On Mon, Feb 02, 2009 at 04:04:55PM +0200, Avi Kivity wrote:
>>
>>> Gleb Natapov wrote:
>>>
> Right, I was thinking about
>
> if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
>
> Should return MASKED if irq is outside the acceptable range?
>
>
Is this ever can be false? Should we BUG() if irq is out of range?
>>> Yes, the number ultimately comes from userspace.
>>>
>>>
>> So may be -EINVAL should be returned to userspace?
>>
>
> Mmm, not sure. An out-of-bounds number here could be caused by
> userspace generating the wrong irq line number, or by the guest
> misconfiguring interrupts.
>
> We should error out on userspace bugs, but not guest bugs.
I don't see this applied yet ?
--
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 v2] report IRQ injection status to userspace.
Gleb Natapov wrote:
On Mon, Feb 02, 2009 at 04:04:55PM +0200, Avi Kivity wrote:
Gleb Natapov wrote:
Right, I was thinking about
if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
Should return MASKED if irq is outside the acceptable range?
Is this ever can be false? Should we BUG() if irq is out of range?
Yes, the number ultimately comes from userspace.
So may be -EINVAL should be returned to userspace?
Mmm, not sure. An out-of-bounds number here could be caused by
userspace generating the wrong irq line number, or by the guest
misconfiguring interrupts.
We should error out on userspace bugs, but not guest bugs.
--
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 v2] report IRQ injection status to userspace.
On Mon, Feb 02, 2009 at 04:04:55PM +0200, Avi Kivity wrote:
> Gleb Natapov wrote:
>>> Right, I was thinking about
>>>
>>> if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
>>>
>>> Should return MASKED if irq is outside the acceptable range?
>>>
>>>
>> Is this ever can be false? Should we BUG() if irq is out of range?
>>
>>
>
> Yes, the number ultimately comes from userspace.
>
So may be -EINVAL should be returned to userspace?
--
Gleb.
--
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 v2] report IRQ injection status to userspace.
Gleb Natapov wrote:
Right, I was thinking about
if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
Should return MASKED if irq is outside the acceptable range?
Is this ever can be false? Should we BUG() if irq is out of range?
Yes, the number ultimately comes from userspace.
--
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 v2] report IRQ injection status to userspace.
On Wed, Jan 28, 2009 at 06:37:36PM +0200, Gleb Natapov wrote:
> On Tue, Jan 27, 2009 at 01:41:07PM -0200, Marcelo Tosatti wrote:
> > On Tue, Jan 27, 2009 at 03:27:39PM +0200, Gleb Natapov wrote:
> > > > -1 here ?
> > > >
> > > I think 1 is better here. For level=0 we always want to report that
> > > interrupt
> > > was injected and for the case of edge triggered interrupt and level=1
> > > ioapic_service() will always be called. BTW it seems that expression
> > > old_irr != ioapic->irr in:
> > > if ((!entry.fields.trig_mode && old_irr != ioapic->irr)
> > > || !entry.fields.remote_irr)
> > > ret = ioapic_service(ioapic, irq);
> > > Will always be true since for edge triggered interrupt irr is always
> > > cleared by ioapic_service(). Am I right?
> >
> > Right, I was thinking about
> >
> > if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> >
> > Should return MASKED if irq is outside the acceptable range?
> >
> Is this ever can be false? Should we BUG() if irq is out of range?
If qemu-kvm passes it ouf range IRQ yes. Its just a nitpicking, ignore
it.
> > That assumes guests won't mask the interrupt temporarily in the irqchip,
> > hope that is OK (as Avi noted earlier guests use CPU to mask irqs
> > temporarily, most of the time).
> And if a guest masks interrupts it can't complain that some are lost. I
> haven't seen Windows masking RTC irq.
Makes sense.
--
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 v2] report IRQ injection status to userspace.
On Tue, Jan 27, 2009 at 01:41:07PM -0200, Marcelo Tosatti wrote:
> On Tue, Jan 27, 2009 at 03:27:39PM +0200, Gleb Natapov wrote:
> > > -1 here ?
> > >
> > I think 1 is better here. For level=0 we always want to report that
> > interrupt
> > was injected and for the case of edge triggered interrupt and level=1
> > ioapic_service() will always be called. BTW it seems that expression
> > old_irr != ioapic->irr in:
> > if ((!entry.fields.trig_mode && old_irr != ioapic->irr)
> > || !entry.fields.remote_irr)
> > ret = ioapic_service(ioapic, irq);
> > Will always be true since for edge triggered interrupt irr is always
> > cleared by ioapic_service(). Am I right?
>
> Right, I was thinking about
>
> if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
>
> Should return MASKED if irq is outside the acceptable range?
>
Is this ever can be false? Should we BUG() if irq is out of range?
> > > +}
> > > +}
> > > #endif
> > >
> > > Is that what you intended ?
> > >
> > Yes! If interrupt was lost due to making it should not be reinjected.
>
> That assumes guests won't mask the interrupt temporarily in the irqchip,
> hope that is OK (as Avi noted earlier guests use CPU to mask irqs
> temporarily, most of the time).
And if a guest masks interrupts it can't complain that some are lost. I
haven't seen Windows masking RTC irq.
--
Gleb.
--
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 v2] report IRQ injection status to userspace.
On Tue, Jan 27, 2009 at 03:27:39PM +0200, Gleb Natapov wrote:
> > -1 here ?
> >
> I think 1 is better here. For level=0 we always want to report that interrupt
> was injected and for the case of edge triggered interrupt and level=1
> ioapic_service() will always be called. BTW it seems that expression
> old_irr != ioapic->irr in:
> if ((!entry.fields.trig_mode && old_irr != ioapic->irr)
> || !entry.fields.remote_irr)
> ret = ioapic_service(ioapic, irq);
> Will always be true since for edge triggered interrupt irr is always
> cleared by ioapic_service(). Am I right?
Right, I was thinking about
if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
Should return MASKED if irq is outside the acceptable range?
> > +}
> > +}
> > #endif
> >
> > Is that what you intended ?
> >
> Yes! If interrupt was lost due to making it should not be reinjected.
That assumes guests won't mask the interrupt temporarily in the irqchip,
hope that is OK (as Avi noted earlier guests use CPU to mask irqs
temporarily, most of the time).
--
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 v2] report IRQ injection status to userspace.
On Mon, Jan 26, 2009 at 02:10:38PM -0200, Marcelo Tosatti wrote:
> Hi Gleb,
>
> On Wed, Jan 21, 2009 at 02:34:28PM +0200, Gleb Natapov wrote:
> > Use this one instead. Adds capabilities checks.
> >
> > Signed-off-by: Gleb Natapov
>
> > index 179dcb0..2752016 100644
> > --- a/arch/x86/kvm/i8259.c
> > +++ b/arch/x86/kvm/i8259.c
> > @@ -76,12 +76,13 @@ void kvm_pic_clear_isr_ack(struct kvm *kvm)
> > /*
> > * set irq level. If an edge is detected, then the IRR is set to 1
> > */
> > -static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int
> > level)
> > +static inline int pic_set_irq1(struct kvm_kpic_state *s, int irq, int
> > level)
> > {
> > - int mask;
> > + int mask, ret = 1;
> > mask = 1 << irq;
> > if (s->elcr & mask) /* level triggered */
> > if (level) {
> > + ret = !(s->irr & mask);
> > s->irr |= mask;
> > s->last_irr |= mask;
> > } else {
> > @@ -90,11 +91,15 @@ static inline void pic_set_irq1(struct kvm_kpic_state
> > *s, int irq, int level)
> > }
> > else/* edge triggered */
> > if (level) {
> > - if ((s->last_irr & mask) == 0)
> > + if ((s->last_irr & mask) == 0) {
> > + ret = !(s->irr & mask);
> > s->irr |= mask;
> > + }
> > s->last_irr |= mask;
> > } else
> > s->last_irr &= ~mask;
> > +
> > + return (s->imr & mask) ? -1 : ret;
> > }
>
> Can you add some documentation, perhaps through definitions, like:
>
> #define KVM_IRQ_LINE_STATUS_MASKED -1
> #define KVM_IRQ_LINE_STATUS_FAIL 0
> #define KVM_IRQ_LINE_STATUS_INJECTED 1
>
> But with better names. This makes the kernel code more explicit too.
>
To find a good name is the hard problem ;) I'll resend.
> > -void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > {
> > u32 old_irr = ioapic->irr;
> > u32 mask = 1 << irq;
> > union ioapic_redir_entry entry;
> > + int ret = 1;
>
> -1 here ?
>
I think 1 is better here. For level=0 we always want to report that interrupt
was injected and for the case of edge triggered interrupt and level=1
ioapic_service() will always be called. BTW it seems that expression
old_irr != ioapic->irr in:
if ((!entry.fields.trig_mode && old_irr != ioapic->irr)
|| !entry.fields.remote_irr)
ret = ioapic_service(ioapic, irq);
Will always be true since for edge triggered interrupt irr is always
cleared by ioapic_service(). Am I right?
> And then, in the userspace part, you consider -1 as "injected":
>
> diff --git a/qemu/hw/i8259.c b/qemu/hw/i8259.c
> index 6d41a5e..9da4360 100644
> --- a/qemu/hw/i8259.c
> +++ b/qemu/hw/i8259.c
> @@ -186,9 +186,14 @@ static void i8259_set_irq(void *opaque, int irq,
> int level)
> {
> PicState2 *s = opaque;
> #ifdef KVM_CAP_IRQCHIP
> -if (kvm_enabled())
> - if (kvm_set_irq(irq, level))
> - return;
> +if (kvm_enabled()) {
> +int pic_ret;
> +if (kvm_set_irq(irq, level, &pic_ret)) {
> +if (pic_ret != 0)
> +apic_set_irq_delivered();
> +return;
> +}
> +}
> #endif
>
> Is that what you intended ?
>
Yes! If interrupt was lost due to making it should not be reinjected.
--
Gleb.
--
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 v2] report IRQ injection status to userspace.
Hi Gleb,
On Wed, Jan 21, 2009 at 02:34:28PM +0200, Gleb Natapov wrote:
> Use this one instead. Adds capabilities checks.
>
> Signed-off-by: Gleb Natapov
> index 179dcb0..2752016 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -76,12 +76,13 @@ void kvm_pic_clear_isr_ack(struct kvm *kvm)
> /*
> * set irq level. If an edge is detected, then the IRR is set to 1
> */
> -static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
> +static inline int pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
> {
> - int mask;
> + int mask, ret = 1;
> mask = 1 << irq;
> if (s->elcr & mask) /* level triggered */
> if (level) {
> + ret = !(s->irr & mask);
> s->irr |= mask;
> s->last_irr |= mask;
> } else {
> @@ -90,11 +91,15 @@ static inline void pic_set_irq1(struct kvm_kpic_state *s,
> int irq, int level)
> }
> else/* edge triggered */
> if (level) {
> - if ((s->last_irr & mask) == 0)
> + if ((s->last_irr & mask) == 0) {
> + ret = !(s->irr & mask);
> s->irr |= mask;
> + }
> s->last_irr |= mask;
> } else
> s->last_irr &= ~mask;
> +
> + return (s->imr & mask) ? -1 : ret;
> }
Can you add some documentation, perhaps through definitions, like:
#define KVM_IRQ_LINE_STATUS_MASKED -1
#define KVM_IRQ_LINE_STATUS_FAIL 0
#define KVM_IRQ_LINE_STATUS_INJECTED 1
But with better names. This makes the kernel code more explicit too.
> -void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> {
> u32 old_irr = ioapic->irr;
> u32 mask = 1 << irq;
> union ioapic_redir_entry entry;
> + int ret = 1;
-1 here ?
And then, in the userspace part, you consider -1 as "injected":
diff --git a/qemu/hw/i8259.c b/qemu/hw/i8259.c
index 6d41a5e..9da4360 100644
--- a/qemu/hw/i8259.c
+++ b/qemu/hw/i8259.c
@@ -186,9 +186,14 @@ static void i8259_set_irq(void *opaque, int irq,
int level)
{
PicState2 *s = opaque;
#ifdef KVM_CAP_IRQCHIP
-if (kvm_enabled())
- if (kvm_set_irq(irq, level))
- return;
+if (kvm_enabled()) {
+int pic_ret;
+if (kvm_set_irq(irq, level, &pic_ret)) {
+if (pic_ret != 0)
+apic_set_irq_delivered();
+return;
+}
+}
#endif
Is that what you intended ?
Other than that looks fine to me.
--
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 v2] report IRQ injection status to userspace.
Use this one instead. Adds capabilities checks.
Signed-off-by: Gleb Natapov
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index dbf527a..d83d3ff 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -182,7 +182,7 @@ int kvm_dev_ioctl_check_extension(long ext)
switch (ext) {
case KVM_CAP_IRQCHIP:
case KVM_CAP_MP_STATE:
-
+ case KVM_CAP_IRQ_INJECT_STATUS:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -925,6 +925,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
goto out;
}
break;
+ case KVM_IRQ_LINE_STATUS:
case KVM_IRQ_LINE: {
struct kvm_irq_level irq_event;
@@ -932,10 +933,17 @@ long kvm_arch_vm_ioctl(struct file *filp,
if (copy_from_user(&irq_event, argp, sizeof irq_event))
goto out;
if (irqchip_in_kernel(kvm)) {
+ __s32 status;
mutex_lock(&kvm->lock);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
+ status = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
irq_event.irq, irq_event.level);
mutex_unlock(&kvm->lock);
+ if (ioctl == KVM_IRQ_LINE_STATUS) {
+ irq_event.status = status;
+ if (copy_to_user(argp, &irq_event,
+ sizeof irq_event))
+ goto out;
+ }
r = 0;
}
break;
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 55fd4c5..f0faf58 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -616,7 +616,7 @@ void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned
nr, u32 error_code);
void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long cr2,
u32 error_code);
-void kvm_pic_set_irq(void *opaque, int irq, int level);
+int kvm_pic_set_irq(void *opaque, int irq, int level);
void kvm_inject_nmi(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 179dcb0..2752016 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -76,12 +76,13 @@ void kvm_pic_clear_isr_ack(struct kvm *kvm)
/*
* set irq level. If an edge is detected, then the IRR is set to 1
*/
-static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
+static inline int pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
{
- int mask;
+ int mask, ret = 1;
mask = 1 << irq;
if (s->elcr & mask) /* level triggered */
if (level) {
+ ret = !(s->irr & mask);
s->irr |= mask;
s->last_irr |= mask;
} else {
@@ -90,11 +91,15 @@ static inline void pic_set_irq1(struct kvm_kpic_state *s,
int irq, int level)
}
else/* edge triggered */
if (level) {
- if ((s->last_irr & mask) == 0)
+ if ((s->last_irr & mask) == 0) {
+ ret = !(s->irr & mask);
s->irr |= mask;
+ }
s->last_irr |= mask;
} else
s->last_irr &= ~mask;
+
+ return (s->imr & mask) ? -1 : ret;
}
/*
@@ -171,16 +176,19 @@ void kvm_pic_update_irq(struct kvm_pic *s)
pic_unlock(s);
}
-void kvm_pic_set_irq(void *opaque, int irq, int level)
+int kvm_pic_set_irq(void *opaque, int irq, int level)
{
struct kvm_pic *s = opaque;
+ int ret = -1;
pic_lock(s);
if (irq >= 0 && irq < PIC_NUM_PINS) {
- pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
+ ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
pic_update_irq(s);
}
pic_unlock(s);
+
+ return ret;
}
/*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e96edda..5754c37 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -995,6 +995,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_MP_STATE:
case KVM_CAP_SYNC_MMU:
case KVM_CAP_REINJECT_CONTROL:
+ case KVM_CAP_IRQ_INJECT_STATUS:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -1852,6 +1853,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
create_pit_unlock:
mutex_unlock(&kvm->lock);
break;
+ case KVM_IRQ_LINE_STATUS:
case KVM_IRQ_LINE: {
struct kvm_irq_level irq_event;
@@ -1859,10 +1861,17 @@ long kvm_arch_vm_ioctl(struct file *filp,
if (copy_
