From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [UNTESTED] KVM: do not call kvm_set_irq from irq disabled section Date: Wed, 21 Apr 2010 19:28:48 +0300 Message-ID: <4BCF27C0.7070505@redhat.com> References: <20100420155401.GA12982@amt.cnet> <4BCEB831.8050007@redhat.com> <20100421160314.GB22052@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm , bonenkamp@gmx.de, Chris Wright , "Yang, Sheng" To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:54631 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755878Ab0DUQ2w (ORCPT ); Wed, 21 Apr 2010 12:28:52 -0400 In-Reply-To: <20100421160314.GB22052@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 04/21/2010 07:03 PM, Marcelo Tosatti wrote: >>> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c >>> index 47ca447..7ac7bbe 100644 >>> --- a/virt/kvm/assigned-dev.c >>> +++ b/virt/kvm/assigned-dev.c >>> @@ -64,24 +64,33 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) >>> interrupt_work); >>> kvm = assigned_dev->kvm; >>> >>> - spin_lock_irq(&assigned_dev->assigned_dev_lock); >>> if (assigned_dev->irq_requested_type& KVM_DEV_IRQ_HOST_MSIX) { >>> struct kvm_guest_msix_entry *guest_entries = >>> assigned_dev->guest_msix_entries; >>> + >>> + spin_lock_irq(&assigned_dev->assigned_dev_lock); >>> for (i = 0; i< assigned_dev->entries_nr; i++) { >>> if (!(guest_entries[i].flags& >>> KVM_ASSIGNED_MSIX_PENDING)) >>> continue; >>> guest_entries[i].flags&= ~KVM_ASSIGNED_MSIX_PENDING; >>> + /* >>> + * If kvm_assigned_dev_intr sets pending for an >>> + * entry smaller than this work instance is >>> + * currently processing, a new work instance >>> + * will be queued. >>> + */ >>> + spin_unlock_irq(&assigned_dev->assigned_dev_lock); >>> >> What happens if assigned_dev->entries_nr is changed here? >> > It does not. Interrupts and work instances are stopped before > irq_requested_type or entries_nr can change. > Interesting. A comment please. > >>> kvm_set_irq(assigned_dev->kvm, >>> assigned_dev->irq_source_id, >>> guest_entries[i].vector, 1); >>> + spin_lock_irq(&assigned_dev->assigned_dev_lock); >>> } >>> + spin_unlock_irq(&assigned_dev->assigned_dev_lock); >>> } else >>> kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id, >>> assigned_dev->guest_irq, 1); >>> >>> - spin_unlock_irq(&assigned_dev->assigned_dev_lock); >>> } >>> >> A less dangerous fix is to copy all vectors to be triggered into a >> local array, drop the lock, and replay the array into kvm_set_irq(). >> The else branch could do this as well. >> > Its a bit large, 256 max vectors. > Yeah. -- error compiling committee.c: too many arguments to function