From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47976) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SWTty-0000bq-Q8 for qemu-devel@nongnu.org; Mon, 21 May 2012 10:48:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SWTtx-00015P-1x for qemu-devel@nongnu.org; Mon, 21 May 2012 10:48:02 -0400 Received: from david.siemens.de ([192.35.17.14]:23681) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SWTtw-00014b-Ox for qemu-devel@nongnu.org; Mon, 21 May 2012 10:48:00 -0400 Message-ID: <4FBA559A.2000707@siemens.com> Date: Mon, 21 May 2012 11:47:54 -0300 From: Jan Kiszka MIME-Version: 1.0 References: <4FBA3F8B.9060103@siemens.com> <4FBA52E9.3060808@redhat.com> In-Reply-To: <4FBA52E9.3060808@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: Alex Williamson , Marcelo Tosatti , qemu-devel , "Michael S. Tsirkin" On 2012-05-21 11:36, Avi Kivity wrote: > On 05/21/2012 04:13 PM, Jan Kiszka wrote: >> Add a PCI IRQ path discovery function that walks from a given device to >> the host bridge, returning the IRQ number that is reported to the >> attached interrupt controller. For this purpose, another PCI bridge >> callback function is introduced: map_host_irq. It is so far only >> implemented by the PIIX3, other host bridges can be added later on as >> required. >> >> Will be used for KVM PCI device assignment. > > This is similar to the memory API, which converts a memory region > hierarchy to a flat list and fires notifiers whenever it changes. In fact, this should become a generic thing one day, independent of PCI. But that's an exercise to be done while reworking the IRQ layer of QEMU (e.g. to support delivery feedback...). > >> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num) >> +{ >> + PCIBus *bus; >> + >> + for (;;) { >> + bus = pci_dev->bus; >> + irq_num = bus->map_irq(pci_dev, irq_num); >> + if (bus->map_host_irq) { >> + break; >> + } >> + pci_dev = bus->parent_dev; >> + assert(pci_dev); >> + } >> + return bus->map_host_irq(bus->irq_opaque, irq_num); >> +} >> + > > My personal preference is to avoid infinite loops with breaks, I'd write > this as a do/while (without the assert). Or maybe supply all buses with > a default map_host_irq that recurses back into > pci_device_get_host_irq(). But this is not an objection to the patch. I've modeled it after pci_change_irq_level. I would suggest to change both later on. BTW, the assert catches host bridges that do not yet implement the required mapping service. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux