From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38829) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtsA2-0002Nv-1F for qemu-devel@nongnu.org; Thu, 19 Dec 2013 23:58:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vts9x-0007lS-3w for qemu-devel@nongnu.org; Thu, 19 Dec 2013 23:58:05 -0500 Date: Fri, 20 Dec 2013 07:01:49 +0200 From: "Michael S. Tsirkin" Message-ID: <20131220050149.GA26160@redhat.com> References: <1385620533-17685-1-git-send-email-Bharat.Bhushan@freescale.com> <1385620533-17685-3-git-send-email-Bharat.Bhushan@freescale.com> <971CB822-467A-4FC4-BA43-3F89A2E096B2@suse.de> <20131218222447.GA27208@redhat.com> <1126AA7C-B95C-4C37-B249-EA1BE1DB8007@suse.de> <20131219183223.GA15126@redhat.com> <31f53f99601b45d7b02267a293bbd2f4@BN1PR03MB266.namprd03.prod.outlook.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <31f53f99601b45d7b02267a293bbd2f4@BN1PR03MB266.namprd03.prod.outlook.com> Subject: Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Bharat.Bhushan@freescale.com" Cc: Scott Wood , qemu-ppc , Alexander Graf , QEMU Developers On Fri, Dec 20, 2013 at 04:15:09AM +0000, Bharat.Bhushan@freescale.com wrote: > > > > -----Original Message----- > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > Sent: Friday, December 20, 2013 12:02 AM > > To: Alexander Graf > > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers; qemu-ppc > > Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing > > > > On Thu, Dec 19, 2013 at 05:28:17PM +0100, Alexander Graf wrote: > > > > > > On 19.12.2013, at 16:39, Bharat.Bhushan@freescale.com wrote: > > > > > > > > > > > > > > >> -----Original Message----- > > > >> From: Michael S. Tsirkin [mailto:mst@redhat.com] > > > >> Sent: Thursday, December 19, 2013 3:55 AM > > > >> To: Alexander Graf > > > >> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers; > > > >> qemu-ppc; Bhushan > > > >> Bharat-R65777 > > > >> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing > > > >> > > > >> On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote: > > > >>> > > > >>> On 28.11.2013, at 07:35, Bharat Bhushan wrote: > > > >>> > > > >>>> This patch adds pci pin to irq_num routing callback Without this > > > >>>> patch we gets below warning > > > >>>> > > > >>>> " > > > >>>> PCI: Bug - unimplemented PCI INTx routing (e500-pcihost) > > > >>>> qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing > > > >>>> (e500-pcihost) " > > > >>>> > > > >>>> Signed-off-by: Bharat Bhushan > > > >>>> --- > > > >>>> hw/pci-host/ppce500.c | 20 ++++++++++++++++++-- > > > >>>> 1 files changed, 18 insertions(+), 2 deletions(-) > > > >>>> > > > >>>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index > > > >>>> 49bfcc6..3c4cf9e 100644 > > > >>>> --- a/hw/pci-host/ppce500.c > > > >>>> +++ b/hw/pci-host/ppce500.c > > > >>>> @@ -88,6 +88,7 @@ struct PPCE500PCIState { > > > >>>> struct pci_inbound pib[PPCE500_PCI_NR_PIBS]; > > > >>>> uint32_t gasket_time; > > > >>>> qemu_irq irq[PCI_NUM_PINS]; > > > >>>> + uint32_t irq_num[PCI_NUM_PINS]; > > > >>>> uint32_t first_slot; > > > >>>> /* mmio maps */ > > > >>>> MemoryRegion container; > > > >>>> @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice > > > >>>> *pci_dev, int pin) > > > >>>> > > > >>>> static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) { > > > >>>> - qemu_irq *pic = opaque; > > > >>>> + PPCE500PCIState *s = opaque; > > > >>>> + qemu_irq *pic = s->irq;; > > > >>> > > > >>> Double semicolon? > > > > > > > > Ok, will correct. > > > > > > > >>> > > > >>>> > > > >>>> pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , > > > >>>> level); > > > >>>> > > > >>>> qemu_set_irq(pic[pin], level); } > > > >>>> > > > >>>> +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int > > > >>>> +pin) { > > > >>>> + PCIINTxRoute route; > > > >>>> + PPCE500PCIState *s = opaque; > > > >>>> + > > > >>>> + route.mode = PCI_INTX_ENABLED; > > > >>>> + route.irq = s->irq_num[pin]; > > > >>>> + > > > >>>> + pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, > > > >>>> + pin, > > > >> route.irq); > > > >>>> + return route; > > > >>>> +} > > > >>>> + > > > >>>> static const VMStateDescription vmstate_pci_outbound = { > > > >>>> .name = "pci_outbound", > > > >>>> .version_id = 0, > > > >>>> @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice > > > >>>> *dev) > > > >>>> > > > >>>> for (i = 0; i < ARRAY_SIZE(s->irq); i++) { > > > >>>> sysbus_init_irq(dev, &s->irq[i]); > > > >>>> + s->irq_num[i] = i + 1; > > > >>> > > > >>> Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()? > > > >>> I don't > > > >> understand the purpose of this whole exercise to be honest. > > > >>> > > > >>> Michael, could you please shed some light on this? > > > >>> > > > >>> > > > >>> Alex > > > >> > > > >> This is printed by pci_device_route_intx_to_irq - it's used by > > > >> device assignment and vfio to figure out which irq does a given pci device > > drive. > > > > > > > > Yes, exactly same reason. > > > > > > Is there any way we could get rid of the information duplication? The fact > > that INTA/B/C/D are mapped to 1,2,3,4 is really a configuration parameter that > > should only live at a single spot. > > > > > > > > > Alex > > > > Yes. In fact I had the idea to only have something like > > pci_device_route_intx_to_irq and call it once for all interrupts and cache that, > > then use this to send interrupts directly to apic. > > Redo this each time routing changes. > > I had a patch like this (and I think Jan had one too), but Anthony said he'll > > rewrite all interrupt routing using QOM so I dropped it. I'll try to resurrect > > it. > > So do we want to have this patch almost in this shape and hope Anthony's changes will handle this well or wait for Anthony patches first ? > > Thanks > -Bharat I think your patch is the right thing to do ATM. > >