On Thu, Dec 19, 2013 at 05:28:17PM +0100, Alexander Graf wrote: > > On 19.12.2013, at 16:39, bharat.bhus...@freescale.com wrote: > > > > > > >> -----Original Message----- > >> From: Michael S. Tsirkin [mailto:m...@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 <r65...@freescale.com> 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 <bharat.bhus...@freescale.com> > >>>> --- > >>>> 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.
