From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Gregory Haskins" Subject: Re: [PATCH 2/3] KVM: Add irqdevice object Date: Mon, 16 Apr 2007 10:42:13 -0400 Message-ID: <462352F1.BA47.005A.0@novell.com> References: <20070416121832.9824.40317.stgit@ghaskins-t60p.haskins.net> <20070416121936.9824.8889.stgit@ghaskins-t60p.haskins.net> <46238338.8070503@qumranet.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: "Avi Kivity" Return-path: In-Reply-To: <46238338.8070503-atKUWr5tajBWk0Htik3J/w@public.gmane.org> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org >>> On Mon, Apr 16, 2007 at 10:07 AM, in message <46238338.8070503-atKUWr5tajBWk0Htik3J/w@public.gmane.org>, Avi Kivity wrote: > Gregory Haskins wrote: >> From: None >> >> The current code is geared towards using a user- mode (A)PIC. This patch adds >> an "irqdevice" abstraction, and implements a "userint" model to handle the >> duties of the original code. Later, we can develop other irqdevice models >> to handle objects like LAPIC, IOAPIC, i8259, etc, as appropriate >> >> > > Viewed in light of 3/3, various races are exposed. > > >> @@ - 2044,13 +2043,11 @@ static int kvm_vcpu_ioctl_set_sregs(struct kvm_vcpu > *vcpu, >> if (mmu_reset_needed) >> kvm_mmu_reset_context(vcpu); >> >> - memcpy(vcpu- >irq_pending, sregs- >interrupt_bitmap, >> - sizeof vcpu- >irq_pending); >> - vcpu- >irq_summary = 0; >> - for (i = 0; i < NR_IRQ_WORDS; ++i) >> - if (vcpu- >irq_pending[i]) >> - __set_bit(i, &vcpu- >irq_summary); >> - >> + /* walk the interrupt- bitmap and inject an IRQ for each bit found */ >> + for (i = 0; i < 256; ++i) >> + if (test_bit(i, &sregs- >interrupt_bitmap[0])) >> + kvm_irqdevice_set_pin(&vcpu- >irq_dev, i, 1); >> + >> > > You need to lower a pin here if it was previously set. Hmm. Good find. It also means my assumption about being able to ignore de-asserts in the userint model is false. I will fix this. > > >> diff -- git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c >> index 61a6116..a0fdf02 100644 >> --- a/drivers/kvm/vmx.c >> +++ b/drivers/kvm/vmx.c >> @@ - 1219,13 +1219,8 @@ static void inject_rmode_irq(struct kvm_vcpu *vcpu, > int irq) >> >> static void kvm_do_inject_irq(struct kvm_vcpu *vcpu) >> { >> - int word_index = __ffs(vcpu- >irq_summary); >> - int bit_index = __ffs(vcpu- >irq_pending[word_index]); >> - int irq = word_index * BITS_PER_LONG + bit_index; >> - >> - clear_bit(bit_index, &vcpu- >irq_pending[word_index]); >> - if (!vcpu- >irq_pending[word_index]) >> - clear_bit(word_index, &vcpu- >irq_summary); >> + int irq = kvm_irqdevice_read_vector(&vcpu- >irq_dev, 0); >> + BUG_ON(irq < 0); >> > > This BUG can trigger. A level- triggered irq was asserted, then deasserted. Ditto for this. > >> >> if (vcpu- >rmode.active) { >> inject_rmode_irq(vcpu, irq); >> @@ - 1246,7 +1241,7 @@ static void do_interrupt_requests(struct kvm_vcpu > *vcpu, >> (vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & 3) == 0); >> >> if (vcpu- >interrupt_window_open && >> - vcpu- >irq_summary && >> + kvm_irqdevice_pending(&vcpu- >irq_dev, 0) && >> !(vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) & INTR_INFO_VALID_MASK)) >> > > What if an irq is made pending here? The only race I see is related to what you pointed out previously: A level-sensitive interrupt could be asserted when pending() is read, and deasserted when read_vector() is read. Handling the irq == -1 from read_vector() should fix the race. Or are you pointing out something else? > or here? possibly a good answer is "don't rely on r_f_i_i if not using > userint". Yeah, exactly. Once the user turns on in-kernel interrupts, some of the kvm-run interface (TBD) will be disabled/ignored due to being redundant/overlapping. Since userspace will have to ack the in-kernel interrupt feature, it will obviously know when to stop using those features as well. ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/