From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH v2] report IRQ injection status to userspace.
Date: Mon, 26 Jan 2009 14:10:38 -0200
Message-ID: <20090126161038.GB3894@amt.cnet>
References: <20090121113227.GH27675@redhat.com> <20090121123428.GI27675@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Avi Kivity <avi@redhat.com>, kvm@vger.kernel.org
To: Gleb Natapov <gleb@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:40698 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751394AbZAZQLI (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 26 Jan 2009 11:11:08 -0500
Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26])
	by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n0QGB6lQ029379
	for <kvm@vger.kernel.org>; Mon, 26 Jan 2009 11:11:06 -0500
Content-Disposition: inline
In-Reply-To: <20090121123428.GI27675@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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 <gleb@redhat.com>

> 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.