Re: [PATCH 1/2] KVM: Handle multiple interrupt sources

Re: [PATCH 1/2] KVM: Handle multiple interrupt sources

[Amit Shah]

----- "Sheng Yang" <sheng@linux.intel.com> wrote:

> On Saturday 11 October 2008 16:10:51 Amit Shah wrote:
> > From: Sheng Yang <sheng@linux.intel.com>
> >
> > Keep a record of current interrupt state before injecting. Don't
> > assert/deassert repeatedly, so that every caller of kvm_set_irq()
> > can be identified as a separate interrupt source for the IOAPIC/PIC
> > to implement logical OR of level triggered interrupts on one IRQ
> line.
> >
> > Notice that userspace devices are treated as one device for each
> IRQ
> > line. The correctness of sharing interrupt for each IRQ line should
> be
> > ensured by the userspace program (QEmu).
> >
> > [Amit: rebase to kvm.git HEAD]
> 
> Hi, Amit
> 
> Thanks for your work! 
> 
> But maybe I miss something. I suppose my later patch can work
> indepently? I 
> think the second patch should solve the whole problem (sorry to reply
> it to 
> the second rather than [0/2] which made confusion...). Can you have a
> check?

I'm not sure I understand. Which concern are you talking about?

I used the latest patch that you sent and I also verified that it works.

Amit

Re: [PATCH 1/2] KVM: Handle multiple interrupt sources

[Sheng Yang]

On Monday 13 October 2008 13:06:18 Amit Shah wrote:
> ----- "Sheng Yang" <sheng@linux.intel.com> wrote:
> > On Saturday 11 October 2008 16:10:51 Amit Shah wrote:
> > > From: Sheng Yang <sheng@linux.intel.com>
> > >
> > > Keep a record of current interrupt state before injecting. Don't
> > > assert/deassert repeatedly, so that every caller of kvm_set_irq()
> > > can be identified as a separate interrupt source for the IOAPIC/PIC
> > > to implement logical OR of level triggered interrupts on one IRQ
> >
> > line.
> >
> > > Notice that userspace devices are treated as one device for each
> >
> > IRQ
> >
> > > line. The correctness of sharing interrupt for each IRQ line should
> >
> > be
> >
> > > ensured by the userspace program (QEmu).
> > >
> > > [Amit: rebase to kvm.git HEAD]
> >
> > Hi, Amit
> >
> > Thanks for your work!
> >
> > But maybe I miss something. I suppose my later patch can work
> > indepently? I
> > think the second patch should solve the whole problem (sorry to reply
> > it to
> > the second rather than [0/2] which made confusion...). Can you have a
> > check?
>
> I'm not sure I understand. Which concern are you talking about?
>
> I used the latest patch that you sent and I also verified that it works.
>
Well, at least I meant to replace all of my first two patches with my later 
one... I suppose the second patch(my later one, derived from Avi's 
suggestion) should work alone without the first one...
--
regards
Yang, Sheng

Re: [PATCH 1/2] KVM: Handle multiple interrupt sources

[Amit Shah]

----- "Sheng Yang" <sheng@linux.intel.com> wrote:
> Well, at least I meant to replace all of my first two patches with my
> later 
> one... I suppose the second patch(my later one, derived from Avi's 
> suggestion) should work alone without the first one...

OK; I just saw that. I'll rebase and test again.

Support sharing of guest IRQ levels for assigned devices

[Amit Shah]

I've tested Sheng's latest patches and they work fine. This series
is essentially the same, rebased on top of HEAD after the move of
the device assignment code to virt/kvm/kvm_main.c from arch/x86/kvm/x86.c

Also contains a small fix to request the irq source only when we register
the interrupt handler.

Please apply for 2.6.28.

Amit.

[PATCH 1/2] KVM: Handle multiple interrupt sources

[Amit Shah]

From: Sheng Yang <sheng@linux.intel.com>

Keep a record of current interrupt state before injecting. Don't
assert/deassert repeatedly, so that every caller of kvm_set_irq()
can be identified as a separate interrupt source for the IOAPIC/PIC
to implement logical OR of level triggered interrupts on one IRQ line.

Notice that userspace devices are treated as one device for each IRQ
line. The correctness of sharing interrupt for each IRQ line should be
ensured by the userspace program (QEmu).

[Amit: rebase to kvm.git HEAD]

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 arch/x86/kvm/x86.c       |   13 ++++++++++++-
 include/linux/kvm_host.h |    3 +++
 virt/kvm/kvm_main.c      |   12 +++++++++---
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index dda478e..6f45428 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1816,7 +1816,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		if (irqchip_in_kernel(kvm)) {
 			mutex_lock(&kvm->lock);
-			kvm_set_irq(kvm, irq_event.irq, irq_event.level);
+			/*
+			 * Take one IRQ line as from one device, shared IRQ
+			 * line should also be handled in the userspace before
+			 * use KVM_IRQ_LINE ioctl to change IRQ line state.
+			 */
+			if (kvm->userspace_intrsource_states[irq_event.irq]
+					!= irq_event.level) {
+				kvm_set_irq(kvm, irq_event.irq,
+					    irq_event.level);
+				kvm->userspace_intrsource_states[irq_event.irq]
+						= irq_event.level;
+			}
 			mutex_unlock(&kvm->lock);
 			r = 0;
 		}
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 3833c48..d392e31 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -129,6 +129,8 @@ struct kvm {
 	unsigned long mmu_notifier_seq;
 	long mmu_notifier_count;
 #endif
+
+	int userspace_intrsource_states[KVM_IOAPIC_NUM_PINS];
 };
 
 /* The guest did something we don't support. */
