From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices Date: Tue, 02 Nov 2010 13:41:17 -0600 Message-ID: <1288726877.3045.127.camel@x201> References: <70406157f1f29ade425369f82310a3963f0d0e97.1288712958.git.jan.kiszka@siemens.com> <20101102174149.GC1304@redhat.com> <4CD050BE.5010703@siemens.com> <20101102182401.GB1939@redhat.com> <4CD05B2E.1050005@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: "Michael S. Tsirkin" , Avi Kivity , Marcelo Tosatti , kvm To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:32118 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754914Ab0KBTlW (ORCPT ); Tue, 2 Nov 2010 15:41:22 -0400 In-Reply-To: <4CD05B2E.1050005@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, 2010-11-02 at 19:40 +0100, Jan Kiszka wrote: > Am 02.11.2010 19:24, Michael S. Tsirkin wrote: > > On Tue, Nov 02, 2010 at 06:56:14PM +0100, Jan Kiszka wrote: > >>>> dev->host_irq_disabled = false; > >>>> } > >>>> - spin_unlock(&dev->intx_lock); > >>>> +out: > >>>> + spin_unlock_irq(&dev->intx_lock); > >>>> + > >>>> + if (reassert) > >>>> + kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1); > >>> > >>> Hmm, I think this still has more overhead than it needs to have. > >>> Instead of setting level to 0 and then back to 1, can't we just > >>> avoid set to 1 in the first place? This would need a different > >>> interface than pci_2_3_irq_check_and_unmask to avoid a race > >>> where interrupt is received while we are acking another one: > >>> > >>> block userspace access > >>> check pending bit > >>> if (!pending) > >>> set irq (0) > >>> clear pending > >>> block userspace access > >>> > >>> Would be worth it for high volume devices. > >> > >> The problem is that we can't reorder guest IRQ line clearing and host > >> IRQ line enabling without taking a lock across host IRQ disable + guest > >> IRQ raise - and that is now distributed across hard and threaded IRQ > >> handlers and we don't want to hold and IRQ-safe lock during kvm_set_irq. > > > > Oh I think I confused you. > > What I mean is: > > > > block userspace access > > check interrupt status bit > > if (!interrupt status bit set) > > set irq (0) > > clear interrupt disable bit > > block userspace access > > > > This way we enable interrupt after set irq so not need for > > extra locks I think. > > OK. Would require some serious refactoring again. > > But what about edge IRQs? Don't we need to toggle the bit for them? And > as we do not differentiate between level and edge, we currently have to > do this unconditionally. > > > > > Hmm one thing I noticed is that pci_block_user_cfg_access > > will BUG_ON if it was already blocked. So I think we have > > a bug here when interrupt handler kicks in right after > > we unmask interrupts. > > > > Probably need some kind of lock to protect against this. > > > > Or an atomic counter. Will have a look. > > Alex, does VFIO take care of this already? Yes, VFIO has a lock used by the interrupt handler and the EOI handler that prevents them from both blocking user cfg access at the same time. Alex