Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933732Ab2FHID5 (ORCPT ); Fri, 8 Jun 2012 04:03:57 -0400 Received: from david.siemens.de ([192.35.17.14]:15064 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932232Ab2FHIDt (ORCPT ); Fri, 8 Jun 2012 04:03:49 -0400 Message-ID: <4FD1B1DE.9080303@siemens.com> Date: Fri, 08 Jun 2012 10:03:42 +0200 From: Jan Kiszka User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 MIME-Version: 1.0 To: "Michael S. Tsirkin" CC: Thomas Gleixner , Avi Kivity , Alex Williamson , "kvm@vger.kernel.org" , "mtosatti@redhat.com" , "linux-kernel@vger.kernel.org" , "yongjie.ren@intel.com" Subject: Re: [PATCH] KVM: Use IRQF_ONESHOT for assigned device MSI interrupts References: <4FC8F042.5050600@siemens.com> <1338570192.23475.25.camel@bling.home> <4FC8F867.7080103@siemens.com> <1338573558.23475.41.camel@bling.home> <4FC90961.8030701@siemens.com> <4FCB2359.9020505@redhat.com> <4FCC9EAC.9090007@siemens.com> <20120608074744.GA524@redhat.com> <4FD1AFD5.70808@siemens.com> <20120608080058.GB524@redhat.com> In-Reply-To: <20120608080058.GB524@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4246 Lines: 112 On 2012-06-08 10:00, Michael S. Tsirkin wrote: > On Fri, Jun 08, 2012 at 09:55:01AM +0200, Jan Kiszka wrote: >> On 2012-06-08 09:47, Michael S. Tsirkin wrote: >>> On Mon, Jun 04, 2012 at 01:40:28PM +0200, Jan Kiszka wrote: >>>> On 2012-06-04 13:21, Thomas Gleixner wrote: >>>>> On Sun, 3 Jun 2012, Avi Kivity wrote: >>>>> >>>>>> On 06/01/2012 09:26 PM, Jan Kiszka wrote: >>>>>>> >>>>>>>> you suggesting we need a request_edge_threaded_only_irq() API? Thanks, >>>>>>> >>>>>>> I'm just wondering if that restriction for threaded IRQs is really >>>>>>> necessary for all use cases we have. Threaded MSIs do not appear to me >>>>>>> like have to be handled that conservatively, but maybe I'm missing some >>>>>>> detail. >>>>>>> >>>>>> >>>>>> btw, I'm hoping we can unthread assigned MSIs. If the delivery is >>>>>> unicast, we can precalculate everything and all the handler has to do is >>>>>> set the IRR, KVM_REQ_EVENT, and kick the vcpu. All of these can be done >>>>>> from interrupt context with just RCU locking. >>>>> >>>>> There is really no need to run MSI/MSI-X interrupts threaded for >>>>> KVM. I'm running the patch below for quite some time and it works like >>>>> a charm. >>>>> >>>>> Thanks, >>>>> >>>>> tglx >>>>> ---- >>>>> Index: linux-2.6/virt/kvm/assigned-dev.c >>>>> =================================================================== >>>>> --- linux-2.6.orig/virt/kvm/assigned-dev.c >>>>> +++ linux-2.6/virt/kvm/assigned-dev.c >>>>> @@ -105,7 +105,7 @@ static irqreturn_t kvm_assigned_dev_thre >>>>> } >>>>> >>>>> #ifdef __KVM_HAVE_MSI >>>>> -static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id) >>>>> +static irqreturn_t kvm_assigned_dev_msi_handler(int irq, void *dev_id) >>>>> { >>>>> struct kvm_assigned_dev_kernel *assigned_dev = dev_id; >>>>> >>>>> @@ -117,7 +117,7 @@ static irqreturn_t kvm_assigned_dev_thre >>>>> #endif >>>>> >>>>> #ifdef __KVM_HAVE_MSIX >>>>> -static irqreturn_t kvm_assigned_dev_thread_msix(int irq, void *dev_id) >>>>> +static irqreturn_t kvm_assigned_dev_msix_handler(int irq, void *dev_id) >>>>> { >>>>> struct kvm_assigned_dev_kernel *assigned_dev = dev_id; >>>>> int index = find_index_from_host_irq(assigned_dev, irq); >>>>> @@ -346,9 +346,8 @@ static int assigned_device_enable_host_m >>>>> } >>>>> >>>>> dev->host_irq = dev->dev->irq; >>>>> - if (request_threaded_irq(dev->host_irq, NULL, >>>>> - kvm_assigned_dev_thread_msi, 0, >>>>> - dev->irq_name, dev)) { >>>>> + if (request_irq(dev->host_irq, kvm_assigned_dev_msi_handler, 0, >>>>> + dev->irq_name, dev)) { >>>>> pci_disable_msi(dev->dev); >>>>> return -EIO; >>>>> } >>>>> @@ -373,9 +372,9 @@ static int assigned_device_enable_host_m >>>>> return r; >>>>> >>>>> for (i = 0; i < dev->entries_nr; i++) { >>>>> - r = request_threaded_irq(dev->host_msix_entries[i].vector, >>>>> - NULL, kvm_assigned_dev_thread_msix, >>>>> - 0, dev->irq_name, dev); >>>>> + r = request_irq(dev->host_msix_entries[i].vector, >>>>> + kvm_assigned_dev_msix_handler, 0, >>>>> + dev->irq_name, dev); >>>>> if (r) >>>>> goto err; >>>>> } >>>> >>>> This may work in practice but has two conceptual problems: >>>> - we do not want to run a potential broadcast to all VCPUs to run in >>>> a host IRQ handler >>>> - crazy user space could have configured the route to end up in the >>>> PIC or IOAPIC, and both are not hard-IRQ safe (this should probably >>>> be caught on setup) >>>> >>>> So this shortcut requires some checks before being applied to a specific >>>> MSI/MSI-X vector. >>> >>> I did this in the past: >>> https://lkml.org/lkml/2012/1/18/287 >>> >>> Have no hw to test this atm but if there are any takers >>> wanting to play with it I can update and post. >> >> Just add check that allow only unicasts, and this should be fine. >> >> Jan > > If I code it up you can test it? Yep, no problem. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/