@@ -306,6 +308,7 @@ struct kvm_assigned_dev_kernel {
 	int host_irq;
 	int guest_irq;
 	int irq_requested;
+	int irq_state;
 	struct pci_dev *dev;
 	struct kvm *kvm;
 };
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cf0ab8e..faa56fb 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -104,8 +104,11 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 	 * finer-grained lock, update this
 	 */
 	mutex_lock(&assigned_dev->kvm->lock);
-	kvm_set_irq(assigned_dev->kvm,
-		    assigned_dev->guest_irq, 1);
+	if (assigned_dev->irq_state == 0) {
+		kvm_set_irq(assigned_dev->kvm,
+			    assigned_dev->guest_irq, 1);
+		assigned_dev->irq_state = 1;
+	}
 	mutex_unlock(&assigned_dev->kvm->lock);
 	kvm_put_kvm(assigned_dev->kvm);
 }
@@ -134,7 +137,10 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 
 	dev = container_of(kian, struct kvm_assigned_dev_kernel,
 			   ack_notifier);
-	kvm_set_irq(dev->kvm, dev->guest_irq, 0);
+	if (dev->irq_state == 1) {
+		kvm_set_irq(dev->kvm, dev->guest_irq, 0);
+		dev->irq_state = 0;
+	}
 	enable_irq(dev->host_irq);
 }
 
-- 
1.5.4.3

Re: [PATCH 1/2] KVM: Handle multiple interrupt sources

[Sheng Yang]

On Saturday 11 October 2008 16:10:51 Amit Shah wrote:
> From: Sheng Yang <sheng@linux.intel.com>
>
> Keep a record of current interrupt state before injecting. Don't
> assert/deassert repeatedly, so that every caller of kvm_set_irq()
> can be identified as a separate interrupt source for the IOAPIC/PIC
> to implement logical OR of level triggered interrupts on one IRQ line.
>
> Notice that userspace devices are treated as one device for each IRQ
> line. The correctness of sharing interrupt for each IRQ line should be
> ensured by the userspace program (QEmu).
>
> [Amit: rebase to kvm.git HEAD]

Hi, Amit

Thanks for your work! 

But maybe I miss something. I suppose my later patch can work indepently? I 
think the second patch should solve the whole problem (sorry to reply it to 
the second rather than [0/2] which made confusion...). Can you have a check?

Thanks!
--
regards
Yang, Sheng
>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  arch/x86/kvm/x86.c       |   13 ++++++++++++-
>  include/linux/kvm_host.h |    3 +++
>  virt/kvm/kvm_main.c      |   12 +++++++++---
>  3 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index dda478e..6f45428 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1816,7 +1816,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  			goto out;
>  		if (irqchip_in_kernel(kvm)) {
>  			mutex_lock(&kvm->lock);
> -			kvm_set_irq(kvm, irq_event.irq, irq_event.level);
> +			/*
> +			 * Take one IRQ line as from one device, shared IRQ
> +			 * line should also be handled in the userspace before
> +			 * use KVM_IRQ_LINE ioctl to change IRQ line state.
> +			 */
> +			if (kvm->userspace_intrsource_states[irq_event.irq]
> +					!= irq_event.level) {
> +				kvm_set_irq(kvm, irq_event.irq,
> +					    irq_event.level);
> +				kvm->userspace_intrsource_states[irq_event.irq]
> +						= irq_event.level;
> +			}
>  			mutex_unlock(&kvm->lock);
>  			r = 0;
>  		}
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 3833c48..d392e31 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -129,6 +129,8 @@ struct kvm {
>  	unsigned long mmu_notifier_seq;
>  	long mmu_notifier_count;
>  #endif
> +
> +	int userspace_intrsource_states[KVM_IOAPIC_NUM_PINS];
>  };
>
>  /* The guest did something we don't support. */
> @@ -306,6 +308,7 @@ struct kvm_assigned_dev_kernel {
>  	int host_irq;
>  	int guest_irq;
>  	int irq_requested;
> +	int irq_state;
>  	struct pci_dev *dev;
>  	struct kvm *kvm;
>  };
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index cf0ab8e..faa56fb 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -104,8 +104,11 @@ static void
> kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) *
> finer-grained lock, update this
>  	 */
>  	mutex_lock(&assigned_dev->kvm->lock);
> -	kvm_set_irq(assigned_dev->kvm,
> -		    assigned_dev->guest_irq, 1);
> +	if (assigned_dev->irq_state == 0) {
> +		kvm_set_irq(assigned_dev->kvm,
> +			    assigned_dev->guest_irq, 1);
> +		assigned_dev->irq_state = 1;
> +	}
>  	mutex_unlock(&assigned_dev->kvm->lock);
>  	kvm_put_kvm(assigned_dev->kvm);
>  }
> @@ -134,7 +137,10 @@ static void kvm_assigned_dev_ack_irq(struct
> kvm_irq_ack_notifier *kian)
>
>  	dev = container_of(kian, struct kvm_assigned_dev_kernel,
>  			   ack_notifier);
> -	kvm_set_irq(dev->kvm, dev->guest_irq, 0);
> +	if (dev->irq_state == 1) {
> +		kvm_set_irq(dev->kvm, dev->guest_irq, 0);
> +		dev->irq_state = 0;
> +	}
>  	enable_irq(dev->host_irq);
>  